[PATCH RFC 0/6] DPAA2 MAC Driver

[PATCH RFC 0/6] DPAA2 MAC Driver

[Ioana Ciornei]

After today's discussion with Russell King about what phylink exposes in
.mac_config(): https://marc.info/?l=linux-netdev&m=156043794316709&w=2
I am submitting for initial review the dpaa2-mac driver model.

At the moment, pause frame support is missing so inherently all the USXGMII
modes that rely on backpressure applied by the PHY in rate adaptation between
network side and system side don't work properly.

As next steps, the driver will have to be integrated with the SFP bus so
commands such as 'ethtool --dump-module-eeprom' will have to work through the
current callpath through firmware. This poses somewhat of a problem, as
dpaa2-eth lacks any handle to the phy so it will probably need further
modification to the API that the firmware exposes (same applies to 'ethtool
--phy-statistics').

The documentation patch provides a more complete view of the software
architecture and the current implementation.

Ioana Ciornei (4):
  net: phy: update the autoneg state in phylink_phy_change
  dpaa2-mac: add MC API for the DPMAC object
  dpaa2-mac: add initial driver
  net: documentation: add MAC/PHY proxy driver documentation

Ioana Radulescu (2):
  dpaa2-eth: add support for new link state APIs
  dpaa2-eth: add autoneg support

 .../freescale/dpaa2/dpmac-driver.rst               | 159 ++++++
 .../device_drivers/freescale/dpaa2/index.rst       |   1 +
 MAINTAINERS                                        |   8 +
 drivers/net/ethernet/freescale/dpaa2/Kconfig       |  13 +
 drivers/net/ethernet/freescale/dpaa2/Makefile      |   2 +
 .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   |  83 +++-
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c   | 541 +++++++++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h   | 107 ++++
 drivers/net/ethernet/freescale/dpaa2/dpmac.c       | 369 ++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpmac.h       | 210 ++++++++
 drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h    |  35 ++
 drivers/net/ethernet/freescale/dpaa2/dpni.c        |  70 +++
 drivers/net/ethernet/freescale/dpaa2/dpni.h        |  27 +
 drivers/net/phy/phylink.c                          |   1 +
 14 files changed, 1612 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.c
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.h

-- 
1.9.1

[PATCH RFC 1/6] net: phy: update the autoneg state in phylink_phy_change

[Ioana Ciornei]

The phy_state field of phylink should carry only valid information
especially when this can be passed to the .mac_config callback.
Update the an_enabled field with the autoneg state in the
phylink_phy_change function.

Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5d0af041b8f9..dd1feb7b5472 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -688,6 +688,7 @@ static void phylink_phy_change(struct phy_device *phydev, bool up,
 		pl->phy_state.pause |= MLO_PAUSE_ASYM;
 	pl->phy_state.interface = phydev->interface;
 	pl->phy_state.link = up;
+	pl->phy_state.an_enabled = phydev->autoneg;
 	mutex_unlock(&pl->state_mutex);
 
 	phylink_run_resolve(pl);
-- 
1.9.1

[PATCH RFC 2/6] dpaa2-eth: add support for new link state APIs

[Ioana Ciornei]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Add v2 of dpni_get_link_state() and dpni_set_link_cfg() commands.  The
new version allows setting and getting advertised and supported link
options. This will allow adding autoneg support in an following patch.

Signed-off-by: Valentin Catalin Neacsu <valentin-catalin.neacsu@nxp.com>
Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h | 35 +++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpni.c     | 70 +++++++++++++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpni.h     | 27 ++++++++++
 3 files changed, 132 insertions(+)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h b/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
index 7b44d7d9b19a..3fb17b35a8df 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
@@ -11,9 +11,11 @@
 #define DPNI_VER_MAJOR				7
 #define DPNI_VER_MINOR				0
 #define DPNI_CMD_BASE_VERSION			1
+#define DPNI_CMD_2ND_VERSION			2
 #define DPNI_CMD_ID_OFFSET			4
 
 #define DPNI_CMD(id)	(((id) << DPNI_CMD_ID_OFFSET) | DPNI_CMD_BASE_VERSION)
+#define DPNI_CMD_V2(id)	(((id) << DPNI_CMD_ID_OFFSET) | DPNI_CMD_2ND_VERSION)
 
 #define DPNI_CMDID_OPEN					DPNI_CMD(0x801)
 #define DPNI_CMDID_CLOSE				DPNI_CMD(0x800)
@@ -42,9 +44,11 @@
 #define DPNI_CMDID_GET_QDID				DPNI_CMD(0x210)
 #define DPNI_CMDID_GET_TX_DATA_OFFSET			DPNI_CMD(0x212)
 #define DPNI_CMDID_GET_LINK_STATE			DPNI_CMD(0x215)
+#define DPNI_CMDID_GET_LINK_STATE_V2			DPNI_CMD_V2(0x215)
 #define DPNI_CMDID_SET_MAX_FRAME_LENGTH			DPNI_CMD(0x216)
 #define DPNI_CMDID_GET_MAX_FRAME_LENGTH			DPNI_CMD(0x217)
 #define DPNI_CMDID_SET_LINK_CFG				DPNI_CMD(0x21A)
+#define DPNI_CMDID_SET_LINK_CFG_V2			DPNI_CMD_V2(0x21A)
 #define DPNI_CMDID_SET_TX_SHAPING			DPNI_CMD(0x21B)
 
 #define DPNI_CMDID_SET_MCAST_PROMISC			DPNI_CMD(0x220)
@@ -294,8 +298,22 @@ struct dpni_cmd_set_link_cfg {
 	__le64 options;
 };
 
+struct dpni_cmd_set_link_cfg_v2 {
+	/* cmd word 0 */
+	__le64 pad0;
+	/* cmd word 1 */
+	__le32 rate;
+	__le32 pad1;
+	/* cmd word 2 */
+	__le64 options;
+	/* cmd word 3 */
+	__le64 advertising;
+};
+
 #define DPNI_LINK_STATE_SHIFT		0
 #define DPNI_LINK_STATE_SIZE		1
+#define DPNI_STATE_VALID_SHIFT		1
+#define DPNI_STATE_VALID_SIZE		1
 
 struct dpni_rsp_get_link_state {
 	/* response word 0 */
@@ -310,6 +328,23 @@ struct dpni_rsp_get_link_state {
 	__le64 options;
 };
 
+struct dpni_rsp_get_link_state_v2 {
+	/* response word 0 */
+	__le32 pad0;
+	/* from LSB: up:1, valid:1 */
+	u8 flags;
+	u8 pad1[3];
+	/* response word 1 */
+	__le32 rate;
+	__le32 pad2;
+	/* response word 2 */
+	__le64 options;
+	/* cmd word 3 */
+	__le64 supported;
+	/* cmd word 4 */
+	__le64 advertising;
+};
+
 struct dpni_cmd_set_max_frame_length {
 	__le16 max_frame_length;
 };
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni.c b/drivers/net/ethernet/freescale/dpaa2/dpni.c
index 220dfc806a24..e7f318e0b41f 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni.c
@@ -853,6 +853,36 @@ int dpni_set_link_cfg(struct fsl_mc_io *mc_io,
 }
 
 /**
+ * dpni_set_link_cfg_v2() - set the link configuration.
+ * @mc_io:      Pointer to MC portal's I/O object
+ * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:      Token of DPNI object
+ * @cfg:        Link configuration
+ *
+ * Return:      '0' on Success; Error code otherwise.
+ */
+int dpni_set_link_cfg_v2(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 const struct dpni_link_cfg *cfg)
+{
+	struct fsl_mc_command cmd = { 0 };
+	struct dpni_cmd_set_link_cfg_v2 *cmd_params;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPNI_CMDID_SET_LINK_CFG_V2,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpni_cmd_set_link_cfg_v2 *)cmd.params;
+	cmd_params->rate = cpu_to_le32(cfg->rate);
+	cmd_params->options = cpu_to_le64(cfg->options);
+	cmd_params->advertising = cpu_to_le64(cfg->advertising);
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
  * dpni_get_link_state() - Return the link state (either up or down)
  * @mc_io:	Pointer to MC portal's I/O object
  * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
@@ -890,6 +920,46 @@ int dpni_get_link_state(struct fsl_mc_io *mc_io,
 }
 
 /**
+ * dpni_get_link_state_v2() - Return the link state (either up or down)
+ * @mc_io:      Pointer to MC portal's I/O object
+ * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:      Token of DPNI object
+ * @state:      Returned link state;
+ *
+ * Return:      '0' on Success; Error code otherwise.
+ */
+int dpni_get_link_state_v2(struct fsl_mc_io *mc_io,
+			   u32 cmd_flags,
+			   u16 token,
+			   struct dpni_link_state *state)
+{
+	struct fsl_mc_command cmd = { 0 };
+	struct dpni_rsp_get_link_state_v2 *rsp_params;
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPNI_CMDID_GET_LINK_STATE_V2,
+					  cmd_flags,
+					  token);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	rsp_params = (struct dpni_rsp_get_link_state_v2 *)cmd.params;
+	state->up = dpni_get_field(rsp_params->flags, LINK_STATE);
+	state->state_valid = dpni_get_field(rsp_params->flags, STATE_VALID);
+	state->rate = le32_to_cpu(rsp_params->rate);
+	state->options = le64_to_cpu(rsp_params->options);
+	state->supported = le64_to_cpu(rsp_params->supported);
+	state->advertising = le64_to_cpu(rsp_params->advertising);
+
+	return 0;
+}
+
+/**
  * dpni_set_max_frame_length() - Set the maximum received frame length.
  * @mc_io:	Pointer to MC portal's I/O object
  * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni.h b/drivers/net/ethernet/freescale/dpaa2/dpni.h
index a521242e2353..ad12e670c6e2 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni.h
@@ -471,6 +471,19 @@ int dpni_get_statistics(struct fsl_mc_io	*mc_io,
 #define DPNI_LINK_OPT_ASYM_PAUSE	0x0000000000000008ULL
 
 /**
+ * Advertised link speeds
+ */
+#define DPNI_ADVERTISED_10BASET_FULL           BIT_ULL(0)
+#define DPNI_ADVERTISED_100BASET_FULL          BIT_ULL(1)
+#define DPNI_ADVERTISED_1000BASET_FULL         BIT_ULL(2)
+#define DPNI_ADVERTISED_10000BASET_FULL        BIT_ULL(4)
+
+/**
+ * Advertise auto-negotiation enabled
+ */
+#define DPNI_ADVERTISED_AUTONEG                BIT_ULL(3)
+
+/**
  * struct - Structure representing DPNI link configuration
  * @rate: Rate
  * @options: Mask of available options; use 'DPNI_LINK_OPT_<X>' values
@@ -478,6 +491,7 @@ int dpni_get_statistics(struct fsl_mc_io	*mc_io,
 struct dpni_link_cfg {
 	u32 rate;
 	u64 options;
+	u64 advertising;
 };
 
 int dpni_set_link_cfg(struct fsl_mc_io			*mc_io,
@@ -485,6 +499,11 @@ int dpni_set_link_cfg(struct fsl_mc_io			*mc_io,
 		      u16				token,
 		      const struct dpni_link_cfg	*cfg);
 
+int dpni_set_link_cfg_v2(struct fsl_mc_io		*mc_io,
+			 u32				cmd_flags,
+			 u16				token,
+			 const struct dpni_link_cfg	*cfg);
+
 /**
  * struct dpni_link_state - Structure representing DPNI link state
  * @rate: Rate
@@ -494,7 +513,10 @@ int dpni_set_link_cfg(struct fsl_mc_io			*mc_io,
 struct dpni_link_state {
 	u32	rate;
 	u64	options;
+	u64	supported;
+	u64	advertising;
 	int	up;
+	int	state_valid;
 };
 
 int dpni_get_link_state(struct fsl_mc_io	*mc_io,
@@ -502,6 +524,11 @@ int dpni_get_link_state(struct fsl_mc_io	*mc_io,
 			u16			token,
 			struct dpni_link_state	*state);
 
+int dpni_get_link_state_v2(struct fsl_mc_io	*mc_io,
+			   u32			cmd_flags,
+			   u16			token,
+			   struct dpni_link_state	*state);
+
 int dpni_set_max_frame_length(struct fsl_mc_io	*mc_io,
 			      u32		cmd_flags,
 			      u16		token,
-- 
1.9.1

Re: [PATCH RFC 2/6] dpaa2-eth: add support for new link state APIs

[Andrew Lunn]

>  /**
> + * Advertised link speeds
> + */
> +#define DPNI_ADVERTISED_10BASET_FULL           BIT_ULL(0)
> +#define DPNI_ADVERTISED_100BASET_FULL          BIT_ULL(1)
> +#define DPNI_ADVERTISED_1000BASET_FULL         BIT_ULL(2)
> +#define DPNI_ADVERTISED_10000BASET_FULL        BIT_ULL(4)

So 10 Half and 100Half are not supported by the PHYs you use?  What
happens if somebody does connect a PHY which supports these speeds? Do
you need to change the firmware? I suppose you do anyway, since it is
the firmware which is driving the PHY.

>  struct dpni_link_state {
>  	u32	rate;
>  	u64	options;
> +	u64	supported;
> +	u64	advertising;
>  	int	up;
> +	int	state_valid;
>  };

Does the firmware report Pause? Asym Pause? EEE? Is this part of
options? Can you control the advertisement of these options?

     Andrew

RE: [PATCH RFC 2/6] dpaa2-eth: add support for new link state APIs

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 2/6] dpaa2-eth: add support for new link state APIs
> 
> >  /**
> > + * Advertised link speeds
> > + */
> > +#define DPNI_ADVERTISED_10BASET_FULL           BIT_ULL(0)
> > +#define DPNI_ADVERTISED_100BASET_FULL          BIT_ULL(1)
> > +#define DPNI_ADVERTISED_1000BASET_FULL         BIT_ULL(2)
> > +#define DPNI_ADVERTISED_10000BASET_FULL        BIT_ULL(4)
> 
> So 10 Half and 100Half are not supported by the PHYs you use?  What happens if
> somebody does connect a PHY which supports these speeds? Do you need to
> change the firmware? I suppose you do anyway, since it is the firmware which is
> driving the PHY.

First of all, if the firmware had access to the open-source PHY driver code, the design wouldn't have been like this. But as it is, the DPMAC object/driver is used squarely as a way to gather this information from the PHY library.

Half duplex modes are not supported in our MAC. This is why the firmware does not export any advertisement bits for these modes.
If somebody connects a PHY which supports 10 Half and 100Half modes, the intersection of the MAC capabilities and the PHY one's will be only the Full duplex modes.

> 
> >  struct dpni_link_state {
> >  	u32	rate;
> >  	u64	options;
> > +	u64	supported;
> > +	u64	advertising;
> >  	int	up;
> > +	int	state_valid;
> >  };
> 
> Does the firmware report Pause? Asym Pause? EEE? Is this part of options? Can
> you control the advertisement of these options?

The firmware knows about conveying the pause and asym pause configuration from/to the mac driver.

EEE is not a feature supported by our MAC so there is no configuration knob for this.

--
Ioana

> 
>      Andrew

[PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object

[Ioana Ciornei]

Add the necessary MC API for the DPMAC object. These functions will be
used in the initial patch that adds the dpaa2-mac driver.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h | 107 +++++++
 drivers/net/ethernet/freescale/dpaa2/dpmac.c     | 369 +++++++++++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpmac.h     | 210 +++++++++++++
 3 files changed, 686 insertions(+)
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.c
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.h

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h b/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
new file mode 100644
index 000000000000..d35f05b32275
--- /dev/null
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/* Copyright 2013-2016 Freescale Semiconductor Inc.
+ * Copyright 2019 NXP
+ */
+#ifndef _FSL_DPMAC_CMD_H
+#define _FSL_DPMAC_CMD_H
+
+/* DPMAC Version */
+#define DPMAC_VER_MAJOR				4
+#define DPMAC_VER_MINOR				4
+#define DPMAC_CMD_BASE_VERSION			1
+#define DPMAC_CMD_2ND_VERSION			2
+#define DPMAC_CMD_ID_OFFSET			4
+
+#define DPMAC_CMD(id)	(((id) << DPMAC_CMD_ID_OFFSET) | DPMAC_CMD_BASE_VERSION)
+#define DPMAC_CMD_V2(id) (((id) << DPMAC_CMD_ID_OFFSET) | DPMAC_CMD_2ND_VERSION)
+
+/* Command IDs */
+#define DPMAC_CMDID_CLOSE		DPMAC_CMD(0x800)
+#define DPMAC_CMDID_OPEN		DPMAC_CMD(0x80c)
+#define DPMAC_CMDID_GET_API_VERSION	DPMAC_CMD(0xa0c)
+
+#define DPMAC_CMDID_GET_ATTR		DPMAC_CMD(0x004)
+
+#define DPMAC_CMDID_SET_IRQ_ENABLE	DPMAC_CMD(0x012)
+#define DPMAC_CMDID_SET_IRQ_MASK	DPMAC_CMD(0x014)
+#define DPMAC_CMDID_GET_IRQ_STATUS	DPMAC_CMD(0x016)
+#define DPMAC_CMDID_CLEAR_IRQ_STATUS	DPMAC_CMD(0x017)
+
+#define DPMAC_CMDID_GET_LINK_CFG	DPMAC_CMD_V2(0x0c2)
+#define DPMAC_CMDID_SET_LINK_STATE	DPMAC_CMD_V2(0x0c3)
+
+/* Macros for accessing command fields smaller than 1byte */
+#define DPMAC_MASK(field)        \
+	GENMASK(DPMAC_##field##_SHIFT + DPMAC_##field##_SIZE - 1, \
+		DPMAC_##field##_SHIFT)
+
+#define dpmac_set_field(var, field, val) \
+	((var) |= (((val) << DPMAC_##field##_SHIFT) & DPMAC_MASK(field)))
+#define dpmac_get_field(var, field)      \
+	(((var) & DPMAC_MASK(field)) >> DPMAC_##field##_SHIFT)
+
+struct dpmac_cmd_open {
+	__le32 dpmac_id;
+};
+
+struct dpmac_cmd_set_irq_enable {
+	u8 enable;
+	u8 pad[3];
+	u8 irq_index;
+};
+
+struct dpmac_cmd_set_irq_mask {
+	__le32 mask;
+	u8 irq_index;
+};
+
+struct dpmac_cmd_get_irq_status {
+	__le32 status;
+	u8 irq_index;
+};
+
+struct dpmac_rsp_get_irq_status {
+	__le32 status;
+};
+
+struct dpmac_cmd_clear_irq_status {
+	__le32 status;
+	u8 irq_index;
+};
+
+struct dpmac_rsp_get_attributes {
+	u8 eth_if;
+	u8 link_type;
+	__le16 id;
+	__le32 max_rate;
+};
+
+struct dpmac_rsp_get_link_cfg {
+	__le64 options;
+	__le32 rate;
+	__le32 pad;
+	__le64 advertising;
+};
+
+#define DPMAC_STATE_SIZE	1
+#define DPMAC_STATE_SHIFT	0
+#define DPMAC_STATE_VALID_SIZE	1
+#define DPMAC_STATE_VALID_SHIFT	1
+
+struct dpmac_cmd_set_link_state {
+	__le64 options;
+	__le32 rate;
+	__le32 pad0;
+	/* from lsb: up:1, state_valid:1 */
+	u8 state;
+	u8 pad1[7];
+	__le64 supported;
+	__le64 advertising;
+};
+
+struct dpmac_rsp_get_api_version {
+	__le16 major;
+	__le16 minor;
+};
+
+#endif /* _FSL_DPMAC_CMD_H */
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac.c b/drivers/net/ethernet/freescale/dpaa2/dpmac.c
new file mode 100644
index 000000000000..b45a4bd60efc
--- /dev/null
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac.c
@@ -0,0 +1,369 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/* Copyright 2013-2016 Freescale Semiconductor Inc.
+ * Copyright 2019 NXP
+ */
+#include <linux/fsl/mc.h>
+#include "dpmac.h"
+#include "dpmac-cmd.h"
+
+/**
+ * dpmac_open() - Open a control session for the specified object.
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @dpmac_id:	DPMAC unique ID
+ * @token:	Returned token; use in subsequent API calls
+ *
+ * This function can be used to open a control session for an
+ * already created object; an object may have been declared in
+ * the DPL or by calling the dpmac_create function.
+ * This function returns a unique authentication token,
+ * associated with the specific object ID and the specific MC
+ * portal; this token must be used in all subsequent commands for
+ * this specific object
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_open(struct fsl_mc_io *mc_io,
+	       u32 cmd_flags,
+	       int dpmac_id,
+	       u16 *token)
+{
+	struct dpmac_cmd_open *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_OPEN,
+					  cmd_flags,
+					  0);
+	cmd_params = (struct dpmac_cmd_open *)cmd.params;
+	cmd_params->dpmac_id = cpu_to_le32(dpmac_id);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	*token = mc_cmd_hdr_read_token(&cmd);
+
+	return err;
+}
+
+/**
+ * dpmac_close() - Close the control session of the object
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ *
+ * After this function is called, no further operations are
+ * allowed on the object without opening a new control session.
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_close(struct fsl_mc_io *mc_io,
+		u32 cmd_flags,
+		u16 token)
+{
+	struct fsl_mc_command cmd = { 0 };
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_CLOSE, cmd_flags,
+					  token);
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
+ * dpmac_set_irq_enable() - Set overall interrupt state.
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ * @irq_index:	The interrupt index to configure
+ * @en:		Interrupt state - enable = 1, disable = 0
+ *
+ * Allows GPP software to control when interrupts are generated.
+ * Each interrupt can have up to 32 causes.  The enable/disable control's the
+ * overall interrupt state. if the interrupt is disabled no causes will cause
+ * an interrupt.
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_set_irq_enable(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 u8 irq_index,
+			 u8 en)
+{
+	struct dpmac_cmd_set_irq_enable *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_SET_IRQ_ENABLE,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpmac_cmd_set_irq_enable *)cmd.params;
+	cmd_params->irq_index = irq_index;
+	cmd_params->enable = en;
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
+ * dpmac_set_irq_mask() - Set interrupt mask.
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ * @irq_index:	The interrupt index to configure
+ * @mask:	Event mask to trigger interrupt;
+ *		each bit:
+ *			0 = ignore event
+ *			1 = consider event for asserting IRQ
+ *
+ * Every interrupt can have up to 32 causes and the interrupt model supports
+ * masking/unmasking each cause independently
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_set_irq_mask(struct fsl_mc_io *mc_io,
+		       u32 cmd_flags,
+		       u16 token,
+		       u8 irq_index,
+		       u32 mask)
+{
+	struct dpmac_cmd_set_irq_mask *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_SET_IRQ_MASK,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpmac_cmd_set_irq_mask *)cmd.params;
+	cmd_params->mask = cpu_to_le32(mask);
+	cmd_params->irq_index = irq_index;
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
+ * dpmac_get_irq_status() - Get the current status of any pending interrupts.
+ *
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ * @irq_index:	The interrupt index to configure
+ * @status:	Returned interrupts status - one bit per cause:
+ *			0 = no interrupt pending
+ *			1 = interrupt pending
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_get_irq_status(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 u8 irq_index,
+			 u32 *status)
+{
+	struct dpmac_cmd_get_irq_status *cmd_params;
+	struct dpmac_rsp_get_irq_status *rsp_params;
+	struct fsl_mc_command cmd = { 0 };
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_IRQ_STATUS,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpmac_cmd_get_irq_status *)cmd.params;
+	cmd_params->status = cpu_to_le32(*status);
+	cmd_params->irq_index = irq_index;
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	rsp_params = (struct dpmac_rsp_get_irq_status *)cmd.params;
+	*status = le32_to_cpu(rsp_params->status);
+
+	return 0;
+}
+
+/**
+ * dpmac_clear_irq_status() - Clear a pending interrupt's status
+ *
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ * @irq_index:	The interrupt index to configure
+ * @status:	Bits to clear (W1C) - one bit per cause:
+ *			0 = don't change
+ *			1 = clear status bit
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_clear_irq_status(struct fsl_mc_io *mc_io,
+			   u32 cmd_flags,
+			   u16 token,
+			   u8 irq_index,
+			   u32 status)
+{
+	struct dpmac_cmd_clear_irq_status *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_CLEAR_IRQ_STATUS,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpmac_cmd_clear_irq_status *)cmd.params;
+	cmd_params->status = cpu_to_le32(status);
+	cmd_params->irq_index = irq_index;
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
+ * dpmac_get_attributes - Retrieve DPMAC attributes.
+ *
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPMAC object
+ * @attr:	Returned object's attributes
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpmac_get_attributes(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 struct dpmac_attr *attr)
+{
+	struct dpmac_rsp_get_attributes *rsp_params;
+	struct fsl_mc_command cmd = { 0 };
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_ATTR,
+					  cmd_flags,
+					  token);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	rsp_params = (struct dpmac_rsp_get_attributes *)cmd.params;
+	attr->eth_if = rsp_params->eth_if;
+	attr->link_type = rsp_params->link_type;
+	attr->id = le16_to_cpu(rsp_params->id);
+	attr->max_rate = le32_to_cpu(rsp_params->max_rate);
+
+	return 0;
+}
+
+/**
+ * dpmac_get_link_cfg() - Get Ethernet link configuration
+ * @mc_io:      Pointer to opaque I/O object
+ * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:      Token of DPMAC object
+ * @cfg:        Returned structure with the link configuration
+ *
+ * Return:      '0' on Success; Error code otherwise.
+ */
+int dpmac_get_link_cfg(struct fsl_mc_io *mc_io,
+		       u32 cmd_flags,
+		       u16 token,
+		       struct dpmac_link_cfg *cfg)
+{
+	struct dpmac_rsp_get_link_cfg *rsp_params;
+	struct fsl_mc_command cmd = { 0 };
+	int err = 0;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_LINK_CFG,
+					  cmd_flags,
+					  token);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	rsp_params = (struct dpmac_rsp_get_link_cfg *)cmd.params;
+	cfg->options = le64_to_cpu(rsp_params->options);
+	cfg->rate = le32_to_cpu(rsp_params->rate);
+	cfg->advertising = le64_to_cpu(rsp_params->advertising);
+
+	return 0;
+}
+
+/**
+ * dpmac_set_link_state() - Set the Ethernet link status
+ * @mc_io:      Pointer to opaque I/O object
+ * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:      Token of DPMAC object
+ * @link_state: Link state configuration
+ *
+ * Return:      '0' on Success; Error code otherwise.
+ */
+int dpmac_set_link_state(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 struct dpmac_link_state *link_state)
+{
+	struct dpmac_cmd_set_link_state *cmd_params;
+	struct fsl_mc_command cmd = { 0 };
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_SET_LINK_STATE,
+					  cmd_flags,
+					  token);
+	cmd_params = (struct dpmac_cmd_set_link_state *)cmd.params;
+	cmd_params->options = cpu_to_le64(link_state->options);
+	cmd_params->rate = cpu_to_le32(link_state->rate);
+	dpmac_set_field(cmd_params->state, STATE, link_state->up);
+	dpmac_set_field(cmd_params->state, STATE_VALID,
+			link_state->state_valid);
+	cmd_params->supported = cpu_to_le64(link_state->supported);
+	cmd_params->advertising = cpu_to_le64(link_state->advertising);
+
+	/* send command to mc*/
+	return mc_send_command(mc_io, &cmd);
+}
+
+/**
+ * dpmac_get_api_version() - Get Data Path MAC version
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @major_ver:	Major version of data path mac API
+ * @minor_ver:	Minor version of data path mac API
+ *
+ * Return:  '0' on Success; Error code otherwise.
+ */
+int dpmac_get_api_version(struct fsl_mc_io *mc_io,
+			  u32 cmd_flags,
+			  u16 *major_ver,
+			  u16 *minor_ver)
+{
+	struct dpmac_rsp_get_api_version *rsp_params;
+	struct fsl_mc_command cmd = { 0 };
+	int err;
+
+	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_API_VERSION,
+					  cmd_flags,
+					  0);
+
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	rsp_params = (struct dpmac_rsp_get_api_version *)cmd.params;
+	*major_ver = le16_to_cpu(rsp_params->major);
+	*minor_ver = le16_to_cpu(rsp_params->minor);
+
+	return 0;
+}
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpmac.h b/drivers/net/ethernet/freescale/dpaa2/dpmac.h
new file mode 100644
index 000000000000..de807547321c
--- /dev/null
+++ b/drivers/net/ethernet/freescale/dpaa2/dpmac.h
@@ -0,0 +1,210 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
+/* Copyright 2013-2016 Freescale Semiconductor Inc.
+ * Copyright 2019 NXP
+ */
+#ifndef __FSL_DPMAC_H
+#define __FSL_DPMAC_H
+
+/* Data Path MAC API
+ * Contains initialization APIs and runtime control APIs for DPMAC
+ */
+
+struct fsl_mc_io;
+
+int dpmac_open(struct fsl_mc_io *mc_io,
+	       u32 cmd_flags,
+	       int dpmac_id,
+	       u16 *token);
+
+int dpmac_close(struct fsl_mc_io *mc_io,
+		u32 cmd_flags,
+		u16 token);
+
+/**
+ * enum dpmac_link_type -  DPMAC link type
+ * @DPMAC_LINK_TYPE_NONE: No link
+ * @DPMAC_LINK_TYPE_FIXED: Link is fixed type
+ * @DPMAC_LINK_TYPE_PHY: Link by PHY ID
+ * @DPMAC_LINK_TYPE_BACKPLANE: Backplane link type
+ */
+enum dpmac_link_type {
+	DPMAC_LINK_TYPE_NONE,
+	DPMAC_LINK_TYPE_FIXED,
+	DPMAC_LINK_TYPE_PHY,
+	DPMAC_LINK_TYPE_BACKPLANE
+};
+
+/**
+ * enum dpmac_eth_if - DPMAC Ethrnet interface
+ * @DPMAC_ETH_IF_MII: MII interface
+ * @DPMAC_ETH_IF_RMII: RMII interface
+ * @DPMAC_ETH_IF_SMII: SMII interface
+ * @DPMAC_ETH_IF_GMII: GMII interface
+ * @DPMAC_ETH_IF_RGMII: RGMII interface
+ * @DPMAC_ETH_IF_SGMII: SGMII interface
+ * @DPMAC_ETH_IF_QSGMII: QSGMII interface
+ * @DPMAC_ETH_IF_XAUI: XAUI interface
+ * @DPMAC_ETH_IF_XFI: XFI interface
+ * @DPMAC_ETH_IF_CAUI: CAUI interface
+ * @DPMAC_ETH_IF_1000BASEX: 1000BASEX interface
+ * @DPMAC_ETH_IF_USXGMII: USXGMII interface
+ */
+enum dpmac_eth_if {
+	DPMAC_ETH_IF_MII,
+	DPMAC_ETH_IF_RMII,
+	DPMAC_ETH_IF_SMII,
+	DPMAC_ETH_IF_GMII,
+	DPMAC_ETH_IF_RGMII,
+	DPMAC_ETH_IF_SGMII,
+	DPMAC_ETH_IF_QSGMII,
+	DPMAC_ETH_IF_XAUI,
+	DPMAC_ETH_IF_XFI,
+	DPMAC_ETH_IF_CAUI,
+	DPMAC_ETH_IF_1000BASEX,
+	DPMAC_ETH_IF_USXGMII,
+};
+
+/**
+ * DPMAC IRQ Index and Events
+ */
+
+/**
+ * IRQ index
+ */
+#define DPMAC_IRQ_INDEX				0
+/**
+ * IRQ event - indicates a change in link state
+ */
+#define DPMAC_IRQ_EVENT_LINK_CFG_REQ		0x00000001
+/**
+ * IRQ event - Indicates that the link state changed
+ */
+#define DPMAC_IRQ_EVENT_LINK_CHANGED		0x00000002
+
+#define DPMAC_IRQ_EVENT_LINK_UP_REQ		0x00000004
+#define DPMAC_IRQ_EVENT_LINK_DOWN_REQ		0x00000008
+
+int dpmac_set_irq_enable(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 u8 irq_index,
+			 u8 en);
+
+int dpmac_set_irq_mask(struct fsl_mc_io *mc_io,
+		       u32 cmd_flags,
+		       u16 token,
+		       u8 irq_index,
+		       u32 mask);
+
+int dpmac_get_irq_status(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 u8 irq_index,
+			 u32 *status);
+
+int dpmac_clear_irq_status(struct fsl_mc_io *mc_io,
+			   u32 cmd_flags,
+			   u16 token,
+			   u8 irq_index,
+			   u32 status);
+
+/**
+ * struct dpmac_attr - Structure representing DPMAC attributes
+ * @id:		DPMAC object ID
+ * @max_rate:	Maximum supported rate - in Mbps
+ * @eth_if:	Ethernet interface
+ * @link_type:	link type
+ */
+struct dpmac_attr {
+	u16 id;
+	u32 max_rate;
+	enum dpmac_eth_if eth_if;
+	enum dpmac_link_type link_type;
+};
+
+int dpmac_get_attributes(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 struct dpmac_attr *attr);
+
+/**
+ * DPMAC link configuration/state options
+ */
+
+/**
+ * Enable auto-negotiation
+ */
+#define DPMAC_LINK_OPT_AUTONEG			BIT_ULL(0)
+/**
+ * Enable half-duplex mode
+ */
+#define DPMAC_LINK_OPT_HALF_DUPLEX		BIT_ULL(1)
+/**
+ * Enable pause frames
+ */
+#define DPMAC_LINK_OPT_PAUSE			BIT_ULL(2)
+/**
+ * Enable a-symmetric pause frames
+ */
+#define DPMAC_LINK_OPT_ASYM_PAUSE		BIT_ULL(3)
+
+/**
+ * Advertised link speeds
+ */
+#define DPMAC_ADVERTISED_10BASET_FULL		BIT_ULL(0)
+#define DPMAC_ADVERTISED_100BASET_FULL		BIT_ULL(1)
+#define DPMAC_ADVERTISED_1000BASET_FULL		BIT_ULL(2)
+#define DPMAC_ADVERTISED_10000BASET_FULL	BIT_ULL(4)
+#define DPMAC_ADVERTISED_2500BASEX_FULL		BIT_ULL(5)
+
+/**
+ * Advertise auto-negotiation enable
+ */
+#define DPMAC_ADVERTISED_AUTONEG		BIT_ULL(3)
+
+/**
+ * struct dpmac_link_cfg - Structure representing DPMAC link configuration
+ * @rate: Link's rate - in Mbps
+ * @options: Enable/Disable DPMAC link cfg features (bitmap)
+ * @advertising: Speeds that are advertised for autoneg (bitmap)
+ */
+struct dpmac_link_cfg {
+	u32 rate;
+	u64 options;
+	u64 advertising;
+};
+
+int dpmac_get_link_cfg(struct fsl_mc_io *mc_io,
+		       u32 cmd_flags,
+		       u16 token,
+		       struct dpmac_link_cfg *cfg);
+
+/**
+ * struct dpmac_link_state - DPMAC link configuration request
+ * @rate: Rate in Mbps
+ * @options: Enable/Disable DPMAC link cfg features (bitmap)
+ * @up: Link state
+ * @state_valid: Ignore/Update the state of the link
+ * @supported: Speeds capability of the phy (bitmap)
+ * @advertising: Speeds that are advertised for autoneg (bitmap)
+ */
+struct dpmac_link_state {
+	u32 rate;
+	u64 options;
+	int up;
+	int state_valid;
+	u64 supported;
+	u64 advertising;
+};
+
+int dpmac_set_link_state(struct fsl_mc_io *mc_io,
+			 u32 cmd_flags,
+			 u16 token,
+			 struct dpmac_link_state *link_state);
+
+int dpmac_get_api_version(struct fsl_mc_io *mc_io,
+			  u32 cmd_flags,
+			  u16 *major_ver,
+			  u16 *minor_ver);
+
+#endif /* __FSL_DPMAC_H */
-- 
1.9.1

Re: [PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object

[Andrew Lunn]

> +/**
> + * dpmac_set_link_state() - Set the Ethernet link status
> + * @mc_io:      Pointer to opaque I/O object
> + * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
> + * @token:      Token of DPMAC object
> + * @link_state: Link state configuration
> + *
> + * Return:      '0' on Success; Error code otherwise.
> + */
> +int dpmac_set_link_state(struct fsl_mc_io *mc_io,
> +			 u32 cmd_flags,
> +			 u16 token,
> +			 struct dpmac_link_state *link_state)
> +{
> +	struct dpmac_cmd_set_link_state *cmd_params;
> +	struct fsl_mc_command cmd = { 0 };
> +
> +	/* prepare command */
> +	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_SET_LINK_STATE,
> +					  cmd_flags,
> +					  token);
> +	cmd_params = (struct dpmac_cmd_set_link_state *)cmd.params;
> +	cmd_params->options = cpu_to_le64(link_state->options);
> +	cmd_params->rate = cpu_to_le32(link_state->rate);
> +	dpmac_set_field(cmd_params->state, STATE, link_state->up);
> +	dpmac_set_field(cmd_params->state, STATE_VALID,
> +			link_state->state_valid);
> +	cmd_params->supported = cpu_to_le64(link_state->supported);
> +	cmd_params->advertising = cpu_to_le64(link_state->advertising);

I don't understand what supported and advertising mean in the context
of a MAC. PHY yes, but MAC?

> + * DPMAC link configuration/state options
> + */
> +
> +/**
> + * Enable auto-negotiation
> + */
> +#define DPMAC_LINK_OPT_AUTONEG			BIT_ULL(0)
> +/**
> + * Enable half-duplex mode
> + */
> +#define DPMAC_LINK_OPT_HALF_DUPLEX		BIT_ULL(1)
> +/**
> + * Enable pause frames
> + */
> +#define DPMAC_LINK_OPT_PAUSE			BIT_ULL(2)
> +/**
> + * Enable a-symmetric pause frames
> + */
> +#define DPMAC_LINK_OPT_ASYM_PAUSE		BIT_ULL(3)

So is this to configure the MAC? The MAC can do half duplex, pause,
asym pause? 

But from the previous patch, the PHY cannot do half duplex?

     Andrew

RE: [PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object
> 
> > +/**
> > + * dpmac_set_link_state() - Set the Ethernet link status
> > + * @mc_io:      Pointer to opaque I/O object
> > + * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
> > + * @token:      Token of DPMAC object
> > + * @link_state: Link state configuration
> > + *
> > + * Return:      '0' on Success; Error code otherwise.
> > + */
> > +int dpmac_set_link_state(struct fsl_mc_io *mc_io,
> > +			 u32 cmd_flags,
> > +			 u16 token,
> > +			 struct dpmac_link_state *link_state) {
> > +	struct dpmac_cmd_set_link_state *cmd_params;
> > +	struct fsl_mc_command cmd = { 0 };
> > +
> > +	/* prepare command */
> > +	cmd.header =
> mc_encode_cmd_header(DPMAC_CMDID_SET_LINK_STATE,
> > +					  cmd_flags,
> > +					  token);
> > +	cmd_params = (struct dpmac_cmd_set_link_state *)cmd.params;
> > +	cmd_params->options = cpu_to_le64(link_state->options);
> > +	cmd_params->rate = cpu_to_le32(link_state->rate);
> > +	dpmac_set_field(cmd_params->state, STATE, link_state->up);
> > +	dpmac_set_field(cmd_params->state, STATE_VALID,
> > +			link_state->state_valid);
> > +	cmd_params->supported = cpu_to_le64(link_state->supported);
> > +	cmd_params->advertising = cpu_to_le64(link_state->advertising);
> 
> I don't understand what supported and advertising mean in the context of a
> MAC. PHY yes, but MAC?

It's still in the context of the PHY. I see that the choice of function name is not great but this is done
only to convey what the supported and the advertising modes are to the Ethernet driver.


> 
> > + * DPMAC link configuration/state options */
> > +
> > +/**
> > + * Enable auto-negotiation
> > + */
> > +#define DPMAC_LINK_OPT_AUTONEG			BIT_ULL(0)
> > +/**
> > + * Enable half-duplex mode
> > + */
> > +#define DPMAC_LINK_OPT_HALF_DUPLEX		BIT_ULL(1)
> > +/**
> > + * Enable pause frames
> > + */
> > +#define DPMAC_LINK_OPT_PAUSE			BIT_ULL(2)
> > +/**
> > + * Enable a-symmetric pause frames
> > + */
> > +#define DPMAC_LINK_OPT_ASYM_PAUSE		BIT_ULL(3)
> 
> So is this to configure the MAC? The MAC can do half duplex, pause, asym
> pause?
> 
> But from the previous patch, the PHY cannot do half duplex?
> 
>      Andrew

As stated in the previous reply, the MAC can do pause, asym pause but not half duplex or EEE.
The DPMAC_LINK_OPT_HALF_DUPLEX bit is just a leftover and can be removed.

--
Ioana

Re: [PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object

[Andrew Lunn]

On Fri, Jun 14, 2019 at 02:06:05PM +0000, Ioana Ciornei wrote:
> > Subject: Re: [PATCH RFC 3/6] dpaa2-mac: add MC API for the DPMAC object
> > 
> > > +/**
> > > + * dpmac_set_link_state() - Set the Ethernet link status
> > > + * @mc_io:      Pointer to opaque I/O object
> > > + * @cmd_flags:  Command flags; one or more of 'MC_CMD_FLAG_'
> > > + * @token:      Token of DPMAC object
> > > + * @link_state: Link state configuration
> > > + *
> > > + * Return:      '0' on Success; Error code otherwise.
> > > + */
> > > +int dpmac_set_link_state(struct fsl_mc_io *mc_io,
> > > +			 u32 cmd_flags,
> > > +			 u16 token,
> > > +			 struct dpmac_link_state *link_state) {
> > > +	struct dpmac_cmd_set_link_state *cmd_params;
> > > +	struct fsl_mc_command cmd = { 0 };
> > > +
> > > +	/* prepare command */
> > > +	cmd.header =
> > mc_encode_cmd_header(DPMAC_CMDID_SET_LINK_STATE,
> > > +					  cmd_flags,
> > > +					  token);
> > > +	cmd_params = (struct dpmac_cmd_set_link_state *)cmd.params;
> > > +	cmd_params->options = cpu_to_le64(link_state->options);
> > > +	cmd_params->rate = cpu_to_le32(link_state->rate);
> > > +	dpmac_set_field(cmd_params->state, STATE, link_state->up);
> > > +	dpmac_set_field(cmd_params->state, STATE_VALID,
> > > +			link_state->state_valid);
> > > +	cmd_params->supported = cpu_to_le64(link_state->supported);
> > > +	cmd_params->advertising = cpu_to_le64(link_state->advertising);
> > 
> > I don't understand what supported and advertising mean in the context of a
> > MAC. PHY yes, but MAC?
> 
> It's still in the context of the PHY.

If this is for the PHY why are you not using DPNI? That is the object
which represents the PHY.

> As stated in the previous reply, the MAC can do pause, asym pause
> but not half duplex or EEE.

I'm very surprised it cannot do EEE! I hope the firmware is getting
the auto-neg advertisement correct.

   Andrew

[PATCH RFC 4/6] dpaa2-mac: add initial driver

[Ioana Ciornei]

The dpaa2-mac driver binds to DPAA2 DPMAC objects, dynamically
discovered on the fsl-mc bus. It acts as a proxy between the PHY
management layer and the MC firmware, delivering any configuration
changes to the firmware and also setting any new configuration requested
though PHYLINK.

A in-depth view of the software architecture and the implementation can
be found in
'Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst'.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 MAINTAINERS                                      |   7 +
 drivers/net/ethernet/freescale/dpaa2/Kconfig     |  13 +
 drivers/net/ethernet/freescale/dpaa2/Makefile    |   2 +
 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 541 +++++++++++++++++++++++
 4 files changed, 563 insertions(+)
 create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dd247a059889..a024ab2b2548 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4929,6 +4929,13 @@ S:	Maintained
 F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp*
 F:	drivers/net/ethernet/freescale/dpaa2/dprtc*
 
+DPAA2 MAC DRIVER
+M:	Ioana Ciornei <ioana.ciornei@nxp.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-mac*
+F:	drivers/net/ethernet/freescale/dpaa2/dpmac*
+
 DPT_I2O SCSI RAID DRIVER
 M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
 L:	linux-scsi@vger.kernel.org
diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig b/drivers/net/ethernet/freescale/dpaa2/Kconfig
index 8bd384720f80..4ffa666c0a43 100644
--- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
+++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
@@ -16,3 +16,16 @@ config FSL_DPAA2_PTP_CLOCK
 	help
 	  This driver adds support for using the DPAA2 1588 timer module
 	  as a PTP clock.
+
+config FSL_DPAA2_MAC
+	tristate "DPAA2 MAC / PHY proxy interface"
+	depends on FSL_MC_BUS
+	select MDIO_BUS_MUX_MMIOREG
+	select FSL_XGMAC_MDIO
+	select PHYLINK
+	help
+	  Prototype driver for DPAA2 MAC / PHY interface object.
+	  This driver works as a proxy between PHYLINK including phy drivers and
+	  the MC firmware.  It receives updates on link state changes from PHYLINK
+	  and forwards them to MC and receives interrupt from MC whenever a
+	  request is made to change the link state or configuration.
diff --git a/drivers/net/ethernet/freescale/dpaa2/Makefile b/drivers/net/ethernet/freescale/dpaa2/Makefile
index d1e78cdd512f..e96386ab23ea 100644
--- a/drivers/net/ethernet/freescale/dpaa2/Makefile
+++ b/drivers/net/ethernet/freescale/dpaa2/Makefile
@@ -5,10 +5,12 @@
 
 obj-$(CONFIG_FSL_DPAA2_ETH)		+= fsl-dpaa2-eth.o
 obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK)	+= fsl-dpaa2-ptp.o
+obj-$(CONFIG_FSL_DPAA2_MAC)		+= fsl-dpaa2-mac.o
 
 fsl-dpaa2-eth-objs	:= dpaa2-eth.o dpaa2-ethtool.o dpni.o
 fsl-dpaa2-eth-${CONFIG_DEBUG_FS} += dpaa2-eth-debugfs.o
 fsl-dpaa2-ptp-objs	:= dpaa2-ptp.o dprtc.o
+fsl-dpaa2-mac-objs	:= dpaa2-mac.o dpmac.o
 
 # Needed by the tracing framework
 CFLAGS_dpaa2-eth.o := -I$(src)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
new file mode 100644
index 000000000000..145ab4771788
--- /dev/null
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -0,0 +1,541 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/* Copyright 2015 Freescale Semiconductor Inc.
+ * Copyright 2018-2019 NXP
+ */
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/msi.h>
+#include <linux/rtnetlink.h>
+#include <linux/if_vlan.h>
+
+#include <net/netlink.h>
+#include <uapi/linux/if_bridge.h>
+
+#include <linux/of.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/phylink.h>
+#include <linux/notifier.h>
+
+#include <linux/fsl/mc.h>
+
+#include "dpmac.h"
+#include "dpmac-cmd.h"
+
+#define to_dpaa2_mac_priv(phylink_config) \
+	container_of(config, struct dpaa2_mac_priv, phylink_config)
+
+struct dpaa2_mac_priv {
+	struct fsl_mc_device *mc_dev;
+	struct dpmac_attr attr;
+	struct dpmac_link_state state;
+	u16 dpmac_ver_major;
+	u16 dpmac_ver_minor;
+
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
+	struct ethtool_link_ksettings kset;
+};
+
+static phy_interface_t phy_mode(enum dpmac_eth_if eth_if)
+{
+	switch (eth_if) {
+	case DPMAC_ETH_IF_RGMII:
+		return PHY_INTERFACE_MODE_RGMII;
+	case DPMAC_ETH_IF_XFI:
+		return PHY_INTERFACE_MODE_10GKR;
+	case DPMAC_ETH_IF_USXGMII:
+		return PHY_INTERFACE_MODE_USXGMII;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cmp_dpmac_ver(struct dpaa2_mac_priv *priv,
+			 u16 ver_major, u16 ver_minor)
+{
+	if (priv->dpmac_ver_major == ver_major)
+		return priv->dpmac_ver_minor - ver_minor;
+	return priv->dpmac_ver_major - ver_major;
+}
+
+struct dpaa2_mac_link_mode_map {
+	u64 dpmac_lm;
+	enum ethtool_link_mode_bit_indices ethtool_lm;
+};
+
+static const struct dpaa2_mac_link_mode_map dpaa2_mac_lm_map[] = {
+	{DPMAC_ADVERTISED_10BASET_FULL, ETHTOOL_LINK_MODE_10baseT_Full_BIT},
+	{DPMAC_ADVERTISED_100BASET_FULL, ETHTOOL_LINK_MODE_100baseT_Full_BIT},
+	{DPMAC_ADVERTISED_1000BASET_FULL, ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
+	{DPMAC_ADVERTISED_10000BASET_FULL, ETHTOOL_LINK_MODE_10000baseT_Full_BIT},
+	{DPMAC_ADVERTISED_AUTONEG, ETHTOOL_LINK_MODE_Autoneg_BIT},
+};
+
+static void link_mode_phydev2dpmac(unsigned long *phydev_lm,
+				   u64 *dpmac_lm)
+{
+	enum ethtool_link_mode_bit_indices link_mode;
+	int i;
+
+	*dpmac_lm = 0;
+	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
+		link_mode = dpaa2_mac_lm_map[i].ethtool_lm;
+		if (linkmode_test_bit(link_mode, phydev_lm))
+			*dpmac_lm |= dpaa2_mac_lm_map[i].dpmac_lm;
+	}
+}
+
+static void dpaa2_mac_ksettings_change(struct dpaa2_mac_priv *priv)
+{
+	struct fsl_mc_device *mc_dev = priv->mc_dev;
+	struct dpmac_link_cfg link_cfg = { 0 };
+	int err, i;
+
+	err = dpmac_get_link_cfg(mc_dev->mc_io, 0,
+				 mc_dev->mc_handle,
+				 &link_cfg);
+
+	if (err) {
+		dev_err(&mc_dev->dev, "dpmac_get_link_cfg() = %d\n", err);
+		return;
+	}
+
+	phylink_ethtool_ksettings_get(priv->phylink, &priv->kset);
+
+	priv->kset.base.speed = link_cfg.rate;
+	priv->kset.base.duplex = !!(link_cfg.options & DPMAC_LINK_OPT_HALF_DUPLEX);
+
+	ethtool_link_ksettings_zero_link_mode(&priv->kset, advertising);
+	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
+		if (link_cfg.advertising & dpaa2_mac_lm_map[i].dpmac_lm)
+			__set_bit(dpaa2_mac_lm_map[i].ethtool_lm,
+				  priv->kset.link_modes.advertising);
+	}
+
+	if (link_cfg.options & DPMAC_LINK_OPT_AUTONEG) {
+		priv->kset.base.autoneg = AUTONEG_ENABLE;
+		__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+			  priv->kset.link_modes.advertising);
+	} else {
+		priv->kset.base.autoneg = AUTONEG_DISABLE;
+		__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+			    priv->kset.link_modes.advertising);
+	}
+
+	phylink_ethtool_ksettings_set(priv->phylink, &priv->kset);
+}
+
+static irqreturn_t dpaa2_mac_irq_handler(int irq_num, void *arg)
+{
+	struct device *dev = arg;
+	struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+	struct dpaa2_mac_priv *priv = dev_get_drvdata(dev);
+	u32 status;
+	int err;
+
+	err = dpmac_get_irq_status(mc_dev->mc_io, 0, mc_dev->mc_handle,
+				   DPMAC_IRQ_INDEX, &status);
+	if (unlikely(err || !status))
+		return IRQ_NONE;
+
+	rtnl_lock();
+	if (status & DPMAC_IRQ_EVENT_LINK_CFG_REQ)
+		dpaa2_mac_ksettings_change(priv);
+
+	if (status & DPMAC_IRQ_EVENT_LINK_UP_REQ)
+		phylink_start(priv->phylink);
+
+	if (status & DPMAC_IRQ_EVENT_LINK_DOWN_REQ)
+		phylink_stop(priv->phylink);
+	rtnl_unlock();
+
+	dpmac_clear_irq_status(mc_dev->mc_io, 0, mc_dev->mc_handle,
+			       DPMAC_IRQ_INDEX, status);
+
+	return IRQ_HANDLED;
+}
+
+static int dpaa2_mac_setup_irqs(struct fsl_mc_device *mc_dev)
+{
+	struct device *dev = &mc_dev->dev;
+	struct fsl_mc_device_irq *irq;
+	u32 irq_mask;
+	int err;
+
+	err = fsl_mc_allocate_irqs(mc_dev);
+	if (err) {
+		dev_err(dev, "fsl_mc_allocate_irqs() = %d\n", err);
+		return err;
+	}
+
+	irq = mc_dev->irqs[0];
+	err = devm_request_threaded_irq(dev, irq->msi_desc->irq,
+					NULL, &dpaa2_mac_irq_handler,
+					IRQF_NO_SUSPEND | IRQF_ONESHOT,
+					dev_name(&mc_dev->dev), dev);
+	if (err) {
+		dev_err(dev, "devm_request_threaded_irq() = %d\n", err);
+		goto free_irq;
+	}
+
+	irq_mask = DPMAC_IRQ_EVENT_LINK_CFG_REQ |
+		   DPMAC_IRQ_EVENT_LINK_CHANGED |
+		   DPMAC_IRQ_EVENT_LINK_UP_REQ |
+		   DPMAC_IRQ_EVENT_LINK_DOWN_REQ;
+
+	err = dpmac_set_irq_mask(mc_dev->mc_io, 0, mc_dev->mc_handle,
+				 DPMAC_IRQ_INDEX, irq_mask);
+	if (err) {
+		dev_err(dev, "dpmac_set_irq_mask() = %d\n", err);
+		goto free_irq;
+	}
+	err = dpmac_set_irq_enable(mc_dev->mc_io, 0, mc_dev->mc_handle,
+				   DPMAC_IRQ_INDEX, 1);
+	if (err) {
+		dev_err(dev, "dpmac_set_irq_enable() = %d\n", err);
+		goto free_irq;
+	}
+
+	return 0;
+
+free_irq:
+	fsl_mc_free_irqs(mc_dev);
+
+	return err;
+}
+
+static void dpaa2_mac_teardown_irqs(struct fsl_mc_device *mc_dev)
+{
+	int err;
+
+	err = dpmac_set_irq_enable(mc_dev->mc_io, 0, mc_dev->mc_handle,
+				   DPMAC_IRQ_INDEX, 0);
+	if (err)
+		dev_err(&mc_dev->dev, "dpmac_set_irq_enable err %d\n", err);
+
+	fsl_mc_free_irqs(mc_dev);
+}
+
+static struct device_node *of_find_dpmac_node(struct device *dev, u16 dpmac_id)
+{
+	struct device_node *dpmacs, *dpmac = NULL;
+	struct device_node *mc_node = dev->of_node;
+	u32 id;
+	int err;
+
+	dpmacs = of_find_node_by_name(mc_node, "dpmacs");
+	if (!dpmacs) {
+		dev_err(dev, "No dpmacs subnode in device-tree\n");
+		return NULL;
+	}
+
+	while ((dpmac = of_get_next_child(dpmacs, dpmac))) {
+		err = of_property_read_u32(dpmac, "reg", &id);
+		if (err)
+			continue;
+		if (id == dpmac_id)
+			return dpmac;
+	}
+
+	return NULL;
+}
+
+static void dpaa2_mac_validate(struct phylink_config *config,
+			       unsigned long *supported,
+			       struct phylink_link_state *state)
+{
+	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
+	struct dpmac_link_state *dpmac_state = &priv->state;
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	phylink_set(mask, Autoneg);
+	phylink_set_port_modes(mask);
+
+	switch (state->interface) {
+	case PHY_INTERFACE_MODE_10GKR:
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Full);
+		phylink_set(mask, 1000baseT_Full);
+		phylink_set(mask, 10000baseT_Full);
+		break;
+	case PHY_INTERFACE_MODE_QSGMII:
+	case PHY_INTERFACE_MODE_RGMII:
+	case PHY_INTERFACE_MODE_RGMII_ID:
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Full);
+		phylink_set(mask, 1000baseT_Full);
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+		phylink_set(mask, 10baseT_Full);
+		phylink_set(mask, 100baseT_Full);
+		phylink_set(mask, 1000baseT_Full);
+		phylink_set(mask, 10000baseT_Full);
+		break;
+	default:
+		goto empty_set;
+	}
+
+	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
+	bitmap_and(state->advertising, state->advertising, mask,
+		   __ETHTOOL_LINK_MODE_MASK_NBITS);
+
+	link_mode_phydev2dpmac(supported, &dpmac_state->supported);
+	link_mode_phydev2dpmac(state->advertising, &dpmac_state->advertising);
+
+	return;
+
+empty_set:
+	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+}
+
+static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
+			     const struct phylink_link_state *state)
+{
+	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
+	struct dpmac_link_state *dpmac_state = &priv->state;
+	struct device *dev = &priv->mc_dev->dev;
+	int err;
+
+	if (state->speed == SPEED_UNKNOWN && state->duplex == DUPLEX_UNKNOWN)
+		return;
+
+	dpmac_state->up = !!state->link;
+	if (dpmac_state->up) {
+		dpmac_state->rate = state->speed;
+
+		if (!state->duplex)
+			dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX;
+		else
+			dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX;
+
+		if (state->an_enabled)
+			dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG;
+		else
+			dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG;
+	}
+
+	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
+				   priv->mc_dev->mc_handle, dpmac_state);
+	if (err)
+		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
+}
+
+static void dpaa2_mac_link_up(struct phylink_config *config, unsigned int mode,
+			      phy_interface_t interface, struct phy_device *phy)
+{
+	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
+	struct dpmac_link_state *dpmac_state = &priv->state;
+	struct device *dev = &priv->mc_dev->dev;
+	int err;
+
+	dpmac_state->up = 1;
+	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
+				   priv->mc_dev->mc_handle, dpmac_state);
+	if (err)
+		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
+}
+
+static void dpaa2_mac_link_down(struct phylink_config *config,
+				unsigned int mode,
+				phy_interface_t interface)
+{
+	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
+	struct dpmac_link_state *dpmac_state = &priv->state;
+	struct device *dev = &priv->mc_dev->dev;
+	int err;
+
+	dpmac_state->up = 0;
+
+	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
+				   priv->mc_dev->mc_handle, dpmac_state);
+	if (err)
+		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
+}
+
+static const struct phylink_mac_ops dpaa2_mac_phylink_ops = {
+	.validate = dpaa2_mac_validate,
+	.mac_config = dpaa2_mac_config,
+	.mac_link_up = dpaa2_mac_link_up,
+	.mac_link_down = dpaa2_mac_link_down,
+};
+
+static int dpaa2_mac_probe(struct fsl_mc_device *mc_dev)
+{
+	struct dpaa2_mac_priv *priv = NULL;
+	struct device_node *dpmac_node;
+	struct phylink *phylink;
+	int if_mode, err = 0;
+	struct device *dev;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev = &mc_dev->dev;
+	priv->mc_dev = mc_dev;
+	dev_set_drvdata(dev, priv);
+
+	/* We may need to issue MC commands while in atomic context */
+	err = fsl_mc_portal_allocate(mc_dev, FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
+				     &mc_dev->mc_io);
+	if (err || !mc_dev->mc_io) {
+		dev_dbg(dev, "fsl_mc_portal_allocate error: %d\n", err);
+		err = -EPROBE_DEFER;
+		goto err_exit;
+	}
+
+	err = dpmac_open(mc_dev->mc_io, 0, mc_dev->obj_desc.id,
+			 &mc_dev->mc_handle);
+	if (err || !mc_dev->mc_handle) {
+		dev_err(dev, "dpmac_open error: %d\n", err);
+		err = -ENODEV;
+		goto err_free_mcp;
+	}
+
+	err = dpmac_get_api_version(mc_dev->mc_io, 0, &priv->dpmac_ver_major,
+				    &priv->dpmac_ver_minor);
+	if (err) {
+		dev_err(dev, "dpmac_get_api_version failed\n");
+		goto err_version;
+	}
+
+	if (cmp_dpmac_ver(priv, DPMAC_VER_MAJOR, DPMAC_VER_MINOR) < 0) {
+		dev_err(dev, "DPMAC version %u.%u lower than supported %u.%u\n",
+			priv->dpmac_ver_major, priv->dpmac_ver_minor,
+			DPMAC_VER_MAJOR, DPMAC_VER_MINOR);
+		err = -ENOTSUPP;
+		goto err_version;
+	}
+
+	err = dpmac_get_attributes(mc_dev->mc_io, 0,
+				   mc_dev->mc_handle, &priv->attr);
+	if (err) {
+		dev_err(dev, "dpmac_get_attributes err %d\n", err);
+		err = -EINVAL;
+		goto err_close;
+	}
+
+	if (priv->attr.link_type == DPMAC_LINK_TYPE_FIXED) {
+		dev_err(dev, "will not be probed because it's listed as TYPE_FIXED\n");
+		err = -EINVAL;
+		goto err_close;
+	}
+
+	/* Look up the DPMAC node in the device-tree. */
+	dpmac_node = of_find_dpmac_node(dev, priv->attr.id);
+	if (!dpmac_node) {
+		dev_err(dev, "No dpmac@%d subnode found.\n", priv->attr.id);
+		err = -ENODEV;
+		goto err_close;
+	}
+
+	err = dpaa2_mac_setup_irqs(mc_dev);
+	if (err) {
+		err = -EFAULT;
+		goto err_close;
+	}
+
+	/* Get the interface mode from the dpmac of node or
+	 * from the MC attributes
+	 */
+	if_mode = of_get_phy_mode(dpmac_node);
+	if (if_mode >= 0) {
+		dev_dbg(dev, "\tusing if mode %s for eth_if %d\n",
+			phy_modes(if_mode), priv->attr.eth_if);
+		goto operation_mode;
+	}
+
+	if_mode = phy_mode(priv->attr.eth_if);
+	if (if_mode >= 0) {
+		dev_dbg(dev, "\tusing if mode %s for eth_if %d\n",
+			phy_modes(if_mode), priv->attr.eth_if);
+	} else {
+		dev_err(dev, "Unexpected interface mode %d\n",
+			priv->attr.eth_if);
+		err = -EINVAL;
+		goto err_no_if_mode;
+	}
+
+operation_mode:
+	priv->phylink_config.dev = dev;
+	priv->phylink_config.type = PHYLINK_DEV;
+
+	phylink = phylink_create(&priv->phylink_config,
+				 of_fwnode_handle(dpmac_node), if_mode,
+				 &dpaa2_mac_phylink_ops);
+	if (IS_ERR(phylink)) {
+		err = PTR_ERR(phylink);
+		goto err_phylink_create;
+	}
+	priv->phylink = phylink;
+
+	err = phylink_of_phy_connect(priv->phylink, dpmac_node, 0);
+	if (err) {
+		pr_err("phylink_of_phy_connect() = %d\n", err);
+		goto err_phylink_connect;
+	}
+
+	return 0;
+
+err_phylink_connect:
+	phylink_destroy(priv->phylink);
+err_phylink_create:
+err_no_if_mode:
+	dpaa2_mac_teardown_irqs(mc_dev);
+err_version:
+err_close:
+	dpmac_close(mc_dev->mc_io, 0, mc_dev->mc_handle);
+err_free_mcp:
+	fsl_mc_portal_free(mc_dev->mc_io);
+err_exit:
+	return err;
+}
+
+static int dpaa2_mac_remove(struct fsl_mc_device *mc_dev)
+{
+	struct device *dev = &mc_dev->dev;
+	struct dpaa2_mac_priv *priv = dev_get_drvdata(dev);
+
+	/* PHY teardown */
+	phylink_stop(priv->phylink);
+	phylink_disconnect_phy(priv->phylink);
+	phylink_destroy(priv->phylink);
+
+	/* free resources */
+	dpaa2_mac_teardown_irqs(priv->mc_dev);
+	dpmac_close(priv->mc_dev->mc_io, 0, priv->mc_dev->mc_handle);
+	fsl_mc_portal_free(priv->mc_dev->mc_io);
+
+	kfree(priv);
+	dev_set_drvdata(dev, NULL);
+
+	return 0;
+}
+
+static const struct fsl_mc_device_id dpaa2_mac_match_id_table[] = {
+	{
+		.vendor = FSL_MC_VENDOR_FREESCALE,
+		.obj_type = "dpmac",
+	},
+	{ .vendor = 0x0 }
+};
+MODULE_DEVICE_TABLE(fslmc, dpaa2_mac_match_id_table);
+
+static struct fsl_mc_driver dpaa2_mac_drv = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.owner = THIS_MODULE,
+	},
+	.probe = dpaa2_mac_probe,
+	.remove = dpaa2_mac_remove,
+	.match_id_table = dpaa2_mac_match_id_table,
+};
+
+module_fsl_mc_driver(dpaa2_mac_drv);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DPAA2 PHY proxy interface driver");
-- 
1.9.1

Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Andrew Lunn]

> +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if)
> +{
> +	switch (eth_if) {
> +	case DPMAC_ETH_IF_RGMII:
> +		return PHY_INTERFACE_MODE_RGMII;

So the MAC cannot insert RGMII delays? I didn't see anything in the
PHY object about configuring the delays. Does the PCB need to add
delays via squiggles in the tracks?

> +static void dpaa2_mac_validate(struct phylink_config *config,
> +			       unsigned long *supported,
> +			       struct phylink_link_state *state)
> +{
> +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> +	struct dpmac_link_state *dpmac_state = &priv->state;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set_port_modes(mask);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_10GKR:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 10000baseT_Full);
> +		break;
> +	case PHY_INTERFACE_MODE_QSGMII:
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 10000baseT_Full);
> +		break;
> +	default:
> +		goto empty_set;
> +	}

I think this is wrong. This is about validating what the MAC can
do. The state->interface should not matter. The PHY will indicate what
interface mode should be used when auto-neg has completed. The MAC is
then expected to change its interface to fit.

But lets see what Russell says.

> +static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
> +			     const struct phylink_link_state *state)
> +{
> +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> +	struct dpmac_link_state *dpmac_state = &priv->state;
> +	struct device *dev = &priv->mc_dev->dev;
> +	int err;
> +
> +	if (state->speed == SPEED_UNKNOWN && state->duplex == DUPLEX_UNKNOWN)
> +		return;
> +
> +	dpmac_state->up = !!state->link;
> +	if (dpmac_state->up) {
> +		dpmac_state->rate = state->speed;
> +
> +		if (!state->duplex)
> +			dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX;
> +		else
> +			dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX;
> +
> +		if (state->an_enabled)
> +			dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG;
> +		else
> +			dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG;

As Russell pointed out, this auto-neg is only valid in a limited
context. The MAC generally does not perform auto-neg. The MAC is only
involved in auto-neg when inband signalling is used between the MAC
and PHY in 802.3z.

As the name says, dpaa2_mac_config is about the MAC.

   Andrew

Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 03:42:23AM +0200, Andrew Lunn wrote:
> > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if)
> > +{
> > +	switch (eth_if) {
> > +	case DPMAC_ETH_IF_RGMII:
> > +		return PHY_INTERFACE_MODE_RGMII;
> 
> So the MAC cannot insert RGMII delays? I didn't see anything in the
> PHY object about configuring the delays. Does the PCB need to add
> delays via squiggles in the tracks?
> 
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state)
> > +{
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set_port_modes(mask);
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_10GKR:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;

How does 10GBASE-KR mode support these lesser speeds - 802.3 makes no
provision for slower speeds for a 10GBASE-KR link, it is a fixed speed
link.  I don't see any other possible phy interface mode supported that
would allow for the 1G, 100M and 10M speeds (i.o.w. SGMII).  If SGMII
is not supported, then how do you expect these other speeds to work?

Does your PHY do speed conversion - if so, we need to come up with a
much better way of handling that (we need phylib to indicate that the
PHY is so capable.)

> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	default:
> > +		goto empty_set;
> > +	}
> 
> I think this is wrong. This is about validating what the MAC can
> do. The state->interface should not matter. The PHY will indicate what
> interface mode should be used when auto-neg has completed. The MAC is
> then expected to change its interface to fit.

The question is whether a PHY/MAC wired up using a particular topology
can switch between other interface types.

For example, SGMII, 802.3z and 10GBASE-KR all use a single serdes lane
which means that as long as both ends are configured for the same
protocol, the result should work.  As an example, Marvell 88x3310 PHYs
switch between these three modes depending on the negotiated speed.

So, this is more to do with saying what the MAC can support with a
particular wiring topology rather than the strict PHY interface type.

Take mvneta:

        /* Half-duplex at speeds higher than 100Mbit is unsupported */
        if (pp->comphy || state->interface != PHY_INTERFACE_MODE_2500BASEX) {
                phylink_set(mask, 1000baseT_Full);
                phylink_set(mask, 1000baseX_Full);
        }
        if (pp->comphy || state->interface == PHY_INTERFACE_MODE_2500BASEX) {
                phylink_set(mask, 2500baseX_Full);
        }

If we have a comphy, we can switch the MAC speed between 1G and 2.5G
here, so we allow both 1G and 2.5G to be set in the supported mask.

If we do not have a comphy, we are not able to change the MAC speed
at runtime, so we are more restrictive with the support mask.

> > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
> > +			     const struct phylink_link_state *state)
> > +{
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	if (state->speed == SPEED_UNKNOWN && state->duplex == DUPLEX_UNKNOWN)
> > +		return;

As I've already pointed out, state->speed and state->duplex are only
valid for fixed-link and PHY setups.  They are not valid for SGMII
and 802.3z, which use in-band configuration/negotiation, but then in
your validate callback, it seems you don't support these.

Since many SFP modules require SGMII and 802.3z, I wonder how this
is going to work.

> > +
> > +	dpmac_state->up = !!state->link;
> > +	if (dpmac_state->up) {

No, whether the link is up or down is not a concern for this function,
and it's not guaranteed to be valid here.

I can see I made a bad choice when designing this interface - it was
simpler to have just one structure for reading the link state from the
MAC and setting the configuration, because the two were very similar.

I can see I should've made them separate and specific to each call
(which would necessitate additional code, but for the sake of enforcing
correct programming interface usage, it would've been the right thing.)

> > +		dpmac_state->rate = state->speed;
> > +
> > +		if (!state->duplex)
> > +			dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX;
> > +		else
> > +			dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > +
> > +		if (state->an_enabled)
> > +			dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG;
> > +		else
> > +			dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG;
> 
> As Russell pointed out, this auto-neg is only valid in a limited
> context. The MAC generally does not perform auto-neg. The MAC is only
> involved in auto-neg when inband signalling is used between the MAC
> and PHY in 802.3z.

or SGMII.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> 
> On Fri, Jun 14, 2019 at 03:42:23AM +0200, Andrew Lunn wrote:
> > > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > > +	switch (eth_if) {
> > > +	case DPMAC_ETH_IF_RGMII:
> > > +		return PHY_INTERFACE_MODE_RGMII;
> >
> > So the MAC cannot insert RGMII delays? I didn't see anything in the
> > PHY object about configuring the delays. Does the PCB need to add
> > delays via squiggles in the tracks?
> >
> > > +static void dpaa2_mac_validate(struct phylink_config *config,
> > > +			       unsigned long *supported,
> > > +			       struct phylink_link_state *state) {
> > > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > > +
> > > +	phylink_set(mask, Autoneg);
> > > +	phylink_set_port_modes(mask);
> > > +
> > > +	switch (state->interface) {
> > > +	case PHY_INTERFACE_MODE_10GKR:
> > > +		phylink_set(mask, 10baseT_Full);
> > > +		phylink_set(mask, 100baseT_Full);
> > > +		phylink_set(mask, 1000baseT_Full);
> > > +		phylink_set(mask, 10000baseT_Full);
> > > +		break;
> 
> How does 10GBASE-KR mode support these lesser speeds - 802.3 makes no
> provision for slower speeds for a 10GBASE-KR link, it is a fixed speed link.  I
> don't see any other possible phy interface mode supported that would allow
> for the 1G, 100M and 10M speeds (i.o.w. SGMII).  If SGMII is not supported,
> then how do you expect these other speeds to work?
> 
> Does your PHY do speed conversion - if so, we need to come up with a much
> better way of handling that (we need phylib to indicate that the PHY is so
> capable.)

These are PHYs connected using an XFI interface that indeed can operate at lower
speeds and are capable of rate adaptation using pause frames.

Also, I've used PHY_INTERFACE_MODE_10GKR since a dedicated XFI mode is not available.
 
> 
> > > +	case PHY_INTERFACE_MODE_QSGMII:
> > > +	case PHY_INTERFACE_MODE_RGMII:
> > > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > > +		phylink_set(mask, 10baseT_Full);
> > > +		phylink_set(mask, 100baseT_Full);
> > > +		phylink_set(mask, 1000baseT_Full);
> > > +		break;
> > > +	case PHY_INTERFACE_MODE_USXGMII:
> > > +		phylink_set(mask, 10baseT_Full);
> > > +		phylink_set(mask, 100baseT_Full);
> > > +		phylink_set(mask, 1000baseT_Full);
> > > +		phylink_set(mask, 10000baseT_Full);
> > > +		break;
> > > +	default:
> > > +		goto empty_set;
> > > +	}
> >
> > I think this is wrong. This is about validating what the MAC can do.
> > The state->interface should not matter. The PHY will indicate what
> > interface mode should be used when auto-neg has completed. The MAC is
> > then expected to change its interface to fit.
> 
> The question is whether a PHY/MAC wired up using a particular topology can
> switch between other interface types.
> 
> For example, SGMII, 802.3z and 10GBASE-KR all use a single serdes lane
> which means that as long as both ends are configured for the same protocol,
> the result should work.  As an example, Marvell 88x3310 PHYs switch
> between these three modes depending on the negotiated speed.
> 
> So, this is more to do with saying what the MAC can support with a particular
> wiring topology rather than the strict PHY interface type.
> 
> Take mvneta:
> 
>         /* Half-duplex at speeds higher than 100Mbit is unsupported */
>         if (pp->comphy || state->interface !=
> PHY_INTERFACE_MODE_2500BASEX) {
>                 phylink_set(mask, 1000baseT_Full);
>                 phylink_set(mask, 1000baseX_Full);
>         }
>         if (pp->comphy || state->interface ==
> PHY_INTERFACE_MODE_2500BASEX) {
>                 phylink_set(mask, 2500baseX_Full);
>         }
> 
> If we have a comphy, we can switch the MAC speed between 1G and 2.5G
> here, so we allow both 1G and 2.5G to be set in the supported mask.
> 
> If we do not have a comphy, we are not able to change the MAC speed at
> runtime, so we are more restrictive with the support mask.
> 
> > > +static void dpaa2_mac_config(struct phylink_config *config, unsigned
> int mode,
> > > +			     const struct phylink_link_state *state) {
> > > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > > +	struct device *dev = &priv->mc_dev->dev;
> > > +	int err;
> > > +
> > > +	if (state->speed == SPEED_UNKNOWN && state->duplex ==
> DUPLEX_UNKNOWN)
> > > +		return;
> 
> As I've already pointed out, state->speed and state->duplex are only valid
> for fixed-link and PHY setups.  They are not valid for SGMII and 802.3z, which
> use in-band configuration/negotiation, but then in your validate callback, it
> seems you don't support these.
> 
> Since many SFP modules require SGMII and 802.3z, I wonder how this is
> going to work.
> 
> > > +
> > > +	dpmac_state->up = !!state->link;
> > > +	if (dpmac_state->up) {
> 
> No, whether the link is up or down is not a concern for this function, and it's
> not guaranteed to be valid here.

Agreed. We can just remove that.

> 
> I can see I made a bad choice when designing this interface - it was simpler to
> have just one structure for reading the link state from the MAC and setting
> the configuration, because the two were very similar.
> 
> I can see I should've made them separate and specific to each call (which
> would necessitate additional code, but for the sake of enforcing correct
> programming interface usage, it would've been the right thing.)
> 
> > > +		dpmac_state->rate = state->speed;
> > > +
> > > +		if (!state->duplex)
> > > +			dpmac_state->options |=
> DPMAC_LINK_OPT_HALF_DUPLEX;
> > > +		else
> > > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > > +
> > > +		if (state->an_enabled)
> > > +			dpmac_state->options |=
> DPMAC_LINK_OPT_AUTONEG;
> > > +		else
> > > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_AUTONEG;
> >
> > As Russell pointed out, this auto-neg is only valid in a limited
> > context. The MAC generally does not perform auto-neg. The MAC is only
> > involved in auto-neg when inband signalling is used between the MAC
> > and PHY in 802.3z.
> 
> or SGMII.
> 
> --

Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 04:54:56PM +0000, Ioana Ciornei wrote:
> > Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> > 
> > On Fri, Jun 14, 2019 at 03:42:23AM +0200, Andrew Lunn wrote:
> > > > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > > > +	switch (eth_if) {
> > > > +	case DPMAC_ETH_IF_RGMII:
> > > > +		return PHY_INTERFACE_MODE_RGMII;
> > >
> > > So the MAC cannot insert RGMII delays? I didn't see anything in the
> > > PHY object about configuring the delays. Does the PCB need to add
> > > delays via squiggles in the tracks?
> > >
> > > > +static void dpaa2_mac_validate(struct phylink_config *config,
> > > > +			       unsigned long *supported,
> > > > +			       struct phylink_link_state *state) {
> > > > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > > > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > > > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > > > +
> > > > +	phylink_set(mask, Autoneg);
> > > > +	phylink_set_port_modes(mask);
> > > > +
> > > > +	switch (state->interface) {
> > > > +	case PHY_INTERFACE_MODE_10GKR:
> > > > +		phylink_set(mask, 10baseT_Full);
> > > > +		phylink_set(mask, 100baseT_Full);
> > > > +		phylink_set(mask, 1000baseT_Full);
> > > > +		phylink_set(mask, 10000baseT_Full);
> > > > +		break;
> > 
> > How does 10GBASE-KR mode support these lesser speeds - 802.3 makes no
> > provision for slower speeds for a 10GBASE-KR link, it is a fixed speed link.  I
> > don't see any other possible phy interface mode supported that would allow
> > for the 1G, 100M and 10M speeds (i.o.w. SGMII).  If SGMII is not supported,
> > then how do you expect these other speeds to work?
> > 
> > Does your PHY do speed conversion - if so, we need to come up with a much
> > better way of handling that (we need phylib to indicate that the PHY is so
> > capable.)
> 
> These are PHYs connected using an XFI interface that indeed can operate at lower
> speeds and are capable of rate adaptation using pause frames.
> 
> Also, I've used PHY_INTERFACE_MODE_10GKR since a dedicated XFI mode is not available.

XFI is basically what that interface mode for - there's a bunch of
different descriptions for it which seems to depend on the module -
SFI for SFP, XFI for XFP, but essentially it's just 10GBASE-R over a
single serdes lane.

My inclusion of the K in there may not be completely correct as that
is for backplane 10GBASE-R connections, but the public information
available is very limited and incomplete.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> 
> > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > +	switch (eth_if) {
> > +	case DPMAC_ETH_IF_RGMII:
> > +		return PHY_INTERFACE_MODE_RGMII;
> 
> So the MAC cannot insert RGMII delays? I didn't see anything in the PHY object
> about configuring the delays. Does the PCB need to add delays via squiggles in
> the tracks?
> 
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set_port_modes(mask);
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_10GKR:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	default:
> > +		goto empty_set;
> > +	}
> 
> I think this is wrong. This is about validating what the MAC can do. The state-
> >interface should not matter. The PHY will indicate what interface mode should
> be used when auto-neg has completed. The MAC is then expected to change its
> interface to fit.
> 
> But lets see what Russell says.

We cannot do reconfiguration of the interface mode at runtime.
The SERDES speaks an ethernet/sata/pcie  coding that is configurable at reset time.

> 
> > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int
> mode,
> > +			     const struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	if (state->speed == SPEED_UNKNOWN && state->duplex ==
> DUPLEX_UNKNOWN)
> > +		return;
> > +
> > +	dpmac_state->up = !!state->link;
> > +	if (dpmac_state->up) {
> > +		dpmac_state->rate = state->speed;
> > +
> > +		if (!state->duplex)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_HALF_DUPLEX;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > +
> > +		if (state->an_enabled)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_AUTONEG;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_AUTONEG;
> 
> As Russell pointed out, this auto-neg is only valid in a limited context. The MAC
> generally does not perform auto-neg. The MAC is only involved in auto-neg
> when inband signalling is used between the MAC and PHY in 802.3z.
> 
> As the name says, dpaa2_mac_config is about the MAC.
> 
>    Andrew

Yes, the dpaa2_mac_config should take care of only the MAC but, in this case, we cannot convey
piecemeal link state information through the firmware to the Ethernet driver - it has to come all at once.

--
Ioana

Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 02:55:51AM +0300, Ioana Ciornei wrote:
> The dpaa2-mac driver binds to DPAA2 DPMAC objects, dynamically
> discovered on the fsl-mc bus. It acts as a proxy between the PHY
> management layer and the MC firmware, delivering any configuration
> changes to the firmware and also setting any new configuration requested
> though PHYLINK.
> 
> A in-depth view of the software architecture and the implementation can
> be found in
> 'Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst'.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  MAINTAINERS                                      |   7 +
>  drivers/net/ethernet/freescale/dpaa2/Kconfig     |  13 +
>  drivers/net/ethernet/freescale/dpaa2/Makefile    |   2 +
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 541 +++++++++++++++++++++++
>  4 files changed, 563 insertions(+)
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dd247a059889..a024ab2b2548 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4929,6 +4929,13 @@ S:	Maintained
>  F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp*
>  F:	drivers/net/ethernet/freescale/dpaa2/dprtc*
>  
> +DPAA2 MAC DRIVER
> +M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-mac*
> +F:	drivers/net/ethernet/freescale/dpaa2/dpmac*
> +
>  DPT_I2O SCSI RAID DRIVER
>  M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
>  L:	linux-scsi@vger.kernel.org
> diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> index 8bd384720f80..4ffa666c0a43 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
> +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> @@ -16,3 +16,16 @@ config FSL_DPAA2_PTP_CLOCK
>  	help
>  	  This driver adds support for using the DPAA2 1588 timer module
>  	  as a PTP clock.
> +
> +config FSL_DPAA2_MAC
> +	tristate "DPAA2 MAC / PHY proxy interface"
> +	depends on FSL_MC_BUS
> +	select MDIO_BUS_MUX_MMIOREG
> +	select FSL_XGMAC_MDIO
> +	select PHYLINK
> +	help
> +	  Prototype driver for DPAA2 MAC / PHY interface object.
> +	  This driver works as a proxy between PHYLINK including phy drivers and
> +	  the MC firmware.  It receives updates on link state changes from PHYLINK
> +	  and forwards them to MC and receives interrupt from MC whenever a
> +	  request is made to change the link state or configuration.
> diff --git a/drivers/net/ethernet/freescale/dpaa2/Makefile b/drivers/net/ethernet/freescale/dpaa2/Makefile
> index d1e78cdd512f..e96386ab23ea 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/Makefile
> +++ b/drivers/net/ethernet/freescale/dpaa2/Makefile
> @@ -5,10 +5,12 @@
>  
>  obj-$(CONFIG_FSL_DPAA2_ETH)		+= fsl-dpaa2-eth.o
>  obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK)	+= fsl-dpaa2-ptp.o
> +obj-$(CONFIG_FSL_DPAA2_MAC)		+= fsl-dpaa2-mac.o
>  
>  fsl-dpaa2-eth-objs	:= dpaa2-eth.o dpaa2-ethtool.o dpni.o
>  fsl-dpaa2-eth-${CONFIG_DEBUG_FS} += dpaa2-eth-debugfs.o
>  fsl-dpaa2-ptp-objs	:= dpaa2-ptp.o dprtc.o
> +fsl-dpaa2-mac-objs	:= dpaa2-mac.o dpmac.o
>  
>  # Needed by the tracing framework
>  CFLAGS_dpaa2-eth.o := -I$(src)
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> new file mode 100644
> index 000000000000..145ab4771788
> --- /dev/null
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -0,0 +1,541 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +/* Copyright 2015 Freescale Semiconductor Inc.
> + * Copyright 2018-2019 NXP
> + */
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/msi.h>
> +#include <linux/rtnetlink.h>
> +#include <linux/if_vlan.h>
> +
> +#include <net/netlink.h>
> +#include <uapi/linux/if_bridge.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +#include <linux/phylink.h>
> +#include <linux/notifier.h>
> +
> +#include <linux/fsl/mc.h>
> +
> +#include "dpmac.h"
> +#include "dpmac-cmd.h"
> +
> +#define to_dpaa2_mac_priv(phylink_config) \
> +	container_of(config, struct dpaa2_mac_priv, phylink_config)
> +
> +struct dpaa2_mac_priv {
> +	struct fsl_mc_device *mc_dev;
> +	struct dpmac_attr attr;
> +	struct dpmac_link_state state;
> +	u16 dpmac_ver_major;
> +	u16 dpmac_ver_minor;
> +
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;
> +	struct ethtool_link_ksettings kset;
> +};
> +
> +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if)
> +{
> +	switch (eth_if) {
> +	case DPMAC_ETH_IF_RGMII:
> +		return PHY_INTERFACE_MODE_RGMII;
> +	case DPMAC_ETH_IF_XFI:
> +		return PHY_INTERFACE_MODE_10GKR;
> +	case DPMAC_ETH_IF_USXGMII:
> +		return PHY_INTERFACE_MODE_USXGMII;

No support for SGMII nor the 802.3z modes?

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cmp_dpmac_ver(struct dpaa2_mac_priv *priv,
> +			 u16 ver_major, u16 ver_minor)
> +{
> +	if (priv->dpmac_ver_major == ver_major)
> +		return priv->dpmac_ver_minor - ver_minor;
> +	return priv->dpmac_ver_major - ver_major;
> +}
> +
> +struct dpaa2_mac_link_mode_map {
> +	u64 dpmac_lm;
> +	enum ethtool_link_mode_bit_indices ethtool_lm;
> +};
> +
> +static const struct dpaa2_mac_link_mode_map dpaa2_mac_lm_map[] = {
> +	{DPMAC_ADVERTISED_10BASET_FULL, ETHTOOL_LINK_MODE_10baseT_Full_BIT},
> +	{DPMAC_ADVERTISED_100BASET_FULL, ETHTOOL_LINK_MODE_100baseT_Full_BIT},
> +	{DPMAC_ADVERTISED_1000BASET_FULL, ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
> +	{DPMAC_ADVERTISED_10000BASET_FULL, ETHTOOL_LINK_MODE_10000baseT_Full_BIT},

No half-duplex support?

> +	{DPMAC_ADVERTISED_AUTONEG, ETHTOOL_LINK_MODE_Autoneg_BIT},
> +};
> +
> +static void link_mode_phydev2dpmac(unsigned long *phydev_lm,
> +				   u64 *dpmac_lm)
> +{
> +	enum ethtool_link_mode_bit_indices link_mode;
> +	int i;
> +
> +	*dpmac_lm = 0;
> +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> +		link_mode = dpaa2_mac_lm_map[i].ethtool_lm;
> +		if (linkmode_test_bit(link_mode, phydev_lm))
> +			*dpmac_lm |= dpaa2_mac_lm_map[i].dpmac_lm;
> +	}
> +}
> +
> +static void dpaa2_mac_ksettings_change(struct dpaa2_mac_priv *priv)
> +{
> +	struct fsl_mc_device *mc_dev = priv->mc_dev;
> +	struct dpmac_link_cfg link_cfg = { 0 };
> +	int err, i;
> +
> +	err = dpmac_get_link_cfg(mc_dev->mc_io, 0,
> +				 mc_dev->mc_handle,
> +				 &link_cfg);
> +
> +	if (err) {
> +		dev_err(&mc_dev->dev, "dpmac_get_link_cfg() = %d\n", err);
> +		return;
> +	}
> +
> +	phylink_ethtool_ksettings_get(priv->phylink, &priv->kset);
> +
> +	priv->kset.base.speed = link_cfg.rate;
> +	priv->kset.base.duplex = !!(link_cfg.options & DPMAC_LINK_OPT_HALF_DUPLEX);

What's the point of setting duplex to anything other than true here -
everything I've read in this driver apart from the above indicates
that there is no support for half duplex.

> +
> +	ethtool_link_ksettings_zero_link_mode(&priv->kset, advertising);
> +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> +		if (link_cfg.advertising & dpaa2_mac_lm_map[i].dpmac_lm)
> +			__set_bit(dpaa2_mac_lm_map[i].ethtool_lm,
> +				  priv->kset.link_modes.advertising);
> +	}
> +
> +	if (link_cfg.options & DPMAC_LINK_OPT_AUTONEG) {
> +		priv->kset.base.autoneg = AUTONEG_ENABLE;
> +		__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +			  priv->kset.link_modes.advertising);
> +	} else {
> +		priv->kset.base.autoneg = AUTONEG_DISABLE;
> +		__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +			    priv->kset.link_modes.advertising);
> +	}
> +
> +	phylink_ethtool_ksettings_set(priv->phylink, &priv->kset);

What if this returns an error?  There seems to be no way to communicate
failure back through the firmware.

> +static void dpaa2_mac_validate(struct phylink_config *config,
> +			       unsigned long *supported,
> +			       struct phylink_link_state *state)
> +{
> +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> +	struct dpmac_link_state *dpmac_state = &priv->state;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set_port_modes(mask);
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_10GKR:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 10000baseT_Full);
> +		break;
> +	case PHY_INTERFACE_MODE_QSGMII:
> +	case PHY_INTERFACE_MODE_RGMII:
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, 1000baseT_Full);
> +		phylink_set(mask, 10000baseT_Full);

Consider using the newer linkmode_set_bit() etc interfaces here.

> +		break;
> +	default:
> +		goto empty_set;
> +	}
> +
> +	bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_and(state->advertising, state->advertising, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +
> +	link_mode_phydev2dpmac(supported, &dpmac_state->supported);
> +	link_mode_phydev2dpmac(state->advertising, &dpmac_state->advertising);

This is not correct.  phylink will make calls to this function to
enquire whether something is supported or not, it isn't strictly used
to say "this is what we are going to use", so storing these does not
reflect the current state.

> +
> +	return;
> +
> +empty_set:
> +	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
> +static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode,
> +			     const struct phylink_link_state *state)
> +{
> +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> +	struct dpmac_link_state *dpmac_state = &priv->state;
> +	struct device *dev = &priv->mc_dev->dev;
> +	int err;
> +
> +	if (state->speed == SPEED_UNKNOWN && state->duplex == DUPLEX_UNKNOWN)
> +		return;
> +
> +	dpmac_state->up = !!state->link;
> +	if (dpmac_state->up) {
> +		dpmac_state->rate = state->speed;
> +
> +		if (!state->duplex)
> +			dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX;
> +		else
> +			dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX;
> +
> +		if (state->an_enabled)
> +			dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG;
> +		else
> +			dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG;
> +	}

Apart from my comments for the above in reply to Andrew, you can store
the "advertising" mask here.

However, what is the point of the "dpmac_state->up = !!state->link"
stuff (despite it being wrong as previously described) when you set
dpmac_state->up in the mac_link_up/mac_link_down functions below.
This makes no sense to me.

> +
> +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> +				   priv->mc_dev->mc_handle, dpmac_state);
> +	if (err)
> +		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
> +}
> +
> +static void dpaa2_mac_link_up(struct phylink_config *config, unsigned int mode,
> +			      phy_interface_t interface, struct phy_device *phy)
> +{
> +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> +	struct dpmac_link_state *dpmac_state = &priv->state;
> +	struct device *dev = &priv->mc_dev->dev;
> +	int err;
> +
> +	dpmac_state->up = 1;
> +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> +				   priv->mc_dev->mc_handle, dpmac_state);
> +	if (err)
> +		dev_err(dev, "dpmac_set_link_state() = %d\n", err);

This is also very suspect - have you read the phylink documentation?
The documentation details that there are some behavioural differences
here depending on the negotiation mode, but your code doesn't even
look at those.

Given that you're not handling those, I don't see how you expect SFP
support to work.  In fact, given that the validate callback doesn't
make any mention of SGMII, 1000BASEX, or 2500BASEX phy modes, I don't
see how you expect this to work with SFP.  Given that, I really
question why you want to use phylink rather than talking to phylib
directly.

I get the overall impression from what I've seen so far that phylink
is entirely unsuited to the structure of this implementation.

phylinks purpose is to support hotpluggable PHYs on SFP modules where
the MAC may be connected _directly_ to the SFP cage without an
intervening PHY, or if there is an intervening PHY, the PHY is
completely transparent.  For that to work, the interface modes that
SFP modules support must be supported by the MAC.

I can't see a reason at the moment for you to use phylink.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH RFC 4/6] dpaa2-mac: add initial driver

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 4/6] dpaa2-mac: add initial driver
> 
> On Fri, Jun 14, 2019 at 02:55:51AM +0300, Ioana Ciornei wrote:
> > The dpaa2-mac driver binds to DPAA2 DPMAC objects, dynamically
> > discovered on the fsl-mc bus. It acts as a proxy between the PHY
> > management layer and the MC firmware, delivering any configuration
> > changes to the firmware and also setting any new configuration
> > requested though PHYLINK.
> >
> > A in-depth view of the software architecture and the implementation
> > can be found in
> > 'Documentation/networking/device_drivers/freescale/dpaa2/dpmac-
> driver.rst'.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  MAINTAINERS                                      |   7 +
> >  drivers/net/ethernet/freescale/dpaa2/Kconfig     |  13 +
> >  drivers/net/ethernet/freescale/dpaa2/Makefile    |   2 +
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 541
> > +++++++++++++++++++++++
> >  4 files changed, 563 insertions(+)
> >  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > dd247a059889..a024ab2b2548 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4929,6 +4929,13 @@ S:	Maintained
> >  F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp*
> >  F:	drivers/net/ethernet/freescale/dpaa2/dprtc*
> >
> > +DPAA2 MAC DRIVER
> > +M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> > +L:	netdev@vger.kernel.org
> > +S:	Maintained
> > +F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-mac*
> > +F:	drivers/net/ethernet/freescale/dpaa2/dpmac*
> > +
> >  DPT_I2O SCSI RAID DRIVER
> >  M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
> >  L:	linux-scsi@vger.kernel.org
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > index 8bd384720f80..4ffa666c0a43 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > +++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
> > @@ -16,3 +16,16 @@ config FSL_DPAA2_PTP_CLOCK
> >  	help
> >  	  This driver adds support for using the DPAA2 1588 timer module
> >  	  as a PTP clock.
> > +
> > +config FSL_DPAA2_MAC
> > +	tristate "DPAA2 MAC / PHY proxy interface"
> > +	depends on FSL_MC_BUS
> > +	select MDIO_BUS_MUX_MMIOREG
> > +	select FSL_XGMAC_MDIO
> > +	select PHYLINK
> > +	help
> > +	  Prototype driver for DPAA2 MAC / PHY interface object.
> > +	  This driver works as a proxy between PHYLINK including phy drivers and
> > +	  the MC firmware.  It receives updates on link state changes from
> PHYLINK
> > +	  and forwards them to MC and receives interrupt from MC whenever a
> > +	  request is made to change the link state or configuration.
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/Makefile
> > b/drivers/net/ethernet/freescale/dpaa2/Makefile
> > index d1e78cdd512f..e96386ab23ea 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/Makefile
> > +++ b/drivers/net/ethernet/freescale/dpaa2/Makefile
> > @@ -5,10 +5,12 @@
> >
> >  obj-$(CONFIG_FSL_DPAA2_ETH)		+= fsl-dpaa2-eth.o
> >  obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK)	+= fsl-dpaa2-ptp.o
> > +obj-$(CONFIG_FSL_DPAA2_MAC)		+= fsl-dpaa2-mac.o
> >
> >  fsl-dpaa2-eth-objs	:= dpaa2-eth.o dpaa2-ethtool.o dpni.o
> >  fsl-dpaa2-eth-${CONFIG_DEBUG_FS} += dpaa2-eth-debugfs.o
> >  fsl-dpaa2-ptp-objs	:= dpaa2-ptp.o dprtc.o
> > +fsl-dpaa2-mac-objs	:= dpaa2-mac.o dpmac.o
> >
> >  # Needed by the tracing framework
> >  CFLAGS_dpaa2-eth.o := -I$(src)
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > new file mode 100644
> > index 000000000000..145ab4771788
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> > @@ -0,0 +1,541 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> > +/* Copyright 2015 Freescale Semiconductor Inc.
> > + * Copyright 2018-2019 NXP
> > + */
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/msi.h>
> > +#include <linux/rtnetlink.h>
> > +#include <linux/if_vlan.h>
> > +
> > +#include <net/netlink.h>
> > +#include <uapi/linux/if_bridge.h>
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/of_net.h>
> > +#include <linux/phylink.h>
> > +#include <linux/notifier.h>
> > +
> > +#include <linux/fsl/mc.h>
> > +
> > +#include "dpmac.h"
> > +#include "dpmac-cmd.h"
> > +
> > +#define to_dpaa2_mac_priv(phylink_config) \
> > +	container_of(config, struct dpaa2_mac_priv, phylink_config)
> > +
> > +struct dpaa2_mac_priv {
> > +	struct fsl_mc_device *mc_dev;
> > +	struct dpmac_attr attr;
> > +	struct dpmac_link_state state;
> > +	u16 dpmac_ver_major;
> > +	u16 dpmac_ver_minor;
> > +
> > +	struct phylink *phylink;
> > +	struct phylink_config phylink_config;
> > +	struct ethtool_link_ksettings kset;
> > +};
> > +
> > +static phy_interface_t phy_mode(enum dpmac_eth_if eth_if) {
> > +	switch (eth_if) {
> > +	case DPMAC_ETH_IF_RGMII:
> > +		return PHY_INTERFACE_MODE_RGMII;
> > +	case DPMAC_ETH_IF_XFI:
> > +		return PHY_INTERFACE_MODE_10GKR;
> > +	case DPMAC_ETH_IF_USXGMII:
> > +		return PHY_INTERFACE_MODE_USXGMII;
> 
> No support for SGMII nor the 802.3z modes?

The Serdes has support for both of them but the driver does not at the moment.

> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int cmp_dpmac_ver(struct dpaa2_mac_priv *priv,
> > +			 u16 ver_major, u16 ver_minor)
> > +{
> > +	if (priv->dpmac_ver_major == ver_major)
> > +		return priv->dpmac_ver_minor - ver_minor;
> > +	return priv->dpmac_ver_major - ver_major; }
> > +
> > +struct dpaa2_mac_link_mode_map {
> > +	u64 dpmac_lm;
> > +	enum ethtool_link_mode_bit_indices ethtool_lm; };
> > +
> > +static const struct dpaa2_mac_link_mode_map dpaa2_mac_lm_map[] = {
> > +	{DPMAC_ADVERTISED_10BASET_FULL,
> ETHTOOL_LINK_MODE_10baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_100BASET_FULL,
> ETHTOOL_LINK_MODE_100baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_1000BASET_FULL,
> ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
> > +	{DPMAC_ADVERTISED_10000BASET_FULL,
> > +ETHTOOL_LINK_MODE_10000baseT_Full_BIT},
> 
> No half-duplex support?

Yes, no half-duplex.

> 
> > +	{DPMAC_ADVERTISED_AUTONEG,
> ETHTOOL_LINK_MODE_Autoneg_BIT}, };
> > +
> > +static void link_mode_phydev2dpmac(unsigned long *phydev_lm,
> > +				   u64 *dpmac_lm)
> > +{
> > +	enum ethtool_link_mode_bit_indices link_mode;
> > +	int i;
> > +
> > +	*dpmac_lm = 0;
> > +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> > +		link_mode = dpaa2_mac_lm_map[i].ethtool_lm;
> > +		if (linkmode_test_bit(link_mode, phydev_lm))
> > +			*dpmac_lm |= dpaa2_mac_lm_map[i].dpmac_lm;
> > +	}
> > +}
> > +
> > +static void dpaa2_mac_ksettings_change(struct dpaa2_mac_priv *priv) {
> > +	struct fsl_mc_device *mc_dev = priv->mc_dev;
> > +	struct dpmac_link_cfg link_cfg = { 0 };
> > +	int err, i;
> > +
> > +	err = dpmac_get_link_cfg(mc_dev->mc_io, 0,
> > +				 mc_dev->mc_handle,
> > +				 &link_cfg);
> > +
> > +	if (err) {
> > +		dev_err(&mc_dev->dev, "dpmac_get_link_cfg() = %d\n", err);
> > +		return;
> > +	}
> > +
> > +	phylink_ethtool_ksettings_get(priv->phylink, &priv->kset);
> > +
> > +	priv->kset.base.speed = link_cfg.rate;
> > +	priv->kset.base.duplex = !!(link_cfg.options &
> > +DPMAC_LINK_OPT_HALF_DUPLEX);
> 
> What's the point of setting duplex to anything other than true here - everything
> I've read in this driver apart from the above indicates that there is no support for
> half duplex.

Yep, that's another mishap on my part. That should have been removed.

> 
> > +
> > +	ethtool_link_ksettings_zero_link_mode(&priv->kset, advertising);
> > +	for (i = 0; i < ARRAY_SIZE(dpaa2_mac_lm_map); i++) {
> > +		if (link_cfg.advertising & dpaa2_mac_lm_map[i].dpmac_lm)
> > +			__set_bit(dpaa2_mac_lm_map[i].ethtool_lm,
> > +				  priv->kset.link_modes.advertising);
> > +	}
> > +
> > +	if (link_cfg.options & DPMAC_LINK_OPT_AUTONEG) {
> > +		priv->kset.base.autoneg = AUTONEG_ENABLE;
> > +		__set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +			  priv->kset.link_modes.advertising);
> > +	} else {
> > +		priv->kset.base.autoneg = AUTONEG_DISABLE;
> > +		__clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > +			    priv->kset.link_modes.advertising);
> > +	}
> > +
> > +	phylink_ethtool_ksettings_set(priv->phylink, &priv->kset);
> 
> What if this returns an error?  There seems to be no way to communicate failure
> back through the firmware.

That's right, we do not have a way of signaling back the failure to the firmware.

> 
> > +static void dpaa2_mac_validate(struct phylink_config *config,
> > +			       unsigned long *supported,
> > +			       struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > +
> > +	phylink_set(mask, Autoneg);
> > +	phylink_set_port_modes(mask);
> > +
> > +	switch (state->interface) {
> > +	case PHY_INTERFACE_MODE_10GKR:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_QSGMII:
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		break;
> > +	case PHY_INTERFACE_MODE_USXGMII:
> > +		phylink_set(mask, 10baseT_Full);
> > +		phylink_set(mask, 100baseT_Full);
> > +		phylink_set(mask, 1000baseT_Full);
> > +		phylink_set(mask, 10000baseT_Full);
> 
> Consider using the newer linkmode_set_bit() etc interfaces here.

Sure.

> 
> > +		break;
> > +	default:
> > +		goto empty_set;
> > +	}
> > +
> > +	bitmap_and(supported, supported, mask,
> __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +	bitmap_and(state->advertising, state->advertising, mask,
> > +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> > +
> > +	link_mode_phydev2dpmac(supported, &dpmac_state->supported);
> > +	link_mode_phydev2dpmac(state->advertising,
> > +&dpmac_state->advertising);
> 
> This is not correct.  phylink will make calls to this function to enquire whether
> something is supported or not, it isn't strictly used to say "this is what we are
> going to use", so storing these does not reflect the current state.

We can update these only on a mac_config.
> 
> > +
> > +	return;
> > +
> > +empty_set:
> > +	bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); }
> > +
> > +static void dpaa2_mac_config(struct phylink_config *config, unsigned int
> mode,
> > +			     const struct phylink_link_state *state) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	if (state->speed == SPEED_UNKNOWN && state->duplex ==
> DUPLEX_UNKNOWN)
> > +		return;
> > +
> > +	dpmac_state->up = !!state->link;
> > +	if (dpmac_state->up) {
> > +		dpmac_state->rate = state->speed;
> > +
> > +		if (!state->duplex)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_HALF_DUPLEX;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_HALF_DUPLEX;
> > +
> > +		if (state->an_enabled)
> > +			dpmac_state->options |=
> DPMAC_LINK_OPT_AUTONEG;
> > +		else
> > +			dpmac_state->options &=
> ~DPMAC_LINK_OPT_AUTONEG;
> > +	}
> 
> Apart from my comments for the above in reply to Andrew, you can store the
> "advertising" mask here.
> 
> However, what is the point of the "dpmac_state->up = !!state->link"
> stuff (despite it being wrong as previously described) when you set dpmac_state-
> >up in the mac_link_up/mac_link_down functions below.
> This makes no sense to me.

Ok, so keep the last known value of the link until a .mac_link_{up/down} is called and only them update it.
I'll change that.

> 
> > +
> > +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> > +				   priv->mc_dev->mc_handle, dpmac_state);
> > +	if (err)
> > +		dev_err(dev, "dpmac_set_link_state() = %d\n", err); }
> > +
> > +static void dpaa2_mac_link_up(struct phylink_config *config, unsigned int
> mode,
> > +			      phy_interface_t interface, struct phy_device *phy) {
> > +	struct dpaa2_mac_priv *priv = to_dpaa2_mac_priv(phylink_config);
> > +	struct dpmac_link_state *dpmac_state = &priv->state;
> > +	struct device *dev = &priv->mc_dev->dev;
> > +	int err;
> > +
> > +	dpmac_state->up = 1;
> > +	err = dpmac_set_link_state(priv->mc_dev->mc_io, 0,
> > +				   priv->mc_dev->mc_handle, dpmac_state);
> > +	if (err)
> > +		dev_err(dev, "dpmac_set_link_state() = %d\n", err);
> 
> This is also very suspect - have you read the phylink documentation?
> The documentation details that there are some behavioural differences here
> depending on the negotiation mode, but your code doesn't even look at those.
> 
> Given that you're not handling those, I don't see how you expect SFP support to
> work.  In fact, given that the validate callback doesn't make any mention of
> SGMII, 1000BASEX, or 2500BASEX phy modes, I don't see how you expect this to
> work with SFP.  Given that, I really question why you want to use phylink rather
> than talking to phylib directly.

I am not handling the XFP/SFP+ support in this version of the driver since, as you noticed, I am not accepting BASEX modes.
Nonetheless, I am not ruling out adding support for SFP on top of this. This is one reason why we're using phylink. 

Also, using phylib is not straight-forward either. The dpaa2-mac should fabricate a net_device for each phy that it wants to connect to.
These net_device should be kept unregistered with the network core so that we don't see "MAC interfaces" in ifconfig.

> 
> I get the overall impression from what I've seen so far that phylink is entirely
> unsuited to the structure of this implementation.
> 
> phylinks purpose is to support hotpluggable PHYs on SFP modules where the
> MAC may be connected _directly_ to the SFP cage without an intervening PHY,
> or if there is an intervening PHY, the PHY is completely transparent.  For that to
> work, the interface modes that SFP modules support must be supported by the
> MAC.
> 
> I can't see a reason at the moment for you to use phylink.
> 

Well, a phylib solution isn't cleaner either. 

--
Ioana

[PATCH RFC 5/6] dpaa2-eth: add autoneg support

[Ioana Ciornei]

From: Ioana Radulescu <ruxandra.radulescu@nxp.com>

For MC versions that support it, use the new DPNI link APIs, which
allow setting/getting of advertised and supported link modes.

A mapping between DPNI link modes and ethtool ones is created to
help converting from one to the other.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
Signed-off-by: Valentin Catalin Neacsu <valentin-catalin.neacsu@nxp.com>
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   | 83 ++++++++++++++++++----
 1 file changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index 7b182f4b263c..38999908a388 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -74,6 +74,44 @@ static void dpaa2_eth_get_drvinfo(struct net_device *net_dev,
 		sizeof(drvinfo->bus_info));
 }
 
+#define DPNI_LINK_AUTONEG_VER_MAJOR		7
+#define DPNI_LINK_AUTONEG_VER_MINOR		8
+
+struct dpaa2_eth_link_mode_map {
+	u64 dpni_lm;
+	u64 ethtool_lm;
+};
+
+static const struct dpaa2_eth_link_mode_map dpaa2_eth_lm_map[] = {
+	{DPNI_ADVERTISED_10BASET_FULL, ETHTOOL_LINK_MODE_10baseT_Full_BIT},
+	{DPNI_ADVERTISED_100BASET_FULL, ETHTOOL_LINK_MODE_100baseT_Full_BIT},
+	{DPNI_ADVERTISED_1000BASET_FULL, ETHTOOL_LINK_MODE_1000baseT_Full_BIT},
+	{DPNI_ADVERTISED_10000BASET_FULL, ETHTOOL_LINK_MODE_10000baseT_Full_BIT},
+	{DPNI_ADVERTISED_AUTONEG, ETHTOOL_LINK_MODE_Autoneg_BIT},
+};
+
+static void link_mode_dpni2ethtool(u64 dpni_lm, unsigned long *ethtool_lm)
+{
+	int i;
+
+	bitmap_zero(ethtool_lm, __ETHTOOL_LINK_MODE_MASK_NBITS);
+	for (i = 0; i < ARRAY_SIZE(dpaa2_eth_lm_map); i++) {
+		if (dpni_lm & dpaa2_eth_lm_map[i].dpni_lm)
+			__set_bit(dpaa2_eth_lm_map[i].ethtool_lm, ethtool_lm);
+	}
+}
+
+static void link_mode_ethtool2dpni(const unsigned long *ethtool_lm,
+				   u64 *dpni_lm)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dpaa2_eth_lm_map); i++) {
+		if (test_bit(dpaa2_eth_lm_map[i].ethtool_lm, ethtool_lm))
+			*dpni_lm |= dpaa2_eth_lm_map[i].dpni_lm;
+	}
+}
+
 static int
 dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
 			     struct ethtool_link_ksettings *link_settings)
@@ -82,19 +120,32 @@ static void dpaa2_eth_get_drvinfo(struct net_device *net_dev,
 	int err = 0;
 	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
 
-	err = dpni_get_link_state(priv->mc_io, 0, priv->mc_token, &state);
-	if (err) {
-		netdev_err(net_dev, "ERROR %d getting link state\n", err);
-		goto out;
+	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_LINK_AUTONEG_VER_MAJOR,
+				   DPNI_LINK_AUTONEG_VER_MINOR) < 0) {
+		err = dpni_get_link_state(priv->mc_io, 0, priv->mc_token,
+					  &state);
+		if (err) {
+			netdev_err(net_dev, "dpni_get_link_state failed\n");
+			goto out;
+		}
+	} else {
+		err = dpni_get_link_state_v2(priv->mc_io, 0, priv->mc_token,
+					     &state);
+		if (err) {
+			netdev_err(net_dev, "dpni_get_link_state_v2 failed\n");
+			goto out;
+		}
+		link_mode_dpni2ethtool(state.supported,
+				       link_settings->link_modes.supported);
+		link_mode_dpni2ethtool(state.advertising,
+				       link_settings->link_modes.advertising);
 	}
 
-	/* At the moment, we have no way of interrogating the DPMAC
-	 * from the DPNI side - and for that matter there may exist
-	 * no DPMAC at all. So for now we just don't report anything
-	 * beyond the DPNI attributes.
-	 */
 	if (state.options & DPNI_LINK_OPT_AUTONEG)
 		link_settings->base.autoneg = AUTONEG_ENABLE;
+	else
+		link_settings->base.autoneg = AUTONEG_DISABLE;
+
 	if (!(state.options & DPNI_LINK_OPT_HALF_DUPLEX))
 		link_settings->base.duplex = DUPLEX_FULL;
 	link_settings->base.speed = state.rate;
@@ -135,12 +186,16 @@ static void dpaa2_eth_get_drvinfo(struct net_device *net_dev,
 	else
 		cfg.options &= ~DPNI_LINK_OPT_HALF_DUPLEX;
 
-	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &cfg);
+	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_LINK_AUTONEG_VER_MAJOR,
+				   DPNI_LINK_AUTONEG_VER_MINOR) < 0) {
+		err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &cfg);
+	} else {
+		link_mode_ethtool2dpni(link_settings->link_modes.advertising,
+				       &cfg.advertising);
+		dpni_set_link_cfg_v2(priv->mc_io, 0, priv->mc_token, &cfg);
+	}
 	if (err)
-		/* ethtool will be loud enough if we return an error; no point
-		 * in putting our own error message on the console by default
-		 */
-		netdev_dbg(net_dev, "ERROR %d setting link cfg\n", err);
+		netdev_err(net_dev, "dpni_set_link_cfg failed");
 
 	return err;
 }
-- 
1.9.1

[PATCH RFC 6/6] net: documentation: add MAC/PHY proxy driver documentation

[Ioana Ciornei]

Add documentation file for the dpaa2-mac driver. This describes the
architecture and implementation of the proxy interface between phylink
and dpaa2-eth.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../freescale/dpaa2/dpmac-driver.rst               | 159 +++++++++++++++++++++
 .../device_drivers/freescale/dpaa2/index.rst       |   1 +
 MAINTAINERS                                        |   1 +
 3 files changed, 161 insertions(+)
 create mode 100644 Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst

diff --git a/Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst b/Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst
new file mode 100644
index 000000000000..ad09aa1c921e
--- /dev/null
+++ b/Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst
@@ -0,0 +1,159 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. include:: <isonum.txt>
+
+===============================
+DPAA2 MAC / PHY proxy interface
+===============================
+
+:Copyright: |copy| 2019 NXP
+
+
+Overview
+--------
+
+The DPAA2 MAC / PHY proxy interface driver binds to DPAA2 DPMAC objects, which
+are dynamically discovered on the fsl-mc bus. Once probed, the driver looks up
+the device tree for PHYLINK-compatible OF bindings (phy-handle) and does the
+following:
+
+- registers itself with the Management Complex (MC) firmware to receive
+  interrupts for:
+
+        - Link up/down (on the PHY)
+        - Link configuration changes requested
+
+- creates a PHYLINK instance based on its device_node and connects to the
+  specified PHY
+
+- notifies the MC firmware of any link status change received from PHYLINK
+
+
+DPAA2 Software Architecture
+---------------------------
+
+Among other DPAA2 objects, the fsl-mc bus exports DPNI objects (abstracting a
+network interface) and DPMAC objects (abstracting a MAC) which are probed and
+managed by two different drivers: dpaa2-eth and dpaa2-mac.
+
+Data connections may be established between a DPNI and a DPMAC, or between two
+DPNIs.  A DPNI may be directly assigned to a guest software partition, whereas
+a DPMAC object is always managed from the root (most privileged) container.
+
+For netif_carrier_on/netif_carrier_off, the MC firmware presents to the DPNI
+object an abstracted view of the link state:
+
+.. code-block:: none
+
+  Sources of abstracted link state information presented by the MC firmware
+
+                                               +--------------------------------------+
+  +------------+                               |                           xgmac_mdio |
+  | net_device |                 +---------+   |  +-----+  +-----+  +-----+  +-----+  |
+  +------------+                 | phylink |<--|  | PHY |  | PHY |  | PHY |  | PHY |  |
+        ^                        +---------+   |  +-----+  +-----+  +-----+  +-----+  |
+        |                             |        |                    External MDIO bus |
+    dpaa2-eth                     dpaa2-mac    +--------------------------------------+
+        ^                             |
+        |                             |                                           Linux
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+        |                             |                                     MC firmware
+        |              /|             V
+  +----------+        / |       +----------+
+  |          |       /  |       |          |
+  |          |       |  |       |          |
+  |   DPNI   |<------|  |<------|   DPMAC  |
+  |          |       |  |       |          |
+  |          |       \  |<---+  |          |
+  +----------+        \ |    |  +----------+
+                       \|    |
+                             |
+           +--------------------------------------+
+           | MC firmware polling MAC PCS for link |
+           |  +-----+  +-----+  +-----+  +-----+  |
+           |  | PCS |  | PCS |  | PCS |  | PCS |  |
+           |  +-----+  +-----+  +-----+  +-----+  |
+           |                    Internal MDIO bus |
+           +--------------------------------------+
+
+
+Depending on an MC firmware configuration setting, each MAC may be in one of two modes:
+
+- DPMAC_LINK_TYPE_FIXED: the link state management is handled exclusively by
+  the MC firmware by polling the MAC PCS.
+
+- DPMAC_LINK_TYPE_PHY: the link state comes as an input to the MC firmware
+  itself through the dpmac_set_link_cfg() function.
+
+In both cases, the MC firmware emits an abstracted link state information to
+the connected DPNI which can be retrieved using the dpni_get_link_state()
+command.  This way, users of DPNI interfaces are not required to implement
+complex PHY drivers.
+
+In DPMAC_LINK_TYPE_FIXED mode, a dpaa2-mac driver is not necessary.
+
+Implementation
+--------------
+
+After the system boots and the DPNIs are connected to DPMACs, all network
+interfaces have their respective net_devices exported and ready to be used.
+
+When the dpaa2-eth interface link is requested to go up, the following set of
+steps must happen::
+
+  Inter-driver communication between fsl-mc bus objects through the MC firmware
+
+                             +-----------+                     +-------------+
+                             |           | ------------------> | MC firmware |
+                             | dpaa2-eth |         (1)         |             |
+                             |           | < - - - - - - - - - |             |
+                             +-----------+         (6)         |             |
+                                   |                           |             |
+                                  eth0                         |             |
+                                                               |             |
+  +---------+                +-----------+                     |             |
+  | PHYLINK | <------------  | dpaa2-mac | <------------------ |             |
+  |         |      (3)       |           |         (2)         |             |
+  |         |                |           |                     |             |
+  |         | ------------>  |           | ------------------> |             |
+  +---------+      (4)       +-----------+         (5)         |             |
+                                                               +-------------+
+
+  (1) dpni_enable() - Enable network interface, allowing receiving/sending frames
+  (2) MC sends DPMAC_IRQ_EVENT_LINK_UP_REQ to the dpmac object
+  (3) The dpaa2-mac driver calls phylink_start() on the PHYLINK instance
+  (4) PHYLINK notifies the dpaa2-mac driver through the .mac_config and
+      .mac_link_up calbacks
+  (5) With the information received in the phylink_link_state structure, the
+      dpaa2-mac driver informs the firmware of the new link state
+  (6) At any later time, the dpaa2-eth driver may find the updated link state
+      by calling dpni_get_link_state() to the MC firmware
+
+And the following output is seen in the console::
+
+ # ip link set dev eth0 up
+ [14894.837845] fsl_dpaa2_mac dpmac.17: configuring for phy/rgmii-id link mode
+ [14896.895953] fsl_dpaa2_mac dpmac.17: Link is Up - 1Gbps/Full - flow control off
+ [14896.897478] fsl_dpaa2_eth dpni.0 eth1: Link Event: state up
+
+
+In case of a link change requested by the user through ethtool on the dpaa2-eth
+interface, the same calling flow as above happens between DPNI driver -> MC ->
+DPMAC driver -> PHYLINK and back. However in this case the functions are
+different::
+
+  (1) The dpaa2-eth driver, on the .set_link_ksettings() callback, sends a
+      dpni_set_link_cfg() to the firmware, informing it about the new
+      configuration requested. This firmware command will carry link state
+      options such as autoneg on/off, advertising, duplex, speed.
+  (2) The MC firmware will trigger an DPMAC_IRQ_EVENT_LINK_CFG_REQ.
+  (3) Upon receiving the interrupt, the dpaa2-mac driver will get the
+      requested configuration parameters and construct a new
+      ethtool_link_ksettings command based on them. This will be passed to
+      phylink_ethtool_ksettings_set.
+  (4) If the link state changes, the .mac_config() routine will be called by
+      PHYLINK.
+  (5) The dpaa2-mac driver passes the current state of link from the
+      phylink_link_state argument and notifies the firmware of all the changes
+      (autoneg, speed, etc) through another dpmac_set_link_state() command.
+  (6) Same as above, the DPNI driver can retrieve the updated link state
+      information at a later time through dpni_get_link_state().
diff --git a/Documentation/networking/device_drivers/freescale/dpaa2/index.rst b/Documentation/networking/device_drivers/freescale/dpaa2/index.rst
index 67bd87fe6c53..2cd5d728da19 100644
--- a/Documentation/networking/device_drivers/freescale/dpaa2/index.rst
+++ b/Documentation/networking/device_drivers/freescale/dpaa2/index.rst
@@ -8,3 +8,4 @@ DPAA2 Documentation
    overview
    dpio-driver
    ethernet-driver
+   dpmac-driver
diff --git a/MAINTAINERS b/MAINTAINERS
index a024ab2b2548..0b9c1df5ff8c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4935,6 +4935,7 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-mac*
 F:	drivers/net/ethernet/freescale/dpaa2/dpmac*
+F:	Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst
 
 DPT_I2O SCSI RAID DRIVER
 M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
-- 
1.9.1

Re: [PATCH RFC 0/6] DPAA2 MAC Driver

[Russell King - ARM Linux admin]

On Fri, Jun 14, 2019 at 02:55:47AM +0300, Ioana Ciornei wrote:
> After today's discussion with Russell King about what phylink exposes in
> .mac_config(): https://marc.info/?l=linux-netdev&m=156043794316709&w=2
> I am submitting for initial review the dpaa2-mac driver model.
> 
> At the moment, pause frame support is missing so inherently all the USXGMII
> modes that rely on backpressure applied by the PHY in rate adaptation between
> network side and system side don't work properly.
> 
> As next steps, the driver will have to be integrated with the SFP bus so
> commands such as 'ethtool --dump-module-eeprom' will have to work through the
> current callpath through firmware.

From what I understand having read the doc patch, would it be fair to
say that every operation that is related to the link state has to be
passed from the ETH driver to the firmware, and then from the firmware
back to the kernel to the MAC driver?

Does this mean that "ethtool --dump-module-eeprom" goes through this
process as well - eth driver into firmware, which then gets forwarded
out of the formware on the MAC side, via phylink to the SFP cage?

If that is true, what is the proposal - to forward a copy of the EEPROM
on module insertion into the firmware, so that it is stored there when
anyone requests it?  What about the diagnostic monitoring that is
real-time?

Or is the SFP cage entirely handled by firmware?

> This poses somewhat of a problem, as
> dpaa2-eth lacks any handle to the phy so it will probably need further
> modification to the API that the firmware exposes (same applies to 'ethtool
> --phy-statistics').

This again sounds like the eth driver forwards the request to firmware
which then forwards it to the mac driver for it to process.

Is that correct?

> 
> The documentation patch provides a more complete view of the software
> architecture and the current implementation.
> 
> Ioana Ciornei (4):
>   net: phy: update the autoneg state in phylink_phy_change
>   dpaa2-mac: add MC API for the DPMAC object
>   dpaa2-mac: add initial driver
>   net: documentation: add MAC/PHY proxy driver documentation
> 
> Ioana Radulescu (2):
>   dpaa2-eth: add support for new link state APIs
>   dpaa2-eth: add autoneg support
> 
>  .../freescale/dpaa2/dpmac-driver.rst               | 159 ++++++
>  .../device_drivers/freescale/dpaa2/index.rst       |   1 +
>  MAINTAINERS                                        |   8 +
>  drivers/net/ethernet/freescale/dpaa2/Kconfig       |  13 +
>  drivers/net/ethernet/freescale/dpaa2/Makefile      |   2 +
>  .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   |  83 +++-
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c   | 541 +++++++++++++++++++++
>  drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h   | 107 ++++
>  drivers/net/ethernet/freescale/dpaa2/dpmac.c       | 369 ++++++++++++++
>  drivers/net/ethernet/freescale/dpaa2/dpmac.h       | 210 ++++++++
>  drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h    |  35 ++
>  drivers/net/ethernet/freescale/dpaa2/dpni.c        |  70 +++
>  drivers/net/ethernet/freescale/dpaa2/dpni.h        |  27 +
>  drivers/net/phy/phylink.c                          |   1 +
>  14 files changed, 1612 insertions(+), 14 deletions(-)
>  create mode 100644 Documentation/networking/device_drivers/freescale/dpaa2/dpmac-driver.rst
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.c
>  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.h
> 
> -- 
> 1.9.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

RE: [PATCH RFC 0/6] DPAA2 MAC Driver

[Ioana Ciornei]

> Subject: Re: [PATCH RFC 0/6] DPAA2 MAC Driver
> 
> On Fri, Jun 14, 2019 at 02:55:47AM +0300, Ioana Ciornei wrote:
> > After today's discussion with Russell King about what phylink exposes
> > in
> > .mac_config(): https://marc.info/?l=linux-netdev&m=156043794316709&w=2
> > I am submitting for initial review the dpaa2-mac driver model.
> >
> > At the moment, pause frame support is missing so inherently all the
> > USXGMII modes that rely on backpressure applied by the PHY in rate
> > adaptation between network side and system side don't work properly.
> >
> > As next steps, the driver will have to be integrated with the SFP bus
> > so commands such as 'ethtool --dump-module-eeprom' will have to work
> > through the current callpath through firmware.
> 
> From what I understand having read the doc patch, would it be fair to say
> that every operation that is related to the link state has to be passed from
> the ETH driver to the firmware, and then from the firmware back to the
> kernel to the MAC driver?

That is correct.

> 
> Does this mean that "ethtool --dump-module-eeprom" goes through this
> process as well - eth driver into firmware, which then gets forwarded out of
> the formware on the MAC side, via phylink to the SFP cage?
> 
> If that is true, what is the proposal - to forward a copy of the EEPROM on
> module insertion into the firmware, so that it is stored there when anyone
> requests it?  What about the diagnostic monitoring that is real-time?
> 

At the moment, we do not have a proposal that could solve all these issues.
We thought about a solution where the eth driver issues a command to the firmware that then issues an IRQ to the mac driver which could retrieve and then pass back the information.
This doesn't seem too feasible since the ethernet driver should be waiting for the data to arrive back from the firmware while in an ethtool callback.


> Or is the SFP cage entirely handled by firmware?

No, the SFP cage is not handled by firmware.

> 
> > This poses somewhat of a problem, as
> > dpaa2-eth lacks any handle to the phy so it will probably need further
> > modification to the API that the firmware exposes (same applies to
> > 'ethtool --phy-statistics').
> 
> This again sounds like the eth driver forwards the request to firmware which
> then forwards it to the mac driver for it to process.
> 
> Is that correct?

Correct.

> 
> >
> > The documentation patch provides a more complete view of the software
> > architecture and the current implementation.
> >
> > Ioana Ciornei (4):
> >   net: phy: update the autoneg state in phylink_phy_change
> >   dpaa2-mac: add MC API for the DPMAC object
> >   dpaa2-mac: add initial driver
> >   net: documentation: add MAC/PHY proxy driver documentation
> >
> > Ioana Radulescu (2):
> >   dpaa2-eth: add support for new link state APIs
> >   dpaa2-eth: add autoneg support
> >
> >  .../freescale/dpaa2/dpmac-driver.rst               | 159 ++++++
> >  .../device_drivers/freescale/dpaa2/index.rst       |   1 +
> >  MAINTAINERS                                        |   8 +
> >  drivers/net/ethernet/freescale/dpaa2/Kconfig       |  13 +
> >  drivers/net/ethernet/freescale/dpaa2/Makefile      |   2 +
> >  .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   |  83 +++-
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c   | 541
> +++++++++++++++++++++
> >  drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h   | 107 ++++
> >  drivers/net/ethernet/freescale/dpaa2/dpmac.c       | 369
> ++++++++++++++
> >  drivers/net/ethernet/freescale/dpaa2/dpmac.h       | 210 ++++++++
> >  drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h    |  35 ++
> >  drivers/net/ethernet/freescale/dpaa2/dpni.c        |  70 +++
> >  drivers/net/ethernet/freescale/dpaa2/dpni.h        |  27 +
> >  drivers/net/phy/phylink.c                          |   1 +
> >  14 files changed, 1612 insertions(+), 14 deletions(-)  create mode
> > 100644
> > Documentation/networking/device_drivers/freescale/dpaa2/dpmac-
> driver.r
> > st  create mode 100644
> > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> >  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac-cmd.h
> >  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.c
> >  create mode 100644 drivers/net/ethernet/freescale/dpaa2/dpmac.h
> >
> > --
> > 1.9.1
> >

Re: [PATCH RFC 0/6] DPAA2 MAC Driver

[Andrew Lunn]

On Fri, Jun 14, 2019 at 03:26:50PM +0000, Ioana Ciornei wrote:
> > Subject: Re: [PATCH RFC 0/6] DPAA2 MAC Driver
> > 
> > On Fri, Jun 14, 2019 at 02:55:47AM +0300, Ioana Ciornei wrote:
> > > After today's discussion with Russell King about what phylink exposes
> > > in
> > > .mac_config(): https://marc.info/?l=linux-netdev&m=156043794316709&w=2
> > > I am submitting for initial review the dpaa2-mac driver model.
> > >
> > > At the moment, pause frame support is missing so inherently all the
> > > USXGMII modes that rely on backpressure applied by the PHY in rate
> > > adaptation between network side and system side don't work properly.
> > >
> > > As next steps, the driver will have to be integrated with the SFP bus
> > > so commands such as 'ethtool --dump-module-eeprom' will have to work
> > > through the current callpath through firmware.
> > 
> > From what I understand having read the doc patch, would it be fair to say
> > that every operation that is related to the link state has to be passed from
> > the ETH driver to the firmware, and then from the firmware back to the
> > kernel to the MAC driver?
> 
> That is correct.
> 
> > 
> > Does this mean that "ethtool --dump-module-eeprom" goes through this
> > process as well - eth driver into firmware, which then gets forwarded out of
> > the formware on the MAC side, via phylink to the SFP cage?
> > 
> > If that is true, what is the proposal - to forward a copy of the EEPROM on
> > module insertion into the firmware, so that it is stored there when anyone
> > requests it?  What about the diagnostic monitoring that is real-time?
> > 
> 
> At the moment, we do not have a proposal that could solve all these issues.
> We thought about a solution where the eth driver issues a command to the firmware that then issues an IRQ to the mac driver which could retrieve and then pass back the information.
> This doesn't seem too feasible since the ethernet driver should be waiting for the data to arrive back from the firmware while in an ethtool callback.
> 
> 
> > Or is the SFP cage entirely handled by firmware?
> 
> No, the SFP cage is not handled by firmware.

Hi Ioana

At the moment, you seem to be in a bad middle ground. The firmware
cannot do everything, so you want Linux and PHYLINK to do some
bits. But Linux expects to do everything, have full control of the
hardware. And it is not fitting together.

Maybe you need to step back and look at the overall architecture. And
then decide which way you want to go to get out of the middle
ground. There are good examples of the firmware controlling
everything, and the driver just using the high level ethtool API calls
to report state. And there are good examples of low levels of the
hardware, the MDIO bus so Linux can control the PHYs, the i2c bus and
GPIOs for the SFP cage, etc being exposed so PHYLINK can control
everything.

To have a sensible architecture and driver implementation you need to
go one way or the other. All or nothing.

   Andrew