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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 18:15:44 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	Geert Uytterhoeven <geert+renesas@...der.be>
Cc: "Lad, Prabhakar" <prabhakar.csengg@...il.com>, 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>, 
	Biju Das <biju.das.jz@...renesas.com>, Magnus Damm <magnus.damm@...il.com>, 
	devicetree@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>
Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

Hi Wolfram,

On Thu, Jun 20, 2024 at 8:39 AM Wolfram Sang
<wsa+renesas@...g-engineering.com> 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@...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";
                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