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
