Re: [PATCH 7/7] net: stmmac: dwc-qos: Add Tegra186 support

[Thierry Reding]

On Mon, Feb 27, 2017 at 01:46:01PM +0200, Mikko Perttunen wrote:
> On 23.02.2017 19:24, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> > 
> > The NVIDIA Tegra186 SoC contains an instance of the Synopsys DWC
> > ethernet QOS IP core. The binding that it uses is slightly different
> > from existing ones because of the integration (clocks, resets, ...).
> > 
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> >  .../ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c    | 252 +++++++++++++++++++++
> >  1 file changed, 252 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > index 5071d3c15adc..54dfbdc48f6d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-dwc-qos-eth.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> >  #include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/io.h>
> >  #include <linux/ioport.h>
> > @@ -22,10 +23,24 @@
> >  #include <linux/of_net.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >  #include <linux/stmmac.h>
> > 
> >  #include "stmmac_platform.h"
> > 
> > +struct tegra_eqos {
> > +	struct device *dev;
> > +	void __iomem *regs;
> > +
> > +	struct reset_control *rst;
> > +	struct clk *clk_master;
> > +	struct clk *clk_slave;
> > +	struct clk *clk_tx;
> > +	struct clk *clk_rx;
> > +
> > +	struct gpio_desc *reset;
> > +};
> > +
> >  static int dwc_eth_dwmac_config_dt(struct platform_device *pdev,
> >  				   struct plat_stmmacenet_data *plat_dat)
> >  {
> > @@ -148,6 +163,237 @@ static int dwc_qos_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> > 
> > +#define SDMEMCOMPPADCTRL 0x8800
> > +#define  SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD BIT(31)
> > +
> > +#define AUTO_CAL_CONFIG 0x8804
> > +#define  AUTO_CAL_CONFIG_START BIT(31)
> > +#define  AUTO_CAL_CONFIG_ENABLE BIT(29)
> > +
> > +#define AUTO_CAL_STATUS 0x880c
> > +#define  AUTO_CAL_STATUS_ACTIVE BIT(31)
> > +
> > +static void tegra_eqos_fix_speed(void *priv, unsigned int speed)
> > +{
> > +	struct tegra_eqos *eqos = priv;
> > +	unsigned long rate = 125000000;
> > +	bool needs_calibration = false;
> > +	unsigned int i;
> > +	u32 value;
> > +
> > +	switch (speed) {
> > +	case SPEED_1000:
> > +		needs_calibration = true;
> > +		rate = 125000000;
> > +		break;
> > +
> > +	case SPEED_100:
> > +		needs_calibration = true;
> > +		rate = 25000000;
> > +		break;
> > +
> > +	case SPEED_10:
> > +		rate = 2500000;
> > +		break;
> > +
> > +	default:
> > +		dev_err(eqos->dev, "invalid speed %u\n", speed);
> > +		break;
> > +	}
> > +
> > +	if (needs_calibration) {
> > +		/* calibrate */
> > +		value = readl(eqos->regs + SDMEMCOMPPADCTRL);
> > +		value |= SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD;
> > +		writel(value, eqos->regs + SDMEMCOMPPADCTRL);
> > +
> > +		udelay(1);
> > +
> > +		value = readl(eqos->regs + AUTO_CAL_CONFIG);
> > +		value |= AUTO_CAL_CONFIG_START | AUTO_CAL_CONFIG_ENABLE;
> > +		writel(value, eqos->regs + AUTO_CAL_CONFIG);
> > +
> > +		for (i = 0; i <= 10; i++) {
> > +			value = readl(eqos->regs + AUTO_CAL_STATUS);
> > +			if (value & AUTO_CAL_STATUS_ACTIVE)
> > +				break;
> > +
> > +			udelay(1);
> > +		}
> > +
> > +		if ((value & AUTO_CAL_STATUS_ACTIVE) == 0) {
> > +			dev_err(eqos->dev, "calibration did not start\n");
> > +			goto failed;
> > +		}
> > +
> > +		for (i = 0; i <= 10; i++) {
> > +			value = readl(eqos->regs + AUTO_CAL_STATUS);
> > +			if ((value & AUTO_CAL_STATUS_ACTIVE) == 0)
> > +				break;
> > +
> > +			udelay(20);
> > +		}
> > +
> > +		if (value & AUTO_CAL_STATUS_ACTIVE) {
> > +			dev_err(eqos->dev, "calibration didn't finish\n");
> > +			goto failed;
> > +		}
> 
> Could use readl_poll_timeout/readl_poll_timeout_atomic for these loops
> instead.

I had considered that, but the code ends up looking a lot uglier with
that. But since you and Joao both prefer it, I'll switch over to the I/O
poll helpers anyway.

> 
> > +
> > +	failed:
> > +		value = readl(eqos->regs + SDMEMCOMPPADCTRL);
> > +		value &= ~SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD;
> > +		writel(value, eqos->regs + SDMEMCOMPPADCTRL);
> > +	} else {
> > +		value = readl(eqos->regs + AUTO_CAL_CONFIG);
> > +		value &= ~AUTO_CAL_CONFIG_ENABLE;
> > +		writel(value, eqos->regs + AUTO_CAL_CONFIG);
> > +	}
> > +
> > +	clk_set_rate(eqos->clk_tx, rate);
> 
> Could check error code here, and for other clock ops too.

Done.

> > +}
> > +
> > +static int tegra_eqos_init(struct platform_device *pdev, void *priv)
> > +{
> > +	struct tegra_eqos *eqos = priv;
> > +	unsigned long rate;
> > +	u32 value;
> > +
> > +	rate = clk_get_rate(eqos->clk_slave);
> > +
> > +	value = readl(eqos->regs + 0xdc);
> 
> No point in reading the value when it is fully overwritten.

Good catch.

> > +	value = (rate / 1000000) - 1;
> > +	writel(value, eqos->regs + 0xdc);
> 
> Please add a define for 0xdc.

Right, I must've overlooked that while cleaning up the patch. I've added
a GMAC_1US_TIC_COUNTER define for it.

> > @@ -245,6 +496,7 @@ static int dwc_eth_dwmac_remove(struct platform_device *pdev)
> > 
> >  static const struct of_device_id dwc_eth_dwmac_match[] = {
> >  	{ .compatible = "snps,dwc-qos-ethernet-4.10", .data = &dwc_qos_data },
> > +	{ .compatible = "nvidia,tegra186-eqos", .data = &tegra_eqos_data},
> 
> Missing space before '}'.

Good catch! Fixed.

Thanks,
Thierry

Attachment: signature.asc
Description: PGP signature