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
From: Li Yu <[email protected]>
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 <[email protected]>
Signed-off-by: Guodong Xu <[email protected]>
---
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
From: Li Yu <[email protected]>
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 <[email protected]>
Signed-off-by: Guodong Xu <[email protected]>
---
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
From: Li Yu <[email protected]>
Add optional property dma_min_chan for k3dma.
Signed-off-by: Li Yu <[email protected]>
---
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
On 22-06-18, 11:24, Guodong Xu wrote:
> From: Li Yu <[email protected]>
>
> Add optional property dma_min_chan for k3dma.
>
> Signed-off-by: Li Yu <[email protected]>
> ---
> 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
On 22-06-18, 11:24, Guodong Xu wrote:
> From: Li Yu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Guodong Xu <[email protected]>
> ---
> 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
On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> From: Li Yu <[email protected]>
>
> Add optional property dma_min_chan for k3dma.
>
> Signed-off-by: Li Yu <[email protected]>
> ---
> 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
On Wed, Jul 4, 2018 at 2:54 AM Rob Herring <[email protected]> wrote:
>
> On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> > From: Li Yu <[email protected]>
> >
> > Add optional property dma_min_chan for k3dma.
> >
> > Signed-off-by: Li Yu <[email protected]>
> > ---
> > 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
On Thu, Jun 28, 2018 at 2:02 PM Vinod <[email protected]> wrote:
>
> On 22-06-18, 11:24, Guodong Xu wrote:
> > From: Li Yu <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Guodong Xu <[email protected]>
> > ---
> > 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
On Wed, Jul 4, 2018 at 9:14 AM Guodong Xu <[email protected]> wrote:
>
> On Wed, Jul 4, 2018 at 2:54 AM Rob Herring <[email protected]> wrote:
> >
> > On Fri, Jun 22, 2018 at 11:24:14AM +0800, Guodong Xu wrote:
> > > From: Li Yu <[email protected]>
> > >
> > > Add optional property dma_min_chan for k3dma.
> > >
> > > Signed-off-by: Li Yu <[email protected]>
> > > ---
> > > 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
On 06-07-18, 11:05, Guodong Xu wrote:
> On Thu, Jun 28, 2018 at 2:02 PM Vinod <[email protected]> wrote:
> >
> > On 22-06-18, 11:24, Guodong Xu wrote:
> > > From: Li Yu <[email protected]>
> > >
> > > 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