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

Lars Povlsen
2020-06-09T10:04:44Z

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