dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma
@ 2021-05-18 13:23 Olivier Dautricourt
  2021-05-18 13:24 ` [PATCH v5 2/3] MAINTAINERS: add entry for Altera mSGDMA Olivier Dautricourt
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Olivier Dautricourt @ 2021-05-18 13:23 UTC (permalink / raw)
  To: Rob Herring, Vinod Koul, Stefan Roese
  Cc: Olivier Dautricourt, dmaengine, devicetree, linux-kernel

add yaml schema for Altera mSGDMA bindings in devicetree.

Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
---

Notes:
    Changes in v2:
     - fix reg size in dt example
     - fix dt_binding check warning
     - add list in MAINTAINERS entry

    Changes from v2 to v3:
     none

    Changes from v3 to v4:
     none

    Changes from v4 to v5:
        as per Rob's comments:
            - change compatible field from 'altr,msgdma' to
              'altr,socfpga-msgdma' to indicate that it's compatible
               with altera socfpga family.
            - describe each region separately
            - remove maxItems/minItems for reg section.
        as per Vinod's comments:
            - separate MAINTAINERS editing in another commit
            - remove description for #dma-cells

 .../devicetree/bindings/dma/altr,msgdma.yaml  | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/altr,msgdma.yaml

diff --git a/Documentation/devicetree/bindings/dma/altr,msgdma.yaml b/Documentation/devicetree/bindings/dma/altr,msgdma.yaml
new file mode 100644
index 000000000000..ce51531e8736
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/altr,msgdma.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/altr,msgdma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Altera mSGDMA IP core
+
+maintainers:
+  - Olivier Dautricourt <olivier.dautricourt@orolia.com>
+
+description: |
+  Altera / Intel modular Scatter-Gather Direct Memory Access (mSGDMA)
+  intellectual property (IP)
+
+allOf:
+  - $ref: "dma-controller.yaml#"
+
+properties:
+  compatible:
+    const: altr,socfpga-msgdma
+
+  reg:
+    items:
+      - description: Control and Status Register Slave Port
+      - description: Descriptor Slave Port
+      - description: Response Slave Port
+
+  reg-names:
+    items:
+      - const: csr
+      - const: desc
+      - const: resp
+
+  interrupts:
+    maxItems: 1
+
+  "#dma-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    msgdma_controller: dma-controller@ff200b00 {
+        compatible = "altr,socfpga-msgdma";
+        reg = <0xff200b00 0x100>, <0xff200c00 0x100>, <0xff200d00 0x100>;
+        reg-names = "csr", "desc", "resp";
+        interrupts = <0 67 IRQ_TYPE_LEVEL_HIGH>;
+        #dma-cells = <1>;
+    };
--
2.31.0.rc2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 2/3] MAINTAINERS: add entry for Altera mSGDMA
  2021-05-18 13:23 [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Olivier Dautricourt
@ 2021-05-18 13:24 ` Olivier Dautricourt
  2021-05-18 13:25 ` [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support Olivier Dautricourt
  2021-05-19 22:11 ` [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Rob Herring
  2 siblings, 0 replies; 9+ messages in thread
From: Olivier Dautricourt @ 2021-05-18 13:24 UTC (permalink / raw)
  To: Rob Herring, Vinod Koul, Stefan Roese
  Cc: Olivier Dautricourt, dmaengine, devicetree, linux-kernel

This entry is for the standalone driver in drivers/dma/altera-msgdma.c
Add myself as 'Odd fixes' maintainer for this driver as i am currently
writing new code and have access to the hardware.

Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
---

Notes:
    splitted commit, introduced in v5

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 008fcad7ac00..ccccce849b27 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -783,6 +783,13 @@ M:	Ley Foon Tan <ley.foon.tan@intel.com>
 S:	Maintained
 F:	drivers/mailbox/mailbox-altera.c

+ALTERA MSGDMA IP CORE DRIVER
+M:	Olivier Dautricourt <olivier.dautricourt@orolia.com>
+L:	dmaengine@vger.kernel.org
+S:	Odd Fixes
+F:	Documentation/devicetree/bindings/dma/altr,msgdma.yaml
+F:	drivers/dma/altera-msgdma.c
+
 ALTERA PIO DRIVER
 M:	Joyce Ooi <joyce.ooi@intel.com>
 L:	linux-gpio@vger.kernel.org
--
2.31.0.rc2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-05-18 13:23 [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Olivier Dautricourt
  2021-05-18 13:24 ` [PATCH v5 2/3] MAINTAINERS: add entry for Altera mSGDMA Olivier Dautricourt
@ 2021-05-18 13:25 ` Olivier Dautricourt
  2021-06-07  6:59   ` Vinod Koul
  2021-05-19 22:11 ` [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Rob Herring
  2 siblings, 1 reply; 9+ messages in thread
From: Olivier Dautricourt @ 2021-05-18 13:25 UTC (permalink / raw)
  To: Rob Herring, Vinod Koul, Stefan Roese
  Cc: Olivier Dautricourt, dmaengine, devicetree, linux-kernel

This driver had no device tree support.

- add compatible field "altr,socfpga-msgdma"
- define msgdma_of_xlate, with no argument
- register dma controller with of_dma_controller_register

Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
---

Notes:
    Changes in v2:
        none

    Changes from v2 to v3:
        Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
        only once.

    Changes from v3 to v4
        Reintroduce #ifdef CONFIG_OF for msgdma_match
        as it produces a unused variable warning

    Changes from v4 to v5
        - As per Rob's comments on patch 1/2:
          change compatible field from altr,msgdma to
          altr,socfpga-msgdma.
        - change commit title to fit previous commits naming
        - As per Vinod's comments:
          - use dma_get_slave_channel instead of dma_get_any_slave_channel which
            makes more sense.
          - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
            as it is taken care by the core

 drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index 9a841ce5f0c5..acf0990d73ae 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of_dma.h>

 #include "dmaengine.h"

@@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
 	return 0;
 }

+static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
+					struct of_dma *ofdma)
+{
+	struct msgdma_device *d = ofdma->of_dma_data;
+
+	return dma_get_slave_channel(&d->dmachan);
+}
+
 /**
  * msgdma_probe - Driver probe function
  * @pdev: Pointer to the platform_device structure
@@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;

+	ret = of_dma_controller_register(pdev->dev.of_node,
+					 msgdma_of_xlate, mdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register dma controller");
+		goto fail;
+	}
+
 	dev_notice(&pdev->dev, "Altera mSGDMA driver probe success\n");

 	return 0;
@@ -916,9 +932,19 @@ static int msgdma_remove(struct platform_device *pdev)
 	return 0;
 }

+#ifdef CONFIG_OF
+static const struct of_device_id msgdma_match[] = {
+	{ .compatible = "altr,socfpga-msgdma", },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, msgdma_match);
+#endif
+
 static struct platform_driver msgdma_driver = {
 	.driver = {
 		.name = "altera-msgdma",
+		.of_match_table = of_match_ptr(msgdma_match),
 	},
 	.probe = msgdma_probe,
 	.remove = msgdma_remove,
--
2.31.0.rc2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma
  2021-05-18 13:23 [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Olivier Dautricourt
  2021-05-18 13:24 ` [PATCH v5 2/3] MAINTAINERS: add entry for Altera mSGDMA Olivier Dautricourt
  2021-05-18 13:25 ` [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support Olivier Dautricourt
@ 2021-05-19 22:11 ` Rob Herring
  2 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2021-05-19 22:11 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Rob Herring, devicetree, Vinod Koul, dmaengine, linux-kernel,
	Stefan Roese

On Tue, 18 May 2021 15:23:34 +0200, Olivier Dautricourt wrote:
> add yaml schema for Altera mSGDMA bindings in devicetree.
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> ---
> 
> Notes:
>     Changes in v2:
>      - fix reg size in dt example
>      - fix dt_binding check warning
>      - add list in MAINTAINERS entry
> 
>     Changes from v2 to v3:
>      none
> 
>     Changes from v3 to v4:
>      none
> 
>     Changes from v4 to v5:
>         as per Rob's comments:
>             - change compatible field from 'altr,msgdma' to
>               'altr,socfpga-msgdma' to indicate that it's compatible
>                with altera socfpga family.
>             - describe each region separately
>             - remove maxItems/minItems for reg section.
>         as per Vinod's comments:
>             - separate MAINTAINERS editing in another commit
>             - remove description for #dma-cells
> 
>  .../devicetree/bindings/dma/altr,msgdma.yaml  | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/altr,msgdma.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-05-18 13:25 ` [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support Olivier Dautricourt
@ 2021-06-07  6:59   ` Vinod Koul
  2021-06-07  8:28     ` Olivier Dautricourt
  0 siblings, 1 reply; 9+ messages in thread
From: Vinod Koul @ 2021-06-07  6:59 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Rob Herring, Stefan Roese, dmaengine, devicetree, linux-kernel

On 18-05-21, 15:25, Olivier Dautricourt wrote:
> This driver had no device tree support.
> 
> - add compatible field "altr,socfpga-msgdma"
> - define msgdma_of_xlate, with no argument
> - register dma controller with of_dma_controller_register
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> ---
> 
> Notes:
>     Changes in v2:
>         none
> 
>     Changes from v2 to v3:
>         Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
>         only once.
> 
>     Changes from v3 to v4
>         Reintroduce #ifdef CONFIG_OF for msgdma_match
>         as it produces a unused variable warning
> 
>     Changes from v4 to v5
>         - As per Rob's comments on patch 1/2:
>           change compatible field from altr,msgdma to
>           altr,socfpga-msgdma.
>         - change commit title to fit previous commits naming
>         - As per Vinod's comments:
>           - use dma_get_slave_channel instead of dma_get_any_slave_channel which
>             makes more sense.
>           - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
>             as it is taken care by the core
> 
>  drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> index 9a841ce5f0c5..acf0990d73ae 100644
> --- a/drivers/dma/altera-msgdma.c
> +++ b/drivers/dma/altera-msgdma.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/of_dma.h>
> 
>  #include "dmaengine.h"
> 
> @@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
>  	return 0;
>  }
> 
> +static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
> +					struct of_dma *ofdma)
> +{
> +	struct msgdma_device *d = ofdma->of_dma_data;
> +
> +	return dma_get_slave_channel(&d->dmachan);
> +}

Why not use of_dma_simple_xlate() instead?

> +
>  /**
>   * msgdma_probe - Driver probe function
>   * @pdev: Pointer to the platform_device structure
> @@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail;
> 
> +	ret = of_dma_controller_register(pdev->dev.of_node,
> +					 msgdma_of_xlate, mdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register dma controller");
> +		goto fail;

Should this be treated as an error.. the probe will be invoked on non of
systems too..

-- 
~Vinod

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-06-07  6:59   ` Vinod Koul
@ 2021-06-07  8:28     ` Olivier Dautricourt
  2021-06-07 10:08       ` Vinod Koul
  0 siblings, 1 reply; 9+ messages in thread
From: Olivier Dautricourt @ 2021-06-07  8:28 UTC (permalink / raw)
  To: Vinod Koul; +Cc: Rob Herring, Stefan Roese, dmaengine, devicetree, linux-kernel

The 06/07/2021 12:29, Vinod Koul wrote:
> On 18-05-21, 15:25, Olivier Dautricourt wrote:
> > This driver had no device tree support.
> >
> > - add compatible field "altr,socfpga-msgdma"
> > - define msgdma_of_xlate, with no argument
> > - register dma controller with of_dma_controller_register
> >
> > Reviewed-by: Stefan Roese <sr@denx.de>
> > Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> > ---
> >
> > Notes:
> >     Changes in v2:
> >         none
> >
> >     Changes from v2 to v3:
> >         Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
> >         only once.
> >
> >     Changes from v3 to v4
> >         Reintroduce #ifdef CONFIG_OF for msgdma_match
> >         as it produces a unused variable warning
> >
> >     Changes from v4 to v5
> >         - As per Rob's comments on patch 1/2:
> >           change compatible field from altr,msgdma to
> >           altr,socfpga-msgdma.
> >         - change commit title to fit previous commits naming
> >         - As per Vinod's comments:
> >           - use dma_get_slave_channel instead of dma_get_any_slave_channel which
> >             makes more sense.
> >           - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
> >             as it is taken care by the core
> >
> >  drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> > index 9a841ce5f0c5..acf0990d73ae 100644
> > --- a/drivers/dma/altera-msgdma.c
> > +++ b/drivers/dma/altera-msgdma.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > +#include <linux/of_dma.h>
> >
> >  #include "dmaengine.h"
> >
> > @@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
> >       return 0;
> >  }
> >
> > +static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
> > +                                     struct of_dma *ofdma)
> > +{
> > +     struct msgdma_device *d = ofdma->of_dma_data;
> > +
> > +     return dma_get_slave_channel(&d->dmachan);
> > +}
>
> Why not use of_dma_simple_xlate() instead?
I guess i could, but i don't think i need to define a filter function,
also there is only one possible channel.
>
> > +
> >  /**
> >   * msgdma_probe - Driver probe function
> >   * @pdev: Pointer to the platform_device structure
> > @@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
> >       if (ret)
> >               goto fail;
> >
> > +     ret = of_dma_controller_register(pdev->dev.of_node,
> > +                                      msgdma_of_xlate, mdev);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to register dma controller");
> > +             goto fail;
>
> Should this be treated as an error.. the probe will be invoked on non of
> systems too..
Ok, i'm a bit confused,
in v4 those lines were enclosed with 'if (IS_ENABLED(CONFIG_OF)) { }'
when you said to me that it was already taken care by the core i though
that of_dma_controller_register will return 0 on non-of systems.
Now i can add back IS_ENABLED(CONFIG_OF) or discard the ret value.

>
> --
> ~Vinod

Thanks,
--
Olivier


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-06-07  8:28     ` Olivier Dautricourt
@ 2021-06-07 10:08       ` Vinod Koul
  2021-06-07 10:45         ` Olivier Dautricourt
  0 siblings, 1 reply; 9+ messages in thread
From: Vinod Koul @ 2021-06-07 10:08 UTC (permalink / raw)
  To: Olivier Dautricourt
  Cc: Rob Herring, Stefan Roese, dmaengine, devicetree, linux-kernel

On 07-06-21, 10:28, Olivier Dautricourt wrote:
> The 06/07/2021 12:29, Vinod Koul wrote:
> > On 18-05-21, 15:25, Olivier Dautricourt wrote:
> > > This driver had no device tree support.
> > >
> > > - add compatible field "altr,socfpga-msgdma"
> > > - define msgdma_of_xlate, with no argument
> > > - register dma controller with of_dma_controller_register
> > >
> > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> > > ---
> > >
> > > Notes:
> > >     Changes in v2:
> > >         none
> > >
> > >     Changes from v2 to v3:
> > >         Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
> > >         only once.
> > >
> > >     Changes from v3 to v4
> > >         Reintroduce #ifdef CONFIG_OF for msgdma_match
> > >         as it produces a unused variable warning
> > >
> > >     Changes from v4 to v5
> > >         - As per Rob's comments on patch 1/2:
> > >           change compatible field from altr,msgdma to
> > >           altr,socfpga-msgdma.
> > >         - change commit title to fit previous commits naming
> > >         - As per Vinod's comments:
> > >           - use dma_get_slave_channel instead of dma_get_any_slave_channel which
> > >             makes more sense.
> > >           - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
> > >             as it is taken care by the core
> > >
> > >  drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> > > index 9a841ce5f0c5..acf0990d73ae 100644
> > > --- a/drivers/dma/altera-msgdma.c
> > > +++ b/drivers/dma/altera-msgdma.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/of_dma.h>
> > >
> > >  #include "dmaengine.h"
> > >
> > > @@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
> > >       return 0;
> > >  }
> > >
> > > +static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
> > > +                                     struct of_dma *ofdma)
> > > +{
> > > +     struct msgdma_device *d = ofdma->of_dma_data;
> > > +
> > > +     return dma_get_slave_channel(&d->dmachan);
> > > +}
> >
> > Why not use of_dma_simple_xlate() instead?
> I guess i could, but i don't think i need to define a filter function,
> also there is only one possible channel.

Yeah no point in adding filter_fn. I guess we need
of_dma_xlate_by_chan_id() here, I guess you are specifying channel in dts
right? If not above would be okay

> >
> > > +
> > >  /**
> > >   * msgdma_probe - Driver probe function
> > >   * @pdev: Pointer to the platform_device structure
> > > @@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
> > >       if (ret)
> > >               goto fail;
> > >
> > > +     ret = of_dma_controller_register(pdev->dev.of_node,
> > > +                                      msgdma_of_xlate, mdev);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "failed to register dma controller");
> > > +             goto fail;
> >
> > Should this be treated as an error.. the probe will be invoked on non of
> > systems too..
> Ok, i'm a bit confused,
> in v4 those lines were enclosed with 'if (IS_ENABLED(CONFIG_OF)) { }'
> when you said to me that it was already taken care by the core i though
> that of_dma_controller_register will return 0 on non-of systems.
> Now i can add back IS_ENABLED(CONFIG_OF) or discard the ret value.

Well including in CONFIG_OF sounded protection from compilation which is
not required.

Now the issue is that you maybe running on a system which may or maynot
have DT and even on DT based systems your device may not be DT one..

So i think the return should be handled here if DT device is not present
and warn that and continue for not DT modes.. Also someone who has this
non DT device should test the changes


Thanks
-- 
~Vinod

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-06-07 10:08       ` Vinod Koul
@ 2021-06-07 10:45         ` Olivier Dautricourt
  2021-06-07 10:50           ` Stefan Roese
  0 siblings, 1 reply; 9+ messages in thread
From: Olivier Dautricourt @ 2021-06-07 10:45 UTC (permalink / raw)
  To: Vinod Koul; +Cc: Rob Herring, Stefan Roese, dmaengine, devicetree, linux-kernel

The 06/07/2021 15:38, Vinod Koul wrote:
> On 07-06-21, 10:28, Olivier Dautricourt wrote:
> > The 06/07/2021 12:29, Vinod Koul wrote:
> > > On 18-05-21, 15:25, Olivier Dautricourt wrote:
> > > > This driver had no device tree support.
> > > >
> > > > - add compatible field "altr,socfpga-msgdma"
> > > > - define msgdma_of_xlate, with no argument
> > > > - register dma controller with of_dma_controller_register
> > > >
> > > > Reviewed-by: Stefan Roese <sr@denx.de>
> > > > Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
> > > > ---
> > > >
> > > > Notes:
> > > >     Changes in v2:
> > > >         none
> > > >
> > > >     Changes from v2 to v3:
> > > >         Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
> > > >         only once.
> > > >
> > > >     Changes from v3 to v4
> > > >         Reintroduce #ifdef CONFIG_OF for msgdma_match
> > > >         as it produces a unused variable warning
> > > >
> > > >     Changes from v4 to v5
> > > >         - As per Rob's comments on patch 1/2:
> > > >           change compatible field from altr,msgdma to
> > > >           altr,socfpga-msgdma.
> > > >         - change commit title to fit previous commits naming
> > > >         - As per Vinod's comments:
> > > >           - use dma_get_slave_channel instead of dma_get_any_slave_channel which
> > > >             makes more sense.
> > > >           - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
> > > >             as it is taken care by the core
> > > >
> > > >  drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
> > > >  1 file changed, 26 insertions(+)
> > > >
> > > > diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> > > > index 9a841ce5f0c5..acf0990d73ae 100644
> > > > --- a/drivers/dma/altera-msgdma.c
> > > > +++ b/drivers/dma/altera-msgdma.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/module.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/of_dma.h>
> > > >
> > > >  #include "dmaengine.h"
> > > >
> > > > @@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
> > > >       return 0;
> > > >  }
> > > >
> > > > +static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
> > > > +                                     struct of_dma *ofdma)
> > > > +{
> > > > +     struct msgdma_device *d = ofdma->of_dma_data;
> > > > +
> > > > +     return dma_get_slave_channel(&d->dmachan);
> > > > +}
> > >
> > > Why not use of_dma_simple_xlate() instead?
> > I guess i could, but i don't think i need to define a filter function,
> > also there is only one possible channel.
>
> Yeah no point in adding filter_fn. I guess we need
> of_dma_xlate_by_chan_id() here, I guess you are specifying channel in dts
> right? If not above would be okay
Yes i am, but as this controller has only one channel I was thinking not to fail
if something other than chan_id == 0 is specified. But it may not be right,
I could also remove the argument in the device tree but dma controller
schema expects at least one argument.
Now i think maybe it makes more sense to use of_dma_xlate_by_chan_id and
expect chan_id == 0 in the dt.
>
> > >
> > > > +
> > > >  /**
> > > >   * msgdma_probe - Driver probe function
> > > >   * @pdev: Pointer to the platform_device structure
> > > > @@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
> > > >       if (ret)
> > > >               goto fail;
> > > >
> > > > +     ret = of_dma_controller_register(pdev->dev.of_node,
> > > > +                                      msgdma_of_xlate, mdev);
> > > > +     if (ret) {
> > > > +             dev_err(&pdev->dev, "failed to register dma controller");
> > > > +             goto fail;
> > >
> > > Should this be treated as an error.. the probe will be invoked on non of
> > > systems too..
> > Ok, i'm a bit confused,
> > in v4 those lines were enclosed with 'if (IS_ENABLED(CONFIG_OF)) { }'
> > when you said to me that it was already taken care by the core i though
> > that of_dma_controller_register will return 0 on non-of systems.
> > Now i can add back IS_ENABLED(CONFIG_OF) or discard the ret value.
>
> Well including in CONFIG_OF sounded protection from compilation which is
> not required.
>
> Now the issue is that you maybe running on a system which may or maynot
> have DT and even on DT based systems your device may not be DT one..
good catch, i forgot this use-case ..
>
> So i think the return should be handled here if DT device is not present
> and warn that and continue for not DT modes.. Also someone who has this
> non DT device should test the changes
I can do that.

I think Stefan used this driver on non-DT platform but he said
that he has no access to the hardware anymore.
>
>
> Thanks
> --
> ~Vinod

--
Olivier Dautricourt


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support
  2021-06-07 10:45         ` Olivier Dautricourt
@ 2021-06-07 10:50           ` Stefan Roese
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Roese @ 2021-06-07 10:50 UTC (permalink / raw)
  To: Olivier Dautricourt, Vinod Koul
  Cc: Rob Herring, dmaengine, devicetree, linux-kernel

On 07.06.21 12:45, Olivier Dautricourt wrote:
> The 06/07/2021 15:38, Vinod Koul wrote:
>> On 07-06-21, 10:28, Olivier Dautricourt wrote:
>>> The 06/07/2021 12:29, Vinod Koul wrote:
>>>> On 18-05-21, 15:25, Olivier Dautricourt wrote:
>>>>> This driver had no device tree support.
>>>>>
>>>>> - add compatible field "altr,socfpga-msgdma"
>>>>> - define msgdma_of_xlate, with no argument
>>>>> - register dma controller with of_dma_controller_register
>>>>>
>>>>> Reviewed-by: Stefan Roese <sr@denx.de>
>>>>> Signed-off-by: Olivier Dautricourt <olivier.dautricourt@orolia.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>      Changes in v2:
>>>>>          none
>>>>>
>>>>>      Changes from v2 to v3:
>>>>>          Removed CONFIG_OF #ifdef's and use if (IS_ENABLED(CONFIG_OF))
>>>>>          only once.
>>>>>
>>>>>      Changes from v3 to v4
>>>>>          Reintroduce #ifdef CONFIG_OF for msgdma_match
>>>>>          as it produces a unused variable warning
>>>>>
>>>>>      Changes from v4 to v5
>>>>>          - As per Rob's comments on patch 1/2:
>>>>>            change compatible field from altr,msgdma to
>>>>>            altr,socfpga-msgdma.
>>>>>          - change commit title to fit previous commits naming
>>>>>          - As per Vinod's comments:
>>>>>            - use dma_get_slave_channel instead of dma_get_any_slave_channel which
>>>>>              makes more sense.
>>>>>            - remove if (IS_ENABLED(CONFIG_OF)) for of_dma_controller_register
>>>>>              as it is taken care by the core
>>>>>
>>>>>   drivers/dma/altera-msgdma.c | 26 ++++++++++++++++++++++++++
>>>>>   1 file changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
>>>>> index 9a841ce5f0c5..acf0990d73ae 100644
>>>>> --- a/drivers/dma/altera-msgdma.c
>>>>> +++ b/drivers/dma/altera-msgdma.c
>>>>> @@ -19,6 +19,7 @@
>>>>>   #include <linux/module.h>
>>>>>   #include <linux/platform_device.h>
>>>>>   #include <linux/slab.h>
>>>>> +#include <linux/of_dma.h>
>>>>>
>>>>>   #include "dmaengine.h"
>>>>>
>>>>> @@ -784,6 +785,14 @@ static int request_and_map(struct platform_device *pdev, const char *name,
>>>>>        return 0;
>>>>>   }
>>>>>
>>>>> +static struct dma_chan *msgdma_of_xlate(struct of_phandle_args *dma_spec,
>>>>> +                                     struct of_dma *ofdma)
>>>>> +{
>>>>> +     struct msgdma_device *d = ofdma->of_dma_data;
>>>>> +
>>>>> +     return dma_get_slave_channel(&d->dmachan);
>>>>> +}
>>>>
>>>> Why not use of_dma_simple_xlate() instead?
>>> I guess i could, but i don't think i need to define a filter function,
>>> also there is only one possible channel.
>>
>> Yeah no point in adding filter_fn. I guess we need
>> of_dma_xlate_by_chan_id() here, I guess you are specifying channel in dts
>> right? If not above would be okay
> Yes i am, but as this controller has only one channel I was thinking not to fail
> if something other than chan_id == 0 is specified. But it may not be right,
> I could also remove the argument in the device tree but dma controller
> schema expects at least one argument.
> Now i think maybe it makes more sense to use of_dma_xlate_by_chan_id and
> expect chan_id == 0 in the dt.
>>
>>>>
>>>>> +
>>>>>   /**
>>>>>    * msgdma_probe - Driver probe function
>>>>>    * @pdev: Pointer to the platform_device structure
>>>>> @@ -888,6 +897,13 @@ static int msgdma_probe(struct platform_device *pdev)
>>>>>        if (ret)
>>>>>                goto fail;
>>>>>
>>>>> +     ret = of_dma_controller_register(pdev->dev.of_node,
>>>>> +                                      msgdma_of_xlate, mdev);
>>>>> +     if (ret) {
>>>>> +             dev_err(&pdev->dev, "failed to register dma controller");
>>>>> +             goto fail;
>>>>
>>>> Should this be treated as an error.. the probe will be invoked on non of
>>>> systems too..
>>> Ok, i'm a bit confused,
>>> in v4 those lines were enclosed with 'if (IS_ENABLED(CONFIG_OF)) { }'
>>> when you said to me that it was already taken care by the core i though
>>> that of_dma_controller_register will return 0 on non-of systems.
>>> Now i can add back IS_ENABLED(CONFIG_OF) or discard the ret value.
>>
>> Well including in CONFIG_OF sounded protection from compilation which is
>> not required.
>>
>> Now the issue is that you maybe running on a system which may or maynot
>> have DT and even on DT based systems your device may not be DT one..
> good catch, i forgot this use-case ..
>>
>> So i think the return should be handled here if DT device is not present
>> and warn that and continue for not DT modes.. Also someone who has this
>> non DT device should test the changes
> I can do that.
> 
> I think Stefan used this driver on non-DT platform but he said
> that he has no access to the hardware anymore.

Correct. Unfortunately I can't do any tests.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-06-07 10:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 13:23 [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Olivier Dautricourt
2021-05-18 13:24 ` [PATCH v5 2/3] MAINTAINERS: add entry for Altera mSGDMA Olivier Dautricourt
2021-05-18 13:25 ` [PATCH v5 3/3] dmaengine: altera-msgdma: add OF support Olivier Dautricourt
2021-06-07  6:59   ` Vinod Koul
2021-06-07  8:28     ` Olivier Dautricourt
2021-06-07 10:08       ` Vinod Koul
2021-06-07 10:45         ` Olivier Dautricourt
2021-06-07 10:50           ` Stefan Roese
2021-05-19 22:11 ` [PATCH v5 1/3] dt-bindings: dma: add schema for altera-msgdma Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).