From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ivan Khoronzhuk Subject: Re: [PATCH] net: ethernet: ti: cpsw: remove rx_descs property Date: Fri, 3 Jun 2016 21:25:04 +0300 Message-ID: <5751CB80.4060802@linaro.org> References: <1464907420-19694-1-git-send-email-ivan.khoronzhuk@linaro.org> <5751B562.8020708@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5751B562.8020708@ti.com> Sender: linux-kernel-owner@vger.kernel.org To: Grygorii Strashko , mugunthanvnm@ti.com, linux-kernel@vger.kernel.org Cc: linux-omap@vger.kernel.org, netdev@vger.kernel.org, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, bcousson@baylibre.com, tony@atomide.com, devicetree@vger.kernel.org List-Id: devicetree@vger.kernel.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 >> --- >> >> 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