[PATCH 0/3] k3dma: add support to reserved channels

[PATCH 0/3] k3dma: add support to reserved channels

[Guodong Xu]

This patchset fixes bug people found on hikey960 when allocating DMA
channels to peripherals such as SPI. It fails because the channel is
reserved, and is not accessible by kernel.

Patch 1 and 2 add support to reserved channels for K3 DMA. Patch 3
includes a removal of axi_config who controls DMA secure/non-secure
access permission but is actually set in early stage by bootloader.

Li Yu (3):
  dt-bindings: k3dma: add optional property dma_min_chan
  k3dma: add support to reserved minimum channels
  k3dma: delete axi_config

 Documentation/devicetree/bindings/dma/k3dma.txt |  6 ++++++
 drivers/dma/k3dma.c                             | 16 ++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/3] dt-bindings: k3dma: add optional property dma_min_chan

[Guodong Xu]

From: Li Yu <liyu65@hisilicon.com>

Add optional property dma_min_chan for k3dma.

Signed-off-by: Li Yu <liyu65@hisilicon.com>
---
 Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
index 4945aeac4dc4..2fa1370c3173 100644
--- a/Documentation/devicetree/bindings/dma/k3dma.txt
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -12,6 +12,11 @@ Required properties:
 		have specific request line
 - clocks: clock required
 
+Optional properties:
+- dma_min_chan: the minimum number of DMA channel which begin to use
+		the default value is 0, but in some platform is
+		configured 1, like hi3660 platform
+
 Example:
 
 Controller:
@@ -21,6 +26,7 @@ Controller:
 			#dma-cells = <1>;
 			dma-channels = <16>;
 			dma-requests = <27>;
+			dma_min_chan = <1>;
 			interrupts = <0 12 4>;
 			clocks = <&pclk>;
 		};
-- 
2.17.1

[PATCH 2/3] k3dma: add support to reserved minimum channels

[Guodong Xu]

From: Li Yu <liyu65@hisilicon.com>

On k3 series of SoC, DMA controller reserves some channels for
other on-chip coprocessors. By adding support to dma_min_chan, kernel
will not be able to use these reserved channels.

One example is on Hi3660 platform, channel 0 is reserved to lpm3.

Please also refer to Documentation/devicetree/bindings/dma/k3dma.txt

Signed-off-by: Li Yu <liyu65@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/dma/k3dma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index fa31cccbe04f..13cec12742e3 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -113,6 +113,7 @@ struct k3_dma_dev {
 	struct dma_pool		*pool;
 	u32			dma_channels;
 	u32			dma_requests;
+	u32			dma_min_chan;
 	unsigned int		irq;
 };
 
@@ -309,7 +310,7 @@ static void k3_dma_tasklet(unsigned long arg)
 
 	/* check new channel request in d->chan_pending */
 	spin_lock_irq(&d->lock);
-	for (pch = 0; pch < d->dma_channels; pch++) {
+	for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
 		p = &d->phy[pch];
 
 		if (p->vchan == NULL && !list_empty(&d->chan_pending)) {
@@ -326,7 +327,7 @@ static void k3_dma_tasklet(unsigned long arg)
 	}
 	spin_unlock_irq(&d->lock);
 
-	for (pch = 0; pch < d->dma_channels; pch++) {
+	for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
 		if (pch_alloc & (1 << pch)) {
 			p = &d->phy[pch];
 			c = p->vchan;
@@ -825,6 +826,8 @@ static int k3_dma_probe(struct platform_device *op)
 				"dma-channels", &d->dma_channels);
 		of_property_read_u32((&op->dev)->of_node,
 				"dma-requests", &d->dma_requests);
+		of_property_read_u32((&op->dev)->of_node,
+				"dma-min-chan", &d->dma_min_chan);
 	}
 
 	d->clk = devm_clk_get(&op->dev, NULL);
@@ -848,12 +851,12 @@ static int k3_dma_probe(struct platform_device *op)
 		return -ENOMEM;
 
 	/* init phy channel */
-	d->phy = devm_kcalloc(&op->dev,
-		d->dma_channels, sizeof(struct k3_dma_phy), GFP_KERNEL);
+	d->phy = devm_kcalloc(&op->dev, (d->dma_channels - d->dma_min_chan),
+			sizeof(struct k3_dma_phy), GFP_KERNEL);
 	if (d->phy == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < d->dma_channels; i++) {
+	for (i = d->dma_min_chan; i < d->dma_channels; i++) {
 		struct k3_dma_phy *p = &d->phy[i];
 
 		p->idx = i;
-- 
2.17.1

[PATCH 3/3] k3dma: delete axi_config

[Guodong Xu]

From: Li Yu <liyu65@hisilicon.com>

Axi_config controls whether DMA resources can be accessed in non-secure
mode, such as linux kernel. The setting is actually done in
bootloader stage.

This patch removes axi_config from k3dma driver.

Signed-off-by: Li Yu <liyu65@hisilicon.com>
Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
---
 drivers/dma/k3dma.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
index 13cec12742e3..ddd7d1e054c0 100644
--- a/drivers/dma/k3dma.c
+++ b/drivers/dma/k3dma.c
@@ -52,8 +52,6 @@
 #define CX_SRC			0x814
 #define CX_DST			0x818
 #define CX_CFG			0x81c
-#define AXI_CFG			0x820
-#define AXI_CFG_DEFAULT		0x201201
 
 #define CX_LLI_CHAIN_EN		0x2
 #define CX_CFG_EN		0x1
@@ -158,7 +156,6 @@ static void k3_dma_set_desc(struct k3_dma_phy *phy, struct k3_desc_hw *hw)
 	writel_relaxed(hw->count, phy->base + CX_CNT0);
 	writel_relaxed(hw->saddr, phy->base + CX_SRC);
 	writel_relaxed(hw->daddr, phy->base + CX_DST);
-	writel_relaxed(AXI_CFG_DEFAULT, phy->base + AXI_CFG);
 	writel_relaxed(hw->config, phy->base + CX_CFG);
 }
 
-- 
2.17.1

Re: [PATCH 1/3] dt-bindings: k3dma: add optional property dma_min_chan

[Vinod]

On 22-06-18, 11:24, Guodong Xu wrote:
> From: Li Yu <liyu65@hisilicon.com>
> 
> Add optional property dma_min_chan for k3dma.
> 
> Signed-off-by: Li Yu <liyu65@hisilicon.com>
> ---
>  Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 4945aeac4dc4..2fa1370c3173 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -12,6 +12,11 @@ Required properties:
>  		have specific request line
>  - clocks: clock required
>  
> +Optional properties:
> +- dma_min_chan: the minimum number of DMA channel which begin to use
> +		the default value is 0, but in some platform is

Sorry I don't understand this property, we already have dma-channels
which describes the channels in a controller. What is purpose of this ?

> +		configured 1, like hi3660 platform
> +
>  Example:
>  
>  Controller:
> @@ -21,6 +26,7 @@ Controller:
>  			#dma-cells = <1>;
>  			dma-channels = <16>;
>  			dma-requests = <27>;
> +			dma_min_chan = <1>;
>  			interrupts = <0 12 4>;
>  			clocks = <&pclk>;
>  		};
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 2/3] k3dma: add support to reserved minimum channels

[Vinod]

On 22-06-18, 11:24, Guodong Xu wrote:
> From: Li Yu <liyu65@hisilicon.com>
> 
> On k3 series of SoC, DMA controller reserves some channels for
> other on-chip coprocessors. By adding support to dma_min_chan, kernel
> will not be able to use these reserved channels.
> 
> One example is on Hi3660 platform, channel 0 is reserved to lpm3.
> 
> Please also refer to Documentation/devicetree/bindings/dma/k3dma.txt

and if some other platform has channel X marked for co-processor, maybe
a last channel or something in middle, how will this work then?

I am thinking this should be a mask, rather than min.

> 
> Signed-off-by: Li Yu <liyu65@hisilicon.com>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
>  drivers/dma/k3dma.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
> index fa31cccbe04f..13cec12742e3 100644
> --- a/drivers/dma/k3dma.c
> +++ b/drivers/dma/k3dma.c
> @@ -113,6 +113,7 @@ struct k3_dma_dev {
>  	struct dma_pool		*pool;
>  	u32			dma_channels;
>  	u32			dma_requests;
> +	u32			dma_min_chan;
>  	unsigned int		irq;
>  };
>  
> @@ -309,7 +310,7 @@ static void k3_dma_tasklet(unsigned long arg)
>  
>  	/* check new channel request in d->chan_pending */
>  	spin_lock_irq(&d->lock);
> -	for (pch = 0; pch < d->dma_channels; pch++) {
> +	for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
>  		p = &d->phy[pch];
>  
>  		if (p->vchan == NULL && !list_empty(&d->chan_pending)) {
> @@ -326,7 +327,7 @@ static void k3_dma_tasklet(unsigned long arg)
>  	}
>  	spin_unlock_irq(&d->lock);
>  
> -	for (pch = 0; pch < d->dma_channels; pch++) {
> +	for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
>  		if (pch_alloc & (1 << pch)) {
>  			p = &d->phy[pch];
>  			c = p->vchan;
> @@ -825,6 +826,8 @@ static int k3_dma_probe(struct platform_device *op)
>  				"dma-channels", &d->dma_channels);
>  		of_property_read_u32((&op->dev)->of_node,
>  				"dma-requests", &d->dma_requests);
> +		of_property_read_u32((&op->dev)->of_node,
> +				"dma-min-chan", &d->dma_min_chan);
>  	}
>  
>  	d->clk = devm_clk_get(&op->dev, NULL);
> @@ -848,12 +851,12 @@ static int k3_dma_probe(struct platform_device *op)
>  		return -ENOMEM;
>  
>  	/* init phy channel */
> -	d->phy = devm_kcalloc(&op->dev,
> -		d->dma_channels, sizeof(struct k3_dma_phy), GFP_KERNEL);
> +	d->phy = devm_kcalloc(&op->dev, (d->dma_channels - d->dma_min_chan),
> +			sizeof(struct k3_dma_phy), GFP_KERNEL);
>  	if (d->phy == NULL)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < d->dma_channels; i++) {
> +	for (i = d->dma_min_chan; i < d->dma_channels; i++) {
>  		struct k3_dma_phy *p = &d->phy[i];
>  
>  		p->idx = i;
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/3] dt-bindings: k3dma: add optional property dma_min_chan

[Rob Herring]

On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> From: Li Yu <liyu65@hisilicon.com>
> 
> Add optional property dma_min_chan for k3dma.
> 
> Signed-off-by: Li Yu <liyu65@hisilicon.com>
> ---
>  Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt b/Documentation/devicetree/bindings/dma/k3dma.txt
> index 4945aeac4dc4..2fa1370c3173 100644
> --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> @@ -12,6 +12,11 @@ Required properties:
>  		have specific request line
>  - clocks: clock required
>  
> +Optional properties:
> +- dma_min_chan: the minimum number of DMA channel which begin to use
> +		the default value is 0, but in some platform is
> +		configured 1, like hi3660 platform

Can't this be implied by the compatible?

If not, needs vendor prefix and don't use '_' in property names.

Rob

Re: [PATCH 1/3] dt-bindings: k3dma: add optional property dma_min_chan

[Guodong Xu]

On Wed, Jul 4, 2018 at 2:54 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> > From: Li Yu <liyu65@hisilicon.com>
> >
> > Add optional property dma_min_chan for k3dma.
> >
> > Signed-off-by: Li Yu <liyu65@hisilicon.com>
> > ---
> >  Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/t b/Documentation/devicetree/bindings/dma/k3dma.txt
> > index 4945aeac4dc4..2fa1370c3173 100644
> > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > @@ -12,6 +12,11 @@ Required properties:
> >               have specific request line
> >  - clocks: clock required
> >
> > +Optional properties:
> > +- dma_min_chan: the minimum number of DMA channel which begin to use
> > +             the default value is 0, but in some platform is
> > +             configured 1, like hi3660 platform
>
> Can't this be implied by the compatible?
>

No. "hisilicon,k3-dma-1.0" can work with series of hisilicon kirin
SoC. And each has different reservation of channels for on-chip
coprocessors.

> If not, needs vendor prefix and don't use '_' in property names.
>

Sure, thanks. Will change that when design new property. As Vinod
suggested, it makes sense to change this to a mask.


-Guodong

> Rob

Re: [PATCH 2/3] k3dma: add support to reserved minimum channels

[Guodong Xu]

On Thu, Jun 28, 2018 at 2:02 PM Vinod <vkoul@kernel.org> wrote:
>
> On 22-06-18, 11:24, Guodong Xu wrote:
> > From: Li Yu <liyu65@hisilicon.com>
> >
> > On k3 series of SoC, DMA controller reserves some channels for
> > other on-chip coprocessors. By adding support to dma_min_chan, kernel
> > will not be able to use these reserved channels.
> >
> > One example is on Hi3660 platform, channel 0 is reserved to lpm3.
> >
> > Please also refer to Documentation/devicetree/bindings/dma/k3dma.txt
>
> and if some other platform has channel X marked for co-processor, maybe
> a last channel or something in middle, how will this work then?
>
Hi， Vinod

Sorry for delayed response. We checked with Kirin hardware design
team, so far their design strategy is all Kirin SoC series reserve
only from minimum side, saying channel 0, then 1, then 2. That impacts
the current SoC in upstreaming, Kirin960 (Hi3660), and next versions
in Kirin SoC, Kirin970 and 980, which may hit upstream later.

> I am thinking this should be a mask, rather than min.
>

So, since this driver k3dma.c is only used by Kirin SoC DMA
controllers, I would prefer to keep the current design dma_min_chan
unchanged.

What do you think?

-Guodong


> >
> > Signed-off-by: Li Yu <liyu65@hisilicon.com>
> > Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> > ---
> >  drivers/dma/k3dma.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/dma/k3dma.c b/drivers/dma/k3dma.c
> > index fa31cccbe04f..13cec12742e3 100644
> > --- a/drivers/dma/k3dma.c
> > +++ b/drivers/dma/k3dma.c
> > @@ -113,6 +113,7 @@ struct k3_dma_dev {
> >       struct dma_pool         *pool;
> >       u32                     dma_channels;
> >       u32                     dma_requests;
> > +     u32                     dma_min_chan;
> >       unsigned int            irq;
> >  };
> >
> > @@ -309,7 +310,7 @@ static void k3_dma_tasklet(unsigned long arg)
> >
> >       /* check new channel request in d->chan_pending */
> >       spin_lock_irq(&d->lock);
> > -     for (pch = 0; pch < d->dma_channels; pch++) {
> > +     for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
> >               p = &d->phy[pch];
> >
> >               if (p->vchan == NULL && !list_empty(&d->chan_pending)) {
> > @@ -326,7 +327,7 @@ static void k3_dma_tasklet(unsigned long arg)
> >       }
> >       spin_unlock_irq(&d->lock);
> >
> > -     for (pch = 0; pch < d->dma_channels; pch++) {
> > +     for (pch = d->dma_min_chan; pch < d->dma_channels; pch++) {
> >               if (pch_alloc & (1 << pch)) {
> >                       p = &d->phy[pch];
> >                       c = p->vchan;
> > @@ -825,6 +826,8 @@ static int k3_dma_probe(struct platform_device *op)
> >                               "dma-channels", &d->dma_channels);
> >               of_property_read_u32((&op->dev)->of_node,
> >                               "dma-requests", &d->dma_requests);
> > +             of_property_read_u32((&op->dev)->of_node,
> > +                             "dma-min-chan", &d->dma_min_chan);
> >       }
> >
> >       d->clk = devm_clk_get(&op->dev, NULL);
> > @@ -848,12 +851,12 @@ static int k3_dma_probe(struct platform_device *op)
> >               return -ENOMEM;
> >
> >       /* init phy channel */
> > -     d->phy = devm_kcalloc(&op->dev,
> > -             d->dma_channels, sizeof(struct k3_dma_phy), GFP_KERNEL);
> > +     d->phy = devm_kcalloc(&op->dev, (d->dma_channels - d->dma_min_chan),
> > +                     sizeof(struct k3_dma_phy), GFP_KERNEL);
> >       if (d->phy == NULL)
> >               return -ENOMEM;
> >
> > -     for (i = 0; i < d->dma_channels; i++) {
> > +     for (i = d->dma_min_chan; i < d->dma_channels; i++) {
> >               struct k3_dma_phy *p = &d->phy[i];
> >
> >               p->idx = i;
> > --
> > 2.17.1
>
> --
> ~Vinod

Re: [PATCH 1/3] dt-bindings: k3dma: add optional property dma_min_chan

[Guodong Xu]

On Wed, Jul 4, 2018 at 9:14 AM Guodong Xu <guodong.xu@linaro.org> wrote:
>
> On Wed, Jul 4, 2018 at 2:54 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> > > From: Li Yu <liyu65@hisilicon.com>
> > >
> > > Add optional property dma_min_chan for k3dma.
> > >
> > > Signed-off-by: Li Yu <liyu65@hisilicon.com>
> > > ---
> > >  Documentation/devicetree/bindings/dma/k3dma.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/t b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > index 4945aeac4dc4..2fa1370c3173 100644
> > > --- a/Documentation/devicetree/bindings/dma/k3dma.txt
> > > +++ b/Documentation/devicetree/bindings/dma/k3dma.txt
> > > @@ -12,6 +12,11 @@ Required properties:
> > >               have specific request line
> > >  - clocks: clock required
> > >
> > > +Optional properties:
> > > +- dma_min_chan: the minimum number of DMA channel which begin to use
> > > +             the default value is 0, but in some platform is
> > > +             configured 1, like hi3660 platform
> >
> > Can't this be implied by the compatible?
> >
>
> No. "hisilicon,k3-dma-1.0" can work with series of hisilicon kirin
> SoC. And each has different reservation of channels for on-chip
> coprocessors.
>
> > If not, needs vendor prefix and don't use '_' in property names.
> >
>
> Sure, thanks. Will change that when design new property. As Vinod
> suggested, it makes sense to change this to a mask.
>

After checking with Kirin SoC design team, I prefer to stay with
minimum channel number instead of mask. So, I will change this
property to:

hisilicon,dma-min-chan

-Guodong



>
> -Guodong
>
> > Rob

Re: [PATCH 2/3] k3dma: add support to reserved minimum channels

[Vinod]

On 06-07-18, 11:05, Guodong Xu wrote:
> On Thu, Jun 28, 2018 at 2:02 PM Vinod <vkoul@kernel.org> wrote:
> >
> > On 22-06-18, 11:24, Guodong Xu wrote:
> > > From: Li Yu <liyu65@hisilicon.com>
> > >
> > > On k3 series of SoC, DMA controller reserves some channels for
> > > other on-chip coprocessors. By adding support to dma_min_chan, kernel
> > > will not be able to use these reserved channels.
> > >
> > > One example is on Hi3660 platform, channel 0 is reserved to lpm3.
> > >
> > > Please also refer to Documentation/devicetree/bindings/dma/k3dma.txt
> >
> > and if some other platform has channel X marked for co-processor, maybe
> > a last channel or something in middle, how will this work then?
> >
> Hi， Vinod
> 
> Sorry for delayed response. We checked with Kirin hardware design
> team, so far their design strategy is all Kirin SoC series reserve
> only from minimum side, saying channel 0, then 1, then 2. That impacts
> the current SoC in upstreaming, Kirin960 (Hi3660), and next versions
> in Kirin SoC, Kirin970 and 980, which may hit upstream later.

And what guarantees that they will not change their mind..

> > I am thinking this should be a mask, rather than min.
> >
> 
> So, since this driver k3dma.c is only used by Kirin SoC DMA
> controllers, I would prefer to keep the current design dma_min_chan
> unchanged.
> 
> What do you think?

I would still prefer bitmask to expose the channels you are supposed to
use

-- 
~Vinod