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

[Ivan Khoronzhuk]

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@xxxxxxxxxx>
> > > > ---
> > > >
> > > > 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