Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Alexander Dahl <ada@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > 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