[net-next,v4,0/4] DP83TD510 Single Pair 10Mbps Ethernet PHY

Hello

The DP83TD510 is an Ethernet PHY supporting single pair of twisted wires. The
PHY is capable of 10Mbps communication over long distances and exceeds the
IEEE 802.3cg 10BASE-T1L single-pair Ethernet specification.  The PHY supports
various voltage level signalling and can be forced to support a specific
voltage or allowed to perfrom auto negotiation on the voltage level. The
default for the PHY is auto negotiation but if the PHY is forced to a specific
voltage then the LP must also support the same voltage.

Add the 10BASE-T1L linkmodes for ethtool to properly advertise the PHY's
capability.

Dan

Dan Murphy (4):
  ethtool: Add 10base-T1L link mode entries
  dt-bindings: net: Add Rx/Tx output configuration for 10base T1L
  dt-bindings: dp83td510: Add binding for DP83TD510 Ethernet PHY
  net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

 .../devicetree/bindings/net/ethernet-phy.yaml |   6 +
 .../devicetree/bindings/net/ti,dp83td510.yaml |  64 +++
 drivers/net/phy/Kconfig                       |   6 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/dp83td510.c                   | 505 ++++++++++++++++++
 drivers/net/phy/phy-core.c                    |   4 +-
 include/uapi/linux/ethtool.h                  |   2 +
 net/ethtool/common.c                          |   2 +
 net/ethtool/linkmodes.c                       |   2 +
 9 files changed, 591 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83td510.yaml
 create mode 100644 drivers/net/phy/dp83td510.c

On Tue, Nov 17, 2020 at 02:15:55PM -0600, Dan Murphy wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
> 
> The device supports both 2.4-V p2p and 1-V p2p output voltage as defined
> by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via
> the device tree or the device is defaulted to auto negotiation to
> determine the proper p2p voltage.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v4 - Considerable rework of the code after secondary test setup was created.
> This version also uses the handle_interrupt call back and reduces the
> configuration arrays as it was determined that 80% of the array was the same.
> 
>  drivers/net/phy/Kconfig     |   6 +
>  drivers/net/phy/Makefile    |   1 +
>  drivers/net/phy/dp83td510.c | 505 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 512 insertions(+)
>  create mode 100644 drivers/net/phy/dp83td510.c
> 

[snip]

> +static int dp83td510_ack_interrupt(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, DP83TD510_INT_REG1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read(phydev, DP83TD510_INT_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_trigger_machine(phydev);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t dp83td510_handle_interrupt(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = dp83td510_ack_interrupt(phydev);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}

From what I can see in the datasheet, the INT_REG1 and INT_REG2 are used
for both interrupt configuration and interrupt status.

If this is the case, the state machine should only be triggered if the
interrupt was triggered (eg DP83TD510_INT1_LINK is set), not if _any_
bit from the register is set. This is broken since anytime you have
interrupts enabled, the lower half of the register will be non-zero
since that contains you interrupt enabled bits.

The .handle_interrupt() should look something like:

	ret = phy_read(phydev, DP83TD510_INT_REG1);
	if (ret < 0)
		return ret;
	
	if (!(ret & (DP83TD510_INT1_ESD | DP83TD510_INT1_LINK | DP83TD510_INT1_RHF)))
		return IRQ_NONE;

	ret = phy_read(phydev, DP83TD510_INT_REG2);
	if (ret < 0)
		return ret;
	
	if (!(ret & (DP83TD510_INT2_POR | DP83TD510_INT2_POL | DP83TD510_INT2_PAGE)))
		return IRQ_NONE;

	phy_trigger_machine(phydev);

	return IRQ_HANDLED;

> +
> +static int dp83td510_config_intr(struct phy_device *phydev)
> +{
> +	int int_status;
> +	int gen_cfg_val;
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		int_status = phy_read(phydev, DP83TD510_INT_REG1);
> +		if (int_status < 0)
> +			return int_status;
> +
> +		int_status = (DP83TD510_INT1_ESD_EN | DP83TD510_INT1_LINK_EN |
> +			      DP83TD510_INT1_RHF_EN);
> +
> +		ret = phy_write(phydev, DP83TD510_INT_REG1, int_status);
> +		if (ret)
> +			return ret;
> +
> +		int_status = phy_read(phydev, DP83TD510_INT_REG2);
> +		if (int_status < 0)
> +			return int_status;
> +
> +		int_status = (DP83TD510_INT2_POR | DP83TD510_INT2_POL |
> +				DP83TD510_INT2_PAGE);
> +

Shouldn't you use DP83TD510_INT2_POR_EN, DP83TD510_INT2_POL_EN etc here?
It seems that you are setting up the bits corresponding with the
interrupt status and not the interrupt enable.

Ioana

> +static int dp83td510_config_init(struct phy_device *phydev)
> +{
> +	struct dp83td510_private *dp83td510 = phydev->priv;
> +	int ret = 0;
> +
> +	if (phy_interface_is_rgmii(phydev)) {

> +		if (dp83td510->rgmii_delay) {
> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
> +					       DP83TD510_MAC_CFG_1,
> +					       dp83td510->rgmii_delay);

Just to be safe, you should always write rgmii_delay, even if it is
zero. We have had too many bugs with RGMII delays which cause bad
backwards compatibility problems, so i would prefer to do a write
which might be unneeded, that find a bug here in a few years time.

> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
> +		ret = phy_modify(phydev, DP83TD510_GEN_CFG,
> +				 DP83TD510_FIFO_DEPTH_MASK,
> +				 dp83td510->tx_fifo_depth);

So there is no need to set the FIFO depth for the other three RGMII
modes? Or should this also be phy_interface_is_rgmii(phydev)?

> +#if IS_ENABLED(CONFIG_OF_MDIO)
> +static int dp83td510_of_init(struct phy_device *phydev)
> +{
> +	struct dp83td510_private *dp83td510 = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;

You need to move this assignment to later in order to keep with
reverse christmas tree.

> +#else
> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
> +	dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB

You don't have DT, so there is no fine control, but you still need to
do the basic 2ns delay as indicated by the phydev->interface value. So
i think you still need to set dp83td510->rgmii_delay depending on
which RGMII mode is requested.

      Andrew

Andrew

On 11/19/20 7:49 PM, Andrew Lunn wrote:
>> +static int dp83td510_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int ret = 0;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		if (dp83td510->rgmii_delay) {
>> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
>> +					       DP83TD510_MAC_CFG_1,
>> +					       dp83td510->rgmii_delay);
> Just to be safe, you should always write rgmii_delay, even if it is
> zero. We have had too many bugs with RGMII delays which cause bad
> backwards compatibility problems, so i would prefer to do a write
> which might be unneeded, that find a bug here in a few years time.

OK.


>
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
>> +		ret = phy_modify(phydev, DP83TD510_GEN_CFG,
>> +				 DP83TD510_FIFO_DEPTH_MASK,
>> +				 dp83td510->tx_fifo_depth);
> So there is no need to set the FIFO depth for the other three RGMII
> modes? Or should this also be phy_interface_is_rgmii(phydev)?

According to the data sheet the FIFO depth is for RMII.

"Fifo depth for RMII Tx fifo"

But I will ask the HW team for clarification.


>
>> +#if IS_ENABLED(CONFIG_OF_MDIO)
>> +static int dp83td510_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
> You need to move this assignment to later in order to keep with
> reverse christmas tree.
Well this is only used once so I will just remove the of_node declaration
>
>> +#else
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
>> +	dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
> You don't have DT, so there is no fine control, but you still need to
> do the basic 2ns delay as indicated by the phydev->interface value. So
> i think you still need to set dp83td510->rgmii_delay depending on
> which RGMII mode is requested.

The RGMII delay is fixed in the PHY.  The user can either turn it on or 
off. The default is 'off' which is 0.

I can explicitly set the rgmii_delay to 0 in non-OF cases.

Dan