mbox series

[00/21] spi: s3c64xx: winter cleanup and gs101 support

Message ID 20240123153421.715951-1-tudor.ambarus@linaro.org (mailing list archive)
Headers show
Series spi: s3c64xx: winter cleanup and gs101 support | expand

Message

Tudor Ambarus Jan. 23, 2024, 3:33 p.m. UTC
Hi,

The patch set cleans a bit the driver and adds support for gs101 SPI.

Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
in asm-generic/io.h. This will allow devices that require 32 bits
register accesses to write data in chunks of 8 or 16 bits (a typical use
case is SPI, where clients can request transfers in words of 8 bits for
example). GS101 only allows 32bit register accesses otherwise it raisses
a Serror Interrupt and hangs the system, thus the accessors are needed
here. If the accessors are fine, I expect they'll be queued either to
the SPI tree or to the ASM header files tree, but by providing an
immutable tag, so that the other tree can merge them too.

The SPI patches were tested with the spi-loopback-test on the gs101
controller.

Thanks!
ta

Tudor Ambarus (21):
  spi: dt-bindings: samsung: add google,gs101-spi compatible
  spi: s3c64xx: sort headers alphabetically
  spi: s3c64xx: remove extra blank line
  spi: s3c64xx: remove unneeded (void *) casts in of_match_table
  spi: s3c64xx: explicitly include <linux/bits.h>
  spi: s3c64xx: remove else after return
  spi: s3c64xx: use bitfield access macros
  spi: s3c64xx: move error check up to avoid rechecking
  spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL
  spi: s3c64xx: move common code outside if else
  spi: s3c64xx: check return code of dmaengine_slave_config()
  spi: s3c64xx: propagate the dma_submit_error() error code
  spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()
  spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()
  spi: s3c64xx: simplify s3c64xx_wait_for_pio()
  spi: s3c64xx: add missing blank line after declaration
  spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
  asm-generic/io.h: add iowrite{8,16}_32 accessors
  spi: s3c64xx: add support for google,gs101-spi
  spi: s3c64xx: make the SPI alias optional for newer SoCs
  MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver

 .../devicetree/bindings/spi/samsung,spi.yaml  |   1 +
 MAINTAINERS                                   |   1 +
 drivers/spi/spi-s3c64xx.c                     | 447 +++++++++---------
 include/asm-generic/io.h                      |  50 ++
 4 files changed, 276 insertions(+), 223 deletions(-)

Comments

Mark Brown Jan. 23, 2024, 7 p.m. UTC | #1
On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote:

> The patch set cleans a bit the driver and adds support for gs101 SPI.
> 
> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> in asm-generic/io.h. This will allow devices that require 32 bits
> register accesses to write data in chunks of 8 or 16 bits (a typical use
> case is SPI, where clients can request transfers in words of 8 bits for
> example). GS101 only allows 32bit register accesses otherwise it raisses
> a Serror Interrupt and hangs the system, thus the accessors are needed
> here. If the accessors are fine, I expect they'll be queued either to
> the SPI tree or to the ASM header files tree, but by providing an
> immutable tag, so that the other tree can merge them too.
> 
> The SPI patches were tested with the spi-loopback-test on the gs101
> controller.

The reformatting in this series will conflict with the SPI changes in:

   https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org

Can you please pull those into this series or otherwise coordinate?
Sam Protsenko Jan. 23, 2024, 7:04 p.m. UTC | #2
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi,
>
> The patch set cleans a bit the driver and adds support for gs101 SPI.
>

It might be more convenient (for review purposes) to extract all the
cleanup patches into a separate series, and base it on top of the
gs101 SPI enablement series.

> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> in asm-generic/io.h. This will allow devices that require 32 bits
> register accesses to write data in chunks of 8 or 16 bits (a typical use
> case is SPI, where clients can request transfers in words of 8 bits for
> example). GS101 only allows 32bit register accesses otherwise it raisses
> a Serror Interrupt and hangs the system, thus the accessors are needed
> here. If the accessors are fine, I expect they'll be queued either to
> the SPI tree or to the ASM header files tree, but by providing an
> immutable tag, so that the other tree can merge them too.
>
> The SPI patches were tested with the spi-loopback-test on the gs101
> controller.
>
> Thanks!
> ta
>
> Tudor Ambarus (21):
>   spi: dt-bindings: samsung: add google,gs101-spi compatible
>   spi: s3c64xx: sort headers alphabetically
>   spi: s3c64xx: remove extra blank line
>   spi: s3c64xx: remove unneeded (void *) casts in of_match_table
>   spi: s3c64xx: explicitly include <linux/bits.h>
>   spi: s3c64xx: remove else after return
>   spi: s3c64xx: use bitfield access macros
>   spi: s3c64xx: move error check up to avoid rechecking
>   spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL
>   spi: s3c64xx: move common code outside if else
>   spi: s3c64xx: check return code of dmaengine_slave_config()
>   spi: s3c64xx: propagate the dma_submit_error() error code
>   spi: s3c64xx: rename prepare_dma() to s3c64xx_prepare_dma()
>   spi: s3c64xx: return ETIMEDOUT for wait_for_completion_timeout()
>   spi: s3c64xx: simplify s3c64xx_wait_for_pio()
>   spi: s3c64xx: add missing blank line after declaration
>   spi: s3c64xx: downgrade dev_warn to dev_dbg for optional dt props
>   asm-generic/io.h: add iowrite{8,16}_32 accessors
>   spi: s3c64xx: add support for google,gs101-spi
>   spi: s3c64xx: make the SPI alias optional for newer SoCs
>   MAINTAINERS: add Tudor Ambarus as R for the samsung SPI driver
>
>  .../devicetree/bindings/spi/samsung,spi.yaml  |   1 +
>  MAINTAINERS                                   |   1 +
>  drivers/spi/spi-s3c64xx.c                     | 447 +++++++++---------
>  include/asm-generic/io.h                      |  50 ++
>  4 files changed, 276 insertions(+), 223 deletions(-)
>
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Tudor Ambarus Jan. 24, 2024, 5:01 a.m. UTC | #3
On 1/23/24 19:00, Mark Brown wrote:
> On Tue, Jan 23, 2024 at 03:33:59PM +0000, Tudor Ambarus wrote:
> 
>> The patch set cleans a bit the driver and adds support for gs101 SPI.
>>
>> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
>> in asm-generic/io.h. This will allow devices that require 32 bits
>> register accesses to write data in chunks of 8 or 16 bits (a typical use
>> case is SPI, where clients can request transfers in words of 8 bits for
>> example). GS101 only allows 32bit register accesses otherwise it raisses
>> a Serror Interrupt and hangs the system, thus the accessors are needed
>> here. If the accessors are fine, I expect they'll be queued either to
>> the SPI tree or to the ASM header files tree, but by providing an
>> immutable tag, so that the other tree can merge them too.
>>
>> The SPI patches were tested with the spi-loopback-test on the gs101
>> controller.
> 
> The reformatting in this series will conflict with the SPI changes in:
> 
>    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> 
> Can you please pull those into this series or otherwise coordinate?

ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
adapt if necessary. I'll review that set in a sec.

Cheers,
ta
Andi Shyti Jan. 25, 2024, 10:25 p.m. UTC | #4
Hi Tudor,

> >> The patch set cleans a bit the driver and adds support for gs101 SPI.
> >>
> >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> >> in asm-generic/io.h. This will allow devices that require 32 bits
> >> register accesses to write data in chunks of 8 or 16 bits (a typical use
> >> case is SPI, where clients can request transfers in words of 8 bits for
> >> example). GS101 only allows 32bit register accesses otherwise it raisses
> >> a Serror Interrupt and hangs the system, thus the accessors are needed
> >> here. If the accessors are fine, I expect they'll be queued either to
> >> the SPI tree or to the ASM header files tree, but by providing an
> >> immutable tag, so that the other tree can merge them too.
> >>
> >> The SPI patches were tested with the spi-loopback-test on the gs101
> >> controller.
> > 
> > The reformatting in this series will conflict with the SPI changes in:
> > 
> >    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> > 
> > Can you please pull those into this series or otherwise coordinate?
> 
> ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
> adapt if necessary. I'll review that set in a sec.

it's a long series, please give it a few days before resending
it.

Thanks,
Andi
Sam Protsenko Jan. 25, 2024, 10:34 p.m. UTC | #5
On Thu, Jan 25, 2024 at 4:25 PM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Tudor,
>
> > >> The patch set cleans a bit the driver and adds support for gs101 SPI.
> > >>
> > >> Apart of the SPI patches, I added support for iowrite{8,16}_32 accessors
> > >> in asm-generic/io.h. This will allow devices that require 32 bits
> > >> register accesses to write data in chunks of 8 or 16 bits (a typical use
> > >> case is SPI, where clients can request transfers in words of 8 bits for
> > >> example). GS101 only allows 32bit register accesses otherwise it raisses
> > >> a Serror Interrupt and hangs the system, thus the accessors are needed
> > >> here. If the accessors are fine, I expect they'll be queued either to
> > >> the SPI tree or to the ASM header files tree, but by providing an
> > >> immutable tag, so that the other tree can merge them too.
> > >>
> > >> The SPI patches were tested with the spi-loopback-test on the gs101
> > >> controller.
> > >
> > > The reformatting in this series will conflict with the SPI changes in:
> > >
> > >    https://lore.kernel.org/r/20240120012948.8836-1-semen.protsenko@linaro.org
> > >
> > > Can you please pull those into this series or otherwise coordinate?
> >
> > ah, I haven't noticed Sam's updates. I'll rebase on top of his set and
> > adapt if necessary. I'll review that set in a sec.
>
> it's a long series, please give it a few days before resending
> it.
>

Also, I recommend splitting it up in a way I suggested before:

  1. First add gs101 support with minimal amount of patches (without
.fifosize introduction, etc)
  2. Then do all those cleanups and reworks on top of that

The reason why I think it's better than doing that vice-versa is that
I feel (2) can take a lot of time/review rounds to get polished and
accepted. So this way you can make sure gs101 support gets applied
sooner. It'll also make it easier to do the backporting work later, if
that's ever needed.

> Thanks,
> Andi