Re: [PATCH net-next 10/12] net: stmmac: tc956x: add TC956x/QPS615 support

[Andrew Lunn <andrew@lunn.ch>]

> +static struct tc956x_mac_speed mac_speed[] = {
> +	{ PHY_INTERFACE_MODE_2500BASEX,	SPEED_2500,  SP_SEL_SGMII_2500M, },
> +	{ PHY_INTERFACE_MODE_SGMII,	SPEED_2500,  SP_SEL_SGMII_2500M, },
> +	{ PHY_INTERFACE_MODE_SGMII,	SPEED_1000,  SP_SEL_SGMII_1000M, },

That looks odd. Some vendors implemented 2500BaseX using SGMII
overclocked. But that is not strictly 2500BaseX. Having the 2500BASEX
entry suggests you have real 2500BASEX, so why have an SGMII entry
with SPEED_2500?

> +/* We have one IRQ chip instance with 25 IRQs in its domain */

One per MAC, or one overall?

> +static struct irq_domain *
> +tc956x_msigen_irq_domain_instantiate(struct tc956x_data *td)
> +{
> +	struct irq_domain_chip_generic_info dgc_info;
> +	struct irq_domain_info info;
> +
> +	dgc_info.name = "tc956x-msigen";

If it is one per MAC, maybe this name should indicate which instance
of the MAC this is.

> +static int tc956x_mac_setup(void *apriv, struct mac_device_info *mac)
> +{
> +	struct stmmac_priv *priv = apriv;
> +	struct stmmac_desc_ops *desc;
> +	struct stmmac_dma_ops *dma;
> +	struct tc956x_data *td;
> +
> +	td = priv->plat->bsp_priv;
> +
> +	/* dwxgmac301_dma_ops needs extending to provide DMA address translation */
> +	dma = &td->dma;
> +	*dma = dwxgmac301_dma_ops;
> +	dma->init_rx_chan = tc956x_dma_init_rx_chan;
> +	dma->init_tx_chan = tc956x_dma_init_tx_chan;
> +	mac->dma = dma;

I could be reading this wrong....

dma points to the global dwxgmac301_dma_ops, which you added a few
patches back.

You then modify it, changing two values in it.

Doesn't that break any other dwxgmac301 in the system? Shouldn't you
be making a copy of the global structure, and then making
modifications to your copy? mac->dma then points to your copy?

> +	/* dwxgmac210_desc_ops also needs extending for the same reason */
> +	desc = &td->desc;
> +	*desc = dwxgmac210_desc_ops;
> +	desc->set_addr = tc956x_desc_set_addr;
> +	desc->set_sec_addr = tc956x_desc_set_sec_addr;
> +	mac->desc = desc;

And the same problem here?

> +/* Called by tc956x_dwmac_probe(); return errors with dev_err_probe() */
> +static int tc956x_dwmac_parse_dt(struct tc956x_data *td)
> +{
> +	struct device_node *mdio_node;
> +	struct device *dev = td->dev;
> +	struct device_node *np;
> +
> +	np = dev_of_node(dev);
> +	if (!np)
> +		return dev_err_probe(dev, -EINVAL, "no devicetree node\n");
> +
> +	/* Find the MDIO bus */
> +	for_each_child_of_node(np, mdio_node) {
> +		if (of_device_is_compatible(mdio_node,
> +					    "snps,dwmac-mdio"))
> +			break;
> +	}

It looks like if you put the ethernet properties into an ethernet node
in DT, this might go away? Or at least allow you to use
stmmac_of_get_mdio(). 

	Andrew