Message ID | 20190409153801.6941-1-dinguyen@kernel.org |
---|---|
State | Changes Requested |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | [PATCHv2] mtd: spi-nor: cadence-quadspi: add reset control | expand |
Hi, Dinh, On 04/09/2019 06:38 PM, Dinh Nguyen wrote: > Get the reset control for the QSPI controller and bring it out of reset. Is there a public datasheet where I can check this? > > Suggested-by: Tien-Fong Chee <tien.fong.chee@intel.com> > Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> > --- > v2: use devm_reset_control_get_optional_exclusive > print an error message > return -EPROBE_DEFER > --- > drivers/mtd/spi-nor/cadence-quadspi.c | 14 ++++++++++++++ would you update the bindings as well? > 1 file changed, 14 insertions(+) > > diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c > index 792628750eec..c548567adcf0 100644 > --- a/drivers/mtd/spi-nor/cadence-quadspi.c > +++ b/drivers/mtd/spi-nor/cadence-quadspi.c > @@ -34,6 +34,7 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/reset.h> > #include <linux/sched.h> > #include <linux/spi/spi.h> > #include <linux/timer.h> > @@ -1336,6 +1337,7 @@ static int cqspi_probe(struct platform_device *pdev) > struct cqspi_st *cqspi; > struct resource *res; > struct resource *res_ahb; > + struct reset_control *rstc; > const struct cqspi_driver_platdata *ddata; > int ret; > int irq; > @@ -1362,6 +1364,18 @@ static int cqspi_probe(struct platform_device *pdev) > return PTR_ERR(cqspi->clk); > } > > + /* Obtain QSPI reset control */ OSPI version of the IP has been added recently. Is this available just for QSPI? > + rstc = devm_reset_control_get_optional_exclusive(dev, NULL); > + if (IS_ERR(rstc)) { > + dev_err(dev, "Cannot get QSPI reset.\n"); > + if (PTR_ERR(rstc) == -EPROBE_DEFER) > + return -EPROBE_DEFER; why ignoring all the other possible errors? Maybe return PTR_ERR(rstc); ? > + } else { > + reset_control_assert(rstc); Shouldn't the clock be enabled before asserting reset? Does it matter if reset_control_assert() is returning any error? > + udelay(1); Is there a range or 1 us is the maximum time that we should wait? > + reset_control_deassert(rstc); Should we check if reset_control_deassert() is returning any error? Cheers, ta > + } > + > /* Obtain and remap controller address. */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > cqspi->iobase = devm_ioremap_resource(dev, res); >
On 4/17/19 3:48 AM, Tudor.Ambarus@microchip.com wrote: > Hi, Dinh, > > On 04/09/2019 06:38 PM, Dinh Nguyen wrote: >> Get the reset control for the QSPI controller and bring it out of reset. > > Is there a public datasheet where I can check this? You can look at the reset manager bits here: https://www.intel.com/content/www/us/en/programmable/hps/arria-10/hps.html#reg_soc_top/sfo1429890575955.html Otherwise, the QSPI block is just the standard IP from Cadence. >> >> Suggested-by: Tien-Fong Chee <tien.fong.chee@intel.com> >> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> >> --- >> v2: use devm_reset_control_get_optional_exclusive >> print an error message >> return -EPROBE_DEFER >> --- >> drivers/mtd/spi-nor/cadence-quadspi.c | 14 ++++++++++++++ > > would you update the bindings as well? Yes, I will do that. > >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c >> index 792628750eec..c548567adcf0 100644 >> --- a/drivers/mtd/spi-nor/cadence-quadspi.c >> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c >> @@ -34,6 +34,7 @@ >> #include <linux/of.h> >> #include <linux/platform_device.h> >> #include <linux/pm_runtime.h> >> +#include <linux/reset.h> >> #include <linux/sched.h> >> #include <linux/spi/spi.h> >> #include <linux/timer.h> >> @@ -1336,6 +1337,7 @@ static int cqspi_probe(struct platform_device *pdev) >> struct cqspi_st *cqspi; >> struct resource *res; >> struct resource *res_ahb; >> + struct reset_control *rstc; >> const struct cqspi_driver_platdata *ddata; >> int ret; >> int irq; >> @@ -1362,6 +1364,18 @@ static int cqspi_probe(struct platform_device *pdev) >> return PTR_ERR(cqspi->clk); >> } >> >> + /* Obtain QSPI reset control */ > > OSPI version of the IP has been added recently. Is this available just for QSPI? > >> + rstc = devm_reset_control_get_optional_exclusive(dev, NULL); >> + if (IS_ERR(rstc)) { >> + dev_err(dev, "Cannot get QSPI reset.\n"); >> + if (PTR_ERR(rstc) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; > > why ignoring all the other possible errors? Maybe return PTR_ERR(rstc); ? Ok.. > >> + } else { >> + reset_control_assert(rstc); > > Shouldn't the clock be enabled before asserting reset? > Yes, it should. Thanks for catching that! > Does it matter if reset_control_assert() is returning any error? > >> + udelay(1); > > Is there a range or 1 us is the maximum time that we should wait? > >> + reset_control_deassert(rstc); I'll add a 1-2 us range. > > Should we check if reset_control_deassert() is returning any error? > I don't think there is a need for error checking here. We've already made sure that the reset property is there. Thanks for the review... Dinh
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c index 792628750eec..c548567adcf0 100644 --- a/drivers/mtd/spi-nor/cadence-quadspi.c +++ b/drivers/mtd/spi-nor/cadence-quadspi.c @@ -34,6 +34,7 @@ #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include <linux/sched.h> #include <linux/spi/spi.h> #include <linux/timer.h> @@ -1336,6 +1337,7 @@ static int cqspi_probe(struct platform_device *pdev) struct cqspi_st *cqspi; struct resource *res; struct resource *res_ahb; + struct reset_control *rstc; const struct cqspi_driver_platdata *ddata; int ret; int irq; @@ -1362,6 +1364,18 @@ static int cqspi_probe(struct platform_device *pdev) return PTR_ERR(cqspi->clk); } + /* Obtain QSPI reset control */ + rstc = devm_reset_control_get_optional_exclusive(dev, NULL); + if (IS_ERR(rstc)) { + dev_err(dev, "Cannot get QSPI reset.\n"); + if (PTR_ERR(rstc) == -EPROBE_DEFER) + return -EPROBE_DEFER; + } else { + reset_control_assert(rstc); + udelay(1); + reset_control_deassert(rstc); + } + /* Obtain and remap controller address. */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); cqspi->iobase = devm_ioremap_resource(dev, res);
Get the reset control for the QSPI controller and bring it out of reset. Suggested-by: Tien-Fong Chee <tien.fong.chee@intel.com> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org> --- v2: use devm_reset_control_get_optional_exclusive print an error message return -EPROBE_DEFER --- drivers/mtd/spi-nor/cadence-quadspi.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)