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

[RFC PATCH 0/4] 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.

Sending this as an RFC series as,
- New regulator API is introduced
- Regulator support is added in renesas_sdhi driver

Cheers,
Prabhakar

Lad Prabhakar (4):
  regulator: core: Ensure the cached state matches the hardware state in
    regulator_set_voltage_unlocked()
  regulator: core: Add regulator_map_voltage_descend() API
  dt-bindings: mmc: renesas,sdhi: Document RZ/V2H(P) support
  mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

 .../devicetree/bindings/mmc/renesas,sdhi.yaml | 20 +++++-
 drivers/mmc/host/renesas_sdhi.h               |  7 ++
 drivers/mmc/host/renesas_sdhi_core.c          | 67 +++++++++++++++++--
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++
 drivers/mmc/host/tmio_mmc.h                   |  4 ++
 drivers/regulator/core.c                      |  9 ++-
 drivers/regulator/helpers.c                   | 31 +++++++++
 include/linux/regulator/driver.h              |  2 +
 8 files changed, 176 insertions(+), 9 deletions(-)

-- 
2.34.1

[RFC PATCH 1/4] regulator: core: Ensure the cached state matches the hardware state in regulator_set_voltage_unlocked()

[Prabhakar]

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

Ensure that the cached state matches the hardware setting before
considering it a no-op in regulator_set_voltage_unlocked().

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Driver code flow:
1> set regulator to 1.8V (BIT0 = 1)
2> Regulator cached state now will be 1.8V
3> Now for some reason driver issues a reset to the IP block
   which resets the registers to default value. In this process
   the regulator is set to 3.3V (BIT0 = 0)
4> Now the driver requests the regulator core to set 1.8V
5> Due to below check of cached state we return back with success
   resulting undesired behaviour.
   
Hence an additional check is introduced to make sure the cache state
is matching with the HW.
---
 drivers/regulator/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5794f4e9dd52..65ee54b13428 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3765,10 +3765,13 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 	/* If we're setting the same range as last time the change
 	 * should be a noop (some cpufreq implementations use the same
-	 * voltage for multiple frequencies, for example).
+	 * voltage for multiple frequencies, for example). Also make sure
+	 * state is the same in HW.
 	 */
-	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV)
-		goto out;
+	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV) {
+		if (regulator_get_voltage_rdev(rdev) == min_uV)
+			goto out;
+	}
 
 	/* If we're trying to set a range that overlaps the current voltage,
 	 * return successfully even though the regulator does not support
-- 
2.34.1

Re: [RFC PATCH 1/4] regulator: core: Ensure the cached state matches the hardware state in regulator_set_voltage_unlocked()

[Mark Brown]

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

On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:

> Driver code flow:
> 1> set regulator to 1.8V (BIT0 = 1)
> 2> Regulator cached state now will be 1.8V
> 3> Now for some reason driver issues a reset to the IP block
>    which resets the registers to default value. In this process
>    the regulator is set to 3.3V (BIT0 = 0)
> 4> Now the driver requests the regulator core to set 1.8V

If something is resetting the regulator like this that's a problem in
general, we need to either have the driver notify the core when that
happens so it can reconfigure the regulator or have it reapply
configuration directly.  Obviously it's not great to have that happen at
all...

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

Re: [RFC PATCH 1/4] regulator: core: Ensure the cached state matches the hardware state in regulator_set_voltage_unlocked()

[Lad, Prabhakar]

Hi Mark,

On Thu, Jun 6, 2024 at 1:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:
>
> > Driver code flow:
> > 1> set regulator to 1.8V (BIT0 = 1)
> > 2> Regulator cached state now will be 1.8V
> > 3> Now for some reason driver issues a reset to the IP block
> >    which resets the registers to default value. In this process
> >    the regulator is set to 3.3V (BIT0 = 0)
> > 4> Now the driver requests the regulator core to set 1.8V
>
> If something is resetting the regulator like this that's a problem in
> general, we need to either have the driver notify the core when that
> happens so it can reconfigure the regulator or have it reapply
> configuration directly.  Obviously it's not great to have that happen at
> all...
>
Currently I am seeing this problem with SDHI driver. For the voltage
switch operation the MMC core requests the driver to do the change and
similarly the MMC core requests the reset operation.

> Having the core driver notify the core when that happens so it can reconfigure the regulator or have it reapply configuration directly.
Again doing this would be a problem as MMC core also maintains the IOS
states, reconfiguring the regulator would cause conflicts between the
states.

Ulf/Wolfram - please correct me if I'm wrong here.

Cheers,
Prabhakar

Re: [RFC PATCH 1/4] regulator: core: Ensure the cached state matches the hardware state in regulator_set_voltage_unlocked()

[Mark Brown]

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

On Thu, Jun 06, 2024 at 03:12:42PM +0100, Lad, Prabhakar wrote:
> On Thu, Jun 6, 2024 at 1:05 PM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jun 05, 2024 at 08:49:33AM +0100, Prabhakar wrote:

> > > Driver code flow:
> > > 1> set regulator to 1.8V (BIT0 = 1)
> > > 2> Regulator cached state now will be 1.8V
> > > 3> Now for some reason driver issues a reset to the IP block
> > >    which resets the registers to default value. In this process
> > >    the regulator is set to 3.3V (BIT0 = 0)
> > > 4> Now the driver requests the regulator core to set 1.8V

> > If something is resetting the regulator like this that's a problem in
> > general, we need to either have the driver notify the core when that
> > happens so it can reconfigure the regulator or have it reapply
> > configuration directly.  Obviously it's not great to have that happen at
> > all...

> Currently I am seeing this problem with SDHI driver. For the voltage
> switch operation the MMC core requests the driver to do the change and
> similarly the MMC core requests the reset operation.

> > Having the core driver notify the core when that happens so it can reconfigure the regulator or have it reapply configuration directly.

> Again doing this would be a problem as MMC core also maintains the IOS
> states, reconfiguring the regulator would cause conflicts between the
> states.

If the device can't cope with the requested configuration being applied
why is this going through the regulator API at all?  This just seems
quite confused, putting a bodge in the core like this clearly isn't the
right solution.

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

[RFC PATCH 2/4] regulator: core: Add regulator_map_voltage_descend() API

[Prabhakar]

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

Similarly to regulator_map_voltage_ascend() api add
regulator_map_voltage_descend() api and export it.

Drivers that have descendant voltage list can use this as their
map_voltage() operation.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/regulator/helpers.c      | 31 +++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |  2 ++
 2 files changed, 33 insertions(+)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index 6e1ace660b8c..ac62d778e3c0 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -368,6 +368,37 @@ int regulator_map_voltage_ascend(struct regulator_dev *rdev,
 }
 EXPORT_SYMBOL_GPL(regulator_map_voltage_ascend);
 
+/**
+ * regulator_map_voltage_descend - map_voltage() for descendant voltage list
+ *
+ * @rdev: Regulator to operate on
+ * @min_uV: Lower bound for voltage
+ * @max_uV: Upper bound for voltage
+ *
+ * Drivers that have descendant voltage list can use this as their
+ * map_voltage() operation.
+ */
+int regulator_map_voltage_descend(struct regulator_dev *rdev,
+				  int min_uV, int max_uV)
+{
+	int i, ret;
+
+	for (i = rdev->desc->n_voltages - 1; i >= 0 ; i--) {
+		ret = rdev->desc->ops->list_voltage(rdev, i);
+		if (ret < 0)
+			continue;
+
+		if (ret > min_uV)
+			break;
+
+		if (ret >= min_uV && ret <= max_uV)
+			return i;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(regulator_map_voltage_descend);
+
 /**
  * regulator_map_voltage_linear - map_voltage() for simple linear mappings
  *
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index f230a472ccd3..6410c57ba85a 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -735,6 +735,8 @@ int regulator_map_voltage_iterate(struct regulator_dev *rdev,
 				  int min_uV, int max_uV);
 int regulator_map_voltage_ascend(struct regulator_dev *rdev,
 				  int min_uV, int max_uV);
+int regulator_map_voltage_descend(struct regulator_dev *rdev,
+				  int min_uV, int max_uV);
 int regulator_get_voltage_sel_pickable_regmap(struct regulator_dev *rdev);
 int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
 						unsigned int sel);
-- 
2.34.1

Re: [RFC PATCH 2/4] regulator: core: Add regulator_map_voltage_descend() API

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, Jun 5, 2024 at 9:49 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Similarly to regulator_map_voltage_ascend() api add
> regulator_map_voltage_descend() api and export it.
>
> Drivers that have descendant voltage list can use this as their
> map_voltage() operation.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/regulator/helpers.c
> +++ b/drivers/regulator/helpers.c
> @@ -368,6 +368,37 @@ int regulator_map_voltage_ascend(struct regulator_dev *rdev,
>  }
>  EXPORT_SYMBOL_GPL(regulator_map_voltage_ascend);
>
> +/**
> + * regulator_map_voltage_descend - map_voltage() for descendant voltage list
> + *
> + * @rdev: Regulator to operate on
> + * @min_uV: Lower bound for voltage
> + * @max_uV: Upper bound for voltage
> + *
> + * Drivers that have descendant voltage list can use this as their
> + * map_voltage() operation.
> + */
> +int regulator_map_voltage_descend(struct regulator_dev *rdev,
> +                                 int min_uV, int max_uV)
> +{
> +       int i, ret;
> +
> +       for (i = rdev->desc->n_voltages - 1; i >= 0 ; i--) {
> +               ret = rdev->desc->ops->list_voltage(rdev, i);
> +               if (ret < 0)
> +                       continue;
> +
> +               if (ret > min_uV)

I know this patch is superseded, but shouldn't this be "<"?

> +                       break;
> +
> +               if (ret >= min_uV && ret <= max_uV)
> +                       return i;
> +       }
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(regulator_map_voltage_descend);
> +
>  /**
>   * regulator_map_voltage_linear - map_voltage() for simple linear mappings
>   *

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 2/4] regulator: core: Add regulator_map_voltage_descend() API

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Thu, Jun 20, 2024 at 3:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, Jun 5, 2024 at 9:49 AM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Similarly to regulator_map_voltage_ascend() api add
> > regulator_map_voltage_descend() api and export it.
> >
> > Drivers that have descendant voltage list can use this as their
> > map_voltage() operation.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/regulator/helpers.c
> > +++ b/drivers/regulator/helpers.c
> > @@ -368,6 +368,37 @@ int regulator_map_voltage_ascend(struct regulator_dev *rdev,
> >  }
> >  EXPORT_SYMBOL_GPL(regulator_map_voltage_ascend);
> >
> > +/**
> > + * regulator_map_voltage_descend - map_voltage() for descendant voltage list
> > + *
> > + * @rdev: Regulator to operate on
> > + * @min_uV: Lower bound for voltage
> > + * @max_uV: Upper bound for voltage
> > + *
> > + * Drivers that have descendant voltage list can use this as their
> > + * map_voltage() operation.
> > + */
> > +int regulator_map_voltage_descend(struct regulator_dev *rdev,
> > +                                 int min_uV, int max_uV)
> > +{
> > +       int i, ret;
> > +
> > +       for (i = rdev->desc->n_voltages - 1; i >= 0 ; i--) {
> > +               ret = rdev->desc->ops->list_voltage(rdev, i);
> > +               if (ret < 0)
> > +                       continue;
> > +
> > +               if (ret > min_uV)
>
> I know this patch is superseded, but shouldn't this be "<"?
>
Agreed, thanks I missed that.

Cheers,
Prabhakar

[RFC PATCH 3/4] 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
voltage level switch of the SD/MMC pins.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../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

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

[Rob Herring]

On Wed, Jun 05, 2024 at 08:49:35AM +0100, Prabhakar wrote:
> 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
> voltage level switch of the SD/MMC pins.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../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:

The node is already conditional on the compatible, so why the chip name? 
Then it doesn't work when the 2nd chip needs this.


> +          type: object
> +          description: VQMMC SD regulator
> +          $ref: /schemas/regulator/regulator.yaml#
> +          unevaluatedProperties: false
> +      required:
> +        - vqmmc-r9a09g057-regulator
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.34.1
> 

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

[Lad, Prabhakar]

Hi Rob,

Thank you for the review.

On Thu, Jun 6, 2024 at 1:26 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 08:49:35AM +0100, Prabhakar wrote:
> > 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
> > voltage level switch of the SD/MMC pins.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../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:
>
> The node is already conditional on the compatible, so why the chip name?
> Then it doesn't work when the 2nd chip needs this.
>
Are you suggesting to use a generic name "vqmmc-regulator"?

Currently depending on the compat value "vqmmc-r9a09g057-regulator" in
the driver the corresponding OF data is populated. In future if a
different chip needs a regulator which varies slightly to the
r9a09g057 chip this will have to have a different OF data. Hence I
added the chip name in the regulator.

Cheers,
Prabhakar

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

[Rob Herring]

On Thu, Jun 6, 2024 at 3:12 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Rob,
>
> Thank you for the review.
>
> On Thu, Jun 6, 2024 at 1:26 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jun 05, 2024 at 08:49:35AM +0100, Prabhakar wrote:
> > > 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
> > > voltage level switch of the SD/MMC pins.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../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:
> >
> > The node is already conditional on the compatible, so why the chip name?
> > Then it doesn't work when the 2nd chip needs this.
> >
> Are you suggesting to use a generic name "vqmmc-regulator"?


Yes, but "regulator-vqmmc" or just "regulator".

>
> Currently depending on the compat value "vqmmc-r9a09g057-regulator" in
> the driver the corresponding OF data is populated. In future if a
> different chip needs a regulator which varies slightly to the
> r9a09g057 chip this will have to have a different OF data. Hence I
> added the chip name in the regulator.

Yes, compatible values distinguish different chips, not node names.

Rob

[RFC PATCH 4/4] 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.

sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit
to handle voltage level control and power enable via SD_STATUS register.

regulator support is added to control the volatage levels of SD pins
via sd_iovs bit in SD_STATUS register.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/mmc/host/renesas_sdhi.h               |  7 ++
 drivers/mmc/host/renesas_sdhi_core.c          | 67 +++++++++++++++++--
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++
 drivers/mmc/host/tmio_mmc.h                   |  4 ++
 4 files changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 586f94d4dbfd..9ef4fdf44280 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/regmap.h>
+#include <linux/regulator/driver.h>
 #include "tmio_mmc.h"
 
 struct renesas_sdhi_scc {
@@ -49,6 +51,11 @@ struct renesas_sdhi_quirks {
 	bool manual_tap_correction;
 	bool old_info1_layout;
 	u32 hs400_bad_taps;
+	bool sd_iovs;
+	bool sd_pwen;
+	struct regulator_desc *rdesc;
+	const struct regmap_config *rdesc_regmap_config;
+	unsigned int rdesc_offset;
 	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
 };
 
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 12f4faaaf4ee..2eeea9513a23 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc)
 		 TMIO_STAT_DAT0);
 }
 
+static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host, bool on)
+{
+	u32 sd_status;
+
+	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
+	if (on)
+		sd_status |=  SD_STATUS_PWEN;
+	else
+		sd_status &=  ~SD_STATUS_PWEN;
+
+	sd_ctrl_write32(host, CTL_SD_STATUS, sd_status);
+}
+
 static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
 						    struct mmc_ios *ios)
 {
@@ -587,6 +600,9 @@ 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, sd_pwen))
+				renesas_sdhi_sd_status_pwen(host, true);
+
 			priv->needs_adjust_hs400 = false;
 			renesas_sdhi_set_clock(host, host->clk_cache);
 
@@ -904,6 +920,34 @@ 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 = priv,
+	};
+	struct regulator_dev *rdev;
+	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;
+	rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset,
+					    quirks->rdesc_regmap_config);
+	if (IS_ERR(rcfg.regmap))
+		return PTR_ERR(rcfg.regmap);
+
+	rdev = devm_regulator_register(&pdev->dev, quirks->rdesc, &rcfg);
+
+	return PTR_ERR_OR_ZERO(rdev);
+}
+
 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 +1095,15 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	if (ret)
 		goto efree;
 
+	if (sdhi_has_quirk(priv, sd_iovs)) {
+		ret = renesas_sdhi_internal_dmac_register_regulator(pdev, quirks);
+		if (ret)
+			goto efree;
+	}
+
+	if (sdhi_has_quirk(priv, sd_pwen))
+		renesas_sdhi_sd_status_pwen(host, true);
+
 	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)
@@ -1110,26 +1163,26 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 	num_irqs = platform_irq_count(pdev);
 	if (num_irqs < 0) {
 		ret = num_irqs;
-		goto eirq;
+		goto epwen;
 	}
 
 	/* There must be at least one IRQ source */
 	if (!num_irqs) {
 		ret = -ENXIO;
-		goto eirq;
+		goto epwen;
 	}
 
 	for (i = 0; i < num_irqs; i++) {
 		irq = platform_get_irq(pdev, i);
 		if (irq < 0) {
 			ret = irq;
-			goto eirq;
+			goto epwen;
 		}
 
 		ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0,
 				       dev_name(&pdev->dev), host);
 		if (ret)
-			goto eirq;
+			goto epwen;
 	}
 
 	ret = tmio_mmc_host_probe(host);
@@ -1141,7 +1194,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 
 	return ret;
 
-eirq:
+epwen:
+	if (sdhi_has_quirk(priv, sd_pwen))
+		renesas_sdhi_sd_status_pwen(host, false);
 	tmio_mmc_host_remove(host);
 edisclk:
 	renesas_sdhi_clk_disable(host);
@@ -1157,6 +1212,8 @@ void renesas_sdhi_remove(struct platform_device *pdev)
 	struct tmio_mmc_host *host = platform_get_drvdata(pdev);
 
 	tmio_mmc_host_remove(host);
+	if (sdhi_has_quirk(host_to_priv(host), sd_pwen))
+		renesas_sdhi_sd_status_pwen(host, false);
 	renesas_sdhi_clk_disable(host);
 	tmio_mmc_host_free(host);
 }
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 422fa63a2e99..f824d167bb09 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -215,6 +215,45 @@ static const struct renesas_sdhi_quirks sdhi_quirks_rzg2l = {
 	.hs400_disabled = true,
 };
 
+static const unsigned int r9a09g057_vqmmc_voltages[] = {
+	3300000, 1800000,
+};
+
+static const struct regulator_ops r9a09g057_regulator_voltage_ops = {
+	.list_voltage = regulator_list_voltage_table,
+	.map_voltage = regulator_map_voltage_descend,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+};
+
+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),
+	.vsel_mask	= 0x10000,
+	.vsel_reg	= 0,
+};
+
+static const struct regmap_config r9a09g057_vqmmc_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.max_register	= 1,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r9a09g057 = {
+	.fixed_addr_mode = true,
+	.hs400_disabled = true,
+	.sd_iovs = true,
+	.sd_pwen = true,
+	.rdesc = &r9a09g057_vqmmc_regulator,
+	.rdesc_regmap_config = &r9a09g057_vqmmc_regmap_config,
+	.rdesc_offset = 0x3C8,
+};
+
 /*
  * 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 +299,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 +328,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..d03aedf61aa3 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,9 @@
 /* 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 some IRQ masks */
 /* This is the mask used at reset by the chip */
 #define TMIO_MASK_ALL           0x837f031d
-- 
2.34.1

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

[Biju Das]

Hi Prabhakar,

Thanks for the feedback.

> -----Original Message-----
> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: Wednesday, June 5, 2024 8:50 AM
> Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> 
> 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.
> 
> sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit to handle voltage level
> control and power enable via SD_STATUS register.
> 
> regulator support is added to control the volatage levels of SD pins via sd_iovs bit in SD_STATUS
> register.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi.h               |  7 ++
>  drivers/mmc/host/renesas_sdhi_core.c          | 67 +++++++++++++++++--
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++
>  drivers/mmc/host/tmio_mmc.h                   |  4 ++
>  4 files changed, 118 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index
> 586f94d4dbfd..9ef4fdf44280 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/regmap.h>
> +#include <linux/regulator/driver.h>
>  #include "tmio_mmc.h"
> 
>  struct renesas_sdhi_scc {
> @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks {
>  	bool manual_tap_correction;
>  	bool old_info1_layout;
>  	u32 hs400_bad_taps;
> +	bool sd_iovs;
> +	bool sd_pwen;
> +	struct regulator_desc *rdesc;
> +	const struct regmap_config *rdesc_regmap_config;
> +	unsigned int rdesc_offset;
>  	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
>  };
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 12f4faaaf4ee..2eeea9513a23 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc)
>  		 TMIO_STAT_DAT0);
>  }
> 
> +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host,
> +bool on) {
> +	u32 sd_status;
> +
> +	sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
> +	if (on)
> +		sd_status |=  SD_STATUS_PWEN;
> +	else
> +		sd_status &=  ~SD_STATUS_PWEN;
> +
> +	sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); }
> +

May be use regulator_set_voltage() to set this??

>  static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
>  						    struct mmc_ios *ios)
>  {
> @@ -587,6 +600,9 @@ 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, sd_pwen))
> +				renesas_sdhi_sd_status_pwen(host, true);
> +
>  			priv->needs_adjust_hs400 = false;
>  			renesas_sdhi_set_clock(host, host->clk_cache);
> 
> @@ -904,6 +920,34 @@ 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 = priv,
> +	};
> +	struct regulator_dev *rdev;
> +	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;
> +	rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset,

This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks.

Cheers,
Biju

> +					    quirks->rdesc_regmap_config);
> +	if (IS_ERR(rcfg.regmap))
> +		return PTR_ERR(rcfg.regmap);
> +
> +	rdev = devm_regulator_register(&pdev->dev, quirks->rdesc, &rcfg);
> +
> +	return PTR_ERR_OR_ZERO(rdev);
> +}
> +
>  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 +1095,15 @@ int
> renesas_sdhi_probe(struct platform_device *pdev,
>  	if (ret)
>  		goto efree;
> 
> +	if (sdhi_has_quirk(priv, sd_iovs)) {
> +		ret = renesas_sdhi_internal_dmac_register_regulator(pdev, quirks);
> +		if (ret)
> +			goto efree;
> +	}
> +
> +	if (sdhi_has_quirk(priv, sd_pwen))
> +		renesas_sdhi_sd_status_pwen(host, true);
> +
>  	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) @@ -1110,26 +1163,26 @@
> int renesas_sdhi_probe(struct platform_device *pdev,
>  	num_irqs = platform_irq_count(pdev);
>  	if (num_irqs < 0) {
>  		ret = num_irqs;
> -		goto eirq;
> +		goto epwen;
>  	}
> 
>  	/* There must be at least one IRQ source */
>  	if (!num_irqs) {
>  		ret = -ENXIO;
> -		goto eirq;
> +		goto epwen;
>  	}
> 
>  	for (i = 0; i < num_irqs; i++) {
>  		irq = platform_get_irq(pdev, i);
>  		if (irq < 0) {
>  			ret = irq;
> -			goto eirq;
> +			goto epwen;
>  		}
> 
>  		ret = devm_request_irq(&pdev->dev, irq, tmio_mmc_irq, 0,
>  				       dev_name(&pdev->dev), host);
>  		if (ret)
> -			goto eirq;
> +			goto epwen;
>  	}
> 
>  	ret = tmio_mmc_host_probe(host);
> @@ -1141,7 +1194,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
> 
>  	return ret;
> 
> -eirq:
> +epwen:
> +	if (sdhi_has_quirk(priv, sd_pwen))
> +		renesas_sdhi_sd_status_pwen(host, false);
>  	tmio_mmc_host_remove(host);
>  edisclk:
>  	renesas_sdhi_clk_disable(host);
> @@ -1157,6 +1212,8 @@ void renesas_sdhi_remove(struct platform_device *pdev)
>  	struct tmio_mmc_host *host = platform_get_drvdata(pdev);
> 
>  	tmio_mmc_host_remove(host);
> +	if (sdhi_has_quirk(host_to_priv(host), sd_pwen))
> +		renesas_sdhi_sd_status_pwen(host, false);
>  	renesas_sdhi_clk_disable(host);
>  	tmio_mmc_host_free(host);
>  }
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 422fa63a2e99..f824d167bb09 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -215,6 +215,45 @@ static const struct renesas_sdhi_quirks sdhi_quirks_rzg2l = {
>  	.hs400_disabled = true,
>  };
> 
> +static const unsigned int r9a09g057_vqmmc_voltages[] = {
> +	3300000, 1800000,
> +};
> +
> +static const struct regulator_ops r9a09g057_regulator_voltage_ops = {
> +	.list_voltage = regulator_list_voltage_table,
> +	.map_voltage = regulator_map_voltage_descend,
> +	.get_voltage_sel = regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel = regulator_set_voltage_sel_regmap, };
> +
> +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),
> +	.vsel_mask	= 0x10000,
> +	.vsel_reg	= 0,
> +};
> +
> +static const struct regmap_config r9a09g057_vqmmc_regmap_config = {
> +	.reg_bits	= 32,
> +	.val_bits	= 32,
> +	.reg_stride	= 4,
> +	.max_register	= 1,
> +};
> +
> +static const struct renesas_sdhi_quirks sdhi_quirks_r9a09g057 = {
> +	.fixed_addr_mode = true,
> +	.hs400_disabled = true,
> +	.sd_iovs = true,
> +	.sd_pwen = true,
> +	.rdesc = &r9a09g057_vqmmc_regulator,
> +	.rdesc_regmap_config = &r9a09g057_vqmmc_regmap_config,
> +	.rdesc_offset = 0x3C8,
> +};
> +
>  /*
>   * 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 +299,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 +328,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..d03aedf61aa3 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,9 @@
>  /* 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 some IRQ masks */
>  /* This is the mask used at reset by the chip */
>  #define TMIO_MASK_ALL           0x837f031d
> --
> 2.34.1

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

[Lad, Prabhakar]

Hi Biju,

Thank you for the review.

On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the feedback.
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Wednesday, June 5, 2024 8:50 AM
> > Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> >
> > 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.
> >
> > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit to handle voltage level
> > control and power enable via SD_STATUS register.
> >
> > regulator support is added to control the volatage levels of SD pins via sd_iovs bit in SD_STATUS
> > register.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/mmc/host/renesas_sdhi.h               |  7 ++
> >  drivers/mmc/host/renesas_sdhi_core.c          | 67 +++++++++++++++++--
> >  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++
> >  drivers/mmc/host/tmio_mmc.h                   |  4 ++
> >  4 files changed, 118 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h index
> > 586f94d4dbfd..9ef4fdf44280 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/regmap.h>
> > +#include <linux/regulator/driver.h>
> >  #include "tmio_mmc.h"
> >
> >  struct renesas_sdhi_scc {
> > @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks {
> >       bool manual_tap_correction;
> >       bool old_info1_layout;
> >       u32 hs400_bad_taps;
> > +     bool sd_iovs;
> > +     bool sd_pwen;
> > +     struct regulator_desc *rdesc;
> > +     const struct regmap_config *rdesc_regmap_config;
> > +     unsigned int rdesc_offset;
> >       const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
> >  };
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > index 12f4faaaf4ee..2eeea9513a23 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc)
> >                TMIO_STAT_DAT0);
> >  }
> >
> > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host,
> > +bool on) {
> > +     u32 sd_status;
> > +
> > +     sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
> > +     if (on)
> > +             sd_status |=  SD_STATUS_PWEN;
> > +     else
> > +             sd_status &=  ~SD_STATUS_PWEN;
> > +
> > +     sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); }
> > +
>
> May be use regulator_set_voltage() to set this??
>
This is the PWEN bit which is not modelled as a regulator, we cannot
use regulator_set_voltage() to set this bit.

> >  static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
> >                                                   struct mmc_ios *ios)
> >  {
> > @@ -587,6 +600,9 @@ 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, sd_pwen))
> > +                             renesas_sdhi_sd_status_pwen(host, true);
> > +
> >                       priv->needs_adjust_hs400 = false;
> >                       renesas_sdhi_set_clock(host, host->clk_cache);
> >
> > @@ -904,6 +920,34 @@ 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 = priv,
> > +     };
> > +     struct regulator_dev *rdev;
> > +     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;
> > +     rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl + quirks->rdesc_offset,
>
> This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks.
>
rdesc_offset is added to make code generic, that is in future if there
is a new chip with a different offset which supports IOVS we can just
pass the offset for it.

Cheers,
Prabhakar

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

[Biju Das]

Hi Prabhakar,


> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Sent: Thursday, June 6, 2024 10:38 AM
> Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> 
> Hi Biju,
> 
> Thank you for the review.
> 
> On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the feedback.
> >
> > > -----Original Message-----
> > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > Sent: Wednesday, June 5, 2024 8:50 AM
> > > Subject: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for
> > > RZ/V2H(P) SoC
> > >
> > > 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.
> > >
> > > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this
> > > bit to handle voltage level control and power enable via SD_STATUS register.
> > >
> > > regulator support is added to control the volatage levels of SD pins
> > > via sd_iovs bit in SD_STATUS register.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/mmc/host/renesas_sdhi.h               |  7 ++
> > >  drivers/mmc/host/renesas_sdhi_core.c          | 67 +++++++++++++++++--
> > >  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 45 +++++++++++++
> > >  drivers/mmc/host/tmio_mmc.h                   |  4 ++
> > >  4 files changed, 118 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/renesas_sdhi.h
> > > b/drivers/mmc/host/renesas_sdhi.h index
> > > 586f94d4dbfd..9ef4fdf44280 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/regmap.h>
> > > +#include <linux/regulator/driver.h>
> > >  #include "tmio_mmc.h"
> > >
> > >  struct renesas_sdhi_scc {
> > > @@ -49,6 +51,11 @@ struct renesas_sdhi_quirks {
> > >       bool manual_tap_correction;
> > >       bool old_info1_layout;
> > >       u32 hs400_bad_taps;
> > > +     bool sd_iovs;
> > > +     bool sd_pwen;
> > > +     struct regulator_desc *rdesc;
> > > +     const struct regmap_config *rdesc_regmap_config;
> > > +     unsigned int rdesc_offset;
> > >       const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
> > >  };
> > >
> > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c
> > > b/drivers/mmc/host/renesas_sdhi_core.c
> > > index 12f4faaaf4ee..2eeea9513a23 100644
> > > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> > > @@ -248,6 +248,19 @@ static int renesas_sdhi_card_busy(struct mmc_host *mmc)
> > >                TMIO_STAT_DAT0);
> > >  }
> > >
> > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host,
> > > +bool on) {
> > > +     u32 sd_status;
> > > +
> > > +     sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
> > > +     if (on)
> > > +             sd_status |=  SD_STATUS_PWEN;
> > > +     else
> > > +             sd_status &=  ~SD_STATUS_PWEN;
> > > +
> > > +     sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); }
> > > +
> >
> > May be use regulator_set_voltage() to set this??
> >
> This is the PWEN bit which is not modelled as a regulator, we cannot use regulator_set_voltage() to
> set this bit.

So, there may be a race between regulator driver and this bit??

> 
> > >  static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
> > >                                                   struct mmc_ios
> > > *ios)  { @@ -587,6 +600,9 @@ 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, sd_pwen))
> > > +                             renesas_sdhi_sd_status_pwen(host,
> > > + true);
> > > +
> > >                       priv->needs_adjust_hs400 = false;
> > >                       renesas_sdhi_set_clock(host, host->clk_cache);
> > >
> > > @@ -904,6 +920,34 @@ 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 = priv,
> > > +     };
> > > +     struct regulator_dev *rdev;
> > > +     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;
> > > +     rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl +
> > > + quirks->rdesc_offset,
> >
> > This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks.
> >
> rdesc_offset is added to make code generic, that is in future if there is a new chip with a
> different offset which supports IOVS we can just pass the offset for it.

Currently there is no consumer for it, so it can save memory. When a future chip comes
we can bring back this variable??

Cheers,
Biju

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

[Lad, Prabhakar]

Hi Biju,

On Thu, Jun 6, 2024 at 10:43 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
>
> > -----Original Message-----
> > From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Thursday, June 6, 2024 10:38 AM
> > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
<snip>
> > > >
> > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host *host,
> > > > +bool on) {
> > > > +     u32 sd_status;
> > > > +
> > > > +     sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
> > > > +     if (on)
> > > > +             sd_status |=  SD_STATUS_PWEN;
> > > > +     else
> > > > +             sd_status &=  ~SD_STATUS_PWEN;
> > > > +
> > > > +     sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); }
> > > > +
> > >
> > > May be use regulator_set_voltage() to set this??
> > >
> > This is the PWEN bit which is not modelled as a regulator, we cannot use regulator_set_voltage() to
> > set this bit.
>
> So, there may be a race between regulator driver and this bit??
>
No, there won't be any race between the regulator driver and this bit
as the regulator driver only controls the IOVS bit and not the PWEN
bit.

> >
> > > >  static int renesas_sdhi_start_signal_voltage_switch(struct mmc_host *mmc,
> > > >                                                   struct mmc_ios
> > > > *ios)  { @@ -587,6 +600,9 @@ 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, sd_pwen))
> > > > +                             renesas_sdhi_sd_status_pwen(host,
> > > > + true);
> > > > +
> > > >                       priv->needs_adjust_hs400 = false;
> > > >                       renesas_sdhi_set_clock(host, host->clk_cache);
> > > >
> > > > @@ -904,6 +920,34 @@ 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 = priv,
> > > > +     };
> > > > +     struct regulator_dev *rdev;
> > > > +     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;
> > > > +     rcfg.regmap = devm_regmap_init_mmio(&pdev->dev, host->ctl +
> > > > + quirks->rdesc_offset,
> > >
> > > This is (CTL_SD_STATUS << 2) , so the variable can be dropped from quirks.
> > >
> > rdesc_offset is added to make code generic, that is in future if there is a new chip with a
> > different offset which supports IOVS we can just pass the offset for it.
>
> Currently there is no consumer for it, so it can save memory. When a future chip comes
> we can bring back this variable??
>
OK.

Cheers,
Prabhakar

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

[Biju Das]

> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Sent: Thursday, June 6, 2024 10:50 AM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>; Ulf Hansson <ulf.hansson@linaro.org>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>;
> Wolfram Sang <wsa+renesas@sang-engineering.com>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Magnus Damm <magnus.damm@gmail.com>; linux-mmc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-renesas-soc@vger.kernel.org;
> Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
> 
> Hi Biju,
> 
> On Thu, Jun 6, 2024 at 10:43 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> >
> > > -----Original Message-----
> > > From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > > Sent: Thursday, June 6, 2024 10:38 AM
> > > Subject: Re: [RFC PATCH 4/4] mmc: renesas_sdhi: Add support for
> > > RZ/V2H(P) SoC
> > >
> > > Hi Biju,
> > >
> > > Thank you for the review.
> > >
> > > On Thu, Jun 6, 2024 at 10:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> <snip>
> > > > >
> > > > > +static void renesas_sdhi_sd_status_pwen(struct tmio_mmc_host
> > > > > +*host, bool on) {
> > > > > +     u32 sd_status;
> > > > > +
> > > > > +     sd_ctrl_read32_rep(host, CTL_SD_STATUS, &sd_status, 1);
> > > > > +     if (on)
> > > > > +             sd_status |=  SD_STATUS_PWEN;
> > > > > +     else
> > > > > +             sd_status &=  ~SD_STATUS_PWEN;
> > > > > +
> > > > > +     sd_ctrl_write32(host, CTL_SD_STATUS, sd_status); }
> > > > > +
> > > >
> > > > May be use regulator_set_voltage() to set this??
> > > >
> > > This is the PWEN bit which is not modelled as a regulator, we cannot
> > > use regulator_set_voltage() to set this bit.
> >
> > So, there may be a race between regulator driver and this bit??
> >
> No, there won't be any race between the regulator driver and this bit as the regulator driver only
> controls the IOVS bit and not the PWEN bit.

I guess, since it is rmw operation, there is a chance that this will overwritten in a race condition.
But mmc driver is manages the calls in serial fashion, so there won't be any races.

Cheers,
Biju

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

[Wolfram Sang]

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

Hi Prabhakar,

thanks for this series!

On Wed, Jun 05, 2024 at 08:49:36AM +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.
> 
> sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit
> to handle voltage level control and power enable via SD_STATUS register.

Two high-level questions:

- can't we use .enable/.disable in regulator_ops for handling pwen?
  Then we could simply use regulator_en/disable in the code and be future
  proof when other SDHI instances have other kinds of regulators (unless
  I am mising something)

- what about not using regmap and use set/get_voltage and friends? My
  concern is that other "new" registers might appear in the future and
  it will be cumbersome to handle the scattered IO regions.

That said, having a regulator is not a quirk in my book. I'd think
'struct renesas_sdhi' is the proper place. Or?

Looking forward to your comments,

   Wolfram


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

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

[Lad, Prabhakar]

Hi Wolfram,

On Thu, Jun 6, 2024 at 11:08 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Prabhakar,
>
> thanks for this series!
>
> On Wed, Jun 05, 2024 at 08:49:36AM +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.
> >
> > sd_iovs and sd_pwen quirks are introduced for SoCs supporting this bit
> > to handle voltage level control and power enable via SD_STATUS register.
>
> Two high-level questions:
>
> - can't we use .enable/.disable in regulator_ops for handling pwen?
>   Then we could simply use regulator_en/disable in the code and be future
>   proof when other SDHI instances have other kinds of regulators (unless
>   I am mising something)
>
Ok let me check on this and get back.

> - what about not using regmap and use set/get_voltage and friends? My
>   concern is that other "new" registers might appear in the future and
>   it will be cumbersome to handle the scattered IO regions.
>
I'll have to do some reading on this. Can you please point me to any
example driver which does not use regmap.

> That said, having a regulator is not a quirk in my book. I'd think
> 'struct renesas_sdhi' is the proper place. Or?
>
Ok, I will move them out of quirks.

Cheers,
Prabhakar

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

[Wolfram Sang]

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

Hi Prabhakar,

> > - can't we use .enable/.disable in regulator_ops for handling pwen?
> >   Then we could simply use regulator_en/disable in the code and be future
> >   proof when other SDHI instances have other kinds of regulators (unless
> >   I am mising something)
> >
> Ok let me check on this and get back.

Thanks!

> > - what about not using regmap and use set/get_voltage and friends? My
> >   concern is that other "new" registers might appear in the future and
> >   it will be cumbersome to handle the scattered IO regions.
> >
> I'll have to do some reading on this. Can you please point me to any
> example driver which does not use regmap.

Sure thing!

~/Kernel/linux/drivers/regulator$ grep -L regmap $(grep -l devm_regulator_register *.c)
aat2870-regulator.c
ab8500.c
ab8500-ext.c
ad5398.c
cros-ec-regulator.c
da903x-regulator.c
db8500-prcmu.c
dummy.c
fixed.c
gpio-regulator.c
isl6271a-regulator.c
lp3971.c
lp3972.c
max1586.c
max8660.c
max8925-regulator.c
max8952.c
max8997-regulator.c
max8998.c
mc13783-regulator.c
mc13892-regulator.c
mtk-dvfsrc-regulator.c
pcap-regulator.c
pwm-regulator.c
qcom-rpmh-regulator.c
qcom_rpm-regulator.c
qcom_smd-regulator.c
scmi-regulator.c
stm32-pwr.c
ti-abb-regulator.c
tps6507x-regulator.c
tps6524x-regulator.c
twl6030-regulator.c
twl-regulator.c
vctrl-regulator.c

> > That said, having a regulator is not a quirk in my book. I'd think
> > 'struct renesas_sdhi' is the proper place. Or?
> >
> Ok, I will move them out of quirks.

Cool!

Happy hacking,

   Wolfram

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