| Message ID | 1461587709-147048-3-git-send-email-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 25-04-16, 15:35, Andy Shevchenko wrote: > There are several changes are done here: > > - Convert the property to be in bytes > > Besides this is common practice for such property the use of a value in bytes > much more convenient than handling the encoded value. > > - Rename data_width to data-width in the device tree bindings > > - While here, replace dwc_fast_ffs() by __ffs() > > The change leaves the support for old format as well just in case someone will > use a newer kernel with an old device tree blob. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > Documentation/devicetree/bindings/dma/snps-dma.txt | 6 ++-- > arch/arc/boot/dts/abilis_tb10x.dtsi | 2 +- > arch/arm/boot/dts/spear13xx.dtsi | 4 +-- > drivers/dma/dw/core.c | 42 ++++++---------------- > drivers/dma/dw/platform.c | 5 ++- > include/linux/platform_data/dma-dw.h | 2 +- > 6 files changed, 21 insertions(+), 40 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt > index c99c1ff..544b9b9 100644 > --- a/Documentation/devicetree/bindings/dma/snps-dma.txt > +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt > @@ -13,8 +13,8 @@ Required properties: > - chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1: > increase from chan n->0 > - block_size: Maximum block size supported by the controller > -- data_width: Maximum data width supported by hardware per AHB master > - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) > +- data-width: Maximum data width supported by hardware per AHB master > + (in bytes, power of 2) > > > Optional properties: > @@ -38,7 +38,7 @@ Example: > chan_allocation_order = <1>; > chan_priority = <1>; > block_size = <0xfff>; > - data_width = <3 3>; > + data-width = <8 8>; > }; You broke backward compatibility with earlier DTs. What's backward compatibility ? Consider that the DT from an earlier version of kernel is part of the bootrom of a SoC. Now that bootrom should work just fine with any new kernel version. i.e. old DT + new kernels should always work.
On Wed, Apr 27, 2016 at 12:13:17PM +0530, Viresh Kumar wrote: > On 25-04-16, 15:35, Andy Shevchenko wrote: > > There are several changes are done here: > > > > - Convert the property to be in bytes > > > > Besides this is common practice for such property the use of a value in bytes > > much more convenient than handling the encoded value. > > > > - Rename data_width to data-width in the device tree bindings > > > > - While here, replace dwc_fast_ffs() by __ffs() > > > > The change leaves the support for old format as well just in case someone will > > use a newer kernel with an old device tree blob. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > Documentation/devicetree/bindings/dma/snps-dma.txt | 6 ++-- > > arch/arc/boot/dts/abilis_tb10x.dtsi | 2 +- > > arch/arm/boot/dts/spear13xx.dtsi | 4 +-- > > drivers/dma/dw/core.c | 42 ++++++---------------- > > drivers/dma/dw/platform.c | 5 ++- > > include/linux/platform_data/dma-dw.h | 2 +- > > 6 files changed, 21 insertions(+), 40 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt > > index c99c1ff..544b9b9 100644 > > --- a/Documentation/devicetree/bindings/dma/snps-dma.txt > > +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt > > @@ -13,8 +13,8 @@ Required properties: > > - chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1: > > increase from chan n->0 > > - block_size: Maximum block size supported by the controller > > -- data_width: Maximum data width supported by hardware per AHB master > > - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) > > +- data-width: Maximum data width supported by hardware per AHB master > > + (in bytes, power of 2) > > > > > > Optional properties: > > @@ -38,7 +38,7 @@ Example: > > chan_allocation_order = <1>; > > chan_priority = <1>; > > block_size = <0xfff>; > > - data_width = <3 3>; > > + data-width = <8 8>; > > }; > > You broke backward compatibility with earlier DTs. > > What's backward compatibility ? > > Consider that the DT from an earlier version of kernel is part of the bootrom of > a SoC. Now that bootrom should work just fine with any new kernel version. i.e. > old DT + new kernels should always work. And this is not fixed yet!.
On Wed, Apr 27, 2016 at 9:43 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 25-04-16, 15:35, Andy Shevchenko wrote: >> There are several changes are done here: >> >> - Convert the property to be in bytes >> >> Besides this is common practice for such property the use of a value in bytes >> much more convenient than handling the encoded value. >> >> - Rename data_width to data-width in the device tree bindings >> >> - While here, replace dwc_fast_ffs() by __ffs() >> >> The change leaves the support for old format as well just in case someone will >> use a newer kernel with an old device tree blob. >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> Documentation/devicetree/bindings/dma/snps-dma.txt | 6 ++-- >> arch/arc/boot/dts/abilis_tb10x.dtsi | 2 +- >> arch/arm/boot/dts/spear13xx.dtsi | 4 +-- >> drivers/dma/dw/core.c | 42 ++++++---------------- >> drivers/dma/dw/platform.c | 5 ++- >> include/linux/platform_data/dma-dw.h | 2 +- >> 6 files changed, 21 insertions(+), 40 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt >> index c99c1ff..544b9b9 100644 >> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt >> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt >> @@ -13,8 +13,8 @@ Required properties: >> - chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1: >> increase from chan n->0 >> - block_size: Maximum block size supported by the controller >> -- data_width: Maximum data width supported by hardware per AHB master >> - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) >> +- data-width: Maximum data width supported by hardware per AHB master >> + (in bytes, power of 2) >> >> >> Optional properties: >> @@ -38,7 +38,7 @@ Example: >> chan_allocation_order = <1>; >> chan_priority = <1>; >> block_size = <0xfff>; >> - data_width = <3 3>; >> + data-width = <8 8>; >> }; > > You broke backward compatibility with earlier DTs. How? > > What's backward compatibility ? > > Consider that the DT from an earlier version of kernel is part of the bootrom of > a SoC. Now that bootrom should work just fine with any new kernel version. i.e. > old DT + new kernels should always work. Yes, the property name is slightly different as meaning. If we find data-width property driver will use it, otherwise it takes old name and converts variable to be in bytes in the driver.
On 27-04-16, 11:25, Andy Shevchenko wrote: > On Wed, Apr 27, 2016 at 9:43 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 25-04-16, 15:35, Andy Shevchenko wrote: > >> There are several changes are done here: > >> > >> - Convert the property to be in bytes > >> > >> Besides this is common practice for such property the use of a value in bytes > >> much more convenient than handling the encoded value. > >> > >> - Rename data_width to data-width in the device tree bindings > >> > >> - While here, replace dwc_fast_ffs() by __ffs() > >> > >> The change leaves the support for old format as well just in case someone will > >> use a newer kernel with an old device tree blob. > >> > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> --- > >> Documentation/devicetree/bindings/dma/snps-dma.txt | 6 ++-- > >> arch/arc/boot/dts/abilis_tb10x.dtsi | 2 +- > >> arch/arm/boot/dts/spear13xx.dtsi | 4 +-- > >> drivers/dma/dw/core.c | 42 ++++++---------------- > >> drivers/dma/dw/platform.c | 5 ++- > >> include/linux/platform_data/dma-dw.h | 2 +- > >> 6 files changed, 21 insertions(+), 40 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt > >> index c99c1ff..544b9b9 100644 > >> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt > >> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt > >> @@ -13,8 +13,8 @@ Required properties: > >> - chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1: > >> increase from chan n->0 > >> - block_size: Maximum block size supported by the controller > >> -- data_width: Maximum data width supported by hardware per AHB master > >> - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) > >> +- data-width: Maximum data width supported by hardware per AHB master > >> + (in bytes, power of 2) > >> > >> > >> Optional properties: > >> @@ -38,7 +38,7 @@ Example: > >> chan_allocation_order = <1>; > >> chan_priority = <1>; > >> block_size = <0xfff>; > >> - data_width = <3 3>; > >> + data-width = <8 8>; > >> }; > > > > You broke backward compatibility with earlier DTs. > > How? > > > > > What's backward compatibility ? > > > > Consider that the DT from an earlier version of kernel is part of the bootrom of > > a SoC. Now that bootrom should work just fine with any new kernel version. i.e. > > old DT + new kernels should always work. > > Yes, the property name is slightly different as meaning. > > If we find data-width property driver will use it, otherwise it takes > old name and converts variable to be in bytes in the driver. Oh, I missed that you renamed the property and taking care of both the properties now. Sorry about that. So, you didn't break them for sure :) But, the DT documentation doesn't contain the old property now but the code does. I think you are required to keep the bindings currently supported by the kernel in there. You can mark them deprecated, but can't remove them.
On Wed, 2016-04-27 at 14:00 +0530, Viresh Kumar wrote: > On 27-04-16, 11:25, Andy Shevchenko wrote: > > > > -- data_width: Maximum data width supported by hardware per AHB > > > > master > > > > - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) > > > > +- data-width: Maximum data width supported by hardware per AHB > > > > master > > > > + (in bytes, power of 2) > > But, the DT documentation doesn't contain the old property now but the > code > does. I think you are required to keep the bindings currently > supported by the > kernel in there. You can mark them deprecated, but can't remove them. This point I take, indeed. Will update series soon. Thanks for review.
diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt index c99c1ff..544b9b9 100644 --- a/Documentation/devicetree/bindings/dma/snps-dma.txt +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt @@ -13,8 +13,8 @@ Required properties: - chan_priority: priority of channels. 0 (default): increase from chan 0->n, 1: increase from chan n->0 - block_size: Maximum block size supported by the controller -- data_width: Maximum data width supported by hardware per AHB master - (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) +- data-width: Maximum data width supported by hardware per AHB master + (in bytes, power of 2) Optional properties: @@ -38,7 +38,7 @@ Example: chan_allocation_order = <1>; chan_priority = <1>; block_size = <0xfff>; - data_width = <3 3>; + data-width = <8 8>; }; DMA clients connected to the Designware DMA controller must use the format diff --git a/arch/arc/boot/dts/abilis_tb10x.dtsi b/arch/arc/boot/dts/abilis_tb10x.dtsi index 663671f2..de53f5c 100644 --- a/arch/arc/boot/dts/abilis_tb10x.dtsi +++ b/arch/arc/boot/dts/abilis_tb10x.dtsi @@ -126,7 +126,7 @@ chan_allocation_order = <0>; chan_priority = <1>; block_size = <0x7ff>; - data_width = <2>; + data-width = <4>; clocks = <&ahb_clk>; clock-names = "hclk"; }; diff --git a/arch/arm/boot/dts/spear13xx.dtsi b/arch/arm/boot/dts/spear13xx.dtsi index 14594ce..449acf0 100644 --- a/arch/arm/boot/dts/spear13xx.dtsi +++ b/arch/arm/boot/dts/spear13xx.dtsi @@ -117,7 +117,7 @@ chan_priority = <1>; block_size = <0xfff>; dma-masters = <2>; - data_width = <3 3>; + data-width = <8 8>; }; dma@eb000000 { @@ -133,7 +133,7 @@ chan_allocation_order = <1>; chan_priority = <1>; block_size = <0xfff>; - data_width = <3 3>; + data-width = <8 8>; }; fsmc: flash@b0000000 { diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 78522dc..992da25 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -162,21 +162,6 @@ static void dwc_initialize(struct dw_dma_chan *dwc) /*----------------------------------------------------------------------*/ -static inline unsigned int dwc_fast_ffs(unsigned long long v) -{ - /* - * We can be a lot more clever here, but this should take care - * of the most common optimization. - */ - if (!(v & 7)) - return 3; - else if (!(v & 3)) - return 2; - else if (!(v & 1)) - return 1; - return 0; -} - static inline void dwc_dump_chan_regs(struct dw_dma_chan *dwc) { dev_err(chan2dev(&dwc->chan), @@ -677,11 +662,12 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, struct dw_desc *prev; size_t xfer_count; size_t offset; + u8 m_master = dwc->m_master; unsigned int src_width; unsigned int dst_width; - unsigned int data_width; + unsigned int data_width = dw->data_width[m_master]; u32 ctllo; - u8 lms = DWC_LLP_LMS(dwc->m_master); + u8 lms = DWC_LLP_LMS(m_master); dev_vdbg(chan2dev(chan), "%s: d%pad s%pad l0x%zx f0x%lx\n", __func__, @@ -694,10 +680,7 @@ dwc_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, dwc->direction = DMA_MEM_TO_MEM; - data_width = dw->data_width[dwc->m_master]; - - src_width = dst_width = min_t(unsigned int, data_width, - dwc_fast_ffs(src | dest | len)); + src_width = dst_width = __ffs(data_width | src | dest | len); ctllo = DWC_DEFAULT_CTLLO(chan) | DWC_CTLL_DST_WIDTH(dst_width) @@ -757,11 +740,12 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, struct dw_desc *prev; struct dw_desc *first; u32 ctllo; - u8 lms = DWC_LLP_LMS(dwc->m_master); + u8 m_master = dwc->m_master; + u8 lms = DWC_LLP_LMS(m_master); dma_addr_t reg; unsigned int reg_width; unsigned int mem_width; - unsigned int data_width; + unsigned int data_width = dw->data_width[m_master]; unsigned int i; struct scatterlist *sg; size_t total_len = 0; @@ -787,8 +771,6 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_M2P) : DWC_CTLL_FC(DW_DMA_FC_D_M2P); - data_width = dw->data_width[dwc->m_master]; - for_each_sg(sgl, sg, sg_len, i) { struct dw_desc *desc; u32 len, dlen, mem; @@ -796,8 +778,7 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, mem = sg_dma_address(sg); len = sg_dma_len(sg); - mem_width = min_t(unsigned int, - data_width, dwc_fast_ffs(mem | len)); + mem_width = __ffs(data_width | mem | len); slave_sg_todev_fill_desc: desc = dwc_desc_get(dwc); @@ -843,8 +824,6 @@ slave_sg_todev_fill_desc: ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) : DWC_CTLL_FC(DW_DMA_FC_D_P2M); - data_width = dw->data_width[dwc->m_master]; - for_each_sg(sgl, sg, sg_len, i) { struct dw_desc *desc; u32 len, dlen, mem; @@ -852,8 +831,7 @@ slave_sg_todev_fill_desc: mem = sg_dma_address(sg); len = sg_dma_len(sg); - mem_width = min_t(unsigned int, - data_width, dwc_fast_ffs(mem | len)); + mem_width = __ffs(data_width | mem | len); slave_sg_fromdev_fill_desc: desc = dwc_desc_get(dwc); @@ -1500,7 +1478,7 @@ int dw_dma_probe(struct dw_dma_chip *chip, struct dw_dma_platform_data *pdata) pdata->nr_masters = (dw_params >> DW_PARAMS_NR_MASTER & 3) + 1; for (i = 0; i < pdata->nr_masters; i++) { pdata->data_width[i] = - (dw_params >> DW_PARAMS_DATA_WIDTH(i) & 3) + 2; + 4 << (dw_params >> DW_PARAMS_DATA_WIDTH(i) & 3); } max_blk_size = dma_readl(dw, MAX_BLK_SIZE); diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c index e65ebe5..2420fb7 100644 --- a/drivers/dma/dw/platform.c +++ b/drivers/dma/dw/platform.c @@ -138,9 +138,12 @@ dw_dma_parse_dt(struct platform_device *pdev) if (!of_property_read_u32(np, "block_size", &tmp)) pdata->block_size = tmp; - if (!of_property_read_u32_array(np, "data_width", arr, nr_masters)) { + if (!of_property_read_u32_array(np, "data-width", arr, nr_masters)) { for (tmp = 0; tmp < nr_masters; tmp++) pdata->data_width[tmp] = arr[tmp]; + } else if (!of_property_read_u32_array(np, "data_width", arr, nr_masters)) { + for (tmp = 0; tmp < nr_masters; tmp++) + pdata->data_width[tmp] = BIT(arr[tmp] & 0x07); } return pdata; diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h index b881b97..ad76811 100644 --- a/include/linux/platform_data/dma-dw.h +++ b/include/linux/platform_data/dma-dw.h @@ -43,7 +43,7 @@ struct dw_dma_slave { * @block_size: Maximum block size supported by the controller * @nr_masters: Number of AHB masters supported by the controller * @data_width: Maximum data width supported by hardware per AHB master - * (0 - 8bits, 1 - 16bits, ..., 5 - 256bits) + * (in bytes, power of 2) */ struct dw_dma_platform_data { unsigned int nr_channels;
There are several changes are done here: - Convert the property to be in bytes Besides this is common practice for such property the use of a value in bytes much more convenient than handling the encoded value. - Rename data_width to data-width in the device tree bindings - While here, replace dwc_fast_ffs() by __ffs() The change leaves the support for old format as well just in case someone will use a newer kernel with an old device tree blob. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- Documentation/devicetree/bindings/dma/snps-dma.txt | 6 ++-- arch/arc/boot/dts/abilis_tb10x.dtsi | 2 +- arch/arm/boot/dts/spear13xx.dtsi | 4 +-- drivers/dma/dw/core.c | 42 ++++++---------------- drivers/dma/dw/platform.c | 5 ++- include/linux/platform_data/dma-dw.h | 2 +- 6 files changed, 21 insertions(+), 40 deletions(-)