| Message ID | 20250126134616.37334-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Add RZ/G3E SDHI support | expand |
On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@bp.renesas.com> wrote: > Arrange local variables in reverse xmas tree for probe(). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
Hi Biju, On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@bp.renesas.com> wrote: > The SDHI/eMMC IPs in the RZ/G3E SoC are similar to those in R-Car Gen3. > However, the RZ/G3E SD0 channel has Voltage level control and PWEN pin > support via SD_STATUS register. > > internal regulator support is added to control the voltage levels of > the SD pins via sd_iovs/sd_pwen bits in SD_STATUS register by populating > vqmmc-regulator child node. > > SD1 and SD2 channels have gpio regulator support and internal regulator > support. Selection of the regulator is based on the regulator phandle. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -1053,6 +1165,23 @@ int renesas_sdhi_probe(struct platform_device *pdev, > if (ret) > goto efree; > > + rcfg.of_node = of_get_child_by_name(dev->of_node, "vqmmc-regulator"); If this node becomes required on RZ/V2H and RZ/G3E, and controlled through status, you also need: if (!of_device_is_available(rcfg.of_node)) { of_node_put(rcfg.of_node); rcfg.of_node = NULL; } Or introduce of_get_available_child_by_name()... > + if (rcfg.of_node) { > + rcfg.driver_data = priv->host; > + > + renesas_sdhi_vqmmc_regulator.name = "sdhi-vqmmc-regulator"; Name conflict in case of multiple instances? > + renesas_sdhi_vqmmc_regulator.of_match = of_match_ptr("vqmmc-regulator"); > + renesas_sdhi_vqmmc_regulator.type = REGULATOR_VOLTAGE; > + renesas_sdhi_vqmmc_regulator.owner = THIS_MODULE; > + rdev = devm_regulator_register(dev, &renesas_sdhi_vqmmc_regulator, &rcfg); > + of_node_put(rcfg.of_node); > + if (IS_ERR(rdev)) { > + dev_err(dev, "regulator register failed err=%ld", PTR_ERR(rdev)); > + goto efree; > + } > + priv->rdev = rdev; > + } > + > ver = sd_ctrl_read16(host, CTL_VERSION); > /* GEN2_SDR104 is first known SDHI to use 32bit block count */ > if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX) Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 28 January 2025 13:32 > Subject: Re: [PATCH 4/7] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > Hi Biju, > > On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > The SDHI/eMMC IPs in the RZ/G3E SoC are similar to those in R-Car Gen3. > > However, the RZ/G3E SD0 channel has Voltage level control and PWEN pin > > support via SD_STATUS register. > > > > internal regulator support is added to control the voltage levels of > > the SD pins via sd_iovs/sd_pwen bits in SD_STATUS register by > > populating vqmmc-regulator child node. > > > > SD1 and SD2 channels have gpio regulator support and internal > > regulator support. Selection of the regulator is based on the regulator phandle. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > @@ -1053,6 +1165,23 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > if (ret) > > goto efree; > > > > + rcfg.of_node = of_get_child_by_name(dev->of_node, > > + "vqmmc-regulator"); > > If this node becomes required on RZ/V2H and RZ/G3E, and controlled through status, you also need: > > if (!of_device_is_available(rcfg.of_node)) { > of_node_put(rcfg.of_node); > rcfg.of_node = NULL; > } OK. > > Or introduce of_get_available_child_by_name()... OK, will send a separate patch for optimizing the above 2 calls. > > > + if (rcfg.of_node) { > > + rcfg.driver_data = priv->host; > > + > > + renesas_sdhi_vqmmc_regulator.name = > > + "sdhi-vqmmc-regulator"; > > Name conflict in case of multiple instances? This regulator name is Overriden by of_regulator() and it will pick the name from there. See below. Am I missing anything? root@smarc-rzg3e:/cip-test-scripts# cat /sys/class/regulator/regulator.*/name regulator-dummy fixed-3.3V fixed-3.3V SDHI1 VccQ SDHI0-VQMMC SDHI2-VQMMC SDHI1-VQMMC Cheers, Biju
Hi Geert, > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: 28 January 2025 13:32 > > Subject: Re: [PATCH 4/7] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > > > Hi Biju, > > > > On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > The SDHI/eMMC IPs in the RZ/G3E SoC are similar to those in R-Car Gen3. > > > However, the RZ/G3E SD0 channel has Voltage level control and PWEN > > > pin support via SD_STATUS register. > > > > > > internal regulator support is added to control the voltage levels of > > > the SD pins via sd_iovs/sd_pwen bits in SD_STATUS register by > > > populating vqmmc-regulator child node. > > > > > > SD1 and SD2 channels have gpio regulator support and internal > > > regulator support. Selection of the regulator is based on the regulator phandle. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > > > @@ -1053,6 +1165,23 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > > if (ret) > > > goto efree; > > > > > > > > + if (rcfg.of_node) { > > > + rcfg.driver_data = priv->host; > > > + > > > + renesas_sdhi_vqmmc_regulator.name = > > > + "sdhi-vqmmc-regulator"; > > > > Name conflict in case of multiple instances? > > This regulator name is Overriden by of_regulator() and it will pick the name from there. > See below. Am I missing anything? Just to add, without the name it returns error see [1], Regulator name from DT node is picked by [2] and [3]. So there won't be conflict for Multiple instances. [1] https://elixir.bootlin.com/linux/v6.13-rc7/source/drivers/regulator/core.c#L5597 [2] https://elixir.bootlin.com/linux/v6.13-rc7/source/drivers/regulator/core.c#L5659 [3] https://elixir.bootlin.com/linux/v6.13-rc7/source/drivers/regulator/of_regulator.c#L97 Cheers, Biju > > root@smarc-rzg3e:/cip-test-scripts# cat /sys/class/regulator/regulator.*/name > regulator-dummy > fixed-3.3V > fixed-3.3V > SDHI1 VccQ > SDHI0-VQMMC > SDHI2-VQMMC > SDHI1-VQMMC > > Cheers, > Biju
Hi Biju, On Wed, 29 Jan 2025 at 15:45, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Sent: 28 January 2025 13:32 > > > Subject: Re: [PATCH 4/7] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > > > > > On Sun, 26 Jan 2025 at 14:46, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > The SDHI/eMMC IPs in the RZ/G3E SoC are similar to those in R-Car Gen3. > > > > However, the RZ/G3E SD0 channel has Voltage level control and PWEN > > > > pin support via SD_STATUS register. > > > > > > > > internal regulator support is added to control the voltage levels of > > > > the SD pins via sd_iovs/sd_pwen bits in SD_STATUS register by > > > > populating vqmmc-regulator child node. > > > > > > > > SD1 and SD2 channels have gpio regulator support and internal > > > > regulator support. Selection of the regulator is based on the regulator phandle. > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > > > > > @@ -1053,6 +1165,23 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > > > if (ret) > > > > goto efree; > > > > > > > > > > > + if (rcfg.of_node) { > > > > + rcfg.driver_data = priv->host; > > > > + > > > > + renesas_sdhi_vqmmc_regulator.name = > > > > + "sdhi-vqmmc-regulator"; > > > > > > Name conflict in case of multiple instances? > > > > This regulator name is Overriden by of_regulator() and it will pick the name from there. > > See below. Am I missing anything? > > Just to add, without the name it returns error see [1], Thanks, I had arrived at the same conclusion. Gr{oetje,eeting}s, Geert
The SD/MMC block on the RZ/G3E ("R9A09G047") SoC is similar to that of the RZ/V2H, but the SD0 channel has only dedicated pins, so we must use SD_STATUS register to control voltage and power enable (internal regulator). For SD1 and SD2 channel we can either use gpio regulator or internal regulator (using SD_STATUS register) for voltage switching. Biju Das (7): dt-bindings: mmc: renesas,sdhi: Document RZ/G3E support clk: renesas: r9a09g047: Add SDHI clocks/resets mmc: renesas_sdhi: Arrange local variables in reverse xmas tree order mmc: renesas_sdhi: Add support for RZ/G3E SoC arm64: dts: renesas: r9a09g047: Add SDHI0-SDHI2 nodes arm64: dts: renesas: rzg3e-smarc-som: Enable SDHI{0,2} arm64: dts: renesas: r9a09g047e57-smarc: Enable SDHI1 .../devicetree/bindings/mmc/renesas,sdhi.yaml | 20 +++ arch/arm64/boot/dts/renesas/r9a09g047.dtsi | 57 ++++++++ .../boot/dts/renesas/r9a09g047e57-smarc.dts | 65 +++++++++ .../boot/dts/renesas/renesas-smarc2.dtsi | 9 ++ .../boot/dts/renesas/rzg3e-smarc-som.dtsi | 89 ++++++++++++ drivers/clk/renesas/r9a09g047-cpg.c | 31 +++++ drivers/mmc/host/renesas_sdhi.h | 1 + drivers/mmc/host/renesas_sdhi_core.c | 131 +++++++++++++++++- drivers/mmc/host/tmio_mmc.h | 5 + 9 files changed, 407 insertions(+), 1 deletion(-)