RE: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN

[Jacky Bai]

> -----Original Message-----
> From: Abel Vesa [mailto:abel.vesa@xxxxxxx]
> Sent: Tuesday, October 27, 2020 7:55 PM
> To: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Cc: Adam Ford <aford173@xxxxxxxxx>; Marek Vasut <marex@xxxxxxx>;
> devicetree <devicetree@xxxxxxxxxxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>;
> Stephen Boyd <sboyd@xxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>;
> Michael Turquette <mturquette@xxxxxxxxxxxx>; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> dl-linux-imx <linux-imx@xxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; linux-clk
> <linux-clk@xxxxxxxxxxxxxxx>; arm-soc <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxx>
> 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&amp;data=04%
> > > >
> 7C01%7Cping.bai%40nxp.com%7C0c54623294334a04a01208d87a6f3163%7
> C686
> > > >
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637393965282215014%7C
> Unkno
> > > >
> wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> a
> > > >
> WwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=vFbBn94CPsShV72rOCtbTcz5u0
> qh7Au
> > > > o44Sb2%2BiBlrE%3D&amp;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
> > >