Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 28 January 2025 13:26
> Subject: Re: [PATCH 5/7] arm64: dts: renesas: r9a09g047: Add SDHI0-SDHI2 nodes
>
> Hi Biju,
>
> On Tue, 28 Jan 2025 at 13:11, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > Sent: 28 January 2025 11:33
> > Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>;
> > > biju.das.au <biju.das.au@xxxxxxxxx>
> > > Subject: Re: [PATCH 5/7] arm64: dts: renesas: r9a09g047: Add
> > > SDHI0-SDHI2 nodes
> > >
> > > On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > Add SDHI0-SDHI2 nodes to RZ/G3E ("R9A09G047") SoC DTSI.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/arch/arm64/boot/dts/renesas/r9a09g047.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r9a09g047.dtsi
> > > > @@ -518,6 +518,63 @@ gic: interrupt-controller@14900000 {
> > > > interrupt-controller;
> > > > interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> > > > };
> > > > +
> > > > + sdhi0: mmc@15c00000 {
> > > > + compatible = "renesas,sdhi-r9a09g047", "renesas,sdhi-r9a09g057";
> > > > + reg = <0x0 0x15c00000 0 0x10000>;
> > > > + interrupts = <GIC_SPI 735 IRQ_TYPE_LEVEL_HIGH>,
> > > > + <GIC_SPI 736 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&cpg CPG_MOD 0xa3>, <&cpg CPG_MOD 0xa5>,
> > > > + <&cpg CPG_MOD 0xa4>, <&cpg CPG_MOD 0xa6>;
> > > > + clock-names = "core", "clkh", "cd", "aclk";
> > > > + resets = <&cpg 0xa7>;
> > > > + power-domains = <&cpg>;
> > > > + status = "disabled";
> > > > +
> > > > + vqmmc_sdhi0: vqmmc-regulator {
> > > > + regulator-name = "SDHI0-VQMMC";
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + };
> > > > + };
> > > > +
> > > > + sdhi1: mmc@15c10000 {
> > > > + compatible = "renesas,sdhi-r9a09g047", "renesas,sdhi-r9a09g057";
> > > > + reg = <0x0 0x15c10000 0 0x10000>;
> > > > + interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
> > > > + <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&cpg CPG_MOD 0xa7>, <&cpg CPG_MOD 0xa9>,
> > > > + <&cpg CPG_MOD 0xa8>, <&cpg CPG_MOD 0xaa>;
> > > > + clock-names = "core", "clkh", "cd", "aclk";
> > > > + resets = <&cpg 0xa8>;
> > > > + power-domains = <&cpg>;
> > > > + status = "disabled";
> > > > +
> > > > + vqmmc_sdhi1: vqmmc-regulator {
> > > > + regulator-name = "SDHI1-VQMMC";
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + };
> > > > + };
> > > > +
> > > > + sdhi2: mmc@15c20000 {
> > > > + compatible = "renesas,sdhi-r9a09g047", "renesas,sdhi-r9a09g057";
> > > > + reg = <0x0 0x15c20000 0 0x10000>;
> > > > + interrupts = <GIC_SPI 739 IRQ_TYPE_LEVEL_HIGH>,
> > > > + <GIC_SPI 740 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&cpg CPG_MOD 0xab>, <&cpg CPG_MOD 0xad>,
> > > > + <&cpg CPG_MOD 0xac>, <&cpg CPG_MOD 0xae>;
> > > > + clock-names = "core", "clkh", "cd", "aclk";
> > > > + resets = <&cpg 0xa9>;
> > > > + power-domains = <&cpg>;
> > > > + status = "disabled";
> > > > +
> > > > + vqmmc_sdhi2: vqmmc-regulator {
> > > > + regulator-name = "SDHI2-VQMMC";
> > > > + regulator-min-microvolt = <1800000>;
> > > > + regulator-max-microvolt = <3300000>;
> > > > + };
> > > > + };
> > > > };
> > > >
> > > > timer {
> > >
> > > Shouldn't the vqmmc-regulator subnodes be added in the board DTS,
> > > when needed (i.e. at least for SDHI[12])? Or do you expect the board DTS to /delete-node/ them
> when they are not needed?
> >
> > I agree.
> >
> > I have provided an example in next patch using /delete-node/ to use gpio-regulator.
>
> Ah, my fault trying to get my reviews out sooner rather than later ;-)
>
> > I am ok for moving it to the board DTS as well. When I sent patch, I
> > am not sure which is the best in terms of user point of view?
> >
> > Now I got the answer to move vqmmc-regulator subnodes to add in the
> > board DTS for atleast SDHI[12]. I will address this in next version.
> >
> > Even for SDHI0 fix type, if we plan to use fixed regulator for eMMC?
> >
> > >
> > > Is it possible that SDHI0 does not need the regulator control, e.g.
> > > in case of a fixed voltage?
> >
> > Yes, for eMMC(fixed case) it is not needed.
>
> Upon second thought: as the internal regulator is always present, what about setting its status to
> disabled in the SoC .dtsi, and changing it to okay in the board DTS when needed, like done for other
> components?
Agreed. Apart from that, I am planning the below changes for V2:
SD0 fixed voltage(eMMC): fixed regulator based on SoM Schematics.
SD2 : Internal regulator.
There will be a new patch for supporting SD0 non fixed voltage(SD0) using
internal regulator.
Cheers,
Biju
