* [PATCH master] mci: dw_mmc: make reset control optional again
@ 2021-11-01 17:52 Ahmad Fatoum
2021-11-02 8:06 ` Sascha Hauer
0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2021-11-01 17:52 UTC (permalink / raw)
To: barebox; +Cc: Ian Abbott, 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH master] mci: dw_mmc: make reset control optional again
2021-11-01 17:52 [PATCH master] mci: dw_mmc: make reset control optional again Ahmad Fatoum
@ 2021-11-02 8:06 ` Sascha Hauer
2021-11-02 9:04 ` Philipp Zabel
2021-11-08 10:28 ` Ahmad Fatoum
0 siblings, 2 replies; 5+ messages in thread
From: Sascha Hauer @ 2021-11-02 8:06 UTC (permalink / raw)
To: Philipp Zabel; +Cc: Ahmad Fatoum, barebox, Ian Abbott
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH master] mci: dw_mmc: make reset control optional again
2021-11-02 8:06 ` Sascha Hauer
@ 2021-11-02 9:04 ` Philipp Zabel
2021-11-08 10:28 ` Ahmad Fatoum
1 sibling, 0 replies; 5+ messages in thread
From: Philipp Zabel @ 2021-11-02 9:04 UTC (permalink / raw)
To: Sascha Hauer, Philipp Zabel; +Cc: Ahmad Fatoum, barebox, Ian Abbott
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH master] mci: dw_mmc: make reset control optional again
2021-11-02 8:06 ` Sascha Hauer
2021-11-02 9:04 ` Philipp Zabel
@ 2021-11-08 10:28 ` Ahmad Fatoum
2021-11-10 8:16 ` Sascha Hauer
1 sibling, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2021-11-08 10:28 UTC (permalink / raw)
To: Sascha Hauer, Philipp Zabel; +Cc: barebox, Ian Abbott
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH master] mci: dw_mmc: make reset control optional again
2021-11-08 10:28 ` Ahmad Fatoum
@ 2021-11-10 8:16 ` Sascha Hauer
0 siblings, 0 replies; 5+ messages in thread
From: Sascha Hauer @ 2021-11-10 8:16 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: Philipp Zabel, barebox, Ian Abbott
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-10 8:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-01 17:52 [PATCH master] mci: dw_mmc: make reset control optional again Ahmad Fatoum
2021-11-02 8:06 ` Sascha Hauer
2021-11-02 9:04 ` Philipp Zabel
2021-11-08 10:28 ` Ahmad Fatoum
2021-11-10 8:16 ` Sascha Hauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox