| Message ID | 20190221214332.4246-6-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | arm: socfpga: implement proper peripheral reset handling | expand |
On 2/21/19 10:43 PM, Simon Goldschmidt wrote: > This adds reset handling to the cadence qspi driver. > > For backwards compatibility, only a warning is printed when failing to > get reset handles. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > > Changes in v2: > - add .remove callback to release the resets > > drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ > drivers/spi/cadence_qspi.h | 4 ++++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > index 11fce9c4fe..3bfa0201c4 100644 > --- a/drivers/spi/cadence_qspi.c > +++ b/drivers/spi/cadence_qspi.c > @@ -8,6 +8,7 @@ > #include <dm.h> > #include <fdtdec.h> > #include <malloc.h> > +#include <reset.h> > #include <spi.h> > #include <linux/errno.h> > #include "cadence_qspi.h" > @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice *bus) > { > struct cadence_spi_platdata *plat = bus->platdata; > struct cadence_spi_priv *priv = dev_get_priv(bus); > + int ret; > > priv->regbase = plat->regbase; > priv->ahbbase = plat->ahbbase; > > + ret = reset_get_bulk(bus, &priv->resets); > + if (ret) > + dev_warn(bus, "Can't get reset: %d\n", ret); > + else > + reset_deassert_bulk(&priv->resets); > + > if (!priv->qspi_is_init) { > cadence_qspi_apb_controller_init(plat); > priv->qspi_is_init = 1; > @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) > return 0; > } > > +static int cadence_spi_remove(struct udevice *dev) > +{ > + struct cadence_spi_priv *priv = dev_get_priv(dev); > + > + return reset_release_bulk(&priv->resets); > +} > + > static int cadence_spi_set_mode(struct udevice *bus, uint mode) > { > struct cadence_spi_priv *priv = dev_get_priv(bus); > @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { > .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), > .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), > .probe = cadence_spi_probe, > + .remove = cadence_spi_remove, .remove() only ever gets executed for drivers setting DM_FLAG_OS_PREPARE flag. Fix this in the other drivers too.
Am Do., 21. Feb. 2019, 22:56 hat Marek Vasut <marex@denx.de> geschrieben: > On 2/21/19 10:43 PM, Simon Goldschmidt wrote: > > This adds reset handling to the cadence qspi driver. > > > > For backwards compatibility, only a warning is printed when failing to > > get reset handles. > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > > --- > > > > Changes in v2: > > - add .remove callback to release the resets > > > > drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ > > drivers/spi/cadence_qspi.h | 4 ++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > > index 11fce9c4fe..3bfa0201c4 100644 > > --- a/drivers/spi/cadence_qspi.c > > +++ b/drivers/spi/cadence_qspi.c > > @@ -8,6 +8,7 @@ > > #include <dm.h> > > #include <fdtdec.h> > > #include <malloc.h> > > +#include <reset.h> > > #include <spi.h> > > #include <linux/errno.h> > > #include "cadence_qspi.h" > > @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice *bus) > > { > > struct cadence_spi_platdata *plat = bus->platdata; > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > + int ret; > > > > priv->regbase = plat->regbase; > > priv->ahbbase = plat->ahbbase; > > > > + ret = reset_get_bulk(bus, &priv->resets); > > + if (ret) > > + dev_warn(bus, "Can't get reset: %d\n", ret); > > + else > > + reset_deassert_bulk(&priv->resets); > > + > > if (!priv->qspi_is_init) { > > cadence_qspi_apb_controller_init(plat); > > priv->qspi_is_init = 1; > > @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) > > return 0; > > } > > > > +static int cadence_spi_remove(struct udevice *dev) > > +{ > > + struct cadence_spi_priv *priv = dev_get_priv(dev); > > + > > + return reset_release_bulk(&priv->resets); > > +} > > + > > static int cadence_spi_set_mode(struct udevice *bus, uint mode) > > { > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { > > .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), > > .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), > > .probe = cadence_spi_probe, > > + .remove = cadence_spi_remove, > > .remove() only ever gets executed for drivers setting DM_FLAG_OS_PREPARE > flag. Fix this in the other drivers too. > Ehrm, I haven't checked, but is this common practice? Why doesn't it always get called? Regards, Simon >
On 2/21/19 11:04 PM, Simon Goldschmidt wrote: > > > Am Do., 21. Feb. 2019, 22:56 hat Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> geschrieben: > > On 2/21/19 10:43 PM, Simon Goldschmidt wrote: > > This adds reset handling to the cadence qspi driver. > > > > For backwards compatibility, only a warning is printed when failing to > > get reset handles. > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com > <mailto:simon.k.r.goldschmidt@gmail.com>> > > --- > > > > Changes in v2: > > - add .remove callback to release the resets > > > > drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ > > drivers/spi/cadence_qspi.h | 4 ++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > > index 11fce9c4fe..3bfa0201c4 100644 > > --- a/drivers/spi/cadence_qspi.c > > +++ b/drivers/spi/cadence_qspi.c > > @@ -8,6 +8,7 @@ > > #include <dm.h> > > #include <fdtdec.h> > > #include <malloc.h> > > +#include <reset.h> > > #include <spi.h> > > #include <linux/errno.h> > > #include "cadence_qspi.h" > > @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice > *bus) > > { > > struct cadence_spi_platdata *plat = bus->platdata; > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > + int ret; > > > > priv->regbase = plat->regbase; > > priv->ahbbase = plat->ahbbase; > > > > + ret = reset_get_bulk(bus, &priv->resets); > > + if (ret) > > + dev_warn(bus, "Can't get reset: %d\n", ret); > > + else > > + reset_deassert_bulk(&priv->resets); > > + > > if (!priv->qspi_is_init) { > > cadence_qspi_apb_controller_init(plat); > > priv->qspi_is_init = 1; > > @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) > > return 0; > > } > > > > +static int cadence_spi_remove(struct udevice *dev) > > +{ > > + struct cadence_spi_priv *priv = dev_get_priv(dev); > > + > > + return reset_release_bulk(&priv->resets); > > +} > > + > > static int cadence_spi_set_mode(struct udevice *bus, uint mode) > > { > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { > > .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), > > .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), > > .probe = cadence_spi_probe, > > + .remove = cadence_spi_remove, > > .remove() only ever gets executed for drivers setting DM_FLAG_OS_PREPARE > flag. Fix this in the other drivers too. > > > Ehrm, I haven't checked, but is this common practice? Why doesn't it > always get called? That's how the code behaves. Probably to speed up booting the kernel on devices which don't need to be torn down.
On Thu, Feb 21, 2019 at 11:14 PM Marek Vasut <marex@denx.de> wrote: > > On 2/21/19 11:04 PM, Simon Goldschmidt wrote: > > > > > > Am Do., 21. Feb. 2019, 22:56 hat Marek Vasut <marex@denx.de > > <mailto:marex@denx.de>> geschrieben: > > > > On 2/21/19 10:43 PM, Simon Goldschmidt wrote: > > > This adds reset handling to the cadence qspi driver. > > > > > > For backwards compatibility, only a warning is printed when failing to > > > get reset handles. > > > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com > > <mailto:simon.k.r.goldschmidt@gmail.com>> > > > --- > > > > > > Changes in v2: > > > - add .remove callback to release the resets > > > > > > drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ > > > drivers/spi/cadence_qspi.h | 4 ++++ > > > 2 files changed, 20 insertions(+) > > > > > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > > > index 11fce9c4fe..3bfa0201c4 100644 > > > --- a/drivers/spi/cadence_qspi.c > > > +++ b/drivers/spi/cadence_qspi.c > > > @@ -8,6 +8,7 @@ > > > #include <dm.h> > > > #include <fdtdec.h> > > > #include <malloc.h> > > > +#include <reset.h> > > > #include <spi.h> > > > #include <linux/errno.h> > > > #include "cadence_qspi.h" > > > @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice > > *bus) > > > { > > > struct cadence_spi_platdata *plat = bus->platdata; > > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > > + int ret; > > > > > > priv->regbase = plat->regbase; > > > priv->ahbbase = plat->ahbbase; > > > > > > + ret = reset_get_bulk(bus, &priv->resets); > > > + if (ret) > > > + dev_warn(bus, "Can't get reset: %d\n", ret); > > > + else > > > + reset_deassert_bulk(&priv->resets); > > > + > > > if (!priv->qspi_is_init) { > > > cadence_qspi_apb_controller_init(plat); > > > priv->qspi_is_init = 1; > > > @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) > > > return 0; > > > } > > > > > > +static int cadence_spi_remove(struct udevice *dev) > > > +{ > > > + struct cadence_spi_priv *priv = dev_get_priv(dev); > > > + > > > + return reset_release_bulk(&priv->resets); > > > +} > > > + > > > static int cadence_spi_set_mode(struct udevice *bus, uint mode) > > > { > > > struct cadence_spi_priv *priv = dev_get_priv(bus); > > > @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { > > > .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), > > > .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), > > > .probe = cadence_spi_probe, > > > + .remove = cadence_spi_remove, > > > > .remove() only ever gets executed for drivers setting DM_FLAG_OS_PREPARE > > flag. Fix this in the other drivers too. > > > > > > Ehrm, I haven't checked, but is this common practice? Why doesn't it > > always get called? > > That's how the code behaves. Probably to speed up booting the kernel on > devices which don't need to be torn down. What surprises me is that the OS_PREPARE flag is used only for one spi driver and for 'mmc_blk' (but this is really new). Is it still the right thing to do? How could this be one of the first drivers releasing its reset before boot? :-) Regards, Simon
On 2/22/19 7:06 AM, Simon Goldschmidt wrote: > On Thu, Feb 21, 2019 at 11:14 PM Marek Vasut <marex@denx.de> wrote: >> >> On 2/21/19 11:04 PM, Simon Goldschmidt wrote: >>> >>> >>> Am Do., 21. Feb. 2019, 22:56 hat Marek Vasut <marex@denx.de >>> <mailto:marex@denx.de>> geschrieben: >>> >>> On 2/21/19 10:43 PM, Simon Goldschmidt wrote: >>> > This adds reset handling to the cadence qspi driver. >>> > >>> > For backwards compatibility, only a warning is printed when failing to >>> > get reset handles. >>> > >>> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com >>> <mailto:simon.k.r.goldschmidt@gmail.com>> >>> > --- >>> > >>> > Changes in v2: >>> > - add .remove callback to release the resets >>> > >>> > drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ >>> > drivers/spi/cadence_qspi.h | 4 ++++ >>> > 2 files changed, 20 insertions(+) >>> > >>> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c >>> > index 11fce9c4fe..3bfa0201c4 100644 >>> > --- a/drivers/spi/cadence_qspi.c >>> > +++ b/drivers/spi/cadence_qspi.c >>> > @@ -8,6 +8,7 @@ >>> > #include <dm.h> >>> > #include <fdtdec.h> >>> > #include <malloc.h> >>> > +#include <reset.h> >>> > #include <spi.h> >>> > #include <linux/errno.h> >>> > #include "cadence_qspi.h" >>> > @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice >>> *bus) >>> > { >>> > struct cadence_spi_platdata *plat = bus->platdata; >>> > struct cadence_spi_priv *priv = dev_get_priv(bus); >>> > + int ret; >>> > >>> > priv->regbase = plat->regbase; >>> > priv->ahbbase = plat->ahbbase; >>> > >>> > + ret = reset_get_bulk(bus, &priv->resets); >>> > + if (ret) >>> > + dev_warn(bus, "Can't get reset: %d\n", ret); >>> > + else >>> > + reset_deassert_bulk(&priv->resets); >>> > + >>> > if (!priv->qspi_is_init) { >>> > cadence_qspi_apb_controller_init(plat); >>> > priv->qspi_is_init = 1; >>> > @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) >>> > return 0; >>> > } >>> > >>> > +static int cadence_spi_remove(struct udevice *dev) >>> > +{ >>> > + struct cadence_spi_priv *priv = dev_get_priv(dev); >>> > + >>> > + return reset_release_bulk(&priv->resets); >>> > +} >>> > + >>> > static int cadence_spi_set_mode(struct udevice *bus, uint mode) >>> > { >>> > struct cadence_spi_priv *priv = dev_get_priv(bus); >>> > @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { >>> > .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), >>> > .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), >>> > .probe = cadence_spi_probe, >>> > + .remove = cadence_spi_remove, >>> >>> .remove() only ever gets executed for drivers setting DM_FLAG_OS_PREPARE >>> flag. Fix this in the other drivers too. >>> >>> >>> Ehrm, I haven't checked, but is this common practice? Why doesn't it >>> always get called? >> >> That's how the code behaves. Probably to speed up booting the kernel on >> devices which don't need to be torn down. > > What surprises me is that the OS_PREPARE flag is used only for one spi driver > and for 'mmc_blk' (but this is really new). Is it still the right > thing to do? How could > this be one of the first drivers releasing its reset before boot? :-) I suspect this might be an area for improvement :)
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index 11fce9c4fe..3bfa0201c4 100644 --- a/drivers/spi/cadence_qspi.c +++ b/drivers/spi/cadence_qspi.c @@ -8,6 +8,7 @@ #include <dm.h> #include <fdtdec.h> #include <malloc.h> +#include <reset.h> #include <spi.h> #include <linux/errno.h> #include "cadence_qspi.h" @@ -154,10 +155,17 @@ static int cadence_spi_probe(struct udevice *bus) { struct cadence_spi_platdata *plat = bus->platdata; struct cadence_spi_priv *priv = dev_get_priv(bus); + int ret; priv->regbase = plat->regbase; priv->ahbbase = plat->ahbbase; + ret = reset_get_bulk(bus, &priv->resets); + if (ret) + dev_warn(bus, "Can't get reset: %d\n", ret); + else + reset_deassert_bulk(&priv->resets); + if (!priv->qspi_is_init) { cadence_qspi_apb_controller_init(plat); priv->qspi_is_init = 1; @@ -166,6 +174,13 @@ static int cadence_spi_probe(struct udevice *bus) return 0; } +static int cadence_spi_remove(struct udevice *dev) +{ + struct cadence_spi_priv *priv = dev_get_priv(dev); + + return reset_release_bulk(&priv->resets); +} + static int cadence_spi_set_mode(struct udevice *bus, uint mode) { struct cadence_spi_priv *priv = dev_get_priv(bus); @@ -342,4 +357,5 @@ U_BOOT_DRIVER(cadence_spi) = { .platdata_auto_alloc_size = sizeof(struct cadence_spi_platdata), .priv_auto_alloc_size = sizeof(struct cadence_spi_priv), .probe = cadence_spi_probe, + .remove = cadence_spi_remove, }; diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index 055900def0..d4ede6e15e 100644 --- a/drivers/spi/cadence_qspi.h +++ b/drivers/spi/cadence_qspi.h @@ -7,6 +7,8 @@ #ifndef __CADENCE_QSPI_H__ #define __CADENCE_QSPI_H__ +#include <reset.h> + #define CQSPI_IS_ADDR(cmd_len) (cmd_len > 1 ? 1 : 0) #define CQSPI_NO_DECODER_MAX_CS 4 @@ -42,6 +44,8 @@ struct cadence_spi_priv { unsigned int qspi_calibrated_hz; unsigned int qspi_calibrated_cs; unsigned int previous_hz; + + struct reset_ctl_bulk resets; }; /* Functions call declaration */
This adds reset handling to the cadence qspi driver. For backwards compatibility, only a warning is printed when failing to get reset handles. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- Changes in v2: - add .remove callback to release the resets drivers/spi/cadence_qspi.c | 16 ++++++++++++++++ drivers/spi/cadence_qspi.h | 4 ++++ 2 files changed, 20 insertions(+)