Patchwork [2/8,v2] Documentation: bindings: k3dma: Add binding for dma-avail-chan

login
register
mail settings
Submitter John Stultz
Date Jan. 4, 2019, 8:56 p.m.
Message ID <1546635388-13795-3-git-send-email-john.stultz@linaro.org>
Download mbox | patch
Permalink /patch/693207/
State New
Headers show

Comments

John Stultz - Jan. 4, 2019, 8:56 p.m.
Some dma channels can be reserved for secure mode or other
hardware on the SoC, so provide a binding for a bitmask
listing the available channels for the kernel to use.

Cc: Vinod Koul <vkoul@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Tanglei Han <hantanglei@huawei.com>
Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
Cc: Ryan Grachek <ryan@edited.us>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: dmaengine@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
 1 file changed, 3 insertions(+)
Manivannan Sadhasivam - Jan. 5, 2019, 4 a.m.
Hi John,

On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> Some dma channels can be reserved for secure mode or other
> hardware on the SoC, so provide a binding for a bitmask
> listing the available channels for the kernel to use.
> 
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Tanglei Han <hantanglei@huawei.com>
> Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> Cc: Ryan Grachek <ryan@edited.us>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: dmaengine@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 10a2f15..1c466c1 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -14,6 +14,9 @@ Required properties:
>  		have specific request line
>  - clocks: clock required
>  
> +Optional properties:
> +- dma-avail-chan: Bitmask of available physical channels
> +

This property looks too generic. Since this is specific to HiSi SoCs,
this could be "hisi-dma-avail-chan"?

Thanks,
Mani

>  Example:
>  
>  Controller:
> -- 
> 2.7.4
>
John Stultz - Jan. 5, 2019, 4:39 a.m.
On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi John,
>
> On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > Some dma channels can be reserved for secure mode or other
> > hardware on the SoC, so provide a binding for a bitmask
> > listing the available channels for the kernel to use.
> >
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Tanglei Han <hantanglei@huawei.com>
> > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > Cc: Ryan Grachek <ryan@edited.us>
> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Cc: dmaengine@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > index 10a2f15..1c466c1 100644
> > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > @@ -14,6 +14,9 @@ Required properties:
> >               have specific request line
> >  - clocks: clock required
> >
> > +Optional properties:
> > +- dma-avail-chan: Bitmask of available physical channels
> > +
>
> This property looks too generic. Since this is specific to HiSi SoCs,
> this could be "hisi-dma-avail-chan"?

I'm fine to change it, but I'm not sure I fully understand the
rational. Can you help me understand?
Are device node-binding names supposed to have global scope? I assumed
the node property names are basically scoped to the entry?
Further, having some dma channels be reserved doesn't seem to be too
unique a concept, so I'm not sure what we gain long term by prefixing
it?

thanks
-john
Manivannan Sadhasivam - Jan. 5, 2019, 4:53 a.m.
On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Hi John,
> >
> > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > Some dma channels can be reserved for secure mode or other
> > > hardware on the SoC, so provide a binding for a bitmask
> > > listing the available channels for the kernel to use.
> > >
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > Cc: Ryan Grachek <ryan@edited.us>
> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Cc: dmaengine@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > index 10a2f15..1c466c1 100644
> > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > @@ -14,6 +14,9 @@ Required properties:
> > >               have specific request line
> > >  - clocks: clock required
> > >
> > > +Optional properties:
> > > +- dma-avail-chan: Bitmask of available physical channels
> > > +
> >
> > This property looks too generic. Since this is specific to HiSi SoCs,
> > this could be "hisi-dma-avail-chan"?
> 
> I'm fine to change it, but I'm not sure I fully understand the
> rational. Can you help me understand?
> Are device node-binding names supposed to have global scope? I assumed
> the node property names are basically scoped to the entry?

IIUC properties documented in subsystem binding (dma.txt in this case)
will have global scope. Those which are not documented in this binding
are specific to vendor IPs and should be prefixed with the vendor
prefix (hisi in this case).

> Further, having some dma channels be reserved doesn't seem to be too
> unique a concept, so I'm not sure what we gain long term by prefixing
> it?
> 

Right, but this brings up the point of having this functionality in
generic DMA engine so that the DMA controller drivers need not handle.
So either we should move this available channel check to DMA Engine
and document the property in dma.txt so that other IPs can also use it
or keep the functionality in K3 driver and use HiSi prefix for the
property.

But I'd like to hear Vinod/Rob's opinion on this!

Thanks,
Mani

> thanks
> -john
John Stultz - Jan. 5, 2019, 4:58 a.m.
On Fri, Jan 4, 2019 at 8:53 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Hi John,
> > >
> > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > Some dma channels can be reserved for secure mode or other
> > > > hardware on the SoC, so provide a binding for a bitmask
> > > > listing the available channels for the kernel to use.
> > > >
> > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Cc: dmaengine@vger.kernel.org
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > index 10a2f15..1c466c1 100644
> > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > @@ -14,6 +14,9 @@ Required properties:
> > > >               have specific request line
> > > >  - clocks: clock required
> > > >
> > > > +Optional properties:
> > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > +
> > >
> > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > this could be "hisi-dma-avail-chan"?
> >
> > I'm fine to change it, but I'm not sure I fully understand the
> > rational. Can you help me understand?
> > Are device node-binding names supposed to have global scope? I assumed
> > the node property names are basically scoped to the entry?
>
> IIUC properties documented in subsystem binding (dma.txt in this case)
> will have global scope. Those which are not documented in this binding
> are specific to vendor IPs and should be prefixed with the vendor
> prefix (hisi in this case).

Thanks I appreciate the explanation here. I hadn't really understood
this point, and really haven't developed much "taste" in what makes a
good or bad binding.

> > Further, having some dma channels be reserved doesn't seem to be too
> > unique a concept, so I'm not sure what we gain long term by prefixing
> > it?
> >
>
> Right, but this brings up the point of having this functionality in
> generic DMA engine so that the DMA controller drivers need not handle.
> So either we should move this available channel check to DMA Engine
> and document the property in dma.txt so that other IPs can also use it
> or keep the functionality in K3 driver and use HiSi prefix for the
> property.
>
> But I'd like to hear Vinod/Rob's opinion on this!

Sure. Though for now I'll prefix it as the logic is handled at the
driver level.

thanks
-john
Vinod Koul - Jan. 5, 2019, 1:46 p.m.
On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > Hi John,
> > >
> > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > Some dma channels can be reserved for secure mode or other
> > > > hardware on the SoC, so provide a binding for a bitmask
> > > > listing the available channels for the kernel to use.
> > > >
> > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Cc: dmaengine@vger.kernel.org
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > index 10a2f15..1c466c1 100644
> > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > @@ -14,6 +14,9 @@ Required properties:
> > > >               have specific request line
> > > >  - clocks: clock required
> > > >
> > > > +Optional properties:
> > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > +
> > >
> > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > this could be "hisi-dma-avail-chan"?
> > 
> > I'm fine to change it, but I'm not sure I fully understand the
> > rational. Can you help me understand?
> > Are device node-binding names supposed to have global scope? I assumed
> > the node property names are basically scoped to the entry?
> 
> IIUC properties documented in subsystem binding (dma.txt in this case)
> will have global scope. Those which are not documented in this binding
> are specific to vendor IPs and should be prefixed with the vendor
> prefix (hisi in this case).
> 
> > Further, having some dma channels be reserved doesn't seem to be too
> > unique a concept, so I'm not sure what we gain long term by prefixing
> > it?
> > 
> 
> Right, but this brings up the point of having this functionality in
> generic DMA engine so that the DMA controller drivers need not handle.
> So either we should move this available channel check to DMA Engine
> and document the property in dma.txt so that other IPs can also use it
> or keep the functionality in K3 driver and use HiSi prefix for the
> property.
> 
> But I'd like to hear Vinod/Rob's opinion on this!

So there are two parts, first is if this new property of using 'some'
channels of controller is generic enough, the answer is unfortunately
yes, so we should move this to dma.txt as a generic property

But I don't agree the dmaengine core should handle it, we may add
helpers, but controllers registers N channels and they would do so, core
should not do filtering
Manivannan Sadhasivam - Jan. 5, 2019, 2:08 p.m.
Hi Vinod,

On Sat, Jan 05, 2019 at 07:16:10PM +0530, Vinod Koul wrote:
> On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> > On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > Hi John,
> > > >
> > > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > > Some dma channels can be reserved for secure mode or other
> > > > > hardware on the SoC, so provide a binding for a bitmask
> > > > > listing the available channels for the kernel to use.
> > > > >
> > > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > Cc: dmaengine@vger.kernel.org
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > index 10a2f15..1c466c1 100644
> > > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > @@ -14,6 +14,9 @@ Required properties:
> > > > >               have specific request line
> > > > >  - clocks: clock required
> > > > >
> > > > > +Optional properties:
> > > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > > +
> > > >
> > > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > > this could be "hisi-dma-avail-chan"?
> > > 
> > > I'm fine to change it, but I'm not sure I fully understand the
> > > rational. Can you help me understand?
> > > Are device node-binding names supposed to have global scope? I assumed
> > > the node property names are basically scoped to the entry?
> > 
> > IIUC properties documented in subsystem binding (dma.txt in this case)
> > will have global scope. Those which are not documented in this binding
> > are specific to vendor IPs and should be prefixed with the vendor
> > prefix (hisi in this case).
> > 
> > > Further, having some dma channels be reserved doesn't seem to be too
> > > unique a concept, so I'm not sure what we gain long term by prefixing
> > > it?
> > > 
> > 
> > Right, but this brings up the point of having this functionality in
> > generic DMA engine so that the DMA controller drivers need not handle.
> > So either we should move this available channel check to DMA Engine
> > and document the property in dma.txt so that other IPs can also use it
> > or keep the functionality in K3 driver and use HiSi prefix for the
> > property.
> > 
> > But I'd like to hear Vinod/Rob's opinion on this!
> 
> So there are two parts, first is if this new property of using 'some'
> channels of controller is generic enough, the answer is unfortunately
> yes, so we should move this to dma.txt as a generic property
> 
> But I don't agree the dmaengine core should handle it, we may add
> helpers, but controllers registers N channels and they would do so, core
> should not do filtering
> 

Okay. But won't it create ambiguity? What if a new driver developer
assmes that he can use this property to filter the channels for his own
DMA controller? Since we are _explicitly_ stating that these channels
should be filtered, why the dmaengine core can't handle it?

If the property is generic, then it makes sense to keep the
functionality also generic.

Thanks,
Mani

> -- 
> ~Vinod
Vinod Koul - Jan. 7, 2019, 6:24 a.m.
On 05-01-19, 19:38, Manivannan Sadhasivam wrote:
> Hi Vinod,
> 
> On Sat, Jan 05, 2019 at 07:16:10PM +0530, Vinod Koul wrote:
> > On 05-01-19, 10:23, Manivannan Sadhasivam wrote:
> > > On Fri, Jan 04, 2019 at 08:39:34PM -0800, John Stultz wrote:
> > > > On Fri, Jan 4, 2019 at 8:00 PM Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > >
> > > > > Hi John,
> > > > >
> > > > > On Fri, Jan 04, 2019 at 12:56:22PM -0800, John Stultz wrote:
> > > > > > Some dma channels can be reserved for secure mode or other
> > > > > > hardware on the SoC, so provide a binding for a bitmask
> > > > > > listing the available channels for the kernel to use.
> > > > > >
> > > > > > Cc: Vinod Koul <vkoul@kernel.org>
> > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > > Cc: Tanglei Han <hantanglei@huawei.com>
> > > > > > Cc: Zhuangluan Su <suzhuangluan@hisilicon.com>
> > > > > > Cc: Ryan Grachek <ryan@edited.us>
> > > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > Cc: dmaengine@vger.kernel.org
> > > > > > Cc: devicetree@vger.kernel.org
> > > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/dma/k3dma.txt | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > index 10a2f15..1c466c1 100644
> > > > > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > > > > @@ -14,6 +14,9 @@ Required properties:
> > > > > >               have specific request line
> > > > > >  - clocks: clock required
> > > > > >
> > > > > > +Optional properties:
> > > > > > +- dma-avail-chan: Bitmask of available physical channels
> > > > > > +
> > > > >
> > > > > This property looks too generic. Since this is specific to HiSi SoCs,
> > > > > this could be "hisi-dma-avail-chan"?
> > > > 
> > > > I'm fine to change it, but I'm not sure I fully understand the
> > > > rational. Can you help me understand?
> > > > Are device node-binding names supposed to have global scope? I assumed
> > > > the node property names are basically scoped to the entry?
> > > 
> > > IIUC properties documented in subsystem binding (dma.txt in this case)
> > > will have global scope. Those which are not documented in this binding
> > > are specific to vendor IPs and should be prefixed with the vendor
> > > prefix (hisi in this case).
> > > 
> > > > Further, having some dma channels be reserved doesn't seem to be too
> > > > unique a concept, so I'm not sure what we gain long term by prefixing
> > > > it?
> > > > 
> > > 
> > > Right, but this brings up the point of having this functionality in
> > > generic DMA engine so that the DMA controller drivers need not handle.
> > > So either we should move this available channel check to DMA Engine
> > > and document the property in dma.txt so that other IPs can also use it
> > > or keep the functionality in K3 driver and use HiSi prefix for the
> > > property.
> > > 
> > > But I'd like to hear Vinod/Rob's opinion on this!
> > 
> > So there are two parts, first is if this new property of using 'some'
> > channels of controller is generic enough, the answer is unfortunately
> > yes, so we should move this to dma.txt as a generic property
> > 
> > But I don't agree the dmaengine core should handle it, we may add
> > helpers, but controllers registers N channels and they would do so, core
> > should not do filtering
> > 
> 
> Okay. But won't it create ambiguity? What if a new driver developer
> assmes that he can use this property to filter the channels for his own
> DMA controller? Since we are _explicitly_ stating that these channels
> should be filtered, why the dmaengine core can't handle it?
> 
> If the property is generic, then it makes sense to keep the
> functionality also generic.

Core doesnt have a view of channels to be filtered, it looks at N
channels getting registered and works on those.

Till now folks do not create channels for 'filtered' ones and register
the ones kernel can use..

Patch

diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
index 10a2f15..1c466c1 100644
--- a/Documentation/devicetree/bindings/dma/k3dma.txt
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -14,6 +14,9 @@  Required properties:
 		have specific request line
 - clocks: clock required
 
+Optional properties:
+- dma-avail-chan: Bitmask of available physical channels
+
 Example:
 
 Controller: