| Message ID | 20250206134047.67866-4-biju.das.jz@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add RZ/G3E SDHI support | expand |
> + /* > + * HW reset might have toggled the regulator state in > + * HW which regulator core might be unaware of so save > + * and restore the regulator state during HW reset. > + */ Since this is a hard reset, can't we just reset the regulator to an initial state? It seems strange to preserve a value when the 'preserve' flag is explicitly not set.
Hi Wolfram, Thanks for the feedback. > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: 27 February 2025 15:16 > Subject: Re: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > > > + /* > > + * HW reset might have toggled the regulator state in > > + * HW which regulator core might be unaware of so save > > + * and restore the regulator state during HW reset. > > + */ > > Since this is a hard reset, can't we just reset the regulator to an initial state? It seems strange to > preserve a value when the 'preserve' flag is explicitly not set. Assume, this happens after the card is switched to UHS state and then the command won't work in UHS state if we bring the regulator initial state of 3.3V , In external regulator case, we don't toggle the regulator to initial state of 3.3V. That is the reason it is still working even when hard reset is applied in UHS state. Am I missing anything please let me know? I can debug further. Cheers, Biju
Hi Wolfram, > -----Original Message----- > From: Biju Das <biju.das.jz@bp.renesas.com> > Sent: 27 February 2025 15:30 > Subject: RE: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > Hi Wolfram, > > Thanks for the feedback. > > > -----Original Message----- > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Sent: 27 February 2025 15:16 > > Subject: Re: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E > > SoC > > > > > > > + /* > > > + * HW reset might have toggled the regulator state in > > > + * HW which regulator core might be unaware of so save > > > + * and restore the regulator state during HW reset. > > > + */ > > > > Since this is a hard reset, can't we just reset the regulator to an > > initial state? It seems strange to preserve a value when the 'preserve' flag is explicitly not set. > > Assume, this happens after the card is switched to UHS state and then the command won't work in UHS > state if we bring the regulator initial state of 3.3V , > > In external regulator case, we don't toggle the regulator to initial state of 3.3V. > That is the reason it is still working even when hard reset is applied in UHS state. > > Am I missing anything please let me know? I can debug further. Previously I got an issue, where the card was unable to detect after hot removal and plug. Now it is working without this change. Not sure the previous issue related to delay associated with hard reset. I am doing more testing without this change. Cheers, Biju
Hi Wolfram, > -----Original Message----- > From: Biju Das > Sent: 27 February 2025 16:27 > Subject: RE: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > Hi Wolfram, > > > -----Original Message----- > > From: Biju Das <biju.das.jz@bp.renesas.com> > > Sent: 27 February 2025 15:30 > > Subject: RE: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E > > SoC > > > > Hi Wolfram, > > > > Thanks for the feedback. > > > > > -----Original Message----- > > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > Sent: 27 February 2025 15:16 > > > Subject: Re: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for > > > RZ/G3E SoC > > > > > > > > > > + /* > > > > + * HW reset might have toggled the regulator state in > > > > + * HW which regulator core might be unaware of so save > > > > + * and restore the regulator state during HW reset. > > > > + */ > > > > > > Since this is a hard reset, can't we just reset the regulator to an > > > initial state? It seems strange to preserve a value when the 'preserve' flag is explicitly not > set. > > > > Assume, this happens after the card is switched to UHS state and then > > the command won't work in UHS state if we bring the regulator initial > > state of 3.3V , > > > > In external regulator case, we don't toggle the regulator to initial state of 3.3V. > > That is the reason it is still working even when hard reset is applied in UHS state. > > > > Am I missing anything please let me know? I can debug further. > > Previously I got an issue, where the card was unable to detect after hot removal and plug. > Now it is working without this change. > > Not sure the previous issue related to delay associated with hard reset. > I am doing more testing without this change. I have added some debug code[1], with SDHI1 using internal regulator as it is easy to do insert/removal of SD cards: Without the regulator register restore code hot removal/plug: ------------------------------------------------------------- root@smarc-rzg3e:~# [ 31.400277] mmc1: card aaaa removed [ 31.450751] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 595 sd_status before reset=10000 [ 31.460812] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 607 sd_status after reset=1 [ 35.811808] SDHI1-VQMMC: disabling [ 36.526838] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 595 sd_status before reset=0 [ 36.536642] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 607 sd_status after reset=1 [ 39.322682] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 607 sd_status after reset=1 [ 39.361826] mmc1: Skipping voltage switch [ 39.610915] mmc1: new high speed SDHC card at address aaaa [ 39.618237] mmcblk1: mmc1:aaaa SE32G 29.7 GiB [ 39.629668] mmcblk1: p1 With the regulator register restore code hot removal/plug: -------------------------------------------------------- root@smarc-rzg3e:~# [ 23.886933] mmc1: card aaaa removed [ 23.937416] renesas_sdhi_internal_dmac 15c10000.mmc: #####renesas_sdhi_reset 595 sd_status before reset=10000 [ 25.581984] mmc1: new UHS-I speed SDR104 SDHC card at address aaaa [ 25.589989] mmcblk1: mmc1:aaaa SE32G 29.7 GiB [ 25.600507] mmcblk1: p1 root@smarc-rzg3e:~# Please share your thoughts. [1] + dev_err(&host->pdev->dev,"#####%s %d sd_status before reset=%x",__func__,__LINE__, sd_status); reset_control_reset(priv->rstc); /* Unknown why but without polling reset status, it will hang */ read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100, @@ -599,8 +602,13 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve) /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); if (priv->rdev) - sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + + dev_err(&host->pdev->dev,"#####%s %d sd_status after reset=%x",__func__,__LINE__, sd_status); Cheers, Biju
Hi Wolfram, > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: 27 February 2025 15:16 > Subject: Re: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > > > + /* > > + * HW reset might have toggled the regulator state in > > + * HW which regulator core might be unaware of so save > > + * and restore the regulator state during HW reset. > > + */ > > Since this is a hard reset, can't we just reset the regulator to an initial state? It seems strange to > preserve a value when the 'preserve' flag is explicitly not set. For internal regulator case this also works OK. In this case, we don't need to preserve the value. This avoids changing internal vqmmc regulator from 1.8V to 3.3V during UHS state. Currently hardware reset causes vqmmc regulator to change from 1.8V to 3.3V. - if (!preserve) { + if (!preserve && !priv->rdev) { Please share your thoughts. Cheers, Biju
> - if (!preserve) { > + if (!preserve && !priv->rdev) { '!preserve' basically means a hard reset is requested. We should then try hard to actually do a hard reset and not only a soft reset. So, this approach is the wrong path IMHO.
On Thu, Feb 06, 2025 at 01:40:27PM +0000, Biju Das 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. > Similar case for SD0 fixed voltage (eMMC) that uses fixed regulator and > SD0 non-fixed voltage (SD0) that uses internal regulator. Okay, since I don't see a constant state of the regulator, let's just restore the original value as you do here. I mean it works. > + if (priv->rdev) > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); What about introducing sd_ctrl_read32? Or ask the engineers to move SD_STATUS_IOVS to bit 15 ;)
Hi Wolfram Sang, Thanks for the feedback. > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: 28 February 2025 09:42 > Subject: Re: [PATCH v3 3/8] mmc: renesas_sdhi: Add support for RZ/G3E SoC > > On Thu, Feb 06, 2025 at 01:40:27PM +0000, Biju Das 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. > > Similar case for SD0 fixed voltage (eMMC) that uses fixed regulator > > and > > SD0 non-fixed voltage (SD0) that uses internal regulator. > > Okay, since I don't see a constant state of the regulator, let's just restore the original value as > you do here. I mean it works. OK. > > > + if (priv->rdev) > > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > > What about introducing sd_ctrl_read32? Agreed, will introduce sd_ctrl_read32(). Cheers, Biju
diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index f12a87442338..291ddb4ad9be 100644 --- a/drivers/mmc/host/renesas_sdhi.h +++ b/drivers/mmc/host/renesas_sdhi.h @@ -95,6 +95,7 @@ struct renesas_sdhi { struct reset_control *rstc; struct tmio_mmc_host *host; + struct regulator_dev *rdev; }; #define host_to_priv(host) \ diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 6ea651409774..99700d89aa8c 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -32,6 +32,8 @@ #include <linux/platform_device.h> #include <linux/pm_domain.h> #include <linux/regulator/consumer.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.h> #include <linux/reset.h> #include <linux/sh_dma.h> #include <linux/slab.h> @@ -581,12 +583,24 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve) if (!preserve) { if (priv->rstc) { + u32 sd_status; + /* + * HW reset might have toggled the regulator state in + * HW which regulator core might be unaware of so save + * and restore the regulator state during HW reset. + */ + if (priv->rdev) + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + reset_control_reset(priv->rstc); /* Unknown why but without polling reset status, it will hang */ read_poll_timeout(reset_control_status, ret, ret == 0, 1, 100, false, priv->rstc); /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); + if (priv->rdev) + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); + priv->needs_adjust_hs400 = false; renesas_sdhi_set_clock(host, host->clk_cache); @@ -904,15 +918,113 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable) renesas_sdhi_sdbuf_width(host, enable ? width : 16); } +static const unsigned int renesas_sdhi_vqmmc_voltages[] = { + 3300000, 1800000 +}; + +static int renesas_sdhi_regulator_disable(struct regulator_dev *rdev) +{ + struct tmio_mmc_host *host = rdev_get_drvdata(rdev); + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + sd_status &= ~SD_STATUS_PWEN; + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); + + return 0; +} + +static int renesas_sdhi_regulator_enable(struct regulator_dev *rdev) +{ + struct tmio_mmc_host *host = rdev_get_drvdata(rdev); + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + sd_status |= SD_STATUS_PWEN; + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); + + return 0; +} + +static int renesas_sdhi_regulator_is_enabled(struct regulator_dev *rdev) +{ + struct tmio_mmc_host *host = rdev_get_drvdata(rdev); + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + + return (sd_status & SD_STATUS_PWEN) ? 1 : 0; +} + +static int renesas_sdhi_regulator_get_voltage(struct regulator_dev *rdev) +{ + struct tmio_mmc_host *host = rdev_get_drvdata(rdev); + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + + return (sd_status & SD_STATUS_IOVS) ? 1800000 : 3300000; +} + +static int renesas_sdhi_regulator_set_voltage(struct regulator_dev *rdev, + int min_uV, int max_uV, + unsigned int *selector) +{ + struct tmio_mmc_host *host = rdev_get_drvdata(rdev); + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + if (min_uV >= 1700000 && max_uV <= 1950000) { + sd_status |= SD_STATUS_IOVS; + *selector = 1; + } else { + sd_status &= ~SD_STATUS_IOVS; + *selector = 0; + } + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); + + return 0; +} + +static int renesas_sdhi_regulator_list_voltage(struct regulator_dev *rdev, + unsigned int selector) +{ + if (selector >= ARRAY_SIZE(renesas_sdhi_vqmmc_voltages)) + return -EINVAL; + + return renesas_sdhi_vqmmc_voltages[selector]; +} + +static const struct regulator_ops renesas_sdhi_regulator_voltage_ops = { + .enable = renesas_sdhi_regulator_enable, + .disable = renesas_sdhi_regulator_disable, + .is_enabled = renesas_sdhi_regulator_is_enabled, + .list_voltage = renesas_sdhi_regulator_list_voltage, + .get_voltage = renesas_sdhi_regulator_get_voltage, + .set_voltage = renesas_sdhi_regulator_set_voltage, +}; + +static struct regulator_desc renesas_sdhi_vqmmc_regulator = { + .of_match = of_match_ptr("vqmmc-regulator"), + .owner = THIS_MODULE, + .type = REGULATOR_VOLTAGE, + .ops = &renesas_sdhi_regulator_voltage_ops, + .volt_table = renesas_sdhi_vqmmc_voltages, + .n_voltages = ARRAY_SIZE(renesas_sdhi_vqmmc_voltages), +}; + int renesas_sdhi_probe(struct platform_device *pdev, const struct tmio_mmc_dma_ops *dma_ops, const struct renesas_sdhi_of_data *of_data, const struct renesas_sdhi_quirks *quirks) { + struct regulator_config rcfg = { .dev = &pdev->dev, }; struct tmio_mmc_data *mmd = pdev->dev.platform_data; struct renesas_sdhi_dma *dma_priv; + struct device *dev = &pdev->dev; struct tmio_mmc_data *mmc_data; struct tmio_mmc_host *host; + struct regulator_dev *rdev; struct renesas_sdhi *priv; int num_irqs, irq, ret, i; struct resource *res; @@ -1053,6 +1165,28 @@ 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 (!of_device_is_available(rcfg.of_node)) { + of_node_put(rcfg.of_node); + rcfg.of_node = NULL; + } + + if (rcfg.of_node) { + rcfg.driver_data = priv->host; + + renesas_sdhi_vqmmc_regulator.name = "sdhi-vqmmc-regulator"; + 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) diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index a75755f31d31..5970ca598850 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -44,6 +44,7 @@ #define CTL_RESET_SD 0xe0 #define CTL_VERSION 0xe2 #define CTL_SDIF_MODE 0xe6 /* only known on R-Car 2+ */ +#define CTL_SD_STATUS 0xf2 /* only known on RZ/{G2L,G3E,V2H} */ /* Definitions for values the CTL_STOP_INTERNAL_ACTION register can take */ #define TMIO_STOP_STP BIT(0) @@ -103,6 +104,10 @@ /* Definitions for values the CTL_SDIF_MODE register can take */ #define SDIF_MODE_HS400 BIT(0) /* only known on R-Car 2+ */ +/* Definitions for values the CTL_SD_STATUS register can take */ +#define SD_STATUS_PWEN BIT(0) /* only known on RZ/{G3E,V2H} */ +#define SD_STATUS_IOVS BIT(16) /* only known on RZ/{G3E,V2H} */ + /* Define some IRQ masks */ /* This is the mask used at reset by the chip */ #define TMIO_MASK_ALL 0x837f031d
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. Similar case for SD0 fixed voltage (eMMC) that uses fixed regulator and SD0 non-fixed voltage (SD0) that uses internal regulator. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * No change. v1->v2: * Updated commit description for regulator used in SD0 fixed and non-fixed voltage case. * As the node enabling of internal regulator is controlled through status, added a check for device availability. --- drivers/mmc/host/renesas_sdhi.h | 1 + drivers/mmc/host/renesas_sdhi_core.c | 134 +++++++++++++++++++++++++++ drivers/mmc/host/tmio_mmc.h | 5 + 3 files changed, 140 insertions(+)