Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tue, Dec 15, 2015 at 4:49 PM, Gilad Avidov wrote: > On Mon, 14 Dec 2015 17:39:09 -0800 > Florian Fainelli wrote: > >> On 14/12/15 16:19, Gilad Avidov wrote: >> >> [snip] >> >> > + "sgmii_irq"; >> > + qcom,emac-gpio-mdc = < 123 0>; >> > +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tue, Dec 15, 2015 at 4:49 PM, Gilad Avidov wrote: > On Mon, 14 Dec 2015 17:39:09 -0800 > Florian Fainelli wrote: > >> On 14/12/15 16:19, Gilad Avidov wrote: >> >> [snip] >> >> > + "sgmii_irq"; >> > + qcom,emac-gpio-mdc

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

David Miller wrote: I think you did something much worse. You quoted the entire huge patch which is entirely inappropriate given the feedback you were trying to give. Sorry about that. I usually do trim it, but I got tired and forgot before I hit send. -- Sent by an employee of the

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

From: Timur Tabi Date: Tue, 15 Dec 2015 18:15:50 -0600 > You forgot to add "[v2]" to the subject line of this email. I think you did something much worse. You quoted the entire huge patch which is entirely inappropriate given the feedback you were trying to give. Thanks. -- To unsubscribe

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Mon, 14 Dec 2015 17:39:09 -0800 Florian Fainelli wrote: > On 14/12/15 16:19, Gilad Avidov wrote: > > [snip] > > > + "sgmii_irq"; > > + qcom,emac-gpio-mdc = < 123 0>; > > + qcom,emac-gpio-mdio = < 124 0>; > > + qcom,emac-tstamp-en; > > +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 15:09:23 Timur Tabi wrote: > Arnd Bergmann wrote: > > If that's in the probe() called from it function, just use writel() > > everywhere, > > a few extra microseconds won't kill the boot time. In general, if a user > > would > > notice the difference, use the relaxed

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Arnd Bergmann wrote: If that's in the probe() called from it function, just use writel() everywhere, a few extra microseconds won't kill the boot time. In general, if a user would notice the difference, use the relaxed version and add a comment to explain how you proved it's correct, otherwise

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 09:17:00 Timur Tabi wrote: > Arnd Bergmann wrote: > > We generally want to use readl/writel rather than the relaxed versions, > > unless it is in performance-critical code. > > What about if we have 20+ writes in a row, for example, when > initializing a part? I've

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Arnd Bergmann wrote: We generally want to use readl/writel rather than the relaxed versions, unless it is in performance-critical code. What about if we have 20+ writes in a row, for example, when initializing a part? I've seen code like this: writel_relaxed(...);

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 09:30:16 Christopher Covington wrote: > > On 12/14/2015 08:39 PM, Florian Fainelli wrote: > > On 14/12/15 16:19, Gilad Avidov wrote: > > >> +static void emac_mac_irq_enable(struct emac_adapter *adpt) > >> +{ > >> +int i; > >> + > >> +for (i = 0; i <

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Hi Florian, Thanks for taking the time to review this code. We'll probably take additional time to review and implement most of your suggestions but I was confused by your two comments below. On 12/14/2015 08:39 PM, Florian Fainelli wrote: > On 14/12/15 16:19, Gilad Avidov wrote: >> +static

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Arnd Bergmann wrote: If that's in the probe() called from it function, just use writel() everywhere, a few extra microseconds won't kill the boot time. In general, if a user would notice the difference, use the relaxed version and add a comment to explain how you proved it's correct, otherwise

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 15:09:23 Timur Tabi wrote: > Arnd Bergmann wrote: > > If that's in the probe() called from it function, just use writel() > > everywhere, > > a few extra microseconds won't kill the boot time. In general, if a user > > would > > notice the difference, use the relaxed

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Mon, 14 Dec 2015 17:39:09 -0800 Florian Fainelli wrote: > On 14/12/15 16:19, Gilad Avidov wrote: > > [snip] > > > + "sgmii_irq"; > > + qcom,emac-gpio-mdc = < 123 0>; > > + qcom,emac-gpio-mdio = < 124 0>; > > +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Hi Florian, Thanks for taking the time to review this code. We'll probably take additional time to review and implement most of your suggestions but I was confused by your two comments below. On 12/14/2015 08:39 PM, Florian Fainelli wrote: > On 14/12/15 16:19, Gilad Avidov wrote: >> +static

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 09:30:16 Christopher Covington wrote: > > On 12/14/2015 08:39 PM, Florian Fainelli wrote: > > On 14/12/15 16:19, Gilad Avidov wrote: > > >> +static void emac_mac_irq_enable(struct emac_adapter *adpt) > >> +{ > >> +int i; > >> + > >> +for (i = 0; i <

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

From: Timur Tabi Date: Tue, 15 Dec 2015 18:15:50 -0600 > You forgot to add "[v2]" to the subject line of this email. I think you did something much worse. You quoted the entire huge patch which is entirely inappropriate given the feedback you were trying to give. Thanks.

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

David Miller wrote: I think you did something much worse. You quoted the entire huge patch which is entirely inappropriate given the feedback you were trying to give. Sorry about that. I usually do trim it, but I got tired and forgot before I hit send. -- Sent by an employee of the

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Arnd Bergmann wrote: We generally want to use readl/writel rather than the relaxed versions, unless it is in performance-critical code. What about if we have 20+ writes in a row, for example, when initializing a part? I've seen code like this: writel_relaxed(...);

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tuesday 15 December 2015 09:17:00 Timur Tabi wrote: > Arnd Bergmann wrote: > > We generally want to use readl/writel rather than the relaxed versions, > > unless it is in performance-critical code. > > What about if we have 20+ writes in a row, for example, when > initializing a part? I've

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On 14/12/15 16:19, Gilad Avidov wrote: [snip] > + "sgmii_irq"; > + qcom,emac-gpio-mdc = < 123 0>; > + qcom,emac-gpio-mdio = < 124 0>; > + qcom,emac-tstamp-en; > + qcom,emac-ptp-frac-ns-adj = <12500 1>; > +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On 14/12/15 16:19, Gilad Avidov wrote: [snip] > + "sgmii_irq"; > + qcom,emac-gpio-mdc = < 123 0>; > + qcom,emac-gpio-mdio = < 124 0>; > + qcom,emac-tstamp-en; > + qcom,emac-ptp-frac-ns-adj = <12500 1>; > +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Gilad Avidov wrote: pointer math on void* ? what is the size of void ? I'm talking about adding and subtracting pointer values, so u32 pkt_len =((void *)ip_hdr(skb) - skb->data) -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Thank you Timur for the good review. On Wed, 9 Dec 2015 14:09:27 -0600 Timur Tabi wrote: > So first of all, thanks for posting this. I know it's missing a bunch > of stuff that's necessary for Qualcomm's Server chip, but it's a > start. > > Unfortunately, 6,000 lines is a lot to review at

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

From: Fabio Estevam Date: Wed, 9 Dec 2015 18:37:35 -0200 > On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi wrote: > >>> +/* set MAC address */ >>> +void emac_mac_addr_clear(struct emac_adapter *adpt, u8 *addr) >>> +{ >>> + u32 sta; >>> + >>> + /* for example: 00-A0-C6-11-22-33 >>> +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi wrote: >> +/* set MAC address */ >> +void emac_mac_addr_clear(struct emac_adapter *adpt, u8 *addr) >> +{ >> + u32 sta; >> + >> + /* for example: 00-A0-C6-11-22-33 >> +* 0<-->C6112233, 1<-->00A0. >> +*/ > > /* > * Multi-line

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

So first of all, thanks for posting this. I know it's missing a bunch of stuff that's necessary for Qualcomm's Server chip, but it's a start. Unfortunately, 6,000 lines is a lot to review at once. Any chance you can break up the next version into smaller patches? On Mon, Dec 7, 2015 at 4:58

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

So first of all, thanks for posting this. I know it's missing a bunch of stuff that's necessary for Qualcomm's Server chip, but it's a start. Unfortunately, 6,000 lines is a lot to review at once. Any chance you can break up the next version into smaller patches? On Mon, Dec 7, 2015 at 4:58

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

From: Fabio Estevam Date: Wed, 9 Dec 2015 18:37:35 -0200 > On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi wrote: > >>> +/* set MAC address */ >>> +void emac_mac_addr_clear(struct emac_adapter *adpt, u8 *addr) >>> +{ >>> + u32 sta; >>> + >>> +

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Wed, Dec 9, 2015 at 6:09 PM, Timur Tabi wrote: >> +/* set MAC address */ >> +void emac_mac_addr_clear(struct emac_adapter *adpt, u8 *addr) >> +{ >> + u32 sta; >> + >> + /* for example: 00-A0-C6-11-22-33 >> +* 0<-->C6112233, 1<-->00A0. >> +*/ >

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Thank you Timur for the good review. On Wed, 9 Dec 2015 14:09:27 -0600 Timur Tabi wrote: > So first of all, thanks for posting this. I know it's missing a bunch > of stuff that's necessary for Qualcomm's Server chip, but it's a > start. > > Unfortunately, 6,000 lines is

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Gilad Avidov wrote: pointer math on void* ? what is the size of void ? I'm talking about adding and subtracting pointer values, so u32 pkt_len =((void *)ip_hdr(skb) - skb->data) -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tue, 8 Dec 2015 00:33:04 +0100 Felix Fietkau wrote: > On 2015-12-07 23:58, Gilad Avidov wrote: > > +/* RRD (Receive Return Descriptor) */ > > +union emac_rrd { > > + struct { > > + /* 32bit word 0 */ > > + u32 xsum:16; > > + u32 nor:4; /* number of RFD

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Hi Gilad, [auto build test ERROR on net/master] [also build test ERROR on v4.4-rc4 next-20151207] url: https://github.com/0day-ci/linux/commits/Gilad-Avidov/net-emac-emac-gigabit-ethernet-controller-driver/20151208-070026 config: openrisc-allyesconfig (attached as .config) reproduce:

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On 2015-12-07 23:58, Gilad Avidov wrote: > +/* RRD (Receive Return Descriptor) */ > +union emac_rrd { > + struct { > + /* 32bit word 0 */ > + u32 xsum:16; > + u32 nor:4; /* number of RFD */ > + u32 si:12; /* start index of rfd-ring

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On 2015-12-07 23:58, Gilad Avidov wrote: > +/* RRD (Receive Return Descriptor) */ > +union emac_rrd { > + struct { > + /* 32bit word 0 */ > + u32 xsum:16; > + u32 nor:4; /* number of RFD */ > + u32 si:12; /* start index of rfd-ring

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

Hi Gilad, [auto build test ERROR on net/master] [also build test ERROR on v4.4-rc4 next-20151207] url: https://github.com/0day-ci/linux/commits/Gilad-Avidov/net-emac-emac-gigabit-ethernet-controller-driver/20151208-070026 config: openrisc-allyesconfig (attached as .config) reproduce:

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

On Tue, 8 Dec 2015 00:33:04 +0100 Felix Fietkau wrote: > On 2015-12-07 23:58, Gilad Avidov wrote: > > +/* RRD (Receive Return Descriptor) */ > > +union emac_rrd { > > + struct { > > + /* 32bit word 0 */ > > + u32 xsum:16; > > + u32 nor:4;