| Message ID | 20201024162016.1003041-1-aford173@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: imx: Implement blk-ctl driver for i.MX8MN | expand |
On 20-10-24 11:20:12, Adam Ford wrote: > There are some less-documented registers which control clocks and > resets for the multimedia block which controls the LCDIF, ISI, MIPI > CSI, and MIPI DSI. > > The i.Mx8M Nano appears to have a subset of the i.MX8MP registers with > a couple shared registers with the i.MX8MM. This series builds on the > series that have been submitted for both of those other two platforms. > > This is an RFC because when enabling the corresponding DTS node, the > system freezes on power on. There are a couple of clocks that don't > correspond to either the imx8mp nor the imx8mm, so I might have something > wrong, and I was hoping for some constructive feedback in order to get > the imx8m Nano to a similar point of the Mini and Plus. > Thanks for the effort. I'm assuming this relies on the following patchset, right ? https://lkml.org/lkml/2020/10/24/139 > Adam Ford (3): > dt-bindings: clock: imx8mn: Add media blk_ctl clock IDs > dt-bindings: reset: imx8mn: Add media blk_ctl reset IDs > clk: imx: Add blk-ctl driver for i.MX8MN > > drivers/clk/imx/clk-blk-ctl-imx8mn.c | 80 ++++++++++++++++++++++++ > include/dt-bindings/clock/imx8mn-clock.h | 11 ++++ > include/dt-bindings/reset/imx8mn-reset.h | 22 +++++++ > 3 files changed, 113 insertions(+) > create mode 100644 drivers/clk/imx/clk-blk-ctl-imx8mn.c > create mode 100644 include/dt-bindings/reset/imx8mn-reset.h > > -- > 2.25.1 >
On Sat, Oct 24, 2020 at 3:23 PM Abel Vesa <abel.vesa@nxp.com> wrote: > > On 20-10-24 11:20:12, Adam Ford wrote: > > There are some less-documented registers which control clocks and > > resets for the multimedia block which controls the LCDIF, ISI, MIPI > > CSI, and MIPI DSI. > > > > The i.Mx8M Nano appears to have a subset of the i.MX8MP registers with > > a couple shared registers with the i.MX8MM. This series builds on the > > series that have been submitted for both of those other two platforms. > > > > This is an RFC because when enabling the corresponding DTS node, the > > system freezes on power on. There are a couple of clocks that don't > > correspond to either the imx8mp nor the imx8mm, so I might have something > > wrong, and I was hoping for some constructive feedback in order to get > > the imx8m Nano to a similar point of the Mini and Plus. > > > > Thanks for the effort. Sure thing! > > I'm assuming this relies on the following patchset, right ? > https://lkml.org/lkml/2020/10/24/139 Abell, Your link points right back to this e-mail. ;-) I based this work off: https://www.spinics.net/lists/arm-kernel/msg843906.html from Marek which I beleive is based on https://www.spinics.net/lists/arm-kernel/msg836165.html from you. I also have a GPC patch series located: https://www.spinics.net/lists/arm-kernel/msg847925.html Together, both the GPC and the clk-blk driver should be able to pull the multimedia block out of reset. Currently, the GPC can handle the USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by the clock block My original patch RFC didn't include the imx8mn node, because it hangs, but the node I added looks like: media_blk_ctl: clock-controller@32e28000 { compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; reg = <0x32e28000 0x1000>; #clock-cells = <1>; #reset-cells = <1>; }; I was hoping you might have some feedback on the 8mn clk-blk driver since you did the 8mp clk-blk drive and they appear to be very similar. adam > > > Adam Ford (3): > > dt-bindings: clock: imx8mn: Add media blk_ctl clock IDs > > dt-bindings: reset: imx8mn: Add media blk_ctl reset IDs > > clk: imx: Add blk-ctl driver for i.MX8MN > > > > drivers/clk/imx/clk-blk-ctl-imx8mn.c | 80 ++++++++++++++++++++++++ > > include/dt-bindings/clock/imx8mn-clock.h | 11 ++++ > > include/dt-bindings/reset/imx8mn-reset.h | 22 +++++++ > > 3 files changed, 113 insertions(+) > > create mode 100644 drivers/clk/imx/clk-blk-ctl-imx8mn.c > > create mode 100644 include/dt-bindings/reset/imx8mn-reset.h > > > > -- > > 2.25.1 > >
On 20-10-24 16:03:17, Adam Ford wrote: > On Sat, Oct 24, 2020 at 3:23 PM Abel Vesa <abel.vesa@nxp.com> wrote: > > > > On 20-10-24 11:20:12, Adam Ford wrote: > > > There are some less-documented registers which control clocks and > > > resets for the multimedia block which controls the LCDIF, ISI, MIPI > > > CSI, and MIPI DSI. > > > > > > The i.Mx8M Nano appears to have a subset of the i.MX8MP registers with > > > a couple shared registers with the i.MX8MM. This series builds on the > > > series that have been submitted for both of those other two platforms. > > > > > > This is an RFC because when enabling the corresponding DTS node, the > > > system freezes on power on. There are a couple of clocks that don't > > > correspond to either the imx8mp nor the imx8mm, so I might have something > > > wrong, and I was hoping for some constructive feedback in order to get > > > the imx8m Nano to a similar point of the Mini and Plus. > > > > > > > Thanks for the effort. > > Sure thing! > > > > > I'm assuming this relies on the following patchset, right ? > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F10%2F24%2F139&data=04%7C01%7Cabel.vesa%40nxp.com%7C00007fdffcb44af4bbe808d8786044cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637391702150893977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RIdR4O2Qx3q7ovfAO7O3kc%2BpXXMPzLQJWPoUAH305%2Bs%3D&reserved=0 > Abell, > > Your link points right back to this e-mail. ;-) Sorry about that. Was kinda late here yesterday. > > I based this work off: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg843906.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C00007fdffcb44af4bbe808d8786044cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637391702150893977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6B3UHZNULMB%2FtMzdWHckIOFlqFxEYwYdQclzqDtJ%2FNQ%3D&reserved=0 from Marek > which I beleive is based on > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg836165.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C00007fdffcb44af4bbe808d8786044cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637391702150893977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=I%2FfgIjzqwnjuSGOPfVKa%2BPbx950Be008sOon%2FDwSO1o%3D&reserved=0 from you. > > I also have a GPC patch series located: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg847925.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C00007fdffcb44af4bbe808d8786044cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637391702150893977%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dF%2Bgth7dwopoB2rvQ8IqspTZ7kWxvMYGS0dY%2F0gWgXE%3D&reserved=0 > > Together, both the GPC and the clk-blk driver should be able to pull > the multimedia block out of reset. Currently, the GPC can handle the > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > the clock block > > My original patch RFC didn't include the imx8mn node, because it > hangs, but the node I added looks like: > > media_blk_ctl: clock-controller@32e28000 { > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > reg = <0x32e28000 0x1000>; > #clock-cells = <1>; > #reset-cells = <1>; > }; > > I was hoping you might have some feedback on the 8mn clk-blk driver > since you did the 8mp clk-blk drive and they appear to be very > similar. > I'll do you one better still. I'll apply the patch in my tree and give it a test tomorrow morning. > adam > > > > > > > Adam Ford (3): > > > dt-bindings: clock: imx8mn: Add media blk_ctl clock IDs > > > dt-bindings: reset: imx8mn: Add media blk_ctl reset IDs > > > clk: imx: Add blk-ctl driver for i.MX8MN > > > > > > drivers/clk/imx/clk-blk-ctl-imx8mn.c | 80 ++++++++++++++++++++++++ > > > include/dt-bindings/clock/imx8mn-clock.h | 11 ++++ > > > include/dt-bindings/reset/imx8mn-reset.h | 22 +++++++ > > > 3 files changed, 113 insertions(+) > > > create mode 100644 drivers/clk/imx/clk-blk-ctl-imx8mn.c > > > create mode 100644 include/dt-bindings/reset/imx8mn-reset.h > > > > > > -- > > > 2.25.1 > > >
On 10/25/20 1:05 PM, Abel Vesa wrote: [...] >> Together, both the GPC and the clk-blk driver should be able to pull >> the multimedia block out of reset. Currently, the GPC can handle the >> USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by >> the clock block >> >> My original patch RFC didn't include the imx8mn node, because it >> hangs, but the node I added looks like: >> >> media_blk_ctl: clock-controller@32e28000 { >> compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; >> reg = <0x32e28000 0x1000>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> }; >> >> I was hoping you might have some feedback on the 8mn clk-blk driver >> since you did the 8mp clk-blk drive and they appear to be very >> similar. >> > > I'll do you one better still. I'll apply the patch in my tree and give it > a test tomorrow morning. You can also apply the one for 8MM: https://lore.kernel.org/linux-arm-kernel/20201003224555.163780-5-marex@denx.de/
On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > [...] > > >> Together, both the GPC and the clk-blk driver should be able to pull > >> the multimedia block out of reset. Currently, the GPC can handle the > >> USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > >> the clock block > >> > >> My original patch RFC didn't include the imx8mn node, because it > >> hangs, but the node I added looks like: > >> > >> media_blk_ctl: clock-controller@32e28000 { > >> compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > >> reg = <0x32e28000 0x1000>; > >> #clock-cells = <1>; > >> #reset-cells = <1>; > >> }; > >> > >> I was hoping you might have some feedback on the 8mn clk-blk driver > >> since you did the 8mp clk-blk drive and they appear to be very > >> similar. > >> > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > a test tomorrow morning. I do have some more updates on how to get the system to not hang, and to enumerate more clocks. Looking at Marek's work on enabling clocks in the 8MM, he added a power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display clocks out of reset. Unfortunately, the i.MX8MN needs to have 0x100 written to both 32e28000 and 32e28004, and the values written for the 8MM are not compatible. By forcing the GPC to write those values, I can get lcdif_pixel_clk and the mipi_dsi_clkref appearing on the Nano. video_pll1_ref_sel 0 0 0 24000000 0 0 50000 video_pll1 0 0 0 594000000 0 0 50000 video_pll1_bypass 0 0 0 594000000 0 0 50000 video_pll1_out 0 0 0 594000000 0 0 50000 disp_pixel 0 0 0 594000000 0 0 50000 lcdif_pixel_clk 0 0 0 594000000 0 0 50000 disp_pixel_clk 0 0 0 594000000 0 0 50000 dsi_phy_ref 0 0 0 27000000 0 0 50000 mipi_dsi_clkref 0 0 0 27000000 0 0 50000 I am not 100% certain the clock parents in the clk block driver for the 8MN are correct, and I am not seeing the mipi_dsi_pclk Once the dust settles on the GPC decision for Mini and Nano, I think we'll need a more generic way to pass the bits we need to set in clock block to the GPC. adam > > You can also apply the one for 8MM: > https://lore.kernel.org/linux-arm-kernel/20201003224555.163780-5-marex@denx.de/
On 20-10-25 11:05:32, Adam Ford wrote: > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > [...] > > > > >> Together, both the GPC and the clk-blk driver should be able to pull > > >> the multimedia block out of reset. Currently, the GPC can handle the > > >> USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > >> the clock block > > >> > > >> My original patch RFC didn't include the imx8mn node, because it > > >> hangs, but the node I added looks like: > > >> > > >> media_blk_ctl: clock-controller@32e28000 { > > >> compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > >> reg = <0x32e28000 0x1000>; > > >> #clock-cells = <1>; > > >> #reset-cells = <1>; > > >> }; > > >> > > >> I was hoping you might have some feedback on the 8mn clk-blk driver > > >> since you did the 8mp clk-blk drive and they appear to be very > > >> similar. > > >> > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > a test tomorrow morning. > > I do have some more updates on how to get the system to not hang, and > to enumerate more clocks. > Looking at Marek's work on enabling clocks in the 8MM, he added a > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > clocks out of reset. > Yeah, that makes sense. Basically, it was trying to disable unused clocks (see clk_disable_unused) but in order to disable the clocks from the media BLK_CTL (which I think should be renamed in display BLK_CTL) the PD need to be on. Since you initially didn't give it any PD, it was trying to blindly write/read the gate bit and therefore freeze. > Unfortunately, the i.MX8MN needs to have 0x100 written to both > 32e28000 and 32e28004, and the values written for the 8MM are not > compatible. > By forcing the GPC to write those values, I can get lcdif_pixel_clk > and the mipi_dsi_clkref appearing on the Nano. I'm trying to make a branch with all the patches for all i.MX8M so I can keep track of it all. On this branch I've also applied the following patchset from Lucas Stach: https://www.spinics.net/lists/arm-kernel/msg843007.html but I'm getting the folowing errors: [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 Lucas, any thoughts? Maybe it's something related to 8MN. Will dig further, see what pops out. > > video_pll1_ref_sel 0 0 0 24000000 > 0 0 50000 > video_pll1 0 0 0 594000000 > 0 0 50000 > video_pll1_bypass 0 0 0 594000000 > 0 0 50000 > video_pll1_out 0 0 0 594000000 > 0 0 50000 > disp_pixel 0 0 0 594000000 > 0 0 50000 > lcdif_pixel_clk 0 0 0 > 594000000 0 0 50000 > disp_pixel_clk 0 0 0 > 594000000 0 0 50000 > dsi_phy_ref 0 0 0 27000000 > 0 0 50000 > mipi_dsi_clkref 0 0 0 > 27000000 0 0 50000 > > I am not 100% certain the clock parents in the clk block driver for > the 8MN are correct, and I am not seeing the mipi_dsi_pclk > > Once the dust settles on the GPC decision for Mini and Nano, I think > we'll need a more generic way to pass the bits we need to set in clock > block to the GPC. > > adam > > > > You can also apply the one for 8MM: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-arm-kernel%2F20201003224555.163780-5-marex%40denx.de%2F&data=04%7C01%7Cabel.vesa%40nxp.com%7Cae966cce11204214fb1908d878ffd492%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637392387462398200%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M944BaOI7Sa1RmI0nwrshKaM7MGMEN5pWgjmYqXZkns%3D&reserved=0
On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > On 20-10-25 11:05:32, Adam Ford wrote: > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > [...] > > > > > > >> Together, both the GPC and the clk-blk driver should be able to pull > > > >> the multimedia block out of reset. Currently, the GPC can handle the > > > >> USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > >> the clock block > > > >> > > > >> My original patch RFC didn't include the imx8mn node, because it > > > >> hangs, but the node I added looks like: > > > >> > > > >> media_blk_ctl: clock-controller@32e28000 { > > > >> compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > >> reg = <0x32e28000 0x1000>; > > > >> #clock-cells = <1>; > > > >> #reset-cells = <1>; > > > >> }; > > > >> > > > >> I was hoping you might have some feedback on the 8mn clk-blk driver > > > >> since you did the 8mp clk-blk drive and they appear to be very > > > >> similar. > > > >> > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > a test tomorrow morning. > > > > I do have some more updates on how to get the system to not hang, and > > to enumerate more clocks. > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > clocks out of reset. > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > (see clk_disable_unused) but in order to disable the clocks from the > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > PD need to be on. Since you initially didn't give it any PD, it was trying > to blindly write/read the gate bit and therefore freeze. > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > 32e28000 and 32e28004, and the values written for the 8MM are not > > compatible. > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > and the mipi_dsi_clkref appearing on the Nano. > > I'm trying to make a branch with all the patches for all i.MX8M so I > can keep track of it all. On this branch I've also applied the > following patchset from Lucas Stach: > https://www.spinics.net/lists/arm-kernel/msg843007.html > but I'm getting the folowing errors: > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > Lucas, any thoughts? > > Maybe it's something related to 8MN. > I will go back and double check this now that we have both the blt_crl->power-domain and the power-domain->blk_ctl. > Will dig further, see what pops out. I wasn't sure which direction to go with the name. I can rename the media_blk_ctl driver to display_blk_ctl. I used Media based on the imx8mp naming convention and the fact that it's controlling both the display and the camera interface, however it's depending on the dispmix GPC. I'll submit a RFC V2 with the cross referencing to the GPC based on Marek's Mini patch set, but we'll still have an issue where the Mini and Nano have different syscon values to enable the clocks, and Marek's branch has it card-coded, so my patch would effectively break the Mini in order to make the Nano operate until we find a better solution. adam > > > > > video_pll1_ref_sel 0 0 0 24000000 > > 0 0 50000 > > video_pll1 0 0 0 594000000 > > 0 0 50000 > > video_pll1_bypass 0 0 0 594000000 > > 0 0 50000 > > video_pll1_out 0 0 0 594000000 > > 0 0 50000 > > disp_pixel 0 0 0 594000000 > > 0 0 50000 > > lcdif_pixel_clk 0 0 0 > > 594000000 0 0 50000 > > disp_pixel_clk 0 0 0 > > 594000000 0 0 50000 > > dsi_phy_ref 0 0 0 27000000 > > 0 0 50000 > > mipi_dsi_clkref 0 0 0 > > 27000000 0 0 50000 > > > > I am not 100% certain the clock parents in the clk block driver for > > the 8MN are correct, and I am not seeing the mipi_dsi_pclk > > > > Once the dust settles on the GPC decision for Mini and Nano, I think > > we'll need a more generic way to pass the bits we need to set in clock > > block to the GPC. > > > > adam > > > > > > You can also apply the one for 8MM: > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-arm-kernel%2F20201003224555.163780-5-marex%40denx.de%2F&data=04%7C01%7Cabel.vesa%40nxp.com%7Cae966cce11204214fb1908d878ffd492%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637392387462398200%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=M944BaOI7Sa1RmI0nwrshKaM7MGMEN5pWgjmYqXZkns%3D&reserved=0
Am Montag, den 26.10.2020, 16:55 +0200 schrieb Abel Vesa: > On 20-10-25 11:05:32, Adam Ford wrote: > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > [...] > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > the clock block > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > hangs, but the node I added looks like: > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > reg = <0x32e28000 0x1000>; > > > > > #clock-cells = <1>; > > > > > #reset-cells = <1>; > > > > > }; > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > similar. > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > a test tomorrow morning. > > > > I do have some more updates on how to get the system to not hang, and > > to enumerate more clocks. > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > clocks out of reset. > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > (see clk_disable_unused) but in order to disable the clocks from the > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > PD need to be on. Since you initially didn't give it any PD, it was trying > to blindly write/read the gate bit and therefore freeze. > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > 32e28000 and 32e28004, and the values written for the 8MM are not > > compatible. > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > and the mipi_dsi_clkref appearing on the Nano. > > I'm trying to make a branch with all the patches for all i.MX8M so I > can keep track of it all. On this branch I've also applied the > following patchset from Lucas Stach: > https://www.spinics.net/lists/arm-kernel/msg843007.html > but I'm getting the folowing errors: > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > Lucas, any thoughts? > > Maybe it's something related to 8MN. The ADB is apparently clocked by one of the BLK_CTL clocks, so the ADB handshake ack will only work when the BLK_CTL clocks are enabled. So I guess the GPC driver should enable those clocks and assert the resets at the right time in the power-up sequencing. Unfortunately this means we can't properly put the BLK_CTL driver in the power-domain without having a cyclic dependency in the DT. I'm still thinking about how to solve this properly. Regards, Lucas
Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > On 20-10-25 11:05:32, Adam Ford wrote: > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > [...] > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > the clock block > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > reg = <0x32e28000 0x1000>; > > > > > > #clock-cells = <1>; > > > > > > #reset-cells = <1>; > > > > > > }; > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > similar. > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > a test tomorrow morning. > > > > > > I do have some more updates on how to get the system to not hang, and > > > to enumerate more clocks. > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > clocks out of reset. > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > (see clk_disable_unused) but in order to disable the clocks from the > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > PD need to be on. Since you initially didn't give it any PD, it was trying > > to blindly write/read the gate bit and therefore freeze. > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > compatible. > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > and the mipi_dsi_clkref appearing on the Nano. > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > can keep track of it all. On this branch I've also applied the > > following patchset from Lucas Stach: > > https://www.spinics.net/lists/arm-kernel/msg843007.html > > but I'm getting the folowing errors: > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > Lucas, any thoughts? > > > > Maybe it's something related to 8MN. > > > I will go back and double check this now that we have both the > blt_crl->power-domain and the power-domain->blk_ctl. > > > Will dig further, see what pops out. > > I wasn't sure which direction to go with the name. I can rename the > media_blk_ctl driver to display_blk_ctl. I used Media based on the > imx8mp naming convention and the fact that it's controlling both the > display and the camera interface, however it's depending on the > dispmix GPC. > > I'll submit a RFC V2 with the cross referencing to the GPC based on > Marek's Mini patch set, but we'll still have an issue where the Mini > and Nano have different syscon values to enable the clocks, and > Marek's branch has it card-coded, so my patch would effectively break > the Mini in order to make the Nano operate until we find a better > solution. The GPC should not write into the BLK_CTL region via syscon, but instead use the clocks and resets as exposed by the BLK_CTL driver. Doing it via syscon is a hack to get things going. The clocks and resets should properly be hooked up to the GPC domains via the clocks and resets DT properties. For the clocks there is one complication: if the clocks are controlled via BLK_CTL we can only enable them once the domain is powered up, however the earlier designs using the GPCv2 assert resets as part of the power up sequence, which needs the clocks to be running for the reset to propagate. So depending on whether we have a power domain with a BLK_CTL or not we need to enable the clocks before or after powering up the domain. I guess we need a new DT property to specify which way the domain needs to handled. Regards, Lucas
On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > [...] > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > the clock block > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > #clock-cells = <1>; > > > > > > > #reset-cells = <1>; > > > > > > > }; > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > similar. > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > a test tomorrow morning. > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > to enumerate more clocks. > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > clocks out of reset. > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > (see clk_disable_unused) but in order to disable the clocks from the > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > compatible. > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > can keep track of it all. On this branch I've also applied the > > > following patchset from Lucas Stach: > > > https://www.spinics.net/lists/arm-kernel/msg843007.html > > > but I'm getting the folowing errors: > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > Lucas, any thoughts? > > > > > > Maybe it's something related to 8MN. > > > > > I will go back and double check this now that we have both the > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > Will dig further, see what pops out. > > > > I wasn't sure which direction to go with the name. I can rename the > > media_blk_ctl driver to display_blk_ctl. I used Media based on the > > imx8mp naming convention and the fact that it's controlling both the > > display and the camera interface, however it's depending on the > > dispmix GPC. > > > > I'll submit a RFC V2 with the cross referencing to the GPC based on > > Marek's Mini patch set, but we'll still have an issue where the Mini > > and Nano have different syscon values to enable the clocks, and > > Marek's branch has it card-coded, so my patch would effectively break > > the Mini in order to make the Nano operate until we find a better > > solution. > > The GPC should not write into the BLK_CTL region via syscon, but > instead use the clocks and resets as exposed by the BLK_CTL driver. > Doing it via syscon is a hack to get things going. The clocks and > resets should properly be hooked up to the GPC domains via the clocks > and resets DT properties. > > For the clocks there is one complication: if the clocks are controlled > via BLK_CTL we can only enable them once the domain is powered up, > however the earlier designs using the GPCv2 assert resets as part of > the power up sequence, which needs the clocks to be running for the > reset to propagate. So depending on whether we have a power domain with > a BLK_CTL or not we need to enable the clocks before or after powering > up the domain. I guess we need a new DT property to specify which way > the domain needs to handled. So in the case of Nano, could we create two blocks instead of one? The first block would enable the bus clock and reset that correspond to writing 0x100 to avoid writing to syscon. From there, we reference that reset and clock from the GPC displaymix_pd to enable the access. Once that's done, we point the 2nd block power-domain to the dispmix_pd to unlock the remaining clocks. Would that work? I can try it later today, but I'm not near the hardware now. adam > > Regards, > Lucas >
On 20-10-26 16:37:51, Lucas Stach wrote: > Am Montag, den 26.10.2020, 16:55 +0200 schrieb Abel Vesa: > > On 20-10-25 11:05:32, Adam Ford wrote: > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > [...] > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > the clock block > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > reg = <0x32e28000 0x1000>; > > > > > > #clock-cells = <1>; > > > > > > #reset-cells = <1>; > > > > > > }; > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > similar. > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > a test tomorrow morning. > > > > > > I do have some more updates on how to get the system to not hang, and > > > to enumerate more clocks. > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > clocks out of reset. > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > (see clk_disable_unused) but in order to disable the clocks from the > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > PD need to be on. Since you initially didn't give it any PD, it was trying > > to blindly write/read the gate bit and therefore freeze. > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > compatible. > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > and the mipi_dsi_clkref appearing on the Nano. > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > can keep track of it all. On this branch I've also applied the > > following patchset from Lucas Stach: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg843007.html&data=04%7C01%7Cabel.vesa%40nxp.com%7Cc930b0f523c44946a93f08d879c51c3f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637393234789815116%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ZzzUpEiEVcWqQQ5azAx8dkjHgiSMwkK04tqi32uLKbU%3D&reserved=0 > > but I'm getting the folowing errors: > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > Lucas, any thoughts? > > > > Maybe it's something related to 8MN. > > The ADB is apparently clocked by one of the BLK_CTL clocks, so the ADB > handshake ack will only work when the BLK_CTL clocks are enabled. So I > guess the GPC driver should enable those clocks and assert the resets > at the right time in the power-up sequencing. Unfortunately this means > we can't properly put the BLK_CTL driver in the power-domain without > having a cyclic dependency in the DT. I'm still thinking about how to > solve this properly. > I remember we had something similar in our internal tree with the bus_blk_clk on 8MP, which was added by the media BLK_CTL. What I did was to just drop the registration of that clock entirely. My rationale was that if the clock is part of the BLK_CTL but also needed by the BLK_CTL to work, I can leave it alone (that is, enabled by default) since when the PD will be powered off the clock will gated too. I guess another option would be to mark it as critical, that way, it will never be disabled (will be left alone by the clk_disable_unused too) but at the same time will be visible in the clock hierarchy. > Regards, > Lucas >
On 20-10-26 16:44:13, Lucas Stach wrote: > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > [...] > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > the clock block > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > #clock-cells = <1>; > > > > > > > #reset-cells = <1>; > > > > > > > }; > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > similar. > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > a test tomorrow morning. > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > to enumerate more clocks. > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > clocks out of reset. > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > (see clk_disable_unused) but in order to disable the clocks from the > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > compatible. > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > can keep track of it all. On this branch I've also applied the > > > following patchset from Lucas Stach: > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg843007.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C02bcfb84d35f4c41a05e08d879c5fe57%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637393238611075979%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=fQnrRpGOnk0B4tFCFsfadKK2qozZwxK4ddycmv4VPls%3D&reserved=0 > > > but I'm getting the folowing errors: > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > Lucas, any thoughts? > > > > > > Maybe it's something related to 8MN. > > > > > I will go back and double check this now that we have both the > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > Will dig further, see what pops out. > > > > I wasn't sure which direction to go with the name. I can rename the > > media_blk_ctl driver to display_blk_ctl. I used Media based on the > > imx8mp naming convention and the fact that it's controlling both the > > display and the camera interface, however it's depending on the > > dispmix GPC. > > > > I'll submit a RFC V2 with the cross referencing to the GPC based on > > Marek's Mini patch set, but we'll still have an issue where the Mini > > and Nano have different syscon values to enable the clocks, and > > Marek's branch has it card-coded, so my patch would effectively break > > the Mini in order to make the Nano operate until we find a better > > solution. > > The GPC should not write into the BLK_CTL region via syscon, but > instead use the clocks and resets as exposed by the BLK_CTL driver. > Doing it via syscon is a hack to get things going. The clocks and > resets should properly be hooked up to the GPC domains via the clocks > and resets DT properties. > Totally agree. The syscon is there for other GPRs of any BLK_CTL which do not fit in a clock or reset controller. So please do not ever use the syscon for touching clocks or reset. Actually, I'm thinking of a way to make sure the use of syscon for clocks and resets in the BLK_CTL would be protected against. > For the clocks there is one complication: if the clocks are controlled > via BLK_CTL we can only enable them once the domain is powered up, > however the earlier designs using the GPCv2 assert resets as part of > the power up sequence, which needs the clocks to be running for the > reset to propagate. So depending on whether we have a power domain with > a BLK_CTL or not we need to enable the clocks before or after powering > up the domain. I guess we need a new DT property to specify which way > the domain needs to handled. > All the BLK_CTLs have their own power domains. No question there. Now if there are resets anc clocks that are part of the BLK_CTL and need to be asserted/enabled on runtime resume, I think we can implement a generic way to do that in the pm_runtime callbacks. Something similar to the .pm_runtime_saved_regs, maybe. > Regards, > Lucas >
On 20-10-27 11:31:10, Abel Vesa wrote: > On 20-10-26 16:37:51, Lucas Stach wrote: > > Am Montag, den 26.10.2020, 16:55 +0200 schrieb Abel Vesa: > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > [...] > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > the clock block > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > #clock-cells = <1>; > > > > > > > #reset-cells = <1>; > > > > > > > }; > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > similar. > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > a test tomorrow morning. > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > to enumerate more clocks. > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > clocks out of reset. > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > (see clk_disable_unused) but in order to disable the clocks from the > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > compatible. > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > can keep track of it all. On this branch I've also applied the > > > following patchset from Lucas Stach: > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg843007.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C5ff46189143747fce45908d87a5b4281%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637393879674506099%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ELDCbfLvxrB6FLwnsA6VyGlU5V3qpA2ImfPAbZnWyzI%3D&reserved=0 > > > but I'm getting the folowing errors: > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > Lucas, any thoughts? > > > > > > Maybe it's something related to 8MN. > > > > The ADB is apparently clocked by one of the BLK_CTL clocks, so the ADB > > handshake ack will only work when the BLK_CTL clocks are enabled. So I > > guess the GPC driver should enable those clocks and assert the resets > > at the right time in the power-up sequencing. Unfortunately this means > > we can't properly put the BLK_CTL driver in the power-domain without > > having a cyclic dependency in the DT. I'm still thinking about how to > > solve this properly. > > > > I remember we had something similar in our internal tree with the > bus_blk_clk on 8MP, which was added by the media BLK_CTL. What I did was to > just drop the registration of that clock entirely. My rationale was that if > the clock is part of the BLK_CTL but also needed by the BLK_CTL to work, > I can leave it alone (that is, enabled by default) since when the PD will be > powered off the clock will gated too. I guess another option would be to > mark it as critical, that way, it will never be disabled (will be left alone > by the clk_disable_unused too) but at the same time will be visible in the > clock hierarchy. > Do ignore evrything I said about the bus_blk_ctl, that did work on our tree since the whole PD power on/off "magic" is done in TF-A. So the problem, as I understand it now, is the fact that the blk_ctl driver won't probe because it needs its PD, but the PD is not registered because the ADB400 can't power up since it needs the bus_blk_ctl clock enabled, clock which is registered by the blk_ctl. > > Regards, > > Lucas > >
> -----Original Message----- > From: Abel Vesa [mailto:abel.vesa@nxp.com] > Sent: Tuesday, October 27, 2020 7:55 PM > To: Lucas Stach <l.stach@pengutronix.de> > Cc: Adam Ford <aford173@gmail.com>; Marek Vasut <marex@denx.de>; > devicetree <devicetree@vger.kernel.org>; Sascha Hauer > <s.hauer@pengutronix.de>; Philipp Zabel <p.zabel@pengutronix.de>; > Stephen Boyd <sboyd@kernel.org>; Fabio Estevam <festevam@gmail.com>; > Michael Turquette <mturquette@baylibre.com>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>; > dl-linux-imx <linux-imx@nxp.com>; Pengutronix Kernel Team > <kernel@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>; linux-clk > <linux-clk@vger.kernel.org>; arm-soc <linux-arm-kernel@lists.infradead.org> > Subject: Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN > > On 20-10-27 11:31:10, Abel Vesa wrote: > > On 20-10-26 16:37:51, Lucas Stach wrote: > > > Am Montag, den 26.10.2020, 16:55 +0200 schrieb Abel Vesa: > > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> > wrote: > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > > > [...] > > > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be > > > > > > > > able to pull the multimedia block out of reset. > > > > > > > > Currently, the GPC can handle the USB OTG and the GPU, but > > > > > > > > the LCDIF and MIPI DSI appear to be gated by the clock > > > > > > > > block > > > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, > > > > > > > > because it hangs, but the node I added looks like: > > > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > > #clock-cells = <1>; > > > > > > > > #reset-cells = <1>; > > > > > > > > }; > > > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn > > > > > > > > clk-blk driver since you did the 8mp clk-blk drive and > > > > > > > > they appear to be very similar. > > > > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my > > > > > > > tree and give it a test tomorrow morning. > > > > > > > > > > I do have some more updates on how to get the system to not > > > > > hang, and to enumerate more clocks. > > > > > Looking at Marek's work on enabling clocks in the 8MM, he added > > > > > a power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the > > > > > display clocks out of reset. > > > > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused > > > > clocks (see clk_disable_unused) but in order to disable the clocks > > > > from the media BLK_CTL (which I think should be renamed in display > > > > BLK_CTL) the PD need to be on. Since you initially didn't give it > > > > any PD, it was trying to blindly write/read the gate bit and therefore > freeze. > > > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > > 32e28000 and 32e28004, and the values written for the 8MM are > > > > > not compatible. > > > > > By forcing the GPC to write those values, I can get > > > > > lcdif_pixel_clk and the mipi_dsi_clkref appearing on the Nano. > > > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so > > > > I can keep track of it all. On this branch I've also applied the > > > > following patchset from Lucas Stach: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > > www.spinics.net%2Flists%2Farm-kernel%2Fmsg843007.html&data=04% > > > > > 7C01%7Cping.bai%40nxp.com%7C0c54623294334a04a01208d87a6f3163%7 > C686 > > > > > ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637393965282215014%7C > Unkno > > > > > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h > a > > > > > WwiLCJXVCI6Mn0%3D%7C1000&sdata=vFbBn94CPsShV72rOCtbTcz5u0 > qh7Au > > > > o44Sb2%2BiBlrE%3D&reserved=0 but I'm getting the folowing > > > > errors: > > > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > > > Lucas, any thoughts? > > > > > > > > Maybe it's something related to 8MN. > > > > > > The ADB is apparently clocked by one of the BLK_CTL clocks, so the > > > ADB handshake ack will only work when the BLK_CTL clocks are > > > enabled. So I guess the GPC driver should enable those clocks and > > > assert the resets at the right time in the power-up sequencing. > > > Unfortunately this means we can't properly put the BLK_CTL driver in > > > the power-domain without having a cyclic dependency in the DT. I'm > > > still thinking about how to solve this properly. > > > > > > > I remember we had something similar in our internal tree with the > > bus_blk_clk on 8MP, which was added by the media BLK_CTL. What I did > > was to just drop the registration of that clock entirely. My rationale > > was that if the clock is part of the BLK_CTL but also needed by the > > BLK_CTL to work, I can leave it alone (that is, enabled by default) > > since when the PD will be powered off the clock will gated too. I > > guess another option would be to mark it as critical, that way, it > > will never be disabled (will be left alone by the clk_disable_unused > > too) but at the same time will be visible in the clock hierarchy. > > > > Do ignore evrything I said about the bus_blk_ctl, that did work on our tree > since the whole PD power on/off "magic" is done in TF-A. > > So the problem, as I understand it now, is the fact that the blk_ctl driver won't > probe because it needs its PD, but the PD is not registered because the > ADB400 can't power up since it needs the bus_blk_ctl clock enabled, clock > which is registered by the blk_ctl. 1. For some MIX's BLK_CTL GPRs, it can only be accessed when the MIX PD is on 2. for some MIX's ADB handshake, need to config some BLK_CTL clock_en bit to enable the MIX internal bus clock. That's why I have concern on implementing such MIX GPR as clock & reset driver, and implementing GPC PD in linux kernel. It will introduce some chicken-egg issue that not easy to handle in linux kernel. BR Jacky Bai > > > > Regards, > > > Lucas > > >
Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford: > On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > > > [...] > > > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > > the clock block > > > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > > #clock-cells = <1>; > > > > > > > > #reset-cells = <1>; > > > > > > > > }; > > > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > > similar. > > > > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > > a test tomorrow morning. > > > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > > to enumerate more clocks. > > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > > clocks out of reset. > > > > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > > (see clk_disable_unused) but in order to disable the clocks from the > > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > > compatible. > > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > > can keep track of it all. On this branch I've also applied the > > > > following patchset from Lucas Stach: > > > > https://www.spinics.net/lists/arm-kernel/msg843007.html > > > > but I'm getting the folowing errors: > > > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > > > Lucas, any thoughts? > > > > > > > > Maybe it's something related to 8MN. > > > > > > > I will go back and double check this now that we have both the > > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > > > Will dig further, see what pops out. > > > > > > I wasn't sure which direction to go with the name. I can rename the > > > media_blk_ctl driver to display_blk_ctl. I used Media based on the > > > imx8mp naming convention and the fact that it's controlling both the > > > display and the camera interface, however it's depending on the > > > dispmix GPC. > > > > > > I'll submit a RFC V2 with the cross referencing to the GPC based on > > > Marek's Mini patch set, but we'll still have an issue where the Mini > > > and Nano have different syscon values to enable the clocks, and > > > Marek's branch has it card-coded, so my patch would effectively break > > > the Mini in order to make the Nano operate until we find a better > > > solution. > > > > The GPC should not write into the BLK_CTL region via syscon, but > > instead use the clocks and resets as exposed by the BLK_CTL driver. > > Doing it via syscon is a hack to get things going. The clocks and > > resets should properly be hooked up to the GPC domains via the clocks > > and resets DT properties. > > > > For the clocks there is one complication: if the clocks are controlled > > via BLK_CTL we can only enable them once the domain is powered up, > > however the earlier designs using the GPCv2 assert resets as part of > > the power up sequence, which needs the clocks to be running for the > > reset to propagate. So depending on whether we have a power domain with > > a BLK_CTL or not we need to enable the clocks before or after powering > > up the domain. I guess we need a new DT property to specify which way > > the domain needs to handled. > > So in the case of Nano, could we create two blocks instead of one? > The first block would enable the bus clock and reset that correspond > to writing 0x100 to avoid writing to syscon. From there, we reference > that reset and clock from the GPC displaymix_pd to enable the access. > Once that's done, we point the 2nd block power-domain to the > dispmix_pd to unlock the remaining clocks. > > Would that work? I can try it later today, but I'm not near the hardware now. Splitting the PD into 2 staged domains might actually work well to get around the cyclic dependency between GPC and BLK_CTL. It's not totally to my liking, as the DT description doesn't map 1:1 to hardware anymore, but it seems to be the most elegant solution to get around the dependency. I'll try to implement this on the i.MX8MM today or tomorrow to see if it holds up in reality or if there are some hidden warts. Regards, Lucas
On 20-10-29 12:54:58, Lucas Stach wrote: > Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford: > > On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > > > the clock block > > > > > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > > > #clock-cells = <1>; > > > > > > > > > #reset-cells = <1>; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > > > similar. > > > > > > > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > > > a test tomorrow morning. > > > > > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > > > to enumerate more clocks. > > > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > > > clocks out of reset. > > > > > > > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > > > (see clk_disable_unused) but in order to disable the clocks from the > > > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > > > compatible. > > > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > > > can keep track of it all. On this branch I've also applied the > > > > > following patchset from Lucas Stach: > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg843007.html&data=04%7C01%7Cabel.vesa%40nxp.com%7C7ee028d464cf451e0e2d08d87c0177cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637395693031971288%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yKGtS%2FTyKjGHZ2xcXSI8%2F74R9vUjPmXgT9FSQfUfZB4%3D&reserved=0 > > > > > but I'm getting the folowing errors: > > > > > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > > > > > Lucas, any thoughts? > > > > > > > > > > Maybe it's something related to 8MN. > > > > > > > > > I will go back and double check this now that we have both the > > > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > > > > > Will dig further, see what pops out. > > > > > > > > I wasn't sure which direction to go with the name. I can rename the > > > > media_blk_ctl driver to display_blk_ctl. I used Media based on the > > > > imx8mp naming convention and the fact that it's controlling both the > > > > display and the camera interface, however it's depending on the > > > > dispmix GPC. > > > > > > > > I'll submit a RFC V2 with the cross referencing to the GPC based on > > > > Marek's Mini patch set, but we'll still have an issue where the Mini > > > > and Nano have different syscon values to enable the clocks, and > > > > Marek's branch has it card-coded, so my patch would effectively break > > > > the Mini in order to make the Nano operate until we find a better > > > > solution. > > > > > > The GPC should not write into the BLK_CTL region via syscon, but > > > instead use the clocks and resets as exposed by the BLK_CTL driver. > > > Doing it via syscon is a hack to get things going. The clocks and > > > resets should properly be hooked up to the GPC domains via the clocks > > > and resets DT properties. > > > > > > For the clocks there is one complication: if the clocks are controlled > > > via BLK_CTL we can only enable them once the domain is powered up, > > > however the earlier designs using the GPCv2 assert resets as part of > > > the power up sequence, which needs the clocks to be running for the > > > reset to propagate. So depending on whether we have a power domain with > > > a BLK_CTL or not we need to enable the clocks before or after powering > > > up the domain. I guess we need a new DT property to specify which way > > > the domain needs to handled. > > > > So in the case of Nano, could we create two blocks instead of one? > > The first block would enable the bus clock and reset that correspond > > to writing 0x100 to avoid writing to syscon. From there, we reference > > that reset and clock from the GPC displaymix_pd to enable the access. > > Once that's done, we point the 2nd block power-domain to the > > dispmix_pd to unlock the remaining clocks. > > > > Would that work? I can try it later today, but I'm not near the hardware now. > > Splitting the PD into 2 staged domains might actually work well to get > around the cyclic dependency between GPC and BLK_CTL. It's not totally > to my liking, as the DT description doesn't map 1:1 to hardware > anymore, but it seems to be the most elegant solution to get around the > dependency. > > I'll try to implement this on the i.MX8MM today or tomorrow to see if > it holds up in reality or if there are some hidden warts. > I started looking into this yesterday, on 8MN. Splitting doesn't solve the issue. I tried splitting into dispmix and dispmix_adb400. Added the bus_blk reset and clock to the dispmix_adb400 PD. Trouble is, you can't add the dispmix_adb400 PD to the blk_ctl because of the cyclic dependency. > Regards, > Lucas >
On Thu, Oct 29, 2020 at 6:55 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford: > > On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> wrote: > > > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <marex@denx.de> wrote: > > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > Together, both the GPC and the clk-blk driver should be able to pull > > > > > > > > > the multimedia block out of reset. Currently, the GPC can handle the > > > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by > > > > > > > > > the clock block > > > > > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn node, because it > > > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon"; > > > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > > > #clock-cells = <1>; > > > > > > > > > #reset-cells = <1>; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver > > > > > > > > > since you did the 8mp clk-blk drive and they appear to be very > > > > > > > > > similar. > > > > > > > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in my tree and give it > > > > > > > > a test tomorrow morning. > > > > > > > > > > > > I do have some more updates on how to get the system to not hang, and > > > > > > to enumerate more clocks. > > > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a > > > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC. > > > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to > > > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring the display > > > > > > clocks out of reset. > > > > > > > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable unused clocks > > > > > (see clk_disable_unused) but in order to disable the clocks from the > > > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the > > > > > PD need to be on. Since you initially didn't give it any PD, it was trying > > > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both > > > > > > 32e28000 and 32e28004, and the values written for the 8MM are not > > > > > > compatible. > > > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk > > > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > > > > > I'm trying to make a branch with all the patches for all i.MX8M so I > > > > > can keep track of it all. On this branch I've also applied the > > > > > following patchset from Lucas Stach: > > > > > https://www.spinics.net/lists/arm-kernel/msg843007.html > > > > > but I'm getting the folowing errors: > > > > > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400 > > > > > > > > > > Lucas, any thoughts? > > > > > > > > > > Maybe it's something related to 8MN. > > > > > > > > > I will go back and double check this now that we have both the > > > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > > > > > Will dig further, see what pops out. > > > > > > > > I wasn't sure which direction to go with the name. I can rename the > > > > media_blk_ctl driver to display_blk_ctl. I used Media based on the > > > > imx8mp naming convention and the fact that it's controlling both the > > > > display and the camera interface, however it's depending on the > > > > dispmix GPC. > > > > > > > > I'll submit a RFC V2 with the cross referencing to the GPC based on > > > > Marek's Mini patch set, but we'll still have an issue where the Mini > > > > and Nano have different syscon values to enable the clocks, and > > > > Marek's branch has it card-coded, so my patch would effectively break > > > > the Mini in order to make the Nano operate until we find a better > > > > solution. > > > > > > The GPC should not write into the BLK_CTL region via syscon, but > > > instead use the clocks and resets as exposed by the BLK_CTL driver. > > > Doing it via syscon is a hack to get things going. The clocks and > > > resets should properly be hooked up to the GPC domains via the clocks > > > and resets DT properties. > > > > > > For the clocks there is one complication: if the clocks are controlled > > > via BLK_CTL we can only enable them once the domain is powered up, > > > however the earlier designs using the GPCv2 assert resets as part of > > > the power up sequence, which needs the clocks to be running for the > > > reset to propagate. So depending on whether we have a power domain with > > > a BLK_CTL or not we need to enable the clocks before or after powering > > > up the domain. I guess we need a new DT property to specify which way > > > the domain needs to handled. > > > > So in the case of Nano, could we create two blocks instead of one? > > The first block would enable the bus clock and reset that correspond > > to writing 0x100 to avoid writing to syscon. From there, we reference > > that reset and clock from the GPC displaymix_pd to enable the access. > > Once that's done, we point the 2nd block power-domain to the > > dispmix_pd to unlock the remaining clocks. > > > > Would that work? I can try it later today, but I'm not near the hardware now. > > Splitting the PD into 2 staged domains might actually work well to get > around the cyclic dependency between GPC and BLK_CTL. It's not totally > to my liking, as the DT description doesn't map 1:1 to hardware > anymore, but it seems to be the most elegant solution to get around the > dependency. > > I'll try to implement this on the i.MX8MM today or tomorrow to see if > it holds up in reality or if there are some hidden warts. I tried it last night on the Nano without success. :-( I was just getting ready to e-mail the group when I saw this come in. adam > > Regards, > Lucas >
Am Donnerstag, den 29.10.2020, 07:18 -0500 schrieb Adam Ford: > On Thu, Oct 29, 2020 at 6:55 AM Lucas Stach <l.stach@pengutronix.de> > wrote: > > Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford: > > > On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach < > > > l.stach@pengutronix.de> wrote: > > > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford: > > > > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa <abel.vesa@nxp.com> > > > > > wrote: > > > > > > On 20-10-25 11:05:32, Adam Ford wrote: > > > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut < > > > > > > > marex@denx.de> wrote: > > > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote: > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > Together, both the GPC and the clk-blk driver > > > > > > > > > > should be able to pull > > > > > > > > > > the multimedia block out of reset. Currently, the > > > > > > > > > > GPC can handle the > > > > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI > > > > > > > > > > appear to be gated by > > > > > > > > > > the clock block > > > > > > > > > > > > > > > > > > > > My original patch RFC didn't include the imx8mn > > > > > > > > > > node, because it > > > > > > > > > > hangs, but the node I added looks like: > > > > > > > > > > > > > > > > > > > > media_blk_ctl: clock-controller@32e28000 { > > > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", > > > > > > > > > > "syscon"; > > > > > > > > > > reg = <0x32e28000 0x1000>; > > > > > > > > > > #clock-cells = <1>; > > > > > > > > > > #reset-cells = <1>; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > I was hoping you might have some feedback on the > > > > > > > > > > 8mn clk-blk driver > > > > > > > > > > since you did the 8mp clk-blk drive and they appear > > > > > > > > > > to be very > > > > > > > > > > similar. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'll do you one better still. I'll apply the patch in > > > > > > > > > my tree and give it > > > > > > > > > a test tomorrow morning. > > > > > > > > > > > > > > I do have some more updates on how to get the system to > > > > > > > not hang, and > > > > > > > to enumerate more clocks. > > > > > > > Looking at Marek's work on enabling clocks in the 8MM, he > > > > > > > added a > > > > > > > power-domain in dispmix_blk_ctl pointing to the dispmix > > > > > > > in the GPC. > > > > > > > By forcing the GPC driver to write 0x1fff to 32e28004, > > > > > > > 0x7f to > > > > > > > 32e28000 and 0x30000 to 32e28008, the i.MX8MM can bring > > > > > > > the display > > > > > > > clocks out of reset. > > > > > > > > > > > > > > > > > > > Yeah, that makes sense. Basically, it was trying to disable > > > > > > unused clocks > > > > > > (see clk_disable_unused) but in order to disable the clocks > > > > > > from the > > > > > > media BLK_CTL (which I think should be renamed in display > > > > > > BLK_CTL) the > > > > > > PD need to be on. Since you initially didn't give it any > > > > > > PD, it was trying > > > > > > to blindly write/read the gate bit and therefore freeze. > > > > > > > > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to > > > > > > > both > > > > > > > 32e28000 and 32e28004, and the values written for the 8MM > > > > > > > are not > > > > > > > compatible. > > > > > > > By forcing the GPC to write those values, I can > > > > > > > get lcdif_pixel_clk > > > > > > > and the mipi_dsi_clkref appearing on the Nano. > > > > > > > > > > > > I'm trying to make a branch with all the patches for all > > > > > > i.MX8M so I > > > > > > can keep track of it all. On this branch I've also applied > > > > > > the > > > > > > following patchset from Lucas Stach: > > > > > > https://www.spinics.net/lists/arm-kernel/msg843007.html > > > > > > but I'm getting the folowing errors: > > > > > > > > > > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up > > > > > > ADB400 > > > > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up > > > > > > ADB400 > > > > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up > > > > > > ADB400 > > > > > > > > > > > > Lucas, any thoughts? > > > > > > > > > > > > Maybe it's something related to 8MN. > > > > > > > > > > > I will go back and double check this now that we have both > > > > > the > > > > > blt_crl->power-domain and the power-domain->blk_ctl. > > > > > > > > > > > Will dig further, see what pops out. > > > > > > > > > > I wasn't sure which direction to go with the name. I can > > > > > rename the > > > > > media_blk_ctl driver to display_blk_ctl. I used Media based > > > > > on the > > > > > imx8mp naming convention and the fact that it's controlling > > > > > both the > > > > > display and the camera interface, however it's depending on > > > > > the > > > > > dispmix GPC. > > > > > > > > > > I'll submit a RFC V2 with the cross referencing to the GPC > > > > > based on > > > > > Marek's Mini patch set, but we'll still have an issue where > > > > > the Mini > > > > > and Nano have different syscon values to enable the clocks, > > > > > and > > > > > Marek's branch has it card-coded, so my patch would > > > > > effectively break > > > > > the Mini in order to make the Nano operate until we find a > > > > > better > > > > > solution. > > > > > > > > The GPC should not write into the BLK_CTL region via syscon, > > > > but > > > > instead use the clocks and resets as exposed by the BLK_CTL > > > > driver. > > > > Doing it via syscon is a hack to get things going. The clocks > > > > and > > > > resets should properly be hooked up to the GPC domains via the > > > > clocks > > > > and resets DT properties. > > > > > > > > For the clocks there is one complication: if the clocks are > > > > controlled > > > > via BLK_CTL we can only enable them once the domain is powered > > > > up, > > > > however the earlier designs using the GPCv2 assert resets as > > > > part of > > > > the power up sequence, which needs the clocks to be running for > > > > the > > > > reset to propagate. So depending on whether we have a power > > > > domain with > > > > a BLK_CTL or not we need to enable the clocks before or after > > > > powering > > > > up the domain. I guess we need a new DT property to specify > > > > which way > > > > the domain needs to handled. > > > > > > So in the case of Nano, could we create two blocks instead of > > > one? > > > The first block would enable the bus clock and reset that > > > correspond > > > to writing 0x100 to avoid writing to syscon. From there, we > > > reference > > > that reset and clock from the GPC displaymix_pd to enable the > > > access. > > > Once that's done, we point the 2nd block power-domain to the > > > dispmix_pd to unlock the remaining clocks. > > > > > > Would that work? I can try it later today, but I'm not near the > > > hardware now. > > > > Splitting the PD into 2 staged domains might actually work well to > > get > > around the cyclic dependency between GPC and BLK_CTL. It's not > > totally > > to my liking, as the DT description doesn't map 1:1 to hardware > > anymore, but it seems to be the most elegant solution to get around > > the > > dependency. > > > > I'll try to implement this on the i.MX8MM today or tomorrow to see > > if > > it holds up in reality or if there are some hidden warts. > > I tried it last night on the Nano without success. :-( > > I was just getting ready to e-mail the group when I saw this come in. I was thinking the other way around, keeping a single BLK_CTL, but splitting the PD to reflect the power-up sequence requirement. I'll let you know how this works out. Regards, Lucas