Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On 23/01/2025 12:42, Dmitry Baryshkov wrote: > On Thu, Jan 23, 2025 at 12:34:28PM +0100, Krzysztof Kozlowski wrote: >> On 13/01/2025 13:13, Dmitry Baryshkov wrote: >>> On Mon, Jan 13, 2025 at 12:02:54PM +0100, Krzysztof Kozlowski wrote: On 13/01/2025 09:29, Dmitry Baryshkov wrote: > On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2025 10:17, Dmitry Baryshkov wrote: >>> On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: On 10/01/2025 00:18, Dmitry Baryshkov wrote: > On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: >> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with >> two >> differences worth noting: >> >> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - >> their >>offsets were just switched. Currently these registers are not >> used >>in the driver, so the easiest is to document both but keep them >>commented out to avoid conflict. >> >> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be >> used as >>parents before they are prepared and initial rate is set. >> Therefore >>assigned-clock-parents are not working here and driver is >> responsible >>for reparenting clocks with proper procedure: see >> dsi_clk_init_6g_v2_9(). > > Isn't it a description of CLK_SET_PARENT_GATE and/or No - must be gated accross reparent - so opposite. > CLK_OPS_PARENT_ENABLE ? Yes, but does not work. Probably enabling parent, before assigned-clocks-parents, happens still too early: [1.623554] DSI PLL(0) lock failed, status=0x [1.623556] PLL(0) lock failed [1.624650] [ cut here ] [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its configuration. Or maybe something is missing in the DSI PHY PLL driver? >>> >>> Do you have the no-zero-freq workaround? >> >> Yes, it is necessary also for my variant. I did not include it here, but >> I should mention it in the cover letter. > > Could you please possibly backtrace the corresponding enable() calls? It's the same backtrace I shared some time ago in internal discussions: https://pastebin.com/kxUFgzD9 Unless you ask for some other backtrace? > I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that > such requirement should be handled by the framework instead of having > the drivers to manually reparent the clocks. I don't know how exactly you would like to solve it. The clocks can be reparented only after some other device specific enable sequence. It's the third device here, but not reflected in the clocks hierarchy. Maybe it's the result how entire Display device nodes were designed in the first place? Assigned clocks are between DSI PHY and DISP cc, but they are a property of DSI controller. This looks exactly too specific for core to handle and drivers, not framework, should manually reparent such clocks. Otherwise we need "clk_pre_prepare_callback_if_we_are_called_when_phy_is_disabled" sort of callback. >>> >>> What kind of PHY programming is required? Is enabling the PLL enough or >>> does it need anything else? Are the PLL supplies properly enabled at >>> this point? >>> >> >> I don't know exactly and checking is tricky. I tried to use >> CLK_OPS_PARENT_ENABLE - with equivalent code, setting proper parents but >> without enabling the DSI PHY PLL manually just with >> CLK_OPS_PARENT_ENABLE - but then you have multiple: >> >> dsi0_pll_bit_clk: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set > > This really looks as if a part of the DSI PHY is unpowered. If you are > sure about your DSI and DSI PHY supplies (and power domains) then I also > have no other ideas. Yes, I triple checked them with downstream and with schematics. The rate setting - which is being discussed/fixed here - is in: msm_dsi_host_power_on() which does: regulator_bulk_enable() pm_runtime_get_sync(&msm_host->pdev->dev); ret = cfg_hnd->ops->link_clk_set_rate(msm_host); If I refactor this code into: regulator_bulk_enable() pm_runtime_get_sync(&msm_host->pdev->dev); MY_NEW_PREPARE_REPARENT_UNPREPARE() ret = cfg_hnd->ops->link_clk_set_rate(msm_host); and use CLK_OPS_PARENT_ENABLE, I still got mentioned errors with calltrace: dev_pm_opp_set_rate (drivers/opp/core.c:1357) line numbers won't help you, so explaining: this is exactly in dsi_link_clk_set_rate_6g dsi_link_clk_set_rate_6g (drivers/gpu/drm/msm/dsi/dsi_host.c:391) msm_dsi_host_power_on (drivers/gpu/drm/msm/dsi/dsi_host.
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On Thu, Jan 23, 2025 at 12:34:28PM +0100, Krzysztof Kozlowski wrote: > On 13/01/2025 13:13, Dmitry Baryshkov wrote: > > On Mon, Jan 13, 2025 at 12:02:54PM +0100, Krzysztof Kozlowski wrote: > >> On 13/01/2025 09:29, Dmitry Baryshkov wrote: > >>> On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: > On 10/01/2025 10:17, Dmitry Baryshkov wrote: > > On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: > >> On 10/01/2025 00:18, Dmitry Baryshkov wrote: > >>> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: > Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with > two > differences worth noting: > > 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - > their > offsets were just switched. Currently these registers are not > used > in the driver, so the easiest is to document both but keep them > commented out to avoid conflict. > > 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be > used as > parents before they are prepared and initial rate is set. > Therefore > assigned-clock-parents are not working here and driver is > responsible > for reparenting clocks with proper procedure: see > dsi_clk_init_6g_v2_9(). > >>> > >>> Isn't it a description of CLK_SET_PARENT_GATE and/or > >> > >> No - must be gated accross reparent - so opposite. > >> > >>> CLK_OPS_PARENT_ENABLE ? > >> > >> Yes, but does not work. Probably enabling parent, before > >> assigned-clocks-parents, happens still too early: > >> > >> [1.623554] DSI PLL(0) lock failed, status=0x > >> [1.623556] PLL(0) lock failed > >> [1.624650] [ cut here ] > >> [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its > >> configuration. > >> > >> Or maybe something is missing in the DSI PHY PLL driver? > > > > Do you have the no-zero-freq workaround? > > Yes, it is necessary also for my variant. I did not include it here, but > I should mention it in the cover letter. > >>> > >>> Could you please possibly backtrace the corresponding enable() calls? > >> > >> > >> It's the same backtrace I shared some time ago in internal discussions: > >> https://pastebin.com/kxUFgzD9 > >> Unless you ask for some other backtrace? > >> > >>> I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that > >>> such requirement should be handled by the framework instead of having > >>> the drivers to manually reparent the clocks. > >> > >> I don't know how exactly you would like to solve it. The clocks can be > >> reparented only after some other device specific enable sequence. It's > >> the third device here, but not reflected in the clocks hierarchy. Maybe > >> it's the result how entire Display device nodes were designed in the > >> first place? > >> > >> Assigned clocks are between DSI PHY and DISP cc, but they are a property > >> of DSI controller. This looks exactly too specific for core to handle > >> and drivers, not framework, should manually reparent such clocks. > >> Otherwise we need > >> "clk_pre_prepare_callback_if_we_are_called_when_phy_is_disabled" sort of > >> callback. > > > > What kind of PHY programming is required? Is enabling the PLL enough or > > does it need anything else? Are the PLL supplies properly enabled at > > this point? > > > > I don't know exactly and checking is tricky. I tried to use > CLK_OPS_PARENT_ENABLE - with equivalent code, setting proper parents but > without enabling the DSI PHY PLL manually just with > CLK_OPS_PARENT_ENABLE - but then you have multiple: > > dsi0_pll_bit_clk: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set This really looks as if a part of the DSI PHY is unpowered. If you are sure about your DSI and DSI PHY supplies (and power domains) then I also have no other ideas. Abhinav? Any input from your side? Or from Taniya Das? > > So how do you supposed to test it? Any assigned-clocks-xxx will be way > too early. Moving code around? Well, if I move preparing the DSI PLL > clocks out of dsi_link_clk_set_rate_6g, then dsi_link_clk_set_rate_6g() > will fail. Always and CLK_OPS_PARENT_ENABLE does not help because of above. > > If you have specific code in mind, I can try it, but I don't see easy > methods to see what has to be enabled exactly because of how everything > is entangled together. > > Best regards, > Krzysztof -- With best wishes Dmitry
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On 13/01/2025 13:13, Dmitry Baryshkov wrote: > On Mon, Jan 13, 2025 at 12:02:54PM +0100, Krzysztof Kozlowski wrote: >> On 13/01/2025 09:29, Dmitry Baryshkov wrote: >>> On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: On 10/01/2025 10:17, Dmitry Baryshkov wrote: > On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2025 00:18, Dmitry Baryshkov wrote: >>> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two differences worth noting: 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their offsets were just switched. Currently these registers are not used in the driver, so the easiest is to document both but keep them commented out to avoid conflict. 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as parents before they are prepared and initial rate is set. Therefore assigned-clock-parents are not working here and driver is responsible for reparenting clocks with proper procedure: see dsi_clk_init_6g_v2_9(). >>> >>> Isn't it a description of CLK_SET_PARENT_GATE and/or >> >> No - must be gated accross reparent - so opposite. >> >>> CLK_OPS_PARENT_ENABLE ? >> >> Yes, but does not work. Probably enabling parent, before >> assigned-clocks-parents, happens still too early: >> >> [1.623554] DSI PLL(0) lock failed, status=0x >> [1.623556] PLL(0) lock failed >> [1.624650] [ cut here ] >> [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its >> configuration. >> >> Or maybe something is missing in the DSI PHY PLL driver? > > Do you have the no-zero-freq workaround? Yes, it is necessary also for my variant. I did not include it here, but I should mention it in the cover letter. >>> >>> Could you please possibly backtrace the corresponding enable() calls? >> >> >> It's the same backtrace I shared some time ago in internal discussions: >> https://pastebin.com/kxUFgzD9 >> Unless you ask for some other backtrace? >> >>> I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that >>> such requirement should be handled by the framework instead of having >>> the drivers to manually reparent the clocks. >> >> I don't know how exactly you would like to solve it. The clocks can be >> reparented only after some other device specific enable sequence. It's >> the third device here, but not reflected in the clocks hierarchy. Maybe >> it's the result how entire Display device nodes were designed in the >> first place? >> >> Assigned clocks are between DSI PHY and DISP cc, but they are a property >> of DSI controller. This looks exactly too specific for core to handle >> and drivers, not framework, should manually reparent such clocks. >> Otherwise we need >> "clk_pre_prepare_callback_if_we_are_called_when_phy_is_disabled" sort of >> callback. > > What kind of PHY programming is required? Is enabling the PLL enough or > does it need anything else? Are the PLL supplies properly enabled at > this point? > I don't know exactly and checking is tricky. I tried to use CLK_OPS_PARENT_ENABLE - with equivalent code, setting proper parents but without enabling the DSI PHY PLL manually just with CLK_OPS_PARENT_ENABLE - but then you have multiple: dsi0_pll_bit_clk: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set So how do you supposed to test it? Any assigned-clocks-xxx will be way too early. Moving code around? Well, if I move preparing the DSI PLL clocks out of dsi_link_clk_set_rate_6g, then dsi_link_clk_set_rate_6g() will fail. Always and CLK_OPS_PARENT_ENABLE does not help because of above. If you have specific code in mind, I can try it, but I don't see easy methods to see what has to be enabled exactly because of how everything is entangled together. Best regards, Krzysztof
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On Mon, Jan 13, 2025 at 12:02:54PM +0100, Krzysztof Kozlowski wrote: > On 13/01/2025 09:29, Dmitry Baryshkov wrote: > > On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: > >> On 10/01/2025 10:17, Dmitry Baryshkov wrote: > >>> On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: > On 10/01/2025 00:18, Dmitry Baryshkov wrote: > > On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: > >> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with > >> two > >> differences worth noting: > >> > >> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their > >>offsets were just switched. Currently these registers are not used > >>in the driver, so the easiest is to document both but keep them > >>commented out to avoid conflict. > >> > >> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used > >> as > >>parents before they are prepared and initial rate is set. Therefore > >>assigned-clock-parents are not working here and driver is > >> responsible > >>for reparenting clocks with proper procedure: see > >> dsi_clk_init_6g_v2_9(). > > > > Isn't it a description of CLK_SET_PARENT_GATE and/or > > No - must be gated accross reparent - so opposite. > > > CLK_OPS_PARENT_ENABLE ? > > Yes, but does not work. Probably enabling parent, before > assigned-clocks-parents, happens still too early: > > [1.623554] DSI PLL(0) lock failed, status=0x > [1.623556] PLL(0) lock failed > [1.624650] [ cut here ] > [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its > configuration. > > Or maybe something is missing in the DSI PHY PLL driver? > >>> > >>> Do you have the no-zero-freq workaround? > >> > >> Yes, it is necessary also for my variant. I did not include it here, but > >> I should mention it in the cover letter. > > > > Could you please possibly backtrace the corresponding enable() calls? > > > It's the same backtrace I shared some time ago in internal discussions: > https://pastebin.com/kxUFgzD9 > Unless you ask for some other backtrace? > > > I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that > > such requirement should be handled by the framework instead of having > > the drivers to manually reparent the clocks. > > I don't know how exactly you would like to solve it. The clocks can be > reparented only after some other device specific enable sequence. It's > the third device here, but not reflected in the clocks hierarchy. Maybe > it's the result how entire Display device nodes were designed in the > first place? > > Assigned clocks are between DSI PHY and DISP cc, but they are a property > of DSI controller. This looks exactly too specific for core to handle > and drivers, not framework, should manually reparent such clocks. > Otherwise we need > "clk_pre_prepare_callback_if_we_are_called_when_phy_is_disabled" sort of > callback. What kind of PHY programming is required? Is enabling the PLL enough or does it need anything else? Are the PLL supplies properly enabled at this point? -- With best wishes Dmitry
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On 13/01/2025 09:29, Dmitry Baryshkov wrote: > On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2025 10:17, Dmitry Baryshkov wrote: >>> On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: On 10/01/2025 00:18, Dmitry Baryshkov wrote: > On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: >> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two >> differences worth noting: >> >> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their >>offsets were just switched. Currently these registers are not used >>in the driver, so the easiest is to document both but keep them >>commented out to avoid conflict. >> >> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as >>parents before they are prepared and initial rate is set. Therefore >>assigned-clock-parents are not working here and driver is responsible >>for reparenting clocks with proper procedure: see >> dsi_clk_init_6g_v2_9(). > > Isn't it a description of CLK_SET_PARENT_GATE and/or No - must be gated accross reparent - so opposite. > CLK_OPS_PARENT_ENABLE ? Yes, but does not work. Probably enabling parent, before assigned-clocks-parents, happens still too early: [1.623554] DSI PLL(0) lock failed, status=0x [1.623556] PLL(0) lock failed [1.624650] [ cut here ] [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its configuration. Or maybe something is missing in the DSI PHY PLL driver? >>> >>> Do you have the no-zero-freq workaround? >> >> Yes, it is necessary also for my variant. I did not include it here, but >> I should mention it in the cover letter. > > Could you please possibly backtrace the corresponding enable() calls? It's the same backtrace I shared some time ago in internal discussions: https://pastebin.com/kxUFgzD9 Unless you ask for some other backtrace? > I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that > such requirement should be handled by the framework instead of having > the drivers to manually reparent the clocks. I don't know how exactly you would like to solve it. The clocks can be reparented only after some other device specific enable sequence. It's the third device here, but not reflected in the clocks hierarchy. Maybe it's the result how entire Display device nodes were designed in the first place? Assigned clocks are between DSI PHY and DISP cc, but they are a property of DSI controller. This looks exactly too specific for core to handle and drivers, not framework, should manually reparent such clocks. Otherwise we need "clk_pre_prepare_callback_if_we_are_called_when_phy_is_disabled" sort of callback. Best regards, Krzysztof
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On Fri, Jan 10, 2025 at 01:43:28PM +0100, Krzysztof Kozlowski wrote: > On 10/01/2025 10:17, Dmitry Baryshkov wrote: > > On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: > >> On 10/01/2025 00:18, Dmitry Baryshkov wrote: > >>> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: > Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two > differences worth noting: > > 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their > offsets were just switched. Currently these registers are not used > in the driver, so the easiest is to document both but keep them > commented out to avoid conflict. > > 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as > parents before they are prepared and initial rate is set. Therefore > assigned-clock-parents are not working here and driver is responsible > for reparenting clocks with proper procedure: see > dsi_clk_init_6g_v2_9(). > >>> > >>> Isn't it a description of CLK_SET_PARENT_GATE and/or > >> > >> No - must be gated accross reparent - so opposite. > >> > >>> CLK_OPS_PARENT_ENABLE ? > >> > >> Yes, but does not work. Probably enabling parent, before > >> assigned-clocks-parents, happens still too early: > >> > >> [1.623554] DSI PLL(0) lock failed, status=0x > >> [1.623556] PLL(0) lock failed > >> [1.624650] [ cut here ] > >> [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its > >> configuration. > >> > >> Or maybe something is missing in the DSI PHY PLL driver? > > > > Do you have the no-zero-freq workaround? > > Yes, it is necessary also for my variant. I did not include it here, but > I should mention it in the cover letter. Could you please possibly backtrace the corresponding enable() calls? I'd let Stephen and/or Bjorn or Konrad to correct me, but I think that such requirement should be handled by the framework instead of having the drivers to manually reparent the clocks. -- With best wishes Dmitry
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On 10/01/2025 10:17, Dmitry Baryshkov wrote: > On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2025 00:18, Dmitry Baryshkov wrote: >>> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote: Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two differences worth noting: 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their offsets were just switched. Currently these registers are not used in the driver, so the easiest is to document both but keep them commented out to avoid conflict. 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as parents before they are prepared and initial rate is set. Therefore assigned-clock-parents are not working here and driver is responsible for reparenting clocks with proper procedure: see dsi_clk_init_6g_v2_9(). >>> >>> Isn't it a description of CLK_SET_PARENT_GATE and/or >> >> No - must be gated accross reparent - so opposite. >> >>> CLK_OPS_PARENT_ENABLE ? >> >> Yes, but does not work. Probably enabling parent, before >> assigned-clocks-parents, happens still too early: >> >> [1.623554] DSI PLL(0) lock failed, status=0x >> [1.623556] PLL(0) lock failed >> [1.624650] [ cut here ] >> [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its >> configuration. >> >> Or maybe something is missing in the DSI PHY PLL driver? > > Do you have the no-zero-freq workaround? Yes, it is necessary also for my variant. I did not include it here, but I should mention it in the cover letter. Best regards, Krzysztof
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On Fri, Jan 10, 2025 at 09:59:26AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2025 00:18, Dmitry Baryshkov wrote:
> > On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote:
> >> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two
> >> differences worth noting:
> >>
> >> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their
> >>offsets were just switched. Currently these registers are not used
> >>in the driver, so the easiest is to document both but keep them
> >>commented out to avoid conflict.
> >>
> >> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as
> >>parents before they are prepared and initial rate is set. Therefore
> >>assigned-clock-parents are not working here and driver is responsible
> >>for reparenting clocks with proper procedure: see
> >> dsi_clk_init_6g_v2_9().
> >
> > Isn't it a description of CLK_SET_PARENT_GATE and/or
>
> No - must be gated accross reparent - so opposite.
>
> > CLK_OPS_PARENT_ENABLE ?
>
> Yes, but does not work. Probably enabling parent, before
> assigned-clocks-parents, happens still too early:
>
> [1.623554] DSI PLL(0) lock failed, status=0x
> [1.623556] PLL(0) lock failed
> [1.624650] [ cut here ]
> [1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its
> configuration.
>
> Or maybe something is missing in the DSI PHY PLL driver?
Do you have the no-zero-freq workaround?
>
> >
> >>
> >> Signed-off-by: Krzysztof Kozlowski
> >> ---
> >> drivers/gpu/drm/msm/dsi/dsi.h | 2 +
> >> drivers/gpu/drm/msm/dsi/dsi_cfg.c | 25 +++
> >> drivers/gpu/drm/msm/dsi/dsi_cfg.h | 1 +
> >> drivers/gpu/drm/msm/dsi/dsi_host.c | 80
> >> ++
> >> drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +
> >> drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 +
> >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 78
> >> +++--
> >> .../gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 14
> >
> > Please separate DSI and PHY changes.
> >
> >> 8 files changed, 197 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> >> b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> >> index
> >> 7754dcec33d06e3d6eb8a9d55e53f24af073adb9..e2a8d6fcc45b6c207a3018ea7c8744fcf34dabd2
> >> 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> >> @@ -205,6 +205,17 @@ static const struct msm_dsi_config sm8650_dsi_cfg = {
> >>},
> >> };
> >>
> >> +static const struct msm_dsi_config sm8750_dsi_cfg = {
> >> + .io_offset = DSI_6G_REG_SHIFT,
> >> + .regulator_data = sm8650_dsi_regulators,
> >> + .num_regulators = ARRAY_SIZE(sm8650_dsi_regulators),
> >> + .bus_clk_names = dsi_v2_4_clk_names,
> >> + .num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
> >> + .io_start = {
> >> + { 0xae94000, 0xae96000 },
> >> + },
> >> +};
> >> +
> >> static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
> >>{ .supply = "vdda", .init_load_uA = 8350 }, /* 1.2 V */
> >>{ .supply = "refgen" },
> >> @@ -257,6 +268,18 @@ static const struct msm_dsi_host_cfg_ops
> >> msm_dsi_6g_v2_host_ops = {
> >>.calc_clk_rate = dsi_calc_clk_rate_6g,
> >> };
> >>
> >> +static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_9_host_ops = {
> >> + .link_clk_set_rate = dsi_link_clk_set_rate_6g_v2_9,
> >> + .link_clk_enable = dsi_link_clk_enable_6g,
> >> + .link_clk_disable = dsi_link_clk_disable_6g,
> >> + .clk_init_ver = dsi_clk_init_6g_v2_9,
> >> + .tx_buf_alloc = dsi_tx_buf_alloc_6g,
> >> + .tx_buf_get = dsi_tx_buf_get_6g,
> >> + .tx_buf_put = dsi_tx_buf_put_6g,
> >> + .dma_base_get = dsi_dma_base_get_6g,
> >> + .calc_clk_rate = dsi_calc_clk_rate_6g,
> >> +};
> >> +
> >> static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
> >>{MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064,
> >>&apq8064_dsi_cfg, &msm_dsi_v2_host_ops},
> >> @@ -300,6 +323,8 @@ static const struct msm_dsi_cfg_handler
> >> dsi_cfg_handlers[] = {
> >>&sm8550_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> >>{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_8_0,
> >>&sm8650_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> >> + {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_9_0,
> >> + &sm8750_dsi_cfg, &msm_dsi_6g_v2_9_host_ops},
> >> };
> >>
> >> const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> >> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> >> index
> >> 120cb65164c1ba1deb9acb513e5f073bd560c496..859c279afbb0377d16f8406f3e6b083640aff5a1
> >> 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> >> @@ -30,6 +30,7 @@
> >> #define MSM_DSI_6G_VER_MINOR_V2_6_0 0x2006
> >> #define MSM_DSI_6G_VER_MINOR_V2_
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On 10/01/2025 00:18, Dmitry Baryshkov wrote:
> On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote:
>> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two
>> differences worth noting:
>>
>> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their
>>offsets were just switched. Currently these registers are not used
>>in the driver, so the easiest is to document both but keep them
>>commented out to avoid conflict.
>>
>> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as
>>parents before they are prepared and initial rate is set. Therefore
>>assigned-clock-parents are not working here and driver is responsible
>>for reparenting clocks with proper procedure: see dsi_clk_init_6g_v2_9().
>
> Isn't it a description of CLK_SET_PARENT_GATE and/or
No - must be gated accross reparent - so opposite.
> CLK_OPS_PARENT_ENABLE ?
Yes, but does not work. Probably enabling parent, before
assigned-clocks-parents, happens still too early:
[1.623554] DSI PLL(0) lock failed, status=0x
[1.623556] PLL(0) lock failed
[1.624650] [ cut here ]
[1.624651] disp_cc_mdss_byte0_clk_src: rcg didn't update its
configuration.
Or maybe something is missing in the DSI PHY PLL driver?
>
>>
>> Signed-off-by: Krzysztof Kozlowski
>> ---
>> drivers/gpu/drm/msm/dsi/dsi.h | 2 +
>> drivers/gpu/drm/msm/dsi/dsi_cfg.c | 25 +++
>> drivers/gpu/drm/msm/dsi/dsi_cfg.h | 1 +
>> drivers/gpu/drm/msm/dsi/dsi_host.c | 80
>> ++
>> drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +
>> drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 +
>> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 78
>> +++--
>> .../gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 14
>
> Please separate DSI and PHY changes.
>
>> 8 files changed, 197 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> index
>> 7754dcec33d06e3d6eb8a9d55e53f24af073adb9..e2a8d6fcc45b6c207a3018ea7c8744fcf34dabd2
>> 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
>> @@ -205,6 +205,17 @@ static const struct msm_dsi_config sm8650_dsi_cfg = {
>> },
>> };
>>
>> +static const struct msm_dsi_config sm8750_dsi_cfg = {
>> +.io_offset = DSI_6G_REG_SHIFT,
>> +.regulator_data = sm8650_dsi_regulators,
>> +.num_regulators = ARRAY_SIZE(sm8650_dsi_regulators),
>> +.bus_clk_names = dsi_v2_4_clk_names,
>> +.num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
>> +.io_start = {
>> +{ 0xae94000, 0xae96000 },
>> +},
>> +};
>> +
>> static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
>> { .supply = "vdda", .init_load_uA = 8350 }, /* 1.2 V */
>> { .supply = "refgen" },
>> @@ -257,6 +268,18 @@ static const struct msm_dsi_host_cfg_ops
>> msm_dsi_6g_v2_host_ops = {
>> .calc_clk_rate = dsi_calc_clk_rate_6g,
>> };
>>
>> +static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_9_host_ops = {
>> +.link_clk_set_rate = dsi_link_clk_set_rate_6g_v2_9,
>> +.link_clk_enable = dsi_link_clk_enable_6g,
>> +.link_clk_disable = dsi_link_clk_disable_6g,
>> +.clk_init_ver = dsi_clk_init_6g_v2_9,
>> +.tx_buf_alloc = dsi_tx_buf_alloc_6g,
>> +.tx_buf_get = dsi_tx_buf_get_6g,
>> +.tx_buf_put = dsi_tx_buf_put_6g,
>> +.dma_base_get = dsi_dma_base_get_6g,
>> +.calc_clk_rate = dsi_calc_clk_rate_6g,
>> +};
>> +
>> static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
>> {MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064,
>> &apq8064_dsi_cfg, &msm_dsi_v2_host_ops},
>> @@ -300,6 +323,8 @@ static const struct msm_dsi_cfg_handler
>> dsi_cfg_handlers[] = {
>> &sm8550_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>> {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_8_0,
>> &sm8650_dsi_cfg, &msm_dsi_6g_v2_host_ops},
>> +{MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_9_0,
>> +&sm8750_dsi_cfg, &msm_dsi_6g_v2_9_host_ops},
>> };
>>
>> const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> index
>> 120cb65164c1ba1deb9acb513e5f073bd560c496..859c279afbb0377d16f8406f3e6b083640aff5a1
>> 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> @@ -30,6 +30,7 @@
>> #define MSM_DSI_6G_VER_MINOR_V2_6_0 0x2006
>> #define MSM_DSI_6G_VER_MINOR_V2_7_0 0x2007
>> #define MSM_DSI_6G_VER_MINOR_V2_8_0 0x2008
>> +#define MSM_DSI_6G_VER_MINOR_V2_9_0 0x2009
>>
>> #define MSM_DSI_V2_VER_MINOR_8064 0x0
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index
>> 2218d4f0c5130a0b13f428e8
Re: [PATCH RFC 08/11] drm/msm/dsi: Add support for SM8750
On Thu, Jan 09, 2025 at 02:08:35PM +0100, Krzysztof Kozlowski wrote:
> Add support for DSI PHY v7.0 on Qualcomm SM8750 SoC which comes with two
> differences worth noting:
>
> 1. ICODE_ACCUM_STATUS_LOW and ALOG_OBSV_BUS_STATUS_1 registers - their
>offsets were just switched. Currently these registers are not used
>in the driver, so the easiest is to document both but keep them
>commented out to avoid conflict.
>
> 2. DSI PHY PLLs, the parents of pixel and byte clocks, cannot be used as
>parents before they are prepared and initial rate is set. Therefore
>assigned-clock-parents are not working here and driver is responsible
>for reparenting clocks with proper procedure: see dsi_clk_init_6g_v2_9().
Isn't it a description of CLK_SET_PARENT_GATE and/or
CLK_OPS_PARENT_ENABLE ?
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> drivers/gpu/drm/msm/dsi/dsi.h | 2 +
> drivers/gpu/drm/msm/dsi/dsi_cfg.c | 25 +++
> drivers/gpu/drm/msm/dsi/dsi_cfg.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 80
> ++
> drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 +
> drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 +
> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 78 +++--
> .../gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 14
Please separate DSI and PHY changes.
> 8 files changed, 197 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> index
> 7754dcec33d06e3d6eb8a9d55e53f24af073adb9..e2a8d6fcc45b6c207a3018ea7c8744fcf34dabd2
> 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c
> @@ -205,6 +205,17 @@ static const struct msm_dsi_config sm8650_dsi_cfg = {
> },
> };
>
> +static const struct msm_dsi_config sm8750_dsi_cfg = {
> + .io_offset = DSI_6G_REG_SHIFT,
> + .regulator_data = sm8650_dsi_regulators,
> + .num_regulators = ARRAY_SIZE(sm8650_dsi_regulators),
> + .bus_clk_names = dsi_v2_4_clk_names,
> + .num_bus_clks = ARRAY_SIZE(dsi_v2_4_clk_names),
> + .io_start = {
> + { 0xae94000, 0xae96000 },
> + },
> +};
> +
> static const struct regulator_bulk_data sc7280_dsi_regulators[] = {
> { .supply = "vdda", .init_load_uA = 8350 }, /* 1.2 V */
> { .supply = "refgen" },
> @@ -257,6 +268,18 @@ static const struct msm_dsi_host_cfg_ops
> msm_dsi_6g_v2_host_ops = {
> .calc_clk_rate = dsi_calc_clk_rate_6g,
> };
>
> +static const struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_9_host_ops = {
> + .link_clk_set_rate = dsi_link_clk_set_rate_6g_v2_9,
> + .link_clk_enable = dsi_link_clk_enable_6g,
> + .link_clk_disable = dsi_link_clk_disable_6g,
> + .clk_init_ver = dsi_clk_init_6g_v2_9,
> + .tx_buf_alloc = dsi_tx_buf_alloc_6g,
> + .tx_buf_get = dsi_tx_buf_get_6g,
> + .tx_buf_put = dsi_tx_buf_put_6g,
> + .dma_base_get = dsi_dma_base_get_6g,
> + .calc_clk_rate = dsi_calc_clk_rate_6g,
> +};
> +
> static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = {
> {MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064,
> &apq8064_dsi_cfg, &msm_dsi_v2_host_ops},
> @@ -300,6 +323,8 @@ static const struct msm_dsi_cfg_handler
> dsi_cfg_handlers[] = {
> &sm8550_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_8_0,
> &sm8650_dsi_cfg, &msm_dsi_6g_v2_host_ops},
> + {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_9_0,
> + &sm8750_dsi_cfg, &msm_dsi_6g_v2_9_host_ops},
> };
>
> const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index
> 120cb65164c1ba1deb9acb513e5f073bd560c496..859c279afbb0377d16f8406f3e6b083640aff5a1
> 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -30,6 +30,7 @@
> #define MSM_DSI_6G_VER_MINOR_V2_6_0 0x2006
> #define MSM_DSI_6G_VER_MINOR_V2_7_0 0x2007
> #define MSM_DSI_6G_VER_MINOR_V2_8_0 0x2008
> +#define MSM_DSI_6G_VER_MINOR_V2_9_0 0x2009
>
> #define MSM_DSI_V2_VER_MINOR_80640x0
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index
> 2218d4f0c5130a0b13f428e89aa30ba2921da572..ced28ee61eedc0a82da9f1d0792f17ee2a5538c4
> 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -119,6 +119,15 @@ struct msm_dsi_host {
> struct clk *pixel_clk;
> struct clk *byte_intf_clk;
>
> + /*
> + * Clocks which needs to be properly parented between DISPCC and DSI PHY
> + * PLL:
> + */
> + struct clk *byte_src_clk;
> + struct clk *pixel_src_clk;
> + struct clk *dsi_pll_byte_clk;
> + struct clk *dsi_pll_pixel_clk;
> +
> unsigned long byte_clk_r
