Hi All,
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";
>
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@xxxxxxxxxxxxxx/
Cheers,
Prabhakar
