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

From: Lad, Prabhakar
Date: Thu Jun 20 2024 - 13:16:52 EST


Hi Wolfram,

On Thu, Jun 20, 2024 at 8:39 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> > I did give it a try with platform_driver_probe() and failed.
>
> Ok, thanks for trying nonetheless!
>
> > - Firstly I had to move the regulator node outside the SDHI node for
> > platform_driver_probe() to succeed or else it failed with -ENODEV (at
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953)
>
> This makes sense to me because it is just a "regular" regulator.
>
OK.

> > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > being for each instance we are calling platform_driver_probe(). Which
> > causes a problem as the regulator node will use the first device.
>
> I see... we would need a reg property to differentiate between the
> internal regulators but that is already used by the parent SDHI node.
>
> Okay, so let's scrap that idea. However, we need to ensure that we can
> still have an external regulator. Seeing the bindings, it looks like you
> enable the internal regulator with the "vqmmc-r9a09g057-regulator"
> property? I wonder now if we can simplify this to an
> "use-internal-regulator" property because we have 'compatible' already
> to differentiate? Needs advice from DT maintainers, probably.
>

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.

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";
regulator-name = "vqmmc-regulator";
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";
};

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 me know your thoughts.

> > Let me know if I have missed something obvious here.
>
> Nope, all good.
>
sigh..

Cheers,
Prabhakar