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()? -- regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html