| Message ID | 20240918082744.379610-1-ada@thorsis.com |
|---|---|
| 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
On Wed, 18 Sep 2024 10:27:42 +0200, Alexander Dahl wrote: > when testing on a SAM9X60 based board with an FPGA implementing a custom > SPI-MEM protocol, we found the need to fully control the delay settings > the atmel/microchip QSPI controller offers. > > Greets > Alex > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: atmel-quadspi: Avoid overwriting delay register settings commit: 329ca3eed4a9a161515a8714be6ba182321385c7 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Wed, 18 Sep 2024 10:27:42 +0200, Alexander Dahl wrote: > when testing on a SAM9X60 based board with an FPGA implementing a custom > SPI-MEM protocol, we found the need to fully control the delay settings > the atmel/microchip QSPI controller offers. > > Greets > Alex > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [2/2] spi: atmel-quadspi: Add cs_hold and cs_inactive setting support commit: 625de1881b5aee6a42a3130004e47dbd632429f8 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark