From: Schuyler Patton <spatton@ti.com> To: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>, <mugunthanvnm@ti.com>, <linux-kernel@vger.kernel.org> Cc: <grygorii.strashko@ti.com>, <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> Subject: Re: [PATCH v2 0/2] net: ethernet: ti: cpsw: delete rx_descs property Date: Wed, 8 Jun 2016 18:11:33 -0500 [thread overview] Message-ID: <5758A625.3090707@ti.com> (raw) In-Reply-To: <57582650.5010409@linaro.org> On 06/08/2016 09:06 AM, Ivan Khoronzhuk wrote: > > > On 08.06.16 17:01, Ivan Khoronzhuk wrote: >> Hi Schuyer, >> >> On 07.06.16 18:26, Schuyler Patton wrote: >>> Hi, >>> >>> On 06/07/2016 08:59 AM, Ivan Khoronzhuk wrote: >>>> There is no reason in rx_descs property because davinici_cpdma >>>> driver splits pool of descriptors equally between tx and rx channels. >>>> So, this patch series makes driver to use available number of >>>> descriptors for rx channels. >>> >>> I agree with the idea of consolidating how the descriptors are >>> defined because of >>> the two variable components, number and size of the pool can be >>> confusing to >>> end users. I would like to request to change how it is being >>> proposed here. >>> >>> I think the number of descriptors should be left in the device tree >>> source file as >>> is and remove the BD size variable and have the driver calculate the >>> size of the >>> pool necessary to support the descriptor request. From an user >>> perspective it is >>> easier I think to be able to list the number of descriptors >>> necessary vs. the size >>> of the pool. >>> >>> Since the patch series points out how it is used so in the driver so >>> to make that >>> consistent is perhaps change rx_descs to total_descs. >>> >>> Regards, >>> Schuyler >> >> The DT entry for cpsw doesn't have property for size of the pool. >> It contains only BD ram size, if you mean this. The size of the pool is >> software decision. Current version of DT entry contain only rx desc >> number. >> That is not correct, as it depends on the size of the descriptor, >> which is also >> h/w parameter. The DT entry has to describe only h/w part and >> shouldn't contain >> driver implementation details, and I'm looking on it from this >> perspective. >> >> Besides, rx_descs describes only rx number of descriptors, that are >> taken from >> the same pool as tx descriptors, and setting rx desc to some new >> value doesn't >> mean that rest of them are freed for tx. Also, I'm going to send >> series that >> adds multi channel support to the driver, and in this case, splitting >> of the >> pool will be more sophisticated than now, after what setting those >> parameters >> for user (he should do this via device tree) can be even more >> confusing. But, > should -> shouldn't > >> as it's supposed, it's software decision that shouldn't leak to the DT. If this rx-desc field is removed how will the number of descriptors be set? This field has been used to increase the number of descriptors so high volume short packets are not dropped due to descriptor exhaustion. The current default number of 64 rx descriptors is too low for gigabit networks. Some users have a strong requirement for zero loss of UDP packets setting this field to a larger number and setting the descriptors off-chip was a means to solve the problem. >> >> >>>> >>>> Based on master branch >>>> >>>> Since v1: >>>> - separate device tree and driver patches >>>> - return number of rx buffers from cpdma driver >>>> >>>> Ivan Khoronzhuk (2): >>>> net: ethernet: ti: cpsw: remove rx_descs property >>>> Documentation: DT: cpsw: remove rx_descs property >>>> >>>> Documentation/devicetree/bindings/net/cpsw.txt | 1 - >>>> 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(+), 16 deletions(-) >>>> >>> >> >
WARNING: multiple messages have this Message-ID (diff)
From: Schuyler Patton <spatton@ti.com> To: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>, mugunthanvnm@ti.com, linux-kernel@vger.kernel.org Cc: grygorii.strashko@ti.com, 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 Subject: Re: [PATCH v2 0/2] net: ethernet: ti: cpsw: delete rx_descs property Date: Wed, 8 Jun 2016 18:11:33 -0500 [thread overview] Message-ID: <5758A625.3090707@ti.com> (raw) In-Reply-To: <57582650.5010409@linaro.org> On 06/08/2016 09:06 AM, Ivan Khoronzhuk wrote: > > > On 08.06.16 17:01, Ivan Khoronzhuk wrote: >> Hi Schuyer, >> >> On 07.06.16 18:26, Schuyler Patton wrote: >>> Hi, >>> >>> On 06/07/2016 08:59 AM, Ivan Khoronzhuk wrote: >>>> There is no reason in rx_descs property because davinici_cpdma >>>> driver splits pool of descriptors equally between tx and rx channels. >>>> So, this patch series makes driver to use available number of >>>> descriptors for rx channels. >>> >>> I agree with the idea of consolidating how the descriptors are >>> defined because of >>> the two variable components, number and size of the pool can be >>> confusing to >>> end users. I would like to request to change how it is being >>> proposed here. >>> >>> I think the number of descriptors should be left in the device tree >>> source file as >>> is and remove the BD size variable and have the driver calculate the >>> size of the >>> pool necessary to support the descriptor request. From an user >>> perspective it is >>> easier I think to be able to list the number of descriptors >>> necessary vs. the size >>> of the pool. >>> >>> Since the patch series points out how it is used so in the driver so >>> to make that >>> consistent is perhaps change rx_descs to total_descs. >>> >>> Regards, >>> Schuyler >> >> The DT entry for cpsw doesn't have property for size of the pool. >> It contains only BD ram size, if you mean this. The size of the pool is >> software decision. Current version of DT entry contain only rx desc >> number. >> That is not correct, as it depends on the size of the descriptor, >> which is also >> h/w parameter. The DT entry has to describe only h/w part and >> shouldn't contain >> driver implementation details, and I'm looking on it from this >> perspective. >> >> Besides, rx_descs describes only rx number of descriptors, that are >> taken from >> the same pool as tx descriptors, and setting rx desc to some new >> value doesn't >> mean that rest of them are freed for tx. Also, I'm going to send >> series that >> adds multi channel support to the driver, and in this case, splitting >> of the >> pool will be more sophisticated than now, after what setting those >> parameters >> for user (he should do this via device tree) can be even more >> confusing. But, > should -> shouldn't > >> as it's supposed, it's software decision that shouldn't leak to the DT. If this rx-desc field is removed how will the number of descriptors be set? This field has been used to increase the number of descriptors so high volume short packets are not dropped due to descriptor exhaustion. The current default number of 64 rx descriptors is too low for gigabit networks. Some users have a strong requirement for zero loss of UDP packets setting this field to a larger number and setting the descriptors off-chip was a means to solve the problem. >> >> >>>> >>>> Based on master branch >>>> >>>> Since v1: >>>> - separate device tree and driver patches >>>> - return number of rx buffers from cpdma driver >>>> >>>> Ivan Khoronzhuk (2): >>>> net: ethernet: ti: cpsw: remove rx_descs property >>>> Documentation: DT: cpsw: remove rx_descs property >>>> >>>> Documentation/devicetree/bindings/net/cpsw.txt | 1 - >>>> 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(+), 16 deletions(-) >>>> >>> >> >
next prev parent reply other threads:[~2016-06-08 23:11 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-07 13:59 [PATCH v2 0/2] net: ethernet: ti: cpsw: delete rx_descs property Ivan Khoronzhuk 2016-06-07 13:59 ` [PATCH v2 1/2] net: ethernet: ti: cpsw: remove " Ivan Khoronzhuk 2016-06-11 5:50 ` David Miller 2016-06-11 10:11 ` Ivan Khoronzhuk 2016-06-07 13:59 ` [PATCH v2 2/2] Documentation: DT: " Ivan Khoronzhuk 2016-06-08 20:11 ` Rob Herring 2016-06-07 15:26 ` [PATCH v2 0/2] net: ethernet: ti: cpsw: delete " Schuyler Patton 2016-06-07 15:26 ` Schuyler Patton 2016-06-08 14:01 ` Ivan Khoronzhuk 2016-06-08 14:01 ` Ivan Khoronzhuk 2016-06-08 14:06 ` Ivan Khoronzhuk 2016-06-08 23:11 ` Schuyler Patton [this message] 2016-06-08 23:11 ` Schuyler Patton 2016-06-09 0:03 ` Ivan Khoronzhuk 2016-06-10 23:04 ` Schuyler Patton 2016-06-10 23:04 ` Schuyler Patton 2016-06-13 8:22 ` Mugunthan V N 2016-06-13 8:22 ` Mugunthan V N 2016-06-13 8:22 ` Mugunthan V N 2016-06-13 15:19 ` Andrew F. Davis 2016-06-13 15:19 ` Andrew F. Davis 2016-06-14 12:38 ` Ivan Khoronzhuk 2016-06-14 14:16 ` Mugunthan V N 2016-06-14 14:16 ` Mugunthan V N 2016-06-14 12:26 ` Ivan Khoronzhuk 2016-06-14 12:25 ` Ivan Khoronzhuk
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5758A625.3090707@ti.com \ --to=spatton@ti.com \ --cc=bcousson@baylibre.com \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=grygorii.strashko@ti.com \ --cc=ijc+devicetree@hellion.org.uk \ --cc=ivan.khoronzhuk@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mugunthanvnm@ti.com \ --cc=netdev@vger.kernel.org \ --cc=pawel.moll@arm.com \ --cc=robh+dt@kernel.org \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.