Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Andrew Lunn]

> +  adi,rx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> +      default value is 0 (which represents 2 ns)
> +    enum: [ 0, 1, 2, 6, 7 ]

We want these numbers to be in ns. So the default value would actually
be 2. The driver needs to convert the number in DT to a value to poke
into a PHY register. Please rename the property adi,rx-internal-delay-ns.

> +
> +  adi,tx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> +      default value is 0 (which represents 2 ns)
> +    enum: [ 0, 1, 2, 6, 7 ]

Same here.

> +
> +  adi,fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      When operating in RMII mode, this option configures the FIFO depth.
> +      See `dt-bindings/net/adin.h`.
> +    enum: [ 0, 1, 2, 3, 4, 5 ]

Units? You should probably rename this adi,fifo-depth-bits and list
the valid values in bits.

> +
> +  adi,eee-enabled:
> +    description: |
> +      Advertise EEE capabilities on power-up/init (default disabled)
> +    type: boolean

It is not the PHY which decides this. The MAC indicates if it is EEE
capable to phylib. phylib looks into the PHY registers to determine if
the PHY supports EEE. phylib will then enable EEE
advertisement. Please remove this, and ensure EEE is disabled by
default.

	Andrew

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Andrew Lunn]

> +static int adin_config_init(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	rc = genphy_config_init(phydev);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}

Why not just

    return genphy_config_init(phydev);

    Andrew

Re: [PATCH 02/16] net: phy: adin: hook genphy_{suspend,resume} into the driver

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:39PM +0300, Alexandru Ardelean wrote:
> The chip supports standard suspend/resume via BMCR reg.
> Hook these functions into the `adin` driver.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 03/16] net: phy: adin: add support for interrupts

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:40PM +0300, Alexandru Ardelean wrote:
> This change adds support for enabling PHY interrupts that can be used by
> the PHY framework to get signal for link/speed/auto-negotiation changes.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index c100a0dd95cd..b75c723bda79 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,22 @@
>  #define PHY_ID_ADIN1200				0x0283bc20
>  #define PHY_ID_ADIN1300				0x0283bc30
>  
> +#define ADIN1300_INT_MASK_REG			0x0018
> +#define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
> +#define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> +#define   ADIN1300_INT_ANEG_PAGE_RX_EN		BIT(6)
> +#define   ADIN1300_INT_IDLE_ERR_CNT_EN		BIT(5)
> +#define   ADIN1300_INT_MAC_FIFO_OU_EN		BIT(4)
> +#define   ADIN1300_INT_RX_STAT_CHNG_EN		BIT(3)
> +#define   ADIN1300_INT_LINK_STAT_CHNG_EN	BIT(2)
> +#define   ADIN1300_INT_SPEED_CHNG_EN		BIT(1)
> +#define   ADIN1300_INT_HW_IRQ_EN		BIT(0)
> +#define ADIN1300_INT_MASK_EN	\
> +	(ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> +	 ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> +	 ADIN1300_INT_HW_IRQ_EN)
> +#define ADIN1300_INT_STATUS_REG			0x0019
> +
>  static int adin_config_init(struct phy_device *phydev)
>  {
>  	int rc;
> @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int adin_phy_ack_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Clear pending interrupts.  */
> +	ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Please go through the whole driver and throw out all the needless

	if (ret < 0)
		return ret;

	return 0;

Thanks
	Andrew

Re: [PATCH 04/16] net: phy: adin: add {write,read}_mmd hooks

[Andrew Lunn]

> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index b75c723bda79..3dd9fe50f4c8 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,9 @@
>  #define PHY_ID_ADIN1200				0x0283bc20
>  #define PHY_ID_ADIN1300				0x0283bc30
>  
> +#define ADIN1300_MII_EXT_REG_PTR		0x10
> +#define ADIN1300_MII_EXT_REG_DATA		0x11
> +
>  #define ADIN1300_INT_MASK_REG			0x0018

Please be consistent with registers. Either use 4 digits, or 2 digits.

       Andrew

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:53PM +0300, Alexandru Ardelean wrote:
> This change adds bindings for the Analog Devices ADIN PHY driver, detailing
> all the properties implemented by the driver.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  .../devicetree/bindings/net/adi,adin.yaml     | 93 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +
>  include/dt-bindings/net/adin.h                | 26 ++++++
>  3 files changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
>  create mode 100644 include/dt-bindings/net/adin.h
> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> new file mode 100644
> index 000000000000..fcf884bb86f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/adi,adin.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADIN1200/ADIN1300 PHY
> +
> +maintainers:
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  Bindings for Analog Devices Industrial Ethernet PHYs
> +
> +properties:
> +  compatible:
> +    description: |
> +      Compatible list, may contain "ethernet-phy-ieee802.3-c45" in which case
> +      Clause 45 will be used to access device management registers. If
> +      unspecified, Clause 22 will be used. Use this only when MDIO supports
> +      Clause 45 access, but there is no other way to determine this.
> +    enum:
> +      - ethernet-phy-ieee802.3-c45

It is valid to list ethernet-phy-ieee802.3-c22, it is just not
required. So maybe you should list it here to keep the DT validater happy?

	  Andrew

Re: [PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:42PM +0300, Alexandru Ardelean wrote:
> The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
> unconfigured) is RGMII.
> This change adds support for configuring these modes via the device
> registers.
> 
> For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),

It would be nice to add the missing space.

> the default delay is 2 ns. This can be configurable and will be done in
> a subsequent change.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 3dd9fe50f4c8..dbdb8f60741c 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -33,14 +33,91 @@
>  	 ADIN1300_INT_HW_IRQ_EN)
>  #define ADIN1300_INT_STATUS_REG			0x0019
>  
> +#define ADIN1300_GE_RGMII_CFG_REG		0xff23
> +#define   ADIN1300_GE_RGMII_RXID_EN		BIT(2)
> +#define   ADIN1300_GE_RGMII_TXID_EN		BIT(1)
> +#define   ADIN1300_GE_RGMII_EN			BIT(0)
> +
> +#define ADIN1300_GE_RMII_CFG_REG		0xff24
> +#define   ADIN1300_GE_RMII_EN			BIT(0)
> +
> +static int adin_config_rgmii_mode(struct phy_device *phydev,
> +				  phy_interface_t intf)
> +{
> +	int reg;
> +
> +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
> +	if (reg < 0)
> +		return reg;
> +
> +	if (!phy_interface_mode_is_rgmii(intf)) {
> +		reg &= ~ADIN1300_GE_RGMII_EN;
> +		goto write;
> +	}
> +
> +	reg |= ADIN1300_GE_RGMII_EN;
> +
> +	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    intf == PHY_INTERFACE_MODE_RGMII_RXID) {
> +		reg |= ADIN1300_GE_RGMII_RXID_EN;
> +	} else {
> +		reg &= ~ADIN1300_GE_RGMII_RXID_EN;
> +	}
> +
> +	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    intf == PHY_INTERFACE_MODE_RGMII_TXID) {
> +		reg |= ADIN1300_GE_RGMII_TXID_EN;
> +	} else {
> +		reg &= ~ADIN1300_GE_RGMII_TXID_EN;
> +	}

Nice. Often driver writers forget to clear the delay, they only set
it. Not so here.

However, is checkpatch happy with this? Each half of the if/else is a
single statement, so the {} are not needed.

> +
> +write:
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			     ADIN1300_GE_RGMII_CFG_REG, reg);
> +}
> +
> +static int adin_config_rmii_mode(struct phy_device *phydev,
> +				 phy_interface_t intf)
> +{
> +	int reg;
> +
> +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
> +	if (reg < 0)
> +		return reg;
> +
> +	if (intf != PHY_INTERFACE_MODE_RMII) {
> +		reg &= ~ADIN1300_GE_RMII_EN;
> +		goto write;

goto? Really?

> +	}
> +
> +	reg |= ADIN1300_GE_RMII_EN;
> +
> +write:
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +			     ADIN1300_GE_RMII_CFG_REG, reg);
> +}
> +
>  static int adin_config_init(struct phy_device *phydev)
>  {
> -	int rc;
> +	phy_interface_t interface, rc;

genphy_config_init() does not return a phy_interface_t!

>  
>  	rc = genphy_config_init(phydev);
>  	if (rc < 0)
>  		return rc;
>  
> +	interface = phydev->interface;
> +
> +	rc = adin_config_rgmii_mode(phydev, interface);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = adin_config_rmii_mode(phydev, interface);
> +	if (rc < 0)
> +		return rc;
> +
> +	dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
> +		 phy_modes(phydev->interface));

phydev_dbg(), or not at all.

	      Andrew

Re: [PATCH 06/16] net: phy: adin: support PHY mode converters

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> Sometimes, the connection between a MAC and PHY is done via a
> mode/interface converter. An example is a GMII-to-RGMII converter, which
> would mean that the MAC operates in GMII mode while the PHY operates in
> RGMII. In this case there is a discrepancy between what the MAC expects &
> what the PHY expects and both need to be configured in their respective
> modes.
> 
> Sometimes, this converter is specified via a board/system configuration (in
> the device-tree for example). But, other times it can be left unspecified.
> The use of these converters is common in boards that have FPGA on them.
> 
> This patch also adds support for a `adi,phy-mode-internal` property that
> can be used in these (implicit convert) cases. The internal PHY mode will
> be used to specify the correct register settings for the PHY.
> 
> `fwnode_handle` is used, since this property may be specified via ACPI as
> well in other setups, but testing has been done in DT context.

Looking at the patch, you seems to assume phy-mode is what the MAC is
using? That seems rather odd, given the name. It seems like a better
solution would be to add a mac-mode, which the MAC uses to configure
its side of the link. The MAC driver would then implement this
property.

I don't see a need for this. phy-mode indicates what the PHY should
use. End of story.

     Andrew

Re: [PATCH 11/16] net: phy: adin: PHY reset mechanisms

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:48PM +0300, Alexandru Ardelean wrote:
> The ADIN PHYs supports 4 types of reset:
> 1. The standard PHY reset via BMCR_RESET bit in MII_BMCR reg
> 2. Reset via GPIO
> 3. Reset via reg GeSftRst (0xff0c) & reload previous pin configs
> 4. Reset via reg GeSftRst (0xff0c) & request new pin configs
> 
> Resets 2 & 4 are almost identical, with the exception that the crystal
> oscillator is available during reset for 2.
> 
> Resetting via GeSftRst or via GPIO is useful when doing a warm reboot. If
> doing various settings via phytool or ethtool, the sub-system registers
> don't reset just via BMCR_RESET.
> 
> This change implements resetting the entire PHY subsystem during probe.
> During PHY HW init (phy_hw_init() logic) the PHY core regs will be reset
> again via BMCR_RESET. This will also need to happen during a PM resume.

phylib already has support for GPIO reset. So if possible, you should
not repeat that code here.

What is the difference between a GPIO reset, and a GPIO reset followed
by a subsystem soft reset?

   Andrew

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Andrew Lunn]

> +static struct phy_driver adin_driver[] = {
> +	{
> +		.phy_id		= PHY_ID_ADIN1200,
> +		.name		= "ADIN1200",
> +		.phy_id_mask	= 0xfffffff0,
> +		.features	= PHY_BASIC_FEATURES,

Do you need this? If the device implements the registers correctly,
phylib can determine this from the registers.

> +		.config_init	= adin_config_init,
> +		.config_aneg	= genphy_config_aneg,
> +		.read_status	= genphy_read_status,
> +	},
> +	{
> +		.phy_id		= PHY_ID_ADIN1300,
> +		.name		= "ADIN1300",
> +		.phy_id_mask	= 0xfffffff0,
> +		.features	= PHY_GBIT_FEATURES,

same here.

> +		.config_init	= adin_config_init,
> +		.config_aneg	= genphy_config_aneg,
> +		.read_status	= genphy_read_status,
> +	},
> +};
> +
> +module_phy_driver(adin_driver);
> +
> +static struct mdio_device_id __maybe_unused adin_tbl[] = {
> +	{ PHY_ID_ADIN1200, 0xfffffff0 },
> +	{ PHY_ID_ADIN1300, 0xfffffff0 },

PHY_ID_MATCH_VENDOR().

	Andrew

Re: [PATCH 12/16] net: phy: adin: read EEE setting from device-tree

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:49PM +0300, Alexandru Ardelean wrote:
> By default, EEE is not advertised on system init. This change allows the
> user to specify a device property to enable EEE advertisements when the PHY
> initializes.
 
This patch is not required. If EEE is not being advertised when it
should, it means there is a MAC driver bug.

	Andrew

Re: [PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:51PM +0300, Alexandru Ardelean wrote:
> Down-speed auto-negotiation may not always be enabled, in which case the
> PHY won't down-shift to 100 or 10 during auto-negotiation.

Please look at how the marvell driver enables and configures this
feature. Ideally we want all PHY drivers to use the same configuration
API for features like this.

    Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Andrew Lunn]

> +struct adin_hw_stat {
> +	const char *string;

> +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> +		memcpy(data + i * ETH_GSTRING_LEN,
> +		       adin_hw_stats[i].string, ETH_GSTRING_LEN);

You define string as a char *. So it will be only as long as it should
be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
end of the string and into whatever follows.


> +	}
> +}
> +
> +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> +				   struct adin_hw_stat *stat,
> +				   u32 *val)
> +{
> +	int ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = (ret & 0xffff);
> +
> +	if (stat->reg2 == 0)
> +		return 0;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val <<= 16;
> +	*val |= (ret & 0xffff);

Does the hardware have a snapshot feature? Is there a danger that
between the two reads stat->reg1 rolls over and you end up with too
big a value?

    Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Andrew Lunn]

On Mon, Aug 05, 2019 at 07:54:52PM +0300, Alexandru Ardelean wrote:
> This change implements retrieving all the error counters from the PHY.
> The PHY supports several error counters/stats. The `Mean Square Errors`
> status values are only valie when a link is established, and shouldn't be
> incremented. These values characterize the quality of a signal.

I think you mean accumulated, not incremented?

> 
> The rest of the error counters are self-clearing on read.
> Most of them are reports from the Frame Checker engine that the PHY has.
> 
> Not retrieving the `LPI Wake Error Count Register` here, since that is used
> by the PHY framework to check for any EEE errors. And that register is
> self-clearing when read (as per IEEE spec).
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 108 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index a1f3456a8504..04896547dac8 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
>  	{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,	ADIN1300_LPI_WAKE_ERR_CNT_REG },
>  };
>  
> +struct adin_hw_stat {
> +	const char *string;
> +	u16 reg1;
> +	u16 reg2;
> +	bool do_not_inc;

do_not_accumulate? or reverse its meaning, clear_on_read?

   Andrew

[PATCH 00/16] net: phy: adin: add support for Analog Devices PHYs

[Alexandru Ardelean]

This changeset adds support for Analog Devices Industrial Ethernet PHYs.
Particularly the PHYs this driver adds support for:
 * ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
 * ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
   Ethernet PHY

The 2 chips are pin & register compatible with one another. The main
difference being that ADIN1200 doesn't operate in gigabit mode.

The chips can be operated by the Generic PHY driver as well via the
standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
kernel as well. This assumes that configuration of the PHY has been done
completely in HW, according to spec, i.e. no extra SW configuration
required.

This changeset also implements the ability to configure the chips via SW
registers.

Datasheets:
  https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
  https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Alexandru Ardelean (16):
  net: phy: adin: add support for Analog Devices PHYs
  net: phy: adin: hook genphy_{suspend,resume} into the driver
  net: phy: adin: add support for interrupts
  net: phy: adin: add {write,read}_mmd hooks
  net: phy: adin: configure RGMII/RMII/MII modes on config
  net: phy: adin: support PHY mode converters
  net: phy: adin: make RGMII internal delays configurable
  net: phy: adin: make RMII fifo depth configurable
  net: phy: adin: add support MDI/MDIX/Auto-MDI selection
  net: phy: adin: add EEE translation layer for Clause 22
  net: phy: adin: PHY reset mechanisms
  net: phy: adin: read EEE setting from device-tree
  net: phy: adin: implement Energy Detect Powerdown mode
  net: phy: adin: make sure down-speed auto-neg is enabled
  net: phy: adin: add ethtool get_stats support
  dt-bindings: net: add bindings for ADIN PHY driver

 .../devicetree/bindings/net/adi,adin.yaml     |  93 +++
 MAINTAINERS                                   |   9 +
 drivers/net/phy/Kconfig                       |   9 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/adin.c                        | 752 ++++++++++++++++++
 include/dt-bindings/net/adin.h                |  26 +
 6 files changed, 890 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
 create mode 100644 drivers/net/phy/adin.c
 create mode 100644 include/dt-bindings/net/adin.h

-- 
2.20.1

[PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Alexandru Ardelean]

This change adds support for Analog Devices Industrial Ethernet PHYs.
Particularly the PHYs this driver adds support for:
 * ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
 * ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
   Ethernet PHY

The 2 chips are pin & register compatible with one another. The main
difference being that ADIN1200 doesn't operate in gigabit mode.

The chips can be operated by the Generic PHY driver as well via the
standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
kernel as well. This assumes that configuration of the PHY has been done
required.

Configuration can also be done via registers, which will be implemented by
the driver in the next changes.

Datasheets:
  https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
  https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 MAINTAINERS              |  7 +++++
 drivers/net/phy/Kconfig  |  9 ++++++
 drivers/net/phy/Makefile |  1 +
 drivers/net/phy/adin.c   | 59 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 drivers/net/phy/adin.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ee663e0e2f2e..faf5723610c8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -938,6 +938,13 @@ S:	Supported
 F:	drivers/mux/adgs1408.c
 F:	Documentation/devicetree/bindings/mux/adi,adgs1408.txt
 
+ANALOG DEVICES INC ADIN DRIVER
+M:	Alexandru Ardelean <alexaundru.ardelean@analog.com>
+L:	netdev@vger.kernel.org
+W:	http://ez.analog.com/community/linux-device-drivers
+S:	Supported
+F:	drivers/net/phy/adin.c
+
 ANALOG DEVICES INC ADIS DRIVER LIBRARY
 M:	Alexandru Ardelean <alexandru.ardelean@analog.com>
 S:	Supported
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 206d8650ee7f..5966d3413676 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -257,6 +257,15 @@ config SFP
 	depends on HWMON || HWMON=n
 	select MDIO_I2C
 
+config ADIN_PHY
+	tristate "Analog Devices Industrial Ethernet PHYs"
+	help
+	  Adds support for the Analog Devices Industrial Ethernet PHYs.
+	  Currently supports the:
+	  - ADIN1200 - Robust,Industrial, Low Power 10/100 Ethernet PHY
+	  - ADIN1300 - Robust,Industrial, Low Latency 10/100/1000 Gigabit
+	    Ethernet PHY
+
 config AMD_PHY
 	tristate "AMD PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index ba07c27e4208..a03437e091f3 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_SFP)		+= sfp.o
 sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
 obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
 
+obj-$(CONFIG_ADIN_PHY)		+= adin.o
 obj-$(CONFIG_AMD_PHY)		+= amd.o
 aquantia-objs			+= aquantia_main.o
 ifdef CONFIG_HWMON
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
new file mode 100644
index 000000000000..6a610d4563c3
--- /dev/null
+++ b/drivers/net/phy/adin.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0+
+/**
+ *  Driver for Analog Devices Industrial Ethernet PHYs
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+
+#define PHY_ID_ADIN1200				0x0283bc20
+#define PHY_ID_ADIN1300				0x0283bc30
+
+static int adin_config_init(struct phy_device *phydev)
+{
+	int rc;
+
+	rc = genphy_config_init(phydev);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static struct phy_driver adin_driver[] = {
+	{
+		.phy_id		= PHY_ID_ADIN1200,
+		.name		= "ADIN1200",
+		.phy_id_mask	= 0xfffffff0,
+		.features	= PHY_BASIC_FEATURES,
+		.config_init	= adin_config_init,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+	},
+	{
+		.phy_id		= PHY_ID_ADIN1300,
+		.name		= "ADIN1300",
+		.phy_id_mask	= 0xfffffff0,
+		.features	= PHY_GBIT_FEATURES,
+		.config_init	= adin_config_init,
+		.config_aneg	= genphy_config_aneg,
+		.read_status	= genphy_read_status,
+	},
+};
+
+module_phy_driver(adin_driver);
+
+static struct mdio_device_id __maybe_unused adin_tbl[] = {
+	{ PHY_ID_ADIN1200, 0xfffffff0 },
+	{ PHY_ID_ADIN1300, 0xfffffff0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, adin_tbl);
+MODULE_DESCRIPTION("Analog Devices Industrial Ethernet PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.20.1

[PATCH 02/16] net: phy: adin: hook genphy_{suspend,resume} into the driver

[Alexandru Ardelean]

The chip supports standard suspend/resume via BMCR reg.
Hook these functions into the `adin` driver.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 6a610d4563c3..c100a0dd95cd 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -34,6 +34,8 @@ static struct phy_driver adin_driver[] = {
 		.config_init	= adin_config_init,
 		.config_aneg	= genphy_config_aneg,
 		.read_status	= genphy_read_status,
+		.resume		= genphy_resume,
+		.suspend	= genphy_suspend,
 	},
 	{
 		.phy_id		= PHY_ID_ADIN1300,
@@ -43,6 +45,8 @@ static struct phy_driver adin_driver[] = {
 		.config_init	= adin_config_init,
 		.config_aneg	= genphy_config_aneg,
 		.read_status	= genphy_read_status,
+		.resume		= genphy_resume,
+		.suspend	= genphy_suspend,
 	},
 };
 
-- 
2.20.1

[PATCH 03/16] net: phy: adin: add support for interrupts

[Alexandru Ardelean]

This change adds support for enabling PHY interrupts that can be used by
the PHY framework to get signal for link/speed/auto-negotiation changes.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index c100a0dd95cd..b75c723bda79 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -14,6 +14,22 @@
 #define PHY_ID_ADIN1200				0x0283bc20
 #define PHY_ID_ADIN1300				0x0283bc30
 
+#define ADIN1300_INT_MASK_REG			0x0018
+#define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
+#define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
+#define   ADIN1300_INT_ANEG_PAGE_RX_EN		BIT(6)
+#define   ADIN1300_INT_IDLE_ERR_CNT_EN		BIT(5)
+#define   ADIN1300_INT_MAC_FIFO_OU_EN		BIT(4)
+#define   ADIN1300_INT_RX_STAT_CHNG_EN		BIT(3)
+#define   ADIN1300_INT_LINK_STAT_CHNG_EN	BIT(2)
+#define   ADIN1300_INT_SPEED_CHNG_EN		BIT(1)
+#define   ADIN1300_INT_HW_IRQ_EN		BIT(0)
+#define ADIN1300_INT_MASK_EN	\
+	(ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
+	 ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
+	 ADIN1300_INT_HW_IRQ_EN)
+#define ADIN1300_INT_STATUS_REG			0x0019
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int adin_phy_ack_intr(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Clear pending interrupts.  */
+	ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int adin_phy_config_intr(struct phy_device *phydev)
+{
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		return phy_set_bits(phydev, ADIN1300_INT_MASK_REG,
+				    ADIN1300_INT_MASK_EN);
+
+	return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG,
+			      ADIN1300_INT_MASK_EN);
+}
+
 static struct phy_driver adin_driver[] = {
 	{
 		.phy_id		= PHY_ID_ADIN1200,
 		.name		= "ADIN1200",
 		.phy_id_mask	= 0xfffffff0,
 		.features	= PHY_BASIC_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
 		.config_aneg	= genphy_config_aneg,
 		.read_status	= genphy_read_status,
+		.ack_interrupt	= adin_phy_ack_intr,
+		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
 	},
@@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = {
 		.name		= "ADIN1300",
 		.phy_id_mask	= 0xfffffff0,
 		.features	= PHY_GBIT_FEATURES,
+		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
 		.config_aneg	= genphy_config_aneg,
 		.read_status	= genphy_read_status,
+		.ack_interrupt	= adin_phy_ack_intr,
+		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
 	},
-- 
2.20.1

[PATCH 04/16] net: phy: adin: add {write,read}_mmd hooks

[Alexandru Ardelean]

Both ADIN1200 & ADIN1300 support Clause 45 access.
The Extended Management Interface (EMI) registers are accessible via both
Clause 45 (at register MDIO_MMD_VEND1) and using Clause 22.

However, the Clause 22 MMD access operations differ from the implementation
in the kernel, in the sense that it uses registers ExtRegPtr (0x10) &
ExtRegData (0x11) to access Clause 45 & EMI registers.

The indirect access is done via the following mechanism (for both R/W):
1. Write the address of the register in the ExtRegPtr
2. Read/write the value of the register (written at ExtRegPtr)

This mechanism is needed to manage configuration of chip settings and to
access EEE registers (via Clause 22).

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index b75c723bda79..3dd9fe50f4c8 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -14,6 +14,9 @@
 #define PHY_ID_ADIN1200				0x0283bc20
 #define PHY_ID_ADIN1300				0x0283bc30
 
+#define ADIN1300_MII_EXT_REG_PTR		0x10
+#define ADIN1300_MII_EXT_REG_DATA		0x11
+
 #define ADIN1300_INT_MASK_REG			0x0018
 #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
 #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
@@ -63,6 +66,45 @@ static int adin_phy_config_intr(struct phy_device *phydev)
 			      ADIN1300_INT_MASK_EN);
 }
 
+static int adin_read_mmd(struct phy_device *phydev, int devad, u16 regnum)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	int phy_addr = phydev->mdio.addr;
+	int err;
+
+	if (phydev->is_c45) {
+		u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
+
+		return __mdiobus_read(bus, phy_addr, addr);
+	}
+
+	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
+	if (err)
+		return err;
+
+	return __mdiobus_read(bus, phy_addr, ADIN1300_MII_EXT_REG_DATA);
+}
+
+static int adin_write_mmd(struct phy_device *phydev, int devad, u16 regnum,
+			  u16 val)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	int phy_addr = phydev->mdio.addr;
+	int err;
+
+	if (phydev->is_c45) {
+		u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
+
+		return __mdiobus_write(bus, phy_addr, addr, val);
+	}
+
+	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
+	if (err)
+		return err;
+
+	return __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_DATA, val);
+}
+
 static struct phy_driver adin_driver[] = {
 	{
 		.phy_id		= PHY_ID_ADIN1200,
@@ -77,6 +119,8 @@ static struct phy_driver adin_driver[] = {
 		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
+		.read_mmd	= adin_read_mmd,
+		.write_mmd	= adin_write_mmd,
 	},
 	{
 		.phy_id		= PHY_ID_ADIN1300,
@@ -91,6 +135,8 @@ static struct phy_driver adin_driver[] = {
 		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
+		.read_mmd	= adin_read_mmd,
+		.write_mmd	= adin_write_mmd,
 	},
 };
 
-- 
2.20.1

[PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config

[Alexandru Ardelean]

The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
unconfigured) is RGMII.
This change adds support for configuring these modes via the device
registers.

For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
the default delay is 2 ns. This can be configurable and will be done in
a subsequent change.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 3dd9fe50f4c8..dbdb8f60741c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -33,14 +33,91 @@
 	 ADIN1300_INT_HW_IRQ_EN)
 #define ADIN1300_INT_STATUS_REG			0x0019
 
+#define ADIN1300_GE_RGMII_CFG_REG		0xff23
+#define   ADIN1300_GE_RGMII_RXID_EN		BIT(2)
+#define   ADIN1300_GE_RGMII_TXID_EN		BIT(1)
+#define   ADIN1300_GE_RGMII_EN			BIT(0)
+
+#define ADIN1300_GE_RMII_CFG_REG		0xff24
+#define   ADIN1300_GE_RMII_EN			BIT(0)
+
+static int adin_config_rgmii_mode(struct phy_device *phydev,
+				  phy_interface_t intf)
+{
+	int reg;
+
+	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
+	if (reg < 0)
+		return reg;
+
+	if (!phy_interface_mode_is_rgmii(intf)) {
+		reg &= ~ADIN1300_GE_RGMII_EN;
+		goto write;
+	}
+
+	reg |= ADIN1300_GE_RGMII_EN;
+
+	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
+	    intf == PHY_INTERFACE_MODE_RGMII_RXID) {
+		reg |= ADIN1300_GE_RGMII_RXID_EN;
+	} else {
+		reg &= ~ADIN1300_GE_RGMII_RXID_EN;
+	}
+
+	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
+	    intf == PHY_INTERFACE_MODE_RGMII_TXID) {
+		reg |= ADIN1300_GE_RGMII_TXID_EN;
+	} else {
+		reg &= ~ADIN1300_GE_RGMII_TXID_EN;
+	}
+
+write:
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			     ADIN1300_GE_RGMII_CFG_REG, reg);
+}
+
+static int adin_config_rmii_mode(struct phy_device *phydev,
+				 phy_interface_t intf)
+{
+	int reg;
+
+	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
+	if (reg < 0)
+		return reg;
+
+	if (intf != PHY_INTERFACE_MODE_RMII) {
+		reg &= ~ADIN1300_GE_RMII_EN;
+		goto write;
+	}
+
+	reg |= ADIN1300_GE_RMII_EN;
+
+write:
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			     ADIN1300_GE_RMII_CFG_REG, reg);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
-	int rc;
+	phy_interface_t interface, rc;
 
 	rc = genphy_config_init(phydev);
 	if (rc < 0)
 		return rc;
 
+	interface = phydev->interface;
+
+	rc = adin_config_rgmii_mode(phydev, interface);
+	if (rc < 0)
+		return rc;
+
+	rc = adin_config_rmii_mode(phydev, interface);
+	if (rc < 0)
+		return rc;
+
+	dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
+		 phy_modes(phydev->interface));
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH 06/16] net: phy: adin: support PHY mode converters

[Alexandru Ardelean]

Sometimes, the connection between a MAC and PHY is done via a
mode/interface converter. An example is a GMII-to-RGMII converter, which
would mean that the MAC operates in GMII mode while the PHY operates in
RGMII. In this case there is a discrepancy between what the MAC expects &
what the PHY expects and both need to be configured in their respective
modes.

Sometimes, this converter is specified via a board/system configuration (in
the device-tree for example). But, other times it can be left unspecified.
The use of these converters is common in boards that have FPGA on them.

This patch also adds support for a `adi,phy-mode-internal` property that
can be used in these (implicit convert) cases. The internal PHY mode will
be used to specify the correct register settings for the PHY.

`fwnode_handle` is used, since this property may be specified via ACPI as
well in other setups, but testing has been done in DT context.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index dbdb8f60741c..e3d2ff8cc09c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
+#include <linux/property.h>
 
 #define PHY_ID_ADIN1200				0x0283bc20
 #define PHY_ID_ADIN1300				0x0283bc30
@@ -41,6 +42,31 @@
 #define ADIN1300_GE_RMII_CFG_REG		0xff24
 #define   ADIN1300_GE_RMII_EN			BIT(0)
 
+static int adin_get_phy_internal_mode(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	const char *pm;
+	int i;
+
+	if (device_property_read_string(dev, "adi,phy-mode-internal", &pm))
+		return phydev->interface;
+
+	/**
+	 * Getting here assumes that there is converter in-between the actual
+	 * PHY, for example a GMII-to-RGMII converter. In this case the MAC
+	 * talks GMII and PHY talks RGMII, so the PHY needs to be set in RGMII
+	 * while the MAC can work in GMII mode.
+	 */
+
+	for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
+		if (!strcasecmp(pm, phy_modes(i)))
+			return i;
+
+	dev_err(dev, "Invalid value for 'phy-mode-internal': '%s'\n", pm);
+
+	return -EINVAL;
+}
+
 static int adin_config_rgmii_mode(struct phy_device *phydev,
 				  phy_interface_t intf)
 {
@@ -105,7 +131,9 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
-	interface = phydev->interface;
+	interface = adin_get_phy_internal_mode(phydev);
+	if (interface < 0)
+		return interface;
 
 	rc = adin_config_rgmii_mode(phydev, interface);
 	if (rc < 0)
@@ -115,8 +143,13 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
-	dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
-		 phy_modes(phydev->interface));
+	if (phydev->interface == interface)
+		dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
+			 phy_modes(phydev->interface));
+	else
+		dev_info(&phydev->mdio.dev,
+			 "PHY is using mode '%s', MAC is using mode '%s'\n",
+			 phy_modes(interface), phy_modes(phydev->interface));
 
 	return 0;
 }
-- 
2.20.1

[PATCH 07/16] net: phy: adin: make RGMII internal delays configurable

[Alexandru Ardelean]

The internal delays for the RGMII are configurable for both RX & TX. This
change adds support for configuring them via device-tree (or ACPI).

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index e3d2ff8cc09c..cb96d47d457e 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -5,6 +5,7 @@
  * Copyright 2019 Analog Devices Inc.
  */
 #include <linux/kernel.h>
+#include <linux/bitfield.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -12,6 +13,8 @@
 #include <linux/phy.h>
 #include <linux/property.h>
 
+#include <dt-bindings/net/adin.h>
+
 #define PHY_ID_ADIN1200				0x0283bc20
 #define PHY_ID_ADIN1300				0x0283bc30
 
@@ -35,6 +38,12 @@
 #define ADIN1300_INT_STATUS_REG			0x0019
 
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
+#define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
+#define   ADIN1300_GE_RGMII_RX_SEL(x)		\
+		FIELD_PREP(ADIN1300_GE_RGMII_RX_MSK, x)
+#define   ADIN1300_GE_RGMII_GTX_MSK		GENMASK(5, 3)
+#define   ADIN1300_GE_RGMII_GTX_SEL(x)		\
+		FIELD_PREP(ADIN1300_GE_RGMII_GTX_MSK, x)
 #define   ADIN1300_GE_RGMII_RXID_EN		BIT(2)
 #define   ADIN1300_GE_RGMII_TXID_EN		BIT(1)
 #define   ADIN1300_GE_RGMII_EN			BIT(0)
@@ -67,6 +76,32 @@ static int adin_get_phy_internal_mode(struct phy_device *phydev)
 	return -EINVAL;
 }
 
+static void adin_config_rgmii_rx_internal_delay(struct phy_device *phydev,
+						int *reg)
+{
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+
+	if (device_property_read_u32(dev, "adi,rx-internal-delay", &val))
+		val = ADIN1300_RGMII_2_00_NS;
+
+	*reg &= ADIN1300_GE_RGMII_RX_MSK;
+	*reg |= ADIN1300_GE_RGMII_RX_SEL(val);
+}
+
+static void adin_config_rgmii_tx_internal_delay(struct phy_device *phydev,
+						int *reg)
+{
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+
+	if (device_property_read_u32(dev, "adi,tx-internal-delay", &val))
+		val = ADIN1300_RGMII_2_00_NS;
+
+	*reg &= ADIN1300_GE_RGMII_GTX_MSK;
+	*reg |= ADIN1300_GE_RGMII_GTX_SEL(val);
+}
+
 static int adin_config_rgmii_mode(struct phy_device *phydev,
 				  phy_interface_t intf)
 {
@@ -86,6 +121,7 @@ static int adin_config_rgmii_mode(struct phy_device *phydev,
 	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
 	    intf == PHY_INTERFACE_MODE_RGMII_RXID) {
 		reg |= ADIN1300_GE_RGMII_RXID_EN;
+		adin_config_rgmii_rx_internal_delay(phydev, &reg);
 	} else {
 		reg &= ~ADIN1300_GE_RGMII_RXID_EN;
 	}
@@ -93,6 +129,7 @@ static int adin_config_rgmii_mode(struct phy_device *phydev,
 	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
 	    intf == PHY_INTERFACE_MODE_RGMII_TXID) {
 		reg |= ADIN1300_GE_RGMII_TXID_EN;
+		adin_config_rgmii_tx_internal_delay(phydev, &reg);
 	} else {
 		reg &= ~ADIN1300_GE_RGMII_TXID_EN;
 	}
-- 
2.20.1

[PATCH 08/16] net: phy: adin: make RMII fifo depth configurable

[Alexandru Ardelean]

The FIFO depth can be configured for the RMII mode. This change adds
support for doing this via device-tree (or ACPI).

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index cb96d47d457e..2e27ffd403b4 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -49,6 +49,9 @@
 #define   ADIN1300_GE_RGMII_EN			BIT(0)
 
 #define ADIN1300_GE_RMII_CFG_REG		0xff24
+#define   ADIN1300_GE_RMII_FIFO_DEPTH_MSK	GENMASK(6, 4)
+#define   ADIN1300_GE_RMII_FIFO_DEPTH_SEL(x)	\
+		FIELD_PREP(ADIN1300_GE_RMII_FIFO_DEPTH_MSK, x)
 #define   ADIN1300_GE_RMII_EN			BIT(0)
 
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
@@ -142,6 +145,8 @@ static int adin_config_rgmii_mode(struct phy_device *phydev,
 static int adin_config_rmii_mode(struct phy_device *phydev,
 				 phy_interface_t intf)
 {
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
 	int reg;
 
 	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
@@ -155,6 +160,12 @@ static int adin_config_rmii_mode(struct phy_device *phydev,
 
 	reg |= ADIN1300_GE_RMII_EN;
 
+	if (device_property_read_u32(dev, "adi,fifo-depth", &val))
+		val = ADIN1300_RMII_8_BITS;
+
+	reg &= ~ADIN1300_GE_RMII_FIFO_DEPTH_MSK;
+	reg |= ADIN1300_GE_RMII_FIFO_DEPTH_SEL(val);
+
 write:
 	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
 			     ADIN1300_GE_RMII_CFG_REG, reg);
-- 
2.20.1

[PATCH 09/16] net: phy: adin: add support MDI/MDIX/Auto-MDI selection

[Alexandru Ardelean]

The ADIN PHYs support automatic MDI/MDIX negotiation. By default this is
disabled, so this is enabled at `config_init`.

This is controlled via the PHY Control 1 register.
The supported modes are:
  1. Manual MDI
  2. Manual MDIX
  3. Auto MDIX - prefer MDIX
  4. Auto MDIX - prefer MDI

The phydev mdix & mdix_ctrl fields include modes 3 & 4 into a single
auto-mode. So, the default mode this driver enables is 4 when Auto-MDI mode
is used.

When detecting MDI/MDIX mode, a combination of the PHY Control 1 register
and PHY Status 1 register is used to determine the correct MDI/MDIX mode.

If Auto-MDI mode is not set, then the manual MDI/MDIX mode is returned.
If Auto-MDI mode is set, then MDIX mode is returned differs from the
preferred MDI/MDIX mode.
This covers all cases where:
  1. MDI preferred  & Pair01Swapped   == MDIX
  2. MDIX preferred & Pair01Swapped   == MDI
  3. MDI preferred  & ! Pair01Swapped == MDIX
  4. MDIX preferred & ! Pair01Swapped == MDI

The preferred MDI/MDIX mode is not configured via SW, but can be configured
via HW pins. Note that the `Pair01Swapped` is the Green-Yellow physical
pairs.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 117 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 2e27ffd403b4..31c600b7ec66 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -21,6 +21,10 @@
 #define ADIN1300_MII_EXT_REG_PTR		0x10
 #define ADIN1300_MII_EXT_REG_DATA		0x11
 
+#define ADIN1300_PHY_CTRL1			0x0012
+#define   ADIN1300_AUTO_MDI_EN			BIT(10)
+#define   ADIN1300_MAN_MDIX_EN			BIT(9)
+
 #define ADIN1300_INT_MASK_REG			0x0018
 #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
 #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
@@ -37,6 +41,9 @@
 	 ADIN1300_INT_HW_IRQ_EN)
 #define ADIN1300_INT_STATUS_REG			0x0019
 
+#define ADIN1300_PHY_STATUS1			0x001a
+#define   ADIN1300_PAIR_01_SWAP			BIT(11)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -175,6 +182,8 @@ static int adin_config_init(struct phy_device *phydev)
 {
 	phy_interface_t interface, rc;
 
+	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+
 	rc = genphy_config_init(phydev);
 	if (rc < 0)
 		return rc;
@@ -263,6 +272,106 @@ static int adin_write_mmd(struct phy_device *phydev, int devad, u16 regnum,
 	return __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_DATA, val);
 }
 
+static int adin_config_mdix(struct phy_device *phydev)
+{
+	bool auto_en, mdix_en;
+	int reg;
+
+	mdix_en = false;
+	auto_en = false;
+	switch (phydev->mdix_ctrl) {
+	case ETH_TP_MDI:
+		break;
+	case ETH_TP_MDI_X:
+		mdix_en = true;
+		break;
+	case ETH_TP_MDI_AUTO:
+		auto_en = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	reg = phy_read(phydev, ADIN1300_PHY_CTRL1);
+	if (reg < 0)
+		return reg;
+
+	if (mdix_en)
+		reg |= ADIN1300_MAN_MDIX_EN;
+	else
+		reg &= ~ADIN1300_MAN_MDIX_EN;
+
+	if (auto_en)
+		reg |= ADIN1300_AUTO_MDI_EN;
+	else
+		reg &= ~ADIN1300_AUTO_MDI_EN;
+
+	return phy_write(phydev, ADIN1300_PHY_CTRL1, reg);
+}
+
+static int adin_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = adin_config_mdix(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_config_aneg(phydev);
+}
+
+static int adin_mdix_update(struct phy_device *phydev)
+{
+	bool auto_en, mdix_en;
+	bool swapped;
+	int reg;
+
+	reg = phy_read(phydev, ADIN1300_PHY_CTRL1);
+	if (reg < 0)
+		return reg;
+
+	auto_en = !!(reg & ADIN1300_AUTO_MDI_EN);
+	mdix_en = !!(reg & ADIN1300_MAN_MDIX_EN);
+
+	/* If MDI/MDIX is forced, just read it from the control reg */
+	if (!auto_en) {
+		if (mdix_en)
+			phydev->mdix = ETH_TP_MDI_X;
+		else
+			phydev->mdix = ETH_TP_MDI;
+		return 0;
+	}
+
+	/**
+	 * Otherwise, we need to deduce it from the PHY status2 reg.
+	 * When Auto-MDI is enabled, the ADIN1300_MAN_MDIX_EN bit implies
+	 * a preference for MDIX when it is set.
+	 */
+	reg = phy_read(phydev, ADIN1300_PHY_STATUS1);
+	if (reg < 0)
+		return reg;
+
+	swapped = !!(reg & ADIN1300_PAIR_01_SWAP);
+
+	if (mdix_en != swapped)
+		phydev->mdix = ETH_TP_MDI_X;
+	else
+		phydev->mdix = ETH_TP_MDI;
+
+	return 0;
+}
+
+static int adin_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = adin_mdix_update(phydev);
+	if (ret < 0)
+		return ret;
+
+	return genphy_read_status(phydev);
+}
+
 static struct phy_driver adin_driver[] = {
 	{
 		.phy_id		= PHY_ID_ADIN1200,
@@ -271,8 +380,8 @@ static struct phy_driver adin_driver[] = {
 		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
-		.config_aneg	= genphy_config_aneg,
-		.read_status	= genphy_read_status,
+		.config_aneg	= adin_config_aneg,
+		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
 		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
@@ -287,8 +396,8 @@ static struct phy_driver adin_driver[] = {
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
-		.config_aneg	= genphy_config_aneg,
-		.read_status	= genphy_read_status,
+		.config_aneg	= adin_config_aneg,
+		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
 		.config_intr	= adin_phy_config_intr,
 		.resume		= genphy_resume,
-- 
2.20.1

[PATCH 10/16] net: phy: adin: add EEE translation layer for Clause 22

[Alexandru Ardelean]

The ADIN1200 & ADIN1300 PHYs support EEE by using standard Clause 45 access
to access MMD registers for EEE.

The EEE register addresses (when using Clause 22) are available at
different addresses (than Clause 45), and since accessing these regs (via
Clause 22) needs a special mechanism, a translation table is required to
convert these addresses.

For Clause 45, this is not needed; the addresses are available as specified
by IEEE.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 61 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 31c600b7ec66..3c559a3ba487 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -44,6 +44,17 @@
 #define ADIN1300_PHY_STATUS1			0x001a
 #define   ADIN1300_PAIR_01_SWAP			BIT(11)
 
+/* EEE register addresses, accessible via Clause 22 access using
+ * ADIN1300_MII_EXT_REG_PTR & ADIN1300_MII_EXT_REG_DATA.
+ * The bit-fields are the same as specified by IEEE, and can be
+ * accessed via standard Clause 45 access.
+ */
+#define ADIN1300_EEE_CAP_REG			0x8000
+#define ADIN1300_EEE_ADV_REG			0x8001
+#define ADIN1300_EEE_LPABLE_REG			0x8002
+#define ADIN1300_CLOCK_STOP_REG			0x9400
+#define ADIN1300_LPI_WAKE_ERR_CNT_REG		0xa000
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -61,6 +72,20 @@
 		FIELD_PREP(ADIN1300_GE_RMII_FIFO_DEPTH_MSK, x)
 #define   ADIN1300_GE_RMII_EN			BIT(0)
 
+struct clause22_mmd_map {
+	int devad;
+	u16 cl22_regnum;
+	u16 adin_regnum;
+};
+
+static struct clause22_mmd_map clause22_mmd_map[] = {
+	{ MDIO_MMD_PCS,	MDIO_PCS_EEE_ABLE,	ADIN1300_EEE_CAP_REG },
+	{ MDIO_MMD_AN,	MDIO_AN_EEE_LPABLE,	ADIN1300_EEE_LPABLE_REG },
+	{ MDIO_MMD_AN,	MDIO_AN_EEE_ADV,	ADIN1300_EEE_ADV_REG },
+	{ MDIO_MMD_PCS,	MDIO_CTRL1,		ADIN1300_CLOCK_STOP_REG },
+	{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,	ADIN1300_LPI_WAKE_ERR_CNT_REG },
+};
+
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -233,10 +258,31 @@ static int adin_phy_config_intr(struct phy_device *phydev)
 			      ADIN1300_INT_MASK_EN);
 }
 
+static int adin_cl22_to_adin_reg(int devad, u16 cl22_regnum)
+{
+	struct clause22_mmd_map *m;
+	int i;
+
+	if (devad == MDIO_MMD_VEND1)
+		return cl22_regnum;
+
+	for (i = 0; i < ARRAY_SIZE(clause22_mmd_map); i++) {
+		m = &clause22_mmd_map[i];
+		if (m->devad == devad && m->cl22_regnum == cl22_regnum)
+			return m->adin_regnum;
+	}
+
+	pr_err("No translation available for devad: %d reg: %04x\n",
+	       devad, cl22_regnum);
+
+	return -EINVAL;
+}
+
 static int adin_read_mmd(struct phy_device *phydev, int devad, u16 regnum)
 {
 	struct mii_bus *bus = phydev->mdio.bus;
 	int phy_addr = phydev->mdio.addr;
+	int adin_regnum;
 	int err;
 
 	if (phydev->is_c45) {
@@ -245,7 +291,12 @@ static int adin_read_mmd(struct phy_device *phydev, int devad, u16 regnum)
 		return __mdiobus_read(bus, phy_addr, addr);
 	}
 
-	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
+	adin_regnum = adin_cl22_to_adin_reg(devad, regnum);
+	if (adin_regnum < 0)
+		return adin_regnum;
+
+	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR,
+			      adin_regnum);
 	if (err)
 		return err;
 
@@ -257,6 +308,7 @@ static int adin_write_mmd(struct phy_device *phydev, int devad, u16 regnum,
 {
 	struct mii_bus *bus = phydev->mdio.bus;
 	int phy_addr = phydev->mdio.addr;
+	int adin_regnum;
 	int err;
 
 	if (phydev->is_c45) {
@@ -265,7 +317,12 @@ static int adin_write_mmd(struct phy_device *phydev, int devad, u16 regnum,
 		return __mdiobus_write(bus, phy_addr, addr, val);
 	}
 
-	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR, regnum);
+	adin_regnum = adin_cl22_to_adin_reg(devad, regnum);
+	if (adin_regnum < 0)
+		return adin_regnum;
+
+	err = __mdiobus_write(bus, phy_addr, ADIN1300_MII_EXT_REG_PTR,
+			      adin_regnum);
 	if (err)
 		return err;
 
-- 
2.20.1

[PATCH 11/16] net: phy: adin: PHY reset mechanisms

[Alexandru Ardelean]

The ADIN PHYs supports 4 types of reset:
1. The standard PHY reset via BMCR_RESET bit in MII_BMCR reg
2. Reset via GPIO
3. Reset via reg GeSftRst (0xff0c) & reload previous pin configs
4. Reset via reg GeSftRst (0xff0c) & request new pin configs

Resets 2 & 4 are almost identical, with the exception that the crystal
oscillator is available during reset for 2.

Resetting via GeSftRst or via GPIO is useful when doing a warm reboot. If
doing various settings via phytool or ethtool, the sub-system registers
don't reset just via BMCR_RESET.

This change implements resetting the entire PHY subsystem during probe.
During PHY HW init (phy_hw_init() logic) the PHY core regs will be reset
again via BMCR_RESET. This will also need to happen during a PM resume.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 82 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 3c559a3ba487..476a81ce9341 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -6,12 +6,14 @@
  */
 #include <linux/kernel.h>
 #include <linux/bitfield.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/property.h>
+#include <linux/gpio/consumer.h>
 
 #include <dt-bindings/net/adin.h>
 
@@ -55,6 +57,9 @@
 #define ADIN1300_CLOCK_STOP_REG			0x9400
 #define ADIN1300_LPI_WAKE_ERR_CNT_REG		0xa000
 
+#define ADIN1300_GE_SOFT_RESET_REG		0xff0c
+#define   ADIN1300_GE_SOFT_RESET		BIT(0)
+
 #define ADIN1300_GE_RGMII_CFG_REG		0xff23
 #define   ADIN1300_GE_RGMII_RX_MSK		GENMASK(8, 6)
 #define   ADIN1300_GE_RGMII_RX_SEL(x)		\
@@ -86,6 +91,14 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
 	{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,	ADIN1300_LPI_WAKE_ERR_CNT_REG },
 };
 
+/**
+ * struct adin_priv - ADIN PHY driver private data
+ * gpiod_reset		optional reset GPIO, to be used in soft_reset() cb
+ */
+struct adin_priv {
+	struct gpio_desc	*gpiod_reset;
+};
+
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -429,6 +442,73 @@ static int adin_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
+static int adin_subsytem_soft_reset(struct phy_device *phydev)
+{
+	int reg, rc, i;
+
+	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_SOFT_RESET_REG);
+	if (reg < 0)
+		return reg;
+
+	reg |= ADIN1300_GE_SOFT_RESET;
+	rc = phy_write_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_SOFT_RESET_REG,
+			   reg);
+	if (rc < 0)
+		return rc;
+
+	for (i = 0; i < 20; i++) {
+		usleep_range(500, 1000);
+		reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
+				   ADIN1300_GE_SOFT_RESET_REG);
+		if (reg < 0 || (reg & ADIN1300_GE_SOFT_RESET))
+			continue;
+		return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int adin_reset(struct phy_device *phydev)
+{
+	struct adin_priv *priv = phydev->priv;
+	int ret;
+
+	if (priv->gpiod_reset) {
+		/* GPIO reset requires min 10 uS low,
+		 * 5 msecs max before we know that the interface is up again
+		 */
+		gpiod_set_value(priv->gpiod_reset, 0);
+		usleep_range(10, 15);
+		gpiod_set_value(priv->gpiod_reset, 1);
+		mdelay(5);
+
+		return 0;
+	}
+
+	/* Reset PHY core regs & subsystem regs */
+	return adin_subsytem_soft_reset(phydev);
+}
+
+static int adin_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct gpio_desc *gpiod_reset;
+	struct adin_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpiod_reset))
+		gpiod_reset = NULL;
+
+	priv->gpiod_reset = gpiod_reset;
+	phydev->priv = priv;
+
+	return adin_reset(phydev);
+}
+
 static struct phy_driver adin_driver[] = {
 	{
 		.phy_id		= PHY_ID_ADIN1200,
@@ -437,6 +517,7 @@ static struct phy_driver adin_driver[] = {
 		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
+		.probe		= adin_probe,
 		.config_aneg	= adin_config_aneg,
 		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
@@ -453,6 +534,7 @@ static struct phy_driver adin_driver[] = {
 		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= adin_config_init,
+		.probe		= adin_probe,
 		.config_aneg	= adin_config_aneg,
 		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
-- 
2.20.1

[PATCH 12/16] net: phy: adin: read EEE setting from device-tree

[Alexandru Ardelean]

By default, EEE is not advertised on system init. This change allows the
user to specify a device property to enable EEE advertisements when the PHY
initializes.

Also, before resetting the PHY, the EEE settings are read, so that after
the reset is complete, they are written back into the EEE advertisement
register.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 476a81ce9341..cf99ccacfeeb 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -94,9 +94,11 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
 /**
  * struct adin_priv - ADIN PHY driver private data
  * gpiod_reset		optional reset GPIO, to be used in soft_reset() cb
+ * eee_modes		EEE modes to advertise after reset
  */
 struct adin_priv {
 	struct gpio_desc	*gpiod_reset;
+	u8			eee_modes;
 };
 
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
@@ -216,6 +218,23 @@ static int adin_config_rmii_mode(struct phy_device *phydev,
 			     ADIN1300_GE_RMII_CFG_REG, reg);
 }
 
+static int adin_config_init_eee(struct phy_device *phydev)
+{
+	struct adin_priv *priv = phydev->priv;
+	int reg;
+
+	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_EEE_ADV_REG);
+	if (reg < 0)
+		return reg;
+
+	if (priv->eee_modes)
+		reg |= priv->eee_modes;
+	else
+		reg &= ~(MDIO_EEE_100TX | MDIO_EEE_1000T);
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_EEE_ADV_REG, reg);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	phy_interface_t interface, rc;
@@ -238,6 +257,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_init_eee(phydev);
+	if (rc < 0)
+		return rc;
+
 	if (phydev->interface == interface)
 		dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
 			 phy_modes(phydev->interface));
@@ -473,6 +496,12 @@ static int adin_reset(struct phy_device *phydev)
 	struct adin_priv *priv = phydev->priv;
 	int ret;
 
+	/* Update EEE settings before resetting, in case ethtool changed them */
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_EEE_ADV_REG);
+	if (ret < 0)
+		return ret;
+	priv->eee_modes = (ret & (MDIO_EEE_100TX | MDIO_EEE_1000T));
+
 	if (priv->gpiod_reset) {
 		/* GPIO reset requires min 10 uS low,
 		 * 5 msecs max before we know that the interface is up again
@@ -504,6 +533,8 @@ static int adin_probe(struct phy_device *phydev)
 		gpiod_reset = NULL;
 
 	priv->gpiod_reset = gpiod_reset;
+	if (device_property_read_bool(dev, "adi,eee-enabled"))
+		priv->eee_modes = (MDIO_EEE_100TX | MDIO_EEE_1000T);
 	phydev->priv = priv;
 
 	return adin_reset(phydev);
-- 
2.20.1

[PATCH 13/16] net: phy: adin: implement Energy Detect Powerdown mode

[Alexandru Ardelean]

The ADIN PHYs support Energy Detect Powerdown mode, which puts the PHY into
a low power mode when there is no signal on the wire (typically cable
unplugged).
This behavior is enabled by default, but can be disabled via device
property.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index cf99ccacfeeb..86848444bd98 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -27,6 +27,11 @@
 #define   ADIN1300_AUTO_MDI_EN			BIT(10)
 #define   ADIN1300_MAN_MDIX_EN			BIT(9)
 
+#define ADIN1300_PHY_CTRL_STATUS2		0x0015
+#define   ADIN1300_NRG_PD_EN			BIT(3)
+#define   ADIN1300_NRG_PD_TX_EN			BIT(2)
+#define   ADIN1300_NRG_PD_STATUS		BIT(1)
+
 #define ADIN1300_INT_MASK_REG			0x0018
 #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
 #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
@@ -95,10 +100,12 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
  * struct adin_priv - ADIN PHY driver private data
  * gpiod_reset		optional reset GPIO, to be used in soft_reset() cb
  * eee_modes		EEE modes to advertise after reset
+ * edpd_enabled		true if Energy Detect Powerdown mode is enabled
  */
 struct adin_priv {
 	struct gpio_desc	*gpiod_reset;
 	u8			eee_modes;
+	bool			edpd_enabled;
 };
 
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
@@ -235,6 +242,18 @@ static int adin_config_init_eee(struct phy_device *phydev)
 	return phy_write_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_EEE_ADV_REG, reg);
 }
 
+static int adin_config_init_edpd(struct phy_device *phydev)
+{
+	struct adin_priv *priv = phydev->priv;
+
+	if (priv->edpd_enabled)
+		return phy_set_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
+				(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
+
+	return phy_clear_bits(phydev, ADIN1300_PHY_CTRL_STATUS2,
+			(ADIN1300_NRG_PD_EN | ADIN1300_NRG_PD_TX_EN));
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	phy_interface_t interface, rc;
@@ -261,6 +280,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_init_edpd(phydev);
+	if (rc < 0)
+		return rc;
+
 	if (phydev->interface == interface)
 		dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
 			 phy_modes(phydev->interface));
@@ -535,6 +558,10 @@ static int adin_probe(struct phy_device *phydev)
 	priv->gpiod_reset = gpiod_reset;
 	if (device_property_read_bool(dev, "adi,eee-enabled"))
 		priv->eee_modes = (MDIO_EEE_100TX | MDIO_EEE_1000T);
+	if (device_property_read_bool(dev, "adi,disable-energy-detect"))
+		priv->edpd_enabled = false;
+	else
+		priv->edpd_enabled = true;
 	phydev->priv = priv;
 
 	return adin_reset(phydev);
-- 
2.20.1

[PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled

[Alexandru Ardelean]

Down-speed auto-negotiation may not always be enabled, in which case the
PHY won't down-shift to 100 or 10 during auto-negotiation.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index 86848444bd98..a1f3456a8504 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -32,6 +32,13 @@
 #define   ADIN1300_NRG_PD_TX_EN			BIT(2)
 #define   ADIN1300_NRG_PD_STATUS		BIT(1)
 
+#define ADIN1300_PHY_CTRL2			0x0016
+#define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
+#define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
+#define   ADIN1300_GROUP_MDIO_EN		BIT(6)
+#define   ADIN1300_DOWNSPEEDS_EN	\
+	(ADIN1300_DOWNSPEED_AN_100_EN | ADIN1300_DOWNSPEED_AN_10_EN)
+
 #define ADIN1300_INT_MASK_REG			0x0018
 #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
 #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
@@ -425,6 +432,22 @@ static int adin_config_mdix(struct phy_device *phydev)
 	return phy_write(phydev, ADIN1300_PHY_CTRL1, reg);
 }
 
+static int adin_config_downspeeds(struct phy_device *phydev)
+{
+	int reg;
+
+	reg = phy_read(phydev, ADIN1300_PHY_CTRL2);
+	if (reg < 0)
+		return reg;
+
+	if ((reg & ADIN1300_DOWNSPEEDS_EN) == ADIN1300_DOWNSPEEDS_EN)
+		return 0;
+
+	reg |= ADIN1300_DOWNSPEEDS_EN;
+
+	return phy_write(phydev, ADIN1300_PHY_CTRL2, reg);
+}
+
 static int adin_config_aneg(struct phy_device *phydev)
 {
 	int ret;
@@ -433,6 +456,10 @@ static int adin_config_aneg(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = adin_config_downspeeds(phydev);
+	if (ret < 0)
+		return ret;
+
 	return genphy_config_aneg(phydev);
 }
 
-- 
2.20.1

[PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Alexandru Ardelean]

This change implements retrieving all the error counters from the PHY.
The PHY supports several error counters/stats. The `Mean Square Errors`
status values are only valie when a link is established, and shouldn't be
incremented. These values characterize the quality of a signal.

The rest of the error counters are self-clearing on read.
Most of them are reports from the Frame Checker engine that the PHY has.

Not retrieving the `LPI Wake Error Count Register` here, since that is used
by the PHY framework to check for any EEE errors. And that register is
self-clearing when read (as per IEEE spec).

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/net/phy/adin.c | 108 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index a1f3456a8504..04896547dac8 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
 	{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,	ADIN1300_LPI_WAKE_ERR_CNT_REG },
 };
 
+struct adin_hw_stat {
+	const char *string;
+	u16 reg1;
+	u16 reg2;
+	bool do_not_inc;
+};
+
+/* Named just like in the datasheet */
+static struct adin_hw_stat adin_hw_stats[] = {
+	{ "RxErrCnt",		0x0014,	},
+	{ "MseA",		0x8402,	0,	true },
+	{ "MseB",		0x8403,	0,	true },
+	{ "MseC",		0x8404,	0,	true },
+	{ "MseD",		0x8405,	0,	true },
+	{ "FcFrmCnt",		0x940A, 0x940B }, /* FcFrmCntH + FcFrmCntL */
+	{ "FcLenErrCnt",	0x940C },
+	{ "FcAlgnErrCnt",	0x940D },
+	{ "FcSymbErrCnt",	0x940E },
+	{ "FcOszCnt",		0x940F },
+	{ "FcUszCnt",		0x9410 },
+	{ "FcOddCnt",		0x9411 },
+	{ "FcOddPreCnt",	0x9412 },
+	{ "FcDribbleBitsCnt",	0x9413 },
+	{ "FcFalseCarrierCnt",	0x9414 },
+};
+
 /**
  * struct adin_priv - ADIN PHY driver private data
  * gpiod_reset		optional reset GPIO, to be used in soft_reset() cb
@@ -113,6 +139,7 @@ struct adin_priv {
 	struct gpio_desc	*gpiod_reset;
 	u8			eee_modes;
 	bool			edpd_enabled;
+	u64			stats[ARRAY_SIZE(adin_hw_stats)];
 };
 
 static int adin_get_phy_internal_mode(struct phy_device *phydev)
@@ -568,6 +595,81 @@ static int adin_reset(struct phy_device *phydev)
 	return adin_subsytem_soft_reset(phydev);
 }
 
+static int adin_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(adin_hw_stats);
+}
+
+static void adin_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
+		memcpy(data + i * ETH_GSTRING_LEN,
+		       adin_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static int adin_read_mmd_stat_regs(struct phy_device *phydev,
+				   struct adin_hw_stat *stat,
+				   u32 *val)
+{
+	int ret;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
+	if (ret < 0)
+		return ret;
+
+	*val = (ret & 0xffff);
+
+	if (stat->reg2 == 0)
+		return 0;
+
+	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
+	if (ret < 0)
+		return ret;
+
+	*val <<= 16;
+	*val |= (ret & 0xffff);
+
+	return 0;
+}
+
+static u64 adin_get_stat(struct phy_device *phydev, int i)
+{
+	struct adin_hw_stat *stat = &adin_hw_stats[i];
+	struct adin_priv *priv = phydev->priv;
+	u32 val;
+	int ret;
+
+	if (stat->reg1 > 0x1f) {
+		ret = adin_read_mmd_stat_regs(phydev, stat, &val);
+		if (ret < 0)
+			return (u64)(~0);
+	} else {
+		ret = phy_read(phydev, stat->reg1);
+		if (ret < 0)
+			return (u64)(~0);
+		val = (ret & 0xffff);
+	}
+
+	if (stat->do_not_inc)
+		priv->stats[i] = val;
+	else
+		priv->stats[i] += val;
+
+	return priv->stats[i];
+}
+
+static void adin_get_stats(struct phy_device *phydev,
+			   struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++)
+		data[i] = adin_get_stat(phydev, i);
+}
+
 static int adin_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
@@ -607,6 +709,9 @@ static struct phy_driver adin_driver[] = {
 		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
 		.config_intr	= adin_phy_config_intr,
+		.get_sset_count	= adin_get_sset_count,
+		.get_strings	= adin_get_strings,
+		.get_stats	= adin_get_stats,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
 		.read_mmd	= adin_read_mmd,
@@ -624,6 +729,9 @@ static struct phy_driver adin_driver[] = {
 		.read_status	= adin_read_status,
 		.ack_interrupt	= adin_phy_ack_intr,
 		.config_intr	= adin_phy_config_intr,
+		.get_sset_count	= adin_get_sset_count,
+		.get_strings	= adin_get_strings,
+		.get_stats	= adin_get_stats,
 		.resume		= genphy_resume,
 		.suspend	= genphy_suspend,
 		.read_mmd	= adin_read_mmd,
-- 
2.20.1

[PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Alexandru Ardelean]

This change adds bindings for the Analog Devices ADIN PHY driver, detailing
all the properties implemented by the driver.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 .../devicetree/bindings/net/adi,adin.yaml     | 93 +++++++++++++++++++
 MAINTAINERS                                   |  2 +
 include/dt-bindings/net/adin.h                | 26 ++++++
 3 files changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
 create mode 100644 include/dt-bindings/net/adin.h

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
new file mode 100644
index 000000000000..fcf884bb86f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/adi,adin.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADIN1200/ADIN1300 PHY
+
+maintainers:
+  - Alexandru Ardelean <alexandru.ardelean@analog.com>
+
+description: |
+  Bindings for Analog Devices Industrial Ethernet PHYs
+
+properties:
+  compatible:
+    description: |
+      Compatible list, may contain "ethernet-phy-ieee802.3-c45" in which case
+      Clause 45 will be used to access device management registers. If
+      unspecified, Clause 22 will be used. Use this only when MDIO supports
+      Clause 45 access, but there is no other way to determine this.
+    enum:
+      - ethernet-phy-ieee802.3-c45
+
+  adi,phy-mode-internal:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: |
+      The internal mode of the PHY. This assumes that there is a PHY converter
+      in-between the MAC & PHY.
+    enum: [ "rgmii", "rgmii-id", "rgmii-txid", "rgmii-rxid", "rmii", "mii" ]
+
+  adi,rx-internal-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
+      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
+      default value is 0 (which represents 2 ns)
+    enum: [ 0, 1, 2, 6, 7 ]
+
+  adi,tx-internal-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
+      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
+      default value is 0 (which represents 2 ns)
+    enum: [ 0, 1, 2, 6, 7 ]
+
+  adi,fifo-depth:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      When operating in RMII mode, this option configures the FIFO depth.
+      See `dt-bindings/net/adin.h`.
+    enum: [ 0, 1, 2, 3, 4, 5 ]
+
+  adi,eee-enabled:
+    description: |
+      Advertise EEE capabilities on power-up/init (default disabled)
+    type: boolean
+
+  adi,disable-energy-detect:
+    description: |
+      Disables Energy Detect Powerdown Mode (default disabled, i.e energy detect
+      is enabled if this property is unspecified)
+    type: boolean
+
+  reset-gpios:
+    description: |
+      GPIO to reset the PHY
+      see Documentation/devicetree/bindings/gpio/gpio.txt.
+
+examples:
+  - |
+    ethernet-phy@0 {
+        compatible = "ethernet-phy-ieee802.3-c45";
+        reg = <0>;
+    };
+  - |
+    #include <dt-bindings/net/adin.h>
+    ethernet-phy@1 {
+        reg = <1>;
+        adi,phy-mode-internal = "rgmii-id";
+
+        adi,rx-internal-delay = <ADIN1300_RGMII_1_80_NS>;
+        adi,tx-internal-delay = <ADIN1300_RGMII_2_20_NS>;
+    };
+  - |
+    #include <dt-bindings/net/adin.h>
+    ethernet-phy@2 {
+        reg = <2>;
+        phy-mode = "rmii";
+
+        adi,fifo-depth = <ADIN1300_RMII_16_BITS>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index faf5723610c8..6ffbb266dee4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -944,6 +944,8 @@ L:	netdev@vger.kernel.org
 W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/net/phy/adin.c
+F:	include/dt-bindings/net/adin.h
+F:	Documentation/devicetree/bindings/net/adi,adin.yaml
 
 ANALOG DEVICES INC ADIS DRIVER LIBRARY
 M:	Alexandru Ardelean <alexandru.ardelean@analog.com>
diff --git a/include/dt-bindings/net/adin.h b/include/dt-bindings/net/adin.h
new file mode 100644
index 000000000000..4c3afa550c59
--- /dev/null
+++ b/include/dt-bindings/net/adin.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/**
+ * Device Tree constants for Analog Devices Industrial Ethernet PHYs
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+
+#ifndef _DT_BINDINGS_ADIN_H
+#define _DT_BINDINGS_ADIN_H
+
+/* RGMII internal delay settings for rx and tx for ADIN1300 */
+#define ADIN1300_RGMII_1_60_NS		0x1
+#define ADIN1300_RGMII_1_80_NS		0x2
+#define	ADIN1300_RGMII_2_00_NS		0x0
+#define	ADIN1300_RGMII_2_20_NS		0x6
+#define	ADIN1300_RGMII_2_40_NS		0x7
+
+/* RMII fifo depth values */
+#define ADIN1300_RMII_4_BITS		0x0
+#define ADIN1300_RMII_8_BITS		0x1
+#define ADIN1300_RMII_12_BITS		0x2
+#define ADIN1300_RMII_16_BITS		0x3
+#define ADIN1300_RMII_20_BITS		0x4
+#define ADIN1300_RMII_24_BITS		0x5
+
+#endif
-- 
2.20.1

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Heiner Kallweit]

On 05.08.2019 18:54, Alexandru Ardelean wrote:
> This change adds support for Analog Devices Industrial Ethernet PHYs.
> Particularly the PHYs this driver adds support for:
>  * ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
>  * ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
>    Ethernet PHY
> 
> The 2 chips are pin & register compatible with one another. The main
> difference being that ADIN1200 doesn't operate in gigabit mode.
> 
> The chips can be operated by the Generic PHY driver as well via the
> standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
> kernel as well. This assumes that configuration of the PHY has been done
> required.
> 
> Configuration can also be done via registers, which will be implemented by
> the driver in the next changes.
> 
> Datasheets:
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
>   https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  MAINTAINERS              |  7 +++++
>  drivers/net/phy/Kconfig  |  9 ++++++
>  drivers/net/phy/Makefile |  1 +
>  drivers/net/phy/adin.c   | 59 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+)
>  create mode 100644 drivers/net/phy/adin.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ee663e0e2f2e..faf5723610c8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -938,6 +938,13 @@ S:	Supported
>  F:	drivers/mux/adgs1408.c
>  F:	Documentation/devicetree/bindings/mux/adi,adgs1408.txt
>  
> +ANALOG DEVICES INC ADIN DRIVER
> +M:	Alexandru Ardelean <alexaundru.ardelean@analog.com>
> +L:	netdev@vger.kernel.org
> +W:	http://ez.analog.com/community/linux-device-drivers
> +S:	Supported
> +F:	drivers/net/phy/adin.c
> +
>  ANALOG DEVICES INC ADIS DRIVER LIBRARY
>  M:	Alexandru Ardelean <alexandru.ardelean@analog.com>
>  S:	Supported
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 206d8650ee7f..5966d3413676 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -257,6 +257,15 @@ config SFP
>  	depends on HWMON || HWMON=n
>  	select MDIO_I2C
>  
> +config ADIN_PHY
> +	tristate "Analog Devices Industrial Ethernet PHYs"
> +	help
> +	  Adds support for the Analog Devices Industrial Ethernet PHYs.
> +	  Currently supports the:
> +	  - ADIN1200 - Robust,Industrial, Low Power 10/100 Ethernet PHY
> +	  - ADIN1300 - Robust,Industrial, Low Latency 10/100/1000 Gigabit
> +	    Ethernet PHY
> +
>  config AMD_PHY
>  	tristate "AMD PHYs"
>  	---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index ba07c27e4208..a03437e091f3 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_SFP)		+= sfp.o
>  sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
>  obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
>  
> +obj-$(CONFIG_ADIN_PHY)		+= adin.o
>  obj-$(CONFIG_AMD_PHY)		+= amd.o
>  aquantia-objs			+= aquantia_main.o
>  ifdef CONFIG_HWMON
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> new file mode 100644
> index 000000000000..6a610d4563c3
> --- /dev/null
> +++ b/drivers/net/phy/adin.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/**
> + *  Driver for Analog Devices Industrial Ethernet PHYs
> + *
> + * Copyright 2019 Analog Devices Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mii.h>
> +#include <linux/phy.h>
> +
> +#define PHY_ID_ADIN1200				0x0283bc20
> +#define PHY_ID_ADIN1300				0x0283bc30
> +
> +static int adin_config_init(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	rc = genphy_config_init(phydev);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static struct phy_driver adin_driver[] = {
> +	{
> +		.phy_id		= PHY_ID_ADIN1200,

You could use PHY_ID_MATCH_MODEL here.

> +		.name		= "ADIN1200",
> +		.phy_id_mask	= 0xfffffff0,
> +		.features	= PHY_BASIC_FEATURES,

Setting features is deprecated, instead the get_features callback
should be implemented if the default genphy_read_abilities needs
to be extended / replaced. You say that the PHY's work with the
genphy driver, so I suppose the default feature detection is ok
in your case. Then you could simply remove setting "features".

> +		.config_init	= adin_config_init,
> +		.config_aneg	= genphy_config_aneg,
> +		.read_status	= genphy_read_status,
> +	},
> +	{
> +		.phy_id		= PHY_ID_ADIN1300,
> +		.name		= "ADIN1300",
> +		.phy_id_mask	= 0xfffffff0,
> +		.features	= PHY_GBIT_FEATURES,
> +		.config_init	= adin_config_init,
> +		.config_aneg	= genphy_config_aneg,
> +		.read_status	= genphy_read_status,
> +	},
> +};
> +
> +module_phy_driver(adin_driver);
> +
> +static struct mdio_device_id __maybe_unused adin_tbl[] = {
> +	{ PHY_ID_ADIN1200, 0xfffffff0 },
> +	{ PHY_ID_ADIN1300, 0xfffffff0 },

PHY_ID_MATCH_MODEL could be used here too.

> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, adin_tbl);
> +MODULE_DESCRIPTION("Analog Devices Industrial Ethernet PHY driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 03/16] net: phy: adin: add support for interrupts

[Heiner Kallweit]

On 05.08.2019 18:54, Alexandru Ardelean wrote:
> This change adds support for enabling PHY interrupts that can be used by
> the PHY framework to get signal for link/speed/auto-negotiation changes.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index c100a0dd95cd..b75c723bda79 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -14,6 +14,22 @@
>  #define PHY_ID_ADIN1200				0x0283bc20
>  #define PHY_ID_ADIN1300				0x0283bc30
>  
> +#define ADIN1300_INT_MASK_REG			0x0018
> +#define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
> +#define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> +#define   ADIN1300_INT_ANEG_PAGE_RX_EN		BIT(6)
> +#define   ADIN1300_INT_IDLE_ERR_CNT_EN		BIT(5)
> +#define   ADIN1300_INT_MAC_FIFO_OU_EN		BIT(4)
> +#define   ADIN1300_INT_RX_STAT_CHNG_EN		BIT(3)
> +#define   ADIN1300_INT_LINK_STAT_CHNG_EN	BIT(2)
> +#define   ADIN1300_INT_SPEED_CHNG_EN		BIT(1)
> +#define   ADIN1300_INT_HW_IRQ_EN		BIT(0)
> +#define ADIN1300_INT_MASK_EN	\
> +	(ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> +	 ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> +	 ADIN1300_INT_HW_IRQ_EN)
> +#define ADIN1300_INT_STATUS_REG			0x0019
> +
>  static int adin_config_init(struct phy_device *phydev)
>  {
>  	int rc;
> @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int adin_phy_ack_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Clear pending interrupts.  */
> +	ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int adin_phy_config_intr(struct phy_device *phydev)
> +{
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		return phy_set_bits(phydev, ADIN1300_INT_MASK_REG,
> +				    ADIN1300_INT_MASK_EN);
> +
> +	return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG,
> +			      ADIN1300_INT_MASK_EN);
> +}
> +
>  static struct phy_driver adin_driver[] = {
>  	{
>  		.phy_id		= PHY_ID_ADIN1200,
>  		.name		= "ADIN1200",
>  		.phy_id_mask	= 0xfffffff0,
>  		.features	= PHY_BASIC_FEATURES,
> +		.flags		= PHY_HAS_INTERRUPT,

This flag doesn't exist any longer. This indicates that you
develop against an older kernel version. Please develop
against net-next. Check up-to-date drivers like the one
for Realtek PHY's for hints.

>  		.config_init	= adin_config_init,
>  		.config_aneg	= genphy_config_aneg,
>  		.read_status	= genphy_read_status,
> +		.ack_interrupt	= adin_phy_ack_intr,
> +		.config_intr	= adin_phy_config_intr,
>  		.resume		= genphy_resume,
>  		.suspend	= genphy_suspend,
>  	},
> @@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = {
>  		.name		= "ADIN1300",
>  		.phy_id_mask	= 0xfffffff0,
>  		.features	= PHY_GBIT_FEATURES,
> +		.flags		= PHY_HAS_INTERRUPT,
>  		.config_init	= adin_config_init,
>  		.config_aneg	= genphy_config_aneg,
>  		.read_status	= genphy_read_status,
> +		.ack_interrupt	= adin_phy_ack_intr,
> +		.config_intr	= adin_phy_config_intr,
>  		.resume		= genphy_resume,
>  		.suspend	= genphy_suspend,
>  	},
> 

Re: [PATCH 10/16] net: phy: adin: add EEE translation layer for Clause 22

[Andrew Lunn]

> +static int adin_cl22_to_adin_reg(int devad, u16 cl22_regnum)
> +{
> +	struct clause22_mmd_map *m;
> +	int i;
> +
> +	if (devad == MDIO_MMD_VEND1)
> +		return cl22_regnum;
> +
> +	for (i = 0; i < ARRAY_SIZE(clause22_mmd_map); i++) {
> +		m = &clause22_mmd_map[i];
> +		if (m->devad == devad && m->cl22_regnum == cl22_regnum)
> +			return m->adin_regnum;
> +	}
> +
> +	pr_err("No translation available for devad: %d reg: %04x\n",
> +	       devad, cl22_regnum);

phydev_err(). 

	      Andrew

Re: [PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled

[Heiner Kallweit]

On 05.08.2019 18:54, Alexandru Ardelean wrote:
> Down-speed auto-negotiation may not always be enabled, in which case the
> PHY won't down-shift to 100 or 10 during auto-negotiation.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/net/phy/adin.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 86848444bd98..a1f3456a8504 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -32,6 +32,13 @@
>  #define   ADIN1300_NRG_PD_TX_EN			BIT(2)
>  #define   ADIN1300_NRG_PD_STATUS		BIT(1)
>  
> +#define ADIN1300_PHY_CTRL2			0x0016
> +#define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
> +#define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
> +#define   ADIN1300_GROUP_MDIO_EN		BIT(6)
> +#define   ADIN1300_DOWNSPEEDS_EN	\
> +	(ADIN1300_DOWNSPEED_AN_100_EN | ADIN1300_DOWNSPEED_AN_10_EN)
> +
>  #define ADIN1300_INT_MASK_REG			0x0018
>  #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
>  #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> @@ -425,6 +432,22 @@ static int adin_config_mdix(struct phy_device *phydev)
>  	return phy_write(phydev, ADIN1300_PHY_CTRL1, reg);
>  }
>  
> +static int adin_config_downspeeds(struct phy_device *phydev)
> +{
> +	int reg;
> +
> +	reg = phy_read(phydev, ADIN1300_PHY_CTRL2);
> +	if (reg < 0)
> +		return reg;
> +
> +	if ((reg & ADIN1300_DOWNSPEEDS_EN) == ADIN1300_DOWNSPEEDS_EN)
> +		return 0;
> +
> +	reg |= ADIN1300_DOWNSPEEDS_EN;
> +
> +	return phy_write(phydev, ADIN1300_PHY_CTRL2, reg);

Using phy_set_bits() would be easier.

> +}
> +
>  static int adin_config_aneg(struct phy_device *phydev)
>  {
>  	int ret;
> @@ -433,6 +456,10 @@ static int adin_config_aneg(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> +	ret = adin_config_downspeeds(phydev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return genphy_config_aneg(phydev);
>  }
>  
> 

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:16 +0200, Andrew Lunn wrote:
> [External]
> 
> > +static int adin_config_init(struct phy_device *phydev)
> > +{
> > +	int rc;
> > +
> > +	rc = genphy_config_init(phydev);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	return 0;
> > +}
> 
> Why not just
> 
>     return genphy_config_init(phydev);

Because stuff will get added after this return statement in the next patches.
I thought maybe this would be a good idea to keep the git changes minimal, but I can do a direct return and update it in
the next patches when needed.

> 
>     Andrew
> 

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:17 +0200, Andrew Lunn wrote:
> [External]
> 
> > +static struct phy_driver adin_driver[] = {
> > +	{
> > +		.phy_id		= PHY_ID_ADIN1200,
> > +		.name		= "ADIN1200",
> > +		.phy_id_mask	= 0xfffffff0,
> > +		.features	= PHY_BASIC_FEATURES,
> 
> Do you need this? If the device implements the registers correctly,
> phylib can determine this from the registers.

ack;
will take a look;

> 
> > +		.config_init	= adin_config_init,
> > +		.config_aneg	= genphy_config_aneg,
> > +		.read_status	= genphy_read_status,
> > +	},
> > +	{
> > +		.phy_id		= PHY_ID_ADIN1300,
> > +		.name		= "ADIN1300",
> > +		.phy_id_mask	= 0xfffffff0,
> > +		.features	= PHY_GBIT_FEATURES,
> 
> same here.

ack;

> 
> > +		.config_init	= adin_config_init,
> > +		.config_aneg	= genphy_config_aneg,
> > +		.read_status	= genphy_read_status,
> > +	},
> > +};
> > +
> > +module_phy_driver(adin_driver);
> > +
> > +static struct mdio_device_id __maybe_unused adin_tbl[] = {
> > +	{ PHY_ID_ADIN1200, 0xfffffff0 },
> > +	{ PHY_ID_ADIN1300, 0xfffffff0 },
> 
> PHY_ID_MATCH_VENDOR().

ack;

> 
> 	Andrew

Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 22:54 +0200, Heiner Kallweit wrote:
> [External]
> 
> On 05.08.2019 18:54, Alexandru Ardelean wrote:
> > This change adds support for Analog Devices Industrial Ethernet PHYs.
> > Particularly the PHYs this driver adds support for:
> >  * ADIN1200 - Robust, Industrial, Low Power 10/100 Ethernet PHY
> >  * ADIN1300 - Robust, Industrial, Low Latency 10/100/1000 Gigabit
> >    Ethernet PHY
> > 
> > The 2 chips are pin & register compatible with one another. The main
> > difference being that ADIN1200 doesn't operate in gigabit mode.
> > 
> > The chips can be operated by the Generic PHY driver as well via the
> > standard IEEE PHY registers (0x0000 - 0x000F) which are supported by the
> > kernel as well. This assumes that configuration of the PHY has been done
> > required.
> > 
> > Configuration can also be done via registers, which will be implemented by
> > the driver in the next changes.
> > 
> > Datasheets:
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
> >   https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1200.pdf
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  MAINTAINERS              |  7 +++++
> >  drivers/net/phy/Kconfig  |  9 ++++++
> >  drivers/net/phy/Makefile |  1 +
> >  drivers/net/phy/adin.c   | 59 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 76 insertions(+)
> >  create mode 100644 drivers/net/phy/adin.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ee663e0e2f2e..faf5723610c8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -938,6 +938,13 @@ S:	Supported
> >  F:	drivers/mux/adgs1408.c
> >  F:	Documentation/devicetree/bindings/mux/adi,adgs1408.txt
> >  
> > +ANALOG DEVICES INC ADIN DRIVER
> > +M:	Alexandru Ardelean <alexaundru.ardelean@analog.com>
> > +L:	netdev@vger.kernel.org
> > +W:	http://ez.analog.com/community/linux-device-drivers
> > +S:	Supported
> > +F:	drivers/net/phy/adin.c
> > +
> >  ANALOG DEVICES INC ADIS DRIVER LIBRARY
> >  M:	Alexandru Ardelean <alexandru.ardelean@analog.com>
> >  S:	Supported
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 206d8650ee7f..5966d3413676 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -257,6 +257,15 @@ config SFP
> >  	depends on HWMON || HWMON=n
> >  	select MDIO_I2C
> >  
> > +config ADIN_PHY
> > +	tristate "Analog Devices Industrial Ethernet PHYs"
> > +	help
> > +	  Adds support for the Analog Devices Industrial Ethernet PHYs.
> > +	  Currently supports the:
> > +	  - ADIN1200 - Robust,Industrial, Low Power 10/100 Ethernet PHY
> > +	  - ADIN1300 - Robust,Industrial, Low Latency 10/100/1000 Gigabit
> > +	    Ethernet PHY
> > +
> >  config AMD_PHY
> >  	tristate "AMD PHYs"
> >  	---help---
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index ba07c27e4208..a03437e091f3 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_SFP)		+= sfp.o
> >  sfp-obj-$(CONFIG_SFP)		+= sfp-bus.o
> >  obj-y				+= $(sfp-obj-y) $(sfp-obj-m)
> >  
> > +obj-$(CONFIG_ADIN_PHY)		+= adin.o
> >  obj-$(CONFIG_AMD_PHY)		+= amd.o
> >  aquantia-objs			+= aquantia_main.o
> >  ifdef CONFIG_HWMON
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > new file mode 100644
> > index 000000000000..6a610d4563c3
> > --- /dev/null
> > +++ b/drivers/net/phy/adin.c
> > @@ -0,0 +1,59 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/**
> > + *  Driver for Analog Devices Industrial Ethernet PHYs
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/mii.h>
> > +#include <linux/phy.h>
> > +
> > +#define PHY_ID_ADIN1200				0x0283bc20
> > +#define PHY_ID_ADIN1300				0x0283bc30
> > +
> > +static int adin_config_init(struct phy_device *phydev)
> > +{
> > +	int rc;
> > +
> > +	rc = genphy_config_init(phydev);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct phy_driver adin_driver[] = {
> > +	{
> > +		.phy_id		= PHY_ID_ADIN1200,
> 
> You could use PHY_ID_MATCH_MODEL here.
> 
> > +		.name		= "ADIN1200",
> > +		.phy_id_mask	= 0xfffffff0,
> > +		.features	= PHY_BASIC_FEATURES,
> 
> Setting features is deprecated, instead the get_features callback
> should be implemented if the default genphy_read_abilities needs
> to be extended / replaced. You say that the PHY's work with the
> genphy driver, so I suppose the default feature detection is ok
> in your case. Then you could simply remove setting "features".

ack;
thanks for the info

> 
> > +		.config_init	= adin_config_init,
> > +		.config_aneg	= genphy_config_aneg,
> > +		.read_status	= genphy_read_status,
> > +	},
> > +	{
> > +		.phy_id		= PHY_ID_ADIN1300,
> > +		.name		= "ADIN1300",
> > +		.phy_id_mask	= 0xfffffff0,
> > +		.features	= PHY_GBIT_FEATURES,
> > +		.config_init	= adin_config_init,
> > +		.config_aneg	= genphy_config_aneg,
> > +		.read_status	= genphy_read_status,
> > +	},
> > +};
> > +
> > +module_phy_driver(adin_driver);
> > +
> > +static struct mdio_device_id __maybe_unused adin_tbl[] = {
> > +	{ PHY_ID_ADIN1200, 0xfffffff0 },
> > +	{ PHY_ID_ADIN1300, 0xfffffff0 },
> 
> PHY_ID_MATCH_MODEL could be used here too.

ack;
will take a look

> 
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(mdio, adin_tbl);
> > +MODULE_DESCRIPTION("Analog Devices Industrial Ethernet PHY driver");
> > +MODULE_LICENSE("GPL");
> > 

Re: [PATCH 03/16] net: phy: adin: add support for interrupts

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:21 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:40PM +0300, Alexandru Ardelean wrote:
> > This change adds support for enabling PHY interrupts that can be used by
> > the PHY framework to get signal for link/speed/auto-negotiation changes.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index c100a0dd95cd..b75c723bda79 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -14,6 +14,22 @@
> >  #define PHY_ID_ADIN1200				0x0283bc20
> >  #define PHY_ID_ADIN1300				0x0283bc30
> >  
> > +#define ADIN1300_INT_MASK_REG			0x0018
> > +#define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
> > +#define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> > +#define   ADIN1300_INT_ANEG_PAGE_RX_EN		BIT(6)
> > +#define   ADIN1300_INT_IDLE_ERR_CNT_EN		BIT(5)
> > +#define   ADIN1300_INT_MAC_FIFO_OU_EN		BIT(4)
> > +#define   ADIN1300_INT_RX_STAT_CHNG_EN		BIT(3)
> > +#define   ADIN1300_INT_LINK_STAT_CHNG_EN	BIT(2)
> > +#define   ADIN1300_INT_SPEED_CHNG_EN		BIT(1)
> > +#define   ADIN1300_INT_HW_IRQ_EN		BIT(0)
> > +#define ADIN1300_INT_MASK_EN	\
> > +	(ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> > +	 ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> > +	 ADIN1300_INT_HW_IRQ_EN)
> > +#define ADIN1300_INT_STATUS_REG			0x0019
> > +
> >  static int adin_config_init(struct phy_device *phydev)
> >  {
> >  	int rc;
> > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static int adin_phy_ack_intr(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	/* Clear pending interrupts.  */
> > +	ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> 
> Please go through the whole driver and throw out all the needless

ack;
i'll re-visit;

> 
> 	if (ret < 0)
> 		return ret;
> 
> 	return 0;
> 
> Thanks
> 	Andrew

Re: [PATCH 03/16] net: phy: adin: add support for interrupts

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 23:02 +0200, Heiner Kallweit wrote:
> [External]
> 
> On 05.08.2019 18:54, Alexandru Ardelean wrote:
> > This change adds support for enabling PHY interrupts that can be used by
> > the PHY framework to get signal for link/speed/auto-negotiation changes.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index c100a0dd95cd..b75c723bda79 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -14,6 +14,22 @@
> >  #define PHY_ID_ADIN1200				0x0283bc20
> >  #define PHY_ID_ADIN1300				0x0283bc30
> >  
> > +#define ADIN1300_INT_MASK_REG			0x0018
> > +#define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
> > +#define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> > +#define   ADIN1300_INT_ANEG_PAGE_RX_EN		BIT(6)
> > +#define   ADIN1300_INT_IDLE_ERR_CNT_EN		BIT(5)
> > +#define   ADIN1300_INT_MAC_FIFO_OU_EN		BIT(4)
> > +#define   ADIN1300_INT_RX_STAT_CHNG_EN		BIT(3)
> > +#define   ADIN1300_INT_LINK_STAT_CHNG_EN	BIT(2)
> > +#define   ADIN1300_INT_SPEED_CHNG_EN		BIT(1)
> > +#define   ADIN1300_INT_HW_IRQ_EN		BIT(0)
> > +#define ADIN1300_INT_MASK_EN	\
> > +	(ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \
> > +	 ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \
> > +	 ADIN1300_INT_HW_IRQ_EN)
> > +#define ADIN1300_INT_STATUS_REG			0x0019
> > +
> >  static int adin_config_init(struct phy_device *phydev)
> >  {
> >  	int rc;
> > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev)
> >  	return 0;
> >  }
> >  
> > +static int adin_phy_ack_intr(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +
> > +	/* Clear pending interrupts.  */
> > +	ret = phy_read(phydev, ADIN1300_INT_STATUS_REG);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int adin_phy_config_intr(struct phy_device *phydev)
> > +{
> > +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> > +		return phy_set_bits(phydev, ADIN1300_INT_MASK_REG,
> > +				    ADIN1300_INT_MASK_EN);
> > +
> > +	return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG,
> > +			      ADIN1300_INT_MASK_EN);
> > +}
> > +
> >  static struct phy_driver adin_driver[] = {
> >  	{
> >  		.phy_id		= PHY_ID_ADIN1200,
> >  		.name		= "ADIN1200",
> >  		.phy_id_mask	= 0xfffffff0,
> >  		.features	= PHY_BASIC_FEATURES,
> > +		.flags		= PHY_HAS_INTERRUPT,
> 
> This flag doesn't exist any longer. This indicates that you
> develop against an older kernel version. Please develop
> against net-next. Check up-to-date drivers like the one
> for Realtek PHY's for hints.

ack;

> 
> >  		.config_init	= adin_config_init,
> >  		.config_aneg	= genphy_config_aneg,
> >  		.read_status	= genphy_read_status,
> > +		.ack_interrupt	= adin_phy_ack_intr,
> > +		.config_intr	= adin_phy_config_intr,
> >  		.resume		= genphy_resume,
> >  		.suspend	= genphy_suspend,
> >  	},
> > @@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = {
> >  		.name		= "ADIN1300",
> >  		.phy_id_mask	= 0xfffffff0,
> >  		.features	= PHY_GBIT_FEATURES,
> > +		.flags		= PHY_HAS_INTERRUPT,
> >  		.config_init	= adin_config_init,
> >  		.config_aneg	= genphy_config_aneg,
> >  		.read_status	= genphy_read_status,
> > +		.ack_interrupt	= adin_phy_ack_intr,
> > +		.config_intr	= adin_phy_config_intr,
> >  		.resume		= genphy_resume,
> >  		.suspend	= genphy_suspend,
> >  	},
> > 

Re: [PATCH 04/16] net: phy: adin: add {write,read}_mmd hooks

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:25 +0200, Andrew Lunn wrote:
> [External]
> 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index b75c723bda79..3dd9fe50f4c8 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -14,6 +14,9 @@
> >  #define PHY_ID_ADIN1200				0x0283bc20
> >  #define PHY_ID_ADIN1300				0x0283bc30
> >  
> > +#define ADIN1300_MII_EXT_REG_PTR		0x10
> > +#define ADIN1300_MII_EXT_REG_DATA		0x11
> > +
> >  #define ADIN1300_INT_MASK_REG			0x0018
> 
> Please be consistent with registers. Either use 4 digits, or 2 digits.

ack;

> 
>        Andrew

Re: [PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:39 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:42PM +0300, Alexandru Ardelean wrote:
> > The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
> > unconfigured) is RGMII.
> > This change adds support for configuring these modes via the device
> > registers.
> > 
> > For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
> 
> It would be nice to add the missing space.
> 
> > the default delay is 2 ns. This can be configurable and will be done in
> > a subsequent change.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 78 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index 3dd9fe50f4c8..dbdb8f60741c 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -33,14 +33,91 @@
> >  	 ADIN1300_INT_HW_IRQ_EN)
> >  #define ADIN1300_INT_STATUS_REG			0x0019
> >  
> > +#define ADIN1300_GE_RGMII_CFG_REG		0xff23
> > +#define   ADIN1300_GE_RGMII_RXID_EN		BIT(2)
> > +#define   ADIN1300_GE_RGMII_TXID_EN		BIT(1)
> > +#define   ADIN1300_GE_RGMII_EN			BIT(0)
> > +
> > +#define ADIN1300_GE_RMII_CFG_REG		0xff24
> > +#define   ADIN1300_GE_RMII_EN			BIT(0)
> > +
> > +static int adin_config_rgmii_mode(struct phy_device *phydev,
> > +				  phy_interface_t intf)
> > +{
> > +	int reg;
> > +
> > +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
> > +	if (reg < 0)
> > +		return reg;
> > +
> > +	if (!phy_interface_mode_is_rgmii(intf)) {
> > +		reg &= ~ADIN1300_GE_RGMII_EN;
> > +		goto write;
> > +	}
> > +
> > +	reg |= ADIN1300_GE_RGMII_EN;
> > +
> > +	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    intf == PHY_INTERFACE_MODE_RGMII_RXID) {
> > +		reg |= ADIN1300_GE_RGMII_RXID_EN;
> > +	} else {
> > +		reg &= ~ADIN1300_GE_RGMII_RXID_EN;
> > +	}
> > +
> > +	if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> > +	    intf == PHY_INTERFACE_MODE_RGMII_TXID) {
> > +		reg |= ADIN1300_GE_RGMII_TXID_EN;
> > +	} else {
> > +		reg &= ~ADIN1300_GE_RGMII_TXID_EN;
> > +	}
> 
> Nice. Often driver writers forget to clear the delay, they only set
> it. Not so here.
> 
> However, is checkpatch happy with this? Each half of the if/else is a
> single statement, so the {} are not needed.

it did not complain;
this whole series is checkpatch friendly [with the version of checkpatch in net-next]
i think it complained about un-balanced if-block; something like:

```
if () {

} else
  single-statement
```

but checkpatch is also a moving target;
so ¯\_(ツ)_/¯

> 
> > +
> > +write:
> > +	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +			     ADIN1300_GE_RGMII_CFG_REG, reg);
> > +}
> > +
> > +static int adin_config_rmii_mode(struct phy_device *phydev,
> > +				 phy_interface_t intf)
> > +{
> > +	int reg;
> > +
> > +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
> > +	if (reg < 0)
> > +		return reg;
> > +
> > +	if (intf != PHY_INTERFACE_MODE_RMII) {
> > +		reg &= ~ADIN1300_GE_RMII_EN;
> > +		goto write;
> 
> goto? Really?

yep;
personally, i used to not like it all that much up until a few years, but sometimes it feels it can help with creating
cleaner patches in certain contexts;

i'll re-spin without it;

> 
> > +	}
> > +
> > +	reg |= ADIN1300_GE_RMII_EN;
> > +
> > +write:
> > +	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +			     ADIN1300_GE_RMII_CFG_REG, reg);
> > +}
> > +
> >  static int adin_config_init(struct phy_device *phydev)
> >  {
> > -	int rc;
> > +	phy_interface_t interface, rc;
> 
> genphy_config_init() does not return a phy_interface_t!

good point;
will check;

> 
> >  
> >  	rc = genphy_config_init(phydev);
> >  	if (rc < 0)
> >  		return rc;
> >  
> > +	interface = phydev->interface;
> > +
> > +	rc = adin_config_rgmii_mode(phydev, interface);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	rc = adin_config_rmii_mode(phydev, interface);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
> > +		 phy_modes(phydev->interface));
> 
> phydev_dbg(), or not at all.

ack

> 
> 	      Andrew

Re: [PATCH 06/16] net: phy: adin: support PHY mode converters

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> > Sometimes, the connection between a MAC and PHY is done via a
> > mode/interface converter. An example is a GMII-to-RGMII converter, which
> > would mean that the MAC operates in GMII mode while the PHY operates in
> > RGMII. In this case there is a discrepancy between what the MAC expects &
> > what the PHY expects and both need to be configured in their respective
> > modes.
> > 
> > Sometimes, this converter is specified via a board/system configuration (in
> > the device-tree for example). But, other times it can be left unspecified.
> > The use of these converters is common in boards that have FPGA on them.
> > 
> > This patch also adds support for a `adi,phy-mode-internal` property that
> > can be used in these (implicit convert) cases. The internal PHY mode will
> > be used to specify the correct register settings for the PHY.
> > 
> > `fwnode_handle` is used, since this property may be specified via ACPI as
> > well in other setups, but testing has been done in DT context.
> 
> Looking at the patch, you seems to assume phy-mode is what the MAC is
> using? That seems rather odd, given the name. It seems like a better
> solution would be to add a mac-mode, which the MAC uses to configure
> its side of the link. The MAC driver would then implement this
> property.
> 

actually, that's a pretty good idea;
i guess i was narrow-minded when writing the driver, and got stuck on phy specifics, and forgot about the MAC-side;
[ i also catch these design elements when reviewing, but i also seem to miss them when writing stuff sometimes ]

thanks

> I don't see a need for this. phy-mode indicates what the PHY should
> use. End of story.
> 
>      Andrew

Re: [PATCH 10/16] net: phy: adin: add EEE translation layer for Clause 22

[Ardelean, Alexandru]

On Tue, 2019-08-06 at 00:11 +0200, Andrew Lunn wrote:
> [External]
> 
> > +static int adin_cl22_to_adin_reg(int devad, u16 cl22_regnum)
> > +{
> > +	struct clause22_mmd_map *m;
> > +	int i;
> > +
> > +	if (devad == MDIO_MMD_VEND1)
> > +		return cl22_regnum;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(clause22_mmd_map); i++) {
> > +		m = &clause22_mmd_map[i];
> > +		if (m->devad == devad && m->cl22_regnum == cl22_regnum)
> > +			return m->adin_regnum;
> > +	}
> > +
> > +	pr_err("No translation available for devad: %d reg: %04x\n",
> > +	       devad, cl22_regnum);
> 
> phydev_err(). 

ack

> 
> 	      Andrew

Re: [PATCH 11/16] net: phy: adin: PHY reset mechanisms

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:15 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:48PM +0300, Alexandru Ardelean wrote:
> > The ADIN PHYs supports 4 types of reset:
> > 1. The standard PHY reset via BMCR_RESET bit in MII_BMCR reg
> > 2. Reset via GPIO
> > 3. Reset via reg GeSftRst (0xff0c) & reload previous pin configs
> > 4. Reset via reg GeSftRst (0xff0c) & request new pin configs
> > 
> > Resets 2 & 4 are almost identical, with the exception that the crystal
> > oscillator is available during reset for 2.
> > 
> > Resetting via GeSftRst or via GPIO is useful when doing a warm reboot. If
> > doing various settings via phytool or ethtool, the sub-system registers
> > don't reset just via BMCR_RESET.
> > 
> > This change implements resetting the entire PHY subsystem during probe.
> > During PHY HW init (phy_hw_init() logic) the PHY core regs will be reset
> > again via BMCR_RESET. This will also need to happen during a PM resume.
> 
> phylib already has support for GPIO reset. So if possible, you should
> not repeat that code here.
> 
> What is the difference between a GPIO reset, and a GPIO reset followed
> by a subsystem soft reset?

there shouldn't be any difference;
it's just 2 consecutive resets;
i'll take a closer look at phylib's GPIO reset and see

> 
>    Andrew

Re: [PATCH 12/16] net: phy: adin: read EEE setting from device-tree

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:19 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:49PM +0300, Alexandru Ardelean wrote:
> > By default, EEE is not advertised on system init. This change allows the
> > user to specify a device property to enable EEE advertisements when the PHY
> > initializes.
>  
> This patch is not required. If EEE is not being advertised when it
> should, it means there is a MAC driver bug.

ack;
same thing about PHY specifics ignoring MAC specifics

thanks

> 
> 	Andrew

Re: [PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:22 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:51PM +0300, Alexandru Ardelean wrote:
> > Down-speed auto-negotiation may not always be enabled, in which case the
> > PHY won't down-shift to 100 or 10 during auto-negotiation.
> 
> Please look at how the marvell driver enables and configures this
> feature. Ideally we want all PHY drivers to use the same configuration
> API for features like this.

ack

> 
>     Andrew

Re: [PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled

[Ardelean, Alexandru]

On Tue, 2019-08-06 at 07:52 +0200, Heiner Kallweit wrote:
> [External]
> 
> On 05.08.2019 18:54, Alexandru Ardelean wrote:
> > Down-speed auto-negotiation may not always be enabled, in which case the
> > PHY won't down-shift to 100 or 10 during auto-negotiation.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index 86848444bd98..a1f3456a8504 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -32,6 +32,13 @@
> >  #define   ADIN1300_NRG_PD_TX_EN			BIT(2)
> >  #define   ADIN1300_NRG_PD_STATUS		BIT(1)
> >  
> > +#define ADIN1300_PHY_CTRL2			0x0016
> > +#define   ADIN1300_DOWNSPEED_AN_100_EN		BIT(11)
> > +#define   ADIN1300_DOWNSPEED_AN_10_EN		BIT(10)
> > +#define   ADIN1300_GROUP_MDIO_EN		BIT(6)
> > +#define   ADIN1300_DOWNSPEEDS_EN	\
> > +	(ADIN1300_DOWNSPEED_AN_100_EN | ADIN1300_DOWNSPEED_AN_10_EN)
> > +
> >  #define ADIN1300_INT_MASK_REG			0x0018
> >  #define   ADIN1300_INT_MDIO_SYNC_EN		BIT(9)
> >  #define   ADIN1300_INT_ANEG_STAT_CHNG_EN	BIT(8)
> > @@ -425,6 +432,22 @@ static int adin_config_mdix(struct phy_device *phydev)
> >  	return phy_write(phydev, ADIN1300_PHY_CTRL1, reg);
> >  }
> >  
> > +static int adin_config_downspeeds(struct phy_device *phydev)
> > +{
> > +	int reg;
> > +
> > +	reg = phy_read(phydev, ADIN1300_PHY_CTRL2);
> > +	if (reg < 0)
> > +		return reg;
> > +
> > +	if ((reg & ADIN1300_DOWNSPEEDS_EN) == ADIN1300_DOWNSPEEDS_EN)
> > +		return 0;
> > +
> > +	reg |= ADIN1300_DOWNSPEEDS_EN;
> > +
> > +	return phy_write(phydev, ADIN1300_PHY_CTRL2, reg);
> 
> Using phy_set_bits() would be easier.

ack;
missed this;

thanks

> 
> > +}
> > +
> >  static int adin_config_aneg(struct phy_device *phydev)
> >  {
> >  	int ret;
> > @@ -433,6 +456,10 @@ static int adin_config_aneg(struct phy_device *phydev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = adin_config_downspeeds(phydev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	return genphy_config_aneg(phydev);
> >  }
> >  
> > 

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:27 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:53PM +0300, Alexandru Ardelean wrote:
> > This change adds bindings for the Analog Devices ADIN PHY driver, detailing
> > all the properties implemented by the driver.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  .../devicetree/bindings/net/adi,adin.yaml     | 93 +++++++++++++++++++
> >  MAINTAINERS                                   |  2 +
> >  include/dt-bindings/net/adin.h                | 26 ++++++
> >  3 files changed, 121 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
> >  create mode 100644 include/dt-bindings/net/adin.h
> > 
> > diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml
> > b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > new file mode 100644
> > index 000000000000..fcf884bb86f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/adi,adin.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADIN1200/ADIN1300 PHY
> > +
> > +maintainers:
> > +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> > +
> > +description: |
> > +  Bindings for Analog Devices Industrial Ethernet PHYsphy
> > +
> > +properties:
> > +  compatible:
> > +    description: |
> > +      Compatible list, may contain "ethernet-phy-ieee802.3-c45" in which case
> > +      Clause 45 will be used to access device management registers. If
> > +      unspecified, Clause 22 will be used. Use this only when MDIO supports
> > +      Clause 45 access, but there is no other way to determine this.
> > +    enum:
> > +      - ethernet-phy-ieee802.3-c45
> 
> It is valid to list ethernet-phy-ieee802.3-c22, it is just not
> required. So maybe you should list it here to keep the DT validater happy?

ack

> 
> 	  Andrew

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:11 +0200, Andrew Lunn wrote:
> [External]
> 
> > +  adi,rx-internal-delay:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> > +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> > +      default value is 0 (which represents 2 ns)
> > +    enum: [ 0, 1, 2, 6, 7 ]
> 
> We want these numbers to be in ns. So the default value would actually
> be 2. The driver needs to convert the number in DT to a value to poke
> into a PHY register. Please rename the property adi,rx-internal-delay-ns.

ack;
also, good point about ns units and PHY driver to convert it to reg values;

> 
> > +
> > +  adi,tx-internal-delay:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> > +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> > +      default value is 0 (which represents 2 ns)
> > +    enum: [ 0, 1, 2, 6, 7 ]
> 
> Same here.
> 
> > +
> > +  adi,fifo-depth:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      When operating in RMII mode, this option configures the FIFO depth.
> > +      See `dt-bindings/net/adin.h`.
> > +    enum: [ 0, 1, 2, 3, 4, 5 ]
> 
> Units? You should probably rename this adi,fifo-depth-bits and list
> the valid values in bits.

units are bits;
will adapt this

> 
> > +
> > +  adi,eee-enabled:
> > +    description: |
> > +      Advertise EEE capabilities on power-up/init (default disabled)
> > +    type: boolean
> 
> It is not the PHY which decides this. The MAC indicates if it is EEE
> capable to phylib. phylib looks into the PHY registers to determine if
> the PHY supports EEE. phylib will then enable EEE
> advertisement. Please remove this, and ensure EEE is disabled by
> default.

ack;
will remove

> 
> 	Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> [External]
> 
> > +struct adin_hw_stat {
> > +	const char *string;
> > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > +		memcpy(data + i * ETH_GSTRING_LEN,
> > +		       adin_hw_stats[i].string, ETH_GSTRING_LEN);
> 
> You define string as a char *. So it will be only as long as it should
> be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> end of the string and into whatever follows.
> 

hmm, will use strlcpy()
i blindedly copied memcpy() from some other driver

> 
> > +	}
> > +}
> > +
> > +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> > +				   struct adin_hw_stat *stat,
> > +				   u32 *val)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = (ret & 0xffff);
> > +
> > +	if (stat->reg2 == 0)
> > +		return 0;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val <<= 16;
> > +	*val |= (ret & 0xffff);
> 
> Does the hardware have a snapshot feature? Is there a danger that
> between the two reads stat->reg1 rolls over and you end up with too
> big a value?

i'm afraid i don't understand about the snapshot feature you are mentioning;
i.e. i don't remember seeing it in other chips;

regarding the danger that stat->reg1 rolls over, i guess that is possible, but it's a bit hard to guard against;
i guess if it ends up in that scenario, [for many counters] things would be horribly bad, and the chip, or cabling would
be unusable;

not sure if this answer is sufficient/satisfactory;

thanks

> 
>     Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 17:30 +0200, Andrew Lunn wrote:
> [External]
> 
> On Mon, Aug 05, 2019 at 07:54:52PM +0300, Alexandru Ardelean wrote:
> > This change implements retrieving all the error counters from the PHY.
> > The PHY supports several error counters/stats. The `Mean Square Errors`
> > status values are only valie when a link is established, and shouldn't be
> > incremented. These values characterize the quality of a signal.
> 
> I think you mean accumulated, not incremented?

accumulated sounds better;


> > The rest of the error counters are self-clearing on read.
> > Most of them are reports from the Frame Checker engine that the PHY has.
> > 
> > Not retrieving the `LPI Wake Error Count Register` here, since that is used
> > by the PHY framework to check for any EEE errors. And that register is
> > self-clearing when read (as per IEEE spec).
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/net/phy/adin.c | 108 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 108 insertions(+)
> > 
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> > index a1f3456a8504..04896547dac8 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
> >  	{ MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR,	ADIN1300_LPI_WAKE_ERR_CNT_REG },
> >  };
> >  
> > +struct adin_hw_stat {
> > +	const char *string;
> > +	u16 reg1;
> > +	u16 reg2;
> > +	bool do_not_inc;
> 
> do_not_accumulate? or reverse its meaning, clear_on_read?

do_not_accumulate works;
there are only 4 regs that need this property set to true

> 
>    Andrew

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Ardelean, Alexandru]

On Mon, 2019-08-05 at 16:11 +0200, Andrew Lunn wrote:
> [External]
> 
> > +  adi,rx-internal-delay:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> > +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> > +      default value is 0 (which represents 2 ns)
> > +    enum: [ 0, 1, 2, 6, 7 ]
> 
> We want these numbers to be in ns. So the default value would actually
> be 2. The driver needs to convert the number in DT to a value to poke
> into a PHY register. Please rename the property adi,rx-internal-delay-ns.
> 

I just realized: this will probably have to be pico-seconds.
Some delays are 1.60 ns, which are not easy to represent in in ns in DT.

The values here are actually the register values corresponding to the delays.

> > +
> > +  adi,tx-internal-delay:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> > +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> > +      default value is 0 (which represents 2 ns)
> > +    enum: [ 0, 1, 2, 6, 7 ]
> 
> Same here.
> 
> > +
> > +  adi,fifo-depth:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      When operating in RMII mode, this option configures the FIFO depth.
> > +      See `dt-bindings/net/adin.h`.
> > +    enum: [ 0, 1, 2, 3, 4, 5 ]
> 
> Units? You should probably rename this adi,fifo-depth-bits and list
> the valid values in bits.
> 
> > +
> > +  adi,eee-enabled:
> > +    description: |
> > +      Advertise EEE capabilities on power-up/init (default disabled)
> > +    type: boolean
> 
> It is not the PHY which decides this. The MAC indicates if it is EEE
> capable to phylib. phylib looks into the PHY registers to determine if
> the PHY supports EEE. phylib will then enable EEE
> advertisement. Please remove this, and ensure EEE is disabled by
> default.
> 
> 	Andrew

Re: [PATCH 16/16] dt-bindings: net: add bindings for ADIN PHY driver

[Rob Herring]

On Mon, Aug 5, 2019 at 7:55 AM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> This change adds bindings for the Analog Devices ADIN PHY driver, detailing
> all the properties implemented by the driver.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  .../devicetree/bindings/net/adi,adin.yaml     | 93 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +
>  include/dt-bindings/net/adin.h                | 26 ++++++
>  3 files changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/adi,adin.yaml
>  create mode 100644 include/dt-bindings/net/adin.h
>
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> new file mode 100644
> index 000000000000..fcf884bb86f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/adi,adin.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADIN1200/ADIN1300 PHY
> +
> +maintainers:
> +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> +
> +description: |
> +  Bindings for Analog Devices Industrial Ethernet PHYs
> +

Needs an:

allOf:
  - $ref: ethernet-phy.yaml#

> +properties:
> +  compatible:
> +    description: |
> +      Compatible list, may contain "ethernet-phy-ieee802.3-c45" in which case
> +      Clause 45 will be used to access device management registers. If
> +      unspecified, Clause 22 will be used. Use this only when MDIO supports
> +      Clause 45 access, but there is no other way to determine this.
> +    enum:
> +      - ethernet-phy-ieee802.3-c45

Then you can drop 'compatible' from here as it is covered by the above schema.

> +
> +  adi,phy-mode-internal:
> +    $ref: /schemas/types.yaml#/definitions/string

This has to be under an 'allOf' or it doesn't actually work. Same below.

> +    description: |

No special formatting here, you can drop '|'.

> +      The internal mode of the PHY. This assumes that there is a PHY converter
> +      in-between the MAC & PHY.
> +    enum: [ "rgmii", "rgmii-id", "rgmii-txid", "rgmii-rxid", "rmii", "mii" ]

Don't need quotes here.

> +
> +  adi,rx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII RX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> +      default value is 0 (which represents 2 ns)

Use 'default: 0' to specify defaults.

> +    enum: [ 0, 1, 2, 6, 7 ]
> +
> +  adi,tx-internal-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      RGMII TX Clock Delay used only when PHY operates in RGMII mode (phy-mode
> +      is "rgmii-id", "rgmii-rxid", "rgmii-txid") see `dt-bindings/net/adin.h`
> +      default value is 0 (which represents 2 ns)
> +    enum: [ 0, 1, 2, 6, 7 ]
> +
> +  adi,fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      When operating in RMII mode, this option configures the FIFO depth.
> +      See `dt-bindings/net/adin.h`.
> +    enum: [ 0, 1, 2, 3, 4, 5 ]
> +
> +  adi,eee-enabled:

Isn't there a standard property for EEE control?

> +    description: |
> +      Advertise EEE capabilities on power-up/init (default disabled)
> +    type: boolean
> +
> +  adi,disable-energy-detect:
> +    description: |
> +      Disables Energy Detect Powerdown Mode (default disabled, i.e energy detect
> +      is enabled if this property is unspecified)
> +    type: boolean
> +
> +  reset-gpios:
> +    description: |
> +      GPIO to reset the PHY
> +      see Documentation/devicetree/bindings/gpio/gpio.txt.

Active high or low?

> +
> +examples:
> +  - |
> +    ethernet-phy@0 {
> +        compatible = "ethernet-phy-ieee802.3-c45";
> +        reg = <0>;
> +    };

Not really anything specific to this binding. Drop it.


> +  - |
> +    #include <dt-bindings/net/adin.h>
> +    ethernet-phy@1 {
> +        reg = <1>;
> +        adi,phy-mode-internal = "rgmii-id";
> +
> +        adi,rx-internal-delay = <ADIN1300_RGMII_1_80_NS>;
> +        adi,tx-internal-delay = <ADIN1300_RGMII_2_20_NS>;
> +    };
> +  - |
> +    #include <dt-bindings/net/adin.h>
> +    ethernet-phy@2 {
> +        reg = <2>;
> +        phy-mode = "rmii";
> +
> +        adi,fifo-depth = <ADIN1300_RMII_16_BITS>;
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index faf5723610c8..6ffbb266dee4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -944,6 +944,8 @@ L:  netdev@vger.kernel.org
>  W:     http://ez.analog.com/community/linux-device-drivers
>  S:     Supported
>  F:     drivers/net/phy/adin.c
> +F:     include/dt-bindings/net/adin.h
> +F:     Documentation/devicetree/bindings/net/adi,adin.yaml
>
>  ANALOG DEVICES INC ADIS DRIVER LIBRARY
>  M:     Alexandru Ardelean <alexandru.ardelean@analog.com>
> diff --git a/include/dt-bindings/net/adin.h b/include/dt-bindings/net/adin.h
> new file mode 100644
> index 000000000000..4c3afa550c59
> --- /dev/null
> +++ b/include/dt-bindings/net/adin.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/**
> + * Device Tree constants for Analog Devices Industrial Ethernet PHYs
> + *
> + * Copyright 2019 Analog Devices Inc.
> + */
> +
> +#ifndef _DT_BINDINGS_ADIN_H
> +#define _DT_BINDINGS_ADIN_H
> +
> +/* RGMII internal delay settings for rx and tx for ADIN1300 */
> +#define ADIN1300_RGMII_1_60_NS         0x1
> +#define ADIN1300_RGMII_1_80_NS         0x2
> +#define        ADIN1300_RGMII_2_00_NS          0x0
> +#define        ADIN1300_RGMII_2_20_NS          0x6
> +#define        ADIN1300_RGMII_2_40_NS          0x7
> +
> +/* RMII fifo depth values */
> +#define ADIN1300_RMII_4_BITS           0x0
> +#define ADIN1300_RMII_8_BITS           0x1
> +#define ADIN1300_RMII_12_BITS          0x2
> +#define ADIN1300_RMII_16_BITS          0x3
> +#define ADIN1300_RMII_20_BITS          0x4
> +#define ADIN1300_RMII_24_BITS          0x5
> +
> +#endif
> --
> 2.20.1
>

Re: [PATCH 06/16] net: phy: adin: support PHY mode converters

[Andrew Lunn]

On Tue, Aug 06, 2019 at 06:47:08AM +0000, Ardelean, Alexandru wrote:
> On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote:
> > [External]
> > 
> > On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> > > Sometimes, the connection between a MAC and PHY is done via a
> > > mode/interface converter. An example is a GMII-to-RGMII converter, which
> > > would mean that the MAC operates in GMII mode while the PHY operates in
> > > RGMII. In this case there is a discrepancy between what the MAC expects &
> > > what the PHY expects and both need to be configured in their respective
> > > modes.
> > > 
> > > Sometimes, this converter is specified via a board/system configuration (in
> > > the device-tree for example). But, other times it can be left unspecified.
> > > The use of these converters is common in boards that have FPGA on them.
> > > 
> > > This patch also adds support for a `adi,phy-mode-internal` property that
> > > can be used in these (implicit convert) cases. The internal PHY mode will
> > > be used to specify the correct register settings for the PHY.
> > > 
> > > `fwnode_handle` is used, since this property may be specified via ACPI as
> > > well in other setups, but testing has been done in DT context.
> > 
> > Looking at the patch, you seems to assume phy-mode is what the MAC is
> > using? That seems rather odd, given the name. It seems like a better
> > solution would be to add a mac-mode, which the MAC uses to configure
> > its side of the link. The MAC driver would then implement this
> > property.
> > 
> 
> actually, that's a pretty good idea;
> i guess i was narrow-minded when writing the driver, and got stuck on phy specifics, and forgot about the MAC-side;
> [ i also catch these design elements when reviewing, but i also seem to miss them when writing stuff sometimes ]
> 

Hi Ardelean

We should also consider the media converter itself. It is passive, or
does it need a driver. You seems to be considering GMII-to-RGMII. But
what about RGMII to SGMII? or RGMII to 1000Base-KX etc? Ideally we
want a generic solution and we need to think about all the parts in
the system.

     Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Andrew Lunn]

On Tue, Aug 06, 2019 at 07:11:57AM +0000, Ardelean, Alexandru wrote:
> On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> > [External]
> > 
> > > +struct adin_hw_stat {
> > > +	const char *string;
> > > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > > +		memcpy(data + i * ETH_GSTRING_LEN,
> > > +		       adin_hw_stats[i].string, ETH_GSTRING_LEN);
> > 
> > You define string as a char *. So it will be only as long as it should
> > be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> > end of the string and into whatever follows.
> > 
> 
> hmm, will use strlcpy()
> i blindedly copied memcpy() from some other driver

Hopefully that driver used const char string[ETH_GSTRING_LEN]. Then a
memcpy is safe. If not, please let me know what driver you copied.

> i'm afraid i don't understand about the snapshot feature you are mentioning;
> i.e. i don't remember seeing it in other chips;

It is frequency done at the MAC layer for statistics. You tell the
hardware to snapshot all the statistics. It atomically makes a copy of
all the statistics into a set of registers. These values are then
static, and consistent between counters. You can read them out knowing
they are not going to change.

> regarding the danger that stat->reg1 rolls over, i guess that is
> possible, but it's a bit hard to guard against;

The normal solution is the read the MSB, the LSB and then the MSB
again. If the MSB value has changed between the two reads, you know a
roll over has happened, and you need to do it all again.

     Andrew

Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support

[Ardelean, Alexandru]

On Tue, 2019-08-06 at 17:46 +0200, Andrew Lunn wrote:
> [External]
> 
> On Tue, Aug 06, 2019 at 07:11:57AM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2019-08-05 at 17:28 +0200, Andrew Lunn wrote:
> > > [External]
> > > 
> > > > +struct adin_hw_stat {
> > > > +	const char *string;
> > > > +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> > > > +		memcpy(data + i * ETH_GSTRING_LEN,
> > > > +		       adin_hw_stats[i].string, ETH_GSTRING_LEN);
> > > 
> > > You define string as a char *. So it will be only as long as it should
> > > be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
> > > end of the string and into whatever follows.
> > > 
> > 
> > hmm, will use strlcpy()
> > i blindedly copied memcpy() from some other driver
> 
> Hopefully that driver used const char string[ETH_GSTRING_LEN]. Then a
> memcpy is safe. If not, please let me know what driver you copied.

It was an older Marvell PHY driver (marvell.c) ; in version 4.14.
I used that as an initial work-base for writing the driver.
Then I did the conversion to a newer kernel, then I also had to also consider an older kernel, then I got confused :)

Well, in any case, I am solely considering net-next master (now) for upstreaming this.

> 
> > i'm afraid i don't understand about the snapshot feature you are mentioning;
> > i.e. i don't remember seeing it in other chips;
> 
> It is frequency done at the MAC layer for statistics. You tell the
> hardware to snapshot all the statistics. It atomically makes a copy of
> all the statistics into a set of registers. These values are then
> static, and consistent between counters. You can read them out knowing
> they are not going to change.
> 
> > regarding the danger that stat->reg1 rolls over, i guess that is
> > possible, but it's a bit hard to guard against;
> 
> The normal solution is the read the MSB, the LSB and then the MSB
> again. If the MSB value has changed between the two reads, you know a
> roll over has happened, and you need to do it all again.

hmm; ok
I'll try to look for an existing example for this.

> 
>      Andrew

Re: [PATCH 06/16] net: phy: adin: support PHY mode converters

[Ardelean, Alexandru]

On Tue, 2019-08-06 at 17:39 +0200, Andrew Lunn wrote:
> [External]
> 
> On Tue, Aug 06, 2019 at 06:47:08AM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote:
> > > [External]
> > > 
> > > On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> > > > Sometimes, the connection between a MAC and PHY is done via a
> > > > mode/interface converter. An example is a GMII-to-RGMII converter, which
> > > > would mean that the MAC operates in GMII mode while the PHY operates in
> > > > RGMII. In this case there is a discrepancy between what the MAC expects &
> > > > what the PHY expects and both need to be configured in their respective
> > > > modes.
> > > > 
> > > > Sometimes, this converter is specified via a board/system configuration (in
> > > > the device-tree for example). But, other times it can be left unspecified.
> > > > The use of these converters is common in boards that have FPGA on them.
> > > > 
> > > > This patch also adds support for a `adi,phy-mode-internal` property that
> > > > can be used in these (implicit convert) cases. The internal PHY mode will
> > > > be used to specify the correct register settings for the PHY.
> > > > 
> > > > `fwnode_handle` is used, since this property may be specified via ACPI as
> > > > well in other setups, but testing has been done in DT context.
> > > 
> > > Looking at the patch, you seems to assume phy-mode is what the MAC is
> > > using? That seems rather odd, given the name. It seems like a better
> > > solution would be to add a mac-mode, which the MAC uses to configure
> > > its side of the link. The MAC driver would then implement this
> > > property.
> > > 
> > 
> > actually, that's a pretty good idea;
> > i guess i was narrow-minded when writing the driver, and got stuck on phy specifics, and forgot about the MAC-side;
> > [ i also catch these design elements when reviewing, but i also seem to miss them when writing stuff sometimes ]
> > 
> 
> Hi Ardelean
> 
> We should also consider the media converter itself. It is passive, or
> does it need a driver. You seems to be considering GMII-to-RGMII. But
> what about RGMII to SGMII? or RGMII to 1000Base-KX etc? Ideally we
> want a generic solution and we need to think about all the parts in
> the system.

In our case the GMII-to-RGMII converter is passive and does not need a driver.
It's an HDL/FPGA block.
There may be other converters that do need a driver.
To be honest, the multitude of possible configurations [given that it's FPGA] can be... a lot.

In one of our cases, specifying the MAC mode to be different than PHY mode [which assumes that there is an implicit
passive media converter in-between] works.

I admit that a generic solution would be nice.
Is it ok if we defer the solution for this drivers/patchset?

If you propose something, I can take a look as part of a different/new discussion.

No guarrantees about how soon it would be implemented.

Thanks
Alex

> 
>      Andrew

Re: [PATCH 06/16] net: phy: adin: support PHY mode converters

[Andrew Lunn]

> Is it ok if we defer the solution for this drivers/patchset?

Yes, not a problem if phy-mode means phy-mode.

     Andrew