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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]

Hi Prabhakar,

On Wed, Jun 26, 2024 at 02:23:41PM +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.
> 
> 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@bp.renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> # 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@sang-engineering.com/
> 
> 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...

And the questions from the other threads need further discussions as
well.

But in general, I still like this approach.

Thank you,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]