RE: [PATCH v2] dmaengine: vdma: Add 64 bit addressing support to the driver

2015-09-23 Thread Anurag Kumar Vulisha 
Hi Vinod,

Thanks for reviewing the patch

> -Original Message-
> From: Vinod Koul [mailto:vinod.k...@intel.com]
> Sent: Monday, September 21, 2015 9:27 PM
> To: Anurag Kumar Vulisha
> Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Michal
> Simek; Soren Brinkmann; Dan Williams; afaer...@suse.de; Maxime Ripard;
> Laurent Pinchart; Appana Durga Kedareswara Rao; Anirudha Sarangi; Srikanth
> Vemula; devicetree@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; dmaeng...@vger.kernel.org; Anurag Kumar
> Vulisha
> Subject: Re: [PATCH v2] dmaengine: vdma: Add 64 bit addressing support to
> the driver
>
> On Thu, Aug 27, 2015 at 09:19:18PM +0530, Anurag Kumar Vulisha wrote:
> > This VDMA  is a soft ip, which can be programmed to support
> > 32 bit addressing or greater than 32 bit addressing.
> >
> > When the VDMA ip is configured for 32 bit address space the transfer
> > start address is specified by a single register.
>
> would be good to specfiy which one
>

Will change  this in v3

> > When the  VDMA core is configured for an address space greater than 32
> > then each start address is specified by a combination of two
> > registers. The first register specifies the LSB 32 bits of address,
> > while the next register specifies the MSB 32 bits of address.For
> > example,5Ch will specify the LSB 32 bits while 60h will specify the
> > MSB 32 bits of the first start address.So we need to program two
> > registers at a time.
>
> can we have spaces after full stops and commas!
>

Will take care of this in v3 patch.

> > +/* Since vdma driver is trying to write to a register offset which is
> > +not a
> > + * multiple of 64 bits(ex : 0x5c), we are writing as two separate 32
> > +bits
> > + * instead of a single 64 bit register write.
> > + */
>
> This is not kernel style for multi-lines, pls refer to
> Documentation/CodingStyle
>

Will address this in v3 patch

> > +
> > +static inline void vdma_desc_write_64(struct xilinx_vdma_chan *chan,
> u32 reg,
> > +u32 value_lsb, u32 value_msb)
> > +{
> > +   /* Write the lsb 32 bits*/
> > +   writel(value_lsb, chan->xdev->regs + chan->desc_offset + reg);
> > +
> > +   /* Write the msb 32 bits */
> > +   writel(value_msb, chan->xdev->regs + chan->desc_offset + reg + 4);
>
> why not writeq

We are trying to write at a register address(ex:0x5c) which is not aligned on 8 
bytes  boundary.So if I try to use 64 bit write on it,unalignment  fault is 
getting  generated.To avoid that we are using two separate 32  bit writes.
We had this discussion in previous versions of this patch with Laurent Pinchart 
.I have also added this exaplanation in the comments above this function.

>
> > +   err = of_property_read_u32(node, "xlnx,addrwidth", &addr_width);
> > +
> > +   if (err < 0) {
> > +   /* Setting addr_width property to default 32 bits */
> > +   addr_width = 32;
> > +   }
>
> braces for a single line statement! Also space is redandant before if 
> condition
>

Will take care of this in v3 patch

Thanks,
Anurag Kumar V

> --
> ~Vinod


This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dmaengine: vdma: Add 64 bit addressing support to the driver

2015-09-21 Thread Vinod Koul 
On Thu, Aug 27, 2015 at 09:19:18PM +0530, Anurag Kumar Vulisha wrote:
> This VDMA  is a soft ip, which can be programmed to support
> 32 bit addressing or greater than 32 bit addressing.
> 
> When the VDMA ip is configured for 32 bit address space the
> transfer start address is specified by a single register.

would be good to specfiy which one

> When the  VDMA core is configured for an address space greater
> than 32 then each start address is specified by a combination
> of two registers. The first register specifies the LSB 32 bits
> of address, while the next register specifies the MSB 32 bits
> of address.For example,5Ch will specify the LSB 32 bits while
> 60h will specify the MSB 32 bits of the first start address.So
> we need to program two registers at a time.

can we have spaces after full stops and commas!

> +/* Since vdma driver is trying to write to a register offset which is not a
> + * multiple of 64 bits(ex : 0x5c), we are writing as two separate 32 bits
> + * instead of a single 64 bit register write.
> + */

This is not kernel style for multi-lines, pls refer to
Documentation/CodingStyle

> +
> +static inline void vdma_desc_write_64(struct xilinx_vdma_chan *chan, u32 reg,
> +  u32 value_lsb, u32 value_msb)
> +{
> + /* Write the lsb 32 bits*/
> + writel(value_lsb, chan->xdev->regs + chan->desc_offset + reg);
> +
> + /* Write the msb 32 bits */
> + writel(value_msb, chan->xdev->regs + chan->desc_offset + reg + 4);

why not writeq

> + err = of_property_read_u32(node, "xlnx,addrwidth", &addr_width);
> +
> + if (err < 0) {
> + /* Setting addr_width property to default 32 bits */
> + addr_width = 32;
> + }

braces for a single line statement! Also space is redandant before if
condition

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] dmaengine: vdma: Add 64 bit addressing support to the driver

2015-08-27 Thread Anurag Kumar Vulisha 
This VDMA  is a soft ip, which can be programmed to support
32 bit addressing or greater than 32 bit addressing.

When the VDMA ip is configured for 32 bit address space the
transfer start address is specified by a single register.

When the  VDMA core is configured for an address space greater
than 32 then each start address is specified by a combination
of two registers. The first register specifies the LSB 32 bits
of address, while the next register specifies the MSB 32 bits
of address.For example,5Ch will specify the LSB 32 bits while
60h will specify the MSB 32 bits of the first start address.So
we need to program two registers at a time.

This patch adds the 64 bit addressing support to the vdma driver.

Signed-off-by: Anurag Kumar Vulisha 
---
  Changes in v2:
  1. Added dma-ranges property in device tree as suggested by Arnd Bergmann.
  2. Added device tree property(xlnx,addrwidth) for an identification of whether
 the IP block itself is configured in 64-bit or 32-bit mode as suggested by
 Laurent Pinchart.
  3. Modified the driver code based on the xlnx,addrwidth.
---
 .../devicetree/bindings/dma/xilinx/xilinx_vdma.txt |4 +
 drivers/dma/Kconfig|2 +-
 drivers/dma/xilinx/xilinx_vdma.c   |   81 +---
 3 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
index e4c4d47..434d380 100644
--- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
+++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
@@ -8,6 +8,8 @@ Required properties:
 - #dma-cells: Should be <1>, see "dmas" property below
 - reg: Should contain VDMA registers location and length.
 - xlnx,num-fstores: Should be the number of framebuffers as configured in h/w.
+- xlnx,addrwidth: Should be the vdma addressing size in bits(ex: 32 bits).
+- dma-ranges: Should be as the following 
 - dma-channel child node: Should have at least one channel and can have up to
two channels per device. This node specifies the properties of each
DMA channel (see child node properties below).
@@ -41,8 +43,10 @@ axi_vdma_0: axivdma@4003 {
compatible = "xlnx,axi-vdma-1.00.a";
#dma_cells = <1>;
reg = < 0x4003 0x1 >;
+   dma-ranges = <0x 0x 0x4000>;
xlnx,num-fstores = <0x8>;
xlnx,flush-fsync = <0x1>;
+   xlnx,addrwidth = <0x20>;
dma-channel@4003 {
compatible = "xlnx,axi-vdma-mm2s-channel";
interrupts = < 0 54 4 >;
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index bda2cb0..a7cd0a8 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -398,7 +398,7 @@ config FSL_EDMA
 
 config XILINX_VDMA
tristate "Xilinx AXI VDMA Engine"
-   depends on (ARCH_ZYNQ || MICROBLAZE)
+   depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
select DMA_ENGINE
help
  Enable support for Xilinx AXI VDMA Soft IP.
diff --git a/drivers/dma/xilinx/xilinx_vdma.c b/drivers/dma/xilinx/xilinx_vdma.c
index d8434d4..d4eebc9 100644
--- a/drivers/dma/xilinx/xilinx_vdma.c
+++ b/drivers/dma/xilinx/xilinx_vdma.c
@@ -98,6 +98,8 @@
 #define XILINX_VDMA_FRMDLY_STRIDE_FRMDLY_SHIFT 24
 #define XILINX_VDMA_FRMDLY_STRIDE_STRIDE_SHIFT 0
 
+#define XILINX_VDMA_REG_START_ADDRESS_64(n)(0x000c + 8 * (n))
+
 #define XILINX_VDMA_REG_START_ADDRESS(n)   (0x000c + 4 * (n))
 
 /* HW specific definitions */
@@ -143,16 +145,18 @@
  * @next_desc: Next Descriptor Pointer @0x00
  * @pad1: Reserved @0x04
  * @buf_addr: Buffer address @0x08
- * @pad2: Reserved @0x0C
- * @vsize: Vertical Size @0x10
- * @hsize: Horizontal Size @0x14
+ * @buf_addr_msb: Buffer msb address @0x0c
+ * @pad2: Reserved @0x10
+ * @vsize: Vertical Size @0x14
+ * @hsize: Horizontal Size @0x18
  * @stride: Number of bytes between the first
- * pixels of each horizontal line @0x18
+ * pixels of each horizontal line @0x1C
  */
 struct xilinx_vdma_desc_hw {
u32 next_desc;
u32 pad1;
u32 buf_addr;
+   u32 buf_addr_msb;
u32 pad2;
u32 vsize;
u32 hsize;
@@ -206,6 +210,7 @@ struct xilinx_vdma_tx_descriptor {
  * @tasklet: Cleanup work after irq
  * @config: Device configuration info
  * @flush_on_fsync: Flush on Frame sync
+ * @ext_addr: Indicates 64 bit addressing is supported by dma channel
  */
 struct xilinx_vdma_chan {
struct xilinx_vdma_device *xdev;
@@ -229,6 +234,7 @@ struct xilinx_vdma_chan {
struct tasklet_struct tasklet;
struct xilinx_vdma_config config;
bool flush_on_fsync;
+   bool ext_addr;
 };
 
 /**
@@ -239,6 +245,7 @@ struct xilinx_vdma_chan {
  * @chan: Driver specific VDMA channel
  * @has_sg: Specifies whether Scatter-Gather is present or not
  * @flush_on_fsync: Flush on frame sync
+ * @ext_addr: Indicates 64 bit addressing