Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Byungho An <bh74.an@xxxxxxxxxxx> :
> From: Siva Reddy <siva.kallam@xxxxxxxxxxx>
> 
> This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
> - sxgbe core initialization
> - Tx and Rx support
> - MDIO support
> - ISRs for Tx and Rx
> - ifconfig support to driver

You'll find a partial review below.

[...]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe_common.h
> new file mode 100644
> index 0000000..3f16220
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe_common.h
[...]
> +enum dma_irq_status {
> +	tx_hard_error = BIT(0),
> +	tx_bump_tc = BIT(1),
> +	handle_tx = BIT(2),
> +	rx_hard_error = BIT(3),
> +	rx_bump_tc = BIT(4),
> +	handle_rx = BIT(5),

Please use tabs before "=" to line things up.

[...]
> +struct sxgbe_hwtimestamp {
> +	void (*config_hw_tstamping)(void __iomem *ioaddr, u32 data);
> +	void (*config_sub_second_increment)(void __iomem *ioaddr);
> +	int (*init_systime)(void __iomem *ioaddr, u32 sec, u32 nsec);
> +	int (*config_addend)(void __iomem *ioaddr, u32 addend);
> +	int (*adjust_systime)(void __iomem *ioaddr, u32 sec, u32 nsec,
> +			      int add_sub);
> +	u64 (*get_systime)(void __iomem *ioaddr);
> +};

None of these method is ever used.

Even annotated with __iomem, I'd rather keep the void * to a minimum and
push the device driver pointer through the call chain. Your call.

[...]
> +struct sxgbe_core_ops {
> +	/* MAC core initialization */
> +	void (*core_init)(void __iomem *ioaddr);
> +	/* Dump MAC registers */
> +	void (*dump_regs)(void __iomem *ioaddr);
> +	/* Handle extra events on specific interrupts hw dependent */
> +	int (*host_irq_status)(void __iomem *ioaddr,
> +			       struct sxgbe_extra_stats *x);
> +	/* Set power management mode (e.g. magic frame) */
> +	void (*pmt)(void __iomem *ioaddr, unsigned long mode);
> +	/* Set/Get Unicast MAC addresses */
> +	void (*set_umac_addr)(void __iomem *ioaddr, unsigned char *addr,
> +			      unsigned int reg_n);
> +	void (*get_umac_addr)(void __iomem *ioaddr, unsigned char *addr,
> +			      unsigned int reg_n);
> +	void (*enable_rx)(void __iomem *ioaddr, bool enable);
> +	void (*enable_tx)(void __iomem *ioaddr, bool enable);
> +
> +	/* controller version specific operations */
> +	int (*get_controller_version)(void __iomem *ioaddr);
> +
> +	/* If supported then get the optional core features */
> +	unsigned int (*get_hw_feature)(void __iomem *ioaddr,
> +				       unsigned char feature_index);
> +	/* adjust SXGBE speed */
> +	void (*set_speed)(void __iomem *ioaddr, unsigned char speed);
> +};

This indirection level is never used.

> +
> +const struct sxgbe_core_ops *sxgbe_get_core_ops(void);
> +
> +struct sxgbe_ops {
> +	const struct sxgbe_core_ops *mac;
> +	const struct sxgbe_desc_ops *desc;
> +	const struct sxgbe_dma_ops *dma;
> +	const struct sxgbe_mtl_ops *mtl;

Will these indirection levels ever be used ?

> +	const struct sxgbe_hwtimestamp *ptp;
> +	struct mii_regs mii;	/* MII register Addresses */
> +	struct mac_link link;
> +	unsigned int ctrl_uid;
> +	unsigned int ctrl_id;
> +};
> +
> +/* SXGBE private data structures */
> +struct sxgbe_tx_queue {
> +	u8 queue_no;
> +	unsigned int irq_no;
> +	struct sxgbe_priv_data *priv_ptr;
> +	struct sxgbe_tx_norm_desc *dma_tx;

You may lay things a bit differently.

[...]
> +/* SXGBE HW capabilities */
> +struct sxgbe_hw_features {
> +	/****** CAP [0] *******/
> +	unsigned int gmii_1000mbps;

This field is never read.

> +	unsigned int vlan_hfilter;

This field is never read.

> +	unsigned int sma_mdio;

This field is never read.

> +	unsigned int pmt_remote_wake_up;

This field *is* read.

> +	unsigned int pmt_magic_frame;

So is this one.

> +	unsigned int rmon;

But this one isn't :o/

> +	unsigned int arp_offload;

Sic.

The storage is a bit expensive. You may pack some boolean into a single
unsigned int.

[...]
> +struct sxgbe_priv_data {
> +	/* DMA descriptos */
> +	struct sxgbe_tx_queue *txq[SXGBE_TX_QUEUES];
> +	struct sxgbe_rx_queue *rxq[SXGBE_RX_QUEUES];
> +	u8 cur_rx_qnum;
> +
> +	unsigned int dma_tx_size;
> +	unsigned int dma_rx_size;
> +	unsigned int dma_buf_sz;
> +	u32 rx_riwt;
> +
> +	struct napi_struct napi;
> +
> +	void __iomem *ioaddr;
> +	struct net_device *dev;
> +	struct device *device;
> +	struct sxgbe_ops *hw;/* sxgbe specific ops */
> +	int no_csum_insertion;
> +	spinlock_t lock;

There is no spin_lock_init for this field.

OTOH it isn't really used at all.

[...]
> +	int oldlink;
> +	int speed;
> +	int oldduplex;
> +	unsigned int flow_ctrl;
> +	unsigned int pause;

You may add some extra inner struct when fields are related.

[...]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.c b/drivers/net/ethernet/samsung/sxgbe_desc.c
> new file mode 100644
> index 0000000..9a93553
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe_desc.c
[...]
> +static u64 sxgbe_get_rx_timestamp(struct sxgbe_rx_ctxt_desc *p)
> +{
> +	u64 ns;
> +	ns = p->tstamp_lo;

Please insert an empty line between the declaration and the body of the
function.

[...]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.h b/drivers/net/ethernet/samsung/sxgbe_desc.h
> new file mode 100644
> index 0000000..761b521
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe_desc.h
[...]
> +struct sxgbe_tx_norm_desc {
> +	u64 tdes01; /* buf1 address */
> +	union {
> +		/* TX Read-Format Desc 2,3 */
> +		struct {
> +			/* TDES2 */
> +			u32 buf1_size:14;
> +			u32 vlan_tag_ctl:2;
> +			u32 buf2_size:14;
> +			u32 timestmp_enable:1;
> +			u32 int_on_com:1;

Is there a device endianness control bit to make it safe ?

It's quite common to use __{le / be}32 and the relevant cpu_to_leXY
helpers.

[...]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_dma.c b/drivers/net/ethernet/samsung/sxgbe_dma.c
> new file mode 100644
> index 0000000..9ee4a3c
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe_dma.c
[...]
> +static int sxgbe_dma_init(void __iomem *ioaddr, int fix_burst,
> +			  int burst_map, int adv_addr_mode)
> +{
> +	int retry_count = 10;
> +	u32 reg_val;
> +
> +	/* reset the DMA */
> +	writel(SXGBE_DMA_SOFT_RESET, ioaddr + SXGBE_DMA_MODE_REG);
> +	while (retry_count--) {
> +		if (!(readl(ioaddr + SXGBE_DMA_MODE_REG) &
> +		      SXGBE_DMA_SOFT_RESET))

		if (!(readl(ioaddr + SXGBE_DMA_MODE_REG) &
			    SXGBE_DMA_SOFT_RESET))

Btw some ad-hoc helpers may shorten the code, for instance:

		if (!(sx_r32(sp, SXGBE_DMA_MODE_REG) & SXGBE_DMA_SOFT_RESET))

[...]
> +static void sxgbe_dma_channel_init(void __iomem *ioaddr, int cha_num,
> +				   int fix_burst, int pbl, dma_addr_t dma_tx,
> +				   dma_addr_t dma_rx, int t_rsize, int r_rsize)
> +{
> +	u32 reg_val;
> +	dma_addr_t dma_addr;
> +
> +	reg_val = readl(ioaddr + SXGBE_DMA_CHA_CTL_REG(cha_num));
> +	/* set the pbl */
> +	if (fix_burst) {
> +		reg_val |= SXGBE_DMA_PBL_X8MODE;
> +		writel(reg_val, ioaddr + SXGBE_DMA_CHA_CTL_REG(cha_num));
> +		/* program the TX pbl */
> +		reg_val = readl(ioaddr + SXGBE_DMA_CHA_TXCTL_REG(cha_num));
> +		reg_val |= (pbl << SXGBE_DMA_TXPBL_LSHIFT);
> +		writel(reg_val, ioaddr + SXGBE_DMA_CHA_TXCTL_REG(cha_num));
> +		/* program the RX pbl */
> +		reg_val = readl(ioaddr + SXGBE_DMA_CHA_RXCTL_REG(cha_num));
> +		reg_val |= (pbl << SXGBE_DMA_RXPBL_LSHIFT);
> +		writel(reg_val, ioaddr + SXGBE_DMA_CHA_RXCTL_REG(cha_num));
> +	}
> +
> +	/* program desc registers */
> +	writel((dma_tx >> 32),

Excess parenthesis.

> +	       ioaddr + SXGBE_DMA_CHA_TXDESC_HADD_REG(cha_num));
> +	writel((dma_tx & 0xFFFFFFFF),
> +	       ioaddr + SXGBE_DMA_CHA_TXDESC_LADD_REG(cha_num));
> +
> +	writel((dma_rx >> 32),
> +	       ioaddr + SXGBE_DMA_CHA_RXDESC_HADD_REG(cha_num));
> +	writel((dma_rx & 0xFFFFFFFF),
> +	       ioaddr + SXGBE_DMA_CHA_RXDESC_LADD_REG(cha_num));
> +
> +	/* program tail pointers */
> +	/* assumption: upper 32 bits are constant and
> +	 * same as TX/RX desc list
> +	 */
> +	dma_addr = dma_tx + ((t_rsize-1) * SXGBE_DESC_SIZE_BYTES);

	dma_addr = dma_tx + ((t_rsize - 1) * SXGBE_DESC_SIZE_BYTES);

[...]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe_main.c
> new file mode 100644
> index 0000000..7b5a6bd
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe_main.c
[...]
> +struct sxgbe_priv_data *sxgbe_dvr_probe(struct device *device,
> +					struct sxgbe_plat_data *plat_dat,
> +					void __iomem *addr)
> +{
[...]
> +	/* Rx Watchdog is available, enable depend on platform data */
> +	if (!priv->plat->riwt_off) {
> +		priv->use_riwt = 1;
> +		pr_info("Enable RX Mitigation via HW Watchdog Timer\n");
> +	}
> +
> +	netif_napi_add(ndev, &priv->napi, sxgbe_poll, 64);

There's no balancing netif_napi_del in sxgbe_dvr_remove.

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




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux