RE: [PATCH 2/6] net: can: xilinx_can: Fix flags field initialization for axi can and canps

[Appana Durga Kedareswara Rao]

Hi Marc,


> On 10/9/19 6:01 AM, Appana Durga Kedareswara Rao wrote:
> > Hi,
> >
> > 
> >> On 18.3.2019 13.32, Appana Durga Kedareswara rao wrote:
> >>> AXI CAN IP and CANPS IP supports tx fifo empty feature, this patch
> >>> updates the flags field for the same.
> >>>
> >>> Signed-off-by: Appana Durga Kedareswara rao
> >>> 
> >>> ---
> >>>  drivers/net/can/xilinx_can.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/net/can/xilinx_can.c
> >>> b/drivers/net/can/xilinx_can.c index 2de51ac..22569ef 100644
> >>> --- a/drivers/net/can/xilinx_can.c
> >>> +++ b/drivers/net/can/xilinx_can.c
> >>> @@ -1428,6 +1428,7 @@ static const struct dev_pm_ops
> xcan_dev_pm_ops
> >> =
> >>> {  };
> >>>
> >>>  static const struct xcan_devtype_data xcan_zynq_data = {
> >>> + .flags = XCAN_FLAG_TXFEMP,
> >>>   .bittiming_const = _bittiming_const,
> >>>   .btr_ts2_shift = XCAN_BTR_TS2_SHIFT,
> >>>   .btr_sjw_shift = XCAN_BTR_SJW_SHIFT,
> >>
> >> Thanks for catching this, this line seemed to have been incorrectly
> >> removed by my 9e5f1b273e ("can: xilinx_can: add support for Xilinx CAN FD
> core").
> >>
> >> But:
> >>
> >>> @@ -1435,6 +1436,7 @@ static const struct xcan_devtype_data
> >>> xcan_zynq_data = {  };
> >>>
> >>>  static const struct xcan_devtype_data xcan_axi_data = {
> >>> + .flags = XCAN_FLAG_TXFEMP,
> >>>   .bittiming_const = _bittiming_const,
> >>>   .btr_ts2_shift = XCAN_BTR_TS2_SHIFT,
> >>>   .btr_sjw_shift = XCAN_BTR_SJW_SHIFT,
> >>
> >>
> >> Are you sure this is right?
> >> In the documentation [1] there does not seem to be any TXFEMP
> >> interrupt, it would be interrupt bit 14 but AXI CAN 5.0 seems to only go up
> to 11.
> >>
> >> Or maybe it is undocumented or there is a newer version somewhere?
> >
> > Sorry for the delay in the reply.
> > Agree TXFEMP interrupt feature is not supported by the Soft IP CAN.
> > Since this patch already got applied will send a separate patch to fix this.
> 
> Please base your patch on net/master and add the appropriate fixes tag:
> 
> Fixes: 3281b380ec9f ("can: xilinx_can: Fix flags field initialization for axi 
> can
> and canps")

Sure Marc will send the patch on top of net/master. 

Regards,
Kedar.

> 
> Marc
> 
> --
> Pengutronix e.K.  | Marc Kleine-Budde   |
> Industrial Linux Solutions| Phone: +49-231-2826-924 |
> Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

[PATCH] net: can: xilinx_can: Fix flags field initialization for axi can

[Appana Durga Kedareswara rao]

AXI CANIP doesn't support tx fifo empty interrupt feature(TXFEMP),
update the flags filed in the driver for AXI CAN case accordingly.

Fixes: 3281b380ec9f ("can: xilinx_can: Fix flags field initialization for axi 
can and canps")
Reported-by: Anssi Hannula 
Signed-off-by: Appana Durga Kedareswara rao 
---
 drivers/net/can/xilinx_can.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 911b34316c9d..7c482b2d78d2 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1599,7 +1599,6 @@ static const struct xcan_devtype_data xcan_zynq_data = {
 
 static const struct xcan_devtype_data xcan_axi_data = {
.cantype = XAXI_CAN,
-   .flags = XCAN_FLAG_TXFEMP,
.bittiming_const = _bittiming_const,
.btr_ts2_shift = XCAN_BTR_TS2_SHIFT,
.btr_sjw_shift = XCAN_BTR_SJW_SHIFT,
-- 
2.7.4

RE: [PATCH 2/6] net: can: xilinx_can: Fix flags field initialization for axi can and canps

[Appana Durga Kedareswara Rao]

Hi,


> On 18.3.2019 13.32, Appana Durga Kedareswara rao wrote:
> > AXI CAN IP and CANPS IP supports tx fifo empty feature, this patch
> > updates the flags field for the same.
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> >  drivers/net/can/xilinx_can.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/can/xilinx_can.c
> > b/drivers/net/can/xilinx_can.c index 2de51ac..22569ef 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -1428,6 +1428,7 @@ static const struct dev_pm_ops xcan_dev_pm_ops
> =
> > {  };
> >
> >  static const struct xcan_devtype_data xcan_zynq_data = {
> > +   .flags = XCAN_FLAG_TXFEMP,
> > .bittiming_const = _bittiming_const,
> > .btr_ts2_shift = XCAN_BTR_TS2_SHIFT,
> > .btr_sjw_shift = XCAN_BTR_SJW_SHIFT,
> 
> Thanks for catching this, this line seemed to have been incorrectly removed by
> my 9e5f1b273e ("can: xilinx_can: add support for Xilinx CAN FD core").
> 
> But:
> 
> > @@ -1435,6 +1436,7 @@ static const struct xcan_devtype_data
> > xcan_zynq_data = {  };
> >
> >  static const struct xcan_devtype_data xcan_axi_data = {
> > +   .flags = XCAN_FLAG_TXFEMP,
> > .bittiming_const = _bittiming_const,
> > .btr_ts2_shift = XCAN_BTR_TS2_SHIFT,
> > .btr_sjw_shift = XCAN_BTR_SJW_SHIFT,
> 
> 
> Are you sure this is right?
> In the documentation [1] there does not seem to be any TXFEMP interrupt, it
> would be interrupt bit 14 but AXI CAN 5.0 seems to only go up to 11.
> 
> Or maybe it is undocumented or there is a newer version somewhere?

Sorry for the delay in the reply. 
Agree TXFEMP interrupt feature is not supported by the Soft IP CAN.
Since this patch already got applied will send a separate patch to fix this.

Regards,
Kedar.

> 
> [1]
> https://www.xilinx.com/support/documentation/ip_documentation/can/v5_0
> /pg096-can.pdf
> 
> --
> Anssi Hannula / Bitwise Oy
> +358 503803997

RE: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Did you get a chance to send your framework changes to upstream?
@Moritz Fischer: If Alan couldn't send his patch series, Can we take this patch 
series??
Please let me know your thoughts on this. 

Regards,
Kedar.
> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
>  wrote:
> 
> Hi Appana,
> 
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
> 
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs 
> implementation.
> I see a lot of other kernel drivers have done it this way.  I have an
> implementation that I haven't submitted yet, it exposes different 
> functionality
> (exposing the image firmware file name and writing to the image file).  It's 
> on
> the downstream github.com/altera-opensource repo [1].  I'll clean this up and
> submit it to the mailing list, it may take a minute for me to get to that.
> Once that's done, your added functionality can be a patch on top of that.
> 
> Alan
> 
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
> 
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> >  drivers/fpga/Kconfig  |  7 +
> >  drivers/fpga/fpga-mgr.c   | 68
> +++
> >  include/linux/fpga/fpga-mgr.h |  5 
> >  3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> >  if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > +   tristate "FPGA Debug fs"
> > +   select DEBUG_FS
> > +   help
> > + FPGA manager debug provides support for reading fpga configuration
> > + information.
> > +
> >  config FPGA_MGR_SOCFPGA
> > tristate "Altera SOCFPGA FPGA Manager"
> > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include 
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +   int ret = 0;
> > +
> > +   if (!mgr->mops->read)
> > +   return -ENOENT;
> > +
> > +   if (!mutex_trylock(>ref_mutex))
> > +   return -EBUSY;
> > +
> > +   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > +   ret = -EPERM;
> > +   goto err_unlock;
> > +   }
> > +
> > +   /* Read the FPGA configuration data from the fabric */
> > +   ret = mgr->mops->read(mgr, s);
> > +   if (ret)
> > +   dev_err(>dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > +   mutex_unlock(>ref_mutex);
> > +
> > +   return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +   return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > +   .owner = THIS_MODULE,
> > +   .open = fpga_mgr_read_open,
> > +   .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> 

[PATCH v2 1/5] can: xilinx_can: Skip error message on deferred probe

[Appana Durga Kedareswara rao]

From: Venkatesh Yadav Abbarapu 

When can clock is provided from the clock wizard, clock wizard driver may
not be available when can driver probes resulting to the error message
"Device clock not found error".

As this error message is not very userful to the end user, skip printing
it in the case of deferred probe.

Fixes: b1201e44 ("can: xilinx CAN controller support")
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Venkatesh Yadav Abbarapu 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index bd95cfa..ac175ab 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1791,7 +1791,8 @@ static int xcan_probe(struct platform_device *pdev)
/* Getting the CAN can_clk info */
priv->can_clk = devm_clk_get(>dev, "can_clk");
if (IS_ERR(priv->can_clk)) {
-   dev_err(>dev, "Device clock not found.\n");
+   if (PTR_ERR(priv->can_clk) != -EPROBE_DEFER)
+   dev_err(>dev, "Device clock not found.\n");
ret = PTR_ERR(priv->can_clk);
goto err_free;
}
-- 
2.7.4

[PATCH v2 3/5] can: xilinx_can: Fix the data updation logic for CANFD FD frames

[Appana Durga Kedareswara rao]

commit c223da689324 ("can: xilinx_can: Add support for CANFD FD frames")
is writing data to a wrong offset for FD frames.

This patch fixes this issue.

Fixes: c223da6 ("can: xilinx_can: Add support for CANFD FD frames")
Reviewed-by: Radhey Shyam Pandey 
Reviewed-by: Shubhrajyoti Datta 
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 2d3399e..c9b951b 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -66,8 +66,7 @@ enum xcan_reg {
 #define XCAN_FRAME_DLC_OFFSET(frame_base)  ((frame_base) + 0x04)
 #define XCAN_FRAME_DW1_OFFSET(frame_base)  ((frame_base) + 0x08)
 #define XCAN_FRAME_DW2_OFFSET(frame_base)  ((frame_base) + 0x0C)
-#define XCANFD_FRAME_DW_OFFSET(frame_base, n)  (((frame_base) + 0x08) + \
-((n) * XCAN_CANFD_FRAME_SIZE))
+#define XCANFD_FRAME_DW_OFFSET(frame_base) ((frame_base) + 0x08)
 
 #define XCAN_CANFD_FRAME_SIZE  0x48
 #define XCAN_TXMSG_FRAME_OFFSET(n) (XCAN_TXMSG_BASE_OFFSET + \
@@ -600,7 +599,7 @@ static void xcan_write_frame(struct xcan_priv *priv, struct 
sk_buff *skb,
if (priv->devtype.cantype == XAXI_CANFD ||
priv->devtype.cantype == XAXI_CANFD_2_0) {
for (i = 0; i < cf->len; i += 4) {
-   ramoff = XCANFD_FRAME_DW_OFFSET(frame_offset, dwindex) +
+   ramoff = XCANFD_FRAME_DW_OFFSET(frame_offset) +
(dwindex * XCANFD_DW_BYTES);
priv->write_reg(priv, ramoff,
be32_to_cpup((__be32 *)(cf->data + i)));
@@ -816,10 +815,8 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
struct net_device_stats *stats = >stats;
struct canfd_frame *cf;
struct sk_buff *skb;
-   u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, fsr, readindex;
+   u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, dw_offset;
 
-   fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
-   readindex = fsr & XCAN_FSR_RI_MASK;
id_xcan = priv->read_reg(priv, XCAN_FRAME_ID_OFFSET(frame_base));
dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
if (dlc & XCAN_DLCR_EDL_MASK)
@@ -863,26 +860,16 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
/* Check the frame received is FD or not*/
if (dlc & XCAN_DLCR_EDL_MASK) {
for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
-   (XCAN_RXMSG_2_FRAME_OFFSET(readindex) +
-   (dwindex * XCANFD_DW_BYTES)));
-   else
-   data[0] = priv->read_reg(priv,
-   (XCAN_RXMSG_FRAME_OFFSET(readindex) +
-   (dwindex * XCANFD_DW_BYTES)));
+   dw_offset = XCANFD_FRAME_DW_OFFSET(frame_base) +
+   (dwindex * XCANFD_DW_BYTES);
+   data[0] = priv->read_reg(priv, dw_offset);
*(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
dwindex++;
}
} else {
for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
-   XCAN_RXMSG_2_FRAME_OFFSET(readindex) +
- i);
-   else
-   data[0] = priv->read_reg(priv,
-   XCAN_RXMSG_FRAME_OFFSET(readindex) + i);
+   dw_offset = XCANFD_FRAME_DW_OFFSET(frame_base);
+   data[0] = priv->read_reg(priv, dw_offset + i);
*(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
}
}
-- 
2.7.4

[PATCH v2 5/5] can: xilinx_can: Fix the data phase btr1 calculation

[Appana Durga Kedareswara rao]

From: Srinivas Neeli 

While calculating bitrate for the data phase, the driver is using phase
segment 1 of the arbitration phase instead of the data phase.

Fixes: c223da6 ("can: xilinx_can: Add support for CANFD FD frames")
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Srinivas Neeli 
Acked-by: Shubhrajyoti Datta 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 4cb8c1c9..ab26691 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -425,7 +425,7 @@ static int xcan_set_bittiming(struct net_device *ndev)
btr0 = dbt->brp - 1;
 
/* Setting Time Segment 1 in BTR Register */
-   btr1 = dbt->prop_seg + bt->phase_seg1 - 1;
+   btr1 = dbt->prop_seg + dbt->phase_seg1 - 1;
 
/* Setting Time Segment 2 in BTR Register */
btr1 |= (dbt->phase_seg2 - 1) << priv->devtype.btr_ts2_shift;
-- 
2.7.4

[PATCH v2 4/5] can: xilinx_can: Fix FSR register FL and RI mask values for canfd 2.0

[Appana Durga Kedareswara rao]

For CANFD 2.0 IP configuration existing driver is using incorrect mask
values for FSR register FL and RI fields.

Fixes: c223da6 ("can: xilinx_can: Add support for CANFD FD frames")
Signed-off-by: Appana Durga Kedareswara rao 
Acked-by: Shubhrajyoti Datta 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index c9b951b..4cb8c1c9 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -123,8 +123,10 @@ enum xcan_reg {
 #define XCAN_IDR_RTR_MASK  0x0001 /* Remote TX request */
 #define XCAN_DLCR_DLC_MASK 0xF000 /* Data length code */
 #define XCAN_FSR_FL_MASK   0x3F00 /* RX Fill Level */
+#define XCAN_2_FSR_FL_MASK 0x7F00 /* RX Fill Level */
 #define XCAN_FSR_IRI_MASK  0x0080 /* RX Increment Read Index */
 #define XCAN_FSR_RI_MASK   0x001F /* RX Read Index */
+#define XCAN_2_FSR_RI_MASK 0x003F /* RX Read Index */
 #define XCAN_DLCR_EDL_MASK 0x0800 /* EDL Mask in DLC */
 #define XCAN_DLCR_BRS_MASK 0x0400 /* BRS Mask in DLC */
 
@@ -1138,7 +1140,7 @@ static int xcan_rx_fifo_get_next_frame(struct xcan_priv 
*priv)
int offset;
 
if (priv->devtype.flags & XCAN_FLAG_RX_FIFO_MULTI) {
-   u32 fsr;
+   u32 fsr, mask;
 
/* clear RXOK before the is-empty check so that any newly
 * received frame will reassert it without a race
@@ -1148,12 +1150,17 @@ static int xcan_rx_fifo_get_next_frame(struct xcan_priv 
*priv)
fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
 
/* check if RX FIFO is empty */
-   if (!(fsr & XCAN_FSR_FL_MASK))
+   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
+   mask = XCAN_2_FSR_FL_MASK;
+   else
+   mask = XCAN_FSR_FL_MASK;
+
+   if (!(fsr & mask))
return -ENOENT;
 
if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
offset =
- XCAN_RXMSG_2_FRAME_OFFSET(fsr & XCAN_FSR_RI_MASK);
+ XCAN_RXMSG_2_FRAME_OFFSET(fsr & XCAN_2_FSR_RI_MASK);
else
offset =
  XCAN_RXMSG_FRAME_OFFSET(fsr & XCAN_FSR_RI_MASK);
-- 
2.7.4

[PATCH v2 2/5] can: xilinx_can: Fix FSR register handling in the rx path

[Appana Durga Kedareswara rao]

After commit c223da689324 ("can: xilinx_can: Add support for
CANFD FD frames") Driver is updating the FSR IRI index multiple
times(i.e in xcanfd_rx() and xcan_rx_fifo_get_next_frame()),
It should be updated once per rx packet this patch fixes this issue,
also this patch removes the unnecessary fsr register checks in
xcanfd_rx() API.

Fixes: c223da6 ("can: xilinx_can: Add support for CANFD FD frames")
Reviewed-by: Radhey Shyam Pandey 
Reviewed-by: Shubhrajyoti Datta 
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 139 ---
 1 file changed, 63 insertions(+), 76 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index ac175ab..2d3399e 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -819,91 +819,78 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, fsr, readindex;
 
fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
-   if (fsr & XCAN_FSR_FL_MASK) {
-   readindex = fsr & XCAN_FSR_RI_MASK;
-   id_xcan = priv->read_reg(priv,
-XCAN_FRAME_ID_OFFSET(frame_base));
-   dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
-   if (dlc & XCAN_DLCR_EDL_MASK)
-   skb = alloc_canfd_skb(ndev, );
-   else
-   skb = alloc_can_skb(ndev, (struct can_frame **));
+   readindex = fsr & XCAN_FSR_RI_MASK;
+   id_xcan = priv->read_reg(priv, XCAN_FRAME_ID_OFFSET(frame_base));
+   dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
+   if (dlc & XCAN_DLCR_EDL_MASK)
+   skb = alloc_canfd_skb(ndev, );
+   else
+   skb = alloc_can_skb(ndev, (struct can_frame **));
 
-   if (unlikely(!skb)) {
-   stats->rx_dropped++;
-   return 0;
-   }
+   if (unlikely(!skb)) {
+   stats->rx_dropped++;
+   return 0;
+   }
 
-   /* Change Xilinx CANFD data length format to socketCAN data
-* format
-*/
-   if (dlc & XCAN_DLCR_EDL_MASK)
-   cf->len = can_dlc2len((dlc & XCAN_DLCR_DLC_MASK) >>
+   /* Change Xilinx CANFD data length format to socketCAN data
+* format
+*/
+   if (dlc & XCAN_DLCR_EDL_MASK)
+   cf->len = can_dlc2len((dlc & XCAN_DLCR_DLC_MASK) >>
+ XCAN_DLCR_DLC_SHIFT);
+   else
+   cf->len = get_can_dlc((dlc & XCAN_DLCR_DLC_MASK) >>
  XCAN_DLCR_DLC_SHIFT);
-   else
-   cf->len = get_can_dlc((dlc & XCAN_DLCR_DLC_MASK) >>
- XCAN_DLCR_DLC_SHIFT);
-
-   /* Change Xilinx CAN ID format to socketCAN ID format */
-   if (id_xcan & XCAN_IDR_IDE_MASK) {
-   /* The received frame is an Extended format frame */
-   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
-   cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
-   XCAN_IDR_ID2_SHIFT;
-   cf->can_id |= CAN_EFF_FLAG;
-   if (id_xcan & XCAN_IDR_RTR_MASK)
-   cf->can_id |= CAN_RTR_FLAG;
-   } else {
-   /* The received frame is a standard format frame */
-   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
-   XCAN_IDR_ID1_SHIFT;
-   if (!(dlc & XCAN_DLCR_EDL_MASK) && (id_xcan &
-   XCAN_IDR_SRR_MASK))
-   cf->can_id |= CAN_RTR_FLAG;
-   }
 
-   /* Check the frame received is FD or not*/
-   if (dlc & XCAN_DLCR_EDL_MASK) {
-   for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
+   /* Change Xilinx CAN ID format to socketCAN ID format */
+   if (id_xcan & XCAN_IDR_IDE_MASK) {
+   /* The received frame is an Extended format frame */
+   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
+   cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
+   XCAN_IDR_ID2_SHIFT;
+   cf->can_id |= CAN_EFF_FLA

[PATCH v2 0/5] can: xilinx_can: Bug fixes

[Appana Durga Kedareswara rao]

This patch series fixes below issues
--> Bugs in the driver w.r.to CANFD 2.0 IP support
--> Defer the probe if clock is not found

Changes for v2:
[1/5]:
--> Improved commit description
[1/5] and [5/5]:
--> Added sob line
[1/5], [2/5], [3/5], [4/5], [5/5]:
--> Added Fixes tag in the commit description

Appana Durga Kedareswara rao (3):
  can: xilinx_can: Fix FSR register handling in the rx path
  can: xilinx_can: Fix the data updation logic for CANFD FD frames
  can: xilinx_can: Fix FSR register FL and RI mask values for canfd 2.0

Srinivas Neeli (1):
  can: xilinx_can: Fix the data phase btr1 calculation

Venkatesh Yadav Abbarapu (1):
  can: xilinx_can: Skip error message on deferred probe

 drivers/net/can/xilinx_can.c | 162 +++
 1 file changed, 72 insertions(+), 90 deletions(-)

-- 
2.7.4

RE: [PATCH 0/5] can: xilinx_can: Bug fixes

[Appana Durga Kedareswara Rao]

Hi Marc,

Thanks for the review.

 
> On 8/12/19 9:28 AM, Appana Durga Kedareswara rao wrote:
> > This patch series fixes below issues
> > --> Bugs in the driver w.r.to CANFD 2.0 IP support Defer the probe if
> > --> clock is not found
> >
> > Appana Durga Kedareswara rao (3):
> >   can: xilinx_can: Fix FSR register handling in the rx path
> >   can: xilinx_can: Fix the data updation logic for CANFD FD frames
> >   can: xilinx_can: Fix FSR register FL and RI mask values for canfd
> > 2.0
> >
> > Srinivas Neeli (1):
> >   can: xilinx_can: Fix the data phase btr1 calculation
> >
> > Venkatesh Yadav Abbarapu (1):
> >   can: xilinx_can: defer the probe if clock is not found
> 
> Please add your S-o-b to patches 4+5.
> 
> As these all are bugfixes please add a reference to the commit it fixes:
> 
> Fixes: commitish ("description")

Sure will fix in v2... 

Regards,
Kedar. 

> 
> Marc
> 
> --
> Pengutronix e.K.  | Marc Kleine-Budde   |
> Industrial Linux Solutions| Phone: +49-231-2826-924 |
> Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

RE: [PATCH 1/5] can: xilinx_can: defer the probe if clock is not found

[Appana Durga Kedareswara Rao]

Hi Marc,

Thanks for the review...

 
> On 8/12/19 9:28 AM, Appana Durga Kedareswara rao wrote:
> > From: Venkatesh Yadav Abbarapu 
> >
> > It's not always the case that clock is already available when can
> > driver get probed at the first time, e.g. the clock is provided by
> > clock wizard which may be probed after can driver. So let's defer the
> > probe when devm_clk_get() call fails and give it chance to try later.
> 
> Technically the patch changes the error message to not being printed in case
> of EPROBE_DEFER. This patch doesn't change any behaviour apart from that.
> Please adjust the patch description accordingly.

Sure will fix in v2... 

Regards,
Kedar.

> 
> Marc
> 
> >
> > Signed-off-by: Venkatesh Yadav Abbarapu
> > 
> > Signed-off-by: Michal Simek 
> > ---
> >  drivers/net/can/xilinx_can.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/xilinx_can.c
> > b/drivers/net/can/xilinx_can.c index bd95cfa..ac175ab 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -1791,7 +1791,8 @@ static int xcan_probe(struct platform_device
> *pdev)
> > /* Getting the CAN can_clk info */
> > priv->can_clk = devm_clk_get(>dev, "can_clk");
> > if (IS_ERR(priv->can_clk)) {
> > -   dev_err(>dev, "Device clock not found.\n");
> > +   if (PTR_ERR(priv->can_clk) != -EPROBE_DEFER)
> > +   dev_err(>dev, "Device clock not found.\n");
> > ret = PTR_ERR(priv->can_clk);
> > goto err_free;
> > }
> >
> 
> 
> --
> Pengutronix e.K.  | Marc Kleine-Budde   |
> Industrial Linux Solutions| Phone: +49-231-2826-924 |
> Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

[PATCH 1/5] can: xilinx_can: defer the probe if clock is not found

[Appana Durga Kedareswara rao]

From: Venkatesh Yadav Abbarapu 

It's not always the case that clock is already available when can
driver get probed at the first time, e.g. the clock is provided by
clock wizard which may be probed after can driver. So let's defer
the probe when devm_clk_get() call fails and give it chance to
try later.

Signed-off-by: Venkatesh Yadav Abbarapu 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index bd95cfa..ac175ab 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -1791,7 +1791,8 @@ static int xcan_probe(struct platform_device *pdev)
/* Getting the CAN can_clk info */
priv->can_clk = devm_clk_get(>dev, "can_clk");
if (IS_ERR(priv->can_clk)) {
-   dev_err(>dev, "Device clock not found.\n");
+   if (PTR_ERR(priv->can_clk) != -EPROBE_DEFER)
+   dev_err(>dev, "Device clock not found.\n");
ret = PTR_ERR(priv->can_clk);
goto err_free;
}
-- 
2.7.4

[PATCH 3/5] can: xilinx_can: Fix the data updation logic for CANFD FD frames

[Appana Durga Kedareswara rao]

commit c223da689324 ("can: xilinx_can: Add support for CANFD FD frames")
is writing data to a wrong offset for FD frames.

This patch fixes this issue.

Reviewed-by: Radhey Shyam Pandey 
Reviewed-by: Shubhrajyoti Datta 
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 2d3399e..c9b951b 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -66,8 +66,7 @@ enum xcan_reg {
 #define XCAN_FRAME_DLC_OFFSET(frame_base)  ((frame_base) + 0x04)
 #define XCAN_FRAME_DW1_OFFSET(frame_base)  ((frame_base) + 0x08)
 #define XCAN_FRAME_DW2_OFFSET(frame_base)  ((frame_base) + 0x0C)
-#define XCANFD_FRAME_DW_OFFSET(frame_base, n)  (((frame_base) + 0x08) + \
-((n) * XCAN_CANFD_FRAME_SIZE))
+#define XCANFD_FRAME_DW_OFFSET(frame_base) ((frame_base) + 0x08)
 
 #define XCAN_CANFD_FRAME_SIZE  0x48
 #define XCAN_TXMSG_FRAME_OFFSET(n) (XCAN_TXMSG_BASE_OFFSET + \
@@ -600,7 +599,7 @@ static void xcan_write_frame(struct xcan_priv *priv, struct 
sk_buff *skb,
if (priv->devtype.cantype == XAXI_CANFD ||
priv->devtype.cantype == XAXI_CANFD_2_0) {
for (i = 0; i < cf->len; i += 4) {
-   ramoff = XCANFD_FRAME_DW_OFFSET(frame_offset, dwindex) +
+   ramoff = XCANFD_FRAME_DW_OFFSET(frame_offset) +
(dwindex * XCANFD_DW_BYTES);
priv->write_reg(priv, ramoff,
be32_to_cpup((__be32 *)(cf->data + i)));
@@ -816,10 +815,8 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
struct net_device_stats *stats = >stats;
struct canfd_frame *cf;
struct sk_buff *skb;
-   u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, fsr, readindex;
+   u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, dw_offset;
 
-   fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
-   readindex = fsr & XCAN_FSR_RI_MASK;
id_xcan = priv->read_reg(priv, XCAN_FRAME_ID_OFFSET(frame_base));
dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
if (dlc & XCAN_DLCR_EDL_MASK)
@@ -863,26 +860,16 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
/* Check the frame received is FD or not*/
if (dlc & XCAN_DLCR_EDL_MASK) {
for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
-   (XCAN_RXMSG_2_FRAME_OFFSET(readindex) +
-   (dwindex * XCANFD_DW_BYTES)));
-   else
-   data[0] = priv->read_reg(priv,
-   (XCAN_RXMSG_FRAME_OFFSET(readindex) +
-   (dwindex * XCANFD_DW_BYTES)));
+   dw_offset = XCANFD_FRAME_DW_OFFSET(frame_base) +
+   (dwindex * XCANFD_DW_BYTES);
+   data[0] = priv->read_reg(priv, dw_offset);
*(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
dwindex++;
}
} else {
for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
-   XCAN_RXMSG_2_FRAME_OFFSET(readindex) +
- i);
-   else
-   data[0] = priv->read_reg(priv,
-   XCAN_RXMSG_FRAME_OFFSET(readindex) + i);
+   dw_offset = XCANFD_FRAME_DW_OFFSET(frame_base);
+   data[0] = priv->read_reg(priv, dw_offset + i);
*(__be32 *)(cf->data + i) = cpu_to_be32(data[0]);
}
}
-- 
2.7.4

[PATCH 2/5] can: xilinx_can: Fix FSR register handling in the rx path

[Appana Durga Kedareswara rao]

After commit c223da689324 ("can: xilinx_can: Add support for
CANFD FD frames") Driver is updating the FSR IRI index multiple
times(i.e in xcanfd_rx() and xcan_rx_fifo_get_next_frame()),
It should be updated once per rx packet this patch fixes this issue,
also this patch removes the unnecessary fsr register checks in
xcanfd_rx() API.

Reviewed-by: Radhey Shyam Pandey 
Reviewed-by: Shubhrajyoti Datta 
Signed-off-by: Appana Durga Kedareswara rao 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 139 ---
 1 file changed, 63 insertions(+), 76 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index ac175ab..2d3399e 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -819,91 +819,78 @@ static int xcanfd_rx(struct net_device *ndev, int 
frame_base)
u32 id_xcan, dlc, data[2] = {0, 0}, dwindex = 0, i, fsr, readindex;
 
fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
-   if (fsr & XCAN_FSR_FL_MASK) {
-   readindex = fsr & XCAN_FSR_RI_MASK;
-   id_xcan = priv->read_reg(priv,
-XCAN_FRAME_ID_OFFSET(frame_base));
-   dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
-   if (dlc & XCAN_DLCR_EDL_MASK)
-   skb = alloc_canfd_skb(ndev, );
-   else
-   skb = alloc_can_skb(ndev, (struct can_frame **));
+   readindex = fsr & XCAN_FSR_RI_MASK;
+   id_xcan = priv->read_reg(priv, XCAN_FRAME_ID_OFFSET(frame_base));
+   dlc = priv->read_reg(priv, XCAN_FRAME_DLC_OFFSET(frame_base));
+   if (dlc & XCAN_DLCR_EDL_MASK)
+   skb = alloc_canfd_skb(ndev, );
+   else
+   skb = alloc_can_skb(ndev, (struct can_frame **));
 
-   if (unlikely(!skb)) {
-   stats->rx_dropped++;
-   return 0;
-   }
+   if (unlikely(!skb)) {
+   stats->rx_dropped++;
+   return 0;
+   }
 
-   /* Change Xilinx CANFD data length format to socketCAN data
-* format
-*/
-   if (dlc & XCAN_DLCR_EDL_MASK)
-   cf->len = can_dlc2len((dlc & XCAN_DLCR_DLC_MASK) >>
+   /* Change Xilinx CANFD data length format to socketCAN data
+* format
+*/
+   if (dlc & XCAN_DLCR_EDL_MASK)
+   cf->len = can_dlc2len((dlc & XCAN_DLCR_DLC_MASK) >>
+ XCAN_DLCR_DLC_SHIFT);
+   else
+   cf->len = get_can_dlc((dlc & XCAN_DLCR_DLC_MASK) >>
  XCAN_DLCR_DLC_SHIFT);
-   else
-   cf->len = get_can_dlc((dlc & XCAN_DLCR_DLC_MASK) >>
- XCAN_DLCR_DLC_SHIFT);
-
-   /* Change Xilinx CAN ID format to socketCAN ID format */
-   if (id_xcan & XCAN_IDR_IDE_MASK) {
-   /* The received frame is an Extended format frame */
-   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
-   cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
-   XCAN_IDR_ID2_SHIFT;
-   cf->can_id |= CAN_EFF_FLAG;
-   if (id_xcan & XCAN_IDR_RTR_MASK)
-   cf->can_id |= CAN_RTR_FLAG;
-   } else {
-   /* The received frame is a standard format frame */
-   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
-   XCAN_IDR_ID1_SHIFT;
-   if (!(dlc & XCAN_DLCR_EDL_MASK) && (id_xcan &
-   XCAN_IDR_SRR_MASK))
-   cf->can_id |= CAN_RTR_FLAG;
-   }
 
-   /* Check the frame received is FD or not*/
-   if (dlc & XCAN_DLCR_EDL_MASK) {
-   for (i = 0; i < cf->len; i += 4) {
-   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
-   data[0] = priv->read_reg(priv,
+   /* Change Xilinx CAN ID format to socketCAN ID format */
+   if (id_xcan & XCAN_IDR_IDE_MASK) {
+   /* The received frame is an Extended format frame */
+   cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
+   cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
+   XCAN_IDR_ID2_SHIFT;
+   cf->can_id |= CAN_EFF_FLAG;
+   if (id_xcan & XCAN_IDR_RTR_MASK)
+   cf->can_id |= CAN_RTR_FLAG;
+ 

[PATCH 5/5] can: xilinx_can: Fix the data phase btr1 calculation

[Appana Durga Kedareswara rao]

From: Srinivas Neeli 

While calculating bitrate for the data phase, the driver is using phase
segment 1 of the arbitration phase instead of the data phase.

Signed-off-by: Srinivas Neeli 
Acked-by: Shubhrajyoti Datta 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 4cb8c1c9..ab26691 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -425,7 +425,7 @@ static int xcan_set_bittiming(struct net_device *ndev)
btr0 = dbt->brp - 1;
 
/* Setting Time Segment 1 in BTR Register */
-   btr1 = dbt->prop_seg + bt->phase_seg1 - 1;
+   btr1 = dbt->prop_seg + dbt->phase_seg1 - 1;
 
/* Setting Time Segment 2 in BTR Register */
btr1 |= (dbt->phase_seg2 - 1) << priv->devtype.btr_ts2_shift;
-- 
2.7.4

[PATCH 4/5] can: xilinx_can: Fix FSR register FL and RI mask values for canfd 2.0

[Appana Durga Kedareswara rao]

For CANFD 2.0 IP configuration existing driver is using incorrect mask
values for FSR register FL and RI fields.

Signed-off-by: Appana Durga Kedareswara rao 
Acked-by: Shubhrajyoti Datta 
Signed-off-by: Michal Simek 
---
 drivers/net/can/xilinx_can.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index c9b951b..4cb8c1c9 100644
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -123,8 +123,10 @@ enum xcan_reg {
 #define XCAN_IDR_RTR_MASK  0x0001 /* Remote TX request */
 #define XCAN_DLCR_DLC_MASK 0xF000 /* Data length code */
 #define XCAN_FSR_FL_MASK   0x3F00 /* RX Fill Level */
+#define XCAN_2_FSR_FL_MASK 0x7F00 /* RX Fill Level */
 #define XCAN_FSR_IRI_MASK  0x0080 /* RX Increment Read Index */
 #define XCAN_FSR_RI_MASK   0x001F /* RX Read Index */
+#define XCAN_2_FSR_RI_MASK 0x003F /* RX Read Index */
 #define XCAN_DLCR_EDL_MASK 0x0800 /* EDL Mask in DLC */
 #define XCAN_DLCR_BRS_MASK 0x0400 /* BRS Mask in DLC */
 
@@ -1138,7 +1140,7 @@ static int xcan_rx_fifo_get_next_frame(struct xcan_priv 
*priv)
int offset;
 
if (priv->devtype.flags & XCAN_FLAG_RX_FIFO_MULTI) {
-   u32 fsr;
+   u32 fsr, mask;
 
/* clear RXOK before the is-empty check so that any newly
 * received frame will reassert it without a race
@@ -1148,12 +1150,17 @@ static int xcan_rx_fifo_get_next_frame(struct xcan_priv 
*priv)
fsr = priv->read_reg(priv, XCAN_FSR_OFFSET);
 
/* check if RX FIFO is empty */
-   if (!(fsr & XCAN_FSR_FL_MASK))
+   if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
+   mask = XCAN_2_FSR_FL_MASK;
+   else
+   mask = XCAN_FSR_FL_MASK;
+
+   if (!(fsr & mask))
return -ENOENT;
 
if (priv->devtype.flags & XCAN_FLAG_CANFD_2)
offset =
- XCAN_RXMSG_2_FRAME_OFFSET(fsr & XCAN_FSR_RI_MASK);
+ XCAN_RXMSG_2_FRAME_OFFSET(fsr & XCAN_2_FSR_RI_MASK);
else
offset =
  XCAN_RXMSG_FRAME_OFFSET(fsr & XCAN_FSR_RI_MASK);
-- 
2.7.4

[PATCH 0/5] can: xilinx_can: Bug fixes

[Appana Durga Kedareswara rao]

This patch series fixes below issues
--> Bugs in the driver w.r.to CANFD 2.0 IP support
--> Defer the probe if clock is not found

Appana Durga Kedareswara rao (3):
  can: xilinx_can: Fix FSR register handling in the rx path
  can: xilinx_can: Fix the data updation logic for CANFD FD frames
  can: xilinx_can: Fix FSR register FL and RI mask values for canfd 2.0

Srinivas Neeli (1):
  can: xilinx_can: Fix the data phase btr1 calculation

Venkatesh Yadav Abbarapu (1):
  can: xilinx_can: defer the probe if clock is not found

 drivers/net/can/xilinx_can.c | 162 +++
 1 file changed, 72 insertions(+), 90 deletions(-)

-- 
2.7.4

RE: [PATCH 0/6] net: can: xilinx_can: Bug fixes and Enhancements

[Appana Durga Kedareswara Rao]

Hi Marc,

Friendly ping !!

> -Original Message-
> From: Appana Durga Kedareswara Rao
> Sent: Tuesday, April 23, 2019 12:08 PM
> To: 'Marc Kleine-Budde' ; 'w...@grandegger.com'
> ; 'da...@davemloft.net' ;
> Michal Simek 
> Cc: 'linux-...@vger.kernel.org' ;
> 'net...@vger.kernel.org' ; 'linux-arm-
> ker...@lists.infradead.org' ; 'linux-
> ker...@vger.kernel.org' 
> Subject: RE: [PATCH 0/6] net: can: xilinx_can: Bug fixes and Enhancements
> 
> Hi Marc,
> 
> > -----Original Message-
> > From: Appana Durga Kedareswara Rao
> > Sent: Thursday, March 21, 2019 12:06 PM
> > To: 'Marc Kleine-Budde' ; w...@grandegger.com;
> > da...@davemloft.net; Michal Simek 
> > Cc: linux-...@vger.kernel.org; net...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH 0/6] net: can: xilinx_can: Bug fixes and
> > Enhancements
> >
> > Hi Marc,
> >
> > > -----Original Message-
> > > From: Marc Kleine-Budde 
> > > Sent: Wednesday, March 20, 2019 6:39 PM
> > > To: Appana Durga Kedareswara Rao ;
> > > w...@grandegger.com; da...@davemloft.net; Michal Simek
> > > 
> > > Cc: linux-...@vger.kernel.org; net...@vger.kernel.org; linux-arm-
> > > ker...@lists.infradead.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 0/6] net: can: xilinx_can: Bug fixes and
> > > Enhancements
> > >
> > > On 3/18/19 12:32 PM, Appana Durga Kedareswara rao wrote:
> > > > This patch series does the below
> > > > --> Added support for CANFD FD frames Fix Checkpatch reported
> > > > --> warnings and checks
> > > >
> > > > Appana Durga Kedareswara rao (6):
> > > >   net: can: xilinx_can: Fix style issues
> > > >   net: can: xilinx_can: Fix flags field initialization for axi can and
> > > > canps
> > > >   net: can: xilinx_can: Add cantype parameter in xcan_devtype_data
> > > > struct
> > > >   net: can: xilinx_can: Add support for CANFD FD frames
> > > >   net: can: xilinx_can: Add SPDX license
> > > >   net: can: xilinx_can: Fix kernel doc warnings
> > > >
> > > >  drivers/net/can/xilinx_can.c | 303
> > > > ---
> > > >  1 file changed, 257 insertions(+), 46 deletions(-)
> > >
> > > Applied to linux-can-next/testing, but in order. First fixes than
> > > enhancements:
> >
> > Thanks...
> > I couldn't find the patches here
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-
> > next.git/log/drivers/net/can/xilinx_can.c?h=testing
> > Am I referring wrong repo/branch??
> 
> There are a couple of bug fixes available for this driver on top of this patch
> series.
> Please let me know the branch where this patch series got applied, will send
> the bug fixes on top of that branch.
> 

Regards,
Kedar.

> Regards,
> Kedar.
> >
> > Regards,
> > Kedar.
> > >
> > > > 241826302854 net: can: xilinx_can: Fix style issues
> > > > 70fb26fadc27 net: can: xilinx_can: Fix kernel doc warnings
> > > > 7beb64351ff1 net: can: xilinx_can: Add SPDX license dd94910bceae net:
> > > > can: xilinx_can: Fix flags field initialization for axi can and
> > > > canps
> > > > 6bd05cece567 net: can: xilinx_can: Add cantype parameter in
> > > > xcan_devtype_data struct
> > > > 34e280170736 net: can: xilinx_can: Add support for CANFD FD frames
> > >
> > > Marc
> > >
> > > --
> > > Pengutronix e.K.  | Marc Kleine-Budde   |
> > > Industrial Linux Solutions| Phone: +49-231-2826-924 |
> > > Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
> > > Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

RE: [PATCH] net: can: Increase tx queue length

[Appana Durga Kedareswara Rao]

Hi Andre,

 
> 
> On 3/9/19 3:07 PM, Appana Durga Kedareswara rao wrote:
> > While stress testing the CAN interface on xilinx axi can in loopback
> > mode getting message "write: no buffer space available"
> > Increasing device tx queue length resolved the above mentioned issue.
> 
> No need to patch the kernel:
> 
> $ ip link set  txqueuelen 500
> 
> does the same thing.

Thanks for the review... 
Agree but it is not an out of box solution right?? 
Do you have any idea for socket can devices why the tx queue length is 10 
whereas
for other network devices (ex: ethernet) it is 1000 ?? 

Regards,
Kedar.
> 
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> > --> Network devices default tx_queue_len is 1000 but for socket
> > can device it is 10 any reason for it??
> >
> >  drivers/net/can/dev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index
> > c05e4d5..32bd5be 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -642,7 +642,7 @@ static void can_setup(struct net_device *dev)
> > dev->mtu = CAN_MTU;
> > dev->hard_header_len = 0;
> > dev->addr_len = 0;
> > -   dev->tx_queue_len = 10;
> > +   dev->tx_queue_len = 500;
> >
> > /* New-style flags. */
> > dev->flags = IFF_NOARP;
> >

[PATCH] net: can: Increase tx queue length

[Appana Durga Kedareswara rao]

While stress testing the CAN interface on xilinx axi can
in loopback mode getting message "write: no buffer space available"
Increasing device tx queue length resolved the above mentioned issue.

Signed-off-by: Appana Durga Kedareswara rao 
---
--> Network devices default tx_queue_len is 1000 but for socket
can device it is 10 any reason for it?? 
 
 drivers/net/can/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c05e4d5..32bd5be 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -642,7 +642,7 @@ static void can_setup(struct net_device *dev)
dev->mtu = CAN_MTU;
dev->hard_header_len = 0;
dev->addr_len = 0;
-   dev->tx_queue_len = 10;
+   dev->tx_queue_len = 500;
 
/* New-style flags. */
dev->flags = IFF_NOARP;
-- 
2.7.4

RE: [PATCH v2 4/4] dmaengine: xilinx_dma: Fix 64-bit simple CDMA transfer

[Appana Durga Kedareswara Rao]

> -Original Message-
> From: Radhey Shyam Pandey 
> Sent: Saturday, September 29, 2018 10:48 PM
> To: vk...@kernel.org; dan.j.willi...@intel.com; Michal Simek
> ; Appana Durga Kedareswara Rao
> ; Radhey Shyam Pandey 
> Cc: dmaeng...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH v2 4/4] dmaengine: xilinx_dma: Fix 64-bit simple CDMA
> transfer
> 
> In AXI CDMA simple mode also pass MSB bits of source and destination
> address to xilinx_write function. This fixes simple CDMA operation mode using
> 64-bit addressing.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Reviewed-by: Appana Durga Kedareswara Rao 

Regards,
Kedar.

> ---
> Changes for v2:
> Use helper macro for preparing dma_addr_t.
> ---
>  drivers/dma/xilinx/xilinx_dma.c |6 --
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index c27ab64..d04ef85 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1247,8 +1247,10 @@ static void xilinx_cdma_start_transfer(struct
> xilinx_dma_chan *chan)
> 
>   hw = >hw;
> 
> - xilinx_write(chan, XILINX_CDMA_REG_SRCADDR, hw-
> >src_addr);
> - xilinx_write(chan, XILINX_CDMA_REG_DSTADDR, hw-
> >dest_addr);
> + xilinx_write(chan, XILINX_CDMA_REG_SRCADDR,
> +  xilinx_prep_dma_addr_t(hw->src_addr));
> + xilinx_write(chan, XILINX_CDMA_REG_DSTADDR,
> +  xilinx_prep_dma_addr_t(hw->dest_addr));
> 
>   /* Start the transfer */
>   dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
> --
> 1.7.1

RE: [PATCH v2 4/4] dmaengine: xilinx_dma: Fix 64-bit simple CDMA transfer

[Appana Durga Kedareswara Rao]

> -Original Message-
> From: Radhey Shyam Pandey 
> Sent: Saturday, September 29, 2018 10:48 PM
> To: vk...@kernel.org; dan.j.willi...@intel.com; Michal Simek
> ; Appana Durga Kedareswara Rao
> ; Radhey Shyam Pandey 
> Cc: dmaeng...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH v2 4/4] dmaengine: xilinx_dma: Fix 64-bit simple CDMA
> transfer
> 
> In AXI CDMA simple mode also pass MSB bits of source and destination
> address to xilinx_write function. This fixes simple CDMA operation mode using
> 64-bit addressing.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Reviewed-by: Appana Durga Kedareswara Rao 

Regards,
Kedar.

> ---
> Changes for v2:
> Use helper macro for preparing dma_addr_t.
> ---
>  drivers/dma/xilinx/xilinx_dma.c |6 --
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index c27ab64..d04ef85 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1247,8 +1247,10 @@ static void xilinx_cdma_start_transfer(struct
> xilinx_dma_chan *chan)
> 
>   hw = >hw;
> 
> - xilinx_write(chan, XILINX_CDMA_REG_SRCADDR, hw-
> >src_addr);
> - xilinx_write(chan, XILINX_CDMA_REG_DSTADDR, hw-
> >dest_addr);
> + xilinx_write(chan, XILINX_CDMA_REG_SRCADDR,
> +  xilinx_prep_dma_addr_t(hw->src_addr));
> + xilinx_write(chan, XILINX_CDMA_REG_DSTADDR,
> +  xilinx_prep_dma_addr_t(hw->dest_addr));
> 
>   /* Start the transfer */
>   dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
> --
> 1.7.1

RE: [PATCH v2 3/4] dmaengine: xilinx_dma: Introduce helper macro for preparing dma address

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the patch...

> 
> This patch introduces the xilinx_prep_dma_addr_t macro which prepares
> dma_addr_t from hardware buffer descriptor LSB and MSB fields. It will be
> used in simple dma 64-bit programming sequence.
> 
> Signed-off-by: Radhey Shyam Pandey 

Reviewed-by: Appana Durga Kedareswara Rao 

Regards,
Kedar.

> ---
> Changes for v2:
> New patch- Preparatory change for 4/4 fix.
> ---
>  drivers/dma/xilinx/xilinx_dma.c |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index a37871e..c27ab64 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -190,6 +190,8 @@
>  /* AXI CDMA Specific Masks */
>  #define XILINX_CDMA_CR_SGMODE  BIT(3)
> 
> +#define xilinx_prep_dma_addr_t(addr) \
> + ((dma_addr_t)((u64)addr##_##msb << 32 | (addr)))
>  /**
>   * struct xilinx_vdma_desc_hw - Hardware Descriptor
>   * @next_desc: Next Descriptor Pointer @0x00
> --
> 1.7.1

RE: [PATCH v2 3/4] dmaengine: xilinx_dma: Introduce helper macro for preparing dma address

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the patch...

> 
> This patch introduces the xilinx_prep_dma_addr_t macro which prepares
> dma_addr_t from hardware buffer descriptor LSB and MSB fields. It will be
> used in simple dma 64-bit programming sequence.
> 
> Signed-off-by: Radhey Shyam Pandey 

Reviewed-by: Appana Durga Kedareswara Rao 

Regards,
Kedar.

> ---
> Changes for v2:
> New patch- Preparatory change for 4/4 fix.
> ---
>  drivers/dma/xilinx/xilinx_dma.c |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index a37871e..c27ab64 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -190,6 +190,8 @@
>  /* AXI CDMA Specific Masks */
>  #define XILINX_CDMA_CR_SGMODE  BIT(3)
> 
> +#define xilinx_prep_dma_addr_t(addr) \
> + ((dma_addr_t)((u64)addr##_##msb << 32 | (addr)))
>  /**
>   * struct xilinx_vdma_desc_hw - Hardware Descriptor
>   * @next_desc: Next Descriptor Pointer @0x00
> --
> 1.7.1

RE: [PATCH 1/3] dmaengine: xilinx_dma: Refactor axidma channel allocation

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the patch... 
> 
> In axidma alloc_chan_resources merge BD and cyclic BD allocation.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-for-series: Appana Durga Kedareswara rao 

Regards,
Kedar.

> ---
>  drivers/dma/xilinx/xilinx_dma.c |   36 ++--
>  1 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index c124423..06d1632 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -887,6 +887,24 @@ static int xilinx_dma_alloc_chan_resources(struct
> dma_chan *dchan)
>   chan->id);
>   return -ENOMEM;
>   }
> + /*
> +  * For cyclic DMA mode we need to program the tail
> Descriptor
> +  * register with a value which is not a part of the BD chain
> +  * so allocating a desc segment during channel allocation for
> +  * programming tail descriptor.
> +  */
> + chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
> + sizeof(*chan->cyclic_seg_v),
> + >cyclic_seg_p, GFP_KERNEL);
> + if (!chan->cyclic_seg_v) {
> + dev_err(chan->dev,
> + "unable to allocate desc segment for cyclic
> DMA\n");
> + dma_free_coherent(chan->dev, sizeof(*chan->seg_v)
> *
> + XILINX_DMA_NUM_DESCS, chan->seg_v,
> + chan->seg_p);
> + return -ENOMEM;
> + }
> + chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
> 
>   for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
>   chan->seg_v[i].hw.next_desc =
> @@ -922,24 +940,6 @@ static int xilinx_dma_alloc_chan_resources(struct
> dma_chan *dchan)
>   return -ENOMEM;
>   }
> 
> - if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> - /*
> -  * For cyclic DMA mode we need to program the tail
> Descriptor
> -  * register with a value which is not a part of the BD chain
> -  * so allocating a desc segment during channel allocation for
> -  * programming tail descriptor.
> -  */
> - chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
> - sizeof(*chan->cyclic_seg_v),
> - >cyclic_seg_p, GFP_KERNEL);
> - if (!chan->cyclic_seg_v) {
> - dev_err(chan->dev,
> - "unable to allocate desc segment for cyclic
> DMA\n");
> - return -ENOMEM;
> - }
> - chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
> - }
> -
>   dma_cookie_init(dchan);
> 
>   if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> --
> 1.7.1

RE: [PATCH 1/3] dmaengine: xilinx_dma: Refactor axidma channel allocation

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the patch... 
> 
> In axidma alloc_chan_resources merge BD and cyclic BD allocation.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-for-series: Appana Durga Kedareswara rao 

Regards,
Kedar.

> ---
>  drivers/dma/xilinx/xilinx_dma.c |   36 ++--
>  1 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index c124423..06d1632 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -887,6 +887,24 @@ static int xilinx_dma_alloc_chan_resources(struct
> dma_chan *dchan)
>   chan->id);
>   return -ENOMEM;
>   }
> + /*
> +  * For cyclic DMA mode we need to program the tail
> Descriptor
> +  * register with a value which is not a part of the BD chain
> +  * so allocating a desc segment during channel allocation for
> +  * programming tail descriptor.
> +  */
> + chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
> + sizeof(*chan->cyclic_seg_v),
> + >cyclic_seg_p, GFP_KERNEL);
> + if (!chan->cyclic_seg_v) {
> + dev_err(chan->dev,
> + "unable to allocate desc segment for cyclic
> DMA\n");
> + dma_free_coherent(chan->dev, sizeof(*chan->seg_v)
> *
> + XILINX_DMA_NUM_DESCS, chan->seg_v,
> + chan->seg_p);
> + return -ENOMEM;
> + }
> + chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
> 
>   for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
>   chan->seg_v[i].hw.next_desc =
> @@ -922,24 +940,6 @@ static int xilinx_dma_alloc_chan_resources(struct
> dma_chan *dchan)
>   return -ENOMEM;
>   }
> 
> - if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> - /*
> -  * For cyclic DMA mode we need to program the tail
> Descriptor
> -  * register with a value which is not a part of the BD chain
> -  * so allocating a desc segment during channel allocation for
> -  * programming tail descriptor.
> -  */
> - chan->cyclic_seg_v = dma_zalloc_coherent(chan->dev,
> - sizeof(*chan->cyclic_seg_v),
> - >cyclic_seg_p, GFP_KERNEL);
> - if (!chan->cyclic_seg_v) {
> - dev_err(chan->dev,
> - "unable to allocate desc segment for cyclic
> DMA\n");
> - return -ENOMEM;
> - }
> - chan->cyclic_seg_v->phys = chan->cyclic_seg_p;
> - }
> -
>   dma_cookie_init(dchan);
> 
>   if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> --
> 1.7.1

RE: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review... 


> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
>  wrote:
> 
> Hi Appana,
> 
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
> 
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs
> implementation.  I see a lot of other kernel drivers have done it this way.  I
> have an implementation that I haven't submitted yet, it exposes different
> functionality (exposing the image firmware file name and writing to the
> image file).  It's on the downstream github.com/altera-opensource repo [1].
> I'll clean this up and submit it to the mailing list, it may take a minute 
> for me
> to get to that.
> Once that's done, your added functionality can be a patch on top of that.

Sounds good...
Let me know if you need any help on testing this will do.

Regards,
Kedar.
> 
> Alan
> 
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
> 
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> >  drivers/fpga/Kconfig  |  7 +
> >  drivers/fpga/fpga-mgr.c   | 68
> +++
> >  include/linux/fpga/fpga-mgr.h |  5 
> >  3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> >  if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > +   tristate "FPGA Debug fs"
> > +   select DEBUG_FS
> > +   help
> > + FPGA manager debug provides support for reading fpga
> configuration
> > + information.
> > +
> >  config FPGA_MGR_SOCFPGA
> > tristate "Altera SOCFPGA FPGA Manager"
> > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include 
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +   int ret = 0;
> > +
> > +   if (!mgr->mops->read)
> > +   return -ENOENT;
> > +
> > +   if (!mutex_trylock(>ref_mutex))
> > +   return -EBUSY;
> > +
> > +   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > +   ret = -EPERM;
> > +   goto err_unlock;
> > +   }
> > +
> > +   /* Read the FPGA configuration data from the fabric */
> > +   ret = mgr->mops->read(mgr, s);
> > +   if (ret)
> > +   dev_err(>dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > +   mutex_unlock(>ref_mutex);
> > +
> > +   return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +   return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > +   .owner = THIS_MODULE,
> > +   .open = fpga_mgr_read_open,
> > +   .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:   fpga manager
&

RE: [PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review... 


> On Fri, Jul 27, 2018 at 1:22 AM, Appana Durga Kedareswara rao
>  wrote:
> 
> Hi Appana,
> 
> There should be some documentation for the debugfs added under
> Documentation/driver-api/fpga/
> 
> Also there are a lot of #ifdefs that were added due to the
> CONFIG_FPGA_MGR_DEBUG_FS.  This has caused a kernel robot complaint.
> The way to fix that is to have a separate c file for the debugfs
> implementation.  I see a lot of other kernel drivers have done it this way.  I
> have an implementation that I haven't submitted yet, it exposes different
> functionality (exposing the image firmware file name and writing to the
> image file).  It's on the downstream github.com/altera-opensource repo [1].
> I'll clean this up and submit it to the mailing list, it may take a minute 
> for me
> to get to that.
> Once that's done, your added functionality can be a patch on top of that.

Sounds good...
Let me know if you need any help on testing this will do.

Regards,
Kedar.
> 
> Alan
> 
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.c
> https://github.com/altera-opensource/linux-socfpga/blob/socfpga-
> 4.17/drivers/fpga/fpga-mgr-debugfs.h
> 
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/fpga0/image
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> > Changes for v4:
> > --> None.
> > Changes for v3:
> > --> None.
> > Changes for v2:
> > --> Fixed debug attribute path and name as suggested by Alan Add
> > --> config entry for DEBUG as suggested by Alan Fixed trival coding
> > --> style issues.
> >
> >  drivers/fpga/Kconfig  |  7 +
> >  drivers/fpga/fpga-mgr.c   | 68
> +++
> >  include/linux/fpga/fpga-mgr.h |  5 
> >  3 files changed, 80 insertions(+)
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 53d3f55..838ad4e 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -11,6 +11,13 @@ menuconfig FPGA
> >
> >  if FPGA
> >
> > +config FPGA_MGR_DEBUG_FS
> > +   tristate "FPGA Debug fs"
> > +   select DEBUG_FS
> > +   help
> > + FPGA manager debug provides support for reading fpga
> configuration
> > + information.
> > +
> >  config FPGA_MGR_SOCFPGA
> > tristate "Altera SOCFPGA FPGA Manager"
> > depends on ARCH_SOCFPGA || COMPILE_TEST diff --git
> > a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..4bea860 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_FPGA_MGR_DEBUG_FS
> > +#include 
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +   int ret = 0;
> > +
> > +   if (!mgr->mops->read)
> > +   return -ENOENT;
> > +
> > +   if (!mutex_trylock(>ref_mutex))
> > +   return -EBUSY;
> > +
> > +   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
> > +   ret = -EPERM;
> > +   goto err_unlock;
> > +   }
> > +
> > +   /* Read the FPGA configuration data from the fabric */
> > +   ret = mgr->mops->read(mgr, s);
> > +   if (ret)
> > +   dev_err(>dev, "Error while reading configuration
> > + data from FPGA\n");
> > +
> > +err_unlock:
> > +   mutex_unlock(>ref_mutex);
> > +
> > +   return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +   return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_ops_image = {
> > +   .owner = THIS_MODULE,
> > +   .open = fpga_mgr_read_open,
> > +   .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:   fpga manager
&

[PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v4:
--> None.
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;

[PATCH v4 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v4:
--> None.
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;

[PATCH v4 2/2] fpga: zynq-fpga: Add support for readback of FPGA configuration data and registers

[Appana Durga Kedareswara rao]

This patch adds support for readback of FPGA configuration data and registers.

Usage:
Readback of PL configuration data
cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration registers
cat /sys/kernel/debug/fpga/f8007000.devcfg/cfg_reg

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v4:
--> Improved commit message description as suggested by Moritz.
--> Used GENMASK and BIT() Macros wherever applicable as suggested by Moritz.
--> Fixed commenting sytle issues as suggested by Moritz and Alan.
--> Get rid of the readback_type module param as suggested by Alan and Moritz.
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 430 +++
 1 file changed, 430 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..1746d18 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Offsets into SLCR regmap */
 
@@ -127,6 +128,69 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg:   Name of the configuration register.
+ * @offset:Register offset.
+ */
+struct zynq_configreg {
+   char *reg;
+   u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+   {.reg = "CRC",  .offset = 0},
+   {.reg = "FAR",  .offset = 1},
+   {.reg = "FDRI", .offset = 2},
+   {.reg = "FDRO", .offset = 3},
+   {.reg = "CMD",  .offset = 4},
+   {.reg = "CTRL0",.offset = 5},
+   {.reg = "MASK", .offset = 6},
+   {.reg = "STAT", .offset = 7},
+   {.reg = "LOUT", .offset = 8},
+   {.reg = "COR0", .offset = 9},
+   {.reg = "MFWR", .offset = 10},
+   {.reg = "CBC",  .offset = 11},
+   {.reg = "IDCODE",   .offset = 12},
+   {.reg = "AXSS", .offset = 13},
+   {.reg = "COR1", .offset = 14},
+   {.reg = "WBSTR",.offset = 16},
+   {.reg = "TIMER",.offset = 17},
+   {.reg = "BOOTSTS",  .offset = 22},
+   {.reg = "CTRL1",.offset = 24},
+   {}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK  0x
+#define RCFG_CMD_MASK  BIT(2)
+#define START_CMD_MASK BIT(2) + BIT(0)
+#define RCRC_CMD_MASK  GENMASK(2, 0)
+#define SHUTDOWN_CMD_MASK  GENMASK(1, 0) + BIT(3)
+#define DESYNC_WORD_MASK   GENMASK(2, 3) + BIT(0)
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK BIT(29)
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASKGENMASK(31, 0)
+
+#define TYPE_HDR_SHIFT 29
+#define TYPE_REG_SHIFT 13
+#define TYPE_OP_SHIFT  27
+#define TYPE_OPCODE_NOOP   0
+#define TYPE_OPCODE_READ   1
+#define TYPE_OPCODE_WRITE  2
+#define TYPE_FAR_OFFSET1
+#define TYPE_FDRO_OFFSET   3
+#define TYPE_CMD_OFFSET4
+
+#define READ_STEP5_NOOPS   6
+#define READ_STEP9_NOOPS   32
+
+#define READ_DMA_SIZE  0x200
+#define DUMMY_FRAMES_SIZE  0x28
+#define SLCR_PCAP_FREQ 1000
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +204,11 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct mutex ref_mutex;
+   struct dentry *dir;
+#endif
+   u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +233,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+   u32 status;
+   int ret;
+
+   ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+status & IXR_D_P_DONE_MASK,
+INIT_POLL_DELAY

[PATCH v4 2/2] fpga: zynq-fpga: Add support for readback of FPGA configuration data and registers

[Appana Durga Kedareswara rao]

This patch adds support for readback of FPGA configuration data and registers.

Usage:
Readback of PL configuration data
cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration registers
cat /sys/kernel/debug/fpga/f8007000.devcfg/cfg_reg

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v4:
--> Improved commit message description as suggested by Moritz.
--> Used GENMASK and BIT() Macros wherever applicable as suggested by Moritz.
--> Fixed commenting sytle issues as suggested by Moritz and Alan.
--> Get rid of the readback_type module param as suggested by Alan and Moritz.
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 430 +++
 1 file changed, 430 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..1746d18 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Offsets into SLCR regmap */
 
@@ -127,6 +128,69 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg:   Name of the configuration register.
+ * @offset:Register offset.
+ */
+struct zynq_configreg {
+   char *reg;
+   u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+   {.reg = "CRC",  .offset = 0},
+   {.reg = "FAR",  .offset = 1},
+   {.reg = "FDRI", .offset = 2},
+   {.reg = "FDRO", .offset = 3},
+   {.reg = "CMD",  .offset = 4},
+   {.reg = "CTRL0",.offset = 5},
+   {.reg = "MASK", .offset = 6},
+   {.reg = "STAT", .offset = 7},
+   {.reg = "LOUT", .offset = 8},
+   {.reg = "COR0", .offset = 9},
+   {.reg = "MFWR", .offset = 10},
+   {.reg = "CBC",  .offset = 11},
+   {.reg = "IDCODE",   .offset = 12},
+   {.reg = "AXSS", .offset = 13},
+   {.reg = "COR1", .offset = 14},
+   {.reg = "WBSTR",.offset = 16},
+   {.reg = "TIMER",.offset = 17},
+   {.reg = "BOOTSTS",  .offset = 22},
+   {.reg = "CTRL1",.offset = 24},
+   {}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK  0x
+#define RCFG_CMD_MASK  BIT(2)
+#define START_CMD_MASK BIT(2) + BIT(0)
+#define RCRC_CMD_MASK  GENMASK(2, 0)
+#define SHUTDOWN_CMD_MASK  GENMASK(1, 0) + BIT(3)
+#define DESYNC_WORD_MASK   GENMASK(2, 3) + BIT(0)
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK BIT(29)
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASKGENMASK(31, 0)
+
+#define TYPE_HDR_SHIFT 29
+#define TYPE_REG_SHIFT 13
+#define TYPE_OP_SHIFT  27
+#define TYPE_OPCODE_NOOP   0
+#define TYPE_OPCODE_READ   1
+#define TYPE_OPCODE_WRITE  2
+#define TYPE_FAR_OFFSET1
+#define TYPE_FDRO_OFFSET   3
+#define TYPE_CMD_OFFSET4
+
+#define READ_STEP5_NOOPS   6
+#define READ_STEP9_NOOPS   32
+
+#define READ_DMA_SIZE  0x200
+#define DUMMY_FRAMES_SIZE  0x28
+#define SLCR_PCAP_FREQ 1000
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +204,11 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct mutex ref_mutex;
+   struct dentry *dir;
+#endif
+   u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +233,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+   u32 status;
+   int ret;
+
+   ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+status & IXR_D_P_DONE_MASK,
+INIT_POLL_DELAY

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...


> > In Zynq Case it supports two types of the readback (Configuration registers,
> Configuration data(fpga image))  which may not be the same case for other
> vendors.
> > Since I need to support both the use cases I have differentiated them using
> module param in the zynq-fpga driver.
> >
> > As you said we shouldn't change the attribute's function, So in the
> > zynq-fpga driver, I am planning to add another debugfs attribute for
> reading back of configuration registers as it is vendor specific readback 
> type.
> >  (Configuration registers     Usage:
> /sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary)
> 
> Is it called '...summary' because it is not all the registers?  Could just be
> "cfg_regs"?

Sure will make it cfg_regs...
Are you ok with the above proposal?? 
if you are ok will make changes accordingly and will post v4...

> 
> > (Configuration data(fpga image)   Usage:
> /sys/kernel/debug/fpga/fpga0/image)
> > Please let me know your thoughts for the same...
> >
> >>
> >> > One other option is sysfs. Do you want me to implement sysfs path???
> >> > Any other suggestions???
> >>
> >> You could implement another sysfs attribute for reading FPGA registers
> >> with a separate ops callback.   Please clarify what it is doing (not
> >> all FPGAs have this function) and what that will look like when the
> >> user reads the from the sysfs
> >
> > AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific
> readback type in the FPGA manager ops.
> > Please correct me if my understanding is wrong.
> 
> You are not wrong :)

Thanks  

Regards,
Kedar.

> 
> >
> > Regards,
> > Kedar.
> >
> >>
> >> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...


> > In Zynq Case it supports two types of the readback (Configuration registers,
> Configuration data(fpga image))  which may not be the same case for other
> vendors.
> > Since I need to support both the use cases I have differentiated them using
> module param in the zynq-fpga driver.
> >
> > As you said we shouldn't change the attribute's function, So in the
> > zynq-fpga driver, I am planning to add another debugfs attribute for
> reading back of configuration registers as it is vendor specific readback 
> type.
> >  (Configuration registers     Usage:
> /sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary)
> 
> Is it called '...summary' because it is not all the registers?  Could just be
> "cfg_regs"?

Sure will make it cfg_regs...
Are you ok with the above proposal?? 
if you are ok will make changes accordingly and will post v4...

> 
> > (Configuration data(fpga image)   Usage:
> /sys/kernel/debug/fpga/fpga0/image)
> > Please let me know your thoughts for the same...
> >
> >>
> >> > One other option is sysfs. Do you want me to implement sysfs path???
> >> > Any other suggestions???
> >>
> >> You could implement another sysfs attribute for reading FPGA registers
> >> with a separate ops callback.   Please clarify what it is doing (not
> >> all FPGAs have this function) and what that will look like when the
> >> user reads the from the sysfs
> >
> > AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific
> readback type in the FPGA manager ops.
> > Please correct me if my understanding is wrong.
> 
> You are not wrong :)

Thanks  

Regards,
Kedar.

> 
> >
> > Regards,
> > Kedar.
> >
> >>
> >> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...

 
> Another minor thing.
> 
> > +
> >
> +/**
> **
> > +/
> 
> Let's keep the coding style consistent by not having

Sure will fix in v4...

> '***'
> 
> > +/**
> > + *
> 
> Also, you don't need /** followed by '*', just take out the essentially blank
> line here, please.

Sure will fix in v4...

Regards,
Kedar.

> 
> > + * Generates a Type 2 packet header that reads back the requested
> > +Configuration
> > + * register.
> > + *
> > + * @paramOpCode is the read/write operation code.
> > + * @paramSize is the size of the word to be read.
> > + *
> > + * @return   Type 2 packet header to read the specified register
> > + *
> > + * @note None.
> > + *
> > +
> >
> +***
> **
> > +/ static int zynq_type2_pkt(u8 OpCode, u32 Size) {
> 
> Thanks,
> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...

 
> Another minor thing.
> 
> > +
> >
> +/**
> **
> > +/
> 
> Let's keep the coding style consistent by not having

Sure will fix in v4...

> '***'
> 
> > +/**
> > + *
> 
> Also, you don't need /** followed by '*', just take out the essentially blank
> line here, please.

Sure will fix in v4...

Regards,
Kedar.

> 
> > + * Generates a Type 2 packet header that reads back the requested
> > +Configuration
> > + * register.
> > + *
> > + * @paramOpCode is the read/write operation code.
> > + * @paramSize is the size of the word to be read.
> > + *
> > + * @return   Type 2 packet header to read the specified register
> > + *
> > + * @note None.
> > + *
> > +
> >
> +***
> **
> > +/ static int zynq_type2_pkt(u8 OpCode, u32 Size) {
> 
> Thanks,
> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...

 
> >
> > 
> >> > +static bool readback_type;
> >> > +module_param(readback_type, bool, 0644);
> >> > +MODULE_PARM_DESC(readback_type,
> >> > +   "readback_type 0-configuration register read "
> >> > +   "1- configuration data read (default: 0)");
> >>
> >> Not sure a module param is the best solution here.
> >
> > Need to provide flexibility to the user to switch between reading of FPGA
> registers and configuration data.
> 
> I suggested calling the attribute 'image' because I thought it would always be
> a read back of the FPGA image information.  I don't think that a sysfs
> attribute's function should change. :)

In Zynq Case it supports two types of the readback (Configuration registers, 
Configuration data(fpga image))  which may not be the same case for other 
vendors. 
Since I need to support both the use cases I have differentiated them using 
module param in the zynq-fpga driver.

As you said we shouldn't change the attribute's function, So in the zynq-fpga 
driver,
I am planning to add another debugfs attribute for reading back of 
configuration registers as it is vendor specific readback type.
 (Configuration registers     Usage:  
/sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary) 
(Configuration data(fpga image)   Usage: 
/sys/kernel/debug/fpga/fpga0/image)
Please let me know your thoughts for the same... 

> 
> > One other option is sysfs. Do you want me to implement sysfs path???
> > Any other suggestions???
> 
> You could implement another sysfs attribute for reading FPGA registers
> with a separate ops callback.   Please clarify what it is doing (not
> all FPGAs have this function) and what that will look like when the user reads
> the from the sysfs

AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific 
readback type in the FPGA manager ops.
Please correct me if my understanding is wrong. 

Regards,
Kedar.

> 
> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review...

 
> >
> > 
> >> > +static bool readback_type;
> >> > +module_param(readback_type, bool, 0644);
> >> > +MODULE_PARM_DESC(readback_type,
> >> > +   "readback_type 0-configuration register read "
> >> > +   "1- configuration data read (default: 0)");
> >>
> >> Not sure a module param is the best solution here.
> >
> > Need to provide flexibility to the user to switch between reading of FPGA
> registers and configuration data.
> 
> I suggested calling the attribute 'image' because I thought it would always be
> a read back of the FPGA image information.  I don't think that a sysfs
> attribute's function should change. :)

In Zynq Case it supports two types of the readback (Configuration registers, 
Configuration data(fpga image))  which may not be the same case for other 
vendors. 
Since I need to support both the use cases I have differentiated them using 
module param in the zynq-fpga driver.

As you said we shouldn't change the attribute's function, So in the zynq-fpga 
driver,
I am planning to add another debugfs attribute for reading back of 
configuration registers as it is vendor specific readback type.
 (Configuration registers     Usage:  
/sys/kernel/debug/fpga/zynq-fpga/cfgreg_summary) 
(Configuration data(fpga image)   Usage: 
/sys/kernel/debug/fpga/fpga0/image)
Please let me know your thoughts for the same... 

> 
> > One other option is sysfs. Do you want me to implement sysfs path???
> > Any other suggestions???
> 
> You could implement another sysfs attribute for reading FPGA registers
> with a separate ops callback.   Please clarify what it is doing (not
> all FPGAs have this function) and what that will look like when the user reads
> the from the sysfs

AFAIK we shouldn't add another sysfs/debugfs attribute for vendor-specific 
readback type in the FPGA manager ops.
Please correct me if my understanding is wrong. 

Regards,
Kedar.

> 
> Alan

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Moritz,

Thanks for the review...

 
> Can you please make the commit message such that you have full sentences?
> 
> "Add support for readback of FPGA configuration data and registers" of 
> example.

Sure will fix in v4.

> 
> >
> > Usage:
> > Readback of PL configuration registers
> > cat /sys/kernel/debug/fpga/fpga0/image
> > Readback of PL configuration data
> > echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
> 
> The patch below is for Zynq if I'm not mistaken, not ZynqMP?

Yes it is for Zynq not ZynqMP by mistake I have kept zynqmp here, thanks for 
pointing  will fix in v4...

 
> > +static bool readback_type;
> > +module_param(readback_type, bool, 0644); 
> > +MODULE_PARM_DESC(readback_type,
> > +   "readback_type 0-configuration register read "
> > +   "1- configuration data read (default: 0)");
> 
> Not sure a module param is the best solution here.

Need to provide flexibility to the user to switch between reading of FPGA 
registers and configuration data.
One other option is sysfs. Do you want me to implement sysfs path??? 
Any other suggestions??? 

 
> > +/* Masks for Configuration registers */
> > +#define FAR_ADDR_MASK  0x
> > +#define RCFG_CMD_MASK  0x0004
> > +#define START_CMD_MASK 0x0005
> > +#define RCRC_CMD_MASK  0x0007
> 
> BIT() and GENMASK() would work here.

Sure will fix in v4... 

> 
> > +#define SHUTDOWN_CMD_MASK  0x000B
> > +#define DESYNC_WORD_MASK   0x000D
> > +#define BUSWIDTH_SYNCWORD_MASK 0x00BB
> > +#define NOOP_WORD_MASK 0x2000
> > +#define BUSWIDTH_DETECT_MASK   0x11220044
> > +#define SYNC_WORD_MASK 0xAA995566
> > +#define DUMMY_WORD_MASK0x
> 
> GENMASK() would probably work for most of the above ones

Sure will use GENMAK wherever applicable will fix in v4.

 
> > +* Generating the Type 1 packet header which involves 
> > +sifting of Type1
> Shifting? Maybe you can just reference the section that documents this 
> in the TRM?

Sure will fix in v4... 


> **
> > +/ static int zynq_type2_pkt(u8 OpCode, u32 Size)
> 
> No camel case please, can you make Size and OpCode small caps please?
> Also please make the two functions consistent.

Sure will fix in v4...

> > +{
> > +   /*
> > +* Type 2 Packet Header Format
> > +* The header section is always a 32-bit word.
> > +*
> > +* HeaderType | Opcode |  Word Count
> > +* [31:29]  [28:27] [26:0]
> > +* --
> > +*   010  xx  x
> > +*
> > +* @R: means the bit is not used and reserved for future use.
> > +* The reserved bits should be written as 0s.
> > +*
> > +* Generating the Type 2 packet header which involves 
> > +sifting of Type 2
> 
> s/sifting/shifting/

Sure will fix in v4...

> > +* Header Mask, OpCode and then performing OR
> > +* operation with the Word Length.
> > +*/
> > +   return (((2 << TYPE_HDR_SHIFT) |
> > +   (OpCode << TYPE_OP_SHIFT)) | Size); }
> > +
> > +static int zynq_fpga_read_cfgdata(struct fpga_manager *mgr,
> > + struct seq_file *s) {
> > +   struct zynq_fpga_priv *priv;
> > +   int ret = 0, i, cmdindex, clk_rate;
> > +   unsigned int *buf;
> > +   dma_addr_t dma_addr;
> > +   u32 intr_status, status;
> > +   size_t size;
> 
> Reverse xmas tree please

Ok sure will v4... 

> > +
> > +   priv = mgr->priv;
> > +   size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
> > +   buf = dma_zalloc_coherent(mgr->dev.parent, size,
> > +_addr, GFP_KERNEL);
> > +   if (!buf)
> > +   return -ENOMEM;
> > +
> > +   seq_puts(s, "zynq FPGA Configuration data contents are\n");
> > +
> > +   /*
> > +* There is no h/w flow control for pcap read
> > +* to prevent the FIFO from over flowing, reduce
> > +* the PCAP operating frequency.
> > +*/
> > +   clk_rate = clk_get_rate(priv->clk);
> > +   ret = clk_set_rate(priv->clk, SLCR_PCAP_FREQ);
> > +   if (ret) {
> > +   dev_err(>dev, "Unable to reduce the PCAP freq\n");
> > +   goto free_dmabuf;
> > +   }
> > +
> > +   ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
> > +status & STATUS_PCFG_INIT_MASK,
> > +INIT_POLL_DELAY,
> > +INIT_POLL_TIMEOUT);
> > +   if (ret) {
> > +   dev_err(>dev, "Timeout waiting for PCFG_INIT\n");
> > +   goto restore_pcap_clk;
> > +   }
> > +
> > +   cmdindex = 0;
> > +   buf[cmdindex++] = DUMMY_WORD_MASK;
> > +   buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
> > +   

RE: [PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara Rao]

Hi Moritz,

Thanks for the review...

 
> Can you please make the commit message such that you have full sentences?
> 
> "Add support for readback of FPGA configuration data and registers" of 
> example.

Sure will fix in v4.

> 
> >
> > Usage:
> > Readback of PL configuration registers
> > cat /sys/kernel/debug/fpga/fpga0/image
> > Readback of PL configuration data
> > echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
> 
> The patch below is for Zynq if I'm not mistaken, not ZynqMP?

Yes it is for Zynq not ZynqMP by mistake I have kept zynqmp here, thanks for 
pointing  will fix in v4...

 
> > +static bool readback_type;
> > +module_param(readback_type, bool, 0644); 
> > +MODULE_PARM_DESC(readback_type,
> > +   "readback_type 0-configuration register read "
> > +   "1- configuration data read (default: 0)");
> 
> Not sure a module param is the best solution here.

Need to provide flexibility to the user to switch between reading of FPGA 
registers and configuration data.
One other option is sysfs. Do you want me to implement sysfs path??? 
Any other suggestions??? 

 
> > +/* Masks for Configuration registers */
> > +#define FAR_ADDR_MASK  0x
> > +#define RCFG_CMD_MASK  0x0004
> > +#define START_CMD_MASK 0x0005
> > +#define RCRC_CMD_MASK  0x0007
> 
> BIT() and GENMASK() would work here.

Sure will fix in v4... 

> 
> > +#define SHUTDOWN_CMD_MASK  0x000B
> > +#define DESYNC_WORD_MASK   0x000D
> > +#define BUSWIDTH_SYNCWORD_MASK 0x00BB
> > +#define NOOP_WORD_MASK 0x2000
> > +#define BUSWIDTH_DETECT_MASK   0x11220044
> > +#define SYNC_WORD_MASK 0xAA995566
> > +#define DUMMY_WORD_MASK0x
> 
> GENMASK() would probably work for most of the above ones

Sure will use GENMAK wherever applicable will fix in v4.

 
> > +* Generating the Type 1 packet header which involves 
> > +sifting of Type1
> Shifting? Maybe you can just reference the section that documents this 
> in the TRM?

Sure will fix in v4... 


> **
> > +/ static int zynq_type2_pkt(u8 OpCode, u32 Size)
> 
> No camel case please, can you make Size and OpCode small caps please?
> Also please make the two functions consistent.

Sure will fix in v4...

> > +{
> > +   /*
> > +* Type 2 Packet Header Format
> > +* The header section is always a 32-bit word.
> > +*
> > +* HeaderType | Opcode |  Word Count
> > +* [31:29]  [28:27] [26:0]
> > +* --
> > +*   010  xx  x
> > +*
> > +* @R: means the bit is not used and reserved for future use.
> > +* The reserved bits should be written as 0s.
> > +*
> > +* Generating the Type 2 packet header which involves 
> > +sifting of Type 2
> 
> s/sifting/shifting/

Sure will fix in v4...

> > +* Header Mask, OpCode and then performing OR
> > +* operation with the Word Length.
> > +*/
> > +   return (((2 << TYPE_HDR_SHIFT) |
> > +   (OpCode << TYPE_OP_SHIFT)) | Size); }
> > +
> > +static int zynq_fpga_read_cfgdata(struct fpga_manager *mgr,
> > + struct seq_file *s) {
> > +   struct zynq_fpga_priv *priv;
> > +   int ret = 0, i, cmdindex, clk_rate;
> > +   unsigned int *buf;
> > +   dma_addr_t dma_addr;
> > +   u32 intr_status, status;
> > +   size_t size;
> 
> Reverse xmas tree please

Ok sure will v4... 

> > +
> > +   priv = mgr->priv;
> > +   size = priv->size + READ_DMA_SIZE + DUMMY_FRAMES_SIZE;
> > +   buf = dma_zalloc_coherent(mgr->dev.parent, size,
> > +_addr, GFP_KERNEL);
> > +   if (!buf)
> > +   return -ENOMEM;
> > +
> > +   seq_puts(s, "zynq FPGA Configuration data contents are\n");
> > +
> > +   /*
> > +* There is no h/w flow control for pcap read
> > +* to prevent the FIFO from over flowing, reduce
> > +* the PCAP operating frequency.
> > +*/
> > +   clk_rate = clk_get_rate(priv->clk);
> > +   ret = clk_set_rate(priv->clk, SLCR_PCAP_FREQ);
> > +   if (ret) {
> > +   dev_err(>dev, "Unable to reduce the PCAP freq\n");
> > +   goto free_dmabuf;
> > +   }
> > +
> > +   ret = zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status,
> > +status & STATUS_PCFG_INIT_MASK,
> > +INIT_POLL_DELAY,
> > +INIT_POLL_TIMEOUT);
> > +   if (ret) {
> > +   dev_err(>dev, "Timeout waiting for PCFG_INIT\n");
> > +   goto restore_pcap_clk;
> > +   }
> > +
> > +   cmdindex = 0;
> > +   buf[cmdindex++] = DUMMY_WORD_MASK;
> > +   buf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
> > +   

[PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara rao]

This patch does the below
--> Adds support for readback of pl configuration data
--> Adds support for readback of pl configuration registers

Usage:
Readback of PL configuration registers
cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration data
echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 400 +++
 1 file changed, 400 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..5f1a1aa 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Offsets into SLCR regmap */
 
@@ -127,6 +128,72 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+static bool readback_type;
+module_param(readback_type, bool, 0644);
+MODULE_PARM_DESC(readback_type,
+   "readback_type 0-configuration register read "
+   "1- configuration data read (default: 0)");
+
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg:   Name of the configuration register.
+ * @offset:Register offset.
+ */
+struct zynq_configreg {
+   char *reg;
+   u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+   {.reg = "CRC",  .offset = 0},
+   {.reg = "FAR",  .offset = 1},
+   {.reg = "FDRI", .offset = 2},
+   {.reg = "FDRO", .offset = 3},
+   {.reg = "CMD",  .offset = 4},
+   {.reg = "CTRL0",.offset = 5},
+   {.reg = "MASK", .offset = 6},
+   {.reg = "STAT", .offset = 7},
+   {.reg = "LOUT", .offset = 8},
+   {.reg = "COR0", .offset = 9},
+   {.reg = "MFWR", .offset = 10},
+   {.reg = "CBC",  .offset = 11},
+   {.reg = "IDCODE",   .offset = 12},
+   {.reg = "AXSS", .offset = 13},
+   {.reg = "COR1", .offset = 14},
+   {.reg = "WBSTR",.offset = 16},
+   {.reg = "TIMER",.offset = 17},
+   {.reg = "BOOTSTS",  .offset = 22},
+   {.reg = "CTRL1",.offset = 24},
+   {}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK  0x
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE_HDR_SHIFT 29
+#define TYPE_REG_SHIFT 13
+#define TYPE_OP_SHIFT  27
+#define TYPE_OPCODE_NOOP   0
+#define TYPE_OPCODE_READ   1
+#define TYPE_OPCODE_WRITE  2
+#define TYPE_FAR_OFFSET1
+#define TYPE_FDRO_OFFSET   3
+#define TYPE_CMD_OFFSET4
+
+#define READ_DMA_SIZE  0x200
+#define DUMMY_FRAMES_SIZE  0x28
+#define SLCR_PCAP_FREQ 1000
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +207,7 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+   u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +232,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+   u32 status;
+   int ret;
+
+   ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+status & IXR_D_P_DONE_MASK,
+INIT_POLL_DELAY,
+INIT_POLL_TIMEOUT);
+   return ret;
+}
+
 /* Must be called with dma_lock held

[PATCH v3 2/2] fpga: zynq-fpga: Add support for readback

[Appana Durga Kedareswara rao]

This patch does the below
--> Adds support for readback of pl configuration data
--> Adds support for readback of pl configuration registers

Usage:
Readback of PL configuration registers
cat /sys/kernel/debug/fpga/fpga0/image
Readback of PL configuration data
echo 1 > /sys/module/zynqmp_fpga/parameters/readback_type
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v3:
--> Added support for pl configuration data readback
--> Improved the pl configuration register readback logic.
Changes for v2:
--> Removed locks from the read_ops as lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 400 +++
 1 file changed, 400 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..5f1a1aa 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /* Offsets into SLCR regmap */
 
@@ -127,6 +128,72 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+static bool readback_type;
+module_param(readback_type, bool, 0644);
+MODULE_PARM_DESC(readback_type,
+   "readback_type 0-configuration register read "
+   "1- configuration data read (default: 0)");
+
+/**
+ * struct zynq_configreg - Configuration register offsets
+ * @reg:   Name of the configuration register.
+ * @offset:Register offset.
+ */
+struct zynq_configreg {
+   char *reg;
+   u32 offset;
+};
+
+static struct zynq_configreg cfgreg[] = {
+   {.reg = "CRC",  .offset = 0},
+   {.reg = "FAR",  .offset = 1},
+   {.reg = "FDRI", .offset = 2},
+   {.reg = "FDRO", .offset = 3},
+   {.reg = "CMD",  .offset = 4},
+   {.reg = "CTRL0",.offset = 5},
+   {.reg = "MASK", .offset = 6},
+   {.reg = "STAT", .offset = 7},
+   {.reg = "LOUT", .offset = 8},
+   {.reg = "COR0", .offset = 9},
+   {.reg = "MFWR", .offset = 10},
+   {.reg = "CBC",  .offset = 11},
+   {.reg = "IDCODE",   .offset = 12},
+   {.reg = "AXSS", .offset = 13},
+   {.reg = "COR1", .offset = 14},
+   {.reg = "WBSTR",.offset = 16},
+   {.reg = "TIMER",.offset = 17},
+   {.reg = "BOOTSTS",  .offset = 22},
+   {.reg = "CTRL1",.offset = 24},
+   {}
+};
+
+/* Masks for Configuration registers */
+#define FAR_ADDR_MASK  0x
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE_HDR_SHIFT 29
+#define TYPE_REG_SHIFT 13
+#define TYPE_OP_SHIFT  27
+#define TYPE_OPCODE_NOOP   0
+#define TYPE_OPCODE_READ   1
+#define TYPE_OPCODE_WRITE  2
+#define TYPE_FAR_OFFSET1
+#define TYPE_FDRO_OFFSET   3
+#define TYPE_CMD_OFFSET4
+
+#define READ_DMA_SIZE  0x200
+#define DUMMY_FRAMES_SIZE  0x28
+#define SLCR_PCAP_FREQ 1000
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +207,7 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+   u32 size;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +232,27 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
+static int zynq_fpga_wait_fordone(struct zynq_fpga_priv *priv)
+{
+   u32 status;
+   int ret;
+
+   ret = zynq_fpga_poll_timeout(priv, INT_STS_OFFSET, status,
+status & IXR_D_P_DONE_MASK,
+INIT_POLL_DELAY,
+INIT_POLL_TIMEOUT);
+   return ret;
+}
+
 /* Must be called with dma_lock held

[PATCH v3 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,

[PATCH v3 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v3:
--> None.
Changes for v2:
--> Fixed debug attribute path and name as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,

[RFC PATCH v2 2/2] fpga: zynq-fpga: Add support for readback of configuration registers

[Appana Durga Kedareswara rao]

This patch adds support for Read-back of
configuration registers in zynq.

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v2:
--> Removed locks from the read ops as
lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 245 +++
 1 file changed, 245 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..b56a6c6 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -127,6 +127,49 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/* Configuration Registers */
+#define TYPE1_CRC_OFFSET   0x0
+#define TYPE1_FAR_OFFSET   0x1
+#define TYPE1_FDRI_OFFSET  0x2
+#define TYPE1_FDRO_OFFSET  0x3
+#define TYPE1_CMD_OFFSET   0x4
+#define TYPE1_CTRL0_OFFSET 0x5
+#define TYPE1_MASK_OFFSET  0x6
+#define TYPE1_STAT_OFFSET  0x7
+#define TYPE1_LOUT_OFFSET  0x8
+#define TYPE1_COR0_OFFSET  0x9
+#define TYPE1_MFWR_OFFSET  0xa
+#define TYPE1_CBC_OFFSET   0xb
+#define TYPE1_IDCODE_OFFSET0xc
+#define TYPE1_AXSS_OFFSET  0xd
+#define TYPE1_COR1_OFFSET  0xe
+#define TYPE1_WBSTR_OFFSET 0x10
+#define TYPE1_TIMER_OFFSET 0x11
+#define TYPE1_BOOTSTS_OFFSET   0x16
+#define TYPE1_CTRL1_OFFSET 0x18
+#define TYPE1_BSPI_OFFSET  0x1f
+
+/* Masks for Configuration registers */
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE1_HDR_SHIFT29
+#define TYPE1_REG_SHIFT13
+#define TYPE1_OP_SHIFT 27
+#define TYPE1_OPCODE_NOOP  0
+#define TYPE1_OPCODE_READ  1
+#define TYPE1_OPCODE_WRITE 2
+
+#define CFGREG_SRCDMA_SIZE 0x40
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -164,6 +207,15 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
 /* Must be called with dma_lock held */
 static void zynq_step_dma(struct zynq_fpga_priv *priv)
 {
@@ -546,12 +598,205 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct 
fpga_manager *mgr)
return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+   /*
+* Type 1 Packet Header Format
+* The header section is always a 32-bit word.
+*
+* HeaderType | Opcode | Register Address | Reserved | Word Count
+* [31:29]  [28:27] [26:13]  [12:11] [10:0]
+* --
+*   001  xx  RxRR  xxx
+*
+* @R: means the bit is not used and reserved for future use.
+* The reserved bits should be written as 0s.
+*
+* Generating the Type 1 packet header which involves sifting of Type1
+* Header Mask, Register value and the OpCode which is 01 in this case
+* as only read operation is to be carried out and then performing OR
+* operation with the Word Length.
+*/
+   return (((1 << TYPE1_HDR_SHIFT) |
+   (reg << TYPE1_REG_SHIFT) |
+   (opcode << TYPE1_OP_SHIFT)) | size);
+
+}
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg)
+{
+   struct zynq_fpga_priv *priv;
+   int ret = 0, cmdindex, src_offset;
+   int *srcbuf, *dstbuf;
+   dma_addr_t src_dma_addr, dst_dma_addr;
+   u32 status, intr_status;
+
+   priv = mgr->priv;
+
+   srcbuf = dma_alloc_coherent(mgr->dev.parent, CFGREG_SRCDMA_SIZE,
+   _dma_addr, GFP_KERNEL);
+   if (!srcbuf)
+   return -ENOMEM;
+
+   dstbuf = dma_alloc_coherent(mgr->dev.parent, sizeof(dstbuf),
+   _dma_addr, GFP_KERNEL);
+   if (!dstbuf)
+   goto free_srcbuf;
+
+   cmdindex = 0;
+   srcbuf[cmdindex++] = DUMMY_WORD_MASK;
+   srcbuf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+   srcbuf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+   srcbuf[cmdindex++] = DUMMY_WORD_MASK;
+   srcbuf[cmdindex++] = SYNC_WORD_MASK;
+   srcbuf[cmdindex++] = 

[RFC PATCH v2 2/2] fpga: zynq-fpga: Add support for readback of configuration registers

[Appana Durga Kedareswara rao]

This patch adds support for Read-back of
configuration registers in zynq.

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v2:
--> Removed locks from the read ops as
lock handling is done in the framework.

 drivers/fpga/zynq-fpga.c | 245 +++
 1 file changed, 245 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..b56a6c6 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -127,6 +127,49 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/* Configuration Registers */
+#define TYPE1_CRC_OFFSET   0x0
+#define TYPE1_FAR_OFFSET   0x1
+#define TYPE1_FDRI_OFFSET  0x2
+#define TYPE1_FDRO_OFFSET  0x3
+#define TYPE1_CMD_OFFSET   0x4
+#define TYPE1_CTRL0_OFFSET 0x5
+#define TYPE1_MASK_OFFSET  0x6
+#define TYPE1_STAT_OFFSET  0x7
+#define TYPE1_LOUT_OFFSET  0x8
+#define TYPE1_COR0_OFFSET  0x9
+#define TYPE1_MFWR_OFFSET  0xa
+#define TYPE1_CBC_OFFSET   0xb
+#define TYPE1_IDCODE_OFFSET0xc
+#define TYPE1_AXSS_OFFSET  0xd
+#define TYPE1_COR1_OFFSET  0xe
+#define TYPE1_WBSTR_OFFSET 0x10
+#define TYPE1_TIMER_OFFSET 0x11
+#define TYPE1_BOOTSTS_OFFSET   0x16
+#define TYPE1_CTRL1_OFFSET 0x18
+#define TYPE1_BSPI_OFFSET  0x1f
+
+/* Masks for Configuration registers */
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE1_HDR_SHIFT29
+#define TYPE1_REG_SHIFT13
+#define TYPE1_OP_SHIFT 27
+#define TYPE1_OPCODE_NOOP  0
+#define TYPE1_OPCODE_READ  1
+#define TYPE1_OPCODE_WRITE 2
+
+#define CFGREG_SRCDMA_SIZE 0x40
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -164,6 +207,15 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
 /* Must be called with dma_lock held */
 static void zynq_step_dma(struct zynq_fpga_priv *priv)
 {
@@ -546,12 +598,205 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct 
fpga_manager *mgr)
return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+   /*
+* Type 1 Packet Header Format
+* The header section is always a 32-bit word.
+*
+* HeaderType | Opcode | Register Address | Reserved | Word Count
+* [31:29]  [28:27] [26:13]  [12:11] [10:0]
+* --
+*   001  xx  RxRR  xxx
+*
+* @R: means the bit is not used and reserved for future use.
+* The reserved bits should be written as 0s.
+*
+* Generating the Type 1 packet header which involves sifting of Type1
+* Header Mask, Register value and the OpCode which is 01 in this case
+* as only read operation is to be carried out and then performing OR
+* operation with the Word Length.
+*/
+   return (((1 << TYPE1_HDR_SHIFT) |
+   (reg << TYPE1_REG_SHIFT) |
+   (opcode << TYPE1_OP_SHIFT)) | size);
+
+}
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg)
+{
+   struct zynq_fpga_priv *priv;
+   int ret = 0, cmdindex, src_offset;
+   int *srcbuf, *dstbuf;
+   dma_addr_t src_dma_addr, dst_dma_addr;
+   u32 status, intr_status;
+
+   priv = mgr->priv;
+
+   srcbuf = dma_alloc_coherent(mgr->dev.parent, CFGREG_SRCDMA_SIZE,
+   _dma_addr, GFP_KERNEL);
+   if (!srcbuf)
+   return -ENOMEM;
+
+   dstbuf = dma_alloc_coherent(mgr->dev.parent, sizeof(dstbuf),
+   _dma_addr, GFP_KERNEL);
+   if (!dstbuf)
+   goto free_srcbuf;
+
+   cmdindex = 0;
+   srcbuf[cmdindex++] = DUMMY_WORD_MASK;
+   srcbuf[cmdindex++] = BUSWIDTH_SYNCWORD_MASK;
+   srcbuf[cmdindex++] = BUSWIDTH_DETECT_MASK;
+   srcbuf[cmdindex++] = DUMMY_WORD_MASK;
+   srcbuf[cmdindex++] = SYNC_WORD_MASK;
+   srcbuf[cmdindex++] = 

[RFC PATCH v2 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v2:
--> Fixed debug attribute path and name
as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,6 +153,9 @@ struct fpga_manager {
enum fpg

[RFC PATCH v2 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/fpga0/image

Signed-off-by: Appana Durga Kedareswara rao 
---
Changes for v2:
--> Fixed debug attribute path and name
as suggested by Alan
--> Add config entry for DEBUG as suggested by Alan
--> Fixed trival coding style issues.

 drivers/fpga/Kconfig  |  7 +
 drivers/fpga/fpga-mgr.c   | 68 +++
 include/linux/fpga/fpga-mgr.h |  5 
 3 files changed, 80 insertions(+)

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 53d3f55..838ad4e 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -11,6 +11,13 @@ menuconfig FPGA
 
 if FPGA
 
+config FPGA_MGR_DEBUG_FS
+   tristate "FPGA Debug fs"
+   select DEBUG_FS
+   help
+ FPGA manager debug provides support for reading fpga configuration
+ information.
+
 config FPGA_MGR_SOCFPGA
tristate "Altera SOCFPGA FPGA Manager"
depends on ARCH_SOCFPGA || COMPILE_TEST
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..4bea860 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,48 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (!mgr->mops->read)
+   return -ENOENT;
+
+   if (!mutex_trylock(>ref_mutex))
+   return -EBUSY;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING) {
+   ret = -EPERM;
+   goto err_unlock;
+   }
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret)
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+
+err_unlock:
+   mutex_unlock(>ref_mutex);
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_ops_image = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +623,29 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   struct dentry *d, *parent;
+
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   parent = mgr->dir;
+   d = debugfs_create_dir(mgr->dev.kobj.name, parent);
+   if (!d) {
+   debugfs_remove_recursive(parent);
+   goto error_device;
+   }
+
+   parent = d;
+   d = debugfs_create_file("image", 0644, parent, mgr,
+   _mgr_ops_image);
+   if (!d) {
+   debugfs_remove_recursive(mgr->dir);
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +669,9 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_FPGA_MGR_DEBUG_FS
+   debugfs_remove_recursive(mgr->dir);
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..e9e17a9 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: optional: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,6 +153,9 @@ struct fpga_manager {
enum fpg

RE: [RFC PATCH 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review... 
Please find comments inline... 


> 
> Hi Appana,
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/readback
> 
> Two things here: I'd prefer calling the attribute "image" rather than
> "readback"
> 
> This should be an entry per fpga manager, not one entry for the whole
> framework, so
> 
> cat /sys/kernel/debug/fpga/fpga0/image

Sure will fix in v2... 

> 
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> >  drivers/fpga/fpga-mgr.c   | 52
> +++
> >  include/linux/fpga/fpga-mgr.h |  6 +
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..7a9fd7c 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,39 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +#include 
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +   int ret = 0;
> 
> Here you should return an error for mgr's that don't support read function:
> 
> if (!mgr->mops->read)
> return -ENOENT;

Will fix in v2...

> 
> Then you probably should lock the mgr so that nobody tries to write it while
> you are reading.  If you can't lock it, return -EBUSY.

Ok sure will fix in v2... 

> 
> > +
> > +   if (mgr->state != FPGA_MGR_STATE_OPERATING)
> > +   return -EBUSY;
> 
> If the FPGA isn't programmed, I'm not sure that EBUSY would be the correct
> error.

Ok, Should I return EPERM (or) EAGAIN here??? 

> 
> > +
> > +   /* Read the FPGA configuration data from the fabric */
> > +   ret = mgr->mops->read(mgr, s);
> > +   if (ret) {
> > +   dev_err(>dev, "Error while reading configuration data 
> > from
> FPGA\n");
> > +   return ret;
> 
> Don't need this return here since we return right afterwards anyway.

Will fix in v2...

> 
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +   return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_debugops = {
> 
> Suggest including the name of the debugfs file in the name since more
> debugfs files will be added over time, so
> 
> s/fpga_mgr_debugops/fpga_mgr_ops_image/ or something.

Sure will use fpga_mgr_ops_image 

> 
> > +   .owner = THIS_MODULE,
> > +   .open = fpga_mgr_read_open,
> > +   .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:   fpga manager
> > @@ -581,6 +614,21 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> > if (ret)
> > goto error_device;
> >
> > +#ifdef CONFIG_DEBUG_FS
> 
> I'd prefer an added config such as CONFIG_FPGA_MGR_DEBUG_FS.

Ok, will add an entry in the Kconfig with the name as you suggested in v2...  

> 
> > +   mgr->dir = debugfs_create_dir("fpga", NULL);
> > +   if (!mgr->dir)
> > +   goto error_device;
> > +
> > +   mgr->parent = mgr->dir;
> > +   mgr->dir = debugfs_create_file("readback", 0644, mgr->parent, mgr,
> > +  _mgr_debugops);
> > +   if (!mgr->dir) {
> > +   debugfs_remove_recursive(mgr->parent);
> > +   mgr->parent = NULL;
> > +   goto error_device;
> > +   }
> > +#endif
> > +
> > dev_info(>dev, "%s registered\n", mgr->name);
> >
> > return 0;
> > @@ -604,6 +652,10 @@ void fpga_mgr_unregister(struct device *dev)
> >
> > dev_info(>dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +   debugfs_remove_recursive(mgr->parent);
> > +   mgr->parent = NULL;
> > +#endif
> > /*
>

RE: [RFC PATCH 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara Rao]

Hi Alan,

Thanks for the review... 
Please find comments inline... 


> 
> Hi Appana,
> 
> > Inorder to debug issues with fpga's users would like to read the fpga
> > configuration information.
> > This patch adds readback support for fpga configuration data in the
> > framework through debugfs interface.
> >
> > Usage:
> > cat /sys/kernel/debug/fpga/readback
> 
> Two things here: I'd prefer calling the attribute "image" rather than
> "readback"
> 
> This should be an entry per fpga manager, not one entry for the whole
> framework, so
> 
> cat /sys/kernel/debug/fpga/fpga0/image

Sure will fix in v2... 

> 
> >
> > Signed-off-by: Appana Durga Kedareswara rao
> > 
> > ---
> >  drivers/fpga/fpga-mgr.c   | 52
> +++
> >  include/linux/fpga/fpga-mgr.h |  6 +
> >  2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > 9939d2c..7a9fd7c 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -484,6 +484,39 @@ void fpga_mgr_put(struct fpga_manager *mgr)  }
> > EXPORT_SYMBOL_GPL(fpga_mgr_put);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +#include 
> > +
> > +static int fpga_mgr_read(struct seq_file *s, void *data) {
> > +   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
> > +   int ret = 0;
> 
> Here you should return an error for mgr's that don't support read function:
> 
> if (!mgr->mops->read)
> return -ENOENT;

Will fix in v2...

> 
> Then you probably should lock the mgr so that nobody tries to write it while
> you are reading.  If you can't lock it, return -EBUSY.

Ok sure will fix in v2... 

> 
> > +
> > +   if (mgr->state != FPGA_MGR_STATE_OPERATING)
> > +   return -EBUSY;
> 
> If the FPGA isn't programmed, I'm not sure that EBUSY would be the correct
> error.

Ok, Should I return EPERM (or) EAGAIN here??? 

> 
> > +
> > +   /* Read the FPGA configuration data from the fabric */
> > +   ret = mgr->mops->read(mgr, s);
> > +   if (ret) {
> > +   dev_err(>dev, "Error while reading configuration data 
> > from
> FPGA\n");
> > +   return ret;
> 
> Don't need this return here since we return right afterwards anyway.

Will fix in v2...

> 
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int fpga_mgr_read_open(struct inode *inode, struct file *file)
> > +{
> > +   return single_open(file, fpga_mgr_read, inode->i_private); }
> > +
> > +static const struct file_operations fpga_mgr_debugops = {
> 
> Suggest including the name of the debugfs file in the name since more
> debugfs files will be added over time, so
> 
> s/fpga_mgr_debugops/fpga_mgr_ops_image/ or something.

Sure will use fpga_mgr_ops_image 

> 
> > +   .owner = THIS_MODULE,
> > +   .open = fpga_mgr_read_open,
> > +   .read = seq_read,
> > +};
> > +#endif
> > +
> >  /**
> >   * fpga_mgr_lock - Lock FPGA manager for exclusive use
> >   * @mgr:   fpga manager
> > @@ -581,6 +614,21 @@ int fpga_mgr_register(struct device *dev, const
> char *name,
> > if (ret)
> > goto error_device;
> >
> > +#ifdef CONFIG_DEBUG_FS
> 
> I'd prefer an added config such as CONFIG_FPGA_MGR_DEBUG_FS.

Ok, will add an entry in the Kconfig with the name as you suggested in v2...  

> 
> > +   mgr->dir = debugfs_create_dir("fpga", NULL);
> > +   if (!mgr->dir)
> > +   goto error_device;
> > +
> > +   mgr->parent = mgr->dir;
> > +   mgr->dir = debugfs_create_file("readback", 0644, mgr->parent, mgr,
> > +  _mgr_debugops);
> > +   if (!mgr->dir) {
> > +   debugfs_remove_recursive(mgr->parent);
> > +   mgr->parent = NULL;
> > +   goto error_device;
> > +   }
> > +#endif
> > +
> > dev_info(>dev, "%s registered\n", mgr->name);
> >
> > return 0;
> > @@ -604,6 +652,10 @@ void fpga_mgr_unregister(struct device *dev)
> >
> > dev_info(>dev, "%s %s\n", __func__, mgr->name);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +   debugfs_remove_recursive(mgr->parent);
> > +   mgr->parent = NULL;
> > +#endif
> > /*
>

[RFC PATCH 2/2] fpga: zynq-fpga: Add support for readback of configuration registers

[Appana Durga Kedareswara rao]

This patch adds support for Read-back of
configuration registers in zynq.

Signed-off-by: Appana Durga Kedareswara rao 
---
 drivers/fpga/zynq-fpga.c | 254 +++
 1 file changed, 254 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..31d39f0 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -127,6 +128,49 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/* Configuration Registers */
+#define TYPE1_CRC_OFFSET   0x0
+#define TYPE1_FAR_OFFSET   0x1
+#define TYPE1_FDRI_OFFSET  0x2
+#define TYPE1_FDRO_OFFSET  0x3
+#define TYPE1_CMD_OFFSET   0x4
+#define TYPE1_CTRL0_OFFSET 0x5
+#define TYPE1_MASK_OFFSET  0x6
+#define TYPE1_STAT_OFFSET  0x7
+#define TYPE1_LOUT_OFFSET  0x8
+#define TYPE1_COR0_OFFSET  0x9
+#define TYPE1_MFWR_OFFSET  0xa
+#define TYPE1_CBC_OFFSET   0xb
+#define TYPE1_IDCODE_OFFSET0xc
+#define TYPE1_AXSS_OFFSET  0xd
+#define TYPE1_COR1_OFFSET  0xe
+#define TYPE1_WBSTR_OFFSET 0x10
+#define TYPE1_TIMER_OFFSET 0x11
+#define TYPE1_BOOTSTS_OFFSET   0x16
+#define TYPE1_CTRL1_OFFSET 0x18
+#define TYPE1_BSPI_OFFSET  0x1f
+
+/* Masks for Configuration registers */
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE1_HDR_SHIFT29
+#define TYPE1_REG_SHIFT13
+#define TYPE1_OP_SHIFT 27
+#define TYPE1_OPCODE_NOOP  0
+#define TYPE1_OPCODE_READ  1
+#define TYPE1_OPCODE_WRITE 2
+
+#define CFGREG_SRCDMA_SIZE 0x40
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +184,7 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+   struct mutex mutex_lock;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +209,15 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
 /* Must be called with dma_lock held */
 static void zynq_step_dma(struct zynq_fpga_priv *priv)
 {
@@ -546,12 +600,211 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct 
fpga_manager *mgr)
return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+   /*
+* Type 1 Packet Header Format
+* The header section is always a 32-bit word.
+*
+* HeaderType | Opcode | Register Address | Reserved | Word Count
+* [31:29]  [28:27] [26:13]  [12:11] [10:0]
+* --
+*   001  xx  RxRR  xxx
+*
+* @R: means the bit is not used and reserved for future use.
+* The reserved bits should be written as 0s.
+*
+* Generating the Type 1 packet header which involves sifting of Type1
+* Header Mask, Register value and the OpCode which is 01 in this case
+* as only read operation is to be carried out and then performing OR
+* operation with the Word Length.
+*/
+   return (((1 << TYPE1_HDR_SHIFT) |
+   (reg << TYPE1_REG_SHIFT) |
+   (opcode << TYPE1_OP_SHIFT)) | size);
+
+}
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg)
+{
+   struct zynq_fpga_priv *priv;
+   int ret = 0, cmdindex, src_offset;
+   int *srcbuf, *dstbuf;
+   dma_addr_t src_dma_addr, dst_dma_addr;
+   u32 status, intr_status;
+
+   priv = mgr->priv;
+
+   srcbuf = dma_alloc_coherent(mgr->dev.parent, CFGREG_SRCDMA_SIZE,
+   _dma_addr, GFP_KERNEL);
+   if (!srcbuf)
+   return -ENOMEM;
+
+   dstbuf = dma_alloc_coherent(mgr->dev.parent, sizeof(dstbuf),
+   _dma_addr, GFP_KERNEL);
+   if (!dstbuf)
+   goto free_srcbuf;
+
+   cmdindex = 0;
+   srcbuf[cmdindex++] = DUMMY_WORD_MA

[RFC PATCH 2/2] fpga: zynq-fpga: Add support for readback of configuration registers

[Appana Durga Kedareswara rao]

This patch adds support for Read-back of
configuration registers in zynq.

Signed-off-by: Appana Durga Kedareswara rao 
---
 drivers/fpga/zynq-fpga.c | 254 +++
 1 file changed, 254 insertions(+)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index 70b15b3..31d39f0 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -127,6 +128,49 @@
 /* Disable global resets */
 #define FPGA_RST_NONE_MASK 0x0
 
+/* Configuration Registers */
+#define TYPE1_CRC_OFFSET   0x0
+#define TYPE1_FAR_OFFSET   0x1
+#define TYPE1_FDRI_OFFSET  0x2
+#define TYPE1_FDRO_OFFSET  0x3
+#define TYPE1_CMD_OFFSET   0x4
+#define TYPE1_CTRL0_OFFSET 0x5
+#define TYPE1_MASK_OFFSET  0x6
+#define TYPE1_STAT_OFFSET  0x7
+#define TYPE1_LOUT_OFFSET  0x8
+#define TYPE1_COR0_OFFSET  0x9
+#define TYPE1_MFWR_OFFSET  0xa
+#define TYPE1_CBC_OFFSET   0xb
+#define TYPE1_IDCODE_OFFSET0xc
+#define TYPE1_AXSS_OFFSET  0xd
+#define TYPE1_COR1_OFFSET  0xe
+#define TYPE1_WBSTR_OFFSET 0x10
+#define TYPE1_TIMER_OFFSET 0x11
+#define TYPE1_BOOTSTS_OFFSET   0x16
+#define TYPE1_CTRL1_OFFSET 0x18
+#define TYPE1_BSPI_OFFSET  0x1f
+
+/* Masks for Configuration registers */
+#define RCFG_CMD_MASK  0x0004
+#define START_CMD_MASK 0x0005
+#define RCRC_CMD_MASK  0x0007
+#define SHUTDOWN_CMD_MASK  0x000B
+#define DESYNC_WORD_MASK   0x000D
+#define BUSWIDTH_SYNCWORD_MASK 0x00BB
+#define NOOP_WORD_MASK 0x2000
+#define BUSWIDTH_DETECT_MASK   0x11220044
+#define SYNC_WORD_MASK 0xAA995566
+#define DUMMY_WORD_MASK0x
+
+#define TYPE1_HDR_SHIFT29
+#define TYPE1_REG_SHIFT13
+#define TYPE1_OP_SHIFT 27
+#define TYPE1_OPCODE_NOOP  0
+#define TYPE1_OPCODE_READ  1
+#define TYPE1_OPCODE_WRITE 2
+
+#define CFGREG_SRCDMA_SIZE 0x40
+
 struct zynq_fpga_priv {
int irq;
struct clk *clk;
@@ -140,6 +184,7 @@ struct zynq_fpga_priv {
struct scatterlist *cur_sg;
 
struct completion dma_done;
+   struct mutex mutex_lock;
 };
 
 static inline void zynq_fpga_write(struct zynq_fpga_priv *priv, u32 offset,
@@ -164,6 +209,15 @@ static inline void zynq_fpga_set_irq(struct zynq_fpga_priv 
*priv, u32 enable)
zynq_fpga_write(priv, INT_MASK_OFFSET, ~enable);
 }
 
+static void zynq_fpga_dma_xfer(struct zynq_fpga_priv *priv, u32 srcaddr,
+  u32 srclen, u32 dstaddr, u32 dstlen)
+{
+   zynq_fpga_write(priv, DMA_SRC_ADDR_OFFSET, srcaddr);
+   zynq_fpga_write(priv, DMA_DST_ADDR_OFFSET, dstaddr);
+   zynq_fpga_write(priv, DMA_SRC_LEN_OFFSET, srclen);
+   zynq_fpga_write(priv, DMA_DEST_LEN_OFFSET, dstlen);
+}
+
 /* Must be called with dma_lock held */
 static void zynq_step_dma(struct zynq_fpga_priv *priv)
 {
@@ -546,12 +600,211 @@ static enum fpga_mgr_states zynq_fpga_ops_state(struct 
fpga_manager *mgr)
return FPGA_MGR_STATE_UNKNOWN;
 }
 
+static int zynq_type1_pkt(u8 reg, u8 opcode, u16 size)
+{
+   /*
+* Type 1 Packet Header Format
+* The header section is always a 32-bit word.
+*
+* HeaderType | Opcode | Register Address | Reserved | Word Count
+* [31:29]  [28:27] [26:13]  [12:11] [10:0]
+* --
+*   001  xx  RxRR  xxx
+*
+* @R: means the bit is not used and reserved for future use.
+* The reserved bits should be written as 0s.
+*
+* Generating the Type 1 packet header which involves sifting of Type1
+* Header Mask, Register value and the OpCode which is 01 in this case
+* as only read operation is to be carried out and then performing OR
+* operation with the Word Length.
+*/
+   return (((1 << TYPE1_HDR_SHIFT) |
+   (reg << TYPE1_REG_SHIFT) |
+   (opcode << TYPE1_OP_SHIFT)) | size);
+
+}
+
+static int zynq_fpga_getconfigreg(struct fpga_manager *mgr, u8 reg)
+{
+   struct zynq_fpga_priv *priv;
+   int ret = 0, cmdindex, src_offset;
+   int *srcbuf, *dstbuf;
+   dma_addr_t src_dma_addr, dst_dma_addr;
+   u32 status, intr_status;
+
+   priv = mgr->priv;
+
+   srcbuf = dma_alloc_coherent(mgr->dev.parent, CFGREG_SRCDMA_SIZE,
+   _dma_addr, GFP_KERNEL);
+   if (!srcbuf)
+   return -ENOMEM;
+
+   dstbuf = dma_alloc_coherent(mgr->dev.parent, sizeof(dstbuf),
+   _dma_addr, GFP_KERNEL);
+   if (!dstbuf)
+   goto free_srcbuf;
+
+   cmdindex = 0;
+   srcbuf[cmdindex++] = DUMMY_WORD_MA

[RFC PATCH 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/readback

Signed-off-by: Appana Durga Kedareswara rao 
---
 drivers/fpga/fpga-mgr.c   | 52 +++
 include/linux/fpga/fpga-mgr.h |  6 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..7a9fd7c 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,39 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING)
+   return -EBUSY;
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret) {
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+   return ret;
+   }
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_debugops = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +614,21 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_DEBUG_FS
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   mgr->parent = mgr->dir;
+   mgr->dir = debugfs_create_file("readback", 0644, mgr->parent, mgr,
+  _mgr_debugops);
+   if (!mgr->dir) {
+   debugfs_remove_recursive(mgr->parent);
+   mgr->parent = NULL;
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +652,10 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_DEBUG_FS
+   debugfs_remove_recursive(mgr->parent);
+   mgr->parent = NULL;
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..6013809 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,6 +153,10 @@ struct fpga_manager {
enum fpga_mgr_states state;
const struct fpga_manager_ops *mops;
void *priv;
+#ifdef CONFIG_DEBUG_FS
+   struct dentry *dir;
+   struct dentry *parent;
+#endif
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.7.4

[RFC PATCH 1/2] fpga: fpga-mgr: Add readback support

[Appana Durga Kedareswara rao]

Inorder to debug issues with fpga's users would
like to read the fpga configuration information.
This patch adds readback support for fpga configuration data
in the framework through debugfs interface.

Usage:
cat /sys/kernel/debug/fpga/readback

Signed-off-by: Appana Durga Kedareswara rao 
---
 drivers/fpga/fpga-mgr.c   | 52 +++
 include/linux/fpga/fpga-mgr.h |  6 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 9939d2c..7a9fd7c 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -484,6 +484,39 @@ void fpga_mgr_put(struct fpga_manager *mgr)
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_put);
 
+#ifdef CONFIG_DEBUG_FS
+#include 
+
+static int fpga_mgr_read(struct seq_file *s, void *data)
+{
+   struct fpga_manager *mgr = (struct fpga_manager *)s->private;
+   int ret = 0;
+
+   if (mgr->state != FPGA_MGR_STATE_OPERATING)
+   return -EBUSY;
+
+   /* Read the FPGA configuration data from the fabric */
+   ret = mgr->mops->read(mgr, s);
+   if (ret) {
+   dev_err(>dev, "Error while reading configuration data from 
FPGA\n");
+   return ret;
+   }
+
+   return ret;
+}
+
+static int fpga_mgr_read_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, fpga_mgr_read, inode->i_private);
+}
+
+static const struct file_operations fpga_mgr_debugops = {
+   .owner = THIS_MODULE,
+   .open = fpga_mgr_read_open,
+   .read = seq_read,
+};
+#endif
+
 /**
  * fpga_mgr_lock - Lock FPGA manager for exclusive use
  * @mgr:   fpga manager
@@ -581,6 +614,21 @@ int fpga_mgr_register(struct device *dev, const char *name,
if (ret)
goto error_device;
 
+#ifdef CONFIG_DEBUG_FS
+   mgr->dir = debugfs_create_dir("fpga", NULL);
+   if (!mgr->dir)
+   goto error_device;
+
+   mgr->parent = mgr->dir;
+   mgr->dir = debugfs_create_file("readback", 0644, mgr->parent, mgr,
+  _mgr_debugops);
+   if (!mgr->dir) {
+   debugfs_remove_recursive(mgr->parent);
+   mgr->parent = NULL;
+   goto error_device;
+   }
+#endif
+
dev_info(>dev, "%s registered\n", mgr->name);
 
return 0;
@@ -604,6 +652,10 @@ void fpga_mgr_unregister(struct device *dev)
 
dev_info(>dev, "%s %s\n", __func__, mgr->name);
 
+#ifdef CONFIG_DEBUG_FS
+   debugfs_remove_recursive(mgr->parent);
+   mgr->parent = NULL;
+#endif
/*
 * If the low level driver provides a method for putting fpga into
 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 3c6de23..6013809 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -114,6 +114,7 @@ struct fpga_image_info {
  * @write: write count bytes of configuration data to the FPGA
  * @write_sg: write the scatter list of configuration data to the FPGA
  * @write_complete: set FPGA to operating state after writing is done
+ * @read: read FPGA configuration information
  * @fpga_remove: optional: Set FPGA into a specific state during driver remove
  * @groups: optional attribute groups.
  *
@@ -131,6 +132,7 @@ struct fpga_manager_ops {
int (*write_sg)(struct fpga_manager *mgr, struct sg_table *sgt);
int (*write_complete)(struct fpga_manager *mgr,
  struct fpga_image_info *info);
+   int (*read)(struct fpga_manager *mgr, struct seq_file *s);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
 };
@@ -151,6 +153,10 @@ struct fpga_manager {
enum fpga_mgr_states state;
const struct fpga_manager_ops *mops;
void *priv;
+#ifdef CONFIG_DEBUG_FS
+   struct dentry *dir;
+   struct dentry *parent;
+#endif
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.7.4

RE: [PATCH 1/2] dt-bindings: dmaengine: xilinx_dma: Add VDMA vertical flip property

[Appana Durga Kedareswara Rao]

 
> 
> The AXI VDMA core supports Vertical flip in S2MM path when Enable Vertical
> Flip (Advanced tab) is selected. To allow vertical flip programming define an
> optional 'xlnx,enable-vert-flip' channel child node property.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-by: Kedareswara rao Appana

Regards,
Kedar.

> ---
>  .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfa..174af2c 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -66,6 +66,8 @@ Optional child node properties:
>  Optional child node properties for VDMA:
>  - xlnx,genlock-mode: Tells Genlock synchronization is
>   enabled/disabled in hardware.
> +- xlnx,enable-vert-flip: Tells vertical flip is
> + enabled/disabled in hardware(S2MM path).
>  Optional child node properties for AXI DMA:
>  -dma-channels: Number of dma channels in child node.
> 
> --
> 1.7.1

RE: [PATCH 1/2] dt-bindings: dmaengine: xilinx_dma: Add VDMA vertical flip property

[Appana Durga Kedareswara Rao]

 
> 
> The AXI VDMA core supports Vertical flip in S2MM path when Enable Vertical
> Flip (Advanced tab) is selected. To allow vertical flip programming define an
> optional 'xlnx,enable-vert-flip' channel child node property.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-by: Kedareswara rao Appana

Regards,
Kedar.

> ---
>  .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfa..174af2c 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -66,6 +66,8 @@ Optional child node properties:
>  Optional child node properties for VDMA:
>  - xlnx,genlock-mode: Tells Genlock synchronization is
>   enabled/disabled in hardware.
> +- xlnx,enable-vert-flip: Tells vertical flip is
> + enabled/disabled in hardware(S2MM path).
>  Optional child node properties for AXI DMA:
>  -dma-channels: Number of dma channels in child node.
> 
> --
> 1.7.1

RE: [PATCH 2/2] dmaengine: xilinx_dma: Enable VDMA S2MM vertical flip support

[Appana Durga Kedareswara Rao]

> 
> Vertical flip state is exported in xilinx_vdma_config and depending on IP
> configuration(c_enable_vert_flip) vertical flip state is programmed in
> hardware.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-by: Kedareswara rao Appana   

> ---
>  drivers/dma/xilinx/xilinx_dma.c |   22 ++
>  include/linux/dma/xilinx_dma.h  |2 ++
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index 27b5235..c124423 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -115,6 +115,9 @@
>  #define XILINX_VDMA_REG_START_ADDRESS(n) (0x000c + 4 * (n))
>  #define XILINX_VDMA_REG_START_ADDRESS_64(n)  (0x000c + 8 * (n))
> 
> +#define XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP 0x00ec
> +#define XILINX_VDMA_ENABLE_VERTICAL_FLIP BIT(0)
> +
>  /* HW specific definitions */
>  #define XILINX_DMA_MAX_CHANS_PER_DEVICE  0x20
> 
> @@ -340,6 +343,7 @@ struct xilinx_dma_tx_descriptor {
>   * @start_transfer: Differentiate b/w DMA IP's transfer
>   * @stop_transfer: Differentiate b/w DMA IP's quiesce
>   * @tdest: TDEST value for mcdma
> + * @has_vflip: S2MM vertical flip
>   */
>  struct xilinx_dma_chan {
>   struct xilinx_dma_device *xdev;
> @@ -376,6 +380,7 @@ struct xilinx_dma_chan {
>   void (*start_transfer)(struct xilinx_dma_chan *chan);
>   int (*stop_transfer)(struct xilinx_dma_chan *chan);
>   u16 tdest;
> + bool has_vflip;
>  };
> 
>  /**
> @@ -1092,6 +1097,14 @@ static void xilinx_vdma_start_transfer(struct
> xilinx_dma_chan *chan)
>   desc->async_tx.phys);
> 
>   /* Configure the hardware using info in the config structure */
> + if (chan->has_vflip) {
> + reg = dma_read(chan,
> XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP);
> + reg &= ~XILINX_VDMA_ENABLE_VERTICAL_FLIP;
> + reg |= config->vflip_en;
> + dma_write(chan,
> XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP,
> +   reg);
> + }
> +
>   reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
> 
>   if (config->frm_cnt_en)
> @@ -2105,6 +2118,8 @@ int xilinx_vdma_channel_set_config(struct
> dma_chan *dchan,
>   }
> 
>   chan->config.frm_cnt_en = cfg->frm_cnt_en;
> + chan->config.vflip_en = cfg->vflip_en;
> +
>   if (cfg->park)
>   chan->config.park_frm = cfg->park_frm;
>   else
> @@ -2428,6 +2443,13 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
>   chan->direction = DMA_DEV_TO_MEM;
>   chan->id = chan_id;
>   chan->tdest = chan_id - xdev->nr_channels;
> + chan->has_vflip = of_property_read_bool(node,
> + "xlnx,enable-vert-flip");
> + if (chan->has_vflip) {
> + chan->config.vflip_en = dma_read(chan,
> + XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP)
> &
> + XILINX_VDMA_ENABLE_VERTICAL_FLIP;
> + }
> 
>   chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>   if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { diff
> --git a/include/linux/dma/xilinx_dma.h b/include/linux/dma/xilinx_dma.h
> index 34b98f2..5b6e61e 100644
> --- a/include/linux/dma/xilinx_dma.h
> +++ b/include/linux/dma/xilinx_dma.h
> @@ -27,6 +27,7 @@
>   * @delay: Delay counter
>   * @reset: Reset Channel
>   * @ext_fsync: External Frame Sync source
> + * @vflip_en:  Vertical Flip enable
>   */
>  struct xilinx_vdma_config {
>   int frm_dly;
> @@ -39,6 +40,7 @@ struct xilinx_vdma_config {
>   int delay;
>   int reset;
>   int ext_fsync;
> + bool vflip_en;
>  };
> 
>  int xilinx_vdma_channel_set_config(struct dma_chan *dchan,
> --
> 1.7.1

RE: [PATCH 2/2] dmaengine: xilinx_dma: Enable VDMA S2MM vertical flip support

[Appana Durga Kedareswara Rao]

> 
> Vertical flip state is exported in xilinx_vdma_config and depending on IP
> configuration(c_enable_vert_flip) vertical flip state is programmed in
> hardware.
> 
> Signed-off-by: Radhey Shyam Pandey 
> Signed-off-by: Michal Simek 

Acked-by: Kedareswara rao Appana   

> ---
>  drivers/dma/xilinx/xilinx_dma.c |   22 ++
>  include/linux/dma/xilinx_dma.h  |2 ++
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dma.c
> b/drivers/dma/xilinx/xilinx_dma.c index 27b5235..c124423 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -115,6 +115,9 @@
>  #define XILINX_VDMA_REG_START_ADDRESS(n) (0x000c + 4 * (n))
>  #define XILINX_VDMA_REG_START_ADDRESS_64(n)  (0x000c + 8 * (n))
> 
> +#define XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP 0x00ec
> +#define XILINX_VDMA_ENABLE_VERTICAL_FLIP BIT(0)
> +
>  /* HW specific definitions */
>  #define XILINX_DMA_MAX_CHANS_PER_DEVICE  0x20
> 
> @@ -340,6 +343,7 @@ struct xilinx_dma_tx_descriptor {
>   * @start_transfer: Differentiate b/w DMA IP's transfer
>   * @stop_transfer: Differentiate b/w DMA IP's quiesce
>   * @tdest: TDEST value for mcdma
> + * @has_vflip: S2MM vertical flip
>   */
>  struct xilinx_dma_chan {
>   struct xilinx_dma_device *xdev;
> @@ -376,6 +380,7 @@ struct xilinx_dma_chan {
>   void (*start_transfer)(struct xilinx_dma_chan *chan);
>   int (*stop_transfer)(struct xilinx_dma_chan *chan);
>   u16 tdest;
> + bool has_vflip;
>  };
> 
>  /**
> @@ -1092,6 +1097,14 @@ static void xilinx_vdma_start_transfer(struct
> xilinx_dma_chan *chan)
>   desc->async_tx.phys);
> 
>   /* Configure the hardware using info in the config structure */
> + if (chan->has_vflip) {
> + reg = dma_read(chan,
> XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP);
> + reg &= ~XILINX_VDMA_ENABLE_VERTICAL_FLIP;
> + reg |= config->vflip_en;
> + dma_write(chan,
> XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP,
> +   reg);
> + }
> +
>   reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR);
> 
>   if (config->frm_cnt_en)
> @@ -2105,6 +2118,8 @@ int xilinx_vdma_channel_set_config(struct
> dma_chan *dchan,
>   }
> 
>   chan->config.frm_cnt_en = cfg->frm_cnt_en;
> + chan->config.vflip_en = cfg->vflip_en;
> +
>   if (cfg->park)
>   chan->config.park_frm = cfg->park_frm;
>   else
> @@ -2428,6 +2443,13 @@ static int xilinx_dma_chan_probe(struct
> xilinx_dma_device *xdev,
>   chan->direction = DMA_DEV_TO_MEM;
>   chan->id = chan_id;
>   chan->tdest = chan_id - xdev->nr_channels;
> + chan->has_vflip = of_property_read_bool(node,
> + "xlnx,enable-vert-flip");
> + if (chan->has_vflip) {
> + chan->config.vflip_en = dma_read(chan,
> + XILINX_VDMA_REG_ENABLE_VERTICAL_FLIP)
> &
> + XILINX_VDMA_ENABLE_VERTICAL_FLIP;
> + }
> 
>   chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>   if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { diff
> --git a/include/linux/dma/xilinx_dma.h b/include/linux/dma/xilinx_dma.h
> index 34b98f2..5b6e61e 100644
> --- a/include/linux/dma/xilinx_dma.h
> +++ b/include/linux/dma/xilinx_dma.h
> @@ -27,6 +27,7 @@
>   * @delay: Delay counter
>   * @reset: Reset Channel
>   * @ext_fsync: External Frame Sync source
> + * @vflip_en:  Vertical Flip enable
>   */
>  struct xilinx_vdma_config {
>   int frm_dly;
> @@ -39,6 +40,7 @@ struct xilinx_vdma_config {
>   int delay;
>   int reset;
>   int ext_fsync;
> + bool vflip_en;
>  };
> 
>  int xilinx_vdma_channel_set_config(struct dma_chan *dchan,
> --
> 1.7.1

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the review... 

>On Tue, Jan 09, 2018 at 04:48:10AM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi,
>>
>> >On Mon, Jan 08, 2018 at 05:25:01PM +0000, Appana Durga Kedareswara
>> >Rao
>> >wrote:
>> >> Hi,
>> >>
>> >> 
>> >> >> >> +   xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> >> >> +   xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >> >> >
>> >> >> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >> >> >What is value of addr_width here typically? Usually controllers
>> >> >> >can support different widths and this is a surprise that you
>> >> >> >support only one value
>> >> >>
>> >> >> Controller supports address width of 32 and 64.
>> >> >
>> >> >Then this should have both 32 and 64 values here
>> >>
>> >> Address width is configurable parameter at the h/w level.
>> >> Since this IP is a soft IP user can create a design with either
>> >> 32-bit or 64-bit address configuration.
>> >
>> >and not both right?
>>
>> Yes not both at the same time...
>> Axi dma controller can be configured for either 32-bit or 64-bit address...
>
>So my suspicion was correct.  I would suggest you to read up on the
>documentation again. The src/dst_addr_widths has _nothing_ to do with 32/64
>bit addresses used.
>
>It is the capability of the dma controller to do transfers with data width as 
>8bits,
>16 bits, so on. iKey is "data width" and not address type.
>This typically translates to DMA FIFO configuration of the controller!

Thanks for the detailed explanation... 
I have gone through the spec again controller does supports 1 byte, 2 byte, 4 
byte up to 128 byte transfers.
In order to do variable length transfers user needs to drive a valid value to 
the tkeep strobe signal at the h/w level.
And user needs to configure the below parameters c_m_axis_mm2s_tdata_width or 
c_m_axis_s2mm_tdata_width
With desired configuration at the h/w level.
Controller supports data width of 8, 16, 32, 64, 128, 256, 512 and 1,024 bits 
(i.e. c_m_axis_mm2s_tdata_width/ c_m_axis_s2mm_tdata_width parameters range)

At the s/w level currently we are getting c_m_axis_mm2s_tdata_width/ 
c_m_axis_s2mm_tdata_width
Configuration as xlnx,datawidth property in the device-tree.

So proper values for the src/dst_addr width fields should be, datawidth 
property in bytes.
Please correct me if I am wrong... 

Changes looks like below...
Here width is in bytes based on the h/w configuration... 

--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -2411,6 +2411,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device 
*xdev,
chan->direction = DMA_MEM_TO_DEV;
chan->id = chan_id;
chan->tdest = chan_id;
+   xdev->common.directions = BIT(DMA_MEM_TO_DEV);
+   xdev->common.src_addr_widths = BIT(width);
 
chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
@@ -2428,6 +2430,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device 
*xdev,
chan->direction = DMA_DEV_TO_MEM;
chan->id = chan_id;
chan->tdest = chan_id - xdev->nr_channels;
+   xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
+   xdev->common.dst_addr_widths = BIT(width);
 
chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {


Regards,
Kedar.

>
>--
>~Vinod

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the review... 

>On Tue, Jan 09, 2018 at 04:48:10AM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi,
>>
>> >On Mon, Jan 08, 2018 at 05:25:01PM +0000, Appana Durga Kedareswara
>> >Rao
>> >wrote:
>> >> Hi,
>> >>
>> >> 
>> >> >> >> +   xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> >> >> +   xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >> >> >
>> >> >> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >> >> >What is value of addr_width here typically? Usually controllers
>> >> >> >can support different widths and this is a surprise that you
>> >> >> >support only one value
>> >> >>
>> >> >> Controller supports address width of 32 and 64.
>> >> >
>> >> >Then this should have both 32 and 64 values here
>> >>
>> >> Address width is configurable parameter at the h/w level.
>> >> Since this IP is a soft IP user can create a design with either
>> >> 32-bit or 64-bit address configuration.
>> >
>> >and not both right?
>>
>> Yes not both at the same time...
>> Axi dma controller can be configured for either 32-bit or 64-bit address...
>
>So my suspicion was correct.  I would suggest you to read up on the
>documentation again. The src/dst_addr_widths has _nothing_ to do with 32/64
>bit addresses used.
>
>It is the capability of the dma controller to do transfers with data width as 
>8bits,
>16 bits, so on. iKey is "data width" and not address type.
>This typically translates to DMA FIFO configuration of the controller!

Thanks for the detailed explanation... 
I have gone through the spec again controller does supports 1 byte, 2 byte, 4 
byte up to 128 byte transfers.
In order to do variable length transfers user needs to drive a valid value to 
the tkeep strobe signal at the h/w level.
And user needs to configure the below parameters c_m_axis_mm2s_tdata_width or 
c_m_axis_s2mm_tdata_width
With desired configuration at the h/w level.
Controller supports data width of 8, 16, 32, 64, 128, 256, 512 and 1,024 bits 
(i.e. c_m_axis_mm2s_tdata_width/ c_m_axis_s2mm_tdata_width parameters range)

At the s/w level currently we are getting c_m_axis_mm2s_tdata_width/ 
c_m_axis_s2mm_tdata_width
Configuration as xlnx,datawidth property in the device-tree.

So proper values for the src/dst_addr width fields should be, datawidth 
property in bytes.
Please correct me if I am wrong... 

Changes looks like below...
Here width is in bytes based on the h/w configuration... 

--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -2411,6 +2411,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device 
*xdev,
chan->direction = DMA_MEM_TO_DEV;
chan->id = chan_id;
chan->tdest = chan_id;
+   xdev->common.directions = BIT(DMA_MEM_TO_DEV);
+   xdev->common.src_addr_widths = BIT(width);
 
chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {
@@ -2428,6 +2430,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device 
*xdev,
chan->direction = DMA_DEV_TO_MEM;
chan->id = chan_id;
chan->tdest = chan_id - xdev->nr_channels;
+   xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
+   xdev->common.dst_addr_widths = BIT(width);
 
chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) {


Regards,
Kedar.

>
>--
>~Vinod

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,

>On Mon, Jan 08, 2018 at 05:25:01PM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi,
>>
>> 
>> >> >> +  xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> >> +  xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >> >
>> >> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >> >What is value of addr_width here typically? Usually controllers
>> >> >can support different widths and this is a surprise that you
>> >> >support only one value
>> >>
>> >> Controller supports address width of 32 and 64.
>> >
>> >Then this should have both 32 and 64 values here
>>
>> Address width is configurable parameter at the h/w level.
>> Since this IP is a soft IP user can create a design with either 32-bit
>> or 64-bit address configuration.
>
>and not both right?

Yes not both at the same time... 
Axi dma controller can be configured for either 32-bit or 64-bit address...

Regards,
Kedar.

>
>> Currently we are reading this configuration through device-tree (xlnx,
>> addr-width property)
>> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/tr
>> ee/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt#n19
>> Based on the h/w configuration setting the dst_addr_widths/src_addr_widths
>variables in this patch.
>> Please let me know if you are still not clear with my explanation will 
>> explain in
>detail...
>>
>> Regards,
>> Kedar.
>>
>> >
>> >> addr_width typical values are 32-bit or 64-bit .
>> >> Here addr_width is device-tree parameter...
>> >> my understanding of src_addr_widths/dst_addr_widths is, it is a bit
>> >> mask of the address with in bytes that DMA supports, please correct
>> >> if my
>> >understanding is wrong.
>> >>
>> >> Regards,
>> >> Kedar.
>> >>
>> >> >
>> >> >--
>> >> >~Vinod
>> >
>> >--
>> >~Vinod
>
>--
>~Vinod
>--
>To unsubscribe from this list: send the line "unsubscribe dmaengine" 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 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,

>On Mon, Jan 08, 2018 at 05:25:01PM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi,
>>
>> 
>> >> >> +  xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> >> +  xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >> >
>> >> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >> >What is value of addr_width here typically? Usually controllers
>> >> >can support different widths and this is a surprise that you
>> >> >support only one value
>> >>
>> >> Controller supports address width of 32 and 64.
>> >
>> >Then this should have both 32 and 64 values here
>>
>> Address width is configurable parameter at the h/w level.
>> Since this IP is a soft IP user can create a design with either 32-bit
>> or 64-bit address configuration.
>
>and not both right?

Yes not both at the same time... 
Axi dma controller can be configured for either 32-bit or 64-bit address...

Regards,
Kedar.

>
>> Currently we are reading this configuration through device-tree (xlnx,
>> addr-width property)
>> https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/tr
>> ee/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt#n19
>> Based on the h/w configuration setting the dst_addr_widths/src_addr_widths
>variables in this patch.
>> Please let me know if you are still not clear with my explanation will 
>> explain in
>detail...
>>
>> Regards,
>> Kedar.
>>
>> >
>> >> addr_width typical values are 32-bit or 64-bit .
>> >> Here addr_width is device-tree parameter...
>> >> my understanding of src_addr_widths/dst_addr_widths is, it is a bit
>> >> mask of the address with in bytes that DMA supports, please correct
>> >> if my
>> >understanding is wrong.
>> >>
>> >> Regards,
>> >> Kedar.
>> >>
>> >> >
>> >> >--
>> >> >~Vinod
>> >
>> >--
>> >~Vinod
>
>--
>~Vinod
>--
>To unsubscribe from this list: send the line "unsubscribe dmaengine" 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 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,


>> >> + xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> + xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >
>> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >What is value of addr_width here typically? Usually controllers can
>> >support different widths and this is a surprise that you support only
>> >one value
>>
>> Controller supports address width of 32 and 64.
>
>Then this should have both 32 and 64 values here

Address width is configurable parameter at the h/w level.
Since this IP is a soft IP user can create a design with either 
32-bit or 64-bit address configuration. 
Currently we are reading this configuration through device-tree (xlnx, 
addr-width property) 
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/tree/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt#n19
Based on the h/w configuration setting the dst_addr_widths/src_addr_widths 
variables in this patch.
Please let me know if you are still not clear with my explanation will explain 
in detail... 

Regards,
Kedar.

>
>> addr_width typical values are 32-bit or 64-bit .
>> Here addr_width is device-tree parameter...
>> my understanding of src_addr_widths/dst_addr_widths is, it is a bit
>> mask of the address with in bytes that DMA supports, please correct if my
>understanding is wrong.
>>
>> Regards,
>> Kedar.
>>
>> >
>> >--
>> >~Vinod
>
>--
>~Vinod

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi,


>> >> + xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> >> + xdev->common.src_addr_widths = BIT(addr_width / 8);
>> >
>> >Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers?
>> >What is value of addr_width here typically? Usually controllers can
>> >support different widths and this is a surprise that you support only
>> >one value
>>
>> Controller supports address width of 32 and 64.
>
>Then this should have both 32 and 64 values here

Address width is configurable parameter at the h/w level.
Since this IP is a soft IP user can create a design with either 
32-bit or 64-bit address configuration. 
Currently we are reading this configuration through device-tree (xlnx, 
addr-width property) 
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/tree/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt#n19
Based on the h/w configuration setting the dst_addr_widths/src_addr_widths 
variables in this patch.
Please let me know if you are still not clear with my explanation will explain 
in detail... 

Regards,
Kedar.

>
>> addr_width typical values are 32-bit or 64-bit .
>> Here addr_width is device-tree parameter...
>> my understanding of src_addr_widths/dst_addr_widths is, it is a bit
>> mask of the address with in bytes that DMA supports, please correct if my
>understanding is wrong.
>>
>> Regards,
>> Kedar.
>>
>> >
>> >--
>> >~Vinod
>
>--
>~Vinod

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review 

>> @@ -2398,6 +2398,7 @@ static int xilinx_dma_chan_probe(struct
>xilinx_dma_device *xdev,
>>  chan->direction = DMA_MEM_TO_DEV;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id;
>> +xdev->common.directions = BIT(DMA_MEM_TO_DEV);
>>
>>  chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2415,6
>> +2416,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>>  chan->direction = DMA_DEV_TO_MEM;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id - xdev->nr_channels;
>> +xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
>>
>>  chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2629,6
>> +2631,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>>  dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
>>  }
>>
>> +xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> +xdev->common.src_addr_widths = BIT(addr_width / 8);
>
>Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers? What is 
>value
>of addr_width here typically? Usually controllers can support different widths 
>and
>this is a surprise that you support only one value

Controller supports address width of 32 and 64.
addr_width typical values are 32-bit or 64-bit .
Here addr_width is device-tree parameter...
my understanding of src_addr_widths/dst_addr_widths is, it is a bit mask of the 
address with in bytes that DMA supports, please correct if my understanding is 
wrong.

Regards,
Kedar.

>
>--
>~Vinod

RE: [PATCH v2 1/4] dmaengine: xilinx_dma: populate dma caps properly

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review 

>> @@ -2398,6 +2398,7 @@ static int xilinx_dma_chan_probe(struct
>xilinx_dma_device *xdev,
>>  chan->direction = DMA_MEM_TO_DEV;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id;
>> +xdev->common.directions = BIT(DMA_MEM_TO_DEV);
>>
>>  chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2415,6
>> +2416,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>>  chan->direction = DMA_DEV_TO_MEM;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id - xdev->nr_channels;
>> +xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
>>
>>  chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2629,6
>> +2631,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>>  dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
>>  }
>>
>> +xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> +xdev->common.src_addr_widths = BIT(addr_width / 8);
>
>Do you not support trf of 1byte, 2 bytes, or 4 bytes wide transfers? What is 
>value
>of addr_width here typically? Usually controllers can support different widths 
>and
>this is a surprise that you support only one value

Controller supports address width of 32 and 64.
addr_width typical values are 32-bit or 64-bit .
Here addr_width is device-tree parameter...
my understanding of src_addr_widths/dst_addr_widths is, it is a bit mask of the 
address with in bytes that DMA supports, please correct if my understanding is 
wrong.

Regards,
Kedar.

>
>--
>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi,


>>> >BTW whats with LINUX tag in patches, pls drop them
>>>
>>> Ok will mention the Linux tag info in the cover letter patch from the
>>> next patch series on wards...
>>
>>Please wrap your replies within 80chars. It is very hard to read! I have 
>>reflown
>for
>>readability
>
>Sure will take care of it next time onwards...
>
>>
>>Can you explain what you mean by that info, what are you trying to convey?
>
>What I mean here is will mention the Linux kernel tag
>Information in the cover letter patch...

Oops sorry I misunderstood your comment... 
In my company we have internally different projects
To differentiate b/w them we usually use LINUX prefix
By mistake I have added the LINUX prefix in this patch series
I have removed it in the v2 series... 

Regards,
Kedar.

>
>Regards,
>Kedar.
>
>>
>>--
>>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi,


>>> >BTW whats with LINUX tag in patches, pls drop them
>>>
>>> Ok will mention the Linux tag info in the cover letter patch from the
>>> next patch series on wards...
>>
>>Please wrap your replies within 80chars. It is very hard to read! I have 
>>reflown
>for
>>readability
>
>Sure will take care of it next time onwards...
>
>>
>>Can you explain what you mean by that info, what are you trying to convey?
>
>What I mean here is will mention the Linux kernel tag
>Information in the cover letter patch...

Oops sorry I misunderstood your comment... 
In my company we have internally different projects
To differentiate b/w them we usually use LINUX prefix
By mistake I have added the LINUX prefix in this patch series
I have removed it in the v2 series... 

Regards,
Kedar.

>
>Regards,
>Kedar.
>
>>
>>--
>>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi Vinod,


>On Wed, Jan 03, 2018 at 05:13:29AM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi Vinod,
>>
>>  Thanks for the review...
>>
>> >
>> >On Thu, Dec 21, 2017 at 03:41:37PM +0530, Kedareswara rao Appana wrote:
>> >
>> >Fix title here too
>>
>> Sure will fix in v2...
>>
>> >
>> >BTW whats with LINUX tag in patches, pls drop them
>>
>> Ok will mention the Linux tag info in the cover letter patch from the
>> next patch series on wards...
>
>Please wrap your replies within 80chars. It is very hard to read! I have 
>reflown for
>readability

Sure will take care of it next time onwards... 

>
>Can you explain what you mean by that info, what are you trying to convey?

What I mean here is will mention the Linux kernel tag
Information in the cover letter patch...

Regards,
Kedar.

>
>--
>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi Vinod,


>On Wed, Jan 03, 2018 at 05:13:29AM +0000, Appana Durga Kedareswara Rao
>wrote:
>> Hi Vinod,
>>
>>  Thanks for the review...
>>
>> >
>> >On Thu, Dec 21, 2017 at 03:41:37PM +0530, Kedareswara rao Appana wrote:
>> >
>> >Fix title here too
>>
>> Sure will fix in v2...
>>
>> >
>> >BTW whats with LINUX tag in patches, pls drop them
>>
>> Ok will mention the Linux tag info in the cover letter patch from the
>> next patch series on wards...
>
>Please wrap your replies within 80chars. It is very hard to read! I have 
>reflown for
>readability

Sure will take care of it next time onwards... 

>
>Can you explain what you mean by that info, what are you trying to convey?

What I mean here is will mention the Linux kernel tag
Information in the cover letter patch...

Regards,
Kedar.

>
>--
>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 

>
>On Thu, Dec 21, 2017 at 03:41:37PM +0530, Kedareswara rao Appana wrote:
>
>Fix title here too

Sure will fix in v2... 

>
>BTW whats with LINUX tag in patches, pls drop them

Ok will mention the Linux tag info in the cover letter patch from the next 
patch series on wards...

Regards,
Kedar.

>
>> This patch fixes the below sparse warning in the driver
>> drivers/dma/xilinx/xilinx_dma.c: In function
>‘xilinx_vdma_dma_prep_interleaved’:
>> drivers/dma/xilinx/xilinx_dma.c:1614:43: warning: variable ‘prev’ set but not
>used [-Wunused-but-set-variable]
>>   struct xilinx_vdma_tx_segment *segment, *prev = NULL;
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 8467671..845e638 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1611,7 +1611,7 @@ xilinx_vdma_dma_prep_interleaved(struct dma_chan
>> *dchan,  {
>>  struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
>>  struct xilinx_dma_tx_descriptor *desc;
>> -struct xilinx_vdma_tx_segment *segment, *prev = NULL;
>> +struct xilinx_vdma_tx_segment *segment;
>>  struct xilinx_vdma_desc_hw *hw;
>>
>>  if (!is_slave_direction(xt->dir))
>> @@ -1665,8 +1665,6 @@ xilinx_vdma_dma_prep_interleaved(struct dma_chan
>*dchan,
>>  /* Insert the segment into the descriptor segments list. */
>>  list_add_tail(>node, >segments);
>>
>> -prev = segment;
>> -
>>  /* Link the last hardware descriptor with the first. */
>>  segment = list_first_entry(>segments,
>> struct xilinx_vdma_tx_segment, node);
>> --
>> 2.7.4
>>
>
>--
>~Vinod

RE: [LINUX PATCH 3/4] dmaengine: xilinx_dma: Fix compilation warning

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 

>
>On Thu, Dec 21, 2017 at 03:41:37PM +0530, Kedareswara rao Appana wrote:
>
>Fix title here too

Sure will fix in v2... 

>
>BTW whats with LINUX tag in patches, pls drop them

Ok will mention the Linux tag info in the cover letter patch from the next 
patch series on wards...

Regards,
Kedar.

>
>> This patch fixes the below sparse warning in the driver
>> drivers/dma/xilinx/xilinx_dma.c: In function
>‘xilinx_vdma_dma_prep_interleaved’:
>> drivers/dma/xilinx/xilinx_dma.c:1614:43: warning: variable ‘prev’ set but not
>used [-Wunused-but-set-variable]
>>   struct xilinx_vdma_tx_segment *segment, *prev = NULL;
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 8467671..845e638 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1611,7 +1611,7 @@ xilinx_vdma_dma_prep_interleaved(struct dma_chan
>> *dchan,  {
>>  struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
>>  struct xilinx_dma_tx_descriptor *desc;
>> -struct xilinx_vdma_tx_segment *segment, *prev = NULL;
>> +struct xilinx_vdma_tx_segment *segment;
>>  struct xilinx_vdma_desc_hw *hw;
>>
>>  if (!is_slave_direction(xt->dir))
>> @@ -1665,8 +1665,6 @@ xilinx_vdma_dma_prep_interleaved(struct dma_chan
>*dchan,
>>  /* Insert the segment into the descriptor segments list. */
>>  list_add_tail(>node, >segments);
>>
>> -prev = segment;
>> -
>>  /* Link the last hardware descriptor with the first. */
>>  segment = list_first_entry(>segments,
>> struct xilinx_vdma_tx_segment, node);
>> --
>> 2.7.4
>>
>
>--
>~Vinod

RE: [LINUX PATCH 2/4] dmaengine: xilinx_dma: Fix race condition in the driver for cdma

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 

>
>On Thu, Dec 21, 2017 at 03:41:36PM +0530, Kedareswara rao Appana wrote:
>
>same issue for patch title here too

Ok will fix in v2... 

>
>> when hardware is idle we need to toggle the SG bit in the control
>> register, inorder to update new value to the current descriptor
>> register other wise undefined results will occur.
>
>can you try making it bit more clear..

Sure will fix in v2... 

Regards,
Kedar.

>
>>
>> This patch updates the same.
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 21ac954..8467671 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1204,6 +1204,12 @@ static void xilinx_cdma_start_transfer(struct
>xilinx_dma_chan *chan)
>>  }
>>
>>  if (chan->has_sg) {
>> +dma_ctrl_clr(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>> +dma_ctrl_set(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>>  xilinx_write(chan, XILINX_DMA_REG_CURDESC,
>>   head_desc->async_tx.phys);
>>
>> @@ -2052,6 +2058,10 @@ static int xilinx_dma_terminate_all(struct dma_chan
>*dchan)
>>  chan->cyclic = false;
>>  }
>>
>> +if ((chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) &&
>chan->has_sg)
>> +dma_ctrl_clr(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>>  return 0;
>>  }
>>
>> --
>> 2.7.4
>>
>
>--
>~Vinod

RE: [LINUX PATCH 2/4] dmaengine: xilinx_dma: Fix race condition in the driver for cdma

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 

>
>On Thu, Dec 21, 2017 at 03:41:36PM +0530, Kedareswara rao Appana wrote:
>
>same issue for patch title here too

Ok will fix in v2... 

>
>> when hardware is idle we need to toggle the SG bit in the control
>> register, inorder to update new value to the current descriptor
>> register other wise undefined results will occur.
>
>can you try making it bit more clear..

Sure will fix in v2... 

Regards,
Kedar.

>
>>
>> This patch updates the same.
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 21ac954..8467671 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1204,6 +1204,12 @@ static void xilinx_cdma_start_transfer(struct
>xilinx_dma_chan *chan)
>>  }
>>
>>  if (chan->has_sg) {
>> +dma_ctrl_clr(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>> +dma_ctrl_set(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>>  xilinx_write(chan, XILINX_DMA_REG_CURDESC,
>>   head_desc->async_tx.phys);
>>
>> @@ -2052,6 +2058,10 @@ static int xilinx_dma_terminate_all(struct dma_chan
>*dchan)
>>  chan->cyclic = false;
>>  }
>>
>> +if ((chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) &&
>chan->has_sg)
>> +dma_ctrl_clr(chan, XILINX_DMA_REG_DMACR,
>> + XILINX_CDMA_CR_SGMODE);
>> +
>>  return 0;
>>  }
>>
>> --
>> 2.7.4
>>
>
>--
>~Vinod

RE: [LINUX PATCH 1/4] dmaengine: xilinx_dma: Fix dma_get_slave_caps() API failures

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 
>
>On Thu, Dec 21, 2017 at 03:41:35PM +0530, Kedareswara rao Appana wrote:
>
>Patch title should say what is does, not the cause/effect

Sure will fix in v2... 

>
>An apt title might be "populate dma caps properly"
>
>> When client driver uses dma_get_slave_caps() api, it checks for
>> certain fields of dma_device struct currently driver is not settings
>> few fields resulting
>> dma_get_slave_caps() returning failure.
>
>It would help to mention the fields you are setting here

Sure will fix in v2... 

Regards,
Kedar.

>
>>
>> This patch fixes this issue by populating proper values to the struct
>> dma_device fields.
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 88d317d..21ac954 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -2398,6 +2398,7 @@ static int xilinx_dma_chan_probe(struct
>xilinx_dma_device *xdev,
>>  chan->direction = DMA_MEM_TO_DEV;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id;
>> +xdev->common.directions = BIT(DMA_MEM_TO_DEV);
>>
>>  chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2415,6
>> +2416,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>>  chan->direction = DMA_DEV_TO_MEM;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id - xdev->nr_channels;
>> +xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
>>
>>  chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2629,6
>> +2631,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>>  dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
>>  }
>>
>> +xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> +xdev->common.src_addr_widths = BIT(addr_width / 8);
>>  xdev->common.device_alloc_chan_resources =
>>  xilinx_dma_alloc_chan_resources;
>>  xdev->common.device_free_chan_resources =
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine"
>> in the body of a message to majord...@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>
>--
>~Vinod

RE: [LINUX PATCH 1/4] dmaengine: xilinx_dma: Fix dma_get_slave_caps() API failures

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review... 
>
>On Thu, Dec 21, 2017 at 03:41:35PM +0530, Kedareswara rao Appana wrote:
>
>Patch title should say what is does, not the cause/effect

Sure will fix in v2... 

>
>An apt title might be "populate dma caps properly"
>
>> When client driver uses dma_get_slave_caps() api, it checks for
>> certain fields of dma_device struct currently driver is not settings
>> few fields resulting
>> dma_get_slave_caps() returning failure.
>
>It would help to mention the fields you are setting here

Sure will fix in v2... 

Regards,
Kedar.

>
>>
>> This patch fixes this issue by populating proper values to the struct
>> dma_device fields.
>>
>> Signed-off-by: Kedareswara rao Appana 
>> ---
>>  drivers/dma/xilinx/xilinx_dma.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c
>> b/drivers/dma/xilinx/xilinx_dma.c index 88d317d..21ac954 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -2398,6 +2398,7 @@ static int xilinx_dma_chan_probe(struct
>xilinx_dma_device *xdev,
>>  chan->direction = DMA_MEM_TO_DEV;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id;
>> +xdev->common.directions = BIT(DMA_MEM_TO_DEV);
>>
>>  chan->ctrl_offset = XILINX_DMA_MM2S_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2415,6
>> +2416,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
>>  chan->direction = DMA_DEV_TO_MEM;
>>  chan->id = chan_id;
>>  chan->tdest = chan_id - xdev->nr_channels;
>> +xdev->common.directions |= BIT(DMA_DEV_TO_MEM);
>>
>>  chan->ctrl_offset = XILINX_DMA_S2MM_CTRL_OFFSET;
>>  if (xdev->dma_config->dmatype == XDMA_TYPE_VDMA) { @@ -
>2629,6
>> +2631,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>>  dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
>>  }
>>
>> +xdev->common.dst_addr_widths = BIT(addr_width / 8);
>> +xdev->common.src_addr_widths = BIT(addr_width / 8);
>>  xdev->common.device_alloc_chan_resources =
>>  xilinx_dma_alloc_chan_resources;
>>  xdev->common.device_free_chan_resources =
>> --
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine"
>> in the body of a message to majord...@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>
>--
>~Vinod

RE: [PATCH v7 1/6] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the review...


>
>On Thu, Dec 07, 2017 at 10:51:02AM +0530, Kedareswara rao Appana wrote:
>
>> @@ -2029,6 +2006,7 @@ static int xilinx_dma_terminate_all(struct
>> dma_chan *dchan)
>>
>>  /* Remove and free all of the descriptors in the lists */
>>  xilinx_dma_free_descriptors(chan);
>> +chan->idle = true;
>>
>>  if (chan->cyclic) {
>>  reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR); @@ -
>2344,6
>> +2322,12 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device
>*xdev,
>>  chan->has_sg = xdev->has_sg;
>>  chan->desc_pendingcount = 0x0;
>>  chan->ext_addr = xdev->ext_addr;
>> +/* This variable enusres that descripotrs are not
> ^^
>
>typo

Since this patch got applied 
Will take care of this next time on wards... 
Thanks for the review... 

Regards,
Kedar.

>
>--
>~Vinod

RE: [PATCH v7 1/6] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi,

Thanks for the review...


>
>On Thu, Dec 07, 2017 at 10:51:02AM +0530, Kedareswara rao Appana wrote:
>
>> @@ -2029,6 +2006,7 @@ static int xilinx_dma_terminate_all(struct
>> dma_chan *dchan)
>>
>>  /* Remove and free all of the descriptors in the lists */
>>  xilinx_dma_free_descriptors(chan);
>> +chan->idle = true;
>>
>>  if (chan->cyclic) {
>>  reg = dma_ctrl_read(chan, XILINX_DMA_REG_DMACR); @@ -
>2344,6
>> +2322,12 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device
>*xdev,
>>  chan->has_sg = xdev->has_sg;
>>  chan->desc_pendingcount = 0x0;
>>  chan->ext_addr = xdev->ext_addr;
>> +/* This variable enusres that descripotrs are not
> ^^
>
>typo

Since this patch got applied 
Will take care of this next time on wards... 
Thanks for the review... 

Regards,
Kedar.

>
>--
>~Vinod

RE: [PATCH v7 0/6] dmaengine: xilinx_dma: Bug fixes

[Appana Durga Kedareswara Rao]

Hi Vinod,

>
>On Thu, Dec 07, 2017 at 10:51:01AM +0530, Kedareswara rao Appana wrote:
>> This patch series fixes below bugs in DMA and VDMA IP's
>> ---> Added channel idle checks in the driver before submitting the buffer
>descriptor to h/w.
>> ---> Fixes bug in Multi frame sotres handling in VDMA Fixes issues
>> ---> w.r.to multi frame descriptors submit with AXI DMA S2MM(recv) Side.
>> ---> Fixed kernel doc warnings in the driver.
>> ---> Fixed checkpatch errors in the driver.
>
>Applied after applying this to fix typo. You seriously need a decent spell 
>checker in
>your editor

Thanks Vinod
Sorry for the noise around spell checks.
Will take care of it next time onwards thanks for pointing it out. 

Regards,
Kedar.

>
>-   /* This variable enusres that descripotrs are not
>-* Submited when dma engine is in progress. This variable is
>-* Added to avoid pollling for a bit in the status register to
>+   /* This variable ensures that descriptors are not
>+* Submitted when dma engine is in progress. This variable is
>+* Added to avoid polling for a bit in the status register to
>
>--
>~Vinod
>--
>To unsubscribe from this list: send the line "unsubscribe dmaengine" in the 
>body
>of a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH v7 0/6] dmaengine: xilinx_dma: Bug fixes

[Appana Durga Kedareswara Rao]

Hi Vinod,

>
>On Thu, Dec 07, 2017 at 10:51:01AM +0530, Kedareswara rao Appana wrote:
>> This patch series fixes below bugs in DMA and VDMA IP's
>> ---> Added channel idle checks in the driver before submitting the buffer
>descriptor to h/w.
>> ---> Fixes bug in Multi frame sotres handling in VDMA Fixes issues
>> ---> w.r.to multi frame descriptors submit with AXI DMA S2MM(recv) Side.
>> ---> Fixed kernel doc warnings in the driver.
>> ---> Fixed checkpatch errors in the driver.
>
>Applied after applying this to fix typo. You seriously need a decent spell 
>checker in
>your editor

Thanks Vinod
Sorry for the noise around spell checks.
Will take care of it next time onwards thanks for pointing it out. 

Regards,
Kedar.

>
>-   /* This variable enusres that descripotrs are not
>-* Submited when dma engine is in progress. This variable is
>-* Added to avoid pollling for a bit in the status register to
>+   /* This variable ensures that descriptors are not
>+* Submitted when dma engine is in progress. This variable is
>+* Added to avoid polling for a bit in the status register to
>
>--
>~Vinod
>--
>To unsubscribe from this list: send the line "unsubscribe dmaengine" in the 
>body
>of a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Mike Looijmans,

Thanks for the review...
Sorry for the long delay in the reply...
Please find comments inline... 


>On 14-01-17 06:35, Kedareswara rao Appana wrote:
>>  When VDMA is configured for more than one frame in the h/w.
>>  For example h/w is configured for n number of frames, user
>>  Submits n number of frames and triggered the DMA using issue_pending API.
>>
>>  In the current driver flow we are submitting one frame at a time,
>>  But we should submit all the n number of frames at one time
>>  As the h/w is configured for n number of frames.
>
>The hardware can always handle a single frame submission, by using the "park"
>bit. This would make a good "cyclic" implementation too (using vdma as
>framebuffer).
>
>It could also handle all cases for "k" frames where n%k==0 (n is a multiple of
>k) by simply replicating the frame pointers.

Agree with your comments will fix it in the next version.
Somehow didn't get enough time to send next version of the patch series.
Will modify the driver as per your comments and will post next version of the 
patch series at the earliest... 

Regards,
Kedar.

RE: [PATCH v6 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Mike Looijmans,

Thanks for the review...
Sorry for the long delay in the reply...
Please find comments inline... 


>On 14-01-17 06:35, Kedareswara rao Appana wrote:
>>  When VDMA is configured for more than one frame in the h/w.
>>  For example h/w is configured for n number of frames, user
>>  Submits n number of frames and triggered the DMA using issue_pending API.
>>
>>  In the current driver flow we are submitting one frame at a time,
>>  But we should submit all the n number of frames at one time
>>  As the h/w is configured for n number of frames.
>
>The hardware can always handle a single frame submission, by using the "park"
>bit. This would make a good "cyclic" implementation too (using vdma as
>framebuffer).
>
>It could also handle all cases for "k" frames where n%k==0 (n is a multiple of
>k) by simply replicating the frame pointers.

Agree with your comments will fix it in the next version.
Somehow didn't get enough time to send next version of the patch series.
Will modify the driver as per your comments and will post next version of the 
patch series at the earliest... 

Regards,
Kedar.

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

[Snip]
> > > Btw how and when does DMA stop, assuming it is circular it never
> > > would, isn't there a valid/stop flag associated with a descriptor
> > > which tells DMA engine what to do next
> >
> > There are two registers that controls the DMA transfers.
> > Current descriptor and tail descriptor register.
> > When current descriptor reaches tail descriptor dma engine will pause.
> >
> > When reprogramming the tail descriptor the DMA engine will starts fetching
> descriptors again.
> >
> > But with the existing driver flow if we reprogram the tail descriptor
> > The tail descriptor next descriptor field is pointing to an invalid
> > location Causing data corruption...
> 
> So the solution is..?

This patch.
I mean if we have a set of descriptors in chain (in circular manner last 
descriptor points to first descriptor)
It always points to valid descriptors. 

Will update the patch commit description in the next version...

> 
> > > Btw there is something wrong with your MUA perhaps line are
> > > titlecased for no reason. This is typically behavious of non linux
> > > tool which may not be great tool for this work.
> >
> > Thanks for pointing it out.
> > I usually replies from outlook from a windows machine.
> > Will check with others in my team how they configured their mail client.
> 
> Yeah that isnt right tool for the job. See Documentation/process/email-
> clients.rst
> 
> FWIW, I use mutt, vim as editor with exchange servers, seems to work well for
> me now.

Thanks for pointing it out will go through it.
Will install mutt in my Linux machine will start replying from that

Regards,
Kedar.

> 
> --
> ~Vinod

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

[Snip]
> > > Btw how and when does DMA stop, assuming it is circular it never
> > > would, isn't there a valid/stop flag associated with a descriptor
> > > which tells DMA engine what to do next
> >
> > There are two registers that controls the DMA transfers.
> > Current descriptor and tail descriptor register.
> > When current descriptor reaches tail descriptor dma engine will pause.
> >
> > When reprogramming the tail descriptor the DMA engine will starts fetching
> descriptors again.
> >
> > But with the existing driver flow if we reprogram the tail descriptor
> > The tail descriptor next descriptor field is pointing to an invalid
> > location Causing data corruption...
> 
> So the solution is..?

This patch.
I mean if we have a set of descriptors in chain (in circular manner last 
descriptor points to first descriptor)
It always points to valid descriptors. 

Will update the patch commit description in the next version...

> 
> > > Btw there is something wrong with your MUA perhaps line are
> > > titlecased for no reason. This is typically behavious of non linux
> > > tool which may not be great tool for this work.
> >
> > Thanks for pointing it out.
> > I usually replies from outlook from a windows machine.
> > Will check with others in my team how they configured their mail client.
> 
> Yeah that isnt right tool for the job. See Documentation/process/email-
> clients.rst
> 
> FWIW, I use mutt, vim as editor with exchange servers, seems to work well for
> me now.

Thanks for pointing it out will go through it.
Will install mutt in my Linux machine will start replying from that

Regards,
Kedar.

> 
> --
> ~Vinod

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

[Snip]
> >
> > > On Sat, Jan 07, 2017 at 12:15:30PM +0530, Kedareswara rao Appana wrote:
> > > > When driver is handling AXI DMA SoftIP When user submits multiple
> > > > descriptors back to back on the S2MM(recv) side with the current
> > > > driver flow the last buffer descriptor next bd points to a invalid
> > > > location resulting the invalid data or errors in the DMA engine.
> > >
> > > Can you rephrase this, it a bit hard to understand.
> >
> > When DMA is receiving packets h/w expects the descriptors Should be in
> > the form of a ring (I mean h/w buffer descriptor Next descriptor field
> > should always point to valid address So that when DMA engine go and
> > fetch that next descriptor it always Sees a valid address).
> >
> >
> > But with the current driver implementation when user queues Multiple
> > descriptors the last descriptor next descriptor field Pointing to an
> > invalid location causing data corruption or Errors from the DMA h/w
> > engine...
> >
> > To avoid this issue creating a Buffer descriptor Chain during Channel
> > allocation and using those buffer descriptors for processing User
> > requested data.
> 
> Is it not doable to to modify the next pointer to point to subsequent 
> transaction.
> IOW you are modifying tail descriptor to point to subsequent descriptor.
> 
> Btw how and when does DMA stop, assuming it is circular it never would, isn't
> there a valid/stop flag associated with a descriptor which tells DMA engine 
> what
> to do next

There are two registers that controls the DMA transfers.
Current descriptor and tail descriptor register. 
When current descriptor reaches tail descriptor dma engine will pause.

When reprogramming the tail descriptor the DMA engine will starts fetching 
descriptors again.

But with the existing driver flow if we reprogram the tail descriptor
The tail descriptor next descriptor field is pointing to an invalid location
Causing data corruption...

> 
> 
> Btw there is something wrong with your MUA perhaps line are titlecased for no
> reason. This is typically behavious of non linux tool which may not be great 
> tool
> for this work.

Thanks for pointing it out.
I usually replies from outlook from a windows machine.
Will check with others in my team how they configured their mail client.

> 
> >
> > Please let me know if the above explanation is not clear will explain in 
> > detail
> >
> > >
> > > >
> > > > This patch fixes this issue by creating a BD Chain during
> > >
> > > whats a BD?
> >
> > Buffer descriptor.
> 
> Thats nowhere mentioned..

Yep sorry I should have been mentioned it...

Regards,
Kedar.

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

[Snip]
> >
> > > On Sat, Jan 07, 2017 at 12:15:30PM +0530, Kedareswara rao Appana wrote:
> > > > When driver is handling AXI DMA SoftIP When user submits multiple
> > > > descriptors back to back on the S2MM(recv) side with the current
> > > > driver flow the last buffer descriptor next bd points to a invalid
> > > > location resulting the invalid data or errors in the DMA engine.
> > >
> > > Can you rephrase this, it a bit hard to understand.
> >
> > When DMA is receiving packets h/w expects the descriptors Should be in
> > the form of a ring (I mean h/w buffer descriptor Next descriptor field
> > should always point to valid address So that when DMA engine go and
> > fetch that next descriptor it always Sees a valid address).
> >
> >
> > But with the current driver implementation when user queues Multiple
> > descriptors the last descriptor next descriptor field Pointing to an
> > invalid location causing data corruption or Errors from the DMA h/w
> > engine...
> >
> > To avoid this issue creating a Buffer descriptor Chain during Channel
> > allocation and using those buffer descriptors for processing User
> > requested data.
> 
> Is it not doable to to modify the next pointer to point to subsequent 
> transaction.
> IOW you are modifying tail descriptor to point to subsequent descriptor.
> 
> Btw how and when does DMA stop, assuming it is circular it never would, isn't
> there a valid/stop flag associated with a descriptor which tells DMA engine 
> what
> to do next

There are two registers that controls the DMA transfers.
Current descriptor and tail descriptor register. 
When current descriptor reaches tail descriptor dma engine will pause.

When reprogramming the tail descriptor the DMA engine will starts fetching 
descriptors again.

But with the existing driver flow if we reprogram the tail descriptor
The tail descriptor next descriptor field is pointing to an invalid location
Causing data corruption...

> 
> 
> Btw there is something wrong with your MUA perhaps line are titlecased for no
> reason. This is typically behavious of non linux tool which may not be great 
> tool
> for this work.

Thanks for pointing it out.
I usually replies from outlook from a windows machine.
Will check with others in my team how they configured their mail client.

> 
> >
> > Please let me know if the above explanation is not clear will explain in 
> > detail
> >
> > >
> > > >
> > > > This patch fixes this issue by creating a BD Chain during
> > >
> > > whats a BD?
> >
> > Buffer descriptor.
> 
> Thats nowhere mentioned..

Yep sorry I should have been mentioned it...

Regards,
Kedar.

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao
> wrote:
> > Hi Vinod,
> >
> > Thanks for the review...
> > >
> > > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > > Add channel idle state to ensure that dma descriptor is not
> > > > submitted when VDMA engine is in progress.
> > >
> > > any reason why you want to make your own varible and not use the HW
> > > to query as done earlier. It is not clear to me why that is removed
> > > from description
> >
> > We need to poll for a bit in the status register to know the dma state.
> > We are currently doing that in the driver hot path To avoid this using
> > own variables.
> 
> It would be worthwhile to document these, down the line people may not
> remeber the motivation

Sure will add comments during the variable initialization
In the driver...

Regards,
Kedar.

> 
> 
> --
> ~Vinod

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Fri, Jan 13, 2017 at 04:28:11AM +, Appana Durga Kedareswara Rao
> wrote:
> > Hi Vinod,
> >
> > Thanks for the review...
> > >
> > > On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > > > Add channel idle state to ensure that dma descriptor is not
> > > > submitted when VDMA engine is in progress.
> > >
> > > any reason why you want to make your own varible and not use the HW
> > > to query as done earlier. It is not clear to me why that is removed
> > > from description
> >
> > We need to poll for a bit in the status register to know the dma state.
> > We are currently doing that in the driver hot path To avoid this using
> > own variables.
> 
> It would be worthwhile to document these, down the line people may not
> remeber the motivation

Sure will add comments during the variable initialization
In the driver...

Regards,
Kedar.

> 
> 
> --
> ~Vinod

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > Add channel idle state to ensure that dma descriptor is not
> > submitted when VDMA engine is in progress.
> 
> any reason why you want to make your own varible and not use the HW to
> query
> as done earlier. It is not clear to me why that is removed from description

We need to poll for a bit in the status register to know the dma state.
We are currently doing that in the driver hot path
To avoid this using own variables.

Regards,
Kedar.

RE: [PATCH v5 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...
> 
> On Sat, Jan 07, 2017 at 12:15:28PM +0530, Kedareswara rao Appana wrote:
> > Add channel idle state to ensure that dma descriptor is not
> > submitted when VDMA engine is in progress.
> 
> any reason why you want to make your own varible and not use the HW to
> query
> as done earlier. It is not clear to me why that is removed from description

We need to poll for a bit in the status register to know the dma state.
We are currently doing that in the driver hot path
To avoid this using own variables.

Regards,
Kedar.

RE: [PATCH v5 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...  

> 
> On Sat, Jan 07, 2017 at 12:15:29PM +0530, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> 
> title case in middle if sentence, no commas, can you make it easier to read
> please..

Ok sure will fix commit message in the next version.

> 
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> 
> s/Is/is

Ok sure will fix in the next version

Regards,
Kedar.

RE: [PATCH v5 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...  

> 
> On Sat, Jan 07, 2017 at 12:15:29PM +0530, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> 
> title case in middle if sentence, no commas, can you make it easier to read
> please..

Ok sure will fix commit message in the next version.

> 
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> 
> s/Is/is

Ok sure will fix in the next version

Regards,
Kedar.

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

> On Sat, Jan 07, 2017 at 12:15:30PM +0530, Kedareswara rao Appana wrote:
> > When driver is handling AXI DMA SoftIP When user submits multiple
> > descriptors back to back on the S2MM(recv) side with the current
> > driver flow the last buffer descriptor next bd points to a invalid
> > location resulting the invalid data or errors in the DMA engine.
> 
> Can you rephrase this, it a bit hard to understand.

When DMA is receiving packets h/w expects the descriptors
Should be in the form of a ring (I mean h/w buffer descriptor
Next descriptor field should always point to valid address
So that when DMA engine go and fetch that next descriptor it always 
Sees a valid address).

But with the current driver implementation when user queues
Multiple descriptors the last descriptor next descriptor field
Pointing to an invalid location causing data corruption or 
Errors from the DMA h/w engine...

To avoid this issue creating a Buffer descriptor Chain during 
Channel allocation and using those buffer descriptors for processing
User requested data.

Please let me know if the above explanation is not clear will explain in 
detail

> 
> >
> > This patch fixes this issue by creating a BD Chain during
> 
> whats a BD?

Buffer descriptor.

> 
> > channel allocation itself and use those BD's.
> >
> > Signed-off-by: Kedareswara rao Appana 
> > ---
> >
> >  drivers/dma/xilinx/xilinx_dma.c | 133
> > +---
> >  1 file changed, 83 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c index 0e9c02e..af2159d 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -163,6 +163,7 @@
> >  #define XILINX_DMA_BD_SOP  BIT(27)
> >  #define XILINX_DMA_BD_EOP  BIT(26)
> >  #define XILINX_DMA_COALESCE_MAX255
> > +#define XILINX_DMA_NUM_DESCS   255
> 
> why 255?

It is not an h/w limitation 
Allocating 255 descriptors (Each descriptor is capable of sending 7MB data)
So roughly using allocated descriptors DMA engine can transfer 1GB data
And in the driver we are reusing the allocated descriptors when they are free.

Regards,
Kedar.

RE: [PATCH v5 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario

[Appana Durga Kedareswara Rao]

Hi Vinod,

Thanks for the review...

> On Sat, Jan 07, 2017 at 12:15:30PM +0530, Kedareswara rao Appana wrote:
> > When driver is handling AXI DMA SoftIP When user submits multiple
> > descriptors back to back on the S2MM(recv) side with the current
> > driver flow the last buffer descriptor next bd points to a invalid
> > location resulting the invalid data or errors in the DMA engine.
> 
> Can you rephrase this, it a bit hard to understand.

When DMA is receiving packets h/w expects the descriptors
Should be in the form of a ring (I mean h/w buffer descriptor
Next descriptor field should always point to valid address
So that when DMA engine go and fetch that next descriptor it always 
Sees a valid address).

But with the current driver implementation when user queues
Multiple descriptors the last descriptor next descriptor field
Pointing to an invalid location causing data corruption or 
Errors from the DMA h/w engine...

To avoid this issue creating a Buffer descriptor Chain during 
Channel allocation and using those buffer descriptors for processing
User requested data.

Please let me know if the above explanation is not clear will explain in 
detail

> 
> >
> > This patch fixes this issue by creating a BD Chain during
> 
> whats a BD?

Buffer descriptor.

> 
> > channel allocation itself and use those BD's.
> >
> > Signed-off-by: Kedareswara rao Appana 
> > ---
> >
> >  drivers/dma/xilinx/xilinx_dma.c | 133
> > +---
> >  1 file changed, 83 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/dma/xilinx/xilinx_dma.c
> > b/drivers/dma/xilinx/xilinx_dma.c index 0e9c02e..af2159d 100644
> > --- a/drivers/dma/xilinx/xilinx_dma.c
> > +++ b/drivers/dma/xilinx/xilinx_dma.c
> > @@ -163,6 +163,7 @@
> >  #define XILINX_DMA_BD_SOP  BIT(27)
> >  #define XILINX_DMA_BD_EOP  BIT(26)
> >  #define XILINX_DMA_COALESCE_MAX255
> > +#define XILINX_DMA_NUM_DESCS   255
> 
> why 255?

It is not an h/w limitation 
Allocating 255 descriptors (Each descriptor is capable of sending 7MB data)
So roughly using allocated descriptors DMA engine can transfer 1GB data
And in the driver we are reusing the allocated descriptors when they are free.

Regards,
Kedar.

RE: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Rob,

Thanks for the review...

 [Snip]
> > -- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> >> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> >> > @@ -66,6 +66,8 @@ Optional child node properties:
> >> >  Optional child node properties for VDMA:
> >> >  - xlnx,genlock-mode: Tells Genlock synchronization is
> >> > enabled/disabled in hardware.
> >> > +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
> >> > +   enabled/disabled in hardware.
> >>
> >> What's the default (when not present)? That should be the most common
> case.
> >> Looks like the code treats this as bool, but that's not clear here.
> >> The name is not clear what it is doing. Enabling or disabling the feature?
> >
> > Default value is zero...
> > When this property is present it tells hardware is configured for frame 
> > store
> configuration.
> 
> So most people will not want "frame store configuration"?

Since the driver is for SoftIP (I mean fpga ip) default h/w configuration of 
this IP is frame store
Configuration disabled that's in the driver making default value of this 
configuration as zero.

If users are trying a use case where this configuration should be enabled but 
in the h/w it is disabled.
In this case driver will warn users to enable this frame store configuration in 
the h/w.
So that users can enable it in their h/w.

Please let me know if the above expiation is not clear will explain in detail...

> 
> > Will fix the explanation part in the next version like below.
> >  xlnx,fstore-config: Tells hardware is configured for frame store 
> > configuration.
> > Is the above explanation clear???
> 
> No, I mean make it obvious from the name of the property:
> xlnx,fstore-config-enable or xlnx,fstore-enable
> 
> And the description needs to say it is boolean.

Sure will fix in the next version...

Regards,
Kedar.

> 
> Rob

RE: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Rob,

Thanks for the review...

 [Snip]
> > -- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> >> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> >> > @@ -66,6 +66,8 @@ Optional child node properties:
> >> >  Optional child node properties for VDMA:
> >> >  - xlnx,genlock-mode: Tells Genlock synchronization is
> >> > enabled/disabled in hardware.
> >> > +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
> >> > +   enabled/disabled in hardware.
> >>
> >> What's the default (when not present)? That should be the most common
> case.
> >> Looks like the code treats this as bool, but that's not clear here.
> >> The name is not clear what it is doing. Enabling or disabling the feature?
> >
> > Default value is zero...
> > When this property is present it tells hardware is configured for frame 
> > store
> configuration.
> 
> So most people will not want "frame store configuration"?

Since the driver is for SoftIP (I mean fpga ip) default h/w configuration of 
this IP is frame store
Configuration disabled that's in the driver making default value of this 
configuration as zero.

If users are trying a use case where this configuration should be enabled but 
in the h/w it is disabled.
In this case driver will warn users to enable this frame store configuration in 
the h/w.
So that users can enable it in their h/w.

Please let me know if the above expiation is not clear will explain in detail...

> 
> > Will fix the explanation part in the next version like below.
> >  xlnx,fstore-config: Tells hardware is configured for frame store 
> > configuration.
> > Is the above explanation clear???
> 
> No, I mean make it obvious from the name of the property:
> xlnx,fstore-config-enable or xlnx,fstore-enable
> 
> And the description needs to say it is boolean.

Sure will fix in the next version...

Regards,
Kedar.

> 
> Rob

RE: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Rob,

Thanks for the review

> On Wed, Jan 04, 2017 at 07:05:53PM +0530, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> 
> Please fix run-on sentences, capitalization, and word wrapping.

Sure will fix in the next version

[Snip]
-- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > @@ -66,6 +66,8 @@ Optional child node properties:
> >  Optional child node properties for VDMA:
> >  - xlnx,genlock-mode: Tells Genlock synchronization is
> > enabled/disabled in hardware.
> > +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
> > +   enabled/disabled in hardware.
> 
> What's the default (when not present)? That should be the most common case.
> Looks like the code treats this as bool, but that's not clear here. The name 
> is not
> clear what it is doing. Enabling or disabling the feature?

Default value is zero...
When this property is present it tells hardware is configured for frame store 
configuration.

Will fix the explanation part in the next version like below.
 xlnx,fstore-config: Tells hardware is configured for frame store configuration.
Is the above explanation clear???

Regards,
Kedar.

RE: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi Rob,

Thanks for the review

> On Wed, Jan 04, 2017 at 07:05:53PM +0530, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> 
> Please fix run-on sentences, capitalization, and word wrapping.

Sure will fix in the next version

[Snip]
-- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> > @@ -66,6 +66,8 @@ Optional child node properties:
> >  Optional child node properties for VDMA:
> >  - xlnx,genlock-mode: Tells Genlock synchronization is
> > enabled/disabled in hardware.
> > +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
> > +   enabled/disabled in hardware.
> 
> What's the default (when not present)? That should be the most common case.
> Looks like the code treats this as bool, but that's not clear here. The name 
> is not
> clear what it is doing. Enabling or disabling the feature?

Default value is zero...
When this property is present it tells hardware is configured for frame store 
configuration.

Will fix the explanation part in the next version like below.
 xlnx,fstore-config: Tells hardware is configured for frame store configuration.
Is the above explanation clear???

Regards,
Kedar.

RE: [PATCH v3 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi 

Thanks for the review...
> 
> Hi Kedar,
> 
> 
> On 04-01-2017 06:54, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> >
> > This patch fixes this issue.
> >
> > Signed-off-by: Kedareswara rao Appana 
> 
> Looks fine. I have a couple of minor comments, if you address them you can add
> "Reviewed-by: Jose Abreu "
> in next version.

Thanks...
Sure will fix the comments and will add your' s Reviewed-by

[snip]
> > +   /*
> > +* Note: When VDMA is built with default h/w configuration
> > +* On the S2MM(recv) side user should submit frames upto
> > +* H/W configured. If users submits less than h/w configured
> > +* VDMA engine tries to write to a invalid location
> > +* Results undefined behaviour/memory corruption.
> > +*
> > +* If user would like to submit frames less than h/w capable
> > +* On S2MM side please enable debug info 13 at the h/w level
> > +* It will allows the frame buffers numbers to be modified at runtime.
> > +*/
> > +   if (!chan->has_fstoreconfig && chan->direction == DMA_DEV_TO_MEM
> &&
> > +   chan->desc_pendingcount < chan->num_frms) {
> > +   dev_dbg(chan->dev, "Frame Store Configuration is not enabled
> at the");
> > +   dev_dbg(chan->dev, " H/w level enable Debug info 13 at the
> h/w level");
> > +   dev_dbg(chan->dev, " OR Submit the frames upto h/w
> Capable\n\r");
> > +
> > +   return;
> > +   }
> 
> Hmm, may dev_warn would be more suitable because with dev_dbg and no
> dynamic debug enabled user will not know what happened. Also, I am aware
> that in direction DMA_MEM_TO_DEV there will be no corruption in PC side but it
> will be corruption in VDMA side because it will read from invalid memory
> locations. Maybe drop the check for channel direction.

Sure will fix it in the next version

> 
> I am also not fancy about dropping prints that are not grep'able (you do not
> break line in each print so a user searching for the whole string will not 
> find it).
> Try to do a line break in each print or change the string to be smaller.
> 

Sure will add a line break in each print in the next version...

Regards,
Kedar.

> Best regards,
> Jose Miguel Abreu
>