| Message ID | 20220211034344.4130-2-jon.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive | expand |
On Fri, Feb 11, 2022 at 11:43:38AM +0800, Jon Lin wrote: > +static int rockchip_spi_setup(struct spi_device *spi) > +{ > + struct rockchip_spi *rs = spi_controller_get_devdata(spi->controller); > + u32 cr0; > + > + pm_runtime_get_sync(rs->dev); > + > + cr0 = readl_relaxed(rs->regs + ROCKCHIP_SPI_CTRLR0); > + > + cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET); > + if (spi->mode & SPI_CS_HIGH) > + cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET; What ensures that this read/modify/write doesn't race with a transfer running on another client device in the case where the controller has more than one device connected? Similarly with the mode, though it's not great to have devices with different modes connected to a single controller.
On Mon, Feb 14, 2022 at 04:40:19PM +0800, Jon Lin wrote: > 在 2022/2/11 19:24, Mark Brown 写道: > > > + cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET); > > > + if (spi->mode & SPI_CS_HIGH) > > > + cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET; > > What ensures that this read/modify/write doesn't race with a transfer > > running on another client device in the case where the controller has > > more than one device connected? Similarly with the mode, though it's > > not great to have devices with different modes connected to a single > > controller. > I have no idea how to deal with the conflict configuration between > different cs, and also I find nothing strategy in others spi drivers. > As we all know, some configurations should be consistent for different > CS devices, such as SPI_CPOL, so I suggest the framework to make > corresponding early warning prompts. As covered in the documentation setup() for one device may run while another is active, therefore if multiple devices are configured in the same register you should use a lock to ensure there can't be multiple writes. Note that the above appears to not just be setting the mode but also the chip select so if you've got two SPI_CS_HIGH devices then they'll both be going in and separately setting cr0.
On Tue, Feb 15, 2022 at 11:00:54AM +0800, Jon Lin wrote: > 在 2022/2/14 20:49, Mark Brown 写道: > > As covered in the documentation setup() for one device may run while > > another is active, therefore if multiple devices are configured in the > > same register you should use a lock to ensure there can't be multiple > > writes. Note that the above appears to not just be setting the mode but > > also the chip select so if you've got two SPI_CS_HIGH devices then > > they'll both be going in and separately setting cr0. > Is the io_mutex in function spi_setup is good enough? It's not supposed to be for that but looking at the code quickly I *think* setup() is never called with io_mutex held so it might well be fine - you should double check though. If not you'd need to add another lock in your driver data.
在 2022/2/15 20:36, Mark Brown 写道: > On Tue, Feb 15, 2022 at 11:00:54AM +0800, Jon Lin wrote: >> 在 2022/2/14 20:49, Mark Brown 写道: > >>> As covered in the documentation setup() for one device may run while >>> another is active, therefore if multiple devices are configured in the >>> same register you should use a lock to ensure there can't be multiple >>> writes. Note that the above appears to not just be setting the mode but >>> also the chip select so if you've got two SPI_CS_HIGH devices then >>> they'll both be going in and separately setting cr0. > >> Is the io_mutex in function spi_setup is good enough? > > It's not supposed to be for that but looking at the code quickly I > *think* setup() is never called with io_mutex held so it might well be > fine - you should double check though. If not you'd need to add another > lock in your driver data. 》setup() is never called with io_mutex held I think so. and I think the io_mutex is enough for me.
diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c index 7ac07569e103..1738a2611a2b 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -711,6 +711,26 @@ static bool rockchip_spi_can_dma(struct spi_controller *ctlr, return xfer->len / bytes_per_word >= rs->fifo_len; } +static int rockchip_spi_setup(struct spi_device *spi) +{ + struct rockchip_spi *rs = spi_controller_get_devdata(spi->controller); + u32 cr0; + + pm_runtime_get_sync(rs->dev); + + cr0 = readl_relaxed(rs->regs + ROCKCHIP_SPI_CTRLR0); + + cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET); + if (spi->mode & SPI_CS_HIGH) + cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET; + + writel_relaxed(cr0, rs->regs + ROCKCHIP_SPI_CTRLR0); + + pm_runtime_put(rs->dev); + + return 0; +} + static int rockchip_spi_probe(struct platform_device *pdev) { int ret; @@ -837,6 +857,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) ctlr->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX; ctlr->max_speed_hz = min(rs->freq / BAUDR_SCKDV_MIN, MAX_SCLK_OUT); + ctlr->setup = rockchip_spi_setup; ctlr->set_cs = rockchip_spi_set_cs; ctlr->transfer_one = rockchip_spi_transfer_one; ctlr->max_transfer_size = rockchip_spi_max_transfer_size;
After power up, the cs and clock is in default status, and the cs-high and clock polarity dts property configuration will take no effect until the calling of rockchip_spi_config in the first transmission. So preset them to make sure a correct voltage before the first transmission coming. Signed-off-by: Jon Lin <jon.lin@rock-chips.com> --- drivers/spi/spi-rockchip.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)