[RFC PATCH v2 0/3] Add SD/MMC support for Renesas RZ/V2H(P) SoC

[RFC PATCH v2 0/3] Add SD/MMC support for Renesas RZ/V2H(P) SoC

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add SD/MMC support for Renesas RZ/V2H(P) SoC.

v1->v2
- Dropped regulator core API changes
- Updated DT binding
- Now controlling PWEN bit via regultor api

v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20240605074936.578687-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (3):
  dt-bindings: mmc: renesas,sdhi: Document RZ/V2H(P) support
  mmc: tmio: Use MMC core APIs to control the vqmmc regulator
  mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

 .../devicetree/bindings/mmc/renesas,sdhi.yaml |  20 ++-
 drivers/mmc/host/renesas_sdhi.h               |   8 ++
 drivers/mmc/host/renesas_sdhi_core.c          |  43 +++++++
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 120 ++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h                   |   5 +
 drivers/mmc/host/tmio_mmc_core.c              |   7 +-
 6 files changed, 198 insertions(+), 5 deletions(-)

-- 
2.34.1

[RFC PATCH v2 1/3] dt-bindings: mmc: renesas,sdhi: Document RZ/V2H(P) support

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The SD/MMC block on the RZ/V2H(P) ("R9A09G057") SoC is similar to that
of the R-Car Gen3, but it has some differences:
- HS400 is not supported.
- It supports the SD_IOVS bit to control the IO voltage level.
- It supports fixed address mode.

To accommodate these differences, a SoC-specific 'renesas,sdhi-r9a09g057'
compatible string is added.

A "vqmmc-r9a09g057-regulator" regulator object is added to handle the
pwen and voltage level switch of the SD/MMC.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Moved vqmmc object in the if block
- Updated commit message
---
 .../devicetree/bindings/mmc/renesas,sdhi.yaml | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml b/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
index 3d0e61e59856..154f5767cf03 100644
--- a/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
+++ b/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
@@ -18,6 +18,7 @@ properties:
           - renesas,sdhi-r7s9210 # SH-Mobile AG5
           - renesas,sdhi-r8a73a4 # R-Mobile APE6
           - renesas,sdhi-r8a7740 # R-Mobile A1
+          - renesas,sdhi-r9a09g057 # RZ/V2H(P)
           - renesas,sdhi-sh73a0  # R-Mobile APE6
       - items:
           - enum:
@@ -118,7 +119,9 @@ allOf:
       properties:
         compatible:
           contains:
-            const: renesas,rzg2l-sdhi
+            enum:
+              - renesas,sdhi-r9a09g057
+              - renesas,rzg2l-sdhi
     then:
       properties:
         clocks:
@@ -204,6 +207,21 @@ allOf:
         sectioned off to be run by a separate second clock source to allow
         the main core clock to be turned off to save power.
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: renesas,sdhi-r9a09g057
+    then:
+      properties:
+        vqmmc-r9a09g057-regulator:
+          type: object
+          description: VQMMC SD regulator
+          $ref: /schemas/regulator/regulator.yaml#
+          unevaluatedProperties: false
+      required:
+        - vqmmc-r9a09g057-regulator
+
 required:
   - compatible
   - reg
-- 
2.34.1

[RFC PATCH v2 2/3] mmc: tmio: Use MMC core APIs to control the vqmmc regulator

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Use the mmc_regulator_enable_vqmmc() and mmc_regulator_disable_vqmmc() APIs
to enable/disable the vqmmc regulator.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- New patch
---
 drivers/mmc/host/tmio_mmc_core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 93e912afd3ae..2ec1a74c85bc 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -897,8 +897,8 @@ static void tmio_mmc_power_on(struct tmio_mmc_host *host, unsigned short vdd)
 	 * It seems, VccQ should be switched on after Vcc, this is also what the
 	 * omap_hsmmc.c driver does.
 	 */
-	if (!IS_ERR(mmc->supply.vqmmc) && !ret) {
-		ret = regulator_enable(mmc->supply.vqmmc);
+	if (!ret) {
+		ret = mmc_regulator_enable_vqmmc(mmc);
 		usleep_range(200, 300);
 	}
 
@@ -911,8 +911,7 @@ static void tmio_mmc_power_off(struct tmio_mmc_host *host)
 {
 	struct mmc_host *mmc = host->mmc;
 
-	if (!IS_ERR(mmc->supply.vqmmc))
-		regulator_disable(mmc->supply.vqmmc);
+	mmc_regulator_disable_vqmmc(mmc);
 
 	if (!IS_ERR(mmc->supply.vmmc))
 		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0);
-- 
2.34.1

Re: [RFC PATCH v2 2/3] mmc: tmio: Use MMC core APIs to control the vqmmc regulator

[Wolfram Sang]

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

On Thu, Jun 13, 2024 at 10:17:20AM GMT, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Use the mmc_regulator_enable_vqmmc() and mmc_regulator_disable_vqmmc() APIs
> to enable/disable the vqmmc regulator.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cool, didn't know these helpers!

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [RFC PATCH v2 2/3] mmc: tmio: Use MMC core APIs to control the vqmmc regulator

[Geert Uytterhoeven]

On Thu, Jun 13, 2024 at 11:17 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Use the mmc_regulator_enable_vqmmc() and mmc_regulator_disable_vqmmc() APIs
> to enable/disable the vqmmc regulator.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Prabhakar]

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.

regulator support is added to control the volatage 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>
---
Hi Wolfram,

- Ive modelled the regulator now to control the PWEN aswell.
- I have still kept regulator bits in quirks I was wondering if I should
  move this to renesas_sdhi_of_data instead?
- 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.
  
Please let me know your thoughts.

Cheers, Prabhakar

v1->v2
- Now controlling PWEN bit get/set_voltage
---
 drivers/mmc/host/renesas_sdhi.h               |   8 ++
 drivers/mmc/host/renesas_sdhi_core.c          |  43 +++++++
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 120 ++++++++++++++++++
 drivers/mmc/host/tmio_mmc.h                   |   5 +
 4 files changed, 176 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 586f94d4dbfd..91e42e680dbb 100644
--- 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>
+#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;
 };
 
 #define host_to_priv(host) \
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 12f4faaaf4ee..47e99ce0b752 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -587,6 +587,20 @@ static void renesas_sdhi_reset(struct tmio_mmc_host *host, bool preserve)
 					  false, priv->rstc);
 			/* At least SDHI_VER_GEN2_SDR50 needs manual release of reset */
 			sd_ctrl_write16(host, CTL_RESET_SD, 0x0001);
+			if (sdhi_has_quirk(priv, internal_regulator)) {
+				unsigned int voltage;
+
+				if (!regulator_is_enabled(host->mmc->supply.vqmmc))
+					if (regulator_enable(host->mmc->supply.vqmmc))
+						dev_err(&host->pdev->dev, "Failed to enable internal regulator\n");
+
+				/* restore back voltage on vqmmc supply */
+				voltage = host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330 ?
+					  3300000 : 1800000;
+				regulator_set_voltage(host->mmc->supply.vqmmc,
+						      voltage, voltage);
+			}
+
 			priv->needs_adjust_hs400 = false;
 			renesas_sdhi_set_clock(host, host->clk_cache);
 
@@ -904,6 +918,29 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
 	renesas_sdhi_sdbuf_width(host, enable ? width : 16);
 }
 
+static int renesas_sdhi_internal_dmac_register_regulator(struct platform_device *pdev,
+							 const struct renesas_sdhi_quirks *quirks)
+{
+	struct tmio_mmc_host *host = platform_get_drvdata(pdev);
+	struct renesas_sdhi *priv = host_to_priv(host);
+	struct regulator_config rcfg = {
+		.dev = &pdev->dev,
+		.driver_data = host,
+		.init_data = quirks->reg_init_data,
+	};
+	const char *devname;
+
+	devname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-vqmmc-regulator",
+				 dev_name(&pdev->dev));
+	if (!devname)
+		return -ENOMEM;
+
+	quirks->rdesc->name = devname;
+	priv->sd_status = devm_regulator_register(&pdev->dev, quirks->rdesc, &rcfg);
+
+	return PTR_ERR_OR_ZERO(priv->sd_status);
+}
+
 int renesas_sdhi_probe(struct platform_device *pdev,
 		       const struct tmio_mmc_dma_ops *dma_ops,
 		       const struct renesas_sdhi_of_data *of_data,
@@ -1051,6 +1088,12 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	if (ret)
 		goto efree;
 
+	if (sdhi_has_quirk(priv, internal_regulator)) {
+		ret = renesas_sdhi_internal_dmac_register_regulator(pdev, quirks);
+		if (ret)
+			goto efree;
+	}
+
 	ver = sd_ctrl_read16(host, CTL_VERSION);
 	/* GEN2_SDR104 is first known SDHI to use 32bit block count */
 	if (ver < SDHI_VER_GEN2_SDR104 && mmc_data->max_blk_count > U16_MAX)
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 422fa63a2e99..ee0aafae9661 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -215,6 +215,120 @@ static const struct renesas_sdhi_quirks sdhi_quirks_rzg2l = {
 	.hs400_disabled = true,
 };
 
+static const unsigned int r9a09g057_vqmmc_voltages[] = {
+	1800000, 3300000,
+};
+
+static int r9a09g057_regulator_disable(struct regulator_dev *rdev)
+{
+	struct tmio_mmc_host *host = rdev_get_drvdata(rdev);
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	sd_status &=  ~SD_STATUS_PWEN;
+	sd_ctrl_write32(host, CTL_SD_STATUS, sd_status);
+
+	return 0;
+}
+
+static int r9a09g057_regulator_enable(struct regulator_dev *rdev)
+{
+	struct tmio_mmc_host *host = rdev_get_drvdata(rdev);
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	sd_status |=  SD_STATUS_PWEN;
+	sd_ctrl_write32(host, CTL_SD_STATUS, sd_status);
+
+	return 0;
+}
+
+static int r9a09g057_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	struct tmio_mmc_host *host = rdev_get_drvdata(rdev);
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	if (sd_status & SD_STATUS_PWEN)
+		return 1;
+
+	return 0;
+}
+
+static int r9a09g057_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	struct tmio_mmc_host *host = rdev_get_drvdata(rdev);
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	if (sd_status & SD_STATUS_IOVS)
+		return 1800000;
+
+	return 3300000;
+}
+
+static int r9a09g057_regulator_set_voltage(struct regulator_dev *rdev,
+					   int min_uV, int max_uV,
+					   unsigned int *selector)
+{
+	struct tmio_mmc_host *host = rdev_get_drvdata(rdev);
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	if (min_uV >= 1700000 && max_uV <= 1950000) {
+		sd_status |=  SD_STATUS_IOVS;
+		*selector = 0;
+	} else {
+		sd_status &=  ~SD_STATUS_IOVS;
+		*selector = 1;
+	}
+	sd_ctrl_write32(host, CTL_SD_STATUS, sd_status);
+
+	return 0;
+}
+
+static int r9a09g057_regulator_list_voltage(struct regulator_dev *rdev,
+					    unsigned selector)
+{
+	if (selector >= ARRAY_SIZE(r9a09g057_vqmmc_voltages))
+		return -EINVAL;
+
+	return r9a09g057_vqmmc_voltages[selector];
+}
+
+static struct regulator_init_data r9a09g057_regulator_init_data = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+
+static const struct regulator_ops r9a09g057_regulator_voltage_ops = {
+	.enable = r9a09g057_regulator_enable,
+	.disable = r9a09g057_regulator_disable,
+	.is_enabled = r9a09g057_regulator_is_enabled,
+	.list_voltage = r9a09g057_regulator_list_voltage,
+	.get_voltage = r9a09g057_regulator_get_voltage,
+	.set_voltage = r9a09g057_regulator_set_voltage,
+	.map_voltage = regulator_map_voltage_ascend,
+};
+
+static struct regulator_desc r9a09g057_vqmmc_regulator = {
+	.of_match	= of_match_ptr("vqmmc-r9a09g057-regulator"),
+	.owner		= THIS_MODULE,
+	.type		= REGULATOR_VOLTAGE,
+	.ops		= &r9a09g057_regulator_voltage_ops,
+	.volt_table	= r9a09g057_vqmmc_voltages,
+	.n_voltages	= ARRAY_SIZE(r9a09g057_vqmmc_voltages),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r9a09g057 = {
+	.fixed_addr_mode = true,
+	.hs400_disabled = true,
+	.internal_regulator = true,
+	.rdesc = &r9a09g057_vqmmc_regulator,
+	.reg_init_data = &r9a09g057_regulator_init_data,
+};
+
 /*
  * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of now.
  * So, we want to treat them equally and only have a match for ES1.2 to enforce
@@ -260,6 +374,11 @@ static const struct renesas_sdhi_of_data_with_quirks of_rzg2l_compatible = {
 	.quirks = &sdhi_quirks_rzg2l,
 };
 
+static const struct renesas_sdhi_of_data_with_quirks of_r9a09g057_compatible = {
+	.of_data = &of_data_rcar_gen3,
+	.quirks = &sdhi_quirks_r9a09g057,
+};
+
 static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen3_compatible = {
 	.of_data = &of_data_rcar_gen3,
 };
@@ -284,6 +403,7 @@ static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = {
 	{ .compatible = "renesas,sdhi-r8a77990", .data = &of_r8a77990_compatible, },
 	{ .compatible = "renesas,sdhi-r8a77995", .data = &of_rcar_gen3_nohs400_compatible, },
 	{ .compatible = "renesas,sdhi-r9a09g011", .data = &of_rzg2l_compatible, },
+	{ .compatible = "renesas,sdhi-r9a09g057", .data = &of_r9a09g057_compatible, },
 	{ .compatible = "renesas,rzg2l-sdhi", .data = &of_rzg2l_compatible, },
 	{ .compatible = "renesas,rcar-gen3-sdhi", .data = &of_rcar_gen3_compatible, },
 	{ .compatible = "renesas,rcar-gen4-sdhi", .data = &of_rcar_gen3_compatible, },
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index de56e6534aea..1a3172786662 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -43,6 +43,7 @@
 #define CTL_RESET_SD 0xe0
 #define CTL_VERSION 0xe2
 #define CTL_SDIF_MODE 0xe6 /* only known on R-Car 2+ */
+#define CTL_SD_STATUS 0xf2 /* only known on RZ/G2L and RZ/V2H(P) */
 
 /* Definitions for values the CTL_STOP_INTERNAL_ACTION register can take */
 #define TMIO_STOP_STP		BIT(0)
@@ -102,6 +103,10 @@
 /* Definitions for values the CTL_SDIF_MODE register can take */
 #define SDIF_MODE_HS400		BIT(0) /* only known on R-Car 2+ */
 
+/* Definitions for values the CTL_SD_STATUS register can take */
+#define SD_STATUS_PWEN		BIT(0) /* only known on RZ/V2H(P) */
+#define SD_STATUS_IOVS		BIT(16) /* only known on RZ/V2H(P) */
+
 /* Define some IRQ masks */
 /* This is the mask used at reset by the chip */
 #define TMIO_MASK_ALL           0x837f031d
-- 
2.34.1

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

[Wolfram Sang]

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

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


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

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

[Lad, Prabhakar]

Hi Wolfram,

Thank you for the review.

On Mon, Jun 17, 2024 at 9:31 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> 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.
>
Agreed.

> > - 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.
>
OK.

> 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).
>
I did give it a try with platform_driver_probe() and failed.

- Firstly I had to move the regulator node outside the SDHI node for
platform_driver_probe() to succeed or else it failed with -ENODEV (at
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953)
- In Renesas SoCs we have multiple instances of SDHI, the problem
being for each instance we are calling platform_driver_probe(). Which
causes a problem as the regulator node will use the first device.

Let me know if I have missed something obvious here.

> 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.
>
Agreed.

> > +#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.
>
I will update it.

> ...
>
> > +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.
>
I will investigate it.

Cheers,
Prabhakar

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

[Wolfram Sang]

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

Hi Prabhakar,

> I did give it a try with platform_driver_probe() and failed.

Ok, thanks for trying nonetheless!

> - Firstly I had to move the regulator node outside the SDHI node for
> platform_driver_probe() to succeed or else it failed with -ENODEV (at
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953)

This makes sense to me because it is just a "regular" regulator.

> - In Renesas SoCs we have multiple instances of SDHI, the problem
> being for each instance we are calling platform_driver_probe(). Which
> causes a problem as the regulator node will use the first device.

I see... we would need a reg property to differentiate between the
internal regulators but that is already used by the parent SDHI node.

Okay, so let's scrap that idea. However, we need to ensure that we can
still have an external regulator. Seeing the bindings, it looks like you
enable the internal regulator with the "vqmmc-r9a09g057-regulator"
property? I wonder now if we can simplify this to an
"use-internal-regulator" property because we have 'compatible' already
to differentiate? Needs advice from DT maintainers, probably.

> Let me know if I have missed something obvious here.

Nope, all good.

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

Thank you.

And happy hacking,

   Wolfram


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

RE: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

[Biju Das]

Hi Wolfram, Prabhakar,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Sent: Thursday, June 20, 2024 8:40 AM
> Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> 
> Hi Prabhakar,
> 
> > I did give it a try with platform_driver_probe() and failed.
> 
> Ok, thanks for trying nonetheless!
> 
> > - Firstly I had to move the regulator node outside the SDHI node for
> > platform_driver_probe() to succeed or else it failed with -ENODEV (at
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c
> > #L953)
> 
> This makes sense to me because it is just a "regular" regulator.
> 
> > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > being for each instance we are calling platform_driver_probe(). Which
> > causes a problem as the regulator node will use the first device.
> 
> I see... we would need a reg property to differentiate between the internal regulators but that is
> already used by the parent SDHI node.
> 
> Okay, so let's scrap that idea. However, we need to ensure that we can still have an external
> regulator. Seeing the bindings, it looks like you enable the internal regulator with the "vqmmc-
> r9a09g057-regulator"
> property? I wonder now if we can simplify this to an "use-internal-regulator" property because we
> have 'compatible' already to differentiate? Needs advice from DT maintainers, probably.

Why this cannot be modelled as a regular "regulator" as a child device of SDHI device?

See [1] and [2]

[1]
https://lore.kernel.org/linux-renesas-soc/20240616105402.45211-1-biju.das.jz@bp.renesas.com/

[2]
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/vqmmc-ipq4019-regulator.c

Cheers,
Biju

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

[Lad, Prabhakar]

Hi Biju,

On Thu, Jun 20, 2024 at 10:30 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Wolfram, Prabhakar,
>
> > -----Original Message-----
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Sent: Thursday, June 20, 2024 8:40 AM
> > Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> >
> > Hi Prabhakar,
> >
> > > I did give it a try with platform_driver_probe() and failed.
> >
> > Ok, thanks for trying nonetheless!
> >
> > > - Firstly I had to move the regulator node outside the SDHI node for
> > > platform_driver_probe() to succeed or else it failed with -ENODEV (at
> > > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c
> > > #L953)
> >
> > This makes sense to me because it is just a "regular" regulator.
> >
> > > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > > being for each instance we are calling platform_driver_probe(). Which
> > > causes a problem as the regulator node will use the first device.
> >
> > I see... we would need a reg property to differentiate between the internal regulators but that is
> > already used by the parent SDHI node.
> >
> > Okay, so let's scrap that idea. However, we need to ensure that we can still have an external
> > regulator. Seeing the bindings, it looks like you enable the internal regulator with the "vqmmc-
> > r9a09g057-regulator"
> > property? I wonder now if we can simplify this to an "use-internal-regulator" property because we
> > have 'compatible' already to differentiate? Needs advice from DT maintainers, probably.
>
> Why this cannot be modelled as a regular "regulator" as a child device of SDHI device?
>
The current implementation does implement the regulator as a child
device of the sdhi node [0] itself.

Wolfram was suggesting to have the regulator outside and use
platform_driver_probe(), which caused an issue as mentioned above.

[0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240613091721.525266-2-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

RE: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

[Biju Das]

Hi Prabhakar,

> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> 
> Hi Biju,
> 
> On Thu, Jun 20, 2024 at 10:30 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Wolfram, Prabhakar,
> >
> > > -----Original Message-----
> > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > Sent: Thursday, June 20, 2024 8:40 AM
> > > Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for
> > > RZ/V2H(P) SoC
> > >
> > > Hi Prabhakar,
> > >
> > > > I did give it a try with platform_driver_probe() and failed.
> > >
> > > Ok, thanks for trying nonetheless!
> > >
> > > > - Firstly I had to move the regulator node outside the SDHI node
> > > > for
> > > > platform_driver_probe() to succeed or else it failed with -ENODEV
> > > > (at
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/platfo
> > > > rm.c
> > > > #L953)
> > >
> > > This makes sense to me because it is just a "regular" regulator.
> > >
> > > > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > > > being for each instance we are calling platform_driver_probe().
> > > > Which causes a problem as the regulator node will use the first device.
> > >
> > > I see... we would need a reg property to differentiate between the
> > > internal regulators but that is already used by the parent SDHI node.
> > >
> > > Okay, so let's scrap that idea. However, we need to ensure that we
> > > can still have an external regulator. Seeing the bindings, it looks
> > > like you enable the internal regulator with the "vqmmc- r9a09g057-regulator"
> > > property? I wonder now if we can simplify this to an
> > > "use-internal-regulator" property because we have 'compatible' already to differentiate? Needs
> advice from DT maintainers, probably.
> >
> > Why this cannot be modelled as a regular "regulator" as a child device of SDHI device?
> >
> The current implementation does implement the regulator as a child device of the sdhi node [0]
> itself.
> 
> Wolfram was suggesting to have the regulator outside and use platform_driver_probe(), which caused
> an issue as mentioned above.

You, mean standalone node with a device compatible for each SDHI device nodes(Assuming 3 sdhi devices)?

3 SDHI devices nodes(stand alone) + 3 regulator device nodes (stand alone) ?

Or

3 SDHI devices nodes(stand alone) + 1 regulator device node(stand alone)


Cheers,
Biju

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

[Lad, Prabhakar]

Hi Biju,

On Thu, Jun 20, 2024 at 10:49 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> >
> > Hi Biju,
> >
> > On Thu, Jun 20, 2024 at 10:30 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Wolfram, Prabhakar,
> > >
> > > > -----Original Message-----
> > > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > > Sent: Thursday, June 20, 2024 8:40 AM
> > > > Subject: Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for
> > > > RZ/V2H(P) SoC
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > > I did give it a try with platform_driver_probe() and failed.
> > > >
> > > > Ok, thanks for trying nonetheless!
> > > >
> > > > > - Firstly I had to move the regulator node outside the SDHI node
> > > > > for
> > > > > platform_driver_probe() to succeed or else it failed with -ENODEV
> > > > > (at
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/platfo
> > > > > rm.c
> > > > > #L953)
> > > >
> > > > This makes sense to me because it is just a "regular" regulator.
> > > >
> > > > > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > > > > being for each instance we are calling platform_driver_probe().
> > > > > Which causes a problem as the regulator node will use the first device.
> > > >
> > > > I see... we would need a reg property to differentiate between the
> > > > internal regulators but that is already used by the parent SDHI node.
> > > >
> > > > Okay, so let's scrap that idea. However, we need to ensure that we
> > > > can still have an external regulator. Seeing the bindings, it looks
> > > > like you enable the internal regulator with the "vqmmc- r9a09g057-regulator"
> > > > property? I wonder now if we can simplify this to an
> > > > "use-internal-regulator" property because we have 'compatible' already to differentiate? Needs
> > advice from DT maintainers, probably.
> > >
> > > Why this cannot be modelled as a regular "regulator" as a child device of SDHI device?
> > >
> > The current implementation does implement the regulator as a child device of the sdhi node [0]
> > itself.
> >
> > Wolfram was suggesting to have the regulator outside and use platform_driver_probe(), which caused
> > an issue as mentioned above.
>
> You, mean standalone node with a device compatible for each SDHI device nodes(Assuming 3 sdhi devices)?
>
Yep.

> 3 SDHI devices nodes(stand alone) + 3 regulator device nodes (stand alone) ?
>
This one (since as per the HW we have three SDHI instances and 3
internal regulators) so we need to describe the same in DT.

Cheers,
Prabhakar

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

[Lad, Prabhakar]

Hi Wolfram,

On Thu, Jun 20, 2024 at 8:39 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Prabhakar,
>
> > I did give it a try with platform_driver_probe() and failed.
>
> Ok, thanks for trying nonetheless!
>
> > - Firstly I had to move the regulator node outside the SDHI node for
> > platform_driver_probe() to succeed or else it failed with -ENODEV (at
> > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L953)
>
> This makes sense to me because it is just a "regular" regulator.
>
OK.

> > - In Renesas SoCs we have multiple instances of SDHI, the problem
> > being for each instance we are calling platform_driver_probe(). Which
> > causes a problem as the regulator node will use the first device.
>
> I see... we would need a reg property to differentiate between the
> internal regulators but that is already used by the parent SDHI node.
>
> Okay, so let's scrap that idea. However, we need to ensure that we can
> still have an external regulator. Seeing the bindings, it looks like you
> enable the internal regulator with the "vqmmc-r9a09g057-regulator"
> property? I wonder now if we can simplify this to an
> "use-internal-regulator" property because we have 'compatible' already
> to differentiate? Needs advice from DT maintainers, probably.
>

Based on the feedback from Rob I have now changed it to below, i.e.
the regulator now probes based on regulator-compatible property value
"vqmmc-r9a09g057-regulator" instead of regulator node name as the
driver has of_match in regulator_desc.

static struct regulator_desc r9a09g057_vqmmc_regulator = {
    .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
    .owner        = THIS_MODULE,
    .type        = REGULATOR_VOLTAGE,
    .ops        = &r9a09g057_regulator_voltage_ops,
    .volt_table    = r9a09g057_vqmmc_voltages,
    .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
};

SoC DTSI:
        sdhi1: mmc@15c10000 {
            compatible = "renesas,sdhi-r9a09g057";
            reg = <0x0 0x15c10000 0 0x10000>;
            interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
            clocks = <&cpg CPG_MOD 167>,
                 <&cpg CPG_MOD 169>,
                 <&cpg CPG_MOD 168>,
                 <&cpg CPG_MOD 170>;
            clock-names = "core", "clkh", "cd", "aclk";
            resets = <&cpg 168>;
            power-domains = <&cpg>;
            status = "disabled";

            vqmmc_sdhi1: vqmmc-regulator {
                regulator-compatible = "vqmmc-r9a09g057-regulator";
                regulator-name = "vqmmc-regulator";
                regulator-min-microvolt = <1800000>;
                regulator-max-microvolt = <3300000>;
                status = "disabled";
            };
        };

Board DTS:

&sdhi1 {
    pinctrl-0 = <&sdhi1_pins>;
    pinctrl-1 = <&sdhi1_pins>;
    pinctrl-names = "default", "state_uhs";
    vmmc-supply = <&reg_3p3v>;
    vqmmc-supply = <&vqmmc_sdhi1>;
    bus-width = <4>;
    sd-uhs-sdr50;
    sd-uhs-sdr104;
    status = "okay";
};

&vqmmc_sdhi1 {
    status = "okay";
};

Based on the feedback provided Geert ie to use set_pwr callback to set
PWEN bit and handle IOVS bit in voltage switch callback by dropping
the regulator altogether. In this case we will have to introduce just
a single "use-internal-regulator" property and if set make the vqmmc
regulator optional?

Let me know your thoughts.

> > Let me know if I have missed something obvious here.
>
> Nope, all good.
>
sigh..

Cheers,
Prabhakar

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

[Wolfram Sang]

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

Hi Prabhakar,

> Based on the feedback from Rob I have now changed it to below, i.e.
> the regulator now probes based on regulator-compatible property value
> "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> driver has of_match in regulator_desc.

I like this a lot! One minor comment.

> static struct regulator_desc r9a09g057_vqmmc_regulator = {
>     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
>     .owner        = THIS_MODULE,
>     .type        = REGULATOR_VOLTAGE,
>     .ops        = &r9a09g057_regulator_voltage_ops,
>     .volt_table    = r9a09g057_vqmmc_voltages,
>     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> };
> 
> SoC DTSI:
>         sdhi1: mmc@15c10000 {
>             compatible = "renesas,sdhi-r9a09g057";
>             reg = <0x0 0x15c10000 0 0x10000>;
>             interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
>                      <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
>             clocks = <&cpg CPG_MOD 167>,
>                  <&cpg CPG_MOD 169>,
>                  <&cpg CPG_MOD 168>,
>                  <&cpg CPG_MOD 170>;
>             clock-names = "core", "clkh", "cd", "aclk";
>             resets = <&cpg 168>;
>             power-domains = <&cpg>;
>             status = "disabled";
> 
>             vqmmc_sdhi1: vqmmc-regulator {
>                 regulator-compatible = "vqmmc-r9a09g057-regulator";
>                 regulator-name = "vqmmc-regulator";

This should have "sdhi<X>" somewhere in the name?

>                 regulator-min-microvolt = <1800000>;
>                 regulator-max-microvolt = <3300000>;
>                 status = "disabled";
>             };
>         };
> 
> Board DTS:
> 
> &sdhi1 {
>     pinctrl-0 = <&sdhi1_pins>;
>     pinctrl-1 = <&sdhi1_pins>;
>     pinctrl-names = "default", "state_uhs";
>     vmmc-supply = <&reg_3p3v>;
>     vqmmc-supply = <&vqmmc_sdhi1>;
>     bus-width = <4>;
>     sd-uhs-sdr50;
>     sd-uhs-sdr104;
>     status = "okay";
> };
> 
> &vqmmc_sdhi1 {
>     status = "okay";
> };

Again, I like this. It looks like proper HW description to me.

> Based on the feedback provided Geert ie to use set_pwr callback to set
> PWEN bit and handle IOVS bit in voltage switch callback by dropping
> the regulator altogether. In this case we will have to introduce just
> a single "use-internal-regulator" property and if set make the vqmmc
> regulator optional?

Let's discuss with Geert. But I am quite convinced of your approach
above.

> > > Let me know if I have missed something obvious here.
> >
> > Nope, all good.

Don't give up, I think we are close...

All the best,

   Wolfram


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

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

[Geert Uytterhoeven]

Hi all,

On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Based on the feedback from Rob I have now changed it to below, i.e.
> > the regulator now probes based on regulator-compatible property value
> > "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> > driver has of_match in regulator_desc.
>
> I like this a lot! One minor comment.
>
> > static struct regulator_desc r9a09g057_vqmmc_regulator = {
> >     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
> >     .owner        = THIS_MODULE,
> >     .type        = REGULATOR_VOLTAGE,
> >     .ops        = &r9a09g057_regulator_voltage_ops,
> >     .volt_table    = r9a09g057_vqmmc_voltages,
> >     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> > };
> >
> > SoC DTSI:
> >         sdhi1: mmc@15c10000 {
> >             compatible = "renesas,sdhi-r9a09g057";
> >             reg = <0x0 0x15c10000 0 0x10000>;
> >             interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
> >                      <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
> >             clocks = <&cpg CPG_MOD 167>,
> >                  <&cpg CPG_MOD 169>,
> >                  <&cpg CPG_MOD 168>,
> >                  <&cpg CPG_MOD 170>;
> >             clock-names = "core", "clkh", "cd", "aclk";
> >             resets = <&cpg 168>;
> >             power-domains = <&cpg>;
> >             status = "disabled";
> >
> >             vqmmc_sdhi1: vqmmc-regulator {
> >                 regulator-compatible = "vqmmc-r9a09g057-regulator";

renesas,r9a09g057-vqmmc-regulator?

> >                 regulator-name = "vqmmc-regulator";
>
> This should have "sdhi<X>" somewhere in the name?

Indeed.

>
> >                 regulator-min-microvolt = <1800000>;
> >                 regulator-max-microvolt = <3300000>;
> >                 status = "disabled";
> >             };
> >         };
> >
> > Board DTS:
> >
> > &sdhi1 {
> >     pinctrl-0 = <&sdhi1_pins>;
> >     pinctrl-1 = <&sdhi1_pins>;
> >     pinctrl-names = "default", "state_uhs";
> >     vmmc-supply = <&reg_3p3v>;
> >     vqmmc-supply = <&vqmmc_sdhi1>;
> >     bus-width = <4>;
> >     sd-uhs-sdr50;
> >     sd-uhs-sdr104;
> >     status = "okay";
> > };
> >
> > &vqmmc_sdhi1 {
> >     status = "okay";
> > };
>
> Again, I like this. It looks like proper HW description to me.
>
> > Based on the feedback provided Geert ie to use set_pwr callback to set
> > PWEN bit and handle IOVS bit in voltage switch callback by dropping
> > the regulator altogether. In this case we will have to introduce just
> > a single "use-internal-regulator" property and if set make the vqmmc
> > regulator optional?
>
> Let's discuss with Geert. But I am quite convinced of your approach
> above.
>
> > > > Let me know if I have missed something obvious here.
> > >
> > > Nope, all good.
>
> Don't give up, I think we are close...

The above indeed starts looking good to me.
IIUIC, the use of the special vqmmc-r9a09g057-regulator is still
optional, and the subnode can be left disabled? E.g. the board
designer may still use a (different) GPIO to control the regulator,
using "regulator-gpio"?

Which brings me to another question: as both the SDmIOVS and SDmPWEN
pins can be configured as GPIOs, why not ignore the special handling
using the SDm_SD_STATUS register, and use "regulator-gpio" instead?
We usually do the same for CD/WP, using "{cd,wp}-gpios" instead.
Exceptions are old SH/R-Mobile and R-Car Gen1 boards:

  arch/arm/boot/dts/renesas/r8a73a4-ape6evm.dts:          groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
  arch/arm/boot/dts/renesas/r8a7740-armadillo800eva.dts:
groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
  arch/arm/boot/dts/renesas/r8a7778-bockw.dts:            groups =
"sdhi0_cd", "sdhi0_wp";
  arch/arm/boot/dts/renesas/r8a7779-marzen.dts:           groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
  arch/arm/boot/dts/renesas/sh73a0-kzm9g.dts:             groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Lad, Prabhakar]

Hi Geert,

On Fri, Jun 21, 2024 at 12:58 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi all,
>
> On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > > Based on the feedback from Rob I have now changed it to below, i.e.
> > > the regulator now probes based on regulator-compatible property value
> > > "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> > > driver has of_match in regulator_desc.
> >
> > I like this a lot! One minor comment.
> >
> > > static struct regulator_desc r9a09g057_vqmmc_regulator = {
> > >     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
> > >     .owner        = THIS_MODULE,
> > >     .type        = REGULATOR_VOLTAGE,
> > >     .ops        = &r9a09g057_regulator_voltage_ops,
> > >     .volt_table    = r9a09g057_vqmmc_voltages,
> > >     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> > > };
> > >
> > > SoC DTSI:
> > >         sdhi1: mmc@15c10000 {
> > >             compatible = "renesas,sdhi-r9a09g057";
> > >             reg = <0x0 0x15c10000 0 0x10000>;
> > >             interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
> > >                      <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
> > >             clocks = <&cpg CPG_MOD 167>,
> > >                  <&cpg CPG_MOD 169>,
> > >                  <&cpg CPG_MOD 168>,
> > >                  <&cpg CPG_MOD 170>;
> > >             clock-names = "core", "clkh", "cd", "aclk";
> > >             resets = <&cpg 168>;
> > >             power-domains = <&cpg>;
> > >             status = "disabled";
> > >
> > >             vqmmc_sdhi1: vqmmc-regulator {
> > >                 regulator-compatible = "vqmmc-r9a09g057-regulator";
>
> renesas,r9a09g057-vqmmc-regulator?
>
> > >                 regulator-name = "vqmmc-regulator";
> >
> > This should have "sdhi<X>" somewhere in the name?
>
> Indeed.
>
> >
> > >                 regulator-min-microvolt = <1800000>;
> > >                 regulator-max-microvolt = <3300000>;
> > >                 status = "disabled";
> > >             };
> > >         };
> > >
> > > Board DTS:
> > >
> > > &sdhi1 {
> > >     pinctrl-0 = <&sdhi1_pins>;
> > >     pinctrl-1 = <&sdhi1_pins>;
> > >     pinctrl-names = "default", "state_uhs";
> > >     vmmc-supply = <&reg_3p3v>;
> > >     vqmmc-supply = <&vqmmc_sdhi1>;
> > >     bus-width = <4>;
> > >     sd-uhs-sdr50;
> > >     sd-uhs-sdr104;
> > >     status = "okay";
> > > };
> > >
> > > &vqmmc_sdhi1 {
> > >     status = "okay";
> > > };
> >
> > Again, I like this. It looks like proper HW description to me.
> >
> > > Based on the feedback provided Geert ie to use set_pwr callback to set
> > > PWEN bit and handle IOVS bit in voltage switch callback by dropping
> > > the regulator altogether. In this case we will have to introduce just
> > > a single "use-internal-regulator" property and if set make the vqmmc
> > > regulator optional?
> >
> > Let's discuss with Geert. But I am quite convinced of your approach
> > above.
> >
> > > > > Let me know if I have missed something obvious here.
> > > >
> > > > Nope, all good.
> >
> > Don't give up, I think we are close...
>
> The above indeed starts looking good to me.
> IIUIC, the use of the special vqmmc-r9a09g057-regulator is still
> optional, and the subnode can be left disabled? E.g. the board
> designer may still use a (different) GPIO to control the regulator,
> using "regulator-gpio"?
>
> Which brings me to another question: as both the SDmIOVS and SDmPWEN
> pins can be configured as GPIOs, why not ignore the special handling
> using the SDm_SD_STATUS register, and use "regulator-gpio" instead?
> We usually do the same for CD/WP, using "{cd,wp}-gpios" instead.
> Exceptions are old SH/R-Mobile and R-Car Gen1 boards:
>
>   arch/arm/boot/dts/renesas/r8a73a4-ape6evm.dts:          groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/r8a7740-armadillo800eva.dts:
> groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7778-bockw.dts:            groups =
> "sdhi0_cd", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7779-marzen.dts:           groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/sh73a0-kzm9g.dts:             groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";
>
Indeed this can be done on RZ/V2H(P) SoC, the future upcoming SoCs may
not have an option for this In this case we will have to use the
internal regulator.

Let me know your thoughts on what approach we take for RZ/V2H(P) SoC.

Cheers,
Prabhakar

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

[Lad, Prabhakar]

Hi All,

On Fri, Jun 21, 2024 at 12:58 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi all,
>
> On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > > Based on the feedback from Rob I have now changed it to below, i.e.
> > > the regulator now probes based on regulator-compatible property value
> > > "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> > > driver has of_match in regulator_desc.
> >
> > I like this a lot! One minor comment.
> >
> > > static struct regulator_desc r9a09g057_vqmmc_regulator = {
> > >     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
> > >     .owner        = THIS_MODULE,
> > >     .type        = REGULATOR_VOLTAGE,
> > >     .ops        = &r9a09g057_regulator_voltage_ops,
> > >     .volt_table    = r9a09g057_vqmmc_voltages,
> > >     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> > > };
> > >
> > > SoC DTSI:
> > >         sdhi1: mmc@15c10000 {
> > >             compatible = "renesas,sdhi-r9a09g057";
> > >             reg = <0x0 0x15c10000 0 0x10000>;
> > >             interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
> > >                      <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
> > >             clocks = <&cpg CPG_MOD 167>,
> > >                  <&cpg CPG_MOD 169>,
> > >                  <&cpg CPG_MOD 168>,
> > >                  <&cpg CPG_MOD 170>;
> > >             clock-names = "core", "clkh", "cd", "aclk";
> > >             resets = <&cpg 168>;
> > >             power-domains = <&cpg>;
> > >             status = "disabled";
> > >
> > >             vqmmc_sdhi1: vqmmc-regulator {
> > >                 regulator-compatible = "vqmmc-r9a09g057-regulator";
>
> renesas,r9a09g057-vqmmc-regulator?
>
> > >                 regulator-name = "vqmmc-regulator";
> >
> > This should have "sdhi<X>" somewhere in the name?
>
> Indeed.
>
> >
> > >                 regulator-min-microvolt = <1800000>;
> > >                 regulator-max-microvolt = <3300000>;
> > >                 status = "disabled";
> > >             };
> > >         };
> > >
> > > Board DTS:
> > >
> > > &sdhi1 {
> > >     pinctrl-0 = <&sdhi1_pins>;
> > >     pinctrl-1 = <&sdhi1_pins>;
> > >     pinctrl-names = "default", "state_uhs";
> > >     vmmc-supply = <&reg_3p3v>;
> > >     vqmmc-supply = <&vqmmc_sdhi1>;
> > >     bus-width = <4>;
> > >     sd-uhs-sdr50;
> > >     sd-uhs-sdr104;
> > >     status = "okay";
> > > };
> > >
> > > &vqmmc_sdhi1 {
> > >     status = "okay";
> > > };
> >
> > Again, I like this. It looks like proper HW description to me.
> >
> > > Based on the feedback provided Geert ie to use set_pwr callback to set
> > > PWEN bit and handle IOVS bit in voltage switch callback by dropping
> > > the regulator altogether. In this case we will have to introduce just
> > > a single "use-internal-regulator" property and if set make the vqmmc
> > > regulator optional?
> >
> > Let's discuss with Geert. But I am quite convinced of your approach
> > above.
> >
> > > > > Let me know if I have missed something obvious here.
> > > >
> > > > Nope, all good.
> >
> > Don't give up, I think we are close...
>
> The above indeed starts looking good to me.
> IIUIC, the use of the special vqmmc-r9a09g057-regulator is still
> optional, and the subnode can be left disabled? E.g. the board
> designer may still use a (different) GPIO to control the regulator,
> using "regulator-gpio"?
>
> Which brings me to another question: as both the SDmIOVS and SDmPWEN
> pins can be configured as GPIOs, why not ignore the special handling
> using the SDm_SD_STATUS register, and use "regulator-gpio" instead?
> We usually do the same for CD/WP, using "{cd,wp}-gpios" instead.
> Exceptions are old SH/R-Mobile and R-Car Gen1 boards:
>
>   arch/arm/boot/dts/renesas/r8a73a4-ape6evm.dts:          groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/r8a7740-armadillo800eva.dts:
> groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7778-bockw.dts:            groups =
> "sdhi0_cd", "sdhi0_wp";
>   arch/arm/boot/dts/renesas/r8a7779-marzen.dts:           groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
>   arch/arm/boot/dts/renesas/sh73a0-kzm9g.dts:             groups =
> "sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";
>
Based on the special handling required to handle the IOVS and PWEN pin
by bypassing the core regulator by function pointers in v4 [0] doesn't
seem an elegant solution.

On the RZ/V2H SoC IOVS and PWEN pins can be used as GPIO and a similar
approach has been used on the other Renesas SoCs. I will withhold
internal regulator support for RZ/V2H SoC and fallback to GPIOs.

[0] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240626132341.342963-4-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

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

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

On Thu, Jun 13, 2024 at 11:17 AM Prabhakar <prabhakar.csengg@gmail.com> 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.
>
> regulator support is added to control the volatage levels of SD pins
> via sd_iovs/sd_pwen bits in SD_STATUS register.

Probably I am missing something obvious in the big picture, but why
must this be modelled as a regulator?  Can't the SDHI driver handle
this register bit directly?

Cfr. tmio_mmc_power_on(), which can use the tmio_mmc_host.set_pwr()
callback[1] instead of/in addition to a regulator.

Thanks!

[1] Oh, no more users... Let's get rid of it... patch sent...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Lad, Prabhakar]

Hi Geert,

On Thu, Jun 20, 2024 at 4:40 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Thu, Jun 13, 2024 at 11:17 AM Prabhakar <prabhakar.csengg@gmail.com> 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.
> >
> > regulator support is added to control the volatage levels of SD pins
> > via sd_iovs/sd_pwen bits in SD_STATUS register.
>
> Probably I am missing something obvious in the big picture, but why
> must this be modelled as a regulator?  Can't the SDHI driver handle
> this register bit directly?
>
It can be handled directly. I had asked for suggestions on how to
implement this ("Subject: Modeling the register bit as a voltage
regulator for SDHI/eMMC '' also CC'd you), based on the feedback below
from Wolfram I took this approach.

> There is a similar instance of regulator driver [1] which is
> controlled via register bit write, but in our case the SD_STATUS
> register is part of the SDHI IP block itself.

... I could imagine that the SDHI driver itself exposes a regulator
driver. Just without a <reg>-property. The compatible will induce which
register and bit to use.


> Cfr. tmio_mmc_power_on(), which can use the tmio_mmc_host.set_pwr()
> callback[1] instead of/in addition to a regulator.
>
PWEN bit would have been controlled via set_pwr()

Cheers,
Prabhakar

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

[Wolfram Sang]

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


> > There is a similar instance of regulator driver [1] which is
> > controlled via register bit write, but in our case the SD_STATUS
> > register is part of the SDHI IP block itself.
> 
> ... I could imagine that the SDHI driver itself exposes a regulator
> driver. Just without a <reg>-property. The compatible will induce which
> register and bit to use.

That would mean we have a compatible-entry per SDHI instance per SoC?
Did I get this right? I think that will be too many compatibles.

What is the drawback of using an "internal-regulator" property within
the SDHI node?


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

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

[Wolfram Sang]

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


> That would mean we have a compatible-entry per SDHI instance per SoC?
> Did I get this right? I think that will be too many compatibles.
> 
> What is the drawback of using an "internal-regulator" property within
> the SDHI node?

Ah, I found the other thread now. Will have a look.


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

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

[Wolfram Sang]

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


> Probably I am missing something obvious in the big picture, but why
> must this be modelled as a regulator?  Can't the SDHI driver handle
> this register bit directly?

I suggested it because we already use external regulators with SDHI. So,
I preferred the design where the internal regulator was just another
regulator. Then, the SDHI core can just keep using regulator API. And
not have two code paths for internal vs. external. Did I miss something?


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