| 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
| ||
|
Message-ID: <CA+V-a8uepnPCgbEtsxL+hMcGcS-X4hEXZsd7Q2+fpqMyF=cCGA@mail.gmail.com>
Date: Fri, 28 Jun 2024 15:17:12 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Wolfram Sang <wsa+renesas@...g-engineering.com>,
Biju Das <biju.das.jz@...renesas.com>
Cc: 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>,
Magnus Damm <magnus.damm@...il.com>, devicetree@...r.kernel.org,
Conor Dooley <conor+dt@...nel.org>, Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
Hi All,
On Fri, Jun 21, 2024 at 12:58 PM Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
>
> Hi all,
>
> On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
> <wsa+renesas@...g-engineering.com> 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@...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";
>
> 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@bp.renesas.com/
Cheers,
Prabhakar
Powered by blists - more mailing lists