diff mbox series

[19/21] spi: s3c64xx: add support for google,gs101-spi

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

Commit Message

Tudor Ambarus Jan. 23, 2024, 3:34 p.m. UTC
Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
register accesses, otherwise a Serror Interrupt is raised. Do the write
reg accesses in 32 bits.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

Comments

Sam Protsenko Jan. 23, 2024, 7:25 p.m. UTC | #1
On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
> register accesses, otherwise a Serror Interrupt is raised. Do the write
> reg accesses in 32 bits.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---

I counted 3 different features in this patch. Would be better to split
it correspondingly into 3 patches, to make patches atomic:

  1. I/O width
  2. FIFO size
  3. Adding support for gs101

And I'm not really convinced about FIFO size change.

>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 62671b2d594a..c4ddd2859ba4 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -20,6 +20,7 @@
>
>  #define MAX_SPI_PORTS                          12
>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
>  #define AUTOSUSPEND_TIMEOUT                    2000
>
>  /* Registers and bit-fields */
> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
>   * @clk_div: Internal clock divider
> + * @fifosize: size of the FIFO
>   * @quirks: Bitmask of known quirks
>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
>   * @clk_from_cmu: True, if the controller does not include a clock mux and
> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
>         int     tx_st_done;
>         int     quirks;
>         int     clk_div;
> +       unsigned int fifosize;
>         bool    high_speed;
>         bool    clk_from_cmu;
>         bool    clk_ioclk;
> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
>   * @port_conf: Local SPI port configuartion data
>   * @port_id: Port identification number
> + * @fifosize: size of the FIFO for this port
>   */
>  struct s3c64xx_spi_driver_data {
>         void __iomem                    *regs;
> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
>         struct s3c64xx_spi_dma_data     tx_dma;
>         const struct s3c64xx_spi_port_config    *port_conf;
>         unsigned int                    port_id;
> +       unsigned int                    fifosize;
>  };
>
>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>
>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
> -               return xfer->len > FIFO_DEPTH(sdd);
> +               return xfer->len > sdd->fifosize;
>
>         return false;
>  }
> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>                                         xfer->tx_buf, xfer->len / 4);
>                                 break;
>                         case 16:
> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len / 2);
> +                               if (sdd->port_conf->quirks &
> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                        xfer->tx_buf, xfer->len / 2);
> +                               else
> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                     xfer->tx_buf, xfer->len / 2);
>                                 break;
>                         default:
> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> -                                       xfer->tx_buf, xfer->len);
> +                               if (sdd->port_conf->quirks &
> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                       xfer->tx_buf, xfer->len);
> +                               else
> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> +                                                    xfer->tx_buf, xfer->len);
>                                 break;
>                         }
>                 }
> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>                                     struct spi_transfer *xfer)
>  {
>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
> +       const unsigned int fifo_len = sdd->fifosize;
>         const void *tx_buf = NULL;
>         void *rx_buf = NULL;
>         int target_len = 0, origin_len = 0;
> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>                 sdd->port_id = pdev->id;
>         }
>
> +       if (sdd->port_conf->fifosize)
> +               sdd->fifosize = sdd->port_conf->fifosize;
> +       else
> +               sdd->fifosize = FIFO_DEPTH(sdd);
> +
>         sdd->cur_bpw = 8;
>
>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
>                                         sdd->port_id, host->num_chipselect);
>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
> -                                       mem_res, FIFO_DEPTH(sdd));
> +                                       mem_res, sdd->fifosize);
>
>         pm_runtime_mark_last_busy(&pdev->dev);
>         pm_runtime_put_autosuspend(&pdev->dev);
> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
>                            s3c64xx_spi_runtime_resume, NULL)
>  };
>
> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> +       .fifosize       = 64,

I think if you rework the the .fifo_lvl_mask, replacing it with
.fifosize, you should also do next things in this series:
  1. Rework it for all supported (existing) chips in this driver
  2. Provide fifosize property for each SPI node for all existing dts
that use this driver
  3. Get rid of .fifo_lvl_mask for good. But the compatibility with
older kernels has to be taken into the account here as well.

Otherwise it looks like a half attempt and not finished, only creating
a duplicated property/struct field for the same (already existing)
thing. Because it's completely possible to do the same using just
.fifo_lvl_mask without introducing new fields or properties. If it
seems to much -- maybe just use .fifo_lvl_mask for now, and do all
that reworking properly later, in a separate patch series?

> +       .rx_lvl_offset  = 15,
> +       .tx_st_done     = 25,
> +       .clk_div        = 4,
> +       .high_speed     = true,
> +       .clk_from_cmu   = true,
> +       .has_loopback   = true,
> +       .quirks         = S3C64XX_SPI_QUIRK_CS_AUTO |
> +                         S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH,
> +};
> +
>  static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
>         .fifo_lvl_mask  = { 0x7f },
>         .rx_lvl_offset  = 13,
> @@ -1452,6 +1484,10 @@ static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
>  };
>
>  static const struct of_device_id s3c64xx_spi_dt_match[] = {
> +       {
> +               .compatible = "google,gs101-spi",
> +               .data = &gs101_spi_port_config,
> +       },
>         {
>                 .compatible = "samsung,s3c2443-spi",
>                 .data = &s3c2443_spi_port_config,
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Tudor Ambarus Jan. 24, 2024, 10:40 a.m. UTC | #2
Hi, Sam! Thanks for the review!

On 1/23/24 19:25, Sam Protsenko wrote:
> On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
>> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
>> register accesses, otherwise a Serror Interrupt is raised. Do the write
>> reg accesses in 32 bits.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
> 
> I counted 3 different features in this patch. Would be better to split
> it correspondingly into 3 patches, to make patches atomic:
> 
>   1. I/O width
>   2. FIFO size

I kept these 2 in the same patch as gs101 to exemplify their use by
gs101. But I'm also fine splitting the patch in 3, will do in v2.

>   3. Adding support for gs101
> 
> And I'm not really convinced about FIFO size change.

I'll explain why it's needed below.

> 
>>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
>>  1 file changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 62671b2d594a..c4ddd2859ba4 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -20,6 +20,7 @@
>>
>>  #define MAX_SPI_PORTS                          12
>>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
>> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
>>  #define AUTOSUSPEND_TIMEOUT                    2000
>>
>>  /* Registers and bit-fields */
>> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
>>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
>>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
>>   * @clk_div: Internal clock divider
>> + * @fifosize: size of the FIFO
>>   * @quirks: Bitmask of known quirks
>>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
>>   * @clk_from_cmu: True, if the controller does not include a clock mux and
>> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
>>         int     tx_st_done;
>>         int     quirks;
>>         int     clk_div;
>> +       unsigned int fifosize;
>>         bool    high_speed;
>>         bool    clk_from_cmu;
>>         bool    clk_ioclk;
>> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
>>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
>>   * @port_conf: Local SPI port configuartion data
>>   * @port_id: Port identification number
>> + * @fifosize: size of the FIFO for this port
>>   */
>>  struct s3c64xx_spi_driver_data {
>>         void __iomem                    *regs;
>> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
>>         struct s3c64xx_spi_dma_data     tx_dma;
>>         const struct s3c64xx_spi_port_config    *port_conf;
>>         unsigned int                    port_id;
>> +       unsigned int                    fifosize;
>>  };
>>
>>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
>>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>>
>>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
>> -               return xfer->len > FIFO_DEPTH(sdd);
>> +               return xfer->len > sdd->fifosize;
>>
>>         return false;
>>  }
>> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>                                         xfer->tx_buf, xfer->len / 4);
>>                                 break;
>>                         case 16:
>> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len / 2);
>> +                               if (sdd->port_conf->quirks &
>> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
>> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                        xfer->tx_buf, xfer->len / 2);
>> +                               else
>> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                     xfer->tx_buf, xfer->len / 2);
>>                                 break;
>>                         default:
>> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> -                                       xfer->tx_buf, xfer->len);
>> +                               if (sdd->port_conf->quirks &
>> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
>> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                       xfer->tx_buf, xfer->len);
>> +                               else
>> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
>> +                                                    xfer->tx_buf, xfer->len);
>>                                 break;
>>                         }
>>                 }
>> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
>>                                     struct spi_transfer *xfer)
>>  {
>>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
>> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
>> +       const unsigned int fifo_len = sdd->fifosize;
>>         const void *tx_buf = NULL;
>>         void *rx_buf = NULL;
>>         int target_len = 0, origin_len = 0;
>> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>>                 sdd->port_id = pdev->id;
>>         }
>>
>> +       if (sdd->port_conf->fifosize)
>> +               sdd->fifosize = sdd->port_conf->fifosize;
>> +       else
>> +               sdd->fifosize = FIFO_DEPTH(sdd);
>> +
>>         sdd->cur_bpw = 8;
>>
>>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
>> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
>>                                         sdd->port_id, host->num_chipselect);
>>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
>> -                                       mem_res, FIFO_DEPTH(sdd));
>> +                                       mem_res, sdd->fifosize);
>>
>>         pm_runtime_mark_last_busy(&pdev->dev);
>>         pm_runtime_put_autosuspend(&pdev->dev);
>> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
>>                            s3c64xx_spi_runtime_resume, NULL)
>>  };
>>
>> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
>> +       .fifosize       = 64,
> 
> I think if you rework the the .fifo_lvl_mask, replacing it with
> .fifosize, you should also do next things in this series:
>   1. Rework it for all supported (existing) chips in this driver
>   2. Provide fifosize property for each SPI node for all existing dts
> that use this driver
>   3. Get rid of .fifo_lvl_mask for good. But the compatibility with
> older kernels has to be taken into the account here as well.

We can't get rid of the .fifo_lvl_mask entirely because we need to be
backward compatible with the device tree files that we have now.

> 
> Otherwise it looks like a half attempt and not finished, only creating
> a duplicated property/struct field for the same (already existing)
> thing. Because it's completely possible to do the same using just
> .fifo_lvl_mask without introducing new fields or properties. If it

Using fifo_lvl_mask works but is wrong on multiple levels.
As the code is now, the device tree spi alias is used as an index in the
fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
have a dependency on an alias in a driver. Not specifying an alias will
make the probe fail, which is even worse. Also, the fifo_lvl_mask value
does not reflect the FIFO level reg field. This is incorrect as we use
partial register fields and is misleading. Other problem is that the
fifo_lvl_mask value is used to determine the FIFO depth which is also
incorrect. The FIFO depth is dictated by the SoC implementing the IP,
not by the FIFO_LVL register field. Having in mind these reasons I
marked the fifo_lvl_mask and the port_id as deprecated in the next
patch, we shouldn't use fifo_lvl_mask or the alias anymore.

In what concerns your first 2 points, to rework all the compatibles and
to introduce a fifosize property, I agree it would be nice to do it, but
it's not mandatory, we can work in an incremental fashion. Emphasizing
what is wrong, marking things as deprecated and guiding contributors on
how things should be handled is good too, which I tried in the next
patch. Anyway, I'll check what the reworking would involve, and if I
think it wouldn't take me a terrible amount of time, I'll do it.

> seems to much -- maybe just use .fifo_lvl_mask for now, and do all
> that reworking properly later, in a separate patch series?
> 

But that means to add gs101 and then to come with patches updating what
I just proposed, and I'm not thrilled about it.

Cheers,
ta
Sam Protsenko Jan. 24, 2024, 7:43 p.m. UTC | #3
On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi, Sam! Thanks for the review!
>
> On 1/23/24 19:25, Sam Protsenko wrote:
> > On Tue, Jan 23, 2024 at 9:34 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Add support for GS101 SPI. All the SPI nodes on GS101 have 64 bytes
> >> FIFOs, infer the FIFO size from the compatible. GS101 allows just 32bit
> >> register accesses, otherwise a Serror Interrupt is raised. Do the write
> >> reg accesses in 32 bits.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> >> ---
> >
> > I counted 3 different features in this patch. Would be better to split
> > it correspondingly into 3 patches, to make patches atomic:
> >
> >   1. I/O width
> >   2. FIFO size
>
> I kept these 2 in the same patch as gs101 to exemplify their use by
> gs101. But I'm also fine splitting the patch in 3, will do in v2.
>
> >   3. Adding support for gs101
> >
> > And I'm not really convinced about FIFO size change.
>
> I'll explain why it's needed below.
>
> >
> >>  drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++++++++++++++------
> >>  1 file changed, 43 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index 62671b2d594a..c4ddd2859ba4 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -20,6 +20,7 @@
> >>
> >>  #define MAX_SPI_PORTS                          12
> >>  #define S3C64XX_SPI_QUIRK_CS_AUTO              BIT(1)
> >> +#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH   BIT(2)
> >>  #define AUTOSUSPEND_TIMEOUT                    2000
> >>
> >>  /* Registers and bit-fields */
> >> @@ -131,6 +132,7 @@ struct s3c64xx_spi_dma_data {
> >>   * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
> >>   * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
> >>   * @clk_div: Internal clock divider
> >> + * @fifosize: size of the FIFO
> >>   * @quirks: Bitmask of known quirks
> >>   * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
> >>   * @clk_from_cmu: True, if the controller does not include a clock mux and
> >> @@ -149,6 +151,7 @@ struct s3c64xx_spi_port_config {
> >>         int     tx_st_done;
> >>         int     quirks;
> >>         int     clk_div;
> >> +       unsigned int fifosize;
> >>         bool    high_speed;
> >>         bool    clk_from_cmu;
> >>         bool    clk_ioclk;
> >> @@ -175,6 +178,7 @@ struct s3c64xx_spi_port_config {
> >>   * @tx_dma: Local transmit DMA data (e.g. chan and direction)
> >>   * @port_conf: Local SPI port configuartion data
> >>   * @port_id: Port identification number
> >> + * @fifosize: size of the FIFO for this port
> >>   */
> >>  struct s3c64xx_spi_driver_data {
> >>         void __iomem                    *regs;
> >> @@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
> >>         struct s3c64xx_spi_dma_data     tx_dma;
> >>         const struct s3c64xx_spi_port_config    *port_conf;
> >>         unsigned int                    port_id;
> >> +       unsigned int                    fifosize;
> >>  };
> >>
> >>  static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> >> @@ -403,7 +408,7 @@ static bool s3c64xx_spi_can_dma(struct spi_controller *host,
> >>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> >>
> >>         if (sdd->rx_dma.ch && sdd->tx_dma.ch)
> >> -               return xfer->len > FIFO_DEPTH(sdd);
> >> +               return xfer->len > sdd->fifosize;
> >>
> >>         return false;
> >>  }
> >> @@ -447,12 +452,22 @@ static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
> >>                                         xfer->tx_buf, xfer->len / 4);
> >>                                 break;
> >>                         case 16:
> >> -                               iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> >> -                                       xfer->tx_buf, xfer->len / 2);
> >> +                               if (sdd->port_conf->quirks &
> >> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> >> +                                       iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                        xfer->tx_buf, xfer->len / 2);
> >> +                               else
> >> +                                       iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                     xfer->tx_buf, xfer->len / 2);
> >>                                 break;
> >>                         default:
> >> -                               iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> >> -                                       xfer->tx_buf, xfer->len);
> >> +                               if (sdd->port_conf->quirks &
> >> +                                   S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
> >> +                                       iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                       xfer->tx_buf, xfer->len);
> >> +                               else
> >> +                                       iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
> >> +                                                    xfer->tx_buf, xfer->len);
> >>                                 break;
> >>                         }
> >>                 }
> >> @@ -696,7 +711,7 @@ static int s3c64xx_spi_transfer_one(struct spi_controller *host,
> >>                                     struct spi_transfer *xfer)
> >>  {
> >>         struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
> >> -       const unsigned int fifo_len = FIFO_DEPTH(sdd);
> >> +       const unsigned int fifo_len = sdd->fifosize;
> >>         const void *tx_buf = NULL;
> >>         void *rx_buf = NULL;
> >>         int target_len = 0, origin_len = 0;
> >> @@ -1145,6 +1160,11 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
> >>                 sdd->port_id = pdev->id;
> >>         }
> >>
> >> +       if (sdd->port_conf->fifosize)
> >> +               sdd->fifosize = sdd->port_conf->fifosize;
> >> +       else
> >> +               sdd->fifosize = FIFO_DEPTH(sdd);
> >> +
> >>         sdd->cur_bpw = 8;
> >>
> >>         sdd->tx_dma.direction = DMA_MEM_TO_DEV;
> >> @@ -1234,7 +1254,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
> >>         dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
> >>                                         sdd->port_id, host->num_chipselect);
> >>         dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
> >> -                                       mem_res, FIFO_DEPTH(sdd));
> >> +                                       mem_res, sdd->fifosize);
> >>
> >>         pm_runtime_mark_last_busy(&pdev->dev);
> >>         pm_runtime_put_autosuspend(&pdev->dev);
> >> @@ -1362,6 +1382,18 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
> >>                            s3c64xx_spi_runtime_resume, NULL)
> >>  };
> >>
> >> +static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
> >> +       .fifosize       = 64,
> >
> > I think if you rework the the .fifo_lvl_mask, replacing it with
> > .fifosize, you should also do next things in this series:
> >   1. Rework it for all supported (existing) chips in this driver
> >   2. Provide fifosize property for each SPI node for all existing dts
> > that use this driver
> >   3. Get rid of .fifo_lvl_mask for good. But the compatibility with
> > older kernels has to be taken into the account here as well.
>
> We can't get rid of the .fifo_lvl_mask entirely because we need to be
> backward compatible with the device tree files that we have now.
>
> >
> > Otherwise it looks like a half attempt and not finished, only creating
> > a duplicated property/struct field for the same (already existing)
> > thing. Because it's completely possible to do the same using just
> > .fifo_lvl_mask without introducing new fields or properties. If it
>
> Using fifo_lvl_mask works but is wrong on multiple levels.
> As the code is now, the device tree spi alias is used as an index in the
> fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
> have a dependency on an alias in a driver. Not specifying an alias will
> make the probe fail, which is even worse. Also, the fifo_lvl_mask value

Ok, I think that's a valid point. I probably missed the alias part
when reading the patch description. I also understand we can't just
remove .fifo_lvl_mask right now, as we have to keep the compatibility
with older/existing out-of-tree device trees, so that the user can
update the kernel image separately.

> does not reflect the FIFO level reg field. This is incorrect as we use
> partial register fields and is misleading. Other problem is that the
> fifo_lvl_mask value is used to determine the FIFO depth which is also
> incorrect. The FIFO depth is dictated by the SoC implementing the IP,
> not by the FIFO_LVL register field. Having in mind these reasons I
> marked the fifo_lvl_mask and the port_id as deprecated in the next
> patch, we shouldn't use fifo_lvl_mask or the alias anymore.
>
> In what concerns your first 2 points, to rework all the compatibles and
> to introduce a fifosize property, I agree it would be nice to do it, but
> it's not mandatory, we can work in an incremental fashion. Emphasizing
> what is wrong, marking things as deprecated and guiding contributors on
> how things should be handled is good too, which I tried in the next
> patch. Anyway, I'll check what the reworking would involve, and if I
> think it wouldn't take me a terrible amount of time, I'll do it.
>

From what I understand, that shouldn't be very hard to do, just a
matter of adding fifosize property to all dts's existing upstream.
That would also provide a good example to follow for anyone who wants
to add the support for new compatibles. But of course I can't ask you
to do the extra work. My point is, with that item done, the first
transition step would be finished right away. And the remaining step
would be to have a strategy for .fifo_lvl_mask removal. I wonder what
maintainers can suggest on that matter, and if it's doable at all.

Btw, just a thought: maybe also add "deprecated" comment to each line
of code where .fifo_lvl_mask is being assigned, just to make sure
noone follows that style in the future (as people often tend to
copy-paste existing implementation)? Because obviously we can't remove
those lines for now.

> > seems to much -- maybe just use .fifo_lvl_mask for now, and do all
> > that reworking properly later, in a separate patch series?
> >
>
> But that means to add gs101 and then to come with patches updating what
> I just proposed, and I'm not thrilled about it.
>

Got it. That's fine with me. I think we don't have to have everything
super-granular w.r.t. patch series split. But I'd still argue that
splitting this particular patch by 3 patches would make things more
atomic and thus better.

> Cheers,
> ta
Mark Brown Jan. 25, 2024, 1:32 p.m. UTC | #4
On Wed, Jan 24, 2024 at 01:43:55PM -0600, Sam Protsenko wrote:
> On Wed, Jan 24, 2024 at 4:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

> > Using fifo_lvl_mask works but is wrong on multiple levels.
> > As the code is now, the device tree spi alias is used as an index in the
> > fifo_lvl_mask to determine the FIFO depth. I find it unacceptable to
> > have a dependency on an alias in a driver. Not specifying an alias will
> > make the probe fail, which is even worse. Also, the fifo_lvl_mask value

> Ok, I think that's a valid point. I probably missed the alias part
> when reading the patch description. I also understand we can't just
> remove .fifo_lvl_mask right now, as we have to keep the compatibility
> with older/existing out-of-tree device trees, so that the user can
> update the kernel image separately.

I don't really agree here, for a given compatible the FIFO depth is
known so it's redundant to specify and it's much simpler to correct
issues if we're not overspecifying things in the DT.
diff mbox series

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 62671b2d594a..c4ddd2859ba4 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -20,6 +20,7 @@ 
 
 #define MAX_SPI_PORTS				12
 #define S3C64XX_SPI_QUIRK_CS_AUTO		BIT(1)
+#define S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH	BIT(2)
 #define AUTOSUSPEND_TIMEOUT			2000
 
 /* Registers and bit-fields */
@@ -131,6 +132,7 @@  struct s3c64xx_spi_dma_data {
  * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
  * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
  * @clk_div: Internal clock divider
+ * @fifosize: size of the FIFO
  * @quirks: Bitmask of known quirks
  * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
  * @clk_from_cmu: True, if the controller does not include a clock mux and
@@ -149,6 +151,7 @@  struct s3c64xx_spi_port_config {
 	int	tx_st_done;
 	int	quirks;
 	int	clk_div;
+	unsigned int fifosize;
 	bool	high_speed;
 	bool	clk_from_cmu;
 	bool	clk_ioclk;
@@ -175,6 +178,7 @@  struct s3c64xx_spi_port_config {
  * @tx_dma: Local transmit DMA data (e.g. chan and direction)
  * @port_conf: Local SPI port configuartion data
  * @port_id: Port identification number
+ * @fifosize: size of the FIFO for this port
  */
 struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
@@ -194,6 +198,7 @@  struct s3c64xx_spi_driver_data {
 	struct s3c64xx_spi_dma_data	tx_dma;
 	const struct s3c64xx_spi_port_config	*port_conf;
 	unsigned int			port_id;
+	unsigned int			fifosize;
 };
 
 static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
@@ -403,7 +408,7 @@  static bool s3c64xx_spi_can_dma(struct spi_controller *host,
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
 
 	if (sdd->rx_dma.ch && sdd->tx_dma.ch)
-		return xfer->len > FIFO_DEPTH(sdd);
+		return xfer->len > sdd->fifosize;
 
 	return false;
 }
@@ -447,12 +452,22 @@  static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 					xfer->tx_buf, xfer->len / 4);
 				break;
 			case 16:
-				iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len / 2);
+				if (sdd->port_conf->quirks &
+				    S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
+					iowrite16_32_rep(regs + S3C64XX_SPI_TX_DATA,
+							 xfer->tx_buf, xfer->len / 2);
+				else
+					iowrite16_rep(regs + S3C64XX_SPI_TX_DATA,
+						      xfer->tx_buf, xfer->len / 2);
 				break;
 			default:
-				iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
-					xfer->tx_buf, xfer->len);
+				if (sdd->port_conf->quirks &
+				    S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH)
+					iowrite8_32_rep(regs + S3C64XX_SPI_TX_DATA,
+							xfer->tx_buf, xfer->len);
+				else
+					iowrite8_rep(regs + S3C64XX_SPI_TX_DATA,
+						     xfer->tx_buf, xfer->len);
 				break;
 			}
 		}
@@ -696,7 +711,7 @@  static int s3c64xx_spi_transfer_one(struct spi_controller *host,
 				    struct spi_transfer *xfer)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_controller_get_devdata(host);
-	const unsigned int fifo_len = FIFO_DEPTH(sdd);
+	const unsigned int fifo_len = sdd->fifosize;
 	const void *tx_buf = NULL;
 	void *rx_buf = NULL;
 	int target_len = 0, origin_len = 0;
@@ -1145,6 +1160,11 @@  static int s3c64xx_spi_probe(struct platform_device *pdev)
 		sdd->port_id = pdev->id;
 	}
 
+	if (sdd->port_conf->fifosize)
+		sdd->fifosize = sdd->port_conf->fifosize;
+	else
+		sdd->fifosize = FIFO_DEPTH(sdd);
+
 	sdd->cur_bpw = 8;
 
 	sdd->tx_dma.direction = DMA_MEM_TO_DEV;
@@ -1234,7 +1254,7 @@  static int s3c64xx_spi_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Targets attached\n",
 					sdd->port_id, host->num_chipselect);
 	dev_dbg(&pdev->dev, "\tIOmem=[%pR]\tFIFO %dbytes\n",
-					mem_res, FIFO_DEPTH(sdd));
+					mem_res, sdd->fifosize);
 
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
@@ -1362,6 +1382,18 @@  static const struct dev_pm_ops s3c64xx_spi_pm = {
 			   s3c64xx_spi_runtime_resume, NULL)
 };
 
+static const struct s3c64xx_spi_port_config gs101_spi_port_config = {
+	.fifosize	= 64,
+	.rx_lvl_offset  = 15,
+	.tx_st_done     = 25,
+	.clk_div        = 4,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.has_loopback	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO |
+			  S3C64XX_SPI_GS1O1_32BIT_REG_IO_WIDTH,
+};
+
 static const struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
 	.fifo_lvl_mask	= { 0x7f },
 	.rx_lvl_offset	= 13,
@@ -1452,6 +1484,10 @@  static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 };
 
 static const struct of_device_id s3c64xx_spi_dt_match[] = {
+	{
+		.compatible = "google,gs101-spi",
+		.data = &gs101_spi_port_config,
+	},
 	{
 		.compatible = "samsung,s3c2443-spi",
 		.data = &s3c2443_spi_port_config,