Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property

[Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>]

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