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

From: Lad, Prabhakar
Date: Thu Jul 04 2024 - 11:58:54 EST


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