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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZoUdUyrld2kZorvU@shikoro>
Date: Wed, 3 Jul 2024 11:43:47 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Ulf Hansson <ulf.hansson@...aro.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Magnus Damm <magnus.damm@...il.com>, linux-mmc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org,
	Biju Das <biju.das.jz@...renesas.com>,
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v4 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

Hi Prabhakar,

On Wed, Jun 26, 2024 at 02:23:41PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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@...renesas.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@...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


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)