Re: [PATCH v4 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the review.

On Wed, Jul 3, 2024 at 10:43 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Wed, Jun 26, 2024 at 02:23:41PM +0100, Prabhakar wrote:
> > 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.
> >
> > internal regulator support is added to control the voltage levels of SD
> > pins via sd_iovs/sd_pwen bits in SD_STATUS register.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> # on RZ/G3S
> > ---
> > v3->v4
> > - Dropped using 'renesas,sdhi-use-internal-regulator' property
> > - Now using of_device_is_available() to check if regulator is available and enabled
> > - Dropped extra spaces during operations
> > - Included tested by tag from Claudiu
> > - Rebased patch on top of https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240626085015.32171-2-wsa+renesas@xxxxxxxxxxxxxxxxxxxx/
> >
> > v2->v3
> > - Moved regulator info to renesas_sdhi_of_data instead of quirks
> > - Added support to configure the init state of regulator
> > - Added function pointers to configure regulator
> > - Added REGULATOR_CHANGE_VOLTAGE mask
> >
> > v1->v2
> > - Now controlling PWEN bit get/set_voltage
> > ---
> >  drivers/mmc/host/renesas_sdhi.h               |  13 ++
> >  drivers/mmc/host/renesas_sdhi_core.c          |  98 ++++++++++++
> >  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 147 ++++++++++++++++++
> >  drivers/mmc/host/tmio_mmc.h                   |   5 +
> >  4 files changed, 263 insertions(+)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
> > index f12a87442338..cd509e7142ba 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/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> >  #include <linux/workqueue.h>
> >  #include "tmio_mmc.h"
> >
> > @@ -36,6 +38,12 @@ struct renesas_sdhi_of_data {
> >       unsigned int max_blk_count;
> >       unsigned short max_segs;
> >       unsigned long sdhi_flags;
> > +     struct regulator_desc *rdesc;
> > +     struct regulator_init_data *reg_init_data;
> > +     bool regulator_init_state;
> > +     unsigned int regulator_init_voltage;
> > +     int (*regulator_force_endis)(struct regulator_dev *rdev, bool enable);
> > +     int (*regulator_force_voltage)(struct regulator_dev *rdev, unsigned int voltage);
>
> I am open for discussing this but maybe here only
>
> +       struct renesas_sdhi_regulator *internal_regulator
>
> or something and create the new struct with the additions above?
>
> > +     int (*regulator_force_endis)(struct regulator_dev *rdev, bool enable);
> > +     int (*regulator_force_voltage)(struct regulator_dev *rdev, unsigned int voltage);
>
> Do we need these functions because the regulator framework cannot force
> these actions because it caches the old state? I wonder if we can avoid
> these functions...
>
Yes, for the voltage setting, it caches the values. However, for the
regulator enable/disable, we can use is_enabled(), which probes the
hardware.

The reset value for PWEN is 1. The regulator_force_endis() callback is
mainly added for a scenario where, consider a code flow where the
regulator is disabled (using regulator_disable()) and now we land in
the reset callback (i.e., renesas_sdhi_reset()). Here, after issuing
the reset, the PWEN value will be 1, but we need to restore it back.
Hence, this callback is necessary. Note that is_enabled() cannot be
used, as it probes the hardware when it switches states after a reset.

The reset value for IOVS is 3.3V. Below is the scenario for which
regulator_force_voltage() is added:

-----> Current value: 1.8V (cached by the regulator)
--------------> After reset:
------------------> Hardware has 3.3V, but the regulator core cache
still has 1.8V.
----------------------> When requested to switch to 1.8V from MMC
core: The regulator core returns success, as it sees 1.8V in the
cached state.
----------------------------> As a result, the SD card won't work.

Cheers,
Prabhakar