linux-kernel - Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 28 Jun 2024 15:17:12 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	Biju Das <biju.das.jz@...renesas.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, linux-mmc@...r.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, linux-kernel@...r.kernel.org, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>, linux-renesas-soc@...r.kernel.org, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, devicetree@...r.kernel.org, 
	Conor Dooley <conor+dt@...nel.org>, Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

Hi All,

On Fri, Jun 21, 2024 at 12:58 PM Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
>
> Hi all,
>
> On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
> <wsa+renesas@...g-engineering.com> wrote:
> > > Based on the feedback from Rob I have now changed it to below, i.e.
> > > the regulator now probes based on regulator-compatible property value
> > > "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> > > driver has of_match in regulator_desc.
> >
> > I like this a lot! One minor comment.
> >
> > > static struct regulator_desc r9a09g057_vqmmc_regulator = {
> > >     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
> > >     .owner        = THIS_MODULE,
> > >     .type        = REGULATOR_VOLTAGE,
> > >     .ops        = &r9a09g057_regulator_voltage_ops,
> > >     .volt_table    = r9a09g057_vqmmc_voltages,
> > >     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> > > };
> > >
> > > SoC DTSI:
> > >         sdhi1: mmc@...10000 {
> > >             compatible = "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 167>,
> > >                  <&cpg CPG_MOD 169>,
> > >                  <&cpg CPG_MOD 168>,
> > >                  <&cpg CPG_MOD 170>;
> > >             clock-names = "core", "clkh", "cd", "aclk";
> > >             resets = <&cpg 168>;
> > >             power-domains = <&cpg>;
> > >             status = "disabled";
> > >
> > >             vqmmc_sdhi1: vqmmc-regulator {
> > >                 regulator-compatible = "vqmmc-r9a09g057-regulator";
>
> renesas,r9a09g057-vqmmc-regulator?
>
> > >                 regulator-name = "vqmmc-regulator";
> >
> > This should have "sdhi<X>" somewhere in the name?
>
> Indeed.
>
> >
> > >                 regulator-min-microvolt = <1800000>;
> > >                 regulator-max-microvolt = <3300000>;
> > >                 status = "disabled";
> > >             };
> > >         };
> > >
> > > Board DTS:
> > >
> > > &sdhi1 {
> > >     pinctrl-0 = <&sdhi1_pins>;
> > >     pinctrl-1 = <&sdhi1_pins>;
> > >     pinctrl-names = "default", "state_uhs";
> > >     vmmc-supply = <&reg_3p3v>;
> > >     vqmmc-supply = <&vqmmc_sdhi1>;
> > >     bus-width = <4>;
> > >     sd-uhs-sdr50;
> > >     sd-uhs-sdr104;
> > >     status = "okay";
> > > };
> > >
> > > &vqmmc_sdhi1 {
> > >     status = "okay";
> > > };
> >
> > Again, I like this. It looks like proper HW description to me.
> >
> > > Based on the feedback provided Geert ie to use set_pwr callback to set
> > > PWEN bit and handle IOVS bit in voltage switch callback by dropping
> > > the regulator altogether. In this case we will have to introduce just
> > > a single "use-internal-regulator" property and if set make the vqmmc
> > > regulator optional?
> >
> > Let's discuss with Geert. But I am quite convinced of your approach
> > above.
> >
> > > > > Let me know if I have missed something obvious here.
> > > >
> > > > Nope, all good.
> >
> > Don't give up, I think we are close...
>
> The above indeed starts looking good to me.
> IIUIC, the use of the special vqmmc-r9a09g057-regulator is still
> optional, and the subnode can be left disabled? E.g. the board
> designer may still use a (different) GPIO to control the regulator,
> using "regulator-gpio"?
>
> Which brings me to another question: as both the SDmIOVS and SDmPWEN
> pins can be configured as GPIOs, why not ignore the special handling
> using the SDm_SD_STATUS register, and use "regulator-gpio" instead?
> We usually do the same for CD/WP, using "{cd,wp}-gpios" instead.
> Exceptions are old SH/R-Mobile and R-Car Gen1 boards:
>
>   arch/arm/boot/dts/renesas/r8a73a4-ape6evm.dts:          groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/r8a7740-armadillo800eva.dts:
> groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7778-bockw.dts:            groups =
> "sdhi0_cd", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7779-marzen.dts:           groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/sh73a0-kzm9g.dts:             groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";
>
Based on the special handling required to handle the IOVS and PWEN pin
by bypassing the core regulator by function pointers in v4 [0] doesn't
seem an elegant solution.

On the RZ/V2H SoC IOVS and PWEN pins can be used as GPIO and a similar
approach has been used on the other Renesas SoCs. I will withhold
internal regulator support for RZ/V2H SoC and fallback to GPIOs.

[0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240626132341.342963-4-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar