On Sunday 06 July 2008, Grant Erickson wrote:
> Various instances of the EMAC core have varying: 1) number of address
> match slots, 2) width of the registers for handling address match slots,
> 3) number of registers for handling address match slots and 4) base
> offset for those registers.
Thanks Grant. Apart from Ben's comments I only have a few nitpicking comment.
Please see below.
<snip>
> diff --git a/drivers/net/ibm_newemac/core.c
> b/drivers/net/ibm_newemac/core.c index 5d2108c..931a061 100644
> --- a/drivers/net/ibm_newemac/core.c
> +++ b/drivers/net/ibm_newemac/core.c
> @@ -363,25 +363,32 @@ static int emac_reset(struct emac_instance *dev)
>
> static void emac_hash_mc(struct emac_instance *dev)
> {
> - struct emac_regs __iomem *p = dev->emacp;
> - u16 gaht[4] = { 0 };
> + const int regs = EMAC_XAHT_REGS(dev);
> + u32 *gaht_base = emac_gaht_base(dev);
> + u32 gaht_temp[regs];
> struct dev_mc_list *dmi;
> + int i;
>
> DBG(dev, "hash_mc %d" NL, dev->ndev->mc_count);
>
> + memset(gaht_temp, 0, sizeof (gaht_temp));
> +
> for (dmi = dev->ndev->mc_list; dmi; dmi = dmi->next) {
> - int bit;
> + int slot, reg, mask;
> DBG2(dev, "mc %02x:%02x:%02x:%02x:%02x:%02x" NL,
> dmi->dmi_addr[0], dmi->dmi_addr[1], dmi->dmi_addr[2],
> dmi->dmi_addr[3], dmi->dmi_addr[4], dmi->dmi_addr[5]);
>
> - bit = 63 - (ether_crc(ETH_ALEN, dmi->dmi_addr) >> 26);
> - gaht[bit >> 4] |= 0x8000 >> (bit & 0x0f);
> + slot = EMAC_XAHT_CRC_TO_SLOT(dev, ether_crc(ETH_ALEN,
> dmi->dmi_addr));
> + reg = EMAC_XAHT_SLOT_TO_REG(dev, slot);
> + mask = EMAC_XAHT_SLOT_TO_MASK(dev, slot);
> +
> + gaht_temp[reg] |= mask;
> + }
> +
> + for (i = 0; i < regs; i++) {
> + out_be32(gaht_base + i, gaht_temp[i]);
> }
No parentheses on single line statements.
> - out_be32(&p->gaht1, gaht[0]);
> - out_be32(&p->gaht2, gaht[1]);
> - out_be32(&p->gaht3, gaht[2]);
> - out_be32(&p->gaht4, gaht[3]);
> }
>
> static inline u32 emac_iff2rmr(struct net_device *ndev)
> @@ -398,7 +405,8 @@ static inline u32 emac_iff2rmr(struct net_device *ndev)
>
> if (ndev->flags & IFF_PROMISC)
> r |= EMAC_RMR_PME;
> - else if (ndev->flags & IFF_ALLMULTI || ndev->mc_count > 32)
> + else if (ndev->flags & IFF_ALLMULTI ||
> + (ndev->mc_count > EMAC_XAHT_SLOTS(dev)))
> r |= EMAC_RMR_PMME;
> else if (ndev->mc_count > 0)
> r |= EMAC_RMR_MAE;
> @@ -2015,10 +2023,10 @@ static int emac_get_regs_len(struct emac_instance
> *dev) {
> if (emac_has_feature(dev, EMAC_FTR_EMAC4))
> return sizeof(struct emac_ethtool_regs_subhdr) +
> - EMAC4_ETHTOOL_REGS_SIZE;
> + EMAC4_ETHTOOL_REGS_SIZE(dev);
> else
> return sizeof(struct emac_ethtool_regs_subhdr) +
> - EMAC_ETHTOOL_REGS_SIZE;
> + EMAC_ETHTOOL_REGS_SIZE(dev);
> }
>
> static int emac_ethtool_get_regs_len(struct net_device *ndev)
> @@ -2045,12 +2053,12 @@ static void *emac_dump_regs(struct emac_instance
> *dev, void *buf) hdr->index = dev->cell_index;
> if (emac_has_feature(dev, EMAC_FTR_EMAC4)) {
> hdr->version = EMAC4_ETHTOOL_REGS_VER;
> - memcpy_fromio(hdr + 1, dev->emacp, EMAC4_ETHTOOL_REGS_SIZE);
> - return ((void *)(hdr + 1) + EMAC4_ETHTOOL_REGS_SIZE);
> + memcpy_fromio(hdr + 1, dev->emacp,
> EMAC4_ETHTOOL_REGS_SIZE(dev));
> + return ((void *)(hdr + 1) + EMAC4_ETHTOOL_REGS_SIZE(dev));
> } else {
> hdr->version = EMAC_ETHTOOL_REGS_VER;
> - memcpy_fromio(hdr + 1, dev->emacp, EMAC_ETHTOOL_REGS_SIZE);
> - return ((void *)(hdr + 1) + EMAC_ETHTOOL_REGS_SIZE);
> + memcpy_fromio(hdr + 1, dev->emacp, EMAC_ETHTOOL_REGS_SIZE(dev));
> + return ((void *)(hdr + 1) + EMAC_ETHTOOL_REGS_SIZE(dev));
> }
> }
>
> @@ -2540,7 +2548,9 @@ static int __devinit emac_init_config(struct
> emac_instance *dev) }
>
> /* Check EMAC version */
> - if (of_device_is_compatible(np, "ibm,emac4")) {
> + if (of_device_is_compatible(np, "ibm,emac4sync")) {
> + dev->features |= (EMAC_FTR_EMAC4 | EMAC_FTR_EMAC4SYNC);
> + } else if (of_device_is_compatible(np, "ibm,emac4")) {
> dev->features |= EMAC_FTR_EMAC4;
> if (of_device_is_compatible(np, "ibm,emac-440gx"))
> dev->features |= EMAC_FTR_440GX_PHY_CLK_FIX;
> @@ -2601,6 +2611,15 @@ static int __devinit emac_init_config(struct
> emac_instance *dev) }
> memcpy(dev->ndev->dev_addr, p, 6);
>
> + /* IAHT and GAHT filter parameterization */
> + if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
> + dev->xaht_slots_shift = EMAC4SYNC_XAHT_SLOTS_SHIFT;
> + dev->xaht_width_shift = EMAC4SYNC_XAHT_WIDTH_SHIFT;
> + } else {
> + dev->xaht_slots_shift = EMAC4_XAHT_SLOTS_SHIFT;
> + dev->xaht_width_shift = EMAC4_XAHT_WIDTH_SHIFT;
> + }
> +
> DBG(dev, "features : 0x%08x / 0x%08x\n", dev->features,
> EMAC_FTRS_POSSIBLE); DBG(dev, "tx_fifo_size : %d (%d gige)\n",
> dev->tx_fifo_size, dev->tx_fifo_size_gige); DBG(dev, "rx_fifo_size : %d (%d
> gige)\n", dev->rx_fifo_size, dev->rx_fifo_size_gige); @@ -2672,7 +2691,8 @@
> static int __devinit emac_probe(struct of_device *ofdev, goto
> err_irq_unmap;
> }
> // TODO : request_mem_region
> - dev->emacp = ioremap(dev->rsrc_regs.start, sizeof(struct emac_regs));
> + dev->emacp = ioremap(dev->rsrc_regs.start,
> + dev->rsrc_regs.end -
> dev->rsrc_regs.start + 1);
Indentation above seems incorrect.
> if (dev->emacp == NULL) {
> printk(KERN_ERR "%s: Can't map device registers!\n",
> np->full_name);
> @@ -2884,6 +2904,10 @@ static struct of_device_id emac_match[] =
> .type = "network",
> .compatible = "ibm,emac4",
> },
> + {
> + .type = "network",
> + .compatible = "ibm,emac4sync",
> + },
> {},
> };
>
> diff --git a/drivers/net/ibm_newemac/core.h
> b/drivers/net/ibm_newemac/core.h index 1683db9..312bfa5 100644
> --- a/drivers/net/ibm_newemac/core.h
> +++ b/drivers/net/ibm_newemac/core.h
> @@ -235,6 +235,10 @@ struct emac_instance {
> u32 fifo_entry_size;
> u32 mal_burst_size; /* move to MAL ? */
>
> + /* IAHT and GAHT filter parameterization */
> + u32 xaht_slots_shift;
> + u32 xaht_width_shift;
> +
> /* Descriptor management
> */
> struct mal_descriptor *tx_desc;
> @@ -309,6 +313,10 @@ struct emac_instance {
> * Set if we need phy clock workaround for 440ep or 440gr
> */
> #define EMAC_FTR_440EP_PHY_CLK_FIX 0x00000100
> +/*
> + * The 405EX and 460EX contain the EMAC4SYNC core
> + */
> +#define EMAC_FTR_EMAC4SYNC 0x00000200
>
>
> /* Right now, we don't quite handle the always/possible masks on the
> @@ -320,7 +328,8 @@ enum {
>
> EMAC_FTRS_POSSIBLE =
> #ifdef CONFIG_IBM_NEW_EMAC_EMAC4
> - EMAC_FTR_EMAC4 | EMAC_FTR_HAS_NEW_STACR |
> + EMAC_FTR_EMAC4 | EMAC_FTR_EMAC4SYNC |
> + EMAC_FTR_HAS_NEW_STACR |
> EMAC_FTR_STACR_OC_INVERT | EMAC_FTR_440GX_PHY_CLK_FIX |
> #endif
> #ifdef CONFIG_IBM_NEW_EMAC_TAH
> @@ -342,6 +351,71 @@ static inline int emac_has_feature(struct
> emac_instance *dev, (EMAC_FTRS_POSSIBLE & dev->features & feature);
> }
>
> +/*
> + * Various instances of the EMAC core have varying 1) number of
> + * address match slots, 2) width of the registers for handling address
> + * match slots, 3) number of registers for handling address match
> + * slots and 4) base offset for those registers.
> + *
> + * These macros and inlines handle these differences based on
> + * parameters supplied by the device tree.
> + */
> +
> +#define EMAC4_XAHT_SLOTS_SHIFT 6
> +#define EMAC4_XAHT_WIDTH_SHIFT 4
> +#define EMAC4_XAHT_BASE_OFFSET 0x30
> +
> +#define EMAC4SYNC_XAHT_SLOTS_SHIFT 8
> +#define EMAC4SYNC_XAHT_WIDTH_SHIFT 5
> +#define EMAC4SYNC_XAHT_BASE_OFFSET 0x80
> +
> +
> +#define EMAC_XAHT_SLOTS(dev) (1 << (dev)->xaht_slots_shift)
> +#define EMAC_XAHT_WIDTH(dev) (1 << (dev)->xaht_width_shift)
> +#define EMAC_XAHT_REGS(dev) (1 << ((dev)->xaht_slots_shift
> - \
> + (dev)->xaht_width_shift))
> +
> +#define EMAC_XAHT_CRC_TO_SLOT(dev, crc) \
> + ((EMAC_XAHT_SLOTS(dev) - 1) - \
> + ((crc) >> ((sizeof (u32) * BITS_PER_BYTE) - \
> + (dev)->xaht_slots_shift)))
> +
> +#define EMAC_XAHT_SLOT_TO_REG(dev, slot) \
> + ((slot) >> (dev)->xaht_width_shift)
> +
> +#define EMAC_XAHT_SLOT_TO_MASK(dev, slot) \
> + ((u32)(1 << (EMAC_XAHT_WIDTH(dev) - 1)) >> \
> + ((slot) & (u32)(EMAC_XAHT_WIDTH(dev) - 1)))
> +
> +static inline u32 *emac_xaht_base(struct emac_instance *dev)
> +{
> + struct emac_regs __iomem *p = dev->emacp;
> + int offset;
> +
> + if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC))
> + offset = EMAC4SYNC_XAHT_BASE_OFFSET;
> + else
> + offset = EMAC4_XAHT_BASE_OFFSET;
Indentation with 4 spaces instead of one tab above "offset =" (twice).
Thanks.
BTW: You should send those ibm_newemac related patches to the netdev list too.
Best regards,
Stefan
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
