Re: [PATCH v4 4/4] drivers: net: Add APM X-Gene SoC ethernet driver support.
From: Iyappan Subramanian
Date: Thu May 29 2014 - 19:48:53 EST
On Wed, May 14, 2014 at 8:18 AM, Dean Nelson <dnelson@xxxxxxxxxx> wrote:
> On 05/05/2014 04:47 PM, Iyappan Subramanian wrote:
>>
>> This patch adds network driver for APM X-Gene SoC ethernet.
>>
>> Signed-off-by: Iyappan Subramanian <isubramanian@xxxxxxx>
>> Signed-off-by: Ravi Patel <rapatel@xxxxxxx>
>> Signed-off-by: Keyur Chudgar <kchudgar@xxxxxxx>
>> ---
>> drivers/net/ethernet/Kconfig | 1 +
>> drivers/net/ethernet/Makefile | 1 +
>> drivers/net/ethernet/apm/Kconfig | 1 +
>> drivers/net/ethernet/apm/Makefile | 5 +
>> drivers/net/ethernet/apm/xgene/Kconfig | 9 +
>> drivers/net/ethernet/apm/xgene/Makefile | 6 +
>> drivers/net/ethernet/apm/xgene/xgene_enet_hw.c | 807
>> +++++++++++++++++++
>> drivers/net/ethernet/apm/xgene/xgene_enet_hw.h | 353 ++++++++
>> drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 936
>> ++++++++++++++++++++++
>> drivers/net/ethernet/apm/xgene/xgene_enet_main.h | 131 +++
>> 10 files changed, 2250 insertions(+)
>> create mode 100644 drivers/net/ethernet/apm/Kconfig
>> create mode 100644 drivers/net/ethernet/apm/Makefile
>> create mode 100644 drivers/net/ethernet/apm/xgene/Kconfig
>> create mode 100644 drivers/net/ethernet/apm/xgene/Makefile
>> create mode 100644 drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> create mode 100644 drivers/net/ethernet/apm/xgene/xgene_enet_hw.h
>> create mode 100644 drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>> create mode 100644 drivers/net/ethernet/apm/xgene/xgene_enet_main.h
>
>
> Below, you'll find some comments from my review (as far as I've
> gotten)...
>
> There's an inter-related piece centered on ring->id and RING_BUFNUM(),
> with comments scattered throughout. You may have to read all the
> comments related to it before what I'm trying to convey makes sense.
> If indeed anything of what I'm trying to say can make any sense at
> all. :-) For I might be missing the obvious.
>
> I'll continue my review as time permits, but thought it best to send
> what I've seen so far for your consideration.
>
> Dean
Thanks Dean. I really appreciate the in-depth review around
RING_OWNER and RING_BUFNUM :-)
X-Gene hardware uses RING_OWNER and RING_BUFNUM to uniquely
identify each ring. ( ring_owner is 4 bits and bufnum is 6 bits
value). That is
why you see (ring_owner << 6 | bufnum) code.
>
>
> <snip>
>
>> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> new file mode 100644
>> index 0000000..421a841
>> --- /dev/null
>> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>
>
> <snip>
>
>> +
>> +struct xgene_enet_desc_ring *xgene_enet_setup_ring(
>> + struct xgene_enet_desc_ring *ring)
>> +{
>> + u32 size = ring->size;
>> + u32 i, data;
>> + u64 *desc;
>> +
>> + xgene_enet_clr_ring_state(ring);
>> + xgene_enet_set_ring_state(ring);
>> + xgene_enet_set_ring_id(ring);
>> +
>> + ring->slots = IS_FP(ring->id) ? size / 16 : size / 32;
>> +
>> + if (IS_FP(ring->id) || RING_OWNER(ring) != RING_OWNER_CPU)
>> + goto out;
>
>
> Since we will bail out here, if (ring->id & 0x20) is true...
>
>
>> +
>> + for (i = 0; i < ring->slots; i++) {
>> + desc = (u64 *)&ring->desc[i];
>> + desc[EMPTY_SLOT_INDEX] = EMPTY_SLOT;
>> + }
>> +
>> + xgene_enet_ring_rd32(ring, CSR_RING_NE_INT_MODE, &data);
>> + data |= (1 << (31 - RING_BUFNUM(ring)));
>
>
> Then RING_BUFNUM(ring) should always be 0 here, since I don't see
> the 'bufnum' portion of ring->id being anything other than 0x20 or 0.
> So why bother?
There is 1 Tx ring, 1 Bufpool ring, 1 Rx ring used in the code that I submitted.
But the plan is to add more rings. Regular bufnum starts at 0 and bufpool
bufnum start at 0x20.
>
>
>
>> + xgene_enet_ring_wr32(ring, CSR_RING_NE_INT_MODE, data);
>> +
>> +out:
>> + return ring;
>> +}
>> +
>> +void xgene_enet_clear_ring(struct xgene_enet_desc_ring *ring)
>> +{
>> + u32 data;
>> +
>> + if (IS_FP(ring->id) || RING_OWNER(ring) != RING_OWNER_CPU)
>> + goto out;
>
>
> And again, since we will bail out here, if (ring->id & 0x20) is true...
>
>
>> +
>> + xgene_enet_ring_rd32(ring, CSR_RING_NE_INT_MODE, &data);
>> + data &= ~(u32) (1 << (31 - RING_BUFNUM(ring)));
>
>
> Then RING_BUFNUM(ring) should always be 0 here, since I don't see
> the 'bufnum' portion of ring->id being anything other than 0x20 or 0.
> So why bother?
>
>
>
>> + xgene_enet_ring_wr32(ring, CSR_RING_NE_INT_MODE, data);
>> +
>> +out:
>> + xgene_enet_clr_desc_ring_id(ring);
>> + xgene_enet_clr_ring_state(ring);
>> +}
>> +
>
>
>
> <snip>
>
>> +
>> +static int xgene_enet_phy_connect(struct net_device *ndev)
>> +{
>> + struct xgene_enet_pdata *pdata = netdev_priv(ndev);
>> + struct device_node *phy_np;
>> + struct phy_device *phy_dev;
>
>
> Initialize phy_dev to NULL here, to assist the addition of a 'goto'
> below.
I changed the code as per your suggestion and try to use goto
only when common code needs to handled. So initializing phy_dev
is not required.
>
>
>> + int ret = 0;
>> +
>> + struct device *dev = &pdata->pdev->dev;
>> +
>> + phy_np = of_parse_phandle(dev->of_node, "phy-handle", 0);
>> +
>
>
> Please remove the preceding blank line.
>
>
>
>> + if (!phy_np) {
>> + netdev_dbg(ndev, "No phy-handle found\n");
>> + ret = -ENODEV;
>
>
> The following line should be added here...
>
> goto out;
>>
>> + }
>> +
>> + phy_dev = of_phy_connect(ndev, phy_np, &xgene_enet_adjust_link,
>>
>> + 0, pdata->phy_mode);
>> + if (!phy_dev) {
>> + netdev_err(ndev, "Could not connect to PHY\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> +out:
>> + pdata->phy_link = 0;
>> + pdata->phy_speed = 0;
>> + pdata->phy_dev = phy_dev;
>> +
>> + return ret;
>> +}
>> +
>
>
>
> <snip>
>
>> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.h
>> b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.h
>> new file mode 100644
>> index 0000000..a4c0a14
>> --- /dev/null
>> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.h
>
>
> <snip>
>
>> +
>> +static inline u32 get_bits(u32 val, u32 start, u32 end)
>> +{
>> + return (val & GENMASK(end, start)) >> start;
>> +}
>> +
>> +#define CSR_RING_ID 0x00000008
>> +#define OVERWRITE BIT(31)
>> +#define IS_BUFFER_POOL BIT(20)
>> +#define PREFETCH_BUF_EN BIT(21)
>> +#define CSR_RING_ID_BUF 0x0000000c
>> +#define CSR_RING_NE_INT_MODE 0x0000017c
>> +#define CSR_RING_CONFIG 0x0000006c
>> +#define CSR_RING_WR_BASE 0x00000070
>> +#define NUM_RING_CONFIG 5
>> +#define BUFPOOL_MODE 3
>> +#define RM3 3
>> +#define INC_DEC_CMD_ADDR 0x2c
>> +#define IS_FP(x) ((x & 0x0020) ? 1 : 0)
>
>
> IS_FP() is only ever called with 'ring->id' as the argument 'x'.
>
> And this macro should really be defined as...
>
> #define IS_FP(x) (((x) & 0x0020) ? 1 : 0)
>
> with a parentheses around the argument x. (And this holds true for
> all your macros defined here, they should have parentheses around each
> of their arguments in the body of the macro.)
>
>
I will add paranthesis around each of the arguments.
I changed IS_FP to function.
>
>> +#define UDP_HDR_SIZE 2
>> +
>> +#define CREATE_MASK(pos, len) GENMASK(pos+len-1, pos)
>> +#define CREATE_MASK_ULL(pos, len) GENMASK_ULL(pos+len-1, pos)
>
>
> Add parentheses around args in the above two macros.
>
>
>
>> +
>> +/* Empty slot soft signature */
>> +#define EMPTY_SLOT_INDEX 1
>> +#define EMPTY_SLOT ~0ULL
>> +
>> +#define RING_BUFNUM(q) (q->id & 0x003F)
>> +#define RING_OWNER(q) ((q->id & 0x03C0) >> 6)
>
>
> Add parentheses around args...
>
> #define RING_BUFNUM(q) ((q)->id & 0x003F)
> #define RING_OWNER(q) (((q)->id & 0x3C0) >> 6)
>
>
> Taking IS_FP(), together with RING_BUFNUM() and RING_OWNER(), I gather
> that ring->id is...
>
> 0x03C0 owner
> 0x0020 buf_pool flag
> 0x001F bufnum
>
> But I don't see bufnum ever being set to anything other than 0.
> Wherever RING_BUFNUM() is called, either a check for 0x0020 being set
> precedes it (and if true returns), or 0x0020 is subtracted from it. So
> that bit can't be playing a part in what one might consider the bufnum
> to be. Is this correct? Or am I missing something (perhaps the obvious)?
Subtraction of 0x20 is required for the bufpool bufnum, since the
hardware expects in that format.
>
>
>> +#define BUF_LEN_CODE_2K 0x5000
>> +
>
>
> <snip>
>
>> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>> b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>> new file mode 100644
>> index 0000000..0feb571
>> --- /dev/null
>> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>
>
>
> <snip>
>
>> +
>> +static int xgene_enet_create_desc_rings(struct net_device *ndev)
>> +{
>> + struct xgene_enet_pdata *pdata = netdev_priv(ndev);
>> + struct device *dev = &pdata->pdev->dev;
>> + struct xgene_enet_desc_ring *rx_ring, *tx_ring, *cp_ring;
>> + struct xgene_enet_desc_ring *buf_pool = NULL;
>> + u32 ring_num = 0;
>> + u32 ring_id;
>> + int ret = 0;
>> +
>> + /* allocate rx descriptor ring */
>> + ring_id = (RING_OWNER_CPU << 6) | RING_BUFNUM_REGULAR;
>
>
> Here we see what will become the value of ring->id being set up
> RING_BUFNUM_REGULAR is 0. I don't see a non-zero bufnum being set
> here (or anywhere else).
>
> And this should be made into a macro and defined along side of
> RING_BUFNUM() and RING_OWNER(). Perhaps something like...
>
> #define SET_RING_ID(owner, bufnum) ((owner) << 6) | (bufnum))
>
> or some such. And you may consider changing these to functions for
> the advantage that has over macros. The compiler can inline them.
I added set_ring_id function.
>
>
>
>> + rx_ring = xgene_enet_create_desc_ring(ndev, ring_num++,
>> + RING_CFGSIZE_16KB, ring_id);
>> + if (IS_ERR_OR_NULL(rx_ring)) {
>> + ret = PTR_ERR(rx_ring);
>> + goto err;
>> + }
>> +
>> + /* allocate buffer pool for receiving packets */
>> + ring_id = (RING_OWNER_ETH0 << 6) | RING_BUFNUM_BUFPOOL;
>
>
> And again here, RING_BUFNUM_BUFPOOL is 0x20. But that's just a flag
> that indicates that this ring is a buf_pool. I don't see a non-zero
> bufnum being set here (or anywhere else).
>
> And a macro like 'SET_RING_ID()' as mentioned above, should be used
> here.
>
>
>
>> + buf_pool = xgene_enet_create_desc_ring(ndev, ring_num++,
>> + RING_CFGSIZE_2KB, ring_id);
>> + if (IS_ERR_OR_NULL(buf_pool)) {
>> + ret = PTR_ERR(buf_pool);
>> + goto err;
>> + }
>> +
>> + rx_ring->nbufpool = NUM_BUFPOOL;
>> + rx_ring->buf_pool = buf_pool;
>> + rx_ring->irq = pdata->rx_irq;
>> + buf_pool->rx_skb = devm_kcalloc(dev, buf_pool->slots,
>> + sizeof(struct sk_buff *),
>> GFP_KERNEL);
>> + if (!buf_pool->rx_skb) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + buf_pool->dst_ring_num = xgene_enet_dst_ring_num(buf_pool);
>> + rx_ring->buf_pool = buf_pool;
>> + pdata->rx_ring = rx_ring;
>> +
>> + /* allocate tx descriptor ring */
>> + ring_id = (RING_OWNER_ETH0 << 6) | RING_BUFNUM_REGULAR;
>
>
> And again here. same story as above.
>
>
>
>> + tx_ring = xgene_enet_create_desc_ring(ndev, ring_num++,
>> + RING_CFGSIZE_16KB, ring_id);
>> + if (IS_ERR_OR_NULL(tx_ring)) {
>> + ret = PTR_ERR(tx_ring);
>> + goto err;
>> + }
>> + pdata->tx_ring = tx_ring;
>> +
>> + cp_ring = pdata->rx_ring;
>> + cp_ring->cp_skb = devm_kcalloc(dev, tx_ring->slots,
>> + sizeof(struct sk_buff *),
>> GFP_KERNEL);
>> + if (!cp_ring->cp_skb) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> + pdata->tx_ring->cp_ring = cp_ring;
>> + pdata->tx_ring->dst_ring_num = xgene_enet_dst_ring_num(cp_ring);
>> +
>> + pdata->tx_qcnt_hi = pdata->tx_ring->slots / 2;
>> + pdata->cp_qcnt_hi = pdata->rx_ring->slots / 2;
>> + pdata->cp_qcnt_low = pdata->cp_qcnt_hi / 2;
>> +
>> + return 0;
>> +
>> +err:
>> + xgene_enet_delete_desc_rings(pdata);
>> + return ret;
>> +}
>> +
>
>
> <snip>
>
>> +
>> +static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
>> +{
>> + struct platform_device *pdev;
>> + struct net_device *ndev;
>> + struct device *dev;
>> + struct resource *res;
>> + void *base_addr;
>> + const char *mac;
>> + int ret = 0;
>> +
>> + pdev = pdata->pdev;
>> + dev = &pdev->dev;
>> + ndev = pdata->ndev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(dev, "Resource IORESOURCE_MEM 0 not defined\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + pdata->base_addr = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(pdata->base_addr)) {
>> + dev_err(dev, "Unable to retrieve ENET Port CSR region\n");
>> + return PTR_ERR(pdata->base_addr);
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (!res) {
>> + dev_err(dev, "Resource IORESOURCE_MEM 1 not defined\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + pdata->ring_csr_addr = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(pdata->ring_csr_addr)) {
>> + dev_err(dev, "Unable to retrieve ENET Ring CSR region\n");
>> + return PTR_ERR(pdata->ring_csr_addr);
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> + if (!res) {
>> + dev_err(dev, "Resource IORESOURCE_MEM 2 not defined\n");
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> + pdata->ring_cmd_addr = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(pdata->ring_cmd_addr)) {
>> + dev_err(dev, "Unable to retrieve ENET Ring command
>> region\n");
>> + return PTR_ERR(pdata->ring_cmd_addr);
>> + }
>> +
>> + ret = platform_get_irq(pdev, 0);
>> + if (ret <= 0) {
>
>
> If you return 0 as an error return value from this function, the caller
> will have no idea anything was amiss.
>
>
>
>> + dev_err(dev, "Unable to get ENET Rx IRQ\n");
>
>
> So you need to at least convert the 0 to a sensible error return,
> leaving the others as is...
>
> ret = ret ? : -ENXIO;
>
> or just reset them all..
>
> ret = -ENXIO;
>
> You can chose the error return value that makes the most sense to you.
> I've seen others use: -ENXIO, -EINVAL, and -ENODEV.
Great catch. I have taken care of 0 case.
>
>
>
>> + goto out;
>> + }
>> + pdata->rx_irq = ret;
>> +
>> + mac = of_get_mac_address(dev->of_node);
>> + if (mac)
>> + memcpy(ndev->dev_addr, mac, ndev->addr_len);
>> + else
>> + eth_hw_addr_random(ndev);
>> + memcpy(ndev->perm_addr, ndev->dev_addr, ndev->addr_len);
>> +
>> + pdata->phy_mode = of_get_phy_mode(pdev->dev.of_node);
>> + if (pdata->phy_mode < 0) {
>> + dev_err(dev, "Incorrect phy-connection-type in DTS\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + pdata->clk = devm_clk_get(&pdev->dev, NULL);
>> + ret = IS_ERR(pdata->clk);
>
>
> IS_ERR() does not yield a proper return error value. For that one needs
> to use PTR_ERR(). So remove the preceding line, and change the following
> line...
>
>> + if (ret) {
>
>
> to...
>
> if (IS_ERR(pdata->clk)) {
>
>
>> + dev_err(&pdev->dev, "can't get clock\n");
>
>
> And add the following line here...
>
> ret = PTR_ERR(info->clk);
I modified the code as per your suggestion.
>
>> + goto out;
>> + }
>> +
>> + base_addr = pdata->base_addr;
>> + pdata->eth_csr_addr = base_addr + BLOCK_ETH_CSR_OFFSET;
>> + pdata->eth_ring_if_addr = base_addr + BLOCK_ETH_RING_IF_OFFSET;
>> + pdata->eth_diag_csr_addr = base_addr + BLOCK_ETH_DIAG_CSR_OFFSET;
>> + pdata->mcx_mac_addr = base_addr + BLOCK_ETH_MAC_OFFSET;
>> + pdata->mcx_stats_addr = base_addr + BLOCK_ETH_STATS_OFFSET;
>> + pdata->mcx_mac_csr_addr = base_addr + BLOCK_ETH_MAC_CSR_OFFSET;
>> + pdata->rx_buff_cnt = NUM_PKT_BUF;
>> +out:
>> + return ret;
>
>
> The mixture of 'goto' and 'return' usage in this function is confusing.
> I'd think it best if they were all the same. Because of the following,
> which is stated in Documentation/CodingStyle (chapter 7)...
>
> The goto statement comes in handy when a function exits from multiple
> locations and some common work such as cleanup has to be done. If there
> is no
> cleanup needed then just return directly.
>
> And since you don't have any common work being done, my vote is with
> using returns and not gotos.
>
> I'd suggest you consider replacing gotos by returns in all functions
> which simply return without having any common work to be done.
I modified the code as per the coding guidelines.
>
>
>
>> +}
>> +
>> +static int xgene_enet_init_hw(struct xgene_enet_pdata *pdata)
>> +{
>> + struct net_device *ndev = pdata->ndev;
>> + struct xgene_enet_desc_ring *buf_pool;
>> + int ret = 0;
>> +
>> + xgene_enet_reset(pdata);
>> +
>> + xgene_gmac_tx_disable(pdata);
>> + xgene_gmac_rx_disable(pdata);
>> +
>> + ret = xgene_enet_create_desc_rings(ndev);
>> + if (ret) {
>> + netdev_err(ndev, "Error in ring configuration\n");
>> + goto out;
>> + }
>> +
>> + /* setup buffer pool */
>> + buf_pool = pdata->rx_ring->buf_pool;
>> + xgene_enet_init_bufpool(buf_pool);
>> + ret = xgene_enet_refill_bufpool(buf_pool, pdata->rx_buff_cnt);
>> + if (ret)
>> + goto out;
>> +
>> + xgene_enet_cle_bypass(pdata,
>> xgene_enet_dst_ring_num(pdata->rx_ring),
>> + RING_BUFNUM(buf_pool) - 0x20, 0);
>
>
> Subtracting an unidentified number (0x20) from RING_BUFNUM(buf_pool)
> doesn't seem to me to be the best approach here. If I'm not mistaken,
> it appears the 0x20 is a flag set in ring->id to indicate that this is
> a buf_pool. And here you're trying to grab just the non-flag portion
> of ring->id's 0x003f, which amounts to 0x001f.
>
> So maybe given the other things I've mentioned about RING_BUFNUM() in
> this review, if it is still needed, change it to be...
>
> #define RING_BUFNUM(q) ((q)->id & 0x001F)
>
> Or since the 3rd argument to xgene_enet_cle_bypass() is called fpsel,
> you might create a new macro called GET_FPSEL(), and make it like the
> one just mentioned.
I moved the complexity inside the function.
>
> But again, as mentioned elsewhere, the value will always be zero for
> the driver as it is now. So is there a point to this?
>
>
>
>> + xgene_gmac_init(pdata, SPEED_1000);
>> +out:
>> + return ret;
>> +}
>
>
> <snip>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/