| Message ID | 20230321115843.2688472-1-joychakr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] spi: dw: Add 32 bpw support to DW DMA Controller | expand |
On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote: > If DW Controller is capable of a maximum of 32 bits per word then SW or > DMA controller has to write up to 32bit or 4byte data to the FIFO at a > time. > > This Patch adds support for AxSize = 4 bytes configuration from dw dma * sorry for referring to the newbie-doc, but please note: https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77 and https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94 > driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4. > It also checks to see if the dma controller is capable of satisfying the > width requirement to achieve a particular bits/word requirement per > transfer. > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > --- > drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++----- > drivers/spi/spi-dw.h | 1 + > 2 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > index ababb910b391..9ac3a1c25e2d 100644 > --- a/drivers/spi/spi-dw-dma.c > +++ b/drivers/spi/spi-dw-dma.c > @@ -23,6 +23,8 @@ > #define DW_SPI_TX_BUSY 1 > #define DW_SPI_TX_BURST_LEVEL 16 > > +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes); > + > static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param) > { > struct dw_dma_slave *s = param; > @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws) > dws->dma_sg_burst = 0; > } > > +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws) > +{ > + struct dma_slave_caps tx = {0}, rx = {0}; > + > + dma_get_slave_caps(dws->txchan, &tx); > + dma_get_slave_caps(dws->rxchan, &rx); Even though in this case any dma_get_slave_caps() failure will effectively disable the DMA-based transfers, in general it would be useful to have the dma_get_slave_caps() return value checked and halt further DMA-init in case if it's not zero. In addition to that if the Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and MEM2DEV direction specified the DMA device won't be suitable for SPI-ing. So further DMA-initialization are pointless in that case too. It's just a general note not obligating or requesting anything since the respective update should have been done in a separate patch anyway. > + > + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths; Hm, in general the addr-width capabilities can mismatch. But it's very much unlikely since both DMA channels normally belong to the same controller. So I guess we can live with the suggested approach for now but please add a comment above that line about the assumption/limitation it implies. > +} > + > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > { > struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx; > @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > dw_spi_dma_sg_burst_init(dws); > > + dw_spi_dma_addr_widths_init(dws); > + > pci_dev_put(dma_dev); > > return 0; > @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) > > dw_spi_dma_sg_burst_init(dws); > > + dw_spi_dma_addr_widths_init(dws); > + > return 0; > > free_rxchan: > @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master, > struct spi_device *spi, struct spi_transfer *xfer) > { > struct dw_spi *dws = spi_controller_get_devdata(master); > + enum dma_slave_buswidth dma_bus_width; > > - return xfer->len > dws->fifo_len; > + if (xfer->len > dws->fifo_len) { > + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes); > + if (dws->dma_addr_widths & BIT(dma_bus_width)) > + return true; > + } < newline would have been nice, but... > + return false; on the other hand a level of indentation could be decreased like this: + enum dma_slave_buswidth width; + + if (xfer->len <= dws->fifo_len) + return false; + + width = dw_spi_dma_convert_width(dws->n_bytes); + + return !!(dws->dma_addr_widths & BIT(width)); -Serge(y) > } > > static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes) > { > - if (n_bytes == 1) > + switch (n_bytes) { > + case 1: > return DMA_SLAVE_BUSWIDTH_1_BYTE; > - else if (n_bytes == 2) > + case 2: > return DMA_SLAVE_BUSWIDTH_2_BYTES; > - > - return DMA_SLAVE_BUSWIDTH_UNDEFINED; > + case 3: > + case 4: > + return DMA_SLAVE_BUSWIDTH_4_BYTES; > + default: > + return DMA_SLAVE_BUSWIDTH_UNDEFINED; > + } > } > > static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed) > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h > index 9e8eb2b52d5c..3962e6dcf880 100644 > --- a/drivers/spi/spi-dw.h > +++ b/drivers/spi/spi-dw.h > @@ -190,6 +190,7 @@ struct dw_spi { > struct dma_chan *rxchan; > u32 rxburst; > u32 dma_sg_burst; > + u32 dma_addr_widths; > unsigned long dma_chan_busy; > dma_addr_t dma_addr; /* phy address of the Data register */ > const struct dw_spi_dma_ops *dma_ops; > -- > 2.40.0.rc1.284.g88254d51c5-goog >
Hi Serge(y), On Wed, Mar 22, 2023 at 12:57 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote: > > If DW Controller is capable of a maximum of 32 bits per word then SW or > > DMA controller has to write up to 32bit or 4byte data to the FIFO at a > > time. > > > > > This Patch adds support for AxSize = 4 bytes configuration from dw dma > > * sorry for referring to the newbie-doc, but please note: > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77 > and > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94 > Thank you for the point, I will rephrase the commit text. > > driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4. > > It also checks to see if the dma controller is capable of satisfying the > > width requirement to achieve a particular bits/word requirement per > > transfer. > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > --- > > drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++----- > > drivers/spi/spi-dw.h | 1 + > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > > index ababb910b391..9ac3a1c25e2d 100644 > > --- a/drivers/spi/spi-dw-dma.c > > +++ b/drivers/spi/spi-dw-dma.c > > @@ -23,6 +23,8 @@ > > #define DW_SPI_TX_BUSY 1 > > #define DW_SPI_TX_BURST_LEVEL 16 > > > > +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes); > > + > > static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param) > > { > > struct dw_dma_slave *s = param; > > @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws) > > dws->dma_sg_burst = 0; > > } > > > > +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws) > > +{ > > + struct dma_slave_caps tx = {0}, rx = {0}; > > + > > > + dma_get_slave_caps(dws->txchan, &tx); > > + dma_get_slave_caps(dws->rxchan, &rx); > > Even though in this case any dma_get_slave_caps() failure will > effectively disable the DMA-based transfers, in general it would be > useful to have the dma_get_slave_caps() return value checked and halt > further DMA-init in case if it's not zero. In addition to that if the > Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and > MEM2DEV direction specified the DMA device won't be suitable for > SPI-ing. So further DMA-initialization are pointless in that case too. > It's just a general note not obligating or requesting anything since > the respective update should have been done in a separate patch > anyway. > I shall add the checks suggested and put 'dw_spi_dma_addr_widths_init' and 'dw_spi_dma_sg_burst_init' in one function. I'll break this up into 2 patches in V3. > > + > > > + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths; > > Hm, in general the addr-width capabilities can mismatch. But it's very > much unlikely since both DMA channels normally belong to the same > controller. So I guess we can live with the suggested approach for now > but please add a comment above that line about the > assumption/limitation it implies. > Even if the address width capabilities mismatch since in dma mode only full duplex is done, hence the subset of the capabilities which apply to both tx and rx should be applicable. I shall put the same as a comment > > +} > > + > > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > { > > struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx; > > @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > > > dw_spi_dma_sg_burst_init(dws); > > > > + dw_spi_dma_addr_widths_init(dws); > > + > > pci_dev_put(dma_dev); > > > > return 0; > > @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) > > > > dw_spi_dma_sg_burst_init(dws); > > > > + dw_spi_dma_addr_widths_init(dws); > > + > > return 0; > > > > free_rxchan: > > @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master, > > struct spi_device *spi, struct spi_transfer *xfer) > > { > > struct dw_spi *dws = spi_controller_get_devdata(master); > > > + enum dma_slave_buswidth dma_bus_width; > > > > - return xfer->len > dws->fifo_len; > > + if (xfer->len > dws->fifo_len) { > > + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes); > > + if (dws->dma_addr_widths & BIT(dma_bus_width)) > > + return true; > > + } > < newline would have been nice, but... > > + return false; > > on the other hand a level of indentation could be decreased like this: > > + enum dma_slave_buswidth width; > + > + if (xfer->len <= dws->fifo_len) > + return false; > + > + width = dw_spi_dma_convert_width(dws->n_bytes); > + > + return !!(dws->dma_addr_widths & BIT(width)); > Sure, I will restructure this but any reason to use '!!' here ? > -Serge(y) > > > } > > > > static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes) > > { > > - if (n_bytes == 1) > > + switch (n_bytes) { > > + case 1: > > return DMA_SLAVE_BUSWIDTH_1_BYTE; > > - else if (n_bytes == 2) > > + case 2: > > return DMA_SLAVE_BUSWIDTH_2_BYTES; > > - > > - return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > + case 3: > > + case 4: > > + return DMA_SLAVE_BUSWIDTH_4_BYTES; > > + default: > > + return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > + } > > } > > > > static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed) > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h > > index 9e8eb2b52d5c..3962e6dcf880 100644 > > --- a/drivers/spi/spi-dw.h > > +++ b/drivers/spi/spi-dw.h > > @@ -190,6 +190,7 @@ struct dw_spi { > > struct dma_chan *rxchan; > > u32 rxburst; > > u32 dma_sg_burst; > > + u32 dma_addr_widths; > > unsigned long dma_chan_busy; > > dma_addr_t dma_addr; /* phy address of the Data register */ > > const struct dw_spi_dma_ops *dma_ops; > > -- > > 2.40.0.rc1.284.g88254d51c5-goog > > I shall upload a V3 based on these comments. Thanks Joy
On Thu, Mar 23, 2023 at 05:32:09PM +0530, Joy Chakraborty wrote: > Hi Serge(y), > > On Wed, Mar 22, 2023 at 12:57 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote: > > > If DW Controller is capable of a maximum of 32 bits per word then SW or > > > DMA controller has to write up to 32bit or 4byte data to the FIFO at a > > > time. > > > > > > > > This Patch adds support for AxSize = 4 bytes configuration from dw dma > > > > * sorry for referring to the newbie-doc, but please note: > > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77 > > and > > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94 > > > > Thank you for the point, I will rephrase the commit text. > > > > driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4. > > > It also checks to see if the dma controller is capable of satisfying the > > > width requirement to achieve a particular bits/word requirement per > > > transfer. > > > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > > --- > > > drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++----- > > > drivers/spi/spi-dw.h | 1 + > > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > > > index ababb910b391..9ac3a1c25e2d 100644 > > > --- a/drivers/spi/spi-dw-dma.c > > > +++ b/drivers/spi/spi-dw-dma.c > > > @@ -23,6 +23,8 @@ > > > #define DW_SPI_TX_BUSY 1 > > > #define DW_SPI_TX_BURST_LEVEL 16 > > > > > > +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes); > > > + > > > static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param) > > > { > > > struct dw_dma_slave *s = param; > > > @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws) > > > dws->dma_sg_burst = 0; > > > } > > > > > > +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws) > > > +{ > > > + struct dma_slave_caps tx = {0}, rx = {0}; > > > + > > > > > + dma_get_slave_caps(dws->txchan, &tx); > > > + dma_get_slave_caps(dws->rxchan, &rx); > > > > Even though in this case any dma_get_slave_caps() failure will > > effectively disable the DMA-based transfers, in general it would be > > useful to have the dma_get_slave_caps() return value checked and halt > > further DMA-init in case if it's not zero. In addition to that if the > > Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and > > MEM2DEV direction specified the DMA device won't be suitable for > > SPI-ing. So further DMA-initialization are pointless in that case too. > > It's just a general note not obligating or requesting anything since > > the respective update should have been done in a separate patch > > anyway. > > > > I shall add the checks suggested and put 'dw_spi_dma_addr_widths_init' > and 'dw_spi_dma_sg_burst_init' in one function. > I'll break this up into 2 patches in V3. Sounds good. Thanks. > > > > + > > > > > + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths; > > > > Hm, in general the addr-width capabilities can mismatch. But it's very > > much unlikely since both DMA channels normally belong to the same > > controller. So I guess we can live with the suggested approach for now > > but please add a comment above that line about the > > assumption/limitation it implies. > > > > Even if the address width capabilities mismatch since in dma mode only > full duplex is done, hence the subset of the capabilities which apply > to both tx and rx should be applicable. > I shall put the same as a comment Actually half-duplex xfers are also possible. See what happens if rx_buf is Null or what the SPI_CONTROLLER_MUST_TX flag means (it's set if the dma_init callback is successfully executed). In the former case the Rx data will be just ignored, in the later case Tx-data will be read from a dummy Tx-buffer. In both cases it doesn't matter what bus-width is initialized in the DMA-controller. But in anyway as I said before it's not that a big deal to have the widths combined. > > > > +} > > > + > > > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > > { > > > struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx; > > > @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > > > > > dw_spi_dma_sg_burst_init(dws); > > > > > > + dw_spi_dma_addr_widths_init(dws); > > > + > > > pci_dev_put(dma_dev); > > > > > > return 0; > > > @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) > > > > > > dw_spi_dma_sg_burst_init(dws); > > > > > > + dw_spi_dma_addr_widths_init(dws); > > > + > > > return 0; > > > > > > free_rxchan: > > > @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master, > > > struct spi_device *spi, struct spi_transfer *xfer) > > > { > > > struct dw_spi *dws = spi_controller_get_devdata(master); > > > > > + enum dma_slave_buswidth dma_bus_width; > > > > > > - return xfer->len > dws->fifo_len; > > > + if (xfer->len > dws->fifo_len) { > > > + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes); > > > + if (dws->dma_addr_widths & BIT(dma_bus_width)) > > > + return true; > > > + } > > < newline would have been nice, but... > > > + return false; > > > > on the other hand a level of indentation could be decreased like this: > > > > + enum dma_slave_buswidth width; > > + > > + if (xfer->len <= dws->fifo_len) > > + return false; > > + > > + width = dw_spi_dma_convert_width(dws->n_bytes); > > + > > + return !!(dws->dma_addr_widths & BIT(width)); > > > > Sure, I will restructure this but > any reason to use '!!' here ? No. It can be omitted indeed. The resultant integer will be implicitly converted to one of the _Bool type values {true, false}. -Serge(y) > > > -Serge(y) > > > > > } > > > > > > static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes) > > > { > > > - if (n_bytes == 1) > > > + switch (n_bytes) { > > > + case 1: > > > return DMA_SLAVE_BUSWIDTH_1_BYTE; > > > - else if (n_bytes == 2) > > > + case 2: > > > return DMA_SLAVE_BUSWIDTH_2_BYTES; > > > - > > > - return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > > + case 3: > > > + case 4: > > > + return DMA_SLAVE_BUSWIDTH_4_BYTES; > > > + default: > > > + return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > > + } > > > } > > > > > > static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed) > > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h > > > index 9e8eb2b52d5c..3962e6dcf880 100644 > > > --- a/drivers/spi/spi-dw.h > > > +++ b/drivers/spi/spi-dw.h > > > @@ -190,6 +190,7 @@ struct dw_spi { > > > struct dma_chan *rxchan; > > > u32 rxburst; > > > u32 dma_sg_burst; > > > + u32 dma_addr_widths; > > > unsigned long dma_chan_busy; > > > dma_addr_t dma_addr; /* phy address of the Data register */ > > > const struct dw_spi_dma_ops *dma_ops; > > > -- > > > 2.40.0.rc1.284.g88254d51c5-goog > > > > > I shall upload a V3 based on these comments. > > Thanks > Joy
Hi Serge(y), On Thu, Mar 23, 2023 at 8:19 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > On Thu, Mar 23, 2023 at 05:32:09PM +0530, Joy Chakraborty wrote: > > Hi Serge(y), > > > > On Wed, Mar 22, 2023 at 12:57 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > > > On Tue, Mar 21, 2023 at 11:58:43AM +0000, Joy Chakraborty wrote: > > > > If DW Controller is capable of a maximum of 32 bits per word then SW or > > > > DMA controller has to write up to 32bit or 4byte data to the FIFO at a > > > > time. > > > > > > > > > > > This Patch adds support for AxSize = 4 bytes configuration from dw dma > > > > > > * sorry for referring to the newbie-doc, but please note: > > > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L77 > > > and > > > https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/process/submitting-patches.rst#L94 > > > > > > > Thank you for the point, I will rephrase the commit text. > > > > > > driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4. > > > > It also checks to see if the dma controller is capable of satisfying the > > > > width requirement to achieve a particular bits/word requirement per > > > > transfer. > > > > > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > > > --- > > > > drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++----- > > > > drivers/spi/spi-dw.h | 1 + > > > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > > > > index ababb910b391..9ac3a1c25e2d 100644 > > > > --- a/drivers/spi/spi-dw-dma.c > > > > +++ b/drivers/spi/spi-dw-dma.c > > > > @@ -23,6 +23,8 @@ > > > > #define DW_SPI_TX_BUSY 1 > > > > #define DW_SPI_TX_BURST_LEVEL 16 > > > > > > > > +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes); > > > > + > > > > static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param) > > > > { > > > > struct dw_dma_slave *s = param; > > > > @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws) > > > > dws->dma_sg_burst = 0; > > > > } > > > > > > > > +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws) > > > > +{ > > > > + struct dma_slave_caps tx = {0}, rx = {0}; > > > > + > > > > > > > + dma_get_slave_caps(dws->txchan, &tx); > > > > + dma_get_slave_caps(dws->rxchan, &rx); > > > > > > Even though in this case any dma_get_slave_caps() failure will > > > effectively disable the DMA-based transfers, in general it would be > > > useful to have the dma_get_slave_caps() return value checked and halt > > > further DMA-init in case if it's not zero. In addition to that if the > > > Tx/Rx DMA device doesn't have the DMA_SLAVE capability or DEV2MEM and > > > MEM2DEV direction specified the DMA device won't be suitable for > > > SPI-ing. So further DMA-initialization are pointless in that case too. > > > It's just a general note not obligating or requesting anything since > > > the respective update should have been done in a separate patch > > > anyway. > > > > > > > > I shall add the checks suggested and put 'dw_spi_dma_addr_widths_init' > > and 'dw_spi_dma_sg_burst_init' in one function. > > I'll break this up into 2 patches in V3. > > Sounds good. Thanks. > > > > > > > + > > > > > > > + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths; > > > > > > Hm, in general the addr-width capabilities can mismatch. But it's very > > > much unlikely since both DMA channels normally belong to the same > > > controller. So I guess we can live with the suggested approach for now > > > but please add a comment above that line about the > > > assumption/limitation it implies. > > > > > > > > Even if the address width capabilities mismatch since in dma mode only > > full duplex is done, hence the subset of the capabilities which apply > > to both tx and rx should be applicable. > > I shall put the same as a comment > > Actually half-duplex xfers are also possible. See what happens if > rx_buf is Null or what the SPI_CONTROLLER_MUST_TX flag means (it's set > if the dma_init callback is successfully executed). In the former case > the Rx data will be just ignored, in the later case Tx-data will be > read from a dummy Tx-buffer. In both cases it doesn't matter what > bus-width is initialized in the DMA-controller. But in anyway as I > said before it's not that a big deal to have the widths combined. > Got it, for TX only transfers we can have half duplex but for RX only transfers the framework will attach a dummy Tx buffer to make it full duplex. But i shall keep it combined for simplicity and keep a comment. > > > > > > +} > > > > + > > > > static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > > > { > > > > struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx; > > > > @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > > > > > > > dw_spi_dma_sg_burst_init(dws); > > > > > > > > + dw_spi_dma_addr_widths_init(dws); > > > > + > > > > pci_dev_put(dma_dev); > > > > > > > > return 0; > > > > @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) > > > > > > > > dw_spi_dma_sg_burst_init(dws); > > > > > > > > + dw_spi_dma_addr_widths_init(dws); > > > > + > > > > return 0; > > > > > > > > free_rxchan: > > > > @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master, > > > > struct spi_device *spi, struct spi_transfer *xfer) > > > > { > > > > struct dw_spi *dws = spi_controller_get_devdata(master); > > > > > > > + enum dma_slave_buswidth dma_bus_width; > > > > > > > > - return xfer->len > dws->fifo_len; > > > > + if (xfer->len > dws->fifo_len) { > > > > + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes); > > > > + if (dws->dma_addr_widths & BIT(dma_bus_width)) > > > > + return true; > > > > + } > > > < newline would have been nice, but... > > > > + return false; > > > > > > on the other hand a level of indentation could be decreased like this: > > > > > > + enum dma_slave_buswidth width; > > > + > > > + if (xfer->len <= dws->fifo_len) > > > + return false; > > > + > > > + width = dw_spi_dma_convert_width(dws->n_bytes); > > > + > > > + return !!(dws->dma_addr_widths & BIT(width)); > > > > > > > Sure, I will restructure this but > > > any reason to use '!!' here ? > > No. It can be omitted indeed. The resultant integer will be implicitly > converted to one of the _Bool type values {true, false}. > > -Serge(y) > > > > > > -Serge(y) > > > > > > > } > > > > > > > > static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes) > > > > { > > > > - if (n_bytes == 1) > > > > + switch (n_bytes) { > > > > + case 1: > > > > return DMA_SLAVE_BUSWIDTH_1_BYTE; > > > > - else if (n_bytes == 2) > > > > + case 2: > > > > return DMA_SLAVE_BUSWIDTH_2_BYTES; > > > > - > > > > - return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > > > + case 3: > > > > + case 4: > > > > + return DMA_SLAVE_BUSWIDTH_4_BYTES; > > > > + default: > > > > + return DMA_SLAVE_BUSWIDTH_UNDEFINED; > > > > + } > > > > } > > > > > > > > static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed) > > > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h > > > > index 9e8eb2b52d5c..3962e6dcf880 100644 > > > > --- a/drivers/spi/spi-dw.h > > > > +++ b/drivers/spi/spi-dw.h > > > > @@ -190,6 +190,7 @@ struct dw_spi { > > > > struct dma_chan *rxchan; > > > > u32 rxburst; > > > > u32 dma_sg_burst; > > > > + u32 dma_addr_widths; > > > > unsigned long dma_chan_busy; > > > > dma_addr_t dma_addr; /* phy address of the Data register */ > > > > const struct dw_spi_dma_ops *dma_ops; > > > > -- > > > > 2.40.0.rc1.284.g88254d51c5-goog > > > > > > > > I shall upload a V3 based on these comments. > > > > Thanks > > Joy Creating a V3 Patch with the updates. Thanks Joy
diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c index ababb910b391..9ac3a1c25e2d 100644 --- a/drivers/spi/spi-dw-dma.c +++ b/drivers/spi/spi-dw-dma.c @@ -23,6 +23,8 @@ #define DW_SPI_TX_BUSY 1 #define DW_SPI_TX_BURST_LEVEL 16 +static inline enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes); + static bool dw_spi_dma_chan_filter(struct dma_chan *chan, void *param) { struct dw_dma_slave *s = param; @@ -89,6 +91,16 @@ static void dw_spi_dma_sg_burst_init(struct dw_spi *dws) dws->dma_sg_burst = 0; } +static void dw_spi_dma_addr_widths_init(struct dw_spi *dws) +{ + struct dma_slave_caps tx = {0}, rx = {0}; + + dma_get_slave_caps(dws->txchan, &tx); + dma_get_slave_caps(dws->rxchan, &rx); + + dws->dma_addr_widths = tx.dst_addr_widths & rx.src_addr_widths; +} + static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) { struct dw_dma_slave dma_tx = { .dst_id = 1 }, *tx = &dma_tx; @@ -128,6 +140,8 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) dw_spi_dma_sg_burst_init(dws); + dw_spi_dma_addr_widths_init(dws); + pci_dev_put(dma_dev); return 0; @@ -167,6 +181,8 @@ static int dw_spi_dma_init_generic(struct device *dev, struct dw_spi *dws) dw_spi_dma_sg_burst_init(dws); + dw_spi_dma_addr_widths_init(dws); + return 0; free_rxchan: @@ -202,18 +218,29 @@ static bool dw_spi_can_dma(struct spi_controller *master, struct spi_device *spi, struct spi_transfer *xfer) { struct dw_spi *dws = spi_controller_get_devdata(master); + enum dma_slave_buswidth dma_bus_width; - return xfer->len > dws->fifo_len; + if (xfer->len > dws->fifo_len) { + dma_bus_width = dw_spi_dma_convert_width(dws->n_bytes); + if (dws->dma_addr_widths & BIT(dma_bus_width)) + return true; + } + return false; } static enum dma_slave_buswidth dw_spi_dma_convert_width(u8 n_bytes) { - if (n_bytes == 1) + switch (n_bytes) { + case 1: return DMA_SLAVE_BUSWIDTH_1_BYTE; - else if (n_bytes == 2) + case 2: return DMA_SLAVE_BUSWIDTH_2_BYTES; - - return DMA_SLAVE_BUSWIDTH_UNDEFINED; + case 3: + case 4: + return DMA_SLAVE_BUSWIDTH_4_BYTES; + default: + return DMA_SLAVE_BUSWIDTH_UNDEFINED; + } } static int dw_spi_dma_wait(struct dw_spi *dws, unsigned int len, u32 speed) diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index 9e8eb2b52d5c..3962e6dcf880 100644 --- a/drivers/spi/spi-dw.h +++ b/drivers/spi/spi-dw.h @@ -190,6 +190,7 @@ struct dw_spi { struct dma_chan *rxchan; u32 rxburst; u32 dma_sg_burst; + u32 dma_addr_widths; unsigned long dma_chan_busy; dma_addr_t dma_addr; /* phy address of the Data register */ const struct dw_spi_dma_ops *dma_ops;
If DW Controller is capable of a maximum of 32 bits per word then SW or DMA controller has to write up to 32bit or 4byte data to the FIFO at a time. This Patch adds support for AxSize = 4 bytes configuration from dw dma driver if n_bytes i.e. number of bytes per write to fifo is 3 or 4. It also checks to see if the dma controller is capable of satisfying the width requirement to achieve a particular bits/word requirement per transfer. Signed-off-by: Joy Chakraborty <joychakr@google.com> --- drivers/spi/spi-dw-dma.c | 37 ++++++++++++++++++++++++++++++++----- drivers/spi/spi-dw.h | 1 + 2 files changed, 33 insertions(+), 5 deletions(-)