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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi all,

On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
<wsa+renesas@sang-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@15c10000 {
> >             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";

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