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

[Florian Fainelli <f.fainelli@gmail.com>]

On 14/12/15 16:19, Gilad Avidov wrote:

[snip]

> +			"sgmii_irq";
> +		qcom,emac-gpio-mdc = <&msmgpio 123 0>;
> +		qcom,emac-gpio-mdio = <&msmgpio 124 0>;
> +		qcom,emac-tstamp-en;
> +		qcom,emac-ptp-frac-ns-adj = <125000000 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                                   0x10000000
> +#define DEBUG_MODE                                           0x8000000
> +#define BROAD_EN                                             0x4000000
> +#define MULTI_ALL                                            0x2000000
> +#define RX_CHKSUM_EN                                         0x1000000
> +#define HUGE                                                  0x800000
> +#define SPEED_BMSK                                            0x300000
> +#define SPEED_SHFT                                                  20
> +#define SIMR                                                   0x80000
> +#define TPAUSE                                                 0x10000
> +#define PROM_MODE                                               0x8000
> +#define VLAN_STRIP                                              0x4000
> +#define PRLEN_BMSK                                              0x3c00
> +#define PRLEN_SHFT                                                  10
> +#define HUGEN                                                    0x200
> +#define FLCHK                                                    0x100
> +#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                         0xffffffff00000000ULL
> +#define DMA_ADDR_LO_MASK                         0x00000000ffffffffULL
> +
> +#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 = &adpt->irq[i];
> +		const struct emac_irq_config	*irq_cfg = &emac_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;
> +		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)
> +{
> +	u32 num_required = 1;
> +	int i;
> +	u16 proto_hdr_len = 0;
> +
> +	if (skb_is_gso(skb)) {
> +		proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);

You cannot do this until you have looked at skb->protocol AFAIR.

[snip]

> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
> new file mode 100644
> index 0000000..45571a5
> --- /dev/null
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c

[snip]

This file implement a large amount of what the PHY library already does
for you if you simply provided a MDIO bus implementation instead, please
consider dropping 80% of this file content and using what is already
there to help you.

I stopped reading there because the driver is very large, I would really
start submitting it in smaller piece that make it more readable, and
dropping things that may not be necessary for now like RSS support,
Wake-on-LAN etc. etc.
-- 
Florian