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: Fri, 21 Jun 2024 09:54:25 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>, 
	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 Prabhakar,

> 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@...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";

This should have "sdhi<X>" somewhere in the name?

>                 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...

All the best,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)