| Message ID | 20240129104106.43141-1-raphael.gallais-pou@foss.st.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Update STM DSI PHY driver | expand |
On 1/29/24 11:41, Raphael Gallais-Pou wrote: > > This patch series aims to add several features of the dw-mipi-dsi phy > driver that are missing or need to be updated. > > First patch update a PM macro. > > Second patch adds runtime PM functionality to the driver. > > Third patch adds a clock provider generated by the PHY itself. As > explained in the commit log of the second patch, a clock declaration is > missing. Since this clock is parent of 'dsi_k', it leads to an orphan > clock. Most importantly this patch is an anticipation for future > versions of the DSI PHY, and its inclusion within the display subsystem > and the DRM framework. > > Last patch fixes a corner effect introduced previously. Since 'dsi' and > 'dsi_k' are gated by the same bit on the same register, both reference > work as peripheral clock in the device-tree. > > --- > Changes in v3-resend: > - Removed last patch as it has been merged > https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/ > > Changes in v3: > - Fix smatch warning (disable dsi->pclk when clk_register fails) > > Changes in v2: > - Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro > and removed __maybe_used for accordingly > - Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS > > Raphael Gallais-Pou (3): > drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro > drm/stm: dsi: expose DSI PHY internal clock > > Yannick Fertre (1): > drm/stm: dsi: add pm runtime ops > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++---- > 1 file changed, 238 insertions(+), 41 deletions(-) > Hi Raphaël & Yannick, Applied on drm-misc-next. Many thanks, Philippe :-)
On Fri, Jun 28, 2024 at 8:47 PM Philippe CORNU <philippe.cornu@foss.st.com> wrote: > > > > On 1/29/24 11:41, Raphael Gallais-Pou wrote: > > > > This patch series aims to add several features of the dw-mipi-dsi phy > > driver that are missing or need to be updated. > > > > First patch update a PM macro. > > > > Second patch adds runtime PM functionality to the driver. > > > > Third patch adds a clock provider generated by the PHY itself. As > > explained in the commit log of the second patch, a clock declaration is > > missing. Since this clock is parent of 'dsi_k', it leads to an orphan > > clock. Most importantly this patch is an anticipation for future > > versions of the DSI PHY, and its inclusion within the display subsystem > > and the DRM framework. > > > > Last patch fixes a corner effect introduced previously. Since 'dsi' and > > 'dsi_k' are gated by the same bit on the same register, both reference > > work as peripheral clock in the device-tree. > > This patch (commit id:185f99b614427360) seems to break the dsi of stm32f469 chip. I'm not familiar with the drm and the clock framework, maybe it's because there is no "ck_dsi_phy" defined for stm32f469. PS: Sorry for receiving multiple copies of this email, I forgot to use plain text mode last time. > > --- > > Changes in v3-resend: > > - Removed last patch as it has been merged > > https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/ > > > > Changes in v3: > > - Fix smatch warning (disable dsi->pclk when clk_register fails) > > > > Changes in v2: > > - Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro > > and removed __maybe_used for accordingly > > - Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS > > > > Raphael Gallais-Pou (3): > > drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro > > drm/stm: dsi: expose DSI PHY internal clock > > > > Yannick Fertre (1): > > drm/stm: dsi: add pm runtime ops > > > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++---- > > 1 file changed, 238 insertions(+), 41 deletions(-) > > > > Hi Raphaël & Yannick, > Applied on drm-misc-next. > Many thanks, > Philippe :-) > _______________________________________________ > Linux-stm32 mailing list > Linux-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
On 7/22/24 10:38, Yanjun Yang wrote: > On Fri, Jun 28, 2024 at 8:47 PM Philippe CORNU > <philippe.cornu@foss.st.com> wrote: >> >> >> >> On 1/29/24 11:41, Raphael Gallais-Pou wrote: >>> >>> This patch series aims to add several features of the dw-mipi-dsi phy >>> driver that are missing or need to be updated. >>> >>> First patch update a PM macro. >>> >>> Second patch adds runtime PM functionality to the driver. >>> >>> Third patch adds a clock provider generated by the PHY itself. As >>> explained in the commit log of the second patch, a clock declaration is >>> missing. Since this clock is parent of 'dsi_k', it leads to an orphan >>> clock. Most importantly this patch is an anticipation for future >>> versions of the DSI PHY, and its inclusion within the display subsystem >>> and the DRM framework. >>> >>> Last patch fixes a corner effect introduced previously. Since 'dsi' and >>> 'dsi_k' are gated by the same bit on the same register, both reference >>> work as peripheral clock in the device-tree. >>> > > This patch (commit id:185f99b614427360) seems to break the dsi of > stm32f469 chip. > I'm not familiar with the drm and the clock framework, maybe it's > because there is no > "ck_dsi_phy" defined for stm32f469. > PS: Sorry for receiving multiple copies of this email, I forgot to > use plain text mode last time. > Hi, Thank you for letting us know that there was this error. We should have detected this before merging, really sorry for the problems caused by this patch. We will investigate the issue and get back to you as soon as possible. In the meantime, I think you can revert this patch in your git tree. Philippe :-) >>> --- >>> Changes in v3-resend: >>> - Removed last patch as it has been merged >>> https://lore.kernel.org/lkml/bf49f4c9-9e81-4c91-972d-13782d996aaa@foss.st.com/ >>> >>> Changes in v3: >>> - Fix smatch warning (disable dsi->pclk when clk_register fails) >>> >>> Changes in v2: >>> - Added patch 1/4 to use SYSTEM_SLEEP_PM_OPS instead of old macro >>> and removed __maybe_used for accordingly >>> - Changed SET_RUNTIME_PM_OPS to RUNTIME_PM_OPS >>> >>> Raphael Gallais-Pou (3): >>> drm/stm: dsi: use new SYSTEM_SLEEP_PM_OPS() macro >>> drm/stm: dsi: expose DSI PHY internal clock >>> >>> Yannick Fertre (1): >>> drm/stm: dsi: add pm runtime ops >>> >>> drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 279 ++++++++++++++++++++++---- >>> 1 file changed, 238 insertions(+), 41 deletions(-) >>> >> >> Hi Raphaël & Yannick, >> Applied on drm-misc-next. >> Many thanks, >> Philippe :-) >> _______________________________________________ >> Linux-stm32 mailing list >> Linux-stm32@st-md-mailman.stormreply.com >> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote: > > > On 7/22/24 10:38, Yanjun Yang wrote: > > > > This patch (commit id:185f99b614427360) seems to break the dsi of > > stm32f469 chip. > > I'm not familiar with the drm and the clock framework, maybe it's > > because there is no > > "ck_dsi_phy" defined for stm32f469. > > PS: Sorry for receiving multiple copies of this email, I forgot to > > use plain text mode last time. > > > > Hi, > Thank you for letting us know that there was this error. We should have > detected this before merging, really sorry for the problems caused by this > patch. We will investigate the issue and get back to you as soon as > possible. In the meantime, I think you can revert this patch in your git > tree. > > Philippe :-) > Hi, After some testing, the reason behind my problem is the parent's name of 'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'. I don't know which is the better why to fix it: 1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined. 2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock rate. Both method can fix my problem. The first one might break other platforms. Maybe I should change the clock name of 'clk-hse'. However, I can't find the defination of this clock name for stm32f4.
Le 29/07/2024 à 15:28, Yanjun Yang a écrit : > On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote: >> >> >> On 7/22/24 10:38, Yanjun Yang wrote: >>> >>> This patch (commit id:185f99b614427360) seems to break the dsi of >>> stm32f469 chip. >>> I'm not familiar with the drm and the clock framework, maybe it's >>> because there is no >>> "ck_dsi_phy" defined for stm32f469. >>> PS: Sorry for receiving multiple copies of this email, I forgot to >>> use plain text mode last time. >>> >> >> Hi, >> Thank you for letting us know that there was this error. We should have >> detected this before merging, really sorry for the problems caused by this >> patch. We will investigate the issue and get back to you as soon as >> possible. In the meantime, I think you can revert this patch in your git >> tree. >> >> Philippe :-) >> > > Hi, Hi, FYI DSI clock tree for stm32f469 can be found here: https://www.st.com/resource/en/reference_manual/rm0386-stm32f469xx-and-stm32f479xx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf Refer to Figure 17: DSI clock tree. After some research I think "ck_dsi_phy" was introduced in stm32h7 platforms. There is a mux which interfaces between various clocks (among ck_hse) and the byte lane clock. stm32f469 has a much simpler clock tree in which we did not bother to implement this "go-between" clock, even though they is an equivalent of the mux. > After some testing, the reason behind my problem is the parent's name of > 'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'. I don't > know which is the better why to fix it: > 1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined. Doing so will definitely break other platforms. > 2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of > "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock > rate. dsi->pllref_clk refers to the HSE clock if you take a look in the device-tree. This is the reason why this work on your setup. I doubt nevertheless that it wouldn't work on other platforms. But this would be a semantic nonsense, since the DSI byte lane clock is not always derived from HSE clock on other platforms. Looking again at the clk-stm32f4 driver and the DSI clock tree linked, we can maybe implement the desired clock even if it is not represented on the diagram. Eventually if this solution does not work we will go to the second solution you suggested and we will test it on all platforms. @Philippe, @Yannick Do you agree with this workflow ? Regards, Raphaël > > Both method can fix my problem. The first one might break other > platforms. Maybe I should change the clock name of 'clk-hse'. However, > I can't find the defination of this clock name for stm32f4.
Hi, we don't give enough attention to older SOCs like stm32f469. This is an error on our part. I think that to fix this point it would be necessary to define the clock hse as clock fix. I hope to be able to release a patch before the end of August Best regards Yannick Fertré Le 01/08/2024 à 11:07, Raphaël Gallais-Pou a écrit : > > > Le 29/07/2024 à 15:28, Yanjun Yang a écrit : >> On Fri, Jul 26, 2024 at 09:55:35AM +0200, Philippe CORNU wrote: >>> >>> >>> On 7/22/24 10:38, Yanjun Yang wrote: >>>> >>>> This patch (commit id:185f99b614427360) seems to break the dsi of >>>> stm32f469 chip. >>>> I'm not familiar with the drm and the clock framework, maybe it's >>>> because there is no >>>> "ck_dsi_phy" defined for stm32f469. >>>> PS: Sorry for receiving multiple copies of this email, I forgot to >>>> use plain text mode last time. >>>> >>> >>> Hi, >>> Thank you for letting us know that there was this error. We should have >>> detected this before merging, really sorry for the problems caused >>> by this >>> patch. We will investigate the issue and get back to you as soon as >>> possible. In the meantime, I think you can revert this patch in your >>> git >>> tree. >>> >>> Philippe :-) >>> >> >> Hi, > Hi, > > FYI > DSI clock tree for stm32f469 can be found here: > https://www.st.com/resource/en/reference_manual/rm0386-stm32f469xx-and-stm32f479xx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf > > > Refer to Figure 17: DSI clock tree. > > After some research I think "ck_dsi_phy" was introduced in stm32h7 > platforms. There is a mux which interfaces between various clocks > (among ck_hse) and the byte lane clock. stm32f469 has a much simpler > clock tree in which we did not bother to implement this "go-between" > clock, even though they is an equivalent of the mux. > >> After some testing, the reason behind my problem is the parent's name of >> 'clk_dsi_phy' for stm32f4 is 'clk-hse' other than 'ck_hse'. I don't >> know which is the better why to fix it: >> 1. Change "ck_hse" to "clk-hse" in where "clk_dsi_phy" is defined. > Doing so will definitely break other platforms. > >> 2. Use "pll_in_khz = clk_get_rate(dsi->pllref_clk) / 1000" instead of >> "pll_in_khz = (unsigned int)(parent_rate / 1000)" when get the clock >> rate. > dsi->pllref_clk refers to the HSE clock if you take a look in the > device-tree. This is the reason why this work on your setup. I doubt > nevertheless that it wouldn't work on other platforms. But this would > be a semantic nonsense, since the DSI byte lane clock is not always > derived from HSE clock on other platforms. > > Looking again at the clk-stm32f4 driver and the DSI clock tree linked, > we can maybe implement the desired clock even if it is not represented > on the diagram. > > Eventually if this solution does not work we will go to the second > solution you suggested and we will test it on all platforms. > > @Philippe, @Yannick > Do you agree with this workflow ? > > Regards, > Raphaël > > >> >> Both method can fix my problem. The first one might break other >> platforms. Maybe I should change the clock name of 'clk-hse'. However, >> I can't find the defination of this clock name for stm32f4.