[PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[Catalin Popescu]

Add compatible value "mmc-pwrseq-simple-reset" to support reset control
instead of gpios. Reset controls being refcounted, they allow to use
shared resets or gpios across drivers. Support of reset control is
limited to one single reset control.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 .../bindings/mmc/mmc-pwrseq-simple.yaml       | 21 +++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
index 00feaafc1063..da218260af01 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
@@ -16,12 +16,13 @@ description:
 
 properties:
   compatible:
-    const: mmc-pwrseq-simple
+    enum:
+      - mmc-pwrseq-simple
+      - mmc-pwrseq-simple-reset
 
   reset-gpios:
     minItems: 1
     # Put some limit to avoid false warnings
-    maxItems: 32
     description:
       contains a list of GPIO specifiers. The reset GPIOs are asserted
       at initialization and prior we start the power up procedure of the card.
@@ -50,6 +51,22 @@ properties:
 required:
   - compatible
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mmc-pwrseq-simple-reset
+    then:
+      properties:
+        reset-gpios:
+          maxItems: 1
+    else:
+      properties:
+        reset-gpios:
+          maxItems: 32
+
 additionalProperties: false
 
 examples:
-- 
2.34.1

[PATCH 2/2] mmc: pwrseq_simple: add support for reset control

[Catalin Popescu]

Reset controls are refcounted and as such allows to use shared resets or
gpios across drivers. Add support for reset control while keeping in
place the support for gpios. Use compatible string to choose between
reset control ("mmc-pwrseq-simple-reset") and reset gpios
("mmc-pwrseq-simple"). Only one reset control is supported.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 drivers/mmc/core/pwrseq_simple.c | 58 ++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 9e016b0746f5..6495b97d48d9 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -17,11 +17,17 @@
 #include <linux/gpio/consumer.h>
 #include <linux/delay.h>
 #include <linux/property.h>
+#include <linux/of.h>
+#include <linux/reset.h>
 
 #include <linux/mmc/host.h>
 
 #include "pwrseq.h"
 
+struct mmc_pwrseq_simple_data {
+	bool use_reset;
+};
+
 struct mmc_pwrseq_simple {
 	struct mmc_pwrseq pwrseq;
 	bool clk_enabled;
@@ -29,6 +35,8 @@ struct mmc_pwrseq_simple {
 	u32 power_off_delay_us;
 	struct clk *ext_clk;
 	struct gpio_descs *reset_gpios;
+	struct reset_control *reset_ctrl;
+	const struct mmc_pwrseq_simple_data *data;
 };
 
 #define to_pwrseq_simple(p) container_of(p, struct mmc_pwrseq_simple, pwrseq)
@@ -67,14 +75,21 @@ static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
 		pwrseq->clk_enabled = true;
 	}
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
+	if (pwrseq->data->use_reset) {
+		reset_control_deassert(pwrseq->reset_ctrl);
+		reset_control_assert(pwrseq->reset_ctrl);
+	} else
+		mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
 }
 
 static void mmc_pwrseq_simple_post_power_on(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 0);
+	if (pwrseq->data->use_reset)
+		reset_control_deassert(pwrseq->reset_ctrl);
+	else
+		mmc_pwrseq_simple_set_gpios_value(pwrseq, 0);
 
 	if (pwrseq->post_power_on_delay_ms)
 		msleep(pwrseq->post_power_on_delay_ms);
@@ -84,7 +99,10 @@ static void mmc_pwrseq_simple_power_off(struct mmc_host *host)
 {
 	struct mmc_pwrseq_simple *pwrseq = to_pwrseq_simple(host->pwrseq);
 
-	mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
+	if (pwrseq->data->use_reset)
+		reset_control_assert(pwrseq->reset_ctrl);
+	else
+		mmc_pwrseq_simple_set_gpios_value(pwrseq, 1);
 
 	if (pwrseq->power_off_delay_us)
 		usleep_range(pwrseq->power_off_delay_us,
@@ -102,8 +120,17 @@ static const struct mmc_pwrseq_ops mmc_pwrseq_simple_ops = {
 	.power_off = mmc_pwrseq_simple_power_off,
 };
 
+struct mmc_pwrseq_simple_data mmc_pwrseq_simple_gpio = {
+	.use_reset = false,
+};
+
+struct mmc_pwrseq_simple_data mmc_pwrseq_simple_reset = {
+	.use_reset = true,
+};
+
 static const struct of_device_id mmc_pwrseq_simple_of_match[] = {
-	{ .compatible = "mmc-pwrseq-simple",},
+	{ .compatible = "mmc-pwrseq-simple", .data = &mmc_pwrseq_simple_gpio },
+	{ .compatible = "mmc-pwrseq-simple-reset", .data = &mmc_pwrseq_simple_reset },
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, mmc_pwrseq_simple_of_match);
@@ -121,12 +148,23 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
 	if (IS_ERR(pwrseq->ext_clk) && PTR_ERR(pwrseq->ext_clk) != -ENOENT)
 		return dev_err_probe(dev, PTR_ERR(pwrseq->ext_clk), "external clock not ready\n");
 
-	pwrseq->reset_gpios = devm_gpiod_get_array(dev, "reset",
-							GPIOD_OUT_HIGH);
-	if (IS_ERR(pwrseq->reset_gpios) &&
-	    PTR_ERR(pwrseq->reset_gpios) != -ENOENT &&
-	    PTR_ERR(pwrseq->reset_gpios) != -ENOSYS) {
-		return dev_err_probe(dev, PTR_ERR(pwrseq->reset_gpios), "reset GPIOs not ready\n");
+	pwrseq->data = of_device_get_match_data(dev);
+	if (!pwrseq->data)
+		return -EINVAL;
+
+	if (pwrseq->data->use_reset) {
+		pwrseq->reset_ctrl = devm_reset_control_get_optional_shared(dev, NULL);
+		if (IS_ERR(pwrseq->reset_ctrl))
+			return dev_err_probe(dev, PTR_ERR(pwrseq->reset_ctrl),
+					     "reset controls not ready\n");
+	} else {
+		pwrseq->reset_gpios = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
+		if (IS_ERR(pwrseq->reset_gpios) &&
+		    PTR_ERR(pwrseq->reset_gpios) != -ENOENT &&
+		    PTR_ERR(pwrseq->reset_gpios) != -ENOSYS) {
+			return dev_err_probe(dev, PTR_ERR(pwrseq->reset_gpios),
+					     "reset GPIOs not ready\n");
+		}
 	}
 
 	device_property_read_u32(dev, "post-power-on-delay-ms",
-- 
2.34.1

Re: [PATCH 2/2] mmc: pwrseq_simple: add support for reset control

[kernel test robot]

Hi Catalin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.12-rc1 next-20241004]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Catalin-Popescu/mmc-pwrseq_simple-add-support-for-reset-control/20241004-200909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20241004120740.2887776-2-catalin.popescu%40leica-geosystems.com
patch subject: [PATCH 2/2] mmc: pwrseq_simple: add support for reset control
config: x86_64-randconfig-123-20241005 (https://download.01.org/0day-ci/archive/20241005/202410052201.xEk9eC0T-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241005/202410052201.xEk9eC0T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410052201.xEk9eC0T-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/mmc/core/pwrseq_simple.c:123:31: sparse: sparse: symbol 'mmc_pwrseq_simple_gpio' was not declared. Should it be static?
>> drivers/mmc/core/pwrseq_simple.c:127:31: sparse: sparse: symbol 'mmc_pwrseq_simple_reset' was not declared. Should it be static?

vim +/mmc_pwrseq_simple_gpio +123 drivers/mmc/core/pwrseq_simple.c

   122	
 > 123	struct mmc_pwrseq_simple_data mmc_pwrseq_simple_gpio = {
   124		.use_reset = false,
   125	};
   126	
 > 127	struct mmc_pwrseq_simple_data mmc_pwrseq_simple_reset = {
   128		.use_reset = true,
   129	};
   130	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[Rob Herring]

On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
> instead of gpios. Reset controls being refcounted, they allow to use
> shared resets or gpios across drivers. Support of reset control is
> limited to one single reset control.

Can't you do this without a binding change? Just use reset controls when 
there is only 1 GPIO.

> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  .../bindings/mmc/mmc-pwrseq-simple.yaml       | 21 +++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[POPESCU Catalin]

On 05/10/2024 20:26, Rob Herring wrote:
> [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
>> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
>> instead of gpios. Reset controls being refcounted, they allow to use
>> shared resets or gpios across drivers. Support of reset control is
>> limited to one single reset control.
> Can't you do this without a binding change? Just use reset controls when
> there is only 1 GPIO.

That's a good question. The idea was to keep in place the gpio support 
w/o impacting any platform using pwrseq-simple.

Also, later on when support for a list of reset gpios will be added to 
the reset framework, this would not work anymore...

>
>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> ---
>>   .../bindings/mmc/mmc-pwrseq-simple.yaml       | 21 +++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[Rob Herring]

On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote:
> On 05/10/2024 20:26, Rob Herring wrote:
> > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
> >
> >
> > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
> >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
> >> instead of gpios. Reset controls being refcounted, they allow to use
> >> shared resets or gpios across drivers. Support of reset control is
> >> limited to one single reset control.
> > Can't you do this without a binding change? Just use reset controls when
> > there is only 1 GPIO.
> 
> That's a good question. The idea was to keep in place the gpio support 
> w/o impacting any platform using pwrseq-simple.

Why would it matter? If not shared, then the behavior should be the 
same. If shared, we want to maintain the broken behavior?

> 
> Also, later on when support for a list of reset gpios will be added to 
> the reset framework, this would not work anymore...

Why not?

How an OS handles reset-gpios is up to the OS. It can evolve. The 
binding can't evolve because it is an ABI.

Also, a list is kind of broken to begin with for a "generic" binding. 
What's the order the lines should be asserted/deasserted? What about 
timing requirements? You don't know because every device is different. 
This binding would not be accepted now, so extending it is questionable.

Rob

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[POPESCU Catalin]

On 07/10/2024 17:59, Rob Herring wrote:
> [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote:
>> On 05/10/2024 20:26, Rob Herring wrote:
>>> [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>
>>>
>>> On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
>>>> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
>>>> instead of gpios. Reset controls being refcounted, they allow to use
>>>> shared resets or gpios across drivers. Support of reset control is
>>>> limited to one single reset control.
>>> Can't you do this without a binding change? Just use reset controls when
>>> there is only 1 GPIO.
>> That's a good question. The idea was to keep in place the gpio support
>> w/o impacting any platform using pwrseq-simple.
> Why would it matter? If not shared, then the behavior should be the
> same. If shared, we want to maintain the broken behavior?
Indeed, you're right. I will provide a new patchset w/o the binding 
change and using reset control for 1 gpio use-case.
>> Also, later on when support for a list of reset gpios will be added to
>> the reset framework, this would not work anymore...
> Why not?
>
> How an OS handles reset-gpios is up to the OS. It can evolve. The
> binding can't evolve because it is an ABI.
>
> Also, a list is kind of broken to begin with for a "generic" binding.
> What's the order the lines should be asserted/deasserted? What about
> timing requirements? You don't know because every device is different.
> This binding would not be accepted now, so extending it is questionable.
>
> Rob

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[Marco Felsch]

On 24-10-07, Rob Herring wrote:
> On Mon, Oct 07, 2024 at 03:32:42PM +0000, POPESCU Catalin wrote:
> > On 05/10/2024 20:26, Rob Herring wrote:
> > > [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > >
> > > This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
> > >
> > >
> > > On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
> > >> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
> > >> instead of gpios. Reset controls being refcounted, they allow to use
> > >> shared resets or gpios across drivers. Support of reset control is
> > >> limited to one single reset control.
> > > Can't you do this without a binding change? Just use reset controls when
> > > there is only 1 GPIO.
> > 
> > That's a good question. The idea was to keep in place the gpio support 
> > w/o impacting any platform using pwrseq-simple.
> 
> Why would it matter? If not shared, then the behavior should be the 
> same. If shared, we want to maintain the broken behavior?

My two cents on this. This will break systems which haven't the
RESET_GPIO driver enabled yet. Therefore we may get regressions mails.
I'm fine with it if that is okay.

Regards,
  Marco

> > Also, later on when support for a list of reset gpios will be added to 
> > the reset framework, this would not work anymore...
> 
> Why not?
> 
> How an OS handles reset-gpios is up to the OS. It can evolve. The 
> binding can't evolve because it is an ABI.
> 
> Also, a list is kind of broken to begin with for a "generic" binding. 
> What's the order the lines should be asserted/deasserted? What about 
> timing requirements? You don't know because every device is different. 
> This binding would not be accepted now, so extending it is questionable.
> 
> Rob
> 

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[Krzysztof Kozlowski]

On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
> instead of gpios. Reset controls being refcounted, they allow to use
> shared resets or gpios across drivers. Support of reset control is
> limited to one single reset control.

Driver support is not that relevant to the bindings.

> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
>  .../bindings/mmc/mmc-pwrseq-simple.yaml       | 21 +++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
> index 00feaafc1063..da218260af01 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
> @@ -16,12 +16,13 @@ description:
>  
>  properties:
>    compatible:
> -    const: mmc-pwrseq-simple
> +    enum:
> +      - mmc-pwrseq-simple
> +      - mmc-pwrseq-simple-reset

Nope, that's the same device.

>  
>    reset-gpios:
>      minItems: 1
>      # Put some limit to avoid false warnings
> -    maxItems: 32
>      description:
>        contains a list of GPIO specifiers. The reset GPIOs are asserted
>        at initialization and prior we start the power up procedure of the card.
> @@ -50,6 +51,22 @@ properties:
>  required:
>    - compatible
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mmc-pwrseq-simple-reset
> +    then:
> +      properties:
> +        reset-gpios:
> +          maxItems: 1
> +    else:
> +      properties:
> +        reset-gpios:
> +          maxItems: 32

So basically they are the same... Sorry, this all patch makes little
sense to me. You are not doing here much. It's exactly the same device
which you now describe in two ways (first no-go) but the two ways are
actually the same (second no-go).

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: mmc: mmc-pwrseq-simple: add support for reset control

[POPESCU Catalin]

On 06/10/2024 14:37, Krzysztof Kozlowski wrote:
> [Some people who received this message don't often get email from krzk@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Fri, Oct 04, 2024 at 02:07:39PM +0200, Catalin Popescu wrote:
>> Add compatible value "mmc-pwrseq-simple-reset" to support reset control
>> instead of gpios. Reset controls being refcounted, they allow to use
>> shared resets or gpios across drivers. Support of reset control is
>> limited to one single reset control.
> Driver support is not that relevant to the bindings.
>
>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> ---
>>   .../bindings/mmc/mmc-pwrseq-simple.yaml       | 21 +++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
>> index 00feaafc1063..da218260af01 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
>> @@ -16,12 +16,13 @@ description:
>>
>>   properties:
>>     compatible:
>> -    const: mmc-pwrseq-simple
>> +    enum:
>> +      - mmc-pwrseq-simple
>> +      - mmc-pwrseq-simple-reset
> Nope, that's the same device.
>
>>     reset-gpios:
>>       minItems: 1
>>       # Put some limit to avoid false warnings
>> -    maxItems: 32
>>       description:
>>         contains a list of GPIO specifiers. The reset GPIOs are asserted
>>         at initialization and prior we start the power up procedure of the card.
>> @@ -50,6 +51,22 @@ properties:
>>   required:
>>     - compatible
>>
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - mmc-pwrseq-simple-reset
>> +    then:
>> +      properties:
>> +        reset-gpios:
>> +          maxItems: 1
>> +    else:
>> +      properties:
>> +        reset-gpios:
>> +          maxItems: 32
> So basically they are the same... Sorry, this all patch makes little
> sense to me. You are not doing here much. It's exactly the same device
> which you now describe in two ways (first no-go) but the two ways are
> actually the same (second no-go).

That's because the reset framework doesn't support a list of reset gpios 
(assuming that RESET_GPIO was enabled), but only a single reset gpio.

Both gpio and reset frameworks are expecting the same DT property 
("reset-gpios") so using a different compatible to differentiate b/w the 
"tool" (gpio or reset control) to use for the power sequence seemed fine 
to me.

>
> Best regards,
> Krzysztof
>