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

[Rob Herring]

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>;
>> > +   qcom,emac-gpio-mdio = < 124 0>;
>> > +   qcom,emac-tstamp-en;
>> > +   qcom,emac-ptp-frac-ns-adj = <12500 1>;
>> > +   phy-addr = <0>;
>>
>> Please use the standard Ethernet PHY and MDIO device tree bindings to
>> describe your MAC to PHY connection here, that includes using a
>> phy-connection-type property to describe the (x)MII lanes.
>>
>
>
> Hi Florian,
>
> Thank you for the review.
>
> Unfortunately this Ethernet controller's PHY is non standard and fits
> poorly into the standard MDIO framework layer. Rather than read/writs
> over MDIO only, this hw have some of the PHY registers internal and
> accessed by memory mapped IO, while others are accessed over the MDIO.
> Some standard functions requires using both. Additionally a number
> of different functions are controlled from different fields of the
> same register.

Even so, the bindings should follow the standard binding for MDIO bus
whether you can use the common kernel infrastructure or not.

Having internal phy connected to external phy is pretty common for
10G. Not sure if that is what you mean here or not.

Rob
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Timur Tabi]

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 stay with the default accessors.


What about adding a wmb() after the last writel()?  This driver does 
that a lot.  Is that something we want to discourage?  I can understand 
how we would want to make sure that the last write is posted before the 
function exits.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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 version and add a comment to explain
> > how you proved it's correct, otherwise stay with the default accessors.
> 
> What about adding a wmb() after the last writel()?  This driver does 
> that a lot.  Is that something we want to discourage?  I can understand 
> how we would want to make sure that the last write is posted before the 
> function exits.

Please explain in a comment specifically which race you are closing by
ensuring that the write gets posted. What does it race against?

As I said earlier, guaranteeing that a write gets posted does not mean
it has arrived at the device, we only get that behavior after a subsequent
read from the same device, but you don't need a wmb() between the
write and the read to guarantee this.

If you have an odd bus that does not follow those rules, it may in fact be
best to have a separate set of I/O accessors and not use readl/writel at all.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Gilad Avidov]

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;
> > +   qcom,emac-ptp-frac-ns-adj = <12500 1>;
> > +   phy-addr = <0>;
> 
> Please use the standard Ethernet PHY and MDIO device tree bindings to
> describe your MAC to PHY connection here, that includes using a
> phy-connection-type property to describe the (x)MII lanes.
> 


Hi Florian,

Thank you for the review.

Unfortunately this Ethernet controller's PHY is non standard and fits
poorly into the standard MDIO framework layer. Rather than read/writs
over MDIO only, this hw have some of the PHY registers internal and
accessed by memory mapped IO, while others are accessed over the MDIO.
Some standard functions requires using both. Additionally a number
of different functions are controlled from different fields of the
same register.

> [snip]
> 
> > +/* EMAC_MAC_CTRL */
> > +#define SINGLE_PAUSE_MODE
> > 0x1000 +#define
> > DEBUG_MODE   0x800
> > +#define BROAD_EN
> > 0x400 +#define
> > MULTI_ALL0x200
> > +#define RX_CHKSUM_EN
> > 0x100 +#define
> > HUGE  0x80
> > +#define SPEED_BMSK
> > 0x30 +#define
> > SPEED_SHFT  20
> > +#define SIMR
> > 0x8 +#define
> > TPAUSE 0x1
> > +#define PROM_MODE
> > 0x8000 +#define
> > VLAN_STRIP  0x4000
> > +#define PRLEN_BMSK
> > 0x3c00 +#define
> > PRLEN_SHFT  10
> > +#define HUGEN
> > 0x200 +#define
> > FLCHK0x100
> > +#define PCRCE
> > 0x80 +#define
> > CRCE  0x40
> > +#define FULLD
> > 0x20 +#define
> > MAC_LP_EN 0x10
> > +#define RXFC
> > 0x8 +#define
> > TXFC   0x4
> > +#define RXEN
> > 0x2 +#define
> > TXEN   0x1
> 
> BIT(x)? which would avoid making this reverse christmas tree, I know
> this is the time of year though.
> 

:)
Agree.

> [snip]
> 
> > +/* DMA address */
> > +#define DMA_ADDR_HI_MASK
> > 0xULL +#define
> > DMA_ADDR_LO_MASK 0xULL +
> > +#define
> > EMAC_DMA_ADDR_HI(_addr)  \
> > +   ((u32)(((u64)(_addr) & DMA_ADDR_HI_MASK) >> 32))
> > +#define
> > EMAC_DMA_ADDR_LO(_addr)  \
> > +   ((u32)((u64)(_addr) & DMA_ADDR_LO_MASK))
> 
> The kernel provides helpers for that: upper_32bits and lower_32bits().
> 

lower_32_bits(n) and upper_32_bits(n), thanks. I'll use them here.

> [snip]
> 
> > +struct emac_skb_cb {
> > +   u32   tpd_idx;
> > +   unsigned long jiffies;
> > +};
> > +
> > +struct emac_tx_ts_cb {
> > +   u32 sec;
> > +   u32 ns;
> > +};
> > +
> > +#define EMAC_SKB_CB(skb)   ((struct emac_skb_cb *)(skb)->cb)
> > +#define EMAC_TX_TS_CB(skb) ((struct emac_tx_ts_cb
> > *)(skb)->cb)
> 
> Should not these two have different offsets within skb->cb in case
> they both end-up being added to the same SKB?
> 

Good point. I'll look into this.


> [snip]
> 
> > +
> > +   /* enable RX/TX Flow Control */
> > +   switch (phy->cur_fc_mode) {
> > +   case EMAC_FC_FULL:
> > +   mac |= (TXFC | RXFC);
> > +   break;
> > +   case EMAC_FC_RX_PAUSE:
> > +   mac |= RXFC;
> > +   break;
> > +   case EMAC_FC_TX_PAUSE:
> > +   mac |= TXFC;
> > +   break;
> > +   default:
> > +   break;
> > +   }
> > +
> > +   /* setup link speed */
> > +   mac &= ~SPEED_BMSK;
> > +   switch (phy->link_speed) {
> > +   case EMAC_LINK_SPEED_1GB_FULL:
> > +   mac |= ((emac_mac_speed_1000 << SPEED_SHFT) &
> > SPEED_BMSK);
> > +   csr1 |= FREQ_MODE;
> > +   break;
> > +   default:
> > +   mac |= ((emac_mac_speed_10_100 << SPEED_SHFT) &
> > SPEED_BMSK);
> > +   csr1 &= ~FREQ_MODE;
> > +   break;
> > +   }
> > +
> > +   switch (phy->link_speed) {
> > +   case EMAC_LINK_SPEED_1GB_FULL:
> > +   case EMAC_LINK_SPEED_100_FULL:
> > +   case EMAC_LINK_SPEED_10_FULL:
> > +   mac |= FULLD;
> > +   break;
> > +   default:
> > +   mac &= ~FULLD;
> > +   }
> 
> You should use the PHY library and implement an adjust_link callback
> which does exactly that above.
> [snip]
> 
> > +static bool emac_tx_has_enough_descs(struct emac_tx_queue *tx_q,
> > +const struct sk_buff *skb)
> > +{
> 

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

[Christopher Covington]

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 void emac_mac_irq_enable(struct emac_adapter *adpt)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < EMAC_NUM_CORE_IRQ; i++) {
>> +struct emac_irq *irq = >irq[i];
>> +const struct emac_irq_config*irq_cfg = _irq_cfg_tbl[i];
>> +
>> +writel_relaxed(~DIS_INT, adpt->base + irq_cfg->status_reg);
>> +writel_relaxed(irq->mask, adpt->base + irq_cfg->mask_reg);
>> +}
>> +
>> +wmb(); /* ensure that irq and ptp setting are flushed to HW */
> 
> Would not using writel() make the appropriate thing here instead of
> using _relaxed which has no barrier?

It appears to me that the barrier in writel() comes before the access
[1]. The barrier in this code comes after the accesses. In addition to
the ordering, if you're suggesting all writel_relaxed be switched out,
that would seem to add 7 unnecessary barriers, which could adversely
affect performance.

1. http://lxr.free-electrons.com/source/arch/arm64/include/asm/io.h#L130

> [snip]
> 
>> +mta = readl_relaxed(adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
>> +mta |= (0x1 << bit);
>> +writel_relaxed(mta, adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
>> +wmb(); /* ensure that the mac address is flushed to HW */
> 
> This is getting too much here, just use the correct I/O accessor for
> your platform, period.

Based on your previous comment, I'm guessing you're suggesting using
readl() and writel() here instead of *_relaxed and an explicit wmb().
Again it's not clear to me why swapping the barrier-access ordering and
adding an additional barrier would result in more correct code.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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 < EMAC_NUM_CORE_IRQ; i++) {
> >> +struct emac_irq *irq = >irq[i];
> >> +const struct emac_irq_config*irq_cfg = 
> >> _irq_cfg_tbl[i];
> >> +
> >> +writel_relaxed(~DIS_INT, adpt->base + irq_cfg->status_reg);
> >> +writel_relaxed(irq->mask, adpt->base + irq_cfg->mask_reg);
> >> +}
> >> +
> >> +wmb(); /* ensure that irq and ptp setting are flushed to HW */
> > 
> > Would not using writel() make the appropriate thing here instead of
> > using _relaxed which has no barrier?
> 
> It appears to me that the barrier in writel() comes before the access
> [1]. The barrier in this code comes after the accesses. In addition to
> the ordering, if you're suggesting all writel_relaxed be switched out,
> that would seem to add 7 unnecessary barriers, which could adversely
> affect performance.
> 
> 1. http://lxr.free-electrons.com/source/arch/arm64/include/asm/io.h#L130

You are right, the writel does not flush the write out to hardware,
and generally that is not needed, in particular since most buses do
not actually wait for a write to complete when a barrier is issued.

I'm missing two explanations here:

a) How performance-critical is the emac_mac_irq_enable() function?
   Is this only called when configuring the device, or each time
   you call napi_complete()?

b) What other code relies on the write being flushed out first?
   Can you move the barrier to the other side? If emac_mac_irq_enable()
   is called a lot, you might be able to avoid that barrier altogether
   if you instead put it whereever you access the device that requires
   the interrupts to be enabled.

> >> +mta = readl_relaxed(adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
> >> +mta |= (0x1 << bit);
> >> +writel_relaxed(mta, adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
> >> +wmb(); /* ensure that the mac address is flushed to HW */
> > 
> > This is getting too much here, just use the correct I/O accessor for
> > your platform, period.
> 
> Based on your previous comment, I'm guessing you're suggesting using
> readl() and writel() here instead of *_relaxed and an explicit wmb().
> Again it's not clear to me why swapping the barrier-access ordering and
> adding an additional barrier would result in more correct code.

We generally want to use readl/writel rather than the relaxed versions,
unless it is in performance-critical code.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

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 from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Timur Tabi]

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 Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Timur Tabi]

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(...);
writel_relaxed(...);
writel_relaxed(...);
...
writel(...); // HW now inited, so enable

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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 seen code like this:
> 
> writel_relaxed(...);
> writel_relaxed(...);
> writel_relaxed(...);
> ...
> writel(...); // HW now inited, so enable

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 stay with the default accessors.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

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>;
> + phy-addr = <0>;

Please use the standard Ethernet PHY and MDIO device tree bindings to
describe your MAC to PHY connection here, that includes using a
phy-connection-type property to describe the (x)MII lanes.

[snip]

> +/* EMAC_MAC_CTRL */
> +#define SINGLE_PAUSE_MODE   0x1000
> +#define DEBUG_MODE   0x800
> +#define BROAD_EN 0x400
> +#define MULTI_ALL0x200
> +#define RX_CHKSUM_EN 0x100
> +#define HUGE  0x80
> +#define SPEED_BMSK0x30
> +#define SPEED_SHFT  20
> +#define SIMR   0x8
> +#define TPAUSE 0x1
> +#define PROM_MODE   0x8000
> +#define VLAN_STRIP  0x4000
> +#define PRLEN_BMSK  0x3c00
> +#define PRLEN_SHFT  10
> +#define HUGEN0x200
> +#define FLCHK0x100
> +#define PCRCE 0x80
> +#define CRCE  0x40
> +#define FULLD 0x20
> +#define MAC_LP_EN 0x10
> +#define RXFC   0x8
> +#define TXFC   0x4
> +#define RXEN   0x2
> +#define TXEN   0x1

BIT(x)? which would avoid making this reverse christmas tree, I know
this is the time of year though.

[snip]

> +/* DMA address */
> +#define DMA_ADDR_HI_MASK 0xULL
> +#define DMA_ADDR_LO_MASK 0xULL
> +
> +#define EMAC_DMA_ADDR_HI(_addr)  \
> + ((u32)(((u64)(_addr) & DMA_ADDR_HI_MASK) >> 32))
> +#define EMAC_DMA_ADDR_LO(_addr)  \
> + ((u32)((u64)(_addr) & DMA_ADDR_LO_MASK))

The kernel provides helpers for that: upper_32bits and lower_32bits().

[snip]

> +struct emac_skb_cb {
> + u32   tpd_idx;
> + unsigned long jiffies;
> +};
> +
> +struct emac_tx_ts_cb {
> + u32 sec;
> + u32 ns;
> +};
> +
> +#define EMAC_SKB_CB(skb) ((struct emac_skb_cb *)(skb)->cb)
> +#define EMAC_TX_TS_CB(skb)   ((struct emac_tx_ts_cb *)(skb)->cb)

Should not these two have different offsets within skb->cb in case they
both end-up being added to the same SKB?

[snip]

> +static void emac_mac_irq_enable(struct emac_adapter *adpt)
> +{
> + int i;
> +
> + for (i = 0; i < EMAC_NUM_CORE_IRQ; i++) {
> + struct emac_irq *irq = >irq[i];
> + const struct emac_irq_config*irq_cfg = _irq_cfg_tbl[i];
> +
> + writel_relaxed(~DIS_INT, adpt->base + irq_cfg->status_reg);
> + writel_relaxed(irq->mask, adpt->base + irq_cfg->mask_reg);
> + }
> +
> + wmb(); /* ensure that irq and ptp setting are flushed to HW */

Would not using writel() make the appropriate thing here instead of
using _relaxed which has no barrier?

[snip]

> + mta = readl_relaxed(adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
> + mta |= (0x1 << bit);
> + writel_relaxed(mta, adpt->base + EMAC_HASH_TAB_REG0 + (reg << 2));
> + wmb(); /* ensure that the mac address is flushed to HW */

This is getting too much here, just use the correct I/O accessor for
your platform, period.

[snip]

> +
> + /* enable RX/TX Flow Control */
> + switch (phy->cur_fc_mode) {
> + case EMAC_FC_FULL:
> + mac |= (TXFC | RXFC);
> + break;
> + case EMAC_FC_RX_PAUSE:
> + mac |= RXFC;
> + break;
> + case EMAC_FC_TX_PAUSE:
> + mac |= TXFC;
> + break;
> + default:
> + break;
> + }
> +
> + /* setup link speed */
> + mac &= ~SPEED_BMSK;
> + switch (phy->link_speed) {
> + case EMAC_LINK_SPEED_1GB_FULL:
> + mac |= ((emac_mac_speed_1000 << SPEED_SHFT) & SPEED_BMSK);
> + csr1 |= FREQ_MODE;
> + 

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

[Timur Tabi]

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 PM, Gilad Avidov  wrote:

> +   qcom,emac-gpio-mdc = < 123 0>;
> +   qcom,emac-gpio-mdio = < 124 0>;

Is there any chance you can remove all references to "MSM" throughout
the entire driver, since the EMAC exists on non-MSM parts?

> +   qcom,emac-tstamp-en;
> +   qcom,emac-ptp-frac-ns-adj = <12500 1>;
> +   phy-addr = <0>;
> +   };
> diff --git a/drivers/net/ethernet/qualcomm/Kconfig 
> b/drivers/net/ethernet/qualcomm/Kconfig
> index a76e380..ae9442d 100644
> --- a/drivers/net/ethernet/qualcomm/Kconfig
> +++ b/drivers/net/ethernet/qualcomm/Kconfig
> @@ -24,4 +24,11 @@ config QCA7000
>   To compile this driver as a module, choose M here. The module
>   will be called qcaspi.
>
> +config QCOM_EMAC
> +   tristate "MSM EMAC Gigabit Ethernet support"
> +   default n

"default n" is redundant

> +   select CRC32
> +   ---help---
> + This driver supports the Qualcomm EMAC Gigabit Ethernet controller.

I think should be longer, perhaps by adding some more info about the
controller itself?

> +
>  endif # NET_VENDOR_QUALCOMM
> diff --git a/drivers/net/ethernet/qualcomm/Makefile 
> b/drivers/net/ethernet/qualcomm/Makefile
> index 9da2d75..b14686e 100644
> --- a/drivers/net/ethernet/qualcomm/Makefile
> +++ b/drivers/net/ethernet/qualcomm/Makefile
> @@ -4,3 +4,5 @@
>
>  obj-$(CONFIG_QCA7000) += qcaspi.o
>  qcaspi-objs := qca_spi.o qca_framing.o qca_7k.o qca_debug.o
> +
> +obj-$(CONFIG_QCOM_EMAC) += emac/
> \ No newline at end of file

Please fix

> +/* RSS */
> +static void emac_mac_rss_config(struct emac_adapter *adpt)
> +{
> +   int key_len_by_u32 = sizeof(adpt->rss_key) / sizeof(u32);
> +   int idt_len_by_u32 = sizeof(adpt->rss_idt) / sizeof(u32);

Can you use ARRAY_SIZE here?

> +   u32 rxq0;
> +   int i;
> +
> +   /* Fill out hash function keys */
> +   for (i = 0; i < key_len_by_u32; i++) {
> +   u32 key, idx_base;
> +
> +   idx_base = (key_len_by_u32 - i) * 4;
> +   key = ((adpt->rss_key[idx_base - 1])   |
> +  (adpt->rss_key[idx_base - 2] << 8)  |
> +  (adpt->rss_key[idx_base - 3] << 16) |
> +  (adpt->rss_key[idx_base - 4] << 24));
> +   writel_relaxed(key, adpt->base + EMAC_RSS_KEY(i, u32));
> +   }
> +
> +   /* Fill out redirection table */
> +   for (i = 0; i < idt_len_by_u32; i++)
> +   writel_relaxed(adpt->rss_idt[i],
> +  adpt->base + EMAC_RSS_TBL(i, u32));
> +
> +   writel_relaxed(adpt->rss_base_cpu, adpt->base + EMAC_BASE_CPU_NUMBER);
> +
> +   rxq0 = readl_relaxed(adpt->base + EMAC_RXQ_CTRL_0);
> +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV4_EN)
> +   rxq0 |= RXQ0_RSS_HSTYP_IPV4_EN;
> +   else
> +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_EN;
> +
> +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP4_EN)
> +   rxq0 |= RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> +   else
> +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> +
> +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV6_EN)
> +   rxq0 |= RXQ0_RSS_HSTYP_IPV6_EN;
> +   else
> +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_EN;
> +
> +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP6_EN)
> +   rxq0 |= RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> +   else
> +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> +
> +   rxq0 |= ((adpt->rss_idt_size << IDT_TABLE_SIZE_SHFT) &
> +   IDT_TABLE_SIZE_BMSK);
> +   rxq0 |= RSS_HASH_EN;
> +
> +   wmb(); /* ensure all parameters are written before enabling RSS */
> +
> +   writel_relaxed(rxq0, adpt->base + EMAC_RXQ_CTRL_0);

Why not just use writel(), which already includes a wmb()

> +/* Power Management */
> +void emac_mac_pm(struct emac_adapter *adpt, u32 speed, bool wol_en, bool 
> rx_en)
> +{
> +   u32 dma_mas, mac;
> +
> +   dma_mas = readl_relaxed(adpt->base + EMAC_DMA_MAS_CTRL);
> +   dma_mas &= ~LPW_CLK_SEL;
> +   dma_mas |= LPW_STATE;
> +
> +   mac = readl_relaxed(adpt->base + EMAC_MAC_CTRL);
> +   mac &= ~(FULLD | RXEN | TXEN);
> +   mac = (mac & ~SPEED_BMSK) |
> + (((u32)emac_mac_speed_10_100 << SPEED_SHFT) & SPEED_BMSK);
> +
> +   if (wol_en) {
> +   if (rx_en)
> +   mac |= (RXEN | BROAD_EN);

You don't need the parentheses.

> +/* Config descriptor rings */
> +static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
> +{
> +   if (adpt->timestamp_en)
> +   emac_reg_update32(adpt->csr + EMAC_EMAC_WRAPPER_CSR1,
> +  

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

[Fabio Estevam]

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 comments
>  * look like this.
>  */

Except in drivers/net. The convention in drivers/net is to use
multi-line format as posted in this patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Miller]

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
>>> +* 0<-->C6112233, 1<-->00A0.
>>> +*/
>>
>> /*
>>  * Multi-line comments
>>  * look like this.
>>  */
> 
> Except in drivers/net. The convention in drivers/net is to use
> multi-line format as posted in this patch.

Correct.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Gilad Avidov]

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 once.  Any chance you
> can break up the next version into smaller patches?

Agree its a lot to review, however:
1) This driver is the what left after I removed all unnecessary
features, thus it is already minimal.
I have removed features such as support for: server, ACPI, ethtool,
ptp/1588, etc.
2) This size seems comparable to first patches of existing Ethernet
drivers.

> 
> On Mon, Dec 7, 2015 at 4:58 PM, Gilad Avidov 
> wrote:
> 
> > +   qcom,emac-gpio-mdc = < 123 0>;
> > +   qcom,emac-gpio-mdio = < 124 0>;
> 
> Is there any chance you can remove all references to "MSM" throughout
> the entire driver, since the EMAC exists on non-MSM parts?

msm appears only in this DT binding example. None in the code.
I will look into removing this instance too.

> 
> > +   qcom,emac-tstamp-en;
> > +   qcom,emac-ptp-frac-ns-adj = <12500 1>;
> > +   phy-addr = <0>;
> > +   };
> > diff --git a/drivers/net/ethernet/qualcomm/Kconfig
> > b/drivers/net/ethernet/qualcomm/Kconfig index a76e380..ae9442d
> > 100644 --- a/drivers/net/ethernet/qualcomm/Kconfig
> > +++ b/drivers/net/ethernet/qualcomm/Kconfig
> > @@ -24,4 +24,11 @@ config QCA7000
> >   To compile this driver as a module, choose M here. The
> > module will be called qcaspi.
> >
> > +config QCOM_EMAC
> > +   tristate "MSM EMAC Gigabit Ethernet support"
> > +   default n
> 
> "default n" is redundant
> 
> > +   select CRC32
> > +   ---help---
> > + This driver supports the Qualcomm EMAC Gigabit Ethernet
> > controller.
> 
> I think should be longer, perhaps by adding some more info about the
> controller itself?

ok.

> 
> > +
> >  endif # NET_VENDOR_QUALCOMM
> > diff --git a/drivers/net/ethernet/qualcomm/Makefile
> > b/drivers/net/ethernet/qualcomm/Makefile index 9da2d75..b14686e
> > 100644 --- a/drivers/net/ethernet/qualcomm/Makefile
> > +++ b/drivers/net/ethernet/qualcomm/Makefile
> > @@ -4,3 +4,5 @@
> >
> >  obj-$(CONFIG_QCA7000) += qcaspi.o
> >  qcaspi-objs := qca_spi.o qca_framing.o qca_7k.o qca_debug.o
> > +
> > +obj-$(CONFIG_QCOM_EMAC) += emac/
> > \ No newline at end of file
> 
> Please fix

ok.

> 
> > +/* RSS */
> > +static void emac_mac_rss_config(struct emac_adapter *adpt)
> > +{
> > +   int key_len_by_u32 = sizeof(adpt->rss_key) / sizeof(u32);
> > +   int idt_len_by_u32 = sizeof(adpt->rss_idt) / sizeof(u32);
> 
> Can you use ARRAY_SIZE here?

Agree.

> 
> > +   u32 rxq0;
> > +   int i;
> > +
> > +   /* Fill out hash function keys */
> > +   for (i = 0; i < key_len_by_u32; i++) {
> > +   u32 key, idx_base;
> > +
> > +   idx_base = (key_len_by_u32 - i) * 4;
> > +   key = ((adpt->rss_key[idx_base - 1])   |
> > +  (adpt->rss_key[idx_base - 2] << 8)  |
> > +  (adpt->rss_key[idx_base - 3] << 16) |
> > +  (adpt->rss_key[idx_base - 4] << 24));
> > +   writel_relaxed(key, adpt->base + EMAC_RSS_KEY(i,
> > u32));
> > +   }
> > +
> > +   /* Fill out redirection table */
> > +   for (i = 0; i < idt_len_by_u32; i++)
> > +   writel_relaxed(adpt->rss_idt[i],
> > +  adpt->base + EMAC_RSS_TBL(i, u32));
> > +
> > +   writel_relaxed(adpt->rss_base_cpu, adpt->base +
> > EMAC_BASE_CPU_NUMBER); +
> > +   rxq0 = readl_relaxed(adpt->base + EMAC_RXQ_CTRL_0);
> > +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV4_EN)
> > +   rxq0 |= RXQ0_RSS_HSTYP_IPV4_EN;
> > +   else
> > +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_EN;
> > +
> > +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP4_EN)
> > +   rxq0 |= RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> > +   else
> > +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV4_TCP_EN;
> > +
> > +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_IPV6_EN)
> > +   rxq0 |= RXQ0_RSS_HSTYP_IPV6_EN;
> > +   else
> > +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_EN;
> > +
> > +   if (adpt->rss_hstype & EMAC_RSS_HSTYP_TCP6_EN)
> > +   rxq0 |= RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> > +   else
> > +   rxq0 &= ~RXQ0_RSS_HSTYP_IPV6_TCP_EN;
> > +
> > +   rxq0 |= ((adpt->rss_idt_size << IDT_TABLE_SIZE_SHFT) &
> > +   IDT_TABLE_SIZE_BMSK);
> > +   rxq0 |= RSS_HASH_EN;
> > +
> > +   wmb(); /* ensure all parameters are written before enabling
> > RSS */ +
> > +   writel_relaxed(rxq0, adpt->base + EMAC_RXQ_CTRL_0);
> 
> Why not just use writel(), which already includes a wmb()
> 

ok.

> > +/* Power Management */
> > +void emac_mac_pm(struct emac_adapter *adpt, u32 

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

[Timur Tabi]

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 the
Code Aurora Forum, hosted by The Linux Foundation.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felix Fietkau]

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 */
> + /* 32bit word 1 */
> + u32  hash;
> + /* 32bit word 2 */
You should never use bitfields for hardware structs.
I think in general, kernel code should be made endian safe, even if you
only care about one particular endian type for your platform.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[kbuild test robot]

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:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=openrisc 

All errors (new ones prefixed by >>):

   drivers/net/ethernet/qualcomm/emac/emac-mac.c: In function 'emac_tso_csum':
>> drivers/net/ethernet/qualcomm/emac/emac-mac.c:2086:4: error: implicit 
>> declaration of function 'csum_ipv6_magic'

vim +/csum_ipv6_magic +2086 drivers/net/ethernet/qualcomm/emac/emac-mac.c

  2080  union emac_tpd extra_tpd;
  2081  
  2082  memset(tpd, 0, sizeof(*tpd));
  2083  memset(_tpd, 0, sizeof(extra_tpd));
  2084  
  2085  ipv6_hdr(skb)->payload_len = 0;
> 2086  tcp_hdr(skb)->check = ~csum_ipv6_magic(
  2087  _hdr(skb)->saddr,
  2088  _hdr(skb)->daddr,
  2089  0, IPPROTO_TCP, 0);

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

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

[Gilad Avidov]

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 */
> > +   u32  si:12;   /* start index of rfd-ring */
> > +   /* 32bit word 1 */
> > +   u32  hash;
> > +   /* 32bit word 2 */
> You should never use bitfields for hardware structs.
> I think in general, kernel code should be made endian safe, even if
> you only care about one particular endian type for your platform.
> 
> - Felix

Thank you Felix,
I will change the bit fields to bitwise operations and macros.

Gilad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html