[PATCH 0/7] net: ethernet: ti: cpsw: support placing CPDMA descriptors into DDR

[PATCH 0/7] net: ethernet: ti: cpsw: support placing CPDMA descriptors into DDR

[Grygorii Strashko]

This series intended to add support for placing CPDMA descriptors into DDR by
introducing new DT property "descs_pool_size" to specify buffer descriptor's
pool size. The "descs_pool_size" defines total number of CPDMA
CPPI descriptors to be used for both ingress/egress packets
processing. If not specified - the default value 256 will be used
which will allow to place descriptor's pool into the internal CPPI
RAM.

This allows significantly to reduce UDP packets drop rate
for bandwidth >301 Mbits/sec (am57x).  

Before enabling this feature, the am437x SoC has to be fixed as it's proved
that it's not working when CPDMA descriptors placed in DDR.
So, the patch 1 fixes this issue.

Grygorii Strashko (7):
  net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr
  net: ethernet: ti: cpdma: fix desc re-queuing
  net: ethernet: ti: cpdma: minimize number of parameters in
    cpdma_desc_pool_create/destroy()
  net: ethernet: ti: cpdma: use devm_ioremap
  Documentation: DT: net: cpsw: allow to specify descriptors pool size
  net: ethernet: ti: cpsw: add support for descs_pool_size dt property
  Documentation: DT: net: cpsw: remove no_bd_ram property

 Documentation/devicetree/bindings/net/cpsw.txt |  8 ++-
 arch/arm/boot/dts/am33xx.dtsi                  |  1 -
 arch/arm/boot/dts/am4372.dtsi                  |  1 -
 arch/arm/boot/dts/dm814x.dtsi                  |  1 -
 arch/arm/boot/dts/dra7.dtsi                    |  1 -
 drivers/net/ethernet/ti/cpsw.c                 |  5 ++
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 drivers/net/ethernet/ti/davinci_cpdma.c        | 90 +++++++++++++++-----------
 drivers/net/ethernet/ti/davinci_cpdma.h        |  1 +
 9 files changed, 63 insertions(+), 46 deletions(-)

-- 
2.10.1

[PATCH 0/7] net: ethernet: ti: cpsw: support placing CPDMA descriptors into DDR

[Grygorii Strashko]

This series intended to add support for placing CPDMA descriptors into DDR by
introducing new DT property "descs_pool_size" to specify buffer descriptor's
pool size. The "descs_pool_size" defines total number of CPDMA
CPPI descriptors to be used for both ingress/egress packets
processing. If not specified - the default value 256 will be used
which will allow to place descriptor's pool into the internal CPPI
RAM.

This allows significantly to reduce UDP packets drop rate
for bandwidth >301 Mbits/sec (am57x).  

Before enabling this feature, the am437x SoC has to be fixed as it's proved
that it's not working when CPDMA descriptors placed in DDR.
So, the patch 1 fixes this issue.

Grygorii Strashko (7):
  net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr
  net: ethernet: ti: cpdma: fix desc re-queuing
  net: ethernet: ti: cpdma: minimize number of parameters in
    cpdma_desc_pool_create/destroy()
  net: ethernet: ti: cpdma: use devm_ioremap
  Documentation: DT: net: cpsw: allow to specify descriptors pool size
  net: ethernet: ti: cpsw: add support for descs_pool_size dt property
  Documentation: DT: net: cpsw: remove no_bd_ram property

 Documentation/devicetree/bindings/net/cpsw.txt |  8 ++-
 arch/arm/boot/dts/am33xx.dtsi                  |  1 -
 arch/arm/boot/dts/am4372.dtsi                  |  1 -
 arch/arm/boot/dts/dm814x.dtsi                  |  1 -
 arch/arm/boot/dts/dra7.dtsi                    |  1 -
 drivers/net/ethernet/ti/cpsw.c                 |  5 ++
 drivers/net/ethernet/ti/cpsw.h                 |  1 +
 drivers/net/ethernet/ti/davinci_cpdma.c        | 90 +++++++++++++++-----------
 drivers/net/ethernet/ti/davinci_cpdma.h        |  1 +
 9 files changed, 63 insertions(+), 46 deletions(-)

-- 
2.10.1

[PATCH 1/7] net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr

[Grygorii Strashko]

It's observed that cpsw/cpdma is not working properly when CPPI
descriptors are placed in DDR instead of internal CPPI RAM on am437x
SoC:
- rx/tx silently stops processing packets;
- or - after boot it's working for sometime, but stuck once Network
load is increased (ping is working, but iperf is not).
(The same issue has not been reproduced on am335x and am57xx).

It seems that write to HDP register processed faster by interconnect
than writing of descriptor memory buffer in DDR, which is probably
caused by store buffer / write buffer differences as these function
are implemented differently across devices. So, to fix this i come up
with two changes:

1) all accesses to the channel register HDP/CP/RXFREE registers should
be done using sync IO accessors readl()/writel(), because all previous
memory writes writes have to be completed before starting channel
(write to HDP) or completing desc processing.

2) the change 1 only doesn't work on am437x and additional reading of
desc's field is required right after the new descriptor was filled
with data and before pointer on it will be stored in
prev_desc->hw_next field or HDP register.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index c776e45..0924014 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -167,10 +167,10 @@ static struct cpdma_control_info controls[] = {
 
 /* various accessors */
 #define dma_reg_read(ctlr, ofs)		__raw_readl((ctlr)->dmaregs + (ofs))
-#define chan_read(chan, fld)		__raw_readl((chan)->fld)
+#define chan_read(chan, fld)		readl((chan)->fld)
 #define desc_read(desc, fld)		__raw_readl(&(desc)->fld)
 #define dma_reg_write(ctlr, ofs, v)	__raw_writel(v, (ctlr)->dmaregs + (ofs))
-#define chan_write(chan, fld, v)	__raw_writel(v, (chan)->fld)
+#define chan_write(chan, fld, v)	writel(v, (chan)->fld)
 #define desc_write(desc, fld, v)	__raw_writel((u32)(v), &(desc)->fld)
 
 #define cpdma_desc_to_port(chan, mode, directed)			\
@@ -1064,6 +1064,7 @@ int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
 	desc_write(desc, sw_token,  token);
 	desc_write(desc, sw_buffer, buffer);
 	desc_write(desc, sw_len,    len);
+	desc_read(desc, sw_len);
 
 	__cpdma_chan_submit(chan, desc);
 
-- 
2.10.1

[PATCH 1/7] net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr

[Grygorii Strashko]

It's observed that cpsw/cpdma is not working properly when CPPI
descriptors are placed in DDR instead of internal CPPI RAM on am437x
SoC:
- rx/tx silently stops processing packets;
- or - after boot it's working for sometime, but stuck once Network
load is increased (ping is working, but iperf is not).
(The same issue has not been reproduced on am335x and am57xx).

It seems that write to HDP register processed faster by interconnect
than writing of descriptor memory buffer in DDR, which is probably
caused by store buffer / write buffer differences as these function
are implemented differently across devices. So, to fix this i come up
with two changes:

1) all accesses to the channel register HDP/CP/RXFREE registers should
be done using sync IO accessors readl()/writel(), because all previous
memory writes writes have to be completed before starting channel
(write to HDP) or completing desc processing.

2) the change 1 only doesn't work on am437x and additional reading of
desc's field is required right after the new descriptor was filled
with data and before pointer on it will be stored in
prev_desc->hw_next field or HDP register.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index c776e45..0924014 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -167,10 +167,10 @@ static struct cpdma_control_info controls[] = {
 
 /* various accessors */
 #define dma_reg_read(ctlr, ofs)		__raw_readl((ctlr)->dmaregs + (ofs))
-#define chan_read(chan, fld)		__raw_readl((chan)->fld)
+#define chan_read(chan, fld)		readl((chan)->fld)
 #define desc_read(desc, fld)		__raw_readl(&(desc)->fld)
 #define dma_reg_write(ctlr, ofs, v)	__raw_writel(v, (ctlr)->dmaregs + (ofs))
-#define chan_write(chan, fld, v)	__raw_writel(v, (chan)->fld)
+#define chan_write(chan, fld, v)	writel(v, (chan)->fld)
 #define desc_write(desc, fld, v)	__raw_writel((u32)(v), &(desc)->fld)
 
 #define cpdma_desc_to_port(chan, mode, directed)			\
@@ -1064,6 +1064,7 @@ int cpdma_chan_submit(struct cpdma_chan *chan, void *token, void *data,
 	desc_write(desc, sw_token,  token);
 	desc_write(desc, sw_buffer, buffer);
 	desc_write(desc, sw_len,    len);
+	desc_read(desc, sw_len);
 
 	__cpdma_chan_submit(chan, desc);
 
-- 
2.10.1

Re: [PATCH 1/7] net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr

[David Miller]

From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Thu, 1 Dec 2016 17:34:26 -0600

> @@ -167,10 +167,10 @@ static struct cpdma_control_info controls[] = {
>  
>  /* various accessors */
>  #define dma_reg_read(ctlr, ofs)		__raw_readl((ctlr)->dmaregs + (ofs))
> -#define chan_read(chan, fld)		__raw_readl((chan)->fld)
> +#define chan_read(chan, fld)		readl((chan)->fld)
>  #define desc_read(desc, fld)		__raw_readl(&(desc)->fld)
>  #define dma_reg_write(ctlr, ofs, v)	__raw_writel(v, (ctlr)->dmaregs + (ofs))
> -#define chan_write(chan, fld, v)	__raw_writel(v, (chan)->fld)
> +#define chan_write(chan, fld, v)	writel(v, (chan)->fld)
>  #define desc_write(desc, fld, v)	__raw_writel((u32)(v), &(desc)->fld)

Unless you want to keep running into subtle errors all over the
place wrt. register vs. memory write ordering, I strong suggest
you use strongly ordered readl/writel for all register accesses.

I see no tangible, worthwhile, advantage to using these relaxed
ordering primitives.  The only result is potential bugs.

People who use the relaxed ordering primitives properly are only
doing so in extremely carefully coded sequences where a series
of writes has no dependency on main memory operations and is
explicitly completed with a barrier operation such as a read
back of a register in the same device.

That's not at all what is going on here, instead the driver is wildly
using relaxed ordered register accesses for basically everything.
This is extremely unwise and it's why you ran into this bug in the
first place.

Therefore, I absolutely require that you fix this by eliminating
any and all usese of relaxed ordering I/O accessors in this driver.

Thank you.

Re: [PATCH 1/7] net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr

[Grygorii Strashko]

On 12/03/2016 02:34 PM, David Miller wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Thu, 1 Dec 2016 17:34:26 -0600
> 
>> @@ -167,10 +167,10 @@ static struct cpdma_control_info controls[] = {
>>  
>>  /* various accessors */
>>  #define dma_reg_read(ctlr, ofs)		__raw_readl((ctlr)->dmaregs + (ofs))
>> -#define chan_read(chan, fld)		__raw_readl((chan)->fld)
>> +#define chan_read(chan, fld)		readl((chan)->fld)
>>  #define desc_read(desc, fld)		__raw_readl(&(desc)->fld)
>>  #define dma_reg_write(ctlr, ofs, v)	__raw_writel(v, (ctlr)->dmaregs + (ofs))
>> -#define chan_write(chan, fld, v)	__raw_writel(v, (chan)->fld)
>> +#define chan_write(chan, fld, v)	writel(v, (chan)->fld)
>>  #define desc_write(desc, fld, v)	__raw_writel((u32)(v), &(desc)->fld)
> 
> Unless you want to keep running into subtle errors all over the
> place wrt. register vs. memory write ordering, I strong suggest
> you use strongly ordered readl/writel for all register accesses.
> 
> I see no tangible, worthwhile, advantage to using these relaxed
> ordering primitives.  The only result is potential bugs.
> 
> People who use the relaxed ordering primitives properly are only
> doing so in extremely carefully coded sequences where a series
> of writes has no dependency on main memory operations and is
> explicitly completed with a barrier operation such as a read
> back of a register in the same device.
> 
> That's not at all what is going on here, instead the driver is wildly
> using relaxed ordered register accesses for basically everything.
> This is extremely unwise and it's why you ran into this bug in the
> first place.
> 
> Therefore, I absolutely require that you fix this by eliminating
> any and all usese of relaxed ordering I/O accessors in this driver.

Thanks for your comments - that's what I've tried first, but that decided
to find out minimal change which still works.

I'll update it. 

-- 
regards,
-grygorii

Re: [PATCH 1/7] net: ethernet: ti: cpdma: am437x: allow descs to be plased in ddr

[Grygorii Strashko]

On 12/03/2016 02:34 PM, David Miller wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> Date: Thu, 1 Dec 2016 17:34:26 -0600
> 
>> @@ -167,10 +167,10 @@ static struct cpdma_control_info controls[] = {
>>  
>>  /* various accessors */
>>  #define dma_reg_read(ctlr, ofs)		__raw_readl((ctlr)->dmaregs + (ofs))
>> -#define chan_read(chan, fld)		__raw_readl((chan)->fld)
>> +#define chan_read(chan, fld)		readl((chan)->fld)
>>  #define desc_read(desc, fld)		__raw_readl(&(desc)->fld)
>>  #define dma_reg_write(ctlr, ofs, v)	__raw_writel(v, (ctlr)->dmaregs + (ofs))
>> -#define chan_write(chan, fld, v)	__raw_writel(v, (chan)->fld)
>> +#define chan_write(chan, fld, v)	writel(v, (chan)->fld)
>>  #define desc_write(desc, fld, v)	__raw_writel((u32)(v), &(desc)->fld)
> 
> Unless you want to keep running into subtle errors all over the
> place wrt. register vs. memory write ordering, I strong suggest
> you use strongly ordered readl/writel for all register accesses.
> 
> I see no tangible, worthwhile, advantage to using these relaxed
> ordering primitives.  The only result is potential bugs.
> 
> People who use the relaxed ordering primitives properly are only
> doing so in extremely carefully coded sequences where a series
> of writes has no dependency on main memory operations and is
> explicitly completed with a barrier operation such as a read
> back of a register in the same device.
> 
> That's not at all what is going on here, instead the driver is wildly
> using relaxed ordered register accesses for basically everything.
> This is extremely unwise and it's why you ran into this bug in the
> first place.
> 
> Therefore, I absolutely require that you fix this by eliminating
> any and all usese of relaxed ordering I/O accessors in this driver.

Thanks for your comments - that's what I've tried first, but that decided
to find out minimal change which still works.

I'll update it. 

-- 
regards,
-grygorii

[PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

The currently processing cpdma descriptor with EOQ flag set may
contain two values in Next Descriptor Pointer field:
- valid pointer: means CPDMA missed addition of new desc in queue;
- null: no more descriptors in queue.
In the later case, it's not required to write to HDP register, but now
CPDMA does it.

Hence, add additional check for Next Descriptor Pointer != null in
cpdma_chan_process() function before writing in HDP register.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 0924014..379314f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
 	chan->count--;
 	chan->stats.good_dequeue++;
 
-	if (status & CPDMA_DESC_EOQ) {
+	if ((status & CPDMA_DESC_EOQ) && chan->head) {
 		chan->stats.requeue++;
 		chan_write(chan, hdp, desc_phys(pool, chan->head));
 	}
-- 
2.10.1

[PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

The currently processing cpdma descriptor with EOQ flag set may
contain two values in Next Descriptor Pointer field:
- valid pointer: means CPDMA missed addition of new desc in queue;
- null: no more descriptors in queue.
In the later case, it's not required to write to HDP register, but now
CPDMA does it.

Hence, add additional check for Next Descriptor Pointer != null in
cpdma_chan_process() function before writing in HDP register.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 0924014..379314f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
 	chan->count--;
 	chan->stats.good_dequeue++;
 
-	if (status & CPDMA_DESC_EOQ) {
+	if ((status & CPDMA_DESC_EOQ) && chan->head) {
 		chan->stats.requeue++;
 		chan_write(chan, hdp, desc_phys(pool, chan->head));
 	}
-- 
2.10.1

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Ivan Khoronzhuk]

On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
> The currently processing cpdma descriptor with EOQ flag set may
> contain two values in Next Descriptor Pointer field:
> - valid pointer: means CPDMA missed addition of new desc in queue;
It shouldn't happen in normal circumstances, right?
So, why it happens only for egress channels? And Does that mean
there is some resynchronization between submit and process function,
or this is h/w issue?

> - null: no more descriptors in queue.
> In the later case, it's not required to write to HDP register, but now
> CPDMA does it.
> 
> Hence, add additional check for Next Descriptor Pointer != null in
> cpdma_chan_process() function before writing in HDP register.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 0924014..379314f 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>  	chan->count--;
>  	chan->stats.good_dequeue++;
>  
> -	if (status & CPDMA_DESC_EOQ) {
> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
>  		chan->stats.requeue++;
>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
>  	}
> -- 
> 2.10.1
> 

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

On 12/02/2016 05:03 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
>> The currently processing cpdma descriptor with EOQ flag set may
>> contain two values in Next Descriptor Pointer field:
>> - valid pointer: means CPDMA missed addition of new desc in queue;
> It shouldn't happen in normal circumstances, right?

it might happen, because desc push compete with desc pop.
You can check stats values:
chan->stats.misqueued
chan->stats.requeue
 under different types of net-loads.

TRM:
"
If the pNext pointer is initially NULL, and more packets need to be queued for transmit, the software
application may alter this pointer to point to a newly appended descriptor. The EMAC will use the new
pointer value and proceed to the next descriptor unless the pNext value has already been read. In this
latter case, the transmitter will halt on the transmit channel in question, and the software application may
restart it at that time. The software can detect this case by checking for an end of queue (EOQ) condition
flag on the updated packet descriptor when it is returned by the EMAC.
"


> So, why it happens only for egress channels? And Does that mean
> there is some resynchronization between submit and process function,
> or this is h/w issue?

no hw issues. this patch just removes one unnecessary I/O access 

> 
>> - null: no more descriptors in queue.
>> In the later case, it's not required to write to HDP register, but now
>> CPDMA does it.
>>
>> Hence, add additional check for Next Descriptor Pointer != null in
>> cpdma_chan_process() function before writing in HDP register.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index 0924014..379314f 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>>  	chan->count--;
>>  	chan->stats.good_dequeue++;
>>  
>> -	if (status & CPDMA_DESC_EOQ) {
>> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
>>  		chan->stats.requeue++;
>>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
>>  	}
>> -- 
>> 2.10.1
>>

-- 
regards,
-grygorii

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

On 12/02/2016 05:03 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
>> The currently processing cpdma descriptor with EOQ flag set may
>> contain two values in Next Descriptor Pointer field:
>> - valid pointer: means CPDMA missed addition of new desc in queue;
> It shouldn't happen in normal circumstances, right?

it might happen, because desc push compete with desc pop.
You can check stats values:
chan->stats.misqueued
chan->stats.requeue
 under different types of net-loads.

TRM:
"
If the pNext pointer is initially NULL, and more packets need to be queued for transmit, the software
application may alter this pointer to point to a newly appended descriptor. The EMAC will use the new
pointer value and proceed to the next descriptor unless the pNext value has already been read. In this
latter case, the transmitter will halt on the transmit channel in question, and the software application may
restart it at that time. The software can detect this case by checking for an end of queue (EOQ) condition
flag on the updated packet descriptor when it is returned by the EMAC.
"


> So, why it happens only for egress channels? And Does that mean
> there is some resynchronization between submit and process function,
> or this is h/w issue?

no hw issues. this patch just removes one unnecessary I/O access 

> 
>> - null: no more descriptors in queue.
>> In the later case, it's not required to write to HDP register, but now
>> CPDMA does it.
>>
>> Hence, add additional check for Next Descriptor Pointer != null in
>> cpdma_chan_process() function before writing in HDP register.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index 0924014..379314f 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>>  	chan->count--;
>>  	chan->stats.good_dequeue++;
>>  
>> -	if (status & CPDMA_DESC_EOQ) {
>> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
>>  		chan->stats.requeue++;
>>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
>>  	}
>> -- 
>> 2.10.1
>>

-- 
regards,
-grygorii

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Ivan Khoronzhuk]

On Fri, Dec 02, 2016 at 10:45:07AM -0600, Grygorii Strashko wrote:
> 
> 
> On 12/02/2016 05:03 AM, Ivan Khoronzhuk wrote:
> > On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
> >> The currently processing cpdma descriptor with EOQ flag set may
> >> contain two values in Next Descriptor Pointer field:
> >> - valid pointer: means CPDMA missed addition of new desc in queue;
> > It shouldn't happen in normal circumstances, right?
> 
> it might happen, because desc push compete with desc pop.
> You can check stats values:
> chan->stats.misqueued
> chan->stats.requeue
>  under different types of net-loads.
I've done this, of-course.
By whole logic the misqueued counter has to cover all cases.
But that's not true.

> 
> TRM:
> "
> If the pNext pointer is initially NULL, and more packets need to be queued for transmit, the software
> application may alter this pointer to point to a newly appended descriptor. The EMAC will use the new
> pointer value and proceed to the next descriptor unless the pNext value has already been read. In this
> latter case, the transmitter will halt on the transmit channel in question, and the software application may
> restart it at that time. The software can detect this case by checking for an end of queue (EOQ) condition
> flag on the updated packet descriptor when it is returned by the EMAC.
> "
That's true. No issues in desc.
In the code no any place to update next_desc except submit function.

And this case is supposed to be caught here:
For submit:
cpdma_chan_submit()
spin_lock_irqsave(&chan->lock);
...
--->__cpdma_chan_submit()
...
------> desc_write(prev, hw_next, desc_dma); // here next pointer is updated, it can be not in time
...
------> mode = desc_read(prev, hw_mode); // pay attention, it's read after updating next pointer
------> if ((mode & CPDMA_DESC_EOQ) &&
------> (chan->state == CPDMA_STATE_ACTIVE)) { // here checked if it was late update
---------> chan_write(chan, hdp, desc_dma); // here transmit is restarted, if needed

For process it only caught the fact of late update, but it has to be caught in
submit() already:
__cpdma_chan_process()
spin_lock_irqsave(&chan->lock);
------> if (mode & CPDMA_DESC_EOQ) // here transmit is restarted, if needed
---------> chan_write(chan, hdp, desc_dma); // but w/o updating next pointer

Seems there is no place where hw_next is updated w/o updating hdp :-| in case
of late hw_next set. And that is strange. I know it happens, I've checked it
before of-course. Then I thought, maybe there is some problem with write order,
thus out of sync, nothing more.

> 
> 
> > So, why it happens only for egress channels? And Does that mean
> > there is some resynchronization between submit and process function,
> > or this is h/w issue?
> 
> no hw issues. this patch just removes one unnecessary I/O access 
No objections against patch. Anyway it's better then before.
Just want to know the real reason why it happens, maybe there is smth else.

> 
> > 
> >> - null: no more descriptors in queue.
> >> In the later case, it's not required to write to HDP register, but now
> >> CPDMA does it.
> >>
> >> Hence, add additional check for Next Descriptor Pointer != null in
> >> cpdma_chan_process() function before writing in HDP register.
> >>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> ---
> >>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> index 0924014..379314f 100644
> >> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> >> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
> >>  	chan->count--;
> >>  	chan->stats.good_dequeue++;
> >>  
> >> -	if (status & CPDMA_DESC_EOQ) {
> >> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
> >>  		chan->stats.requeue++;
> >>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
> >>  	}
> >> -- 
> >> 2.10.1
> >>
> 
> -- 
> regards,
> -grygorii

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

On 12/02/2016 05:28 PM, Ivan Khoronzhuk wrote:
> On Fri, Dec 02, 2016 at 10:45:07AM -0600, Grygorii Strashko wrote:
>>
>>
>> On 12/02/2016 05:03 AM, Ivan Khoronzhuk wrote:
>>> On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
>>>> The currently processing cpdma descriptor with EOQ flag set may
>>>> contain two values in Next Descriptor Pointer field:
>>>> - valid pointer: means CPDMA missed addition of new desc in queue;
>>> It shouldn't happen in normal circumstances, right?
>>
>> it might happen, because desc push compete with desc pop.
>> You can check stats values:
>> chan->stats.misqueued
>> chan->stats.requeue
>>  under different types of net-loads.
> I've done this, of-course.
> By whole logic the misqueued counter has to cover all cases.
> But that's not true.
> 
>>
>> TRM:
>> "
>> If the pNext pointer is initially NULL, and more packets need to be queued for transmit, the software
>> application may alter this pointer to point to a newly appended descriptor. The EMAC will use the new
>> pointer value and proceed to the next descriptor unless the pNext value has already been read. In this
>> latter case, the transmitter will halt on the transmit channel in question, and the software application may
>> restart it at that time. The software can detect this case by checking for an end of queue (EOQ) condition
>> flag on the updated packet descriptor when it is returned by the EMAC.
>> "
> That's true. No issues in desc.
> In the code no any place to update next_desc except submit function.
> 
> And this case is supposed to be caught here:
> For submit:
> cpdma_chan_submit()
> spin_lock_irqsave(&chan->lock);
> ...
> --->__cpdma_chan_submit()
> ...
> ------> desc_write(prev, hw_next, desc_dma); // here next pointer is updated, it can be not in time
> ...
> ------> mode = desc_read(prev, hw_mode); // pay attention, it's read after updating next pointer
> ------> if ((mode & CPDMA_DESC_EOQ) &&
> ------> (chan->state == CPDMA_STATE_ACTIVE)) { // here checked if it was late update
> ---------> chan_write(chan, hdp, desc_dma); // here transmit is restarted, if needed

You've forgot about CPPI hw itself - it's not sync by sw, and so continue run in
background - as result, CPPI might read "next" already, but flags are not updated yet.

> 
> For process it only caught the fact of late update, but it has to be caught in
> submit() already:
> __cpdma_chan_process()
> spin_lock_irqsave(&chan->lock);
> ------> if (mode & CPDMA_DESC_EOQ) // here transmit is restarted, if needed
> ---------> chan_write(chan, hdp, desc_dma); // but w/o updating next pointer
> 
> Seems there is no place where hw_next is updated w/o updating hdp :-| in case
> of late hw_next set. And that is strange. I know it happens, I've checked it
> before of-course. Then I thought, maybe there is some problem with write order,
> thus out of sync, nothing more.
> 
>>
>>
>>> So, why it happens only for egress channels? And Does that mean
>>> there is some resynchronization between submit and process function,
>>> or this is h/w issue?
>>
>> no hw issues. this patch just removes one unnecessary I/O access 
> No objections against patch. Anyway it's better then before.
> Just want to know the real reason why it happens, maybe there is smth else.
> 
>>
>>>
>>>> - null: no more descriptors in queue.
>>>> In the later case, it's not required to write to HDP register, but now
>>>> CPDMA does it.
>>>>
>>>> Hence, add additional check for Next Descriptor Pointer != null in
>>>> cpdma_chan_process() function before writing in HDP register.
>>>>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> index 0924014..379314f 100644
>>>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>>>>  	chan->count--;
>>>>  	chan->stats.good_dequeue++;
>>>>  
>>>> -	if (status & CPDMA_DESC_EOQ) {
>>>> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
>>>>  		chan->stats.requeue++;
>>>>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
>>>>  	}
>>>> -- 
>>>> 2.10.1
>>>>
>>
>> -- 
>> regards,
>> -grygorii

-- 
regards,
-grygorii

Re: [PATCH 2/7] net: ethernet: ti: cpdma: fix desc re-queuing

[Grygorii Strashko]

On 12/02/2016 05:28 PM, Ivan Khoronzhuk wrote:
> On Fri, Dec 02, 2016 at 10:45:07AM -0600, Grygorii Strashko wrote:
>>
>>
>> On 12/02/2016 05:03 AM, Ivan Khoronzhuk wrote:
>>> On Thu, Dec 01, 2016 at 05:34:27PM -0600, Grygorii Strashko wrote:
>>>> The currently processing cpdma descriptor with EOQ flag set may
>>>> contain two values in Next Descriptor Pointer field:
>>>> - valid pointer: means CPDMA missed addition of new desc in queue;
>>> It shouldn't happen in normal circumstances, right?
>>
>> it might happen, because desc push compete with desc pop.
>> You can check stats values:
>> chan->stats.misqueued
>> chan->stats.requeue
>>  under different types of net-loads.
> I've done this, of-course.
> By whole logic the misqueued counter has to cover all cases.
> But that's not true.
> 
>>
>> TRM:
>> "
>> If the pNext pointer is initially NULL, and more packets need to be queued for transmit, the software
>> application may alter this pointer to point to a newly appended descriptor. The EMAC will use the new
>> pointer value and proceed to the next descriptor unless the pNext value has already been read. In this
>> latter case, the transmitter will halt on the transmit channel in question, and the software application may
>> restart it at that time. The software can detect this case by checking for an end of queue (EOQ) condition
>> flag on the updated packet descriptor when it is returned by the EMAC.
>> "
> That's true. No issues in desc.
> In the code no any place to update next_desc except submit function.
> 
> And this case is supposed to be caught here:
> For submit:
> cpdma_chan_submit()
> spin_lock_irqsave(&chan->lock);
> ...
> --->__cpdma_chan_submit()
> ...
> ------> desc_write(prev, hw_next, desc_dma); // here next pointer is updated, it can be not in time
> ...
> ------> mode = desc_read(prev, hw_mode); // pay attention, it's read after updating next pointer
> ------> if ((mode & CPDMA_DESC_EOQ) &&
> ------> (chan->state == CPDMA_STATE_ACTIVE)) { // here checked if it was late update
> ---------> chan_write(chan, hdp, desc_dma); // here transmit is restarted, if needed

You've forgot about CPPI hw itself - it's not sync by sw, and so continue run in
background - as result, CPPI might read "next" already, but flags are not updated yet.

> 
> For process it only caught the fact of late update, but it has to be caught in
> submit() already:
> __cpdma_chan_process()
> spin_lock_irqsave(&chan->lock);
> ------> if (mode & CPDMA_DESC_EOQ) // here transmit is restarted, if needed
> ---------> chan_write(chan, hdp, desc_dma); // but w/o updating next pointer
> 
> Seems there is no place where hw_next is updated w/o updating hdp :-| in case
> of late hw_next set. And that is strange. I know it happens, I've checked it
> before of-course. Then I thought, maybe there is some problem with write order,
> thus out of sync, nothing more.
> 
>>
>>
>>> So, why it happens only for egress channels? And Does that mean
>>> there is some resynchronization between submit and process function,
>>> or this is h/w issue?
>>
>> no hw issues. this patch just removes one unnecessary I/O access 
> No objections against patch. Anyway it's better then before.
> Just want to know the real reason why it happens, maybe there is smth else.
> 
>>
>>>
>>>> - null: no more descriptors in queue.
>>>> In the later case, it's not required to write to HDP register, but now
>>>> CPDMA does it.
>>>>
>>>> Hence, add additional check for Next Descriptor Pointer != null in
>>>> cpdma_chan_process() function before writing in HDP register.
>>>>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>>> ---
>>>>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> index 0924014..379314f 100644
>>>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>>>> @@ -1152,7 +1152,7 @@ static int __cpdma_chan_process(struct cpdma_chan *chan)
>>>>  	chan->count--;
>>>>  	chan->stats.good_dequeue++;
>>>>  
>>>> -	if (status & CPDMA_DESC_EOQ) {
>>>> +	if ((status & CPDMA_DESC_EOQ) && chan->head) {
>>>>  		chan->stats.requeue++;
>>>>  		chan_write(chan, hdp, desc_phys(pool, chan->head));
>>>>  	}
>>>> -- 
>>>> 2.10.1
>>>>
>>
>> -- 
>> regards,
>> -grygorii

-- 
regards,
-grygorii

[PATCH 3/7] net: ethernet: ti: cpdma: minimize number of parameters in cpdma_desc_pool_create/destroy()

[Grygorii Strashko]

Update cpdma_desc_pool_create/destroy() to accept only one parameter
struct cpdma_ctlr*, as this structure contains all required
information for pool creation/destruction.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 66 ++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 379314f..db0a7fd 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -181,8 +181,10 @@ static struct cpdma_control_info controls[] = {
 				 (directed << CPDMA_TO_PORT_SHIFT));	\
 	} while (0)
 
-static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
+static void cpdma_desc_pool_destroy(struct cpdma_ctlr *ctlr)
 {
+	struct cpdma_desc_pool *pool = ctlr->pool;
+
 	if (!pool)
 		return;
 
@@ -191,7 +193,7 @@ static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
 	     gen_pool_size(pool->gen_pool),
 	     gen_pool_avail(pool->gen_pool));
 	if (pool->cpumap)
-		dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
+		dma_free_coherent(ctlr->dev, pool->mem_size, pool->cpumap,
 				  pool->phys);
 	else
 		iounmap(pool->iomap);
@@ -203,57 +205,60 @@ static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
  * devices (e.g. cpsw switches) use plain old memory.  Descriptor pools
  * abstract out these details
  */
-static struct cpdma_desc_pool *
-cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr,
-				int size, int align)
+int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 {
+	struct cpdma_params *cpdma_params = &ctlr->params;
 	struct cpdma_desc_pool *pool;
-	int ret;
+	int ret = 0;
 
-	pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL);
+	pool = devm_kzalloc(ctlr->dev, sizeof(*pool), GFP_KERNEL);
 	if (!pool)
 		goto gen_pool_create_fail;
+	ctlr->pool = pool;
 
-	pool->dev	= dev;
-	pool->mem_size	= size;
-	pool->desc_size	= ALIGN(sizeof(struct cpdma_desc), align);
-	pool->num_desc	= size / pool->desc_size;
+	pool->mem_size	= cpdma_params->desc_mem_size;
+	pool->desc_size	= ALIGN(sizeof(struct cpdma_desc),
+				cpdma_params->desc_align);
+	pool->num_desc	= pool->mem_size / pool->desc_size;
 
-	pool->gen_pool = devm_gen_pool_create(dev, ilog2(pool->desc_size), -1,
-					      "cpdma");
+	pool->gen_pool = devm_gen_pool_create(ctlr->dev, ilog2(pool->desc_size),
+					      -1, "cpdma");
 	if (IS_ERR(pool->gen_pool)) {
-		dev_err(dev, "pool create failed %ld\n",
-			PTR_ERR(pool->gen_pool));
+		ret = PTR_ERR(pool->gen_pool);
+		dev_err(ctlr->dev, "pool create failed %d\n", ret);
 		goto gen_pool_create_fail;
 	}
 
-	if (phys) {
-		pool->phys  = phys;
-		pool->iomap = ioremap(phys, size); /* should be memremap? */
-		pool->hw_addr = hw_addr;
+	if (cpdma_params->desc_mem_phys) {
+		pool->phys  = cpdma_params->desc_mem_phys;
+		pool->iomap = ioremap(pool->phys, pool->mem_size);
+		pool->hw_addr = cpdma_params->desc_hw_addr;
 	} else {
-		pool->cpumap = dma_alloc_coherent(dev, size, &pool->hw_addr,
-						  GFP_KERNEL);
+		pool->cpumap = dma_alloc_coherent(ctlr->dev,  pool->mem_size,
+						  &pool->hw_addr, GFP_KERNEL);
 		pool->iomap = (void __iomem __force *)pool->cpumap;
 		pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */
 	}
 
-	if (!pool->iomap)
+	if (!pool->iomap) {
+		ret = -ENOMEM;
 		goto gen_pool_create_fail;
+	}
 
 	ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap,
 				pool->phys, pool->mem_size, -1);
 	if (ret < 0) {
-		dev_err(dev, "pool add failed %d\n", ret);
+		dev_err(ctlr->dev, "pool add failed %d\n", ret);
 		goto gen_pool_add_virt_fail;
 	}
 
-	return pool;
+	return 0;
 
 gen_pool_add_virt_fail:
-	cpdma_desc_pool_destroy(pool);
+	cpdma_desc_pool_destroy(ctlr);
 gen_pool_create_fail:
-	return NULL;
+	ctlr->pool = NULL;
+	return ret;
 }
 
 static inline dma_addr_t desc_phys(struct cpdma_desc_pool *pool,
@@ -502,12 +507,7 @@ struct cpdma_ctlr *cpdma_ctlr_create(struct cpdma_params *params)
 	ctlr->chan_num = 0;
 	spin_lock_init(&ctlr->lock);
 
-	ctlr->pool = cpdma_desc_pool_create(ctlr->dev,
-					    ctlr->params.desc_mem_phys,
-					    ctlr->params.desc_hw_addr,
-					    ctlr->params.desc_mem_size,
-					    ctlr->params.desc_align);
-	if (!ctlr->pool)
+	if (cpdma_desc_pool_create(ctlr))
 		return NULL;
 
 	if (WARN_ON(ctlr->num_chan > CPDMA_MAX_CHANNELS))
@@ -623,7 +623,7 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
 	for (i = 0; i < ARRAY_SIZE(ctlr->channels); i++)
 		cpdma_chan_destroy(ctlr->channels[i]);
 
-	cpdma_desc_pool_destroy(ctlr->pool);
+	cpdma_desc_pool_destroy(ctlr);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpdma_ctlr_destroy);
-- 
2.10.1

[PATCH 3/7] net: ethernet: ti: cpdma: minimize number of parameters in cpdma_desc_pool_create/destroy()

[Grygorii Strashko]

Update cpdma_desc_pool_create/destroy() to accept only one parameter
struct cpdma_ctlr*, as this structure contains all required
information for pool creation/destruction.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 66 ++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 379314f..db0a7fd 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -181,8 +181,10 @@ static struct cpdma_control_info controls[] = {
 				 (directed << CPDMA_TO_PORT_SHIFT));	\
 	} while (0)
 
-static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
+static void cpdma_desc_pool_destroy(struct cpdma_ctlr *ctlr)
 {
+	struct cpdma_desc_pool *pool = ctlr->pool;
+
 	if (!pool)
 		return;
 
@@ -191,7 +193,7 @@ static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
 	     gen_pool_size(pool->gen_pool),
 	     gen_pool_avail(pool->gen_pool));
 	if (pool->cpumap)
-		dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
+		dma_free_coherent(ctlr->dev, pool->mem_size, pool->cpumap,
 				  pool->phys);
 	else
 		iounmap(pool->iomap);
@@ -203,57 +205,60 @@ static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
  * devices (e.g. cpsw switches) use plain old memory.  Descriptor pools
  * abstract out these details
  */
-static struct cpdma_desc_pool *
-cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr,
-				int size, int align)
+int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 {
+	struct cpdma_params *cpdma_params = &ctlr->params;
 	struct cpdma_desc_pool *pool;
-	int ret;
+	int ret = 0;
 
-	pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL);
+	pool = devm_kzalloc(ctlr->dev, sizeof(*pool), GFP_KERNEL);
 	if (!pool)
 		goto gen_pool_create_fail;
+	ctlr->pool = pool;
 
-	pool->dev	= dev;
-	pool->mem_size	= size;
-	pool->desc_size	= ALIGN(sizeof(struct cpdma_desc), align);
-	pool->num_desc	= size / pool->desc_size;
+	pool->mem_size	= cpdma_params->desc_mem_size;
+	pool->desc_size	= ALIGN(sizeof(struct cpdma_desc),
+				cpdma_params->desc_align);
+	pool->num_desc	= pool->mem_size / pool->desc_size;
 
-	pool->gen_pool = devm_gen_pool_create(dev, ilog2(pool->desc_size), -1,
-					      "cpdma");
+	pool->gen_pool = devm_gen_pool_create(ctlr->dev, ilog2(pool->desc_size),
+					      -1, "cpdma");
 	if (IS_ERR(pool->gen_pool)) {
-		dev_err(dev, "pool create failed %ld\n",
-			PTR_ERR(pool->gen_pool));
+		ret = PTR_ERR(pool->gen_pool);
+		dev_err(ctlr->dev, "pool create failed %d\n", ret);
 		goto gen_pool_create_fail;
 	}
 
-	if (phys) {
-		pool->phys  = phys;
-		pool->iomap = ioremap(phys, size); /* should be memremap? */
-		pool->hw_addr = hw_addr;
+	if (cpdma_params->desc_mem_phys) {
+		pool->phys  = cpdma_params->desc_mem_phys;
+		pool->iomap = ioremap(pool->phys, pool->mem_size);
+		pool->hw_addr = cpdma_params->desc_hw_addr;
 	} else {
-		pool->cpumap = dma_alloc_coherent(dev, size, &pool->hw_addr,
-						  GFP_KERNEL);
+		pool->cpumap = dma_alloc_coherent(ctlr->dev,  pool->mem_size,
+						  &pool->hw_addr, GFP_KERNEL);
 		pool->iomap = (void __iomem __force *)pool->cpumap;
 		pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */
 	}
 
-	if (!pool->iomap)
+	if (!pool->iomap) {
+		ret = -ENOMEM;
 		goto gen_pool_create_fail;
+	}
 
 	ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap,
 				pool->phys, pool->mem_size, -1);
 	if (ret < 0) {
-		dev_err(dev, "pool add failed %d\n", ret);
+		dev_err(ctlr->dev, "pool add failed %d\n", ret);
 		goto gen_pool_add_virt_fail;
 	}
 
-	return pool;
+	return 0;
 
 gen_pool_add_virt_fail:
-	cpdma_desc_pool_destroy(pool);
+	cpdma_desc_pool_destroy(ctlr);
 gen_pool_create_fail:
-	return NULL;
+	ctlr->pool = NULL;
+	return ret;
 }
 
 static inline dma_addr_t desc_phys(struct cpdma_desc_pool *pool,
@@ -502,12 +507,7 @@ struct cpdma_ctlr *cpdma_ctlr_create(struct cpdma_params *params)
 	ctlr->chan_num = 0;
 	spin_lock_init(&ctlr->lock);
 
-	ctlr->pool = cpdma_desc_pool_create(ctlr->dev,
-					    ctlr->params.desc_mem_phys,
-					    ctlr->params.desc_hw_addr,
-					    ctlr->params.desc_mem_size,
-					    ctlr->params.desc_align);
-	if (!ctlr->pool)
+	if (cpdma_desc_pool_create(ctlr))
 		return NULL;
 
 	if (WARN_ON(ctlr->num_chan > CPDMA_MAX_CHANNELS))
@@ -623,7 +623,7 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
 	for (i = 0; i < ARRAY_SIZE(ctlr->channels); i++)
 		cpdma_chan_destroy(ctlr->channels[i]);
 
-	cpdma_desc_pool_destroy(ctlr->pool);
+	cpdma_desc_pool_destroy(ctlr);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cpdma_ctlr_destroy);
-- 
2.10.1

[PATCH 4/7] net: ethernet: ti: cpdma: use devm_ioremap

[Grygorii Strashko]

Use devm_ioremap() and simplify the code.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index db0a7fd..ba892bb 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -195,8 +195,6 @@ static void cpdma_desc_pool_destroy(struct cpdma_ctlr *ctlr)
 	if (pool->cpumap)
 		dma_free_coherent(ctlr->dev, pool->mem_size, pool->cpumap,
 				  pool->phys);
-	else
-		iounmap(pool->iomap);
 }
 
 /*
@@ -231,7 +229,8 @@ int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 
 	if (cpdma_params->desc_mem_phys) {
 		pool->phys  = cpdma_params->desc_mem_phys;
-		pool->iomap = ioremap(pool->phys, pool->mem_size);
+		pool->iomap = devm_ioremap(ctlr->dev, pool->phys,
+					   pool->mem_size);
 		pool->hw_addr = cpdma_params->desc_hw_addr;
 	} else {
 		pool->cpumap = dma_alloc_coherent(ctlr->dev,  pool->mem_size,
-- 
2.10.1

[PATCH 4/7] net: ethernet: ti: cpdma: use devm_ioremap

[Grygorii Strashko]

Use devm_ioremap() and simplify the code.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/davinci_cpdma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index db0a7fd..ba892bb 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -195,8 +195,6 @@ static void cpdma_desc_pool_destroy(struct cpdma_ctlr *ctlr)
 	if (pool->cpumap)
 		dma_free_coherent(ctlr->dev, pool->mem_size, pool->cpumap,
 				  pool->phys);
-	else
-		iounmap(pool->iomap);
 }
 
 /*
@@ -231,7 +229,8 @@ int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 
 	if (cpdma_params->desc_mem_phys) {
 		pool->phys  = cpdma_params->desc_mem_phys;
-		pool->iomap = ioremap(pool->phys, pool->mem_size);
+		pool->iomap = devm_ioremap(ctlr->dev, pool->phys,
+					   pool->mem_size);
 		pool->hw_addr = cpdma_params->desc_hw_addr;
 	} else {
 		pool->cpumap = dma_alloc_coherent(ctlr->dev,  pool->mem_size,
-- 
2.10.1

[PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

Add optional property "descs_pool_size" to specify buffer descriptor's
pool size. The "descs_pool_size" should define total number of CPDMA
CPPI descriptors to be used for both ingress/egress packets
processing. If not specified - the default value 256 will be used
which will allow to place descriptor's pool into the internal CPPI
RAM on most of TI SoC.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 5ad439f..b99d196 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -35,6 +35,11 @@ Optional properties:
 			  For example in dra72x-evm, pcf gpio has to be
 			  driven low so that cpsw slave 0 and phy data
 			  lines are connected via mux.
+- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
+			  both ingress/egress packets processing. if not
+			  specified the default value 256 will be used which
+			  will allow to place descriptors pool into the
+			  internal CPPI RAM.
 
 
 Slave Properties:
-- 
2.10.1

[PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

Add optional property "descs_pool_size" to specify buffer descriptor's
pool size. The "descs_pool_size" should define total number of CPDMA
CPPI descriptors to be used for both ingress/egress packets
processing. If not specified - the default value 256 will be used
which will allow to place descriptor's pool into the internal CPPI
RAM on most of TI SoC.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 5ad439f..b99d196 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -35,6 +35,11 @@ Optional properties:
 			  For example in dra72x-evm, pcf gpio has to be
 			  driven low so that cpsw slave 0 and phy data
 			  lines are connected via mux.
+- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
+			  both ingress/egress packets processing. if not
+			  specified the default value 256 will be used which
+			  will allow to place descriptors pool into the
+			  internal CPPI RAM.
 
 
 Slave Properties:
-- 
2.10.1

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Ivan Khoronzhuk]

On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
> Add optional property "descs_pool_size" to specify buffer descriptor's
> pool size. The "descs_pool_size" should define total number of CPDMA
> CPPI descriptors to be used for both ingress/egress packets
> processing. If not specified - the default value 256 will be used
> which will allow to place descriptor's pool into the internal CPPI
> RAM on most of TI SoC.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 5ad439f..b99d196 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -35,6 +35,11 @@ Optional properties:
>  			  For example in dra72x-evm, pcf gpio has to be
>  			  driven low so that cpsw slave 0 and phy data
>  			  lines are connected via mux.
> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
> +			  both ingress/egress packets processing. if not
> +			  specified the default value 256 will be used which
> +			  will allow to place descriptors pool into the
> +			  internal CPPI RAM.
Does it describe h/w? Why now module parameter? or even smth like ethtool num
ring entries?

>  
>  Slave Properties:
> -- 
> 2.10.1
> 

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>> Add optional property "descs_pool_size" to specify buffer descriptor's
>> pool size. The "descs_pool_size" should define total number of CPDMA
>> CPPI descriptors to be used for both ingress/egress packets
>> processing. If not specified - the default value 256 will be used
>> which will allow to place descriptor's pool into the internal CPPI
>> RAM on most of TI SoC.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 5ad439f..b99d196 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -35,6 +35,11 @@ Optional properties:
>>  			  For example in dra72x-evm, pcf gpio has to be
>>  			  driven low so that cpsw slave 0 and phy data
>>  			  lines are connected via mux.
>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>> +			  both ingress/egress packets processing. if not
>> +			  specified the default value 256 will be used which
>> +			  will allow to place descriptors pool into the
>> +			  internal CPPI RAM.
> Does it describe h/w? Why now module parameter? or even smth like ethtool num
> ring entries?
> 

It can be module parameter too. for the use cases i'm aware of -
this is one-time boot setting only.  

----- OR
So, do you propose to use 
       ethtool -g ethX

       ethtool -G ethX [rx N] [tx N]
?

Now cpdma has one pool for all RX/TX channels, so changing this settings
by ethtool will require: pause interfaces, reallocate cpdma pool, 
re-arrange buffers between channels, resume interface. Correct?

How do you think - we can move forward with one pool or better to have two (Rx and Tx)?

Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?

How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
- increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
- decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)

----- OR ----
Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?



-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>> Add optional property "descs_pool_size" to specify buffer descriptor's
>> pool size. The "descs_pool_size" should define total number of CPDMA
>> CPPI descriptors to be used for both ingress/egress packets
>> processing. If not specified - the default value 256 will be used
>> which will allow to place descriptor's pool into the internal CPPI
>> RAM on most of TI SoC.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 5ad439f..b99d196 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -35,6 +35,11 @@ Optional properties:
>>  			  For example in dra72x-evm, pcf gpio has to be
>>  			  driven low so that cpsw slave 0 and phy data
>>  			  lines are connected via mux.
>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>> +			  both ingress/egress packets processing. if not
>> +			  specified the default value 256 will be used which
>> +			  will allow to place descriptors pool into the
>> +			  internal CPPI RAM.
> Does it describe h/w? Why now module parameter? or even smth like ethtool num
> ring entries?
> 

It can be module parameter too. for the use cases i'm aware of -
this is one-time boot setting only.  

----- OR
So, do you propose to use 
       ethtool -g ethX

       ethtool -G ethX [rx N] [tx N]
?

Now cpdma has one pool for all RX/TX channels, so changing this settings
by ethtool will require: pause interfaces, reallocate cpdma pool, 
re-arrange buffers between channels, resume interface. Correct?

How do you think - we can move forward with one pool or better to have two (Rx and Tx)?

Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?

How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
- increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
- decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)

----- OR ----
Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?



-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 11:21 AM, Grygorii Strashko wrote:
> 
> 
> On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
>> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>>> Add optional property "descs_pool_size" to specify buffer descriptor's
>>> pool size. The "descs_pool_size" should define total number of CPDMA
>>> CPPI descriptors to be used for both ingress/egress packets
>>> processing. If not specified - the default value 256 will be used
>>> which will allow to place descriptor's pool into the internal CPPI
>>> RAM on most of TI SoC.
>>>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 5ad439f..b99d196 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -35,6 +35,11 @@ Optional properties:
>>>  			  For example in dra72x-evm, pcf gpio has to be
>>>  			  driven low so that cpsw slave 0 and phy data
>>>  			  lines are connected via mux.
>>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>>> +			  both ingress/egress packets processing. if not
>>> +			  specified the default value 256 will be used which
>>> +			  will allow to place descriptors pool into the
>>> +			  internal CPPI RAM.
>> Does it describe h/w? Why now module parameter? or even smth like ethtool num
>> ring entries?
>>
> 
> It can be module parameter too. for the use cases i'm aware of -
> this is one-time boot setting only.  
> 
> ----- OR
> So, do you propose to use 
>        ethtool -g ethX
> 
>        ethtool -G ethX [rx N] [tx N]
> ?
> 
> Now cpdma has one pool for all RX/TX channels, so changing this settings
> by ethtool will require: pause interfaces, reallocate cpdma pool, 
> re-arrange buffers between channels, resume interface. Correct?
> 
> How do you think - we can move forward with one pool or better to have two (Rx and Tx)?
> 
> Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
> cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?
> 
> How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
> - increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
> - decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)
> 
> ----- OR ----
> Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
> and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?
> 
> 

if no comments here, I'll rework patches to use module parameter for descs_pool_size and
will add possibility to re-split RX/TX buffers using  ethtool -G ethX [rx N] [tx N]

-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 11:21 AM, Grygorii Strashko wrote:
> 
> 
> On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
>> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>>> Add optional property "descs_pool_size" to specify buffer descriptor's
>>> pool size. The "descs_pool_size" should define total number of CPDMA
>>> CPPI descriptors to be used for both ingress/egress packets
>>> processing. If not specified - the default value 256 will be used
>>> which will allow to place descriptor's pool into the internal CPPI
>>> RAM on most of TI SoC.
>>>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 5ad439f..b99d196 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -35,6 +35,11 @@ Optional properties:
>>>  			  For example in dra72x-evm, pcf gpio has to be
>>>  			  driven low so that cpsw slave 0 and phy data
>>>  			  lines are connected via mux.
>>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>>> +			  both ingress/egress packets processing. if not
>>> +			  specified the default value 256 will be used which
>>> +			  will allow to place descriptors pool into the
>>> +			  internal CPPI RAM.
>> Does it describe h/w? Why now module parameter? or even smth like ethtool num
>> ring entries?
>>
> 
> It can be module parameter too. for the use cases i'm aware of -
> this is one-time boot setting only.  
> 
> ----- OR
> So, do you propose to use 
>        ethtool -g ethX
> 
>        ethtool -G ethX [rx N] [tx N]
> ?
> 
> Now cpdma has one pool for all RX/TX channels, so changing this settings
> by ethtool will require: pause interfaces, reallocate cpdma pool, 
> re-arrange buffers between channels, resume interface. Correct?
> 
> How do you think - we can move forward with one pool or better to have two (Rx and Tx)?
> 
> Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
> cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?
> 
> How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
> - increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
> - decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)
> 
> ----- OR ----
> Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
> and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?
> 
> 

if no comments here, I'll rework patches to use module parameter for descs_pool_size and
will add possibility to re-split RX/TX buffers using  ethtool -G ethX [rx N] [tx N]

-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>> Add optional property "descs_pool_size" to specify buffer descriptor's
>> pool size. The "descs_pool_size" should define total number of CPDMA
>> CPPI descriptors to be used for both ingress/egress packets
>> processing. If not specified - the default value 256 will be used
>> which will allow to place descriptor's pool into the internal CPPI
>> RAM on most of TI SoC.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 5ad439f..b99d196 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -35,6 +35,11 @@ Optional properties:
>>  			  For example in dra72x-evm, pcf gpio has to be
>>  			  driven low so that cpsw slave 0 and phy data
>>  			  lines are connected via mux.
>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>> +			  both ingress/egress packets processing. if not
>> +			  specified the default value 256 will be used which
>> +			  will allow to place descriptors pool into the
>> +			  internal CPPI RAM.
> Does it describe h/w? Why now module parameter? or even smth like ethtool num
> ring entries?
> 

It can be module parameter too. in general this is expected to be 
 one-time boot setting only.  

----- OR
So, do you propose to use 
       ethtool -g ethX

       ethtool -G ethX [rx N] [tx N]
?

Now cpdma has one pool for all RX/TX channels, so changing this settings
by ethtool will require: pause interfaces, reallocate cpdma pool, 
re-arrange buffers between channels, resume interface. Correct?

How do you think - we can move forward with one pool or better to have two (Rx and Tx)?

Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?

How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
- increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
- decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)

----- OR ----
Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?



-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Grygorii Strashko]

On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
> On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
>> Add optional property "descs_pool_size" to specify buffer descriptor's
>> pool size. The "descs_pool_size" should define total number of CPDMA
>> CPPI descriptors to be used for both ingress/egress packets
>> processing. If not specified - the default value 256 will be used
>> which will allow to place descriptor's pool into the internal CPPI
>> RAM on most of TI SoC.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 5ad439f..b99d196 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -35,6 +35,11 @@ Optional properties:
>>  			  For example in dra72x-evm, pcf gpio has to be
>>  			  driven low so that cpsw slave 0 and phy data
>>  			  lines are connected via mux.
>> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
>> +			  both ingress/egress packets processing. if not
>> +			  specified the default value 256 will be used which
>> +			  will allow to place descriptors pool into the
>> +			  internal CPPI RAM.
> Does it describe h/w? Why now module parameter? or even smth like ethtool num
> ring entries?
> 

It can be module parameter too. in general this is expected to be 
 one-time boot setting only.  

----- OR
So, do you propose to use 
       ethtool -g ethX

       ethtool -G ethX [rx N] [tx N]
?

Now cpdma has one pool for all RX/TX channels, so changing this settings
by ethtool will require: pause interfaces, reallocate cpdma pool, 
re-arrange buffers between channels, resume interface. Correct?

How do you think - we can move forward with one pool or better to have two (Rx and Tx)?

Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?

How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
- increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
- decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)

----- OR ----
Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?



-- 
regards,
-grygorii

Re: [PATCH 5/7] Documentation: DT: net: cpsw: allow to specify descriptors pool size

[Ivan Khoronzhuk]

On Fri, Dec 02, 2016 at 11:22:28AM -0600, Grygorii Strashko wrote:
> 
> 
> On 12/02/2016 05:28 AM, Ivan Khoronzhuk wrote:
> > On Thu, Dec 01, 2016 at 05:34:30PM -0600, Grygorii Strashko wrote:
> >> Add optional property "descs_pool_size" to specify buffer descriptor's
> >> pool size. The "descs_pool_size" should define total number of CPDMA
> >> CPPI descriptors to be used for both ingress/egress packets
> >> processing. If not specified - the default value 256 will be used
> >> which will allow to place descriptor's pool into the internal CPPI
> >> RAM on most of TI SoC.
> >>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> ---
> >>  Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> >> index 5ad439f..b99d196 100644
> >> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> >> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> >> @@ -35,6 +35,11 @@ Optional properties:
> >>  			  For example in dra72x-evm, pcf gpio has to be
> >>  			  driven low so that cpsw slave 0 and phy data
> >>  			  lines are connected via mux.
> >> +- descs_pool_size	: total number of CPDMA CPPI descriptors to be used for
> >> +			  both ingress/egress packets processing. if not
> >> +			  specified the default value 256 will be used which
> >> +			  will allow to place descriptors pool into the
> >> +			  internal CPPI RAM.
> > Does it describe h/w? Why now module parameter? or even smth like ethtool num
> > ring entries?
> > 
> 
> It can be module parameter too. in general this is expected to be 
>  one-time boot setting only.  
> 
> ----- OR
> So, do you propose to use 
>        ethtool -g ethX
> 
>        ethtool -G ethX [rx N] [tx N]
> ?
It has a little different names, but yes, why not?
No need, maybe, but....It's just a proposition, at least I was thinking
about it after proposition from +cc Schuyler Patton to leave rx desc num
property. In this case it's possible to tune tx/rx desc num ratio, even
with SRAM descs.

> 
> Now cpdma has one pool for all RX/TX channels, so changing this settings
> by ethtool will require: pause interfaces, reallocate cpdma pool, 
Pause can lead to losts only for rx, and only for very short time, so
it's not very bad, especially when user knows what he is doing.


> re-arrange buffers between channels, resume interface. Correct?
correct.

But, some alternative variants can be used, like replacing descriptors.
Shrink num of desc for every channels to 1, replace/add others, and expand.
In this case no losts, but it's harder to debug issues after....

> 
> How do you think - we can move forward with one pool or better to have two (Rx and Tx)?
I think one is enough, just split, if no harm on perf.

> 
> Wouldn't it be reasonable to still have DT (or module) parameter to avoid 
> cpdma reconfiguration on system startup (pause/resume interfaces) (faster boot)?
Would be, your choice, but it's not flexible.

> 
> How about cpdma re-allocation policy (with expectation that is shouldn't happen too often)?
> - increasing of Rx, Tx will grow total number of physically allocated buffers (total_desc_num)
> - decreasing of Rx, Tx will just change number of available buffers (no memory re-allocation)
> 
> ----- OR ----
> Can we move forward with current patch (total number of CPDMA CPPI descriptors defined in DT) 
> and add ethtool -G ethX [rx N] [tx N] which will allow to re-split descs between RX and TX?
No objections, It anyway requires re-allocations. Re-split of Rx and Tx will
not have a lot changes as most code exists already.

> 
> 
> 
> -- 
> regards,
> -grygorii

[PATCH 6/7] net: ethernet: ti: cpsw: add support for descs_pool_size dt property

[Grygorii Strashko]

The CPSW CPDMA can process buffer descriptors placed as in internal
CPPI RAM as in DDR. This patch adds support in CPSW and CPDMA for
descs_pool_size DT property, which defines total number of CPDMA CPPI
descriptors to be used for both ingress/egress packets processing:
 - memory size required for CPDMA descriptor pool is calculated basing
on number of descriptors specified by user in descs_pool_size and
CPDMA descriptor size;
 - allocate CPDMA descriptor pool in DDR if pool memory size >
internal CPPI RAM or use internal CPPI RAM otherwise;
 - if descs_pool_size not specified in DT - the default value 256 will
be used which will allow to place CPDMA descriptors pool into the
internal CPPI RAM (current default behaviour);
 - CPDMA will ignore descs_pool_size if descs_pool_size = 0 for
backward comaptiobility with davinci_emac.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c          |  5 +++++
 drivers/net/ethernet/ti/cpsw.h          |  1 +
 drivers/net/ethernet/ti/davinci_cpdma.c | 12 ++++++++++++
 drivers/net/ethernet/ti/davinci_cpdma.h |  1 +
 4 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index dd5d830..a98c6260 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -145,6 +145,7 @@ do {								\
 		cpsw->data.active_slave)
 #define IRQ_NUM			2
 #define CPSW_MAX_QUEUES		8
+#define CPSW_CPDMA_DESCS_POOL_SIZE_DEFAULT 256
 
 static int debug_level;
 module_param(debug_level, int, 0);
@@ -2557,6 +2558,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	if (of_property_read_bool(node, "dual_emac"))
 		data->dual_emac = 1;
 
+	data->descs_pool_size = CPSW_CPDMA_DESCS_POOL_SIZE_DEFAULT;
+	of_property_read_u32(node, "descs_pool_size", &data->descs_pool_size);
+
 	/*
 	 * Populate all the child nodes here...
 	 */
@@ -2967,6 +2971,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	dma_params.has_ext_regs		= true;
 	dma_params.desc_hw_addr         = dma_params.desc_mem_phys;
 	dma_params.bus_freq_mhz		= cpsw->bus_freq_mhz;
+	dma_params.descs_pool_size	= cpsw->data.descs_pool_size;
 
 	cpsw->dma = cpdma_ctlr_create(&dma_params);
 	if (!cpsw->dma) {
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 16b54c6..8835d79 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -38,6 +38,7 @@ struct cpsw_platform_data {
 	u32	mac_control;	/* Mac control register */
 	u16	default_vlan;	/* Def VLAN for ALE lookup in VLAN aware mode*/
 	bool	dual_emac;	/* Enable Dual EMAC mode */
+	u32	descs_pool_size;	/* Number of Rx/Tx Descriptios */
 };
 
 void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave);
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index ba892bb..f45bb8a 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -219,6 +219,18 @@ int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 				cpdma_params->desc_align);
 	pool->num_desc	= pool->mem_size / pool->desc_size;
 
+	if (cpdma_params->descs_pool_size) {
+		/* recalculate memory size required cpdma descriptor pool
+		 * basing on number of descriptors specified by user and
+		 * if memory size > CPPI internal RAM size (desc_mem_size)
+		 * then switch to use DDR
+		 */
+		pool->num_desc = cpdma_params->descs_pool_size;
+		pool->mem_size = pool->desc_size * pool->num_desc;
+		if (pool->mem_size > cpdma_params->desc_mem_size)
+			cpdma_params->desc_mem_phys = 0;
+	}
+
 	pool->gen_pool = devm_gen_pool_create(ctlr->dev, ilog2(pool->desc_size),
 					      -1, "cpdma");
 	if (IS_ERR(pool->gen_pool)) {
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
index 4a167db..cb45f8f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.h
+++ b/drivers/net/ethernet/ti/davinci_cpdma.h
@@ -37,6 +37,7 @@ struct cpdma_params {
 	int			desc_mem_size;
 	int			desc_align;
 	u32			bus_freq_mhz;
+	u32			descs_pool_size;
 
 	/*
 	 * Some instances of embedded cpdma controllers have extra control and
-- 
2.10.1

[PATCH 6/7] net: ethernet: ti: cpsw: add support for descs_pool_size dt property

[Grygorii Strashko]

The CPSW CPDMA can process buffer descriptors placed as in internal
CPPI RAM as in DDR. This patch adds support in CPSW and CPDMA for
descs_pool_size DT property, which defines total number of CPDMA CPPI
descriptors to be used for both ingress/egress packets processing:
 - memory size required for CPDMA descriptor pool is calculated basing
on number of descriptors specified by user in descs_pool_size and
CPDMA descriptor size;
 - allocate CPDMA descriptor pool in DDR if pool memory size >
internal CPPI RAM or use internal CPPI RAM otherwise;
 - if descs_pool_size not specified in DT - the default value 256 will
be used which will allow to place CPDMA descriptors pool into the
internal CPPI RAM (current default behaviour);
 - CPDMA will ignore descs_pool_size if descs_pool_size = 0 for
backward comaptiobility with davinci_emac.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c          |  5 +++++
 drivers/net/ethernet/ti/cpsw.h          |  1 +
 drivers/net/ethernet/ti/davinci_cpdma.c | 12 ++++++++++++
 drivers/net/ethernet/ti/davinci_cpdma.h |  1 +
 4 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index dd5d830..a98c6260 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -145,6 +145,7 @@ do {								\
 		cpsw->data.active_slave)
 #define IRQ_NUM			2
 #define CPSW_MAX_QUEUES		8
+#define CPSW_CPDMA_DESCS_POOL_SIZE_DEFAULT 256
 
 static int debug_level;
 module_param(debug_level, int, 0);
@@ -2557,6 +2558,9 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	if (of_property_read_bool(node, "dual_emac"))
 		data->dual_emac = 1;
 
+	data->descs_pool_size = CPSW_CPDMA_DESCS_POOL_SIZE_DEFAULT;
+	of_property_read_u32(node, "descs_pool_size", &data->descs_pool_size);
+
 	/*
 	 * Populate all the child nodes here...
 	 */
@@ -2967,6 +2971,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	dma_params.has_ext_regs		= true;
 	dma_params.desc_hw_addr         = dma_params.desc_mem_phys;
 	dma_params.bus_freq_mhz		= cpsw->bus_freq_mhz;
+	dma_params.descs_pool_size	= cpsw->data.descs_pool_size;
 
 	cpsw->dma = cpdma_ctlr_create(&dma_params);
 	if (!cpsw->dma) {
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index 16b54c6..8835d79 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -38,6 +38,7 @@ struct cpsw_platform_data {
 	u32	mac_control;	/* Mac control register */
 	u16	default_vlan;	/* Def VLAN for ALE lookup in VLAN aware mode*/
 	bool	dual_emac;	/* Enable Dual EMAC mode */
+	u32	descs_pool_size;	/* Number of Rx/Tx Descriptios */
 };
 
 void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave);
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index ba892bb..f45bb8a 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -219,6 +219,18 @@ int cpdma_desc_pool_create(struct cpdma_ctlr *ctlr)
 				cpdma_params->desc_align);
 	pool->num_desc	= pool->mem_size / pool->desc_size;
 
+	if (cpdma_params->descs_pool_size) {
+		/* recalculate memory size required cpdma descriptor pool
+		 * basing on number of descriptors specified by user and
+		 * if memory size > CPPI internal RAM size (desc_mem_size)
+		 * then switch to use DDR
+		 */
+		pool->num_desc = cpdma_params->descs_pool_size;
+		pool->mem_size = pool->desc_size * pool->num_desc;
+		if (pool->mem_size > cpdma_params->desc_mem_size)
+			cpdma_params->desc_mem_phys = 0;
+	}
+
 	pool->gen_pool = devm_gen_pool_create(ctlr->dev, ilog2(pool->desc_size),
 					      -1, "cpdma");
 	if (IS_ERR(pool->gen_pool)) {
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
index 4a167db..cb45f8f 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.h
+++ b/drivers/net/ethernet/ti/davinci_cpdma.h
@@ -37,6 +37,7 @@ struct cpdma_params {
 	int			desc_mem_size;
 	int			desc_align;
 	u32			bus_freq_mhz;
+	u32			descs_pool_size;
 
 	/*
 	 * Some instances of embedded cpdma controllers have extra control and
-- 
2.10.1

[PATCH 7/7] Documentation: DT: net: cpsw: remove no_bd_ram property

[Grygorii Strashko]

Even if no_bd_ram property is described in TI CPSW bindings the
support for it has never been introduced in CPSW driver, so there are
no real users of it. Hence, remove no_bd_ram property.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt | 3 ---
 arch/arm/boot/dts/am33xx.dtsi                  | 1 -
 arch/arm/boot/dts/am4372.dtsi                  | 1 -
 arch/arm/boot/dts/dm814x.dtsi                  | 1 -
 arch/arm/boot/dts/dra7.dtsi                    | 1 -
 5 files changed, 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index b99d196..4e8b673 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -25,7 +25,6 @@ Required properties:
 
 Optional properties:
 - ti,hwmods		: Must be "cpgmac0"
-- no_bd_ram		: Must be 0 or 1
 - dual_emac		: Specifies Switch to act as Dual EMAC
 - syscon		: Phandle to the system control device node, which is
 			  the control module device of the am33x
@@ -73,7 +72,6 @@ Examples:
 		cpdma_channels = <8>;
 		ale_entries = <1024>;
 		bd_ram_size = <0x2000>;
-		no_bd_ram = <0>;
 		rx_descs = <64>;
 		mac_control = <0x20>;
 		slaves = <2>;
@@ -102,7 +100,6 @@ Examples:
 		cpdma_channels = <8>;
 		ale_entries = <1024>;
 		bd_ram_size = <0x2000>;
-		no_bd_ram = <0>;
 		rx_descs = <64>;
 		mac_control = <0x20>;
 		slaves = <2>;
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 194d884..7af5520 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -777,7 +777,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index a275fa9..4f651be 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -668,7 +668,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/dm814x.dtsi b/arch/arm/boot/dts/dm814x.dtsi
index ff90a6c..614a4ba 100644
--- a/arch/arm/boot/dts/dm814x.dtsi
+++ b/arch/arm/boot/dts/dm814x.dtsi
@@ -508,7 +508,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index d4fcd68..cf7325d 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1706,7 +1706,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
-- 
2.10.1

[PATCH 7/7] Documentation: DT: net: cpsw: remove no_bd_ram property

[Grygorii Strashko]

Even if no_bd_ram property is described in TI CPSW bindings the
support for it has never been introduced in CPSW driver, so there are
no real users of it. Hence, remove no_bd_ram property.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 Documentation/devicetree/bindings/net/cpsw.txt | 3 ---
 arch/arm/boot/dts/am33xx.dtsi                  | 1 -
 arch/arm/boot/dts/am4372.dtsi                  | 1 -
 arch/arm/boot/dts/dm814x.dtsi                  | 1 -
 arch/arm/boot/dts/dra7.dtsi                    | 1 -
 5 files changed, 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index b99d196..4e8b673 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -25,7 +25,6 @@ Required properties:
 
 Optional properties:
 - ti,hwmods		: Must be "cpgmac0"
-- no_bd_ram		: Must be 0 or 1
 - dual_emac		: Specifies Switch to act as Dual EMAC
 - syscon		: Phandle to the system control device node, which is
 			  the control module device of the am33x
@@ -73,7 +72,6 @@ Examples:
 		cpdma_channels = <8>;
 		ale_entries = <1024>;
 		bd_ram_size = <0x2000>;
-		no_bd_ram = <0>;
 		rx_descs = <64>;
 		mac_control = <0x20>;
 		slaves = <2>;
@@ -102,7 +100,6 @@ Examples:
 		cpdma_channels = <8>;
 		ale_entries = <1024>;
 		bd_ram_size = <0x2000>;
-		no_bd_ram = <0>;
 		rx_descs = <64>;
 		mac_control = <0x20>;
 		slaves = <2>;
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 194d884..7af5520 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -777,7 +777,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index a275fa9..4f651be 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -668,7 +668,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/dm814x.dtsi b/arch/arm/boot/dts/dm814x.dtsi
index ff90a6c..614a4ba 100644
--- a/arch/arm/boot/dts/dm814x.dtsi
+++ b/arch/arm/boot/dts/dm814x.dtsi
@@ -508,7 +508,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index d4fcd68..cf7325d 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1706,7 +1706,6 @@
 			cpdma_channels = <8>;
 			ale_entries = <1024>;
 			bd_ram_size = <0x2000>;
-			no_bd_ram = <0>;
 			mac_control = <0x20>;
 			slaves = <2>;
 			active_slave = <0>;
-- 
2.10.1