[PATCH master] mci: dw_mmc: make reset control optional again

[PATCH master] mci: dw_mmc: make reset control optional again

[Ahmad Fatoum]

As documented in 90bdf1e5d1e4 ("mci: dw_mmc: match against StarFive MMC
compatibles"), it was intended for the reset to remain optional as to
not break existing users. Unfortunately, my later a3cf324593ea
("mci: dw_mmc: add optional reset line") didn't heed that and made it
required, breaking SoCFPGA DW-MMC use as a result.

Revert that line to fix the regression.

Fixes: a3cf324593ea ("mci: dw_mmc: add optional reset line")
Reported-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/dw_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mci/dw_mmc.c b/drivers/mci/dw_mmc.c
index b402090ab3cb..86c4f43e88f5 100644
--- a/drivers/mci/dw_mmc.c
+++ b/drivers/mci/dw_mmc.c
@@ -572,7 +572,7 @@ static int dw_mmc_probe(struct device_d *dev)
 
 	rst = reset_control_get(dev, "reset");
 	if (IS_ERR(rst)) {
-		return PTR_ERR(rst);
+		dev_warn(dev, "error claiming reset: %pe\n", rst);
 	} else if (rst) {
 		reset_control_assert(rst);
 		udelay(10);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] mci: dw_mmc: make reset control optional again

[Sascha Hauer]

On Mon, Nov 01, 2021 at 06:52:07PM +0100, Ahmad Fatoum wrote:
> As documented in 90bdf1e5d1e4 ("mci: dw_mmc: match against StarFive MMC
> compatibles"), it was intended for the reset to remain optional as to
> not break existing users. Unfortunately, my later a3cf324593ea
> ("mci: dw_mmc: add optional reset line") didn't heed that and made it
> required, breaking SoCFPGA DW-MMC use as a result.
> 
> Revert that line to fix the regression.
> 
> Fixes: a3cf324593ea ("mci: dw_mmc: add optional reset line")
> Reported-by: Ian Abbott <abbotti@mev.co.uk>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/mci/dw_mmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mci/dw_mmc.c b/drivers/mci/dw_mmc.c
> index b402090ab3cb..86c4f43e88f5 100644
> --- a/drivers/mci/dw_mmc.c
> +++ b/drivers/mci/dw_mmc.c
> @@ -572,7 +572,7 @@ static int dw_mmc_probe(struct device_d *dev)
>  
>  	rst = reset_control_get(dev, "reset");

Philipp, the reset binding lists the reset-names property as optional.
What's the expected behaviour of the reset_control_get() above when the
reset-names property is not present in the device tree? Should it return
an error or should it return the unnamed reset control?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] mci: dw_mmc: make reset control optional again

[Philipp Zabel]

On Tue, 2021-11-02 at 09:06 +0100, Sascha Hauer wrote:
> On Mon, Nov 01, 2021 at 06:52:07PM +0100, Ahmad Fatoum wrote:
> > As documented in 90bdf1e5d1e4 ("mci: dw_mmc: match against StarFive MMC
> > compatibles"), it was intended for the reset to remain optional as to
> > not break existing users. Unfortunately, my later a3cf324593ea
> > ("mci: dw_mmc: add optional reset line") didn't heed that and made it
> > required, breaking SoCFPGA DW-MMC use as a result.
> > 
> > Revert that line to fix the regression.
> > 
> > Fixes: a3cf324593ea ("mci: dw_mmc: add optional reset line")
> > Reported-by: Ian Abbott <abbotti@mev.co.uk>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > ---
> >  drivers/mci/dw_mmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mci/dw_mmc.c b/drivers/mci/dw_mmc.c
> > index b402090ab3cb..86c4f43e88f5 100644
> > --- a/drivers/mci/dw_mmc.c
> > +++ b/drivers/mci/dw_mmc.c
> > @@ -572,7 +572,7 @@ static int dw_mmc_probe(struct device_d *dev)
> >  
> > 
> >  	rst = reset_control_get(dev, "reset");
> 
> Philipp, the reset binding lists the reset-names property as optional.
> What's the expected behaviour of the reset_control_get() above when the
> reset-names property is not present in the device tree? Should it return
> an error or should it return the unnamed reset control?

I'd expect it to return -ENOENT.

The common reset binding only has reset-names optional for the single-
reset case - but that should be requested with reset_control_get(dev,
NULL) by the driver.
If the device specific binding specifies a reset-names propertiy, it
should either be required, or the resets property should be optional as
well, with either both or none present in the device tree.

In the kernel there's also reset_control_get_optional_... variants that
return NULL instead of -ENOENT to simplify the optional reset case.

I think the issue here may be that barebox of_reset_control_get() is
missing 3d81216fde46 ("reset: Fix of_reset_control_get() for consistent
return values") [1]. The driver should then ignore the -ENOENT error.

[1] https://lore.kernel.org/lkml/1441121311-31628-1-git-send-email-albeu@free.fr/

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] mci: dw_mmc: make reset control optional again

[Ahmad Fatoum]

Hello Sascha,

On 02.11.21 09:06, Sascha Hauer wrote:
> On Mon, Nov 01, 2021 at 06:52:07PM +0100, Ahmad Fatoum wrote:
>> As documented in 90bdf1e5d1e4 ("mci: dw_mmc: match against StarFive MMC
>> compatibles"), it was intended for the reset to remain optional as to
>> not break existing users. Unfortunately, my later a3cf324593ea
>> ("mci: dw_mmc: add optional reset line") didn't heed that and made it
>> required, breaking SoCFPGA DW-MMC use as a result.
>>
>> Revert that line to fix the regression.
>>
>> Fixes: a3cf324593ea ("mci: dw_mmc: add optional reset line")
>> Reported-by: Ian Abbott <abbotti@mev.co.uk>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/mci/dw_mmc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mci/dw_mmc.c b/drivers/mci/dw_mmc.c
>> index b402090ab3cb..86c4f43e88f5 100644
>> --- a/drivers/mci/dw_mmc.c
>> +++ b/drivers/mci/dw_mmc.c
>> @@ -572,7 +572,7 @@ static int dw_mmc_probe(struct device_d *dev)
>>  
>>  	rst = reset_control_get(dev, "reset");
> 
> Philipp, the reset binding lists the reset-names property as optional.
> What's the expected behaviour of the reset_control_get() above when the
> reset-names property is not present in the device tree? Should it return
> an error or should it return the unnamed reset control?

can this revert still be applied for master? I'll look into reworking this
for next.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] mci: dw_mmc: make reset control optional again

[Sascha Hauer]

On Mon, Nov 08, 2021 at 11:28:11AM +0100, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 02.11.21 09:06, Sascha Hauer wrote:
> > On Mon, Nov 01, 2021 at 06:52:07PM +0100, Ahmad Fatoum wrote:
> >> As documented in 90bdf1e5d1e4 ("mci: dw_mmc: match against StarFive MMC
> >> compatibles"), it was intended for the reset to remain optional as to
> >> not break existing users. Unfortunately, my later a3cf324593ea
> >> ("mci: dw_mmc: add optional reset line") didn't heed that and made it
> >> required, breaking SoCFPGA DW-MMC use as a result.
> >>
> >> Revert that line to fix the regression.
> >>
> >> Fixes: a3cf324593ea ("mci: dw_mmc: add optional reset line")
> >> Reported-by: Ian Abbott <abbotti@mev.co.uk>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  drivers/mci/dw_mmc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mci/dw_mmc.c b/drivers/mci/dw_mmc.c
> >> index b402090ab3cb..86c4f43e88f5 100644
> >> --- a/drivers/mci/dw_mmc.c
> >> +++ b/drivers/mci/dw_mmc.c
> >> @@ -572,7 +572,7 @@ static int dw_mmc_probe(struct device_d *dev)
> >>  
> >>  	rst = reset_control_get(dev, "reset");
> > 
> > Philipp, the reset binding lists the reset-names property as optional.
> > What's the expected behaviour of the reset_control_get() above when the
> > reset-names property is not present in the device tree? Should it return
> > an error or should it return the unnamed reset control?
> 
> can this revert still be applied for master? I'll look into reworking this
> for next.

Did that.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox