Hi Geert,
On Fri, Jun 21, 2024 at 12:58 PM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi all,
>
> On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
> <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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 = <®_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";
>
Indeed this can be done on RZ/V2H(P) SoC, the future upcoming SoCs may
not have an option for this In this case we will have to use the
internal regulator.
Let me know your thoughts on what approach we take for RZ/V2H(P) SoC.
Cheers,
Prabhakar
