linux-kernel - Re: [RFC PATCH v2 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: <prdvmtsp35yy2naohivbrya3go6vh5ji2cdjoebw3ovditcujo@bhck6tca6mhj>
Date: Mon, 17 Jun 2024 10:31:01 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>, 
	Ulf Hansson <ulf.hansson@...aro.org>, 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>
Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P)
 SoC

Hi Prabhakar,

> - Ive modelled the regulator now to control the PWEN aswell.

Yay, this looks much better. Good work!

> - I have still kept regulator bits in quirks I was wondering if I should
>   move this to renesas_sdhi_of_data instead?

I think so. An internal regulator is not a quirk.

> - I still need to add checks if the internal regulator used and
>   only then call regulator_enable/regulator_set_voltage. ATM I am still
>   unclear on differentiating if internal/external regulator is used.

When it comes to re-enabling the regulator in sdhi_reset, I think this
can be a sdhi_flag like SDHI_FLAG_ENABLE_REGULATOR_IN_RESET or alike.

When it comes to the regulator, I wonder if it wouldn't be clearer to
replace renesas_sdhi_internal_dmac_register_regulator() with a proper
probe function and a dedicated compatible value for it. We could use
platform_driver_probe() to instantiate the new driver within the SDHI
probe function. This will ensure that the regulator driver will only be
started once the main driver got all needed resources (mapped
registers).

My gut feeling is that it will pay off if the internal regulator will be
described in DT as any other regulator. Like, we could name the
regulator in DT as always etc...

More opinions on this idea are welcome, though...

> --- a/drivers/mmc/host/renesas_sdhi.h
> +++ b/drivers/mmc/host/renesas_sdhi.h
> @@ -11,6 +11,9 @@
>  
>  #include <linux/dmaengine.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>

Regmap can luckily go now.

> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
>  #include "tmio_mmc.h"
>  
>  struct renesas_sdhi_scc {
> @@ -49,6 +52,9 @@ struct renesas_sdhi_quirks {
>  	bool manual_tap_correction;
>  	bool old_info1_layout;
>  	u32 hs400_bad_taps;
> +	bool internal_regulator;
> +	struct regulator_desc *rdesc;
> +	struct regulator_init_data *reg_init_data;
>  	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
>  };
>  
> @@ -93,6 +99,8 @@ struct renesas_sdhi {
>  	unsigned int tap_set;
>  
>  	struct reset_control *rstc;
> +
> +	struct regulator_dev *sd_status;

This is a strange name for the regulater. Especially given that you have
as well the more fitting 'u32 sd_status' in the code later.

...

> +static struct regulator_init_data r9a09g057_regulator_init_data = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,

Don't we need REGULATOR_CHANGE_VOLTAGE here as well? Or is this implicit
because of REGULATOR_VOLTAGE? Can't find this, though.

So much for now. Thanks!

   Wolfram

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