* [PATCH] net: ethernet: ti: cpsw: remove rx_descs property
@ 2016-06-02 22:43 Ivan Khoronzhuk
2016-06-03 16:50 ` Grygorii Strashko
0 siblings, 1 reply; 5+ messages in thread
From: Ivan Khoronzhuk @ 2016-06-02 22:43 UTC (permalink / raw)
To: mugunthanvnm, linux-kernel
Cc: grygorii.strashko, linux-omap, netdev, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, bcousson, tony, devicetree,
Ivan Khoronzhuk
There is no reason to hold s/w dependent parameter in device tree.
Even more, there is no reason in this parameter because davinici_cpdma
driver splits pool of descriptors equally between tx and rx channels.
That is, if number of descriptors 256, 128 of them are for rx
channels. While receiving, the descriptor is freed to the pool and
then allocated with new skb. And if in DT the "rx_descs" is set to
64, then 128 - 64 = 64 descriptors are always in the pool and cannot
be used, for tx, for instance. It's not correct resource usage,
better to set it to half of pool, then the rx pool can be used in
full. It will not have any impact on performance, as anyway, the
"redundant" descriptors were unused.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
Based on master
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 -
drivers/net/ethernet/ti/cpsw.c | 13 +++----------
drivers/net/ethernet/ti/cpsw.h | 1 -
drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++
drivers/net/ethernet/ti/davinci_cpdma.h | 1 +
9 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
index 0ae0649..5fe6239 100644
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -15,7 +15,6 @@ Required properties:
- cpdma_channels : Specifies number of channels in CPDMA
- ale_entries : Specifies No of entries ALE can hold
- bd_ram_size : Specifies internal descriptor RAM size
-- rx_descs : Specifies number of Rx descriptors
- mac_control : Specifies Default MAC control register content
for the specific platform
- slaves : Specifies number for slaves
@@ -70,7 +69,6 @@ Examples:
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
mac_control = <0x20>;
slaves = <2>;
active_slave = <0>;
@@ -99,7 +97,6 @@ Examples:
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
mac_control = <0x20>;
slaves = <2>;
active_slave = <0>;
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 52be48b..702126f 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -766,7 +766,6 @@
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
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 12fcde4..a10fa7f 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -626,7 +626,6 @@
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
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 d4537dc..f23cae0c 100644
--- a/arch/arm/boot/dts/dm814x.dtsi
+++ b/arch/arm/boot/dts/dm814x.dtsi
@@ -509,7 +509,6 @@
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
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 e007401..b7ddc64 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1626,7 +1626,6 @@
ale_entries = <1024>;
bd_ram_size = <0x2000>;
no_bd_ram = <0>;
- rx_descs = <64>;
mac_control = <0x20>;
slaves = <2>;
active_slave = <0>;
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 4b08a2f..635be3e 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1277,6 +1277,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
if (!cpsw_common_res_usage_state(priv)) {
+ int buf_num;
struct cpsw_priv *priv_sl0 = cpsw_get_slave_priv(priv, 0);
/* setup tx dma to fixed prio and zero offset */
@@ -1305,10 +1306,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
enable_irq(priv->irqs_table[0]);
}
- if (WARN_ON(!priv->data.rx_descs))
- priv->data.rx_descs = 128;
-
- for (i = 0; i < priv->data.rx_descs; i++) {
+ buf_num = cpdma_chan_get_buf_num(priv->dma) / 2;
+ for (i = 0; i < buf_num; i++) {
struct sk_buff *skb;
ret = -ENOMEM;
@@ -1999,12 +1998,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
}
data->bd_ram_size = prop;
- if (of_property_read_u32(node, "rx_descs", &prop)) {
- dev_err(&pdev->dev, "Missing rx_descs property in the DT.\n");
- return -EINVAL;
- }
- data->rx_descs = prop;
-
if (of_property_read_u32(node, "mac_control", &prop)) {
dev_err(&pdev->dev, "Missing mac_control property in the DT.\n");
return -EINVAL;
diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
index e50afd1..16b54c6 100644
--- a/drivers/net/ethernet/ti/cpsw.h
+++ b/drivers/net/ethernet/ti/cpsw.h
@@ -35,7 +35,6 @@ struct cpsw_platform_data {
u32 cpts_clock_shift; /* convert input clock ticks to nanoseconds */
u32 ale_entries; /* ale table size */
u32 bd_ram_size; /*buffer descriptor ram size */
- u32 rx_descs; /* Number of Rx Descriptios */
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 */
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 18bf3a8..395647c 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -543,6 +543,12 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
}
EXPORT_SYMBOL_GPL(cpdma_chan_create);
+int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr)
+{
+ return ctlr->pool->num_desc;
+}
+EXPORT_SYMBOL_GPL(cpdma_chan_get_buf_num);
+
int cpdma_chan_destroy(struct cpdma_chan *chan)
{
struct cpdma_ctlr *ctlr;
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
index 86dee48..f059856 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.h
+++ b/drivers/net/ethernet/ti/davinci_cpdma.h
@@ -81,6 +81,7 @@ int cpdma_ctlr_dump(struct cpdma_ctlr *ctlr);
struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
cpdma_handler_fn handler);
+int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr);
int cpdma_chan_destroy(struct cpdma_chan *chan);
int cpdma_chan_start(struct cpdma_chan *chan);
int cpdma_chan_stop(struct cpdma_chan *chan);
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property
2016-06-02 22:43 [PATCH] net: ethernet: ti: cpsw: remove rx_descs property Ivan Khoronzhuk
@ 2016-06-03 16:50 ` Grygorii Strashko
2016-06-03 18:25 ` Ivan Khoronzhuk
0 siblings, 1 reply; 5+ messages in thread
From: Grygorii Strashko @ 2016-06-03 16:50 UTC (permalink / raw)
To: Ivan Khoronzhuk, mugunthanvnm, linux-kernel
Cc: linux-omap, netdev, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, bcousson, tony, devicetree
On 06/03/2016 01:43 AM, Ivan Khoronzhuk wrote:
> There is no reason to hold s/w dependent parameter in device tree.
> Even more, there is no reason in this parameter because davinici_cpdma
> driver splits pool of descriptors equally between tx and rx channels.
> That is, if number of descriptors 256, 128 of them are for rx
> channels. While receiving, the descriptor is freed to the pool and
> then allocated with new skb. And if in DT the "rx_descs" is set to
> 64, then 128 - 64 = 64 descriptors are always in the pool and cannot
> be used, for tx, for instance. It's not correct resource usage,
> better to set it to half of pool, then the rx pool can be used in
> full. It will not have any impact on performance, as anyway, the
> "redundant" descriptors were unused.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>
> Based on master
>
> 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 -
Pls, split DT and non-DT changes, seems code changes should go first.
> drivers/net/ethernet/ti/cpsw.c | 13 +++----------
> drivers/net/ethernet/ti/cpsw.h | 1 -
> drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++
> drivers/net/ethernet/ti/davinci_cpdma.h | 1 +
> 9 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
> index 0ae0649..5fe6239 100644
> --- a/Documentation/devicetree/bindings/net/cpsw.txt
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -15,7 +15,6 @@ Required properties:
> - cpdma_channels : Specifies number of channels in CPDMA
> - ale_entries : Specifies No of entries ALE can hold
> - bd_ram_size : Specifies internal descriptor RAM size
> -- rx_descs : Specifies number of Rx descriptors
> - mac_control : Specifies Default MAC control register content
> for the specific platform
> - slaves : Specifies number for slaves
> @@ -70,7 +69,6 @@ Examples:
[....]
> slaves = <2>;
> active_slave = <0>;
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 4b08a2f..635be3e 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1277,6 +1277,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
> ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>
> if (!cpsw_common_res_usage_state(priv)) {
> + int buf_num;
> struct cpsw_priv *priv_sl0 = cpsw_get_slave_priv(priv, 0);
>
> /* setup tx dma to fixed prio and zero offset */
> @@ -1305,10 +1306,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
> enable_irq(priv->irqs_table[0]);
> }
>
> - if (WARN_ON(!priv->data.rx_descs))
> - priv->data.rx_descs = 128;
> -
> - for (i = 0; i < priv->data.rx_descs; i++) {
> + buf_num = cpdma_chan_get_buf_num(priv->dma) / 2;
Could you get rid of "/ 2", pls?
> + for (i = 0; i < buf_num; i++) {
> struct sk_buff *skb;
>
> ret = -ENOMEM;
> @@ -1999,12 +1998,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
> }
> data->bd_ram_size = prop;
>
> - if (of_property_read_u32(node, "rx_descs", &prop)) {
> - dev_err(&pdev->dev, "Missing rx_descs property in the DT.\n");
> - return -EINVAL;
> - }
> - data->rx_descs = prop;
> -
> if (of_property_read_u32(node, "mac_control", &prop)) {
> dev_err(&pdev->dev, "Missing mac_control property in the DT.\n");
> return -EINVAL;
> diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
> index e50afd1..16b54c6 100644
> --- a/drivers/net/ethernet/ti/cpsw.h
> +++ b/drivers/net/ethernet/ti/cpsw.h
> @@ -35,7 +35,6 @@ struct cpsw_platform_data {
> u32 cpts_clock_shift; /* convert input clock ticks to nanoseconds */
> u32 ale_entries; /* ale table size */
> u32 bd_ram_size; /*buffer descriptor ram size */
> - u32 rx_descs; /* Number of Rx Descriptios */
> 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 */
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index 18bf3a8..395647c 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -543,6 +543,12 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
> }
> EXPORT_SYMBOL_GPL(cpdma_chan_create);
>
> +int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr)
> +{
> + return ctlr->pool->num_desc;
> +}
> +EXPORT_SYMBOL_GPL(cpdma_chan_get_buf_num);
> +
> int cpdma_chan_destroy(struct cpdma_chan *chan)
> {
> struct cpdma_ctlr *ctlr;
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
> index 86dee48..f059856 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.h
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.h
> @@ -81,6 +81,7 @@ int cpdma_ctlr_dump(struct cpdma_ctlr *ctlr);
>
> struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
> cpdma_handler_fn handler);
> +int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr);
> int cpdma_chan_destroy(struct cpdma_chan *chan);
> int cpdma_chan_start(struct cpdma_chan *chan);
> int cpdma_chan_stop(struct cpdma_chan *chan);
>
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property
2016-06-03 16:50 ` Grygorii Strashko
@ 2016-06-03 18:25 ` Ivan Khoronzhuk
2016-06-03 19:13 ` Grygorii Strashko
0 siblings, 1 reply; 5+ messages in thread
From: Ivan Khoronzhuk @ 2016-06-03 18:25 UTC (permalink / raw)
To: Grygorii Strashko, mugunthanvnm, linux-kernel
Cc: linux-omap, netdev, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, bcousson, tony, devicetree
On 03.06.16 19:50, Grygorii Strashko wrote:
> On 06/03/2016 01:43 AM, Ivan Khoronzhuk wrote:
>> There is no reason to hold s/w dependent parameter in device tree.
>> Even more, there is no reason in this parameter because davinici_cpdma
>> driver splits pool of descriptors equally between tx and rx channels.
>> That is, if number of descriptors 256, 128 of them are for rx
>> channels. While receiving, the descriptor is freed to the pool and
>> then allocated with new skb. And if in DT the "rx_descs" is set to
>> 64, then 128 - 64 = 64 descriptors are always in the pool and cannot
>> be used, for tx, for instance. It's not correct resource usage,
>> better to set it to half of pool, then the rx pool can be used in
>> full. It will not have any impact on performance, as anyway, the
>> "redundant" descriptors were unused.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>
>> Based on master
>>
>> 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 -
>
>
> Pls, split DT and non-DT changes, seems code changes should go first.
Ok.
>
>> drivers/net/ethernet/ti/cpsw.c | 13 +++----------
>> drivers/net/ethernet/ti/cpsw.h | 1 -
>> drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++
>> drivers/net/ethernet/ti/davinci_cpdma.h | 1 +
>> 9 files changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>> index 0ae0649..5fe6239 100644
>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>> @@ -15,7 +15,6 @@ Required properties:
>> - cpdma_channels : Specifies number of channels in CPDMA
>> - ale_entries : Specifies No of entries ALE can hold
>> - bd_ram_size : Specifies internal descriptor RAM size
>> -- rx_descs : Specifies number of Rx descriptors
>> - mac_control : Specifies Default MAC control register content
>> for the specific platform
>> - slaves : Specifies number for slaves
>> @@ -70,7 +69,6 @@ Examples:
>
> [....]
>
>> slaves = <2>;
>> active_slave = <0>;
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index 4b08a2f..635be3e 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -1277,6 +1277,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>> ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>>
>> if (!cpsw_common_res_usage_state(priv)) {
>> + int buf_num;
>> struct cpsw_priv *priv_sl0 = cpsw_get_slave_priv(priv, 0);
>>
>> /* setup tx dma to fixed prio and zero offset */
>> @@ -1305,10 +1306,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
>> enable_irq(priv->irqs_table[0]);
>> }
>>
>> - if (WARN_ON(!priv->data.rx_descs))
>> - priv->data.rx_descs = 128;
>> -
>> - for (i = 0; i < priv->data.rx_descs; i++) {
>> + buf_num = cpdma_chan_get_buf_num(priv->dma) / 2;
>
> Could you get rid of "/ 2", pls?
>
Why? compiler is smart enough to translate it to shift.
And this is not time critical place.
Anyway, will change it to >> 1 while splitting.
>> + for (i = 0; i < buf_num; i++) {
>> struct sk_buff *skb;
>>
>> ret = -ENOMEM;
>> @@ -1999,12 +1998,6 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
>> }
>> data->bd_ram_size = prop;
>>
>> - if (of_property_read_u32(node, "rx_descs", &prop)) {
>> - dev_err(&pdev->dev, "Missing rx_descs property in the DT.\n");
>> - return -EINVAL;
>> - }
>> - data->rx_descs = prop;
>> -
>> if (of_property_read_u32(node, "mac_control", &prop)) {
>> dev_err(&pdev->dev, "Missing mac_control property in the DT.\n");
>> return -EINVAL;
>> diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h
>> index e50afd1..16b54c6 100644
>> --- a/drivers/net/ethernet/ti/cpsw.h
>> +++ b/drivers/net/ethernet/ti/cpsw.h
>> @@ -35,7 +35,6 @@ struct cpsw_platform_data {
>> u32 cpts_clock_shift; /* convert input clock ticks to nanoseconds */
>> u32 ale_entries; /* ale table size */
>> u32 bd_ram_size; /*buffer descriptor ram size */
>> - u32 rx_descs; /* Number of Rx Descriptios */
>> 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 */
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index 18bf3a8..395647c 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -543,6 +543,12 @@ struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
>> }
>> EXPORT_SYMBOL_GPL(cpdma_chan_create);
>>
>> +int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr)
>> +{
>> + return ctlr->pool->num_desc;
>> +}
>> +EXPORT_SYMBOL_GPL(cpdma_chan_get_buf_num);
>> +
>> int cpdma_chan_destroy(struct cpdma_chan *chan)
>> {
>> struct cpdma_ctlr *ctlr;
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.h b/drivers/net/ethernet/ti/davinci_cpdma.h
>> index 86dee48..f059856 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.h
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.h
>> @@ -81,6 +81,7 @@ int cpdma_ctlr_dump(struct cpdma_ctlr *ctlr);
>>
>> struct cpdma_chan *cpdma_chan_create(struct cpdma_ctlr *ctlr, int chan_num,
>> cpdma_handler_fn handler);
>> +int cpdma_chan_get_buf_num(struct cpdma_ctlr *ctlr);
>> int cpdma_chan_destroy(struct cpdma_chan *chan);
>> int cpdma_chan_start(struct cpdma_chan *chan);
>> int cpdma_chan_stop(struct cpdma_chan *chan);
>>
>
>
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property
2016-06-03 18:25 ` Ivan Khoronzhuk
@ 2016-06-03 19:13 ` Grygorii Strashko
2016-06-03 19:36 ` Ivan Khoronzhuk
0 siblings, 1 reply; 5+ messages in thread
From: Grygorii Strashko @ 2016-06-03 19:13 UTC (permalink / raw)
To: Ivan Khoronzhuk, mugunthanvnm, linux-kernel
Cc: linux-omap, netdev, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, bcousson, tony, devicetree
On 06/03/2016 09:25 PM, Ivan Khoronzhuk wrote:
>
>
> On 03.06.16 19:50, Grygorii Strashko wrote:
>> On 06/03/2016 01:43 AM, Ivan Khoronzhuk wrote:
>>> There is no reason to hold s/w dependent parameter in device tree.
>>> Even more, there is no reason in this parameter because davinici_cpdma
>>> driver splits pool of descriptors equally between tx and rx channels.
>>> That is, if number of descriptors 256, 128 of them are for rx
>>> channels. While receiving, the descriptor is freed to the pool and
>>> then allocated with new skb. And if in DT the "rx_descs" is set to
>>> 64, then 128 - 64 = 64 descriptors are always in the pool and cannot
>>> be used, for tx, for instance. It's not correct resource usage,
>>> better to set it to half of pool, then the rx pool can be used in
>>> full. It will not have any impact on performance, as anyway, the
>>> "redundant" descriptors were unused.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>> ---
>>>
>>> Based on master
>>>
>>> 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 -
>>
>>
>> Pls, split DT and non-DT changes, seems code changes should go first.
> Ok.
>
>>
>>> drivers/net/ethernet/ti/cpsw.c | 13 +++----------
>>> drivers/net/ethernet/ti/cpsw.h | 1 -
>>> drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++
>>> drivers/net/ethernet/ti/davinci_cpdma.h | 1 +
>>> 9 files changed, 10 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt
>>> b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 0ae0649..5fe6239 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -15,7 +15,6 @@ Required properties:
>>> - cpdma_channels : Specifies number of channels in CPDMA
>>> - ale_entries : Specifies No of entries ALE can hold
>>> - bd_ram_size : Specifies internal descriptor RAM size
>>> -- rx_descs : Specifies number of Rx descriptors
>>> - mac_control : Specifies Default MAC control register content
>>> for the specific platform
>>> - slaves : Specifies number for slaves
>>> @@ -70,7 +69,6 @@ Examples:
>>
>> [....]
>>
>>> slaves = <2>;
>>> active_slave = <0>;
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>>> b/drivers/net/ethernet/ti/cpsw.c
>>> index 4b08a2f..635be3e 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -1277,6 +1277,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>> ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>>>
>>> if (!cpsw_common_res_usage_state(priv)) {
>>> + int buf_num;
>>> struct cpsw_priv *priv_sl0 = cpsw_get_slave_priv(priv, 0);
>>>
>>> /* setup tx dma to fixed prio and zero offset */
>>> @@ -1305,10 +1306,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>> enable_irq(priv->irqs_table[0]);
>>> }
>>>
>>> - if (WARN_ON(!priv->data.rx_descs))
>>> - priv->data.rx_descs = 128;
>>> -
>>> - for (i = 0; i < priv->data.rx_descs; i++) {
>>> + buf_num = cpdma_chan_get_buf_num(priv->dma) / 2;
>>
>> Could you get rid of "/ 2", pls?
>>
> Why? compiler is smart enough to translate it to shift.
> And this is not time critical place.
> Anyway, will change it to >> 1 while splitting.
>
I mean here that cpsw, in general, should not have any knowledge about rules
used by cpdma to split pool on rx and tx part. How about cpdma_chan_get_rx_buf_num()?
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property
2016-06-03 19:13 ` Grygorii Strashko
@ 2016-06-03 19:36 ` Ivan Khoronzhuk
0 siblings, 0 replies; 5+ messages in thread
From: Ivan Khoronzhuk @ 2016-06-03 19:36 UTC (permalink / raw)
To: Grygorii Strashko, mugunthanvnm, linux-kernel
Cc: linux-omap, netdev, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, bcousson, tony, devicetree
On 03.06.16 22:13, Grygorii Strashko wrote:
> On 06/03/2016 09:25 PM, Ivan Khoronzhuk wrote:
>>
>>
>> On 03.06.16 19:50, Grygorii Strashko wrote:
>>> On 06/03/2016 01:43 AM, Ivan Khoronzhuk wrote:
>>>> There is no reason to hold s/w dependent parameter in device tree.
>>>> Even more, there is no reason in this parameter because davinici_cpdma
>>>> driver splits pool of descriptors equally between tx and rx channels.
>>>> That is, if number of descriptors 256, 128 of them are for rx
>>>> channels. While receiving, the descriptor is freed to the pool and
>>>> then allocated with new skb. And if in DT the "rx_descs" is set to
>>>> 64, then 128 - 64 = 64 descriptors are always in the pool and cannot
>>>> be used, for tx, for instance. It's not correct resource usage,
>>>> better to set it to half of pool, then the rx pool can be used in
>>>> full. It will not have any impact on performance, as anyway, the
>>>> "redundant" descriptors were unused.
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>>> ---
>>>>
>>>> Based on master
>>>>
>>>> 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 -
>>>
>>>
>>> Pls, split DT and non-DT changes, seems code changes should go first.
>> Ok.
>>
>>>
>>>> drivers/net/ethernet/ti/cpsw.c | 13 +++----------
>>>> drivers/net/ethernet/ti/cpsw.h | 1 -
>>>> drivers/net/ethernet/ti/davinci_cpdma.c | 6 ++++++
>>>> drivers/net/ethernet/ti/davinci_cpdma.h | 1 +
>>>> 9 files changed, 10 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt
>>>> b/Documentation/devicetree/bindings/net/cpsw.txt
>>>> index 0ae0649..5fe6239 100644
>>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>>> @@ -15,7 +15,6 @@ Required properties:
>>>> - cpdma_channels : Specifies number of channels in CPDMA
>>>> - ale_entries : Specifies No of entries ALE can hold
>>>> - bd_ram_size : Specifies internal descriptor RAM size
>>>> -- rx_descs : Specifies number of Rx descriptors
>>>> - mac_control : Specifies Default MAC control register content
>>>> for the specific platform
>>>> - slaves : Specifies number for slaves
>>>> @@ -70,7 +69,6 @@ Examples:
>>>
>>> [....]
>>>
>>>> slaves = <2>;
>>>> active_slave = <0>;
>>>> diff --git a/drivers/net/ethernet/ti/cpsw.c
>>>> b/drivers/net/ethernet/ti/cpsw.c
>>>> index 4b08a2f..635be3e 100644
>>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>>> @@ -1277,6 +1277,7 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>> ALE_ALL_PORTS, ALE_ALL_PORTS, 0, 0);
>>>>
>>>> if (!cpsw_common_res_usage_state(priv)) {
>>>> + int buf_num;
>>>> struct cpsw_priv *priv_sl0 = cpsw_get_slave_priv(priv, 0);
>>>>
>>>> /* setup tx dma to fixed prio and zero offset */
>>>> @@ -1305,10 +1306,8 @@ static int cpsw_ndo_open(struct net_device *ndev)
>>>> enable_irq(priv->irqs_table[0]);
>>>> }
>>>>
>>>> - if (WARN_ON(!priv->data.rx_descs))
>>>> - priv->data.rx_descs = 128;
>>>> -
>>>> - for (i = 0; i < priv->data.rx_descs; i++) {
>>>> + buf_num = cpdma_chan_get_buf_num(priv->dma) / 2;
>>>
>>> Could you get rid of "/ 2", pls?
>>>
>> Why? compiler is smart enough to translate it to shift.
>> And this is not time critical place.
>> Anyway, will change it to >> 1 while splitting.
>>
>
> I mean here that cpsw, in general, should not have any knowledge about rules
> used by cpdma to split pool on rx and tx part. How about cpdma_chan_get_rx_buf_num()?
Yes, it be correct.
Will change it in v2
>
>
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-03 19:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-02 22:43 [PATCH] net: ethernet: ti: cpsw: remove rx_descs property Ivan Khoronzhuk
2016-06-03 16:50 ` Grygorii Strashko
2016-06-03 18:25 ` Ivan Khoronzhuk
2016-06-03 19:13 ` Grygorii Strashko
2016-06-03 19:36 ` Ivan Khoronzhuk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).