Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys DW AXI DMA bindings

[Alexey Brodkin]

Hi Vinod,

On Fri, 2018-03-02 at 13:44 +0530, Vinod Koul wrote:
> On Mon, Feb 26, 2018 at 05:56:28PM +0300, Eugeniy Paltsev wrote:
> > This patch adds documentation of device tree bindings for the Synopsys
> > DesignWare AXI DMA controller.
> > 
> > Signed-off-by: Eugeniy Paltsev 
> > ---
> >  .../devicetree/bindings/dma/snps,dw-axi-dmac.txt   | 41 
> > ++
> >  1 file changed, 41 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt 
> > b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > new file mode 100644
> > index 000..f237b79
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> > @@ -0,0 +1,41 @@
> > +Synopsys DesignWare AXI DMA Controller
> > +
> > +Required properties:
> > +- compatible: "snps,axi-dma-1.01a"
> > +- reg: Address range of the DMAC registers. This should include
> > +  all of the per-channel registers.
> > +- interrupt: Should contain the DMAC interrupt number.
> > +- interrupt-parent: Should be the phandle for the interrupt controller
> > +  that services interrupts for this device.
> > +- dma-channels: Number of channels supported by hardware.
> > +- snps,dma-masters: Number of AXI masters supported by the hardware.
> > +- snps,data-width: Maximum AXI data width supported by hardware.
> > +  (0 - 8bits, 1 - 16bits, 2 - 32bits, ..., 6 - 512bits)
> > +- snps,priority: Priority of channel. Array size is equal to the number of
> > +  dma-channels. Priority value must be programmed within [0:dma-channels-1]
> > +  range. (0 - minimum priority)
> > +- snps,block-size: Maximum block size supported by the controller channel.
> > +  Array size is equal to the number of dma-channels.
> > +
> > +Optional properties:
> > +- snps,axi-max-burst-len: Restrict master AXI burst length by value 
> > specified
> > +  in this property. If this property is missing the maximum AXI burst 
> > length
> > +  supported by DMAC is used. [1:256]
> > +
> > +Example:
> > +
> > +dmac: dma-controller@8 {
> > +   compatible = "snps,axi-dma-1.01a";
> 
> do we need "snps here..?

Synopsys is this IP-block vendor so shouldn't we put it that way?

-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 1/2] dmaengine: Introduce DW AXI DMAC driver

[Vinod Koul]

On Mon, Feb 26, 2018 at 05:56:27PM +0300, Eugeniy Paltsev wrote:

> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017-2018 Synopsys, Inc. (www.synopsys.com)
> + * Author: Eugeniy Paltsev 
> + *
> + * SPDX-License-Identifier:  GPL-2.0
> + */

This needs to be in below style

// SPDX-License-Identifier:  GPL-2.0
// Copyright (C) 

/*
 * other things u want to add
 */

> +#define DRV_NAME "axi_dw_dmac"

how about use KBUILD_MODNAME here?

> +static enum dma_status
> +dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
> +   struct dma_tx_state *txstate)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> + enum dma_status ret;
> +
> + ret = dma_cookie_status(dchan, cookie, txstate);
> +
> + if (chan->is_paused && ret == DMA_IN_PROGRESS)
> + return DMA_PAUSED;
> +
> + return ret;

nitpick, how about

if (chan->is_paused && ret == DMA_IN_PROGRESS)
ret = DMA_PAUSED;

return ret;

that way we have single point of return.

> +static int dma_chan_alloc_chan_resources(struct dma_chan *dchan)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> +
> + /* ASSERT: channel is idle */
> + if (axi_chan_is_hw_enable(chan)) {
> + dev_err(chan2dev(chan), "%s is non-idle!\n",
> + axi_chan_name(chan));
> + return -EBUSY;
> + }
> +
> + dev_vdbg(dchan2dev(dchan), "%s: allocating\n", axi_chan_name(chan));
> +
> + dma_cookie_init(dchan);

This is already done as part of vchan_init(), so why is it called explicitly
and why is that not invoked here?

> +static void dma_chan_free_chan_resources(struct dma_chan *dchan)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> +
> + /* ASSERT: channel is idle */
> + if (axi_chan_is_hw_enable(chan))
> + dev_err(dchan2dev(dchan), "%s is non-idle!\n",
> + axi_chan_name(chan));
> +
> + axi_chan_disable(chan);
> + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL);
> +
> + vchan_free_chan_resources(>vc);
> +
> + dev_vdbg(dchan2dev(dchan), "%s: free resources, descriptor still 
> allocated: %u\n",

making the string in subsequent line make it lesser than 80 chars, here and
few other places can benefit..

> +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id)
/s/intretupt/interrupt/ ?

> +static int axi_dma_resume(struct axi_dma_chip *chip)
> +{
> + int ret = 0;

superfluous init

> +static int parse_device_properties(struct axi_dma_chip *chip)
> +{
> + struct device *dev = chip->dev;
> + u32 tmp, carr[DMAC_MAX_CHANNELS];
> + int ret;
> +
> + ret = device_property_read_u32(dev, "dma-channels", );
> + if (ret)
> + return ret;
> + if (tmp == 0 || tmp > DMAC_MAX_CHANNELS)
> + return -EINVAL;
> +
> + chip->dw->hdata->nr_channels = tmp;
> +
> + ret = device_property_read_u32(dev, "snps,dma-masters", );

why is it snps,... shouldn't it be only dma-masters?

> +enum {
> + DWAXIDMAC_ARWLEN_1  = 0x0,
> + DWAXIDMAC_ARWLEN_2  = 0x1,
> + DWAXIDMAC_ARWLEN_4  = 0x3,
> + DWAXIDMAC_ARWLEN_8  = 0x7,
> + DWAXIDMAC_ARWLEN_16 = 0xF,
> + DWAXIDMAC_ARWLEN_32 = 0x1F,
> + DWAXIDMAC_ARWLEN_64 = 0x3F,
> + DWAXIDMAC_ARWLEN_128= 0x7F,
> + DWAXIDMAC_ARWLEN_256= 0xFF,

GENMASK() for these?

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 2/2] dt-bindings: Document the Synopsys DW AXI DMA bindings

[Vinod Koul]

On Mon, Feb 26, 2018 at 05:56:28PM +0300, Eugeniy Paltsev wrote:
> This patch adds documentation of device tree bindings for the Synopsys
> DesignWare AXI DMA controller.
> 
> Signed-off-by: Eugeniy Paltsev 
> ---
>  .../devicetree/bindings/dma/snps,dw-axi-dmac.txt   | 41 
> ++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt 
> b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> new file mode 100644
> index 000..f237b79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/snps,dw-axi-dmac.txt
> @@ -0,0 +1,41 @@
> +Synopsys DesignWare AXI DMA Controller
> +
> +Required properties:
> +- compatible: "snps,axi-dma-1.01a"
> +- reg: Address range of the DMAC registers. This should include
> +  all of the per-channel registers.
> +- interrupt: Should contain the DMAC interrupt number.
> +- interrupt-parent: Should be the phandle for the interrupt controller
> +  that services interrupts for this device.
> +- dma-channels: Number of channels supported by hardware.
> +- snps,dma-masters: Number of AXI masters supported by the hardware.
> +- snps,data-width: Maximum AXI data width supported by hardware.
> +  (0 - 8bits, 1 - 16bits, 2 - 32bits, ..., 6 - 512bits)
> +- snps,priority: Priority of channel. Array size is equal to the number of
> +  dma-channels. Priority value must be programmed within [0:dma-channels-1]
> +  range. (0 - minimum priority)
> +- snps,block-size: Maximum block size supported by the controller channel.
> +  Array size is equal to the number of dma-channels.
> +
> +Optional properties:
> +- snps,axi-max-burst-len: Restrict master AXI burst length by value specified
> +  in this property. If this property is missing the maximum AXI burst length
> +  supported by DMAC is used. [1:256]
> +
> +Example:
> +
> +dmac: dma-controller@8 {
> + compatible = "snps,axi-dma-1.01a";

do we need "snps here..?

> + reg = <0x8 0x400>;
> + clocks = <_clk>, <_clk>;
> + clock-names = "core-clk", "cfgr-clk";
> + interrupt-parent = <>;
> + interrupts = <27>;
> +
> + dma-channels = <4>;
> + snps,dma-masters = <2>;
> + snps,data-width = <3>;
> + snps,block-size = <4096 4096 4096 4096>;
> + snps,priority = <0 1 2 3>;
> + snps,axi-max-burst-len = <16>;
> +};
> -- 
> 2.9.3
> 

-- 
~Vinod

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v4] mmc: dw_mmc: Fix the DTO/CTO timeout overflow calculation for 32-bit systems

[Jaehoon Chung]

On 03/01/2018 02:57 PM, Jisheng Zhang wrote:
> On Wed, 28 Feb 2018 14:53:18 +0300 Evgeniy Didin wrote:
> 
>> In commit 9d9491a7da2a ("mmc: dw_mmc: Fix the DTO timeout calculation") and
>> commit 4c2357f57dd5 ("mmc: dw_mmc: Fix the CTO timeout calculation") 
>> have been made changes which cause multiply overflow for 32-bit systems.
>> The broken timeout calculations leads to unexpected ETIMEDOUT errors and 
>> causes stacktrace splat (such as below) during normal data exchange 
>> with SD-card.
>>
>> | Running :  4M-check-reassembly-tcp-cmykw2-rotatew2.out -v0 -w1
>> | -  Info: Finished target initialization.
>> | mmcblk0: error -110 transferring data, sector 320544, nr 2048, cmd response
>> |  0x900, card status 0x0
>>
>> DIV_ROUND_UP_ULL helps to escape usage of __udivdi3() from libgcc and so
>> code gets compiled on all 32-bit platforms as opposed to usage
>> of DIV_ROUND_UP when we may only compile stuff on a very few arches.
>>
>> Lets cast this multiply to u64 type which prevents overflow.
> 
> Reviewed-by: Jisheng Zhang 

Acked-by: Jaehoon Chung 

Best Regards,
Jaehoon Chung

> 
>>
>> Tested-by: Vineet Gupta 
>> Reported-by: Vineet Gupta  # ARC STAR 9001306872 
>> HSDK, sdio: board crashes when copying big files
>> Fixes: 9d9491a7da2a ("mmc: dw_mmc: Fix the DTO timeout calculation")
>> Fixes: 4c2357f57dd5 ("mmc: dw_mmc: Fix the CTO timeout calculation")
>>
>> Signed-off-by: Evgeniy Didin 
>>
>> CC: Alexey Brodkin 
>> CC: Eugeniy Paltsev 
>> CC: Douglas Anderson 
>> CC: Ulf Hansson 
>> CC: Andy Shevchenko 
>> CC: Jisheng Zhang 
>> CC: Shawn Lin 
>> CC: Vineet Gupta 
>> CC: linux-ker...@vger.kernel.org
>> CC: linux-snps-arc@lists.infradead.org
>> Cc:  
>> ---
>> Changes since v3:
>> -Switch DIV_ROUND_UP macro to DIV_ROUND_UP_ULL
>> -Make one patch from two patches
>> -Modify commit message
>>
>> Changes sinve v2:
>> -add fix for cto_ms
>>
>> Changes since v1:
>> -uint64_t switched to u64
>>
>>  drivers/mmc/host/dw_mmc.c | 9 ++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 0aa39975f33b..cba534d4c81b 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -409,7 +409,9 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
>>  cto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
>>  if (cto_div == 0)
>>  cto_div = 1;
>> -cto_ms = DIV_ROUND_UP(MSEC_PER_SEC * cto_clks * cto_div, host->bus_hz);
>> +
>> +cto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * cto_clks * cto_div,
>> +  host->bus_hz);
>>  
>>  /* add a bit spare time */
>>  cto_ms += 10;
>> @@ -1944,8 +1946,9 @@ static void dw_mci_set_drto(struct dw_mci *host)
>>  drto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
>>  if (drto_div == 0)
>>  drto_div = 1;
>> -drto_ms = DIV_ROUND_UP(MSEC_PER_SEC * drto_clks * drto_div,
>> -   host->bus_hz);
>> +
>> +drto_ms = DIV_ROUND_UP_ULL((u64)MSEC_PER_SEC * drto_clks * drto_div,
>> +   host->bus_hz);
>>  
>>  /* add a bit spare time */
>>  drto_ms += 10;
> 
> 
> 
> 


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc