RE: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add optional SDnH clock

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>
Subject: RE: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add optional SDnH clock
Date: Tue, 16 Nov 2021 19:41:28 +0000	[thread overview]
Message-ID: <OS0PR01MB592210D033924785A1C33A3386999@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OS0PR01MB59220069B3113A206F31305286999@OS0PR01MB5922.jpnprd01.prod.outlook.com>

Hi Geert and Wolfram,

> Subject: RE: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add optional SDnH
> clock
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add optional
> > SDnH clock
> >
> > Hi Biju,
> >
> > On Tue, Nov 16, 2021 at 12:26 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add
> > > > optional SDnH clock On Mon, Nov 15, 2021 at 9:32 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > > Subject: Re: [PATCH v3] dt-bindings: mmc: renesas,sdhi: add
> > > > > > optional SDnH clock
> > > > > >
> > > > > >
> > > > > > > > +      if:
> > > > > > > > +        properties:
> > > > > > > > +          compatible:
> > > > > > > > +            contains:
> > > > > > > > +              enum:
> > > > > > > > +                - renesas,rcar-gen2-sdhi
> > > > > > > > +                - renesas,rcar-gen3-sdhi
> > > > > > >
> > > > > > > What about RZ/G2L, as it has 4 clocks?
> > > > > >
> > > > > > They are a few lines above this in a seperate block if I am
> > > > > > not confusing the SoC numbering.
> > > > >
> > > > > Ah ok, I thought, since RZ/G2L using generic rcar-gen3-sdhi
> > > > > compatible, We need to move that Separate block inside this if
> > block.
> > > > > With in gen3 compatible, if RZG2L then Max 4 clocks else Max 3
> > > > > Clocks. I
> > > > may be completely wrong.
> > > >
> > > > But is it working?
> > > > With this series, the driver looks for the "sdh" clock, while it
> > > > is called "clk_hs" on RZ/G2L.
> > > >
> > > > As the RZ/G2L part declares compatibility with the generic
> > > > rcar-gen3-sdhi compatible, it is subject to
> > SDHI_FLAG_NEED_CLKH_FALLBACK.
> > > > In the absence of an "sdh" clock, the driver will use
> > > > clk_get_parent(clk_get_parent(priv->clk) instead.
> > > > On RZ/G2L, we have:
> > > >   SD0 -> SD0_DIV4 -> imclk
> > > >       -> clk_hs
> > > > So that will pick up SD0, which might be correct, accidentally ;-)
> > > > As quirks is not set, it will use clkh_shift = 2.
> > > >
> > > > So all is good? I think we still want the driver to check for
> > > > "clk_hs", too, to avoid having to depend on the fallback.
> > >
> > > I have added below piece of code and tested clk_hs. It works ok.
> > >
> > > Can we change the binding to update to use "clkh" instead of "clk_hs"
> > > for RZ/G2L?, so that we don't need below code change in driver. Any
> > > way
> > it is optional.
> >
> > Fine for me.
> > Should we also rename imclk2 to cd?
> 
> Yes, we can rename imclk2 to cd.

Please let me know, if you want me to do this changes as separate patch(binding + dtsi) or
Will you take care this? Both are ok to me.

Regards,
Biju

> 
> >
> > Note that on RZ/G2L, it will be handled by Runtime PM regardless.
> > > +
> > > +       if(!priv->clkh) {
> > > +               priv->clkh = devm_clk_get_optional(&pdev->dev,
> > "clk_hs");
> > > +               if (IS_ERR(priv->clkh))
> > > +                       return dev_err_probe(&pdev->dev,
> > > + PTR_ERR(priv->clkh), "cannot get clk_hs");
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> > geert@linux- m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker.
> > But when I'm talking to journalists I just say "programmer" or
> > something like that.
> >                                 -- Linus Torvalds