Re: [PATCH 02/10] spi: dw: Add support for RX sample delay register

[Lars Povlsen <lars.povlsen@microchip.com>]

Serge Semin writes:

> On Wed, May 13, 2020 at 04:00:23PM +0200, Lars Povlsen wrote:
>> This add support for the RX_SAMPLE_DLY register. If enabled in the
>> Designware IP, it allows tuning of the rx data signal by means of an
>> internal rx sample fifo.
>>
>> The register is located at offset 0xf0, and if the option is not
>> enabled in the IP, changing the register will have no effect.
>>
>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> ---
>>  drivers/spi/spi-dw.c | 7 +++++++
>>  drivers/spi/spi-dw.h | 2 ++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
>> index e572eb34a3c1a..32997f28fa5bb 100644
>> --- a/drivers/spi/spi-dw.c
>> +++ b/drivers/spi/spi-dw.c
>> @@ -81,6 +81,9 @@ static ssize_t dw_spi_show_regs(struct file *file, char __user *user_buf,
>>                       "DMATDLR: \t0x%08x\n", dw_readl(dws, DW_SPI_DMATDLR));
>>       len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
>>                       "DMARDLR: \t0x%08x\n", dw_readl(dws, DW_SPI_DMARDLR));
>
>> +     len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
>> +                      "RX_SAMPLE_DLY: \t0x%08x\n",
>> +                      dw_readl(dws, DW_SPI_RX_SAMPLE_DLY));
>
> debugfs_reg32 interface is now utilized in the driver to dump the registers
> state. So this will have to be converted to just a new entry in the
> dw_spi_dbgfs_regs array.
>

Ok, I'll have a look at this.

>>       len += scnprintf(buf + len, SPI_REGS_BUFSIZE - len,
>>                       "=================================\n");
>>
>> @@ -315,6 +318,10 @@ static int dw_spi_transfer_one(struct spi_controller *master,
>>               spi_set_clk(dws, chip->clk_div);
>>       }
>>
>
>> +     /* Apply RX sample delay, iff requested (nonzero) */
>
> s/iff/if
>
>> +     if (dws->rx_sample_dly)
>> +             dw_writel(dws, DW_SPI_RX_SAMPLE_DLY, dws->rx_sample_dly);
>> +
>>       dws->n_bytes = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
>>       dws->dma_width = DIV_ROUND_UP(transfer->bits_per_word, BITS_PER_BYTE);
>>
>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
>> index 1bf5713e047d3..ed6e47b3f50da 100644
>> --- a/drivers/spi/spi-dw.h
>> +++ b/drivers/spi/spi-dw.h
>> @@ -31,6 +31,7 @@
>>  #define DW_SPI_IDR                   0x58
>>  #define DW_SPI_VERSION                       0x5c
>>  #define DW_SPI_DR                    0x60
>> +#define DW_SPI_RX_SAMPLE_DLY         0xf0
>>  #define DW_SPI_CS_OVERRIDE           0xf4
>>
>>  /* Bit fields in CTRLR0 */
>> @@ -111,6 +112,7 @@ struct dw_spi {
>>
>>       int                     cs_override;
>>       u32                     reg_io_width;   /* DR I/O width in bytes */
>
>> +     u8                      rx_sample_dly;  /* RX fifo tuning (option) */
>
> This doesn't seem like a good place for this parameter. The sample delay is
> SPI-slave specific. So as I see it, the parameter should be moved to the
> chip_data.
>

Yes, I got that in other comments, and you are right I guess.

I will apply that approach of having rx_sample_dly per SPI slave.

---Lars

> -Sergey
>
>>       u16                     bus_num;
>>       u16                     num_cs;         /* supported slave numbers */
>>       void (*set_cs)(struct spi_device *spi, bool enable);
>> --
>> 2.26.2
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Lars Povlsen,
Microchip