diff mbox series

[v2,2/2] net: stmmac: Add Ingenic SoCs MAC support.

Message ID 1623260110-25842-3-git-send-email-zhouyanjie@wanyeetech.com (mailing list archive)
State New, archived
Headers show
Series Add Ingenic SoCs MAC support. | expand

Commit Message

Zhou Yanjie June 9, 2021, 5:35 p.m. UTC
Add support for Ingenic SoC MAC glue layer support for the stmmac
device driver. This driver is used on for the MAC ethernet controller
found in the JZ4775 SoC, the X1000 SoC, the X1600 SoC, the X1830 SoC,
and the X2000 SoC.

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
---

Notes:
    v1->v2:
    1.Fix uninitialized variable.
    2.Add missing RGMII-ID, RGMII-RXID, and RGMII-TXID.
    3.Change variable val from int to unsinged int.
    4.Get tx clock delay and rx clock delay from devicetree.

 drivers/net/ethernet/stmicro/stmmac/Kconfig        |  12 +
 drivers/net/ethernet/stmicro/stmmac/Makefile       |   1 +
 .../net/ethernet/stmicro/stmmac/dwmac-ingenic.c    | 434 +++++++++++++++++++++
 3 files changed, 447 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c

Comments

Andrew Lunn June 10, 2021, 3:19 a.m. UTC | #1
> +static int jz4775_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
> +{
> +	struct ingenic_mac *mac = plat_dat->bsp_priv;
> +	unsigned int val;


> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
> +			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
> +		break;

So this does what DT writes expect. They put 'rgmii-id' as phy
mode. The MAC does not add a delay. PHY_INTERFACE_MODE_RGMII_ID is
passed to the PHY and it adds the delay. And frames flow to/from the
PHY and users are happy. The majority of MAC drivers are like this.

> +static int x2000_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
> +{
> +	struct ingenic_mac *mac = plat_dat->bsp_priv;
> +	unsigned int val;

Here we have a complete different story. 


> +	case PHY_INTERFACE_MODE_RGMII:
> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
> +
> +		if (mac->tx_delay == 0) {
> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
> +		} else {
> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
> +
> +			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
> +			else
> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
> +		}

What are the units of tx_delay. The DT binding should be pS, and you
need to convert from that to whatever the hardware is using.

If mac->tx_delay is greater than MACPHYC_TX_DELAY_MAX, please return
-EINVAL when parsing the binding. We want the DT writer to know they
have requested something the hardware cannot do.

So if the device tree contains 'rgmii' for PHY mode, you can use this
for when you have long clock lines on your board adding the delay, and
you just need to fine tune the delay, add a few pS. The PHY will also
not add a delay, due to receiving PHY_INTERFACE_MODE_RGMII.

> +
> +		if (mac->rx_delay == 0) {
> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
> +		} else {
> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
> +
> +			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
> +			else
> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
> +		}
> +
> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
> +		break;
> +
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		val = FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN) |
> +			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN) |
> +			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_ID\n");
> +		break;

So this one is pretty normal. The MAC does not add a delay,
PHY_INTERFACE_MODE_RGMII_ID is passed to the PHY, and it adds the
delay. The interface will likely work.

> +
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
> +			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
> +
> +		if (mac->tx_delay == 0) {
> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
> +		} else {
> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
> +
> +			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
> +			else
> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
> +		}

So here, the PHY is going to be passed PHY_INTERFACE_MODE_RGMII_RXID.
The PHY will add a delay in the receive path. The MAC needs to add the
delay in the transmit path. So tx_delay needs to be the full 2ns, not
just a small fine tuning value, or the PCB is adding the delay. And
you also cannot fine tune the RX delay, since rx_delay is ignored.

> +
> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_RXID\n");
> +		break;
> +
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
> +			  FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
> +
> +		if (mac->rx_delay == 0) {
> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
> +		} else {
> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
> +
> +			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
> +			else
> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
> +		}

And here we have the opposite to PHY_INTERFACE_MODE_RGMII_RXID.

So you need to clearly document in the device tree binding when
rx_delay and tx_delay are used, and when they are ignored. You don't
want to have DT writers having to look deep into the code to figure
this out.

Personally, i would simply this, in a big way. I see two options:

1) The MAC never adds a delay. The hardware is there, but simply don't
use it, to keep thing simple, and the same as nearly every other MAC.

2) If the hardware can do small steps of delay, allow this delay, both
RX and TX, to be configured in all four modes, in order to allow for
fine tuning. Leave the PHY to insert the majority of the delay.

> +	/* Get MAC PHY control register */
> +	mac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "mode-reg");
> +	if (IS_ERR(mac->regmap)) {
> +		dev_err(&pdev->dev, "%s: failed to get syscon regmap\n", __func__);
> +		goto err_remove_config_dt;
> +	}

Please document this in the device tree binding.

> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "rx-clk-delay", &mac->rx_delay);
> +	if (ret)
> +		mac->rx_delay = 0;
> +
> +	ret = of_property_read_u32(pdev->dev.of_node, "tx-clk-delay", &mac->tx_delay);
> +	if (ret)
> +		mac->tx_delay = 0;

Please take a look at dwmac-mediatek.c. It handles delays nicely. I
would suggest that is the model to follow.

       Andrew
Zhou Yanjie June 10, 2021, 8 a.m. UTC | #2
Hi Andrew,

On 2021/6/10 上午11:19, Andrew Lunn wrote:
>> +static int jz4775_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
>> +{
>> +	struct ingenic_mac *mac = plat_dat->bsp_priv;
>> +	unsigned int val;
>
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>> +		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
>> +			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
>> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
>> +		break;
> So this does what DT writes expect. They put 'rgmii-id' as phy
> mode. The MAC does not add a delay. PHY_INTERFACE_MODE_RGMII_ID is
> passed to the PHY and it adds the delay. And frames flow to/from the
> PHY and users are happy. The majority of MAC drivers are like this.


Got it, thanks!


>
>> +static int x2000_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
>> +{
>> +	struct ingenic_mac *mac = plat_dat->bsp_priv;
>> +	unsigned int val;
> Here we have a complete different story.
>
>
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
>> +
>> +		if (mac->tx_delay == 0) {
>> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
>> +		} else {
>> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
>> +
>> +			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
>> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
>> +			else
>> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
>> +		}
> What are the units of tx_delay. The DT binding should be pS, and you
> need to convert from that to whatever the hardware is using.


The manual does not tell how much ps a unit is.

I am confirming with Ingenic, but there is no reply

at the moment. Can we follow Rockchip's approach?

According to the description in "rockchip-dwmac.yaml"

and the related code in "dwmac-rk.c", it seems that their

delay parameter seems to be the value used by the hardware

directly instead of ps.


> If mac->tx_delay is greater than MACPHYC_TX_DELAY_MAX, please return
> -EINVAL when parsing the binding. We want the DT writer to know they
> have requested something the hardware cannot do.


Sure, I'll change it in the next version.


> So if the device tree contains 'rgmii' for PHY mode, you can use this
> for when you have long clock lines on your board adding the delay, and
> you just need to fine tune the delay, add a few pS. The PHY will also
> not add a delay, due to receiving PHY_INTERFACE_MODE_RGMII.
>
>> +
>> +		if (mac->rx_delay == 0) {
>> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
>> +		} else {
>> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
>> +
>> +			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
>> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
>> +			else
>> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
>> +		}
>> +
>> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
>> +		break;
>> +
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>> +		val = FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN) |
>> +			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN) |
>> +			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
>> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_ID\n");
>> +		break;
> So this one is pretty normal. The MAC does not add a delay,
> PHY_INTERFACE_MODE_RGMII_ID is passed to the PHY, and it adds the
> delay. The interface will likely work.
>
>> +
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
>> +			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
>> +
>> +		if (mac->tx_delay == 0) {
>> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
>> +		} else {
>> +			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
>> +
>> +			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
>> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
>> +			else
>> +				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
>> +		}
> So here, the PHY is going to be passed PHY_INTERFACE_MODE_RGMII_RXID.
> The PHY will add a delay in the receive path. The MAC needs to add the
> delay in the transmit path. So tx_delay needs to be the full 2ns, not
> just a small fine tuning value, or the PCB is adding the delay. And
> you also cannot fine tune the RX delay, since rx_delay is ignored.
>
>> +
>> +		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_RXID\n");
>> +		break;
>> +
>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>> +		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
>> +			  FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
>> +
>> +		if (mac->rx_delay == 0) {
>> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
>> +		} else {
>> +			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
>> +
>> +			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
>> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
>> +			else
>> +				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
>> +		}
> And here we have the opposite to PHY_INTERFACE_MODE_RGMII_RXID.
>
> So you need to clearly document in the device tree binding when
> rx_delay and tx_delay are used, and when they are ignored. You don't
> want to have DT writers having to look deep into the code to figure
> this out.


Sure, maybe I should write a new independent document

for Ingenic instead of just making corresponding changes

in "snps, dwmac.yaml"


>
> Personally, i would simply this, in a big way. I see two options:
>
> 1) The MAC never adds a delay. The hardware is there, but simply don't
> use it, to keep thing simple, and the same as nearly every other MAC.
>
> 2) If the hardware can do small steps of delay, allow this delay, both
> RX and TX, to be configured in all four modes, in order to allow for
> fine tuning. Leave the PHY to insert the majority of the delay.


It seems that this method is better, I will adopt it in v3.


>> +	/* Get MAC PHY control register */
>> +	mac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "mode-reg");
>> +	if (IS_ERR(mac->regmap)) {
>> +		dev_err(&pdev->dev, "%s: failed to get syscon regmap\n", __func__);
>> +		goto err_remove_config_dt;
>> +	}
> Please document this in the device tree binding.


Sure.


>
>> +
>> +	ret = of_property_read_u32(pdev->dev.of_node, "rx-clk-delay", &mac->rx_delay);
>> +	if (ret)
>> +		mac->rx_delay = 0;
>> +
>> +	ret = of_property_read_u32(pdev->dev.of_node, "tx-clk-delay", &mac->tx_delay);
>> +	if (ret)
>> +		mac->tx_delay = 0;
> Please take a look at dwmac-mediatek.c. It handles delays nicely. I
> would suggest that is the model to follow.


Sure.


Thanks and best regards!


>
>         Andrew
Andrew Lunn June 10, 2021, 12:15 p.m. UTC | #3
> The manual does not tell how much ps a unit is.
> 
> I am confirming with Ingenic, but there is no reply
> 
> at the moment. Can we follow Rockchip's approach?
> 
> According to the description in "rockchip-dwmac.yaml"
> 
> and the related code in "dwmac-rk.c", it seems that their
> 
> delay parameter seems to be the value used by the hardware
> 
> directly instead of ps.

We are much more strict about this now than before. You have to use
standard units and convert to hardware values. It also makes it a lot
easier for DT writers, if they have an idea what the units mean.

Having the MAC add small delays is something you can add later,
without breaking backwards compatibility. So if you cannot determine
what the units are now, just submit the glue driver without support
for this feature. If anybody really needs it, they can do the needed
research, maybe do some measurements, and then add the code.

    Andrew
Andrew Lunn June 10, 2021, 2:42 p.m. UTC | #4
>     We are much more strict about this now than before. You have to use
>     standard units and convert to hardware values. It also makes it a lot
>     easier for DT writers, if they have an idea what the units mean.
> 
>     Having the MAC add small delays is something you can add later,
>     without breaking backwards compatibility. So if you cannot determine
>     what the units are now, just submit the glue driver without support
>     for this feature. If anybody really needs it, they can do the needed
>     research, maybe do some measurements, and then add the code.
> 
> 
> I did an experiment, when the tx delay is not set, RGMII works a

You had rgmii-id in your device tree, so that the PHY added the
delays?

	Andrew
Andrew Lunn June 10, 2021, 2:57 p.m. UTC | #5
> Here is Ingenic's reply, the time length corresponding to a unit is 19.5ps
> (19500fs).

Sometimes, there is a negative offset in the delays. So a delay value
of 0 written to the register actually means -200ps or something.

   Andrew
Zhou Yanjie June 13, 2021, 8:26 a.m. UTC | #6
Hi Andrew,

于 Thu, 10 Jun 2021 16:42:24 +0200
Andrew Lunn <andrew@lunn.ch> 写道:

> >     We are much more strict about this now than before. You have to
> > use standard units and convert to hardware values. It also makes it
> > a lot easier for DT writers, if they have an idea what the units
> > mean.
> > 
> >     Having the MAC add small delays is something you can add later,
> >     without breaking backwards compatibility. So if you cannot
> > determine what the units are now, just submit the glue driver
> > without support for this feature. If anybody really needs it, they
> > can do the needed research, maybe do some measurements, and then
> > add the code.
> > 
> > 
> > I did an experiment, when the tx delay is not set, RGMII works a  
> 
> You had rgmii-id in your device tree, so that the PHY added the
> delays?

I have tried rgmii-id and rgmii-txid. If we don’t add a fine-tuning
parameter, it still can’t work properly. In these two modes, we still
need to add about 500ps delay on the mac side to ensure it works
properly.

Thanks and best regards!

> 
> 	Andrew
Zhou Yanjie June 13, 2021, 8:34 a.m. UTC | #7
于 Thu, 10 Jun 2021 16:57:29 +0200
Andrew Lunn <andrew@lunn.ch> 写道:

> > Here is Ingenic's reply, the time length corresponding to a unit is
> > 19.5ps (19500fs).  
> 
> Sometimes, there is a negative offset in the delays. So a delay value
> of 0 written to the register actually means -200ps or something.

Ah, perhaps this explains why we still need to add fine-tuning
parameter in rgmii-id and rgmii-txid modes to ensure that the network
works properly.

> 
>    Andrew
Andrew Lunn June 13, 2021, 4:31 p.m. UTC | #8
On Sun, Jun 13, 2021 at 04:34:52PM +0800, 周琰杰 wrote:
> 于 Thu, 10 Jun 2021 16:57:29 +0200
> Andrew Lunn <andrew@lunn.ch> 写道:
> 
> > > Here is Ingenic's reply, the time length corresponding to a unit is
> > > 19.5ps (19500fs).  
> > 
> > Sometimes, there is a negative offset in the delays. So a delay value
> > of 0 written to the register actually means -200ps or something.
> 
> Ah, perhaps this explains why we still need to add fine-tuning
> parameter in rgmii-id and rgmii-txid modes to ensure that the network
> works properly.

Please try to find this out. rgmii means no delay. If the hardware is
doing -500pS by default, you need to take this into account, and add
the 500pS back on.

    Andrew
Zhou Yanjie June 14, 2021, 3:57 p.m. UTC | #9
Hi Andrew,

于 Sun, 13 Jun 2021 18:31:36 +0200
Andrew Lunn <andrew@lunn.ch> 写道:

> On Sun, Jun 13, 2021 at 04:34:52PM +0800, 周琰杰 wrote:
> > 于 Thu, 10 Jun 2021 16:57:29 +0200
> > Andrew Lunn <andrew@lunn.ch> 写道:
> >   
> > > > Here is Ingenic's reply, the time length corresponding to a
> > > > unit is 19.5ps (19500fs).    
> > > 
> > > Sometimes, there is a negative offset in the delays. So a delay
> > > value of 0 written to the register actually means -200ps or
> > > something.  
> > 
> > Ah, perhaps this explains why we still need to add fine-tuning
> > parameter in rgmii-id and rgmii-txid modes to ensure that the
> > network works properly.  
> 
> Please try to find this out. rgmii means no delay. If the hardware is
> doing -500pS by default, you need to take this into account, and add
> the 500pS back on.

I think I may have found the problem. At present, my PHY uses a
general driver, and there is no specific setting for delay-related
registers. The default delay value of PHY is 1ns, which does not meet
the delay requirement of 2ns, after and the MAC side add 500ps delay
(and possibly some delays introduced on the hardware circuit), it just
meets the requirement of 2ns delay.

> 
>     Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 7737e4d0..9a19e4d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -66,6 +66,18 @@  config DWMAC_ANARION
 
 	  This selects the Anarion SoC glue layer support for the stmmac driver.
 
+config DWMAC_INGENIC
+	tristate "Ingenic MAC support"
+	default MACH_INGENIC
+	depends on OF && HAS_IOMEM && (MACH_INGENIC || COMPILE_TEST)
+	select MFD_SYSCON
+	help
+	  Support for ethernet controller on Ingenic SoCs.
+
+	  This selects Ingenic SoCs glue layer support for the stmmac
+	  device driver. This driver is used on for the Ingenic SoCs
+	  MAC ethernet controller.
+
 config DWMAC_IPQ806X
 	tristate "QCA IPQ806x DWMAC support"
 	default ARCH_QCOM
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index f2e478b..6471f93 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -14,6 +14,7 @@  stmmac-$(CONFIG_STMMAC_SELFTESTS) += stmmac_selftests.o
 # Ordering matters. Generic driver must be last.
 obj-$(CONFIG_STMMAC_PLATFORM)	+= stmmac-platform.o
 obj-$(CONFIG_DWMAC_ANARION)	+= dwmac-anarion.o
+obj-$(CONFIG_DWMAC_INGENIC)	+= dwmac-ingenic.o
 obj-$(CONFIG_DWMAC_IPQ806X)	+= dwmac-ipq806x.o
 obj-$(CONFIG_DWMAC_LPC18XX)	+= dwmac-lpc18xx.o
 obj-$(CONFIG_DWMAC_MEDIATEK)	+= dwmac-mediatek.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c
new file mode 100644
index 00000000..fbe2727
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-ingenic.c
@@ -0,0 +1,434 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * dwmac-ingenic.c - Ingenic SoCs DWMAC specific glue layer
+ *
+ * Copyright (c) 2021 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_net.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "stmmac_platform.h"
+
+#define MACPHYC_TXCLK_SEL_MASK		GENMASK(31, 31)
+#define MACPHYC_TXCLK_SEL_OUTPUT	0x1
+#define MACPHYC_TXCLK_SEL_INPUT		0x0
+#define MACPHYC_MODE_SEL_MASK		GENMASK(31, 31)
+#define MACPHYC_MODE_SEL_RMII		0x0
+#define MACPHYC_TX_SEL_MASK			GENMASK(19, 19)
+#define MACPHYC_TX_SEL_ORIGIN		0x0
+#define MACPHYC_TX_SEL_DELAY		0x1
+#define MACPHYC_TX_DELAY_MASK		GENMASK(18, 12)
+#define MACPHYC_TX_DELAY_MAX		0x80
+#define MACPHYC_RX_SEL_MASK			GENMASK(11, 11)
+#define MACPHYC_RX_SEL_ORIGIN		0x0
+#define MACPHYC_RX_SEL_DELAY		0x1
+#define MACPHYC_RX_DELAY_MASK		GENMASK(10, 4)
+#define MACPHYC_RX_DELAY_MAX		0x80
+#define MACPHYC_SOFT_RST_MASK		GENMASK(3, 3)
+#define MACPHYC_PHY_INFT_MASK		GENMASK(2, 0)
+#define MACPHYC_PHY_INFT_RMII		0x4
+#define MACPHYC_PHY_INFT_RGMII		0x1
+#define MACPHYC_PHY_INFT_GMII		0x0
+#define MACPHYC_PHY_INFT_MII		0x0
+
+enum ingenic_mac_version {
+	ID_JZ4775,
+	ID_X1000,
+	ID_X1600,
+	ID_X1830,
+	ID_X2000,
+};
+
+struct ingenic_mac {
+	const struct ingenic_soc_info *soc_info;
+	struct device *dev;
+	struct regmap *regmap;
+
+	int rx_delay;
+	int tx_delay;
+};
+
+struct ingenic_soc_info {
+	enum ingenic_mac_version version;
+	u32 mask;
+
+	int (*set_mode)(struct plat_stmmacenet_data *plat_dat);
+};
+
+static int ingenic_mac_init(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+	int ret;
+
+	if (mac->soc_info->set_mode) {
+		ret = mac->soc_info->set_mode(plat_dat);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int jz4775_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+	unsigned int val;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_MII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_MII\n");
+		break;
+
+	case PHY_INTERFACE_MODE_GMII:
+		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_GMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_GMII\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RMII:
+		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RMII\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = FIELD_PREP(MACPHYC_TXCLK_SEL_MASK, MACPHYC_TXCLK_SEL_INPUT) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
+		break;
+
+	default:
+		dev_err(mac->dev, "unsupported interface %d", plat_dat->interface);
+		return -EINVAL;
+	}
+
+	/* Update MAC PHY control register */
+	return regmap_update_bits(mac->regmap, 0, mac->soc_info->mask, val);
+}
+
+static int x1000_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_RMII:
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RMII\n");
+		break;
+
+	default:
+		dev_err(mac->dev, "unsupported interface %d", plat_dat->interface);
+		return -EINVAL;
+	}
+
+	/* Update MAC PHY control register */
+	return regmap_update_bits(mac->regmap, 0, mac->soc_info->mask, 0);
+}
+
+static int x1600_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+	unsigned int val;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_RMII:
+		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RMII\n");
+		break;
+
+	default:
+		dev_err(mac->dev, "unsupported interface %d", plat_dat->interface);
+		return -EINVAL;
+	}
+
+	/* Update MAC PHY control register */
+	return regmap_update_bits(mac->regmap, 0, mac->soc_info->mask, val);
+}
+
+static int x1830_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+	unsigned int val;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_RMII:
+		val = FIELD_PREP(MACPHYC_MODE_SEL_MASK, MACPHYC_MODE_SEL_RMII) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RMII\n");
+		break;
+
+	default:
+		dev_err(mac->dev, "unsupported interface %d", plat_dat->interface);
+		return -EINVAL;
+	}
+
+	/* Update MAC PHY control register */
+	return regmap_update_bits(mac->regmap, 0, mac->soc_info->mask, val);
+}
+
+static int x2000_mac_set_mode(struct plat_stmmacenet_data *plat_dat)
+{
+	struct ingenic_mac *mac = plat_dat->bsp_priv;
+	unsigned int val;
+
+	switch (plat_dat->interface) {
+	case PHY_INTERFACE_MODE_RMII:
+		val = FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN) |
+			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RMII\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII:
+		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
+
+		if (mac->tx_delay == 0) {
+			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
+		} else {
+			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
+
+			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
+				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
+			else
+				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
+		}
+
+		if (mac->rx_delay == 0) {
+			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
+		} else {
+			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
+
+			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
+				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
+			else
+				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
+		}
+
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		val = FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN) |
+			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN) |
+			  FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII);
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_ID\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
+			  FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
+
+		if (mac->tx_delay == 0) {
+			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
+		} else {
+			val |= FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_DELAY);
+
+			if (mac->tx_delay > MACPHYC_TX_DELAY_MAX)
+				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, MACPHYC_TX_DELAY_MAX - 1);
+			else
+				val |= FIELD_PREP(MACPHYC_TX_DELAY_MASK, mac->tx_delay - 1);
+		}
+
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_RXID\n");
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		val = FIELD_PREP(MACPHYC_PHY_INFT_MASK, MACPHYC_PHY_INFT_RGMII) |
+			  FIELD_PREP(MACPHYC_TX_SEL_MASK, MACPHYC_TX_SEL_ORIGIN);
+
+		if (mac->rx_delay == 0) {
+			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_ORIGIN);
+		} else {
+			val |= FIELD_PREP(MACPHYC_RX_SEL_MASK, MACPHYC_RX_SEL_DELAY);
+
+			if (mac->rx_delay > MACPHYC_RX_DELAY_MAX)
+				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, MACPHYC_RX_DELAY_MAX - 1);
+			else
+				val |= FIELD_PREP(MACPHYC_RX_DELAY_MASK, mac->rx_delay - 1);
+		}
+
+		dev_dbg(mac->dev, "MAC PHY Control Register: PHY_INTERFACE_MODE_RGMII_TXID\n");
+		break;
+
+	default:
+		dev_err(mac->dev, "unsupported interface %d", plat_dat->interface);
+		return -EINVAL;
+	}
+
+	/* Update MAC PHY control register */
+	return regmap_update_bits(mac->regmap, 0, mac->soc_info->mask, val);
+}
+
+static int ingenic_mac_probe(struct platform_device *pdev)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct stmmac_resources stmmac_res;
+	struct ingenic_mac *mac;
+	const struct ingenic_soc_info *data;
+	int ret;
+
+	ret = stmmac_get_platform_resources(pdev, &stmmac_res);
+	if (ret)
+		return ret;
+
+	plat_dat = stmmac_probe_config_dt(pdev, stmmac_res.mac);
+	if (IS_ERR(plat_dat))
+		return PTR_ERR(plat_dat);
+
+	mac = devm_kzalloc(&pdev->dev, sizeof(*mac), GFP_KERNEL);
+	if (!mac) {
+		ret = -ENOMEM;
+		goto err_remove_config_dt;
+	}
+
+	data = of_device_get_match_data(&pdev->dev);
+	if (!data) {
+		dev_err(&pdev->dev, "no of match data provided\n");
+		ret = -EINVAL;
+		goto err_remove_config_dt;
+	}
+
+	/* Get MAC PHY control register */
+	mac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "mode-reg");
+	if (IS_ERR(mac->regmap)) {
+		dev_err(&pdev->dev, "%s: failed to get syscon regmap\n", __func__);
+		goto err_remove_config_dt;
+	}
+
+	ret = of_property_read_u32(pdev->dev.of_node, "rx-clk-delay", &mac->rx_delay);
+	if (ret)
+		mac->rx_delay = 0;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "tx-clk-delay", &mac->tx_delay);
+	if (ret)
+		mac->tx_delay = 0;
+
+	mac->soc_info = data;
+	mac->dev = &pdev->dev;
+
+	plat_dat->bsp_priv = mac;
+
+	ret = ingenic_mac_init(plat_dat);
+	if (ret)
+		goto err_remove_config_dt;
+
+	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
+	if (ret)
+		goto err_remove_config_dt;
+
+	return 0;
+
+err_remove_config_dt:
+	stmmac_remove_config_dt(pdev, plat_dat);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ingenic_mac_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	struct ingenic_mac *mac = priv->plat->bsp_priv;
+	int ret;
+
+	ret = stmmac_suspend(dev);
+
+	return ret;
+}
+
+static int ingenic_mac_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	struct ingenic_mac *mac = priv->plat->bsp_priv;
+	int ret;
+
+	ret = ingenic_mac_init(priv->plat);
+	if (ret)
+		return ret;
+
+	ret = stmmac_resume(dev);
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(ingenic_mac_pm_ops, ingenic_mac_suspend, ingenic_mac_resume);
+
+static struct ingenic_soc_info jz4775_soc_info = {
+	.version = ID_JZ4775,
+	.mask = MACPHYC_TXCLK_SEL_MASK | MACPHYC_SOFT_RST_MASK | MACPHYC_PHY_INFT_MASK,
+
+	.set_mode = jz4775_mac_set_mode,
+};
+
+static struct ingenic_soc_info x1000_soc_info = {
+	.version = ID_X1000,
+	.mask = MACPHYC_SOFT_RST_MASK,
+
+	.set_mode = x1000_mac_set_mode,
+};
+
+static struct ingenic_soc_info x1600_soc_info = {
+	.version = ID_X1600,
+	.mask = MACPHYC_SOFT_RST_MASK | MACPHYC_PHY_INFT_MASK,
+
+	.set_mode = x1600_mac_set_mode,
+};
+
+static struct ingenic_soc_info x1830_soc_info = {
+	.version = ID_X1830,
+	.mask = MACPHYC_MODE_SEL_MASK | MACPHYC_SOFT_RST_MASK | MACPHYC_PHY_INFT_MASK,
+
+	.set_mode = x1830_mac_set_mode,
+};
+
+static struct ingenic_soc_info x2000_soc_info = {
+	.version = ID_X2000,
+	.mask = MACPHYC_TX_SEL_MASK | MACPHYC_TX_DELAY_MASK | MACPHYC_RX_SEL_MASK |
+			MACPHYC_RX_DELAY_MASK | MACPHYC_SOFT_RST_MASK | MACPHYC_PHY_INFT_MASK,
+
+	.set_mode = x2000_mac_set_mode,
+};
+
+static const struct of_device_id ingenic_mac_of_matches[] = {
+	{ .compatible = "ingenic,jz4775-mac", .data = &jz4775_soc_info },
+	{ .compatible = "ingenic,x1000-mac", .data = &x1000_soc_info },
+	{ .compatible = "ingenic,x1600-mac", .data = &x1600_soc_info },
+	{ .compatible = "ingenic,x1830-mac", .data = &x1830_soc_info },
+	{ .compatible = "ingenic,x2000-mac", .data = &x2000_soc_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ingenic_mac_of_matches);
+
+static struct platform_driver ingenic_mac_driver = {
+	.probe		= ingenic_mac_probe,
+	.remove		= stmmac_pltfr_remove,
+	.driver		= {
+		.name	= "ingenic-mac",
+		.pm		= pm_ptr(&ingenic_mac_pm_ops),
+		.of_match_table = ingenic_mac_of_matches,
+	},
+};
+module_platform_driver(ingenic_mac_driver);
+
+MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
+MODULE_DESCRIPTION("Ingenic SoCs DWMAC specific glue layer");
+MODULE_LICENSE("GPL v2");