Message ID | 20240605074936.578687-5-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Add SD/MMC support for Renesas RZ/V2H(P) SoC | expand |
Hi Prabhakar, Thanks for the feedback. > -----Original Message----- > From: Prabhakar <prabhakar.csengg@gmail.com> > Sent: Wednesday, June 5, 2024 8:50 AM > Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very similar to those found in R- > Car Gen3. However, they are not identical, necessitating an SoC-specific compatible string for > fine-tuning driver support. > > Key features of the RZ/V2H(P) SDHI/eMMC IPs include: > - Voltage level control via the IOVS bit. > - PWEN pin support via SD_STATUS register. > - Lack of HS400 support. > - Fixed address mode operation. > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit to handle voltage level > control and power enable via SD_STATUS register. > > regulator support is added to control the volatage levels of SD pins via sd_iovs bit in SD_STATUS > register. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/mmc/host/renesas_sdhi.h | 7 ++ > drivers/mmc/host/renesas_sdhi_core.c | 67 +++++++++++++++++-- > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++ > drivers/mmc/host/tmio_mmc.h | 4 ++ > 4 files changed, 118 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index > 586f94d4dbfd..9ef4fdf44280 100644 > --- a/drivers/mmc/host/renesas_sdhi.h > +++ b/drivers/mmc/host/renesas_sdhi.h > @@ -11,6 +11,8 @@ > > #include <linux/dmaengine.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/regulator/driver.h> > #include "tmio_mmc.h" > > struct renesas_sdhi_scc { > @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks { > bool manual_tap_correction; > bool old_info1_layout; > u32 hs400_bad_taps; > + bool sd_iovs; > + bool sd_pwen; > + struct regulator_desc *rdesc; > + const struct regmap_config *rdesc_regmap_config; > + unsigned int rdesc_offset; > const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; > }; > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > index 12f4faaaf4ee..2eeea9513a23 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc) > TMIO_STAT_DAT0); > } > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, > +bool on) { > + u32 sd_status; > + > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > + if (on) > + sd_status |= SD_STATUS_PWEN; > + else > + sd_status &= ~SD_STATUS_PWEN; > + > + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); } > + May be use regulator_set_voltage() to set this?? > static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc, > struct mmc_ios *ios) > { > @@ -587,6 +600,9 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve) > false, priv->rstc); > /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ > sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); > + if (sdhi_has_quirk(priv, sd_pwen)) > + renesas_sdhi_sd_status_pwen(host, true); > + > priv->needs_adjust_hs400 = false; > renesas_sdhi_set_clock(host, host->clk_cache); > > @@ -904,6 +920,34 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable) > renesas_sdhi_sdbuf_width(host, enable ? width : 16); } > > +static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev, > + const struct renesas_sdhi_quirks *quirks) { > + struct tmio_mmc_host *host = platform_get_drvdata(pdev); > + struct renesas_sdhi *priv = host_to_priv(host); > + struct regulator_config rcfg = { > + .dev = &pdev->dev, > + .driver_data = priv, > + }; > + struct regulator_dev *rdev; > + const char *devname; > + > + devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator", > + dev_name(&pdev->dev)); > + if (!devname) > + return -ENOMEM; > + > + quirks->rdesc->name = devname; > + rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset, This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks. Cheers, Biju > + quirks->rdesc_regmap_config); > + if (IS_ERR(rcfg.regmap)) > + return PTR_ERR(rcfg.regmap); > + > + rdev = devm_regulator_register(&pdev->dev, quirks->rdesc, &rcfg); > + > + return PTR_ERR_OR_ZERO(rdev); > +} > + > int renesas_sdhi_probe(struct platform_device *pdev, > const struct tmio_mmc_dma_ops *dma_ops, > const struct renesas_sdhi_of_data *of_data, @@ -1051,6 +1095,15 @@ int > renesas_sdhi_probe(struct platform_device *pdev, > if (ret) > goto efree; > > + if (sdhi_has_quirk(priv, sd_iovs)) { > + ret = renesas_sdhi_internal_dmac_register_regulator(pdev, quirks); > + if (ret) > + goto efree; > + } > + > + if (sdhi_has_quirk(priv, sd_pwen)) > + renesas_sdhi_sd_status_pwen(host, true); > + > 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) @@ -1110,26 +1163,26 @@ > int renesas_sdhi_probe(struct platform_device *pdev, > num_irqs = platform_irq_count(pdev); > if (num_irqs < 0) { > ret = num_irqs; > - goto eirq; > + goto epwen; > } > > /* There must be at least one IRQ source */ > if (!num_irqs) { > ret = -ENXIO; > - goto eirq; > + goto epwen; > } > > for (i = 0; i < num_irqs; i++) { > irq = platform_get_irq(pdev, i); > if (irq < 0) { > ret = irq; > - goto eirq; > + goto epwen; > } > > ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0, > dev_name(&pdev->dev), host); > if (ret) > - goto eirq; > + goto epwen; > } > > ret = tmio_mmc_host_probe(host); > @@ -1141,7 +1194,9 @@ int renesas_sdhi_probe(struct platform_device *pdev, > > return ret; > > -eirq: > +epwen: > + if (sdhi_has_quirk(priv, sd_pwen)) > + renesas_sdhi_sd_status_pwen(host, false); > tmio_mmc_host_remove(host); > edisclk: > renesas_sdhi_clk_disable(host); > @@ -1157,6 +1212,8 @@ void renesas_sdhi_remove(struct platform_device *pdev) > struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > tmio_mmc_host_remove(host); > + if (sdhi_has_quirk(host_to_priv(host), sd_pwen)) > + renesas_sdhi_sd_status_pwen(host, false); > renesas_sdhi_clk_disable(host); > tmio_mmc_host_free(host); > } > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > index 422fa63a2e99..f824d167bb09 100644 > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > @@ -215,6 +215,45 @@ static const struct renesas_sdhi_quirks sdhi_quirks_rzg2l = { > .hs400_disabled = true, > }; > > +static const unsigned int r9a09g057_vqmmc_voltages[] = { > + 3300000, 1800000, > +}; > + > +static const struct regulator_ops r9a09g057_regulator_voltage_ops = { > + .list_voltage = regulator_list_voltage_table, > + .map_voltage = regulator_map_voltage_descend, > + .get_voltage_sel = regulator_get_voltage_sel_regmap, > + .set_voltage_sel = regulator_set_voltage_sel_regmap, }; > + > +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), > + .vsel_mask = 0x10000, > + .vsel_reg = 0, > +}; > + > +static const struct regmap_config r9a09g057_vqmmc_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = 1, > +}; > + > +static const struct renesas_sdhi_quirks sdhi_quirks_r9a09g057 = { > + .fixed_addr_mode = true, > + .hs400_disabled = true, > + .sd_iovs = true, > + .sd_pwen = true, > + .rdesc = &r9a09g057_vqmmc_regulator, > + .rdesc_regmap_config = &r9a09g057_vqmmc_regmap_config, > + .rdesc_offset = 0x3C8, > +}; > + > /* > * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now. > * So, we want to treat them equally and only have a match for ES1.2 to enforce @@ -260,6 +299,11 > @@ static const struct renesas_sdhi_of_data_with_quirks of_rzg2l_compatible = { > .quirks = &sdhi_quirks_rzg2l, > }; > > +static const struct renesas_sdhi_of_data_with_quirks of_r9a09g057_compatible = { > + .of_data = &of_data_rcar_gen3, > + .quirks = &sdhi_quirks_r9a09g057, > +}; > + > static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen3_compatible = { > .of_data = &of_data_rcar_gen3, > }; > @@ -284,6 +328,7 @@ static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = { > { .compatible = "renesas,sdhi-r8a77990", .data = &of_r8a77990_compatible, }, > { .compatible = "renesas,sdhi-r8a77995", .data = &of_rcar_gen3_nohs400_compatible, }, > { .compatible = "renesas,sdhi-r9a09g011", .data = &of_rzg2l_compatible, }, > + { .compatible = "renesas,sdhi-r9a09g057", .data = > +&of_r9a09g057_compatible, }, > { .compatible = "renesas,rzg2l-sdhi", .data = &of_rzg2l_compatible, }, > { .compatible = "renesas,rcar-gen3-sdhi", .data = &of_rcar_gen3_compatible, }, > { .compatible = "renesas,rcar-gen4-sdhi", .data = &of_rcar_gen3_compatible, }, diff --git > a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index de56e6534aea..d03aedf61aa3 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -43,6 +43,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 and RZ/V2H(P) */ > > /* Definitions for values the CTL_STOP_INTERNAL_ACTION register can take */ > #define TMIO_STOP_STP BIT(0) > @@ -102,6 +103,9 @@ > /* 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/V2H(P) */ > + > /* Define some IRQ masks */ > /* This is the mask used at reset by the chip */ > #define TMIO_MASK_ALL 0x837f031d > -- > 2.34.1
Hi Biju, Thank you for the review. On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > Thanks for the feedback. > > > -----Original Message----- > > From: Prabhakar <prabhakar.csengg@gmail.com> > > Sent: Wednesday, June 5, 2024 8:50 AM > > Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very similar to those found in R- > > Car Gen3. However, they are not identical, necessitating an SoC-specific compatible string for > > fine-tuning driver support. > > > > Key features of the RZ/V2H(P) SDHI/eMMC IPs include: > > - Voltage level control via the IOVS bit. > > - PWEN pin support via SD_STATUS register. > > - Lack of HS400 support. > > - Fixed address mode operation. > > > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit to handle voltage level > > control and power enable via SD_STATUS register. > > > > regulator support is added to control the volatage levels of SD pins via sd_iovs bit in SD_STATUS > > register. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/mmc/host/renesas_sdhi.h | 7 ++ > > drivers/mmc/host/renesas_sdhi_core.c | 67 +++++++++++++++++-- > > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++ > > drivers/mmc/host/tmio_mmc.h | 4 ++ > > 4 files changed, 118 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index > > 586f94d4dbfd..9ef4fdf44280 100644 > > --- a/drivers/mmc/host/renesas_sdhi.h > > +++ b/drivers/mmc/host/renesas_sdhi.h > > @@ -11,6 +11,8 @@ > > > > #include <linux/dmaengine.h> > > #include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > #include "tmio_mmc.h" > > > > struct renesas_sdhi_scc { > > @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks { > > bool manual_tap_correction; > > bool old_info1_layout; > > u32 hs400_bad_taps; > > + bool sd_iovs; > > + bool sd_pwen; > > + struct regulator_desc *rdesc; > > + const struct regmap_config *rdesc_regmap_config; > > + unsigned int rdesc_offset; > > const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; > > }; > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c > > index 12f4faaaf4ee..2eeea9513a23 100644 > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc) > > TMIO_STAT_DAT0); > > } > > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, > > +bool on) { > > + u32 sd_status; > > + > > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > > + if (on) > > + sd_status |= SD_STATUS_PWEN; > > + else > > + sd_status &= ~SD_STATUS_PWEN; > > + > > + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); } > > + > > May be use regulator_set_voltage() to set this?? > This is the PWEN bit which is not modelled as a regulator, we cannot use regulator_set_voltage() to set this bit. > > static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc, > > struct mmc_ios *ios) > > { > > @@ -587,6 +600,9 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve) > > false, priv->rstc); > > /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ > > sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); > > + if (sdhi_has_quirk(priv, sd_pwen)) > > + renesas_sdhi_sd_status_pwen(host, true); > > + > > priv->needs_adjust_hs400 = false; > > renesas_sdhi_set_clock(host, host->clk_cache); > > > > @@ -904,6 +920,34 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable) > > renesas_sdhi_sdbuf_width(host, enable ? width : 16); } > > > > +static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev, > > + const struct renesas_sdhi_quirks *quirks) { > > + struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > + struct renesas_sdhi *priv = host_to_priv(host); > > + struct regulator_config rcfg = { > > + .dev = &pdev->dev, > > + .driver_data = priv, > > + }; > > + struct regulator_dev *rdev; > > + const char *devname; > > + > > + devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator", > > + dev_name(&pdev->dev)); > > + if (!devname) > > + return -ENOMEM; > > + > > + quirks->rdesc->name = devname; > > + rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset, > > This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks. > rdesc_offset is added to make code generic, that is in future if there is a new chip with a different offset which supports IOVS we can just pass the offset for it. Cheers, Prabhakar
Hi Prabhakar, > -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: Thursday, June 6, 2024 10:38 AM > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC > > Hi Biju, > > Thank you for the review. > > On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > Thanks for the feedback. > > > > > -----Original Message----- > > > From: Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: Wednesday, June 5, 2024 8:50 AM > > > Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for > > > RZ/V2H(P) SoC > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very > > > similar to those found in R- Car Gen3. However, they are not > > > identical, necessitating an SoC-specific compatible string for fine-tuning driver support. > > > > > > Key features of the RZ/V2H(P) SDHI/eMMC IPs include: > > > - Voltage level control via the IOVS bit. > > > - PWEN pin support via SD_STATUS register. > > > - Lack of HS400 support. > > > - Fixed address mode operation. > > > > > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this > > > bit to handle voltage level control and power enable via SD_STATUS register. > > > > > > regulator support is added to control the volatage levels of SD pins > > > via sd_iovs bit in SD_STATUS register. > > > > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/mmc/host/renesas_sdhi.h | 7 ++ > > > drivers/mmc/host/renesas_sdhi_core.c | 67 +++++++++++++++++-- > > > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++ > > > drivers/mmc/host/tmio_mmc.h | 4 ++ > > > 4 files changed, 118 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/mmc/host/renesas_sdhi.h > > > b/drivers/mmc/host/renesas_sdhi.h index > > > 586f94d4dbfd..9ef4fdf44280 100644 > > > --- a/drivers/mmc/host/renesas_sdhi.h > > > +++ b/drivers/mmc/host/renesas_sdhi.h > > > @@ -11,6 +11,8 @@ > > > > > > #include <linux/dmaengine.h> > > > #include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > +#include <linux/regulator/driver.h> > > > #include "tmio_mmc.h" > > > > > > struct renesas_sdhi_scc { > > > @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks { > > > bool manual_tap_correction; > > > bool old_info1_layout; > > > u32 hs400_bad_taps; > > > + bool sd_iovs; > > > + bool sd_pwen; > > > + struct regulator_desc *rdesc; > > > + const struct regmap_config *rdesc_regmap_config; > > > + unsigned int rdesc_offset; > > > const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; > > > }; > > > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c > > > b/drivers/mmc/host/renesas_sdhi_core.c > > > index 12f4faaaf4ee..2eeea9513a23 100644 > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc) > > > TMIO_STAT_DAT0); > > > } > > > > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, > > > +bool on) { > > > + u32 sd_status; > > > + > > > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > > > + if (on) > > > + sd_status |= SD_STATUS_PWEN; > > > + else > > > + sd_status &= ~SD_STATUS_PWEN; > > > + > > > + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); } > > > + > > > > May be use regulator_set_voltage() to set this?? > > > This is the PWEN bit which is not modelled as a regulator, we cannot use regulator_set_voltage() to > set this bit. So, there may be a race between regulator driver and this bit?? > > > > static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc, > > > struct mmc_ios > > > *ios) { @@ -587,6 +600,9 @@ static void renesas_sdhi_reset(struct > > > tmio_mmc_host *host, bool preserve) > > > false, priv->rstc); > > > /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ > > > sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); > > > + if (sdhi_has_quirk(priv, sd_pwen)) > > > + renesas_sdhi_sd_status_pwen(host, > > > + true); > > > + > > > priv->needs_adjust_hs400 = false; > > > renesas_sdhi_set_clock(host, host->clk_cache); > > > > > > @@ -904,6 +920,34 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool > enable) > > > renesas_sdhi_sdbuf_width(host, enable ? width : 16); } > > > > > > +static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev, > > > + const struct renesas_sdhi_quirks > *quirks) { > > > + struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > > + struct renesas_sdhi *priv = host_to_priv(host); > > > + struct regulator_config rcfg = { > > > + .dev = &pdev->dev, > > > + .driver_data = priv, > > > + }; > > > + struct regulator_dev *rdev; > > > + const char *devname; > > > + > > > + devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator", > > > + dev_name(&pdev->dev)); > > > + if (!devname) > > > + return -ENOMEM; > > > + > > > + quirks->rdesc->name = devname; > > > + rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + > > > + quirks->rdesc_offset, > > > > This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks. > > > rdesc_offset is added to make code generic, that is in future if there is a new chip with a > different offset which supports IOVS we can just pass the offset for it. Currently there is no consumer for it, so it can save memory. When a future chip comes we can bring back this variable?? Cheers, Biju
Hi Biju, On Thu, Jun 6, 2024 at 10:43 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar, > > > > -----Original Message----- > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > Sent: Thursday, June 6, 2024 10:38 AM > > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC > > > > Hi Biju, > > > > Thank you for the review. > > > > On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > Hi Prabhakar, > > > <snip> > > > > > > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, > > > > +bool on) { > > > > + u32 sd_status; > > > > + > > > > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > > > > + if (on) > > > > + sd_status |= SD_STATUS_PWEN; > > > > + else > > > > + sd_status &= ~SD_STATUS_PWEN; > > > > + > > > > + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); } > > > > + > > > > > > May be use regulator_set_voltage() to set this?? > > > > > This is the PWEN bit which is not modelled as a regulator, we cannot use regulator_set_voltage() to > > set this bit. > > So, there may be a race between regulator driver and this bit?? > No, there won't be any race between the regulator driver and this bit as the regulator driver only controls the IOVS bit and not the PWEN bit. > > > > > > static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc, > > > > struct mmc_ios > > > > *ios) { @@ -587,6 +600,9 @@ static void renesas_sdhi_reset(struct > > > > tmio_mmc_host *host, bool preserve) > > > > false, priv->rstc); > > > > /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ > > > > sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); > > > > + if (sdhi_has_quirk(priv, sd_pwen)) > > > > + renesas_sdhi_sd_status_pwen(host, > > > > + true); > > > > + > > > > priv->needs_adjust_hs400 = false; > > > > renesas_sdhi_set_clock(host, host->clk_cache); > > > > > > > > @@ -904,6 +920,34 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool > > enable) > > > > renesas_sdhi_sdbuf_width(host, enable ? width : 16); } > > > > > > > > +static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev, > > > > + const struct renesas_sdhi_quirks > > *quirks) { > > > > + struct tmio_mmc_host *host = platform_get_drvdata(pdev); > > > > + struct renesas_sdhi *priv = host_to_priv(host); > > > > + struct regulator_config rcfg = { > > > > + .dev = &pdev->dev, > > > > + .driver_data = priv, > > > > + }; > > > > + struct regulator_dev *rdev; > > > > + const char *devname; > > > > + > > > > + devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator", > > > > + dev_name(&pdev->dev)); > > > > + if (!devname) > > > > + return -ENOMEM; > > > > + > > > > + quirks->rdesc->name = devname; > > > > + rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + > > > > + quirks->rdesc_offset, > > > > > > This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks. > > > > > rdesc_offset is added to make code generic, that is in future if there is a new chip with a > > different offset which supports IOVS we can just pass the offset for it. > > Currently there is no consumer for it, so it can save memory. When a future chip comes > we can bring back this variable?? > OK. Cheers, Prabhakar
> -----Original Message----- > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > Sent: Thursday, June 6, 2024 10:50 AM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Ulf Hansson <ulf.hansson@linaro.org>; Rob Herring > <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; > Wolfram Sang <wsa+renesas@sang-engineering.com>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown > <broonie@kernel.org>; Magnus Damm <magnus.damm@gmail.com>; linux-mmc@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org; > Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com> > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC > > Hi Biju, > > On Thu, Jun 6, 2024 at 10:43 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > Hi Prabhakar, > > > > > > > -----Original Message----- > > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > > Sent: Thursday, June 6, 2024 10:38 AM > > > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for > > > RZ/V2H(P) SoC > > > > > > Hi Biju, > > > > > > Thank you for the review. > > > > > > On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > > Hi Prabhakar, > > > > > <snip> > > > > > > > > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host > > > > > +*host, bool on) { > > > > > + u32 sd_status; > > > > > + > > > > > + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); > > > > > + if (on) > > > > > + sd_status |= SD_STATUS_PWEN; > > > > > + else > > > > > + sd_status &= ~SD_STATUS_PWEN; > > > > > + > > > > > + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); } > > > > > + > > > > > > > > May be use regulator_set_voltage() to set this?? > > > > > > > This is the PWEN bit which is not modelled as a regulator, we cannot > > > use regulator_set_voltage() to set this bit. > > > > So, there may be a race between regulator driver and this bit?? > > > No, there won't be any race between the regulator driver and this bit as the regulator driver only > controls the IOVS bit and not the PWEN bit. I guess, since it is rmw operation, there is a chance that this will overwritten in a race condition. But mmc driver is manages the calls in serial fashion, so there won't be any races. Cheers, Biju
Hi Prabhakar, thanks for this series! On Wed, Jun 05, 2024 at 08:49:36AM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very > similar to those found in R-Car Gen3. However, they are not identical, > necessitating an SoC-specific compatible string for fine-tuning driver > support. > > Key features of the RZ/V2H(P) SDHI/eMMC IPs include: > - Voltage level control via the IOVS bit. > - PWEN pin support via SD_STATUS register. > - Lack of HS400 support. > - Fixed address mode operation. > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit > to handle voltage level control and power enable via SD_STATUS register. Two high-level questions: - can't we use .enable/.disable in regulator_ops for handling pwen? Then we could simply use regulator_en/disable in the code and be future proof when other SDHI instances have other kinds of regulators (unless I am mising something) - what about not using regmap and use set/get_voltage and friends? My concern is that other "new" registers might appear in the future and it will be cumbersome to handle the scattered IO regions. That said, having a regulator is not a quirk in my book. I'd think 'struct renesas_sdhi' is the proper place. Or? Looking forward to your comments, Wolfram
Hi Wolfram, On Thu, Jun 6, 2024 at 11:08 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Prabhakar, > > thanks for this series! > > On Wed, Jun 05, 2024 at 08:49:36AM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very > > similar to those found in R-Car Gen3. However, they are not identical, > > necessitating an SoC-specific compatible string for fine-tuning driver > > support. > > > > Key features of the RZ/V2H(P) SDHI/eMMC IPs include: > > - Voltage level control via the IOVS bit. > > - PWEN pin support via SD_STATUS register. > > - Lack of HS400 support. > > - Fixed address mode operation. > > > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit > > to handle voltage level control and power enable via SD_STATUS register. > > Two high-level questions: > > - can't we use .enable/.disable in regulator_ops for handling pwen? > Then we could simply use regulator_en/disable in the code and be future > proof when other SDHI instances have other kinds of regulators (unless > I am mising something) > Ok let me check on this and get back. > - what about not using regmap and use set/get_voltage and friends? My > concern is that other "new" registers might appear in the future and > it will be cumbersome to handle the scattered IO regions. > I'll have to do some reading on this. Can you please point me to any example driver which does not use regmap. > That said, having a regulator is not a quirk in my book. I'd think > 'struct renesas_sdhi' is the proper place. Or? > Ok, I will move them out of quirks. Cheers, Prabhakar
Hi Prabhakar, > > - can't we use .enable/.disable in regulator_ops for handling pwen? > > Then we could simply use regulator_en/disable in the code and be future > > proof when other SDHI instances have other kinds of regulators (unless > > I am mising something) > > > Ok let me check on this and get back. Thanks! > > - what about not using regmap and use set/get_voltage and friends? My > > concern is that other "new" registers might appear in the future and > > it will be cumbersome to handle the scattered IO regions. > > > I'll have to do some reading on this. Can you please point me to any > example driver which does not use regmap. Sure thing! ~/Kernel/linux/drivers/regulator$ grep -L regmap $(grep -l devm_regulator_register *.c) aat2870-regulator.c ab8500.c ab8500-ext.c ad5398.c cros-ec-regulator.c da903x-regulator.c db8500-prcmu.c dummy.c fixed.c gpio-regulator.c isl6271a-regulator.c lp3971.c lp3972.c max1586.c max8660.c max8925-regulator.c max8952.c max8997-regulator.c max8998.c mc13783-regulator.c mc13892-regulator.c mtk-dvfsrc-regulator.c pcap-regulator.c pwm-regulator.c qcom-rpmh-regulator.c qcom_rpm-regulator.c qcom_smd-regulator.c scmi-regulator.c stm32-pwr.c ti-abb-regulator.c tps6507x-regulator.c tps6524x-regulator.c twl6030-regulator.c twl-regulator.c vctrl-regulator.c > > That said, having a regulator is not a quirk in my book. I'd think > > 'struct renesas_sdhi' is the proper place. Or? > > > Ok, I will move them out of quirks. Cool! Happy hacking, Wolfram
diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index 586f94d4dbfd..9ef4fdf44280 100644 --- a/drivers/mmc/host/renesas_sdhi.h +++ b/drivers/mmc/host/renesas_sdhi.h @@ -11,6 +11,8 @@ #include <linux/dmaengine.h> #include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> #include "tmio_mmc.h" struct renesas_sdhi_scc { @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks { bool manual_tap_correction; bool old_info1_layout; u32 hs400_bad_taps; + bool sd_iovs; + bool sd_pwen; + struct regulator_desc *rdesc; + const struct regmap_config *rdesc_regmap_config; + unsigned int rdesc_offset; const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; }; diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 12f4faaaf4ee..2eeea9513a23 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc) TMIO_STAT_DAT0); } +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, bool on) +{ + u32 sd_status; + + sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1); + if (on) + sd_status |= SD_STATUS_PWEN; + else + sd_status &= ~SD_STATUS_PWEN; + + sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); +} + static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) { @@ -587,6 +600,9 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve) false, priv->rstc); /* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */ sd_ctrl_write16(host, CTL_RESET_SD, 0x0001); + if (sdhi_has_quirk(priv, sd_pwen)) + renesas_sdhi_sd_status_pwen(host, true); + priv->needs_adjust_hs400 = false; renesas_sdhi_set_clock(host, host->clk_cache); @@ -904,6 +920,34 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable) renesas_sdhi_sdbuf_width(host, enable ? width : 16); } +static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev, + const struct renesas_sdhi_quirks *quirks) +{ + struct tmio_mmc_host *host = platform_get_drvdata(pdev); + struct renesas_sdhi *priv = host_to_priv(host); + struct regulator_config rcfg = { + .dev = &pdev->dev, + .driver_data = priv, + }; + struct regulator_dev *rdev; + const char *devname; + + devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator", + dev_name(&pdev->dev)); + if (!devname) + return -ENOMEM; + + quirks->rdesc->name = devname; + rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset, + quirks->rdesc_regmap_config); + if (IS_ERR(rcfg.regmap)) + return PTR_ERR(rcfg.regmap); + + rdev = devm_regulator_register(&pdev->dev, quirks->rdesc, &rcfg); + + return PTR_ERR_OR_ZERO(rdev); +} + int renesas_sdhi_probe(struct platform_device *pdev, const struct tmio_mmc_dma_ops *dma_ops, const struct renesas_sdhi_of_data *of_data, @@ -1051,6 +1095,15 @@ int renesas_sdhi_probe(struct platform_device *pdev, if (ret) goto efree; + if (sdhi_has_quirk(priv, sd_iovs)) { + ret = renesas_sdhi_internal_dmac_register_regulator(pdev, quirks); + if (ret) + goto efree; + } + + if (sdhi_has_quirk(priv, sd_pwen)) + renesas_sdhi_sd_status_pwen(host, true); + 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) @@ -1110,26 +1163,26 @@ int renesas_sdhi_probe(struct platform_device *pdev, num_irqs = platform_irq_count(pdev); if (num_irqs < 0) { ret = num_irqs; - goto eirq; + goto epwen; } /* There must be at least one IRQ source */ if (!num_irqs) { ret = -ENXIO; - goto eirq; + goto epwen; } for (i = 0; i < num_irqs; i++) { irq = platform_get_irq(pdev, i); if (irq < 0) { ret = irq; - goto eirq; + goto epwen; } ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0, dev_name(&pdev->dev), host); if (ret) - goto eirq; + goto epwen; } ret = tmio_mmc_host_probe(host); @@ -1141,7 +1194,9 @@ int renesas_sdhi_probe(struct platform_device *pdev, return ret; -eirq: +epwen: + if (sdhi_has_quirk(priv, sd_pwen)) + renesas_sdhi_sd_status_pwen(host, false); tmio_mmc_host_remove(host); edisclk: renesas_sdhi_clk_disable(host); @@ -1157,6 +1212,8 @@ void renesas_sdhi_remove(struct platform_device *pdev) struct tmio_mmc_host *host = platform_get_drvdata(pdev); tmio_mmc_host_remove(host); + if (sdhi_has_quirk(host_to_priv(host), sd_pwen)) + renesas_sdhi_sd_status_pwen(host, false); renesas_sdhi_clk_disable(host); tmio_mmc_host_free(host); } diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c index 422fa63a2e99..f824d167bb09 100644 --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c @@ -215,6 +215,45 @@ static const struct renesas_sdhi_quirks sdhi_quirks_rzg2l = { .hs400_disabled = true, }; +static const unsigned int r9a09g057_vqmmc_voltages[] = { + 3300000, 1800000, +}; + +static const struct regulator_ops r9a09g057_regulator_voltage_ops = { + .list_voltage = regulator_list_voltage_table, + .map_voltage = regulator_map_voltage_descend, + .get_voltage_sel = regulator_get_voltage_sel_regmap, + .set_voltage_sel = regulator_set_voltage_sel_regmap, +}; + +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), + .vsel_mask = 0x10000, + .vsel_reg = 0, +}; + +static const struct regmap_config r9a09g057_vqmmc_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .max_register = 1, +}; + +static const struct renesas_sdhi_quirks sdhi_quirks_r9a09g057 = { + .fixed_addr_mode = true, + .hs400_disabled = true, + .sd_iovs = true, + .sd_pwen = true, + .rdesc = &r9a09g057_vqmmc_regulator, + .rdesc_regmap_config = &r9a09g057_vqmmc_regmap_config, + .rdesc_offset = 0x3C8, +}; + /* * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now. * So, we want to treat them equally and only have a match for ES1.2 to enforce @@ -260,6 +299,11 @@ static const struct renesas_sdhi_of_data_with_quirks of_rzg2l_compatible = { .quirks = &sdhi_quirks_rzg2l, }; +static const struct renesas_sdhi_of_data_with_quirks of_r9a09g057_compatible = { + .of_data = &of_data_rcar_gen3, + .quirks = &sdhi_quirks_r9a09g057, +}; + static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen3_compatible = { .of_data = &of_data_rcar_gen3, }; @@ -284,6 +328,7 @@ static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = { { .compatible = "renesas,sdhi-r8a77990", .data = &of_r8a77990_compatible, }, { .compatible = "renesas,sdhi-r8a77995", .data = &of_rcar_gen3_nohs400_compatible, }, { .compatible = "renesas,sdhi-r9a09g011", .data = &of_rzg2l_compatible, }, + { .compatible = "renesas,sdhi-r9a09g057", .data = &of_r9a09g057_compatible, }, { .compatible = "renesas,rzg2l-sdhi", .data = &of_rzg2l_compatible, }, { .compatible = "renesas,rcar-gen3-sdhi", .data = &of_rcar_gen3_compatible, }, { .compatible = "renesas,rcar-gen4-sdhi", .data = &of_rcar_gen3_compatible, }, diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index de56e6534aea..d03aedf61aa3 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -43,6 +43,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 and RZ/V2H(P) */ /* Definitions for values the CTL_STOP_INTERNAL_ACTION register can take */ #define TMIO_STOP_STP BIT(0) @@ -102,6 +103,9 @@ /* 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/V2H(P) */ + /* Define some IRQ masks */ /* This is the mask used at reset by the chip */ #define TMIO_MASK_ALL 0x837f031d