RE: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Stephen Hemminger wrote: > On Sun, 16 Mar 2014 20:53:25 -0700 > Byungho An wrote: > > > > > > > They are used but they always point to the same set of methods. > > > Those methods could thus be directly called. > > Yes, those methods can be called directly. > > But I think it is acceptable for manageability and extension for future. > > That argument is only valid if: > 1. you have hardware that will use it but it is not ready. > 2. they will get used in next release (in < 6 mo) I agree, it is expected that they will be used in 3 ~ 4 mo > > Ths set of indirection has negative cost: it impedes readability, has to be > maintained, and hurts performance. > > One of the principles of agile programming is NOT to build infrastructure until > it is needed. Otherwise you are likely to build it wrong. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
On Sun, 16 Mar 2014 20:53:25 -0700 Byungho An wrote: > > > > They are used but they always point to the same set of methods. > > Those methods could thus be directly called. > Yes, those methods can be called directly. > But I think it is acceptable for manageability and extension for future. That argument is only valid if: 1. you have hardware that will use it but it is not ready. 2. they will get used in next release (in < 6 mo) Ths set of indirection has negative cost: it impedes readability, has to be maintained, and hurts performance. One of the principles of agile programming is NOT to build infrastructure until it is needed. Otherwise you are likely to build it wrong. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Francois Romieu :
> [...]
> > > > +struct sxgbe_core_ops {
> > > > + /* MAC core initialization */
> > > > + void (*core_init)(void __iomem *ioaddr);
> [...]
> > > > + /* adjust SXGBE speed */
> > > > + void (*set_speed)(void __iomem *ioaddr, unsigned char
speed); };
> > >
> > > This indirection level is never used.
> > Those are used, can you give more detail?
>
> They are used but they always point to the same set of methods.
> Those methods could thus be directly called.
Yes, those methods can be called directly.
But I think it is acceptable for manageability and extension for future.
>
> [...]
> > > > +/* 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.
> > can you give more detail?
>
> Bigger fields first, u8 at the end. It will save padding in the struct.
OK.
>
> --
> Ueimor
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Andrew.an :
[...]
> > > +struct sxgbe_core_ops {
> > > + /* MAC core initialization */
> > > + void (*core_init)(void __iomem *ioaddr);
[...]
> > > + /* adjust SXGBE speed */
> > > + void (*set_speed)(void __iomem *ioaddr, unsigned char speed);
> > > +};
> >
> > This indirection level is never used.
> Those are used, can you give more detail?
They are used but they always point to the same set of methods.
Those methods could thus be directly called.
[...]
> > > +/* 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.
> can you give more detail?
Bigger fields first, u8 at the end. It will save padding in the struct.
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Francois Romieu
> Byungho An :
> > From: Siva Reddy
> >
> > 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 000..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.
OK.
>
> [...]
> > +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.
OK. Those methods will be posted later.
>
> 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.
I think either will be fine.
>
> [...]
> > +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.
Those are used, can you give more detail?
>
> > +
> > +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 ?
Those are used, can you give more detail?
>
> > + 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.
can you give more detail?
>
> [...]
> > +/* SXGBE HW capabilities */
> > +struct sxgbe_hw_features {
> > + /** CAP [0] ***/
> > + unsigned int gmii_1000mbps;
>
> This field is never read.
It will be used later and will be removed in the next post.
>
> > + unsigned int vlan_hfilter;
>
> This field is never read.
Same above.
>
> > + unsigned int sma_mdio;
>
> This field is never read.
Same above.
>
> > + 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.
It can be packed but not for all.
>
> [...]
> > +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;
> > +
> > + vo
Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
Byungho An :
> From: Siva Reddy
>
> 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 000..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/
Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
On Thu, 2014-03-13 at 05:53 -0700, Joe Perches wrote:
> Maybe this was supposed to be something like
>
> ns = p->tstamp_lo
> ns |= ((u64)tstamp_hi) << 32;
>
> If not, maybe it warrants a comment around
> here or on the descriptor definition
[]
> > +struct sxgbe_rx_ctxt_desc {
> > + u32 tstamp_lo:32;
> > + u32 tstamp_hi:32;
or maybe these would be better renamed like:
u32 tstamp_nsecs;
u32 tstamp_secs;
btw2: I still think using
u32 foo:32;
is asking for gcc to generate bad code.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver
On Thu, 2014-03-13 at 15:55 +0900, Byungho An wrote:
> 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
[]
> diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.c
> b/drivers/net/ethernet/samsung/sxgbe_desc.c
[]
> +/* Set Time stamp */
> +static void sxgbe_tx_ctxt_desc_set_tstamp(struct sxgbe_tx_ctxt_desc *p,
> + u8 ostc_enable, u64 tstamp)
> +{
> + if (ostc_enable) {
> + p->ostc = ostc_enable;
> + p->tstamp_lo = (u32) tstamp;
> + p->tstamp_hi = (u32) (tstamp>>32);
> + }
> +}
[]
> +static u64 sxgbe_get_rx_timestamp(struct sxgbe_rx_ctxt_desc *p)
> +{
> + u64 ns;
> + ns = p->tstamp_lo;
> + ns += p->tstamp_hi * 10ULL;
> +
> + return ns;
> +}
This looks odd.
rx and tx timestamps are different clocks?
rx tstamp_lo resets every second but tx_stamp
is free-running?
Maybe this was supposed to be something like
ns = p->tstamp_lo
ns |= ((u64)tstamp_hi) << 32;
If not, maybe it warrants a comment around
here or on the descriptor definition
> diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.h
> b/drivers/net/ethernet/samsung/sxgbe_desc.h
[]
> +/* Context descriptor structure */
> +struct sxgbe_tx_ctxt_desc {
> + u32 tstamp_lo:32;
> + u32 tstamp_hi:32;
[]
> +struct sxgbe_rx_ctxt_desc {
> + u32 tstamp_lo:32;
> + u32 tstamp_hi:32;
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
