| Message ID | 20240918082744.379610-2-ada@thorsis.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 329ca3eed4a9a161515a8714be6ba182321385c7 |
| Headers | show |
| Series | spi: atmel-quadspi: Fix and add full CS delay support | expand |
On Wed, Sep 18, 2024 at 10:27:43AM +0200, Alexander Dahl wrote: > Previously the MR and SCR registers were just set with the supposedly > required values, from cached register values (cached reg content > initialized to zero). > > All parts fixed here did not consider the current register (cache) > content, which would make future support of cs_setup, cs_hold, and > cs_inactive impossible. > > Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from > atmel_qspi_set_cs_timing(). The DLYBS setting is applied by ORing over > the current setting, without resetting the bits first. All writes to MR > did not consider possible settings of DLYCS and DLYBCT. > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing") This isn't actually a fix AFAICT since nothing yet sets any of these fields?
Hello Mark, Am Fri, Sep 20, 2024 at 10:22:19AM +0200 schrieb Mark Brown: > On Wed, Sep 18, 2024 at 10:27:43AM +0200, Alexander Dahl wrote: > > Previously the MR and SCR registers were just set with the supposedly > > required values, from cached register values (cached reg content > > initialized to zero). > > > > All parts fixed here did not consider the current register (cache) > > content, which would make future support of cs_setup, cs_hold, and > > cs_inactive impossible. > > > > Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from > > atmel_qspi_set_cs_timing(). The DLYBS setting is applied by ORing over > > the current setting, without resetting the bits first. All writes to MR > > did not consider possible settings of DLYCS and DLYBCT. > > > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > > Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing") > > This isn't actually a fix AFAICT since nothing yet sets any of these > fields? You're right if we just consider board dts files in mainline. None of those using the atmel-quadspi driver have a spi-cs-*-delay property set in a SPI slave device node in current master. The changes in this patch to MR writes do not change behaviour. For changes to SCR however I see two possible bugs: 1. if atmel_qspi_set_cs_timing() is called before atmel_qspi_setup(), the second call just overwrites what the first call set. 2. if atmel_qspi_set_cs_timing() is called multiple times with different values, the values written to the register from the second call onwards are just wrong. Maybe both are scenarios not happening in practice? Long story short, I could just remove the Fixes line, and the rest is fine? Or should I split up with changes for MR and SCR going to separate patches? Greets Alex
Hello everyone, Am Wed, Sep 18, 2024 at 10:27:43AM +0200 schrieb Alexander Dahl: > Previously the MR and SCR registers were just set with the supposedly > required values, from cached register values (cached reg content > initialized to zero). > > All parts fixed here did not consider the current register (cache) > content, which would make future support of cs_setup, cs_hold, and > cs_inactive impossible. > > Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from > atmel_qspi_set_cs_timing(). The DLYBS setting is applied by ORing over > the current setting, without resetting the bits first. All writes to MR > did not consider possible settings of DLYCS and DLYBCT. > > Signed-off-by: Alexander Dahl <ada@thorsis.com> > Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing") > --- > drivers/spi/atmel-quadspi.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c > index 5aaff3bee1b7..fcd57cf1f2cf 100644 > --- a/drivers/spi/atmel-quadspi.c > +++ b/drivers/spi/atmel-quadspi.c > @@ -375,9 +375,9 @@ static int atmel_qspi_set_cfg(struct atmel_qspi *aq, > * If the QSPI controller is set in regular SPI mode, set it in > * Serial Memory Mode (SMM). > */ > - if (aq->mr != QSPI_MR_SMM) { > - atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR); > - aq->mr = QSPI_MR_SMM; > + if (!(aq->mr & QSPI_MR_SMM)) { > + aq->mr |= QSPI_MR_SMM; > + atmel_qspi_write(aq->scr, aq, QSPI_MR); On a second glance, this looks wrong, value for Mode Register (MR) is written into Serial Clock Register (SCR), should be like this instead: atmel_qspi_write(aq->mr, aq, QSPI_MR); This is somewhat embarrassing, because the patch was already applied to master. Should it be reverted, then I would send a v2 of the series? Or should I send a quick fixup? Greets Alex > } > > /* Clear pending interrupts */ > @@ -501,7 +501,8 @@ static int atmel_qspi_setup(struct spi_device *spi) > if (ret < 0) > return ret; > > - aq->scr = QSPI_SCR_SCBR(scbr); > + aq->scr &= ~QSPI_SCR_SCBR_MASK; > + aq->scr |= QSPI_SCR_SCBR(scbr); > atmel_qspi_write(aq->scr, aq, QSPI_SCR); > > pm_runtime_mark_last_busy(ctrl->dev.parent); > @@ -534,6 +535,7 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi) > if (ret < 0) > return ret; > > + aq->scr &= ~QSPI_SCR_DLYBS_MASK; > aq->scr |= QSPI_SCR_DLYBS(cs_setup); > atmel_qspi_write(aq->scr, aq, QSPI_SCR); > > @@ -549,8 +551,8 @@ static void atmel_qspi_init(struct atmel_qspi *aq) > atmel_qspi_write(QSPI_CR_SWRST, aq, QSPI_CR); > > /* Set the QSPI controller by default in Serial Memory Mode */ > - atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR); > - aq->mr = QSPI_MR_SMM; > + aq->mr |= QSPI_MR_SMM; > + atmel_qspi_write(aq->mr, aq, QSPI_MR); > > /* Enable the QSPI controller */ > atmel_qspi_write(QSPI_CR_QSPIEN, aq, QSPI_CR); > -- > 2.39.5 > >
On Thu, Sep 26, 2024 at 09:25:10AM +0200, Alexander Dahl wrote: > This is somewhat embarrassing, because the patch was already applied > to master. Should it be reverted, then I would send a v2 of the > series? Or should I send a quick fixup? A quick fixup is better.
diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c index 5aaff3bee1b7..fcd57cf1f2cf 100644 --- a/drivers/spi/atmel-quadspi.c +++ b/drivers/spi/atmel-quadspi.c @@ -375,9 +375,9 @@ static int atmel_qspi_set_cfg(struct atmel_qspi *aq, * If the QSPI controller is set in regular SPI mode, set it in * Serial Memory Mode (SMM). */ - if (aq->mr != QSPI_MR_SMM) { - atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR); - aq->mr = QSPI_MR_SMM; + if (!(aq->mr & QSPI_MR_SMM)) { + aq->mr |= QSPI_MR_SMM; + atmel_qspi_write(aq->scr, aq, QSPI_MR); } /* Clear pending interrupts */ @@ -501,7 +501,8 @@ static int atmel_qspi_setup(struct spi_device *spi) if (ret < 0) return ret; - aq->scr = QSPI_SCR_SCBR(scbr); + aq->scr &= ~QSPI_SCR_SCBR_MASK; + aq->scr |= QSPI_SCR_SCBR(scbr); atmel_qspi_write(aq->scr, aq, QSPI_SCR); pm_runtime_mark_last_busy(ctrl->dev.parent); @@ -534,6 +535,7 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi) if (ret < 0) return ret; + aq->scr &= ~QSPI_SCR_DLYBS_MASK; aq->scr |= QSPI_SCR_DLYBS(cs_setup); atmel_qspi_write(aq->scr, aq, QSPI_SCR); @@ -549,8 +551,8 @@ static void atmel_qspi_init(struct atmel_qspi *aq) atmel_qspi_write(QSPI_CR_SWRST, aq, QSPI_CR); /* Set the QSPI controller by default in Serial Memory Mode */ - atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR); - aq->mr = QSPI_MR_SMM; + aq->mr |= QSPI_MR_SMM; + atmel_qspi_write(aq->mr, aq, QSPI_MR); /* Enable the QSPI controller */ atmel_qspi_write(QSPI_CR_QSPIEN, aq, QSPI_CR);
Previously the MR and SCR registers were just set with the supposedly required values, from cached register values (cached reg content initialized to zero). All parts fixed here did not consider the current register (cache) content, which would make future support of cs_setup, cs_hold, and cs_inactive impossible. Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from atmel_qspi_set_cs_timing(). The DLYBS setting is applied by ORing over the current setting, without resetting the bits first. All writes to MR did not consider possible settings of DLYCS and DLYBCT. Signed-off-by: Alexander Dahl <ada@thorsis.com> Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing") --- drivers/spi/atmel-quadspi.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)