| Message ID | 20170917144539.3497-1-stefan.bruens@rwth-aachen.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, Sep 17, 2017 at 04:45:39PM +0200, Stefan Brüns wrote: > Obviously, the current value for the burst widths are wrong, and if this > value is retrieved from some other subsystem using dma_get_slave_caps, > it will wrongly assume burst width of e.g. 3 bytes are supported. > > Each bit in the bitmask corresponds to a supported width, but it uses > an encoding of BIT(<widths>), not BIT(log2<widths>), as it must be able > to encode a width of 3 bytes. > > The corollary is, it is not possible to encode either a width of 32 or > 64 bytes, as the field has a size of 32 bits, and only a subset of the > controller capabilities can be exposed. well the right way would have been to fix this in the core by extending the src/dst_addr_widths. I am sending a patch for that. please use that and update this > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > drivers/dma/sh/rcar-dmac.c | 14 ++++++++++---- > drivers/dma/sh/shdmac.c | 13 +++++++++---- > drivers/dma/sh/usb-dmac.c | 9 ++++++--- > 3 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c > index 2b2c7db3e480..768076caccfd 100644 > --- a/drivers/dma/sh/rcar-dmac.c > +++ b/drivers/dma/sh/rcar-dmac.c > @@ -1734,10 +1734,16 @@ static int rcar_dmac_parse_of(struct device *dev, struct rcar_dmac *dmac) > > static int rcar_dmac_probe(struct platform_device *pdev) > { > - const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | > - DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES | > - DMA_SLAVE_BUSWIDTH_8_BYTES | DMA_SLAVE_BUSWIDTH_16_BYTES | > - DMA_SLAVE_BUSWIDTH_32_BYTES | DMA_SLAVE_BUSWIDTH_64_BYTES; > + /* > + * FIXME: Controller supports DMA_SLAVE_BUSWIDTH_32_BYTES > + * and DMA_SLAVE_BUSWIDTH_64_BYTES, > + * but this overflows the u32 src/dst_addr_widths fields > + */ > + const u32 widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_16_BYTES); > unsigned int channels_offset = 0; > struct dma_device *engine; > struct rcar_dmac *dmac; > diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c > index c94ffab0d25c..bf35b5588b11 100644 > --- a/drivers/dma/sh/shdmac.c > +++ b/drivers/dma/sh/shdmac.c > @@ -679,10 +679,15 @@ MODULE_DEVICE_TABLE(of, sh_dmae_of_match); > > static int sh_dmae_probe(struct platform_device *pdev) > { > - const enum dma_slave_buswidth widths = > - DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES | > - DMA_SLAVE_BUSWIDTH_4_BYTES | DMA_SLAVE_BUSWIDTH_8_BYTES | > - DMA_SLAVE_BUSWIDTH_16_BYTES | DMA_SLAVE_BUSWIDTH_32_BYTES; > + /* > + * FIXME: Controller supports DMA_SLAVE_BUSWIDTH_32_BYTES > + * but this overflows the u32 src/dst_addr_widths fields > + */ > + const u32 widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | > + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) | > + BIT(DMA_SLAVE_BUSWIDTH_16_BYTES); > const struct sh_dmae_pdata *pdata; > unsigned long chan_flag[SH_DMAE_MAX_CHANNELS] = {}; > int chan_irq[SH_DMAE_MAX_CHANNELS]; > diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c > index 31a145154e9f..0ac7e842b70c 100644 > --- a/drivers/dma/sh/usb-dmac.c > +++ b/drivers/dma/sh/usb-dmac.c > @@ -768,7 +768,6 @@ static int usb_dmac_parse_of(struct device *dev, struct usb_dmac *dmac) > > static int usb_dmac_probe(struct platform_device *pdev) > { > - const enum dma_slave_buswidth widths = USB_DMAC_SLAVE_BUSWIDTH; > struct dma_device *engine; > struct usb_dmac *dmac; > struct resource *mem; > @@ -837,8 +836,12 @@ static int usb_dmac_probe(struct platform_device *pdev) > > engine->dev = &pdev->dev; > > - engine->src_addr_widths = widths; > - engine->dst_addr_widths = widths; > + /* > + * FIXME: The controller supports a width of USB_DMAC_SLAVE_BUSWIDTH, > + * i.e. 32 bytes, but BIT(32) overflows the u32 bitmask fields. > + */ > + engine->src_addr_widths = 0; > + engine->dst_addr_widths = 0; > engine->directions = BIT(DMA_MEM_TO_DEV) | BIT(DMA_DEV_TO_MEM); > engine->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST; > > -- > 2.14.1 > > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Donnerstag, 12. Oktober 2017 16:18:12 CEST Vinod Koul wrote: > On Sun, Sep 17, 2017 at 04:45:39PM +0200, Stefan Brüns wrote: > > Obviously, the current value for the burst widths are wrong, and if this > > value is retrieved from some other subsystem using dma_get_slave_caps, > > it will wrongly assume burst width of e.g. 3 bytes are supported. > > > > Each bit in the bitmask corresponds to a supported width, but it uses > > an encoding of BIT(<widths>), not BIT(log2<widths>), as it must be able > > to encode a width of 3 bytes. > > > > The corollary is, it is not possible to encode either a width of 32 or > > 64 bytes, as the field has a size of 32 bits, and only a subset of the > > controller capabilities can be exposed. > > well the right way would have been to fix this in the core by extending the > src/dst_addr_widths. I am sending a patch for that. please use that and > update this Thats the reason I sent this as RFC, I never expected this to go mainline as is, but wanted to trigger a discussion. Kind regards, Stefan -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 2b2c7db3e480..768076caccfd 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1734,10 +1734,16 @@ static int rcar_dmac_parse_of(struct device *dev, struct rcar_dmac *dmac) static int rcar_dmac_probe(struct platform_device *pdev) { - const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | - DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES | - DMA_SLAVE_BUSWIDTH_8_BYTES | DMA_SLAVE_BUSWIDTH_16_BYTES | - DMA_SLAVE_BUSWIDTH_32_BYTES | DMA_SLAVE_BUSWIDTH_64_BYTES; + /* + * FIXME: Controller supports DMA_SLAVE_BUSWIDTH_32_BYTES + * and DMA_SLAVE_BUSWIDTH_64_BYTES, + * but this overflows the u32 src/dst_addr_widths fields + */ + const u32 widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_16_BYTES); unsigned int channels_offset = 0; struct dma_device *engine; struct rcar_dmac *dmac; diff --git a/drivers/dma/sh/shdmac.c b/drivers/dma/sh/shdmac.c index c94ffab0d25c..bf35b5588b11 100644 --- a/drivers/dma/sh/shdmac.c +++ b/drivers/dma/sh/shdmac.c @@ -679,10 +679,15 @@ MODULE_DEVICE_TABLE(of, sh_dmae_of_match); static int sh_dmae_probe(struct platform_device *pdev) { - const enum dma_slave_buswidth widths = - DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES | - DMA_SLAVE_BUSWIDTH_4_BYTES | DMA_SLAVE_BUSWIDTH_8_BYTES | - DMA_SLAVE_BUSWIDTH_16_BYTES | DMA_SLAVE_BUSWIDTH_32_BYTES; + /* + * FIXME: Controller supports DMA_SLAVE_BUSWIDTH_32_BYTES + * but this overflows the u32 src/dst_addr_widths fields + */ + const u32 widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_8_BYTES) | + BIT(DMA_SLAVE_BUSWIDTH_16_BYTES); const struct sh_dmae_pdata *pdata; unsigned long chan_flag[SH_DMAE_MAX_CHANNELS] = {}; int chan_irq[SH_DMAE_MAX_CHANNELS]; diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c index 31a145154e9f..0ac7e842b70c 100644 --- a/drivers/dma/sh/usb-dmac.c +++ b/drivers/dma/sh/usb-dmac.c @@ -768,7 +768,6 @@ static int usb_dmac_parse_of(struct device *dev, struct usb_dmac *dmac) static int usb_dmac_probe(struct platform_device *pdev) { - const enum dma_slave_buswidth widths = USB_DMAC_SLAVE_BUSWIDTH; struct dma_device *engine; struct usb_dmac *dmac; struct resource *mem; @@ -837,8 +836,12 @@ static int usb_dmac_probe(struct platform_device *pdev) engine->dev = &pdev->dev; - engine->src_addr_widths = widths; - engine->dst_addr_widths = widths; + /* + * FIXME: The controller supports a width of USB_DMAC_SLAVE_BUSWIDTH, + * i.e. 32 bytes, but BIT(32) overflows the u32 bitmask fields. + */ + engine->src_addr_widths = 0; + engine->dst_addr_widths = 0; engine->directions = BIT(DMA_MEM_TO_DEV) | BIT(DMA_DEV_TO_MEM); engine->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
Obviously, the current value for the burst widths are wrong, and if this value is retrieved from some other subsystem using dma_get_slave_caps, it will wrongly assume burst width of e.g. 3 bytes are supported. Each bit in the bitmask corresponds to a supported width, but it uses an encoding of BIT(<widths>), not BIT(log2<widths>), as it must be able to encode a width of 3 bytes. The corollary is, it is not possible to encode either a width of 32 or 64 bytes, as the field has a size of 32 bits, and only a subset of the controller capabilities can be exposed. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/dma/sh/rcar-dmac.c | 14 ++++++++++---- drivers/dma/sh/shdmac.c | 13 +++++++++---- drivers/dma/sh/usb-dmac.c | 9 ++++++--- 3 files changed, 25 insertions(+), 11 deletions(-)