Hi Prabhakar,
> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
> 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@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the feedback.
> >
> > > -----Original Message-----
> > > From: Prabhakar <prabhakar.csengg@xxxxxxxxx>
> > > 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@xxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> > > ---
> > > 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
