mbox series

[net-next,v3,00/11] mscc: ocelot: add support for SerDes muxing configuration

Message ID cover.ff40d591b548a6da31716e6e600f11a303e0e643.1536912834.git-series.quentin.schulz@bootlin.com
Headers show
Series mscc: ocelot: add support for SerDes muxing configuration | expand

Message

Quentin Schulz Sept. 14, 2018, 8:15 a.m. UTC 
The Ocelot switch has currently an hardcoded SerDes muxing that suits only
a particular use case. Any other board setup will fail to work.

To prepare for upcoming boards' support that do not have the same muxing,
create a PHY driver that will handle all possible cases.

A SerDes can work in SGMII, QSGMII or PCIe and is also muxed to use a
given port depending on the selected mode or board design.

The SerDes configuration is in the middle of an address space (HSIO) that
is used to configure some parts in the MAC controller driver, that is why
we need to use a syscon so that we can write to the same address space from
different drivers safely using regmap.

This breaks backward compatibility but it's fine because there's only one
board at the moment that is using what's modified in this patch series.
This will break git bisect.

Even though this patch series is about SerDes __muxing__ configuration, the
DT node is named serdes for the simple reason that I couldn't find any
mention to SerDes anywhere else from the address space handled by this
driver.

Thanks,
Quentin

v3:
  - add Paul Burton's Acked-By on MIPS patches so that the patch series can
  be merged in the net tree in its entirety,

v2:
  - use a switch case for setting the phy_mode in the SerDes driver as
  suggested by Andrew,
  - stop replacing the value of the error pointer in the SerDes driver,
  - use a dev_dbg for the deferring of the probe in the SerDes driver,
  - use constants in the Device Tree to select the SerDes macro in use with
  a port,
  - adapt the SerDes driver to use those constants,
  - add a header file in include/dt-bindings for the constants,
  - fix space/tab issue,

Quentin Schulz (11):
  MIPS: mscc: ocelot: make HSIO registers address range a syscon
  dt-bindings: net: ocelot: remove hsio from the list of register address spaces
  net: mscc: ocelot: get HSIO regmap from syscon
  net: mscc: ocelot: move the HSIO header to include/soc
  net: mscc: ocelot: simplify register access for PLL5 configuration
  phy: add QSGMII and PCIE modes
  dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing
  MIPS: mscc: ocelot: add SerDes mux DT node
  dt-bindings: add constants for Microsemi Ocelot SerDes driver
  phy: add driver for Microsemi Ocelot SerDes muxing
  net: mscc: ocelot: make use of SerDes PHYs for handling their configuration

 Documentation/devicetree/bindings/mips/mscc.txt             |  16 +-
 Documentation/devicetree/bindings/net/mscc-ocelot.txt       |   9 +-
 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt |  40 +-
 arch/mips/boot/dts/mscc/ocelot.dtsi                         |  19 +-
 drivers/net/ethernet/mscc/Kconfig                           |   2 +-
 drivers/net/ethernet/mscc/ocelot.c                          |  16 +-
 drivers/net/ethernet/mscc/ocelot.h                          |  79 +-
 drivers/net/ethernet/mscc/ocelot_board.c                    |  61 +-
 drivers/net/ethernet/mscc/ocelot_hsio.h                     | 785 +------
 drivers/net/ethernet/mscc/ocelot_regs.c                     |  93 +-
 drivers/phy/Kconfig                                         |   1 +-
 drivers/phy/Makefile                                        |   1 +-
 drivers/phy/mscc/Kconfig                                    |  11 +-
 drivers/phy/mscc/Makefile                                   |   5 +-
 drivers/phy/mscc/phy-ocelot-serdes.c                        | 288 ++-
 include/dt-bindings/phy/phy-ocelot-serdes.h                 |  19 +-
 include/linux/phy/phy.h                                     |   2 +-
 include/soc/mscc/ocelot_hsio.h                              | 859 +++++++-
 18 files changed, 1341 insertions(+), 965 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt
 delete mode 100644 drivers/net/ethernet/mscc/ocelot_hsio.h
 create mode 100644 drivers/phy/mscc/Kconfig
 create mode 100644 drivers/phy/mscc/Makefile
 create mode 100644 drivers/phy/mscc/phy-ocelot-serdes.c
 create mode 100644 include/dt-bindings/phy/phy-ocelot-serdes.h
 create mode 100644 include/soc/mscc/ocelot_hsio.h

base-commit: ee4fccbee7d397c4d937e20d8c76212ffc23a7e3

Comments

Florian Fainelli Sept. 15, 2018, 2:23 a.m. UTC | #1 
On 09/14/18 01:16, Quentin Schulz wrote:
> HSIO address space was moved to a syscon, hence we need to get the
> regmap of this address space from there and no more from the device
> node.
> 
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Sept. 15, 2018, 2:24 a.m. UTC | #2 
On 09/14/18 01:16, Quentin Schulz wrote:
> Since HSIO address space can be used by different drivers (PLL, SerDes
> muxing, temperature sensor), let's move it somewhere it can be included
> by all drivers.
> 
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Nothing wrong with the patch, you likely would have wanted to use git
format-patch -M such that the diff would have been showing that the file
was renamed.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Sept. 15, 2018, 2:26 a.m. UTC | #3 
On 09/14/18 01:16, Quentin Schulz wrote:
> Since HSIO address space can be accessed by different drivers, let's
> simplify the register address definitions so that it can be easily used
> by all drivers and put the register address definition in the
> include/soc/mscc/ocelot_hsio.h header file.
> 
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Sept. 15, 2018, 2:27 a.m. UTC | #4 
On 09/14/18 01:16, Quentin Schulz wrote:
> Prepare for upcoming phys that'll handle QSGMII or PCIe.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Sept. 15, 2018, 2:30 a.m. UTC | #5 
On 09/14/18 01:16, Quentin Schulz wrote:
> The Microsemi Ocelot has a set of register for SerDes/switch port muxing
> as well as PCIe muxing for a specific SerDes, so let's add the device
> and all SerDes in the Device Tree.
> 
> Acked-by: Paul Burton <paul.burton@mips.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Florian Fainelli Sept. 15, 2018, 9:20 p.m. UTC | #6 
On 09/14/18 01:16, Quentin Schulz wrote:
> The Microsemi Ocelot can mux SerDes lanes (aka macros) to different
> switch ports or even make it act as a PCIe interface.
> 
> This adds support for the muxing of the SerDes.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---

[snip]

> +
> +struct serdes_macro {
> +	u8			idx;
> +	/* Not used when in QSGMII or PCIe mode */
> +	int			port;

u8 port to be consistent with the mux table?

[snip]

> +#define SERDES_MUX(_idx, _port, _mode, _mask, _mux) {	\
> +	.idx = _idx,						\
> +	.port = _port,						\
> +	.mode = _mode,						\
> +	.mask = _mask,						\
> +	.mux = _mux,						\
> +}
> +
> +static const struct serdes_mux ocelot_serdes_muxes[] = {
> +	SERDES_MUX(SERDES1G_0, 0, PHY_MODE_SGMII, 0, 0),
> +	SERDES_MUX(SERDES1G_1, 1, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_5_MODE, 0),
> +	SERDES_MUX(SERDES1G_1, 5, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> +		   HSIO_HW_CFG_DEV1G_5_MODE, HSIO_HW_CFG_DEV1G_5_MODE),

Why not go one step further and define a SERDES_MUX_SGMII() macro which
automatically resolves the correct bit definitions to use?

The current macro does not make this particularly easy to read :/

> +	SERDES_MUX(SERDES1G_2, 2, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_4_MODE, 0),
> +	SERDES_MUX(SERDES1G_2, 4, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> +		   HSIO_HW_CFG_DEV1G_4_MODE, HSIO_HW_CFG_DEV1G_4_MODE),
> +	SERDES_MUX(SERDES1G_3, 3, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_6_MODE, 0),
> +	SERDES_MUX(SERDES1G_3, 6, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> +		   HSIO_HW_CFG_DEV1G_6_MODE, HSIO_HW_CFG_DEV1G_6_MODE),
> +	SERDES_MUX(SERDES1G_4, 4, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> +		   HSIO_HW_CFG_DEV1G_4_MODE | HSIO_HW_CFG_DEV1G_9_MODE, 0),
> +	SERDES_MUX(SERDES1G_4, 9, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_4_MODE |
> +		   HSIO_HW_CFG_DEV1G_9_MODE, HSIO_HW_CFG_DEV1G_4_MODE |
> +		   HSIO_HW_CFG_DEV1G_9_MODE),
> +	SERDES_MUX(SERDES1G_5, 5, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> +		   HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE, 0),
> +	SERDES_MUX(SERDES1G_5, 10, PHY_MODE_SGMII, HSIO_HW_CFG_PCIE_ENA |
> +		   HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE,
> +		   HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE),
> +	SERDES_MUX(SERDES6G_0, 4, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> +		   HSIO_HW_CFG_QSGMII_ENA),
> +	SERDES_MUX(SERDES6G_0, 5, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> +		   HSIO_HW_CFG_QSGMII_ENA),
> +	SERDES_MUX(SERDES6G_0, 6, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> +		   HSIO_HW_CFG_QSGMII_ENA),
> +	SERDES_MUX(SERDES6G_0, 7, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA, 0),
> +	SERDES_MUX(SERDES6G_0, 7, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> +		   HSIO_HW_CFG_QSGMII_ENA),
> +	SERDES_MUX(SERDES6G_1, 8, PHY_MODE_SGMII, 0, 0),
> +	SERDES_MUX(SERDES6G_2, 10, PHY_MODE_SGMII, HSIO_HW_CFG_PCIE_ENA |
> +		   HSIO_HW_CFG_DEV2G5_10_MODE, 0),
> +	SERDES_MUX(SERDES6G_2, 10, PHY_MODE_PCIE, HSIO_HW_CFG_PCIE_ENA,
> +		   HSIO_HW_CFG_PCIE_ENA),
> +};
> +
> +static int serdes_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> +	struct serdes_macro *macro = phy_get_drvdata(phy);
> +	int ret, i;

unsigned int i;

> +
> +	for (i = 0; i < ARRAY_SIZE(ocelot_serdes_muxes); i++) {
> +		if (macro->idx != ocelot_serdes_muxes[i].idx ||
> +		    mode != ocelot_serdes_muxes[i].mode)
> +			continue;
> +
> +		if (mode != PHY_MODE_QSGMII &&
> +		    macro->port != ocelot_serdes_muxes[i].port)
> +			continue;
> +
> +		ret = regmap_update_bits(macro->ctrl->regs, HSIO_HW_CFG,
> +					 ocelot_serdes_muxes[i].mask,
> +					 ocelot_serdes_muxes[i].mux);
> +		if (ret)
> +			return ret;
> +
> +		if (macro->idx < SERDES1G_MAX)
> +			return serdes_init_s1g(macro->ctrl->regs, macro->idx);
> +
> +		/* SERDES6G and PCIe not supported yet */
> +		return 0;

Would not returning -EOPNOTSUPP be more helpful rather than leaving the
PHY unconfigured (or did the bootloader somehow configure it before for us)?

> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct phy_ops serdes_ops = {
> +	.set_mode	= serdes_set_mode,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *serdes_simple_xlate(struct device *dev,
> +				       struct of_phandle_args *args)
> +{
> +	struct serdes_ctrl *ctrl = dev_get_drvdata(dev);
> +	int port, idx, i;

unsigned int port, idx, i;

[snip]


> +
> +static int serdes_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *provider;
> +	struct serdes_ctrl *ctrl;
> +	int i, ret;

unsigned int i;

> +
> +	ctrl = devm_kzalloc(&pdev->dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->dev = &pdev->dev;
> +	ctrl->regs = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (!ctrl->regs)
> +		return -ENODEV;
> +
> +	for (i = 0; i <= SERDES_MAX; i++) {

Every other loop you have is using <, is this one off-by-one?
Florian Fainelli Sept. 15, 2018, 9:25 p.m. UTC | #7 
On 09/14/18 01:16, Quentin Schulz wrote:
> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> controller driver. Now, the SerDes muxing is configured within the
> Device Tree and is enforced in the MAC controller driver so we can have
> a lot of different SerDes configurations.
> 
> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> according to the SerDes<->switch port mapping and the communication mode
> with the Ethernet PHY.

This looks good, just a few comments below:

[snip]

> +		err = of_get_phy_mode(portnp);
> +		if (err < 0)
> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> +		else
> +			ocelot->ports[port]->phy_mode = err;
> +
> +		switch (ocelot->ports[port]->phy_mode) {
> +		case PHY_INTERFACE_MODE_NA:
> +			continue;

Would not you want to issue a message indicating that the Device Tree
must be updated here? AFAICT with your patch series, this should no
longer be a condition that you will hit unless you kept the old DTB
around, right?

> +		case PHY_INTERFACE_MODE_SGMII:
> +			phy_mode = PHY_MODE_SGMII;
> +			break;
> +		case PHY_INTERFACE_MODE_QSGMII:
> +			phy_mode = PHY_MODE_QSGMII;
> +			break;
> +		default:
> +			dev_err(ocelot->dev,
> +				"invalid phy mode for port%d, (Q)SGMII only\n",
> +				port);
> +			return -EINVAL;
> +		}
> +
> +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> +		if (IS_ERR(serdes)) {
> +			err = PTR_ERR(serdes);
> +			if (err == -EPROBE_DEFER) {

This can be simplified into:

			if (err == -EPROBE_DEFER)
				dev_dbg();
			else
				dev_err();
			goto err_probe_ports;

> +				dev_dbg(ocelot->dev, "deferring probe\n");
> +				goto err_probe_ports;
> +			}
> +
> +			dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
> +				port);
>  			goto err_probe_ports;
>  		}
> +
> +		ocelot->ports[port]->serdes = serdes;
>  	}
>  
>  	register_netdevice_notifier(&ocelot_netdevice_nb);
>
Quentin Schulz Oct. 1, 2018, 9:42 a.m. UTC | #8 
Hi Florian,

On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
> 
> 
> On 09/14/18 01:16, Quentin Schulz wrote:
> > Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> > controller driver. Now, the SerDes muxing is configured within the
> > Device Tree and is enforced in the MAC controller driver so we can have
> > a lot of different SerDes configurations.
> > 
> > Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> > according to the SerDes<->switch port mapping and the communication mode
> > with the Ethernet PHY.
> 
> This looks good, just a few comments below:
> 
> [snip]
> 
> > +		err = of_get_phy_mode(portnp);
> > +		if (err < 0)
> > +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> > +		else
> > +			ocelot->ports[port]->phy_mode = err;
> > +
> > +		switch (ocelot->ports[port]->phy_mode) {
> > +		case PHY_INTERFACE_MODE_NA:
> > +			continue;
> 
> Would not you want to issue a message indicating that the Device Tree
> must be updated here? AFAICT with your patch series, this should no
> longer be a condition that you will hit unless you kept the old DTB
> around, right?
> 

It'll occur for internal PHYs. On the PCB123[1], there are four of them,
so we need to be able to give no mode in the DT for those. For the
upcoming PCB120, there'll be 4 external PHYs that require a mode in the
DT and 4 internal PHYs that do not require any mode. I could put a debug
message that says this or that PHY is configured as an internal PHY but
I wouldn't put a message that is printed with the default log level.

So I think we should keep it, shouldn't we?

[1] https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/mscc/ocelot_pcb123.dts

> > +		case PHY_INTERFACE_MODE_SGMII:
> > +			phy_mode = PHY_MODE_SGMII;
> > +			break;
> > +		case PHY_INTERFACE_MODE_QSGMII:
> > +			phy_mode = PHY_MODE_QSGMII;
> > +			break;
> > +		default:
> > +			dev_err(ocelot->dev,
> > +				"invalid phy mode for port%d, (Q)SGMII only\n",
> > +				port);
> > +			return -EINVAL;
> > +		}
> > +
> > +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> > +		if (IS_ERR(serdes)) {
> > +			err = PTR_ERR(serdes);
> > +			if (err == -EPROBE_DEFER) {
> 
> This can be simplified into:
> 
> 			if (err == -EPROBE_DEFER)
> 				dev_dbg();
> 			else
> 				dev_err();
> 			goto err_probe_ports;
> 

Indeed, good catch.

Thanks,
Quentin
Quentin Schulz Oct. 1, 2018, 10:02 a.m. UTC | #9 
Hi Florian,

On Sat, Sep 15, 2018 at 02:20:25PM -0700, Florian Fainelli wrote:
> 
> 
> On 09/14/18 01:16, Quentin Schulz wrote:
> > The Microsemi Ocelot can mux SerDes lanes (aka macros) to different
> > switch ports or even make it act as a PCIe interface.
> > 
> > This adds support for the muxing of the SerDes.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> 
> [snip]
> 
> > +
> > +struct serdes_macro {
> > +	u8			idx;
> > +	/* Not used when in QSGMII or PCIe mode */
> > +	int			port;
> 
> u8 port to be consistent with the mux table?
> 

Not wanted in the current implementation.

In serdes_phy_create, I put the port to -1. In serdes_simple_xlate, I
make sure that once port is set to anything else than -1, it cannot be
set again (cannot have 2+ PHYs sharing the same SerDes (except for
SERDES6G_0 which is used for QSGMII)).

I cannot use u8 for this simple reason. However, I'm all ears for a
nicer solution :)

> [snip]
> 
> > +#define SERDES_MUX(_idx, _port, _mode, _mask, _mux) {	\
> > +	.idx = _idx,						\
> > +	.port = _port,						\
> > +	.mode = _mode,						\
> > +	.mask = _mask,						\
> > +	.mux = _mux,						\
> > +}
> > +
> > +static const struct serdes_mux ocelot_serdes_muxes[] = {
> > +	SERDES_MUX(SERDES1G_0, 0, PHY_MODE_SGMII, 0, 0),
> > +	SERDES_MUX(SERDES1G_1, 1, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_5_MODE, 0),
> > +	SERDES_MUX(SERDES1G_1, 5, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> > +		   HSIO_HW_CFG_DEV1G_5_MODE, HSIO_HW_CFG_DEV1G_5_MODE),
> 
> Why not go one step further and define a SERDES_MUX_SGMII() macro which
> automatically resolves the correct bit definitions to use?
> 
> The current macro does not make this particularly easy to read :/
> 

I agree on the readability. But SERDES_MUX_SGMII would basically just
abstract the third argument (mode) and that's it, right? That's still
one argument less but do you see something even more intuitive and more
readable?

[...]

> > +
> > +	for (i = 0; i < ARRAY_SIZE(ocelot_serdes_muxes); i++) {
> > +		if (macro->idx != ocelot_serdes_muxes[i].idx ||
> > +		    mode != ocelot_serdes_muxes[i].mode)
> > +			continue;
> > +
> > +		if (mode != PHY_MODE_QSGMII &&
> > +		    macro->port != ocelot_serdes_muxes[i].port)
> > +			continue;
> > +
> > +		ret = regmap_update_bits(macro->ctrl->regs, HSIO_HW_CFG,
> > +					 ocelot_serdes_muxes[i].mask,
> > +					 ocelot_serdes_muxes[i].mux);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (macro->idx < SERDES1G_MAX)
> > +			return serdes_init_s1g(macro->ctrl->regs, macro->idx);
> > +
> > +		/* SERDES6G and PCIe not supported yet */
> > +		return 0;
> 
> Would not returning -EOPNOTSUPP be more helpful rather than leaving the
> PHY unconfigured (or did the bootloader somehow configure it before for us)?
> 

Yup, you're right, if the SerDes needs to be configured by the kernel,
the user of the SerDes mux is "broken" anyway so it makes sense to
return -EOPNOTSUPP.

[...]

> > +
> > +	ctrl = devm_kzalloc(&pdev->dev, sizeof(*ctrl), GFP_KERNEL);
> > +	if (!ctrl)
> > +		return -ENOMEM;
> > +
> > +	ctrl->dev = &pdev->dev;
> > +	ctrl->regs = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > +	if (!ctrl->regs)
> > +		return -ENODEV;
> > +
> > +	for (i = 0; i <= SERDES_MAX; i++) {
> 
> Every other loop you have is using <, is this one off-by-one?

That is an error.

Thanks,
Quentin
Florian Fainelli Oct. 1, 2018, 4:29 p.m. UTC | #10 
On 10/01/2018 02:42 AM, Quentin Schulz wrote:
> Hi Florian,
> 
> On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
>>
>>
>> On 09/14/18 01:16, Quentin Schulz wrote:
>>> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
>>> controller driver. Now, the SerDes muxing is configured within the
>>> Device Tree and is enforced in the MAC controller driver so we can have
>>> a lot of different SerDes configurations.
>>>
>>> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
>>> according to the SerDes<->switch port mapping and the communication mode
>>> with the Ethernet PHY.
>>
>> This looks good, just a few comments below:
>>
>> [snip]
>>
>>> +		err = of_get_phy_mode(portnp);
>>> +		if (err < 0)
>>> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
>>> +		else
>>> +			ocelot->ports[port]->phy_mode = err;
>>> +
>>> +		switch (ocelot->ports[port]->phy_mode) {
>>> +		case PHY_INTERFACE_MODE_NA:
>>> +			continue;
>>
>> Would not you want to issue a message indicating that the Device Tree
>> must be updated here? AFAICT with your patch series, this should no
>> longer be a condition that you will hit unless you kept the old DTB
>> around, right?
>>
> 
> It'll occur for internal PHYs. On the PCB123[1], there are four of them,
> so we need to be able to give no mode in the DT for those. For the
> upcoming PCB120, there'll be 4 external PHYs that require a mode in the
> DT and 4 internal PHYs that do not require any mode. I could put a debug
> message that says this or that PHY is configured as an internal PHY but
> I wouldn't put a message that is printed with the default log level.
> 
> So I think we should keep it, shouldn't we?

Internal PHYs either use a standard connection internally (e.g: GMII) or
they are using a proprietary connection interface, in which case
phy-mode = "internal" is what we defined to represent those.

> 
> [1] https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> 
>>> +		case PHY_INTERFACE_MODE_SGMII:
>>> +			phy_mode = PHY_MODE_SGMII;
>>> +			break;
>>> +		case PHY_INTERFACE_MODE_QSGMII:
>>> +			phy_mode = PHY_MODE_QSGMII;
>>> +			break;
>>> +		default:
>>> +			dev_err(ocelot->dev,
>>> +				"invalid phy mode for port%d, (Q)SGMII only\n",
>>> +				port);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
>>> +		if (IS_ERR(serdes)) {
>>> +			err = PTR_ERR(serdes);
>>> +			if (err == -EPROBE_DEFER) {
>>
>> This can be simplified into:
>>
>> 			if (err == -EPROBE_DEFER)
>> 				dev_dbg();
>> 			else
>> 				dev_err();
>> 			goto err_probe_ports;
>>
> 
> Indeed, good catch.
> 
> Thanks,
> Quentin
>
Quentin Schulz Oct. 4, 2018, 12:20 p.m. UTC | #11 
Hi Florian,

On Mon, Oct 01, 2018 at 09:29:07AM -0700, Florian Fainelli wrote:
> On 10/01/2018 02:42 AM, Quentin Schulz wrote:
> > Hi Florian,
> > 
> > On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
> >>
> >>
> >> On 09/14/18 01:16, Quentin Schulz wrote:
> >>> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> >>> controller driver. Now, the SerDes muxing is configured within the
> >>> Device Tree and is enforced in the MAC controller driver so we can have
> >>> a lot of different SerDes configurations.
> >>>
> >>> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> >>> according to the SerDes<->switch port mapping and the communication mode
> >>> with the Ethernet PHY.
> >>
> >> This looks good, just a few comments below:
> >>
> >> [snip]
> >>
> >>> +		err = of_get_phy_mode(portnp);
> >>> +		if (err < 0)
> >>> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> >>> +		else
> >>> +			ocelot->ports[port]->phy_mode = err;
> >>> +
> >>> +		switch (ocelot->ports[port]->phy_mode) {
> >>> +		case PHY_INTERFACE_MODE_NA:
> >>> +			continue;
> >>
> >> Would not you want to issue a message indicating that the Device Tree
> >> must be updated here? AFAICT with your patch series, this should no
> >> longer be a condition that you will hit unless you kept the old DTB
> >> around, right?
> >>
> > 
> > It'll occur for internal PHYs. On the PCB123[1], there are four of them,
> > so we need to be able to give no mode in the DT for those. For the
> > upcoming PCB120, there'll be 4 external PHYs that require a mode in the
> > DT and 4 internal PHYs that do not require any mode. I could put a debug
> > message that says this or that PHY is configured as an internal PHY but
> > I wouldn't put a message that is printed with the default log level.
> > 
> > So I think we should keep it, shouldn't we?
> 
> Internal PHYs either use a standard connection internally (e.g: GMII) or
> they are using a proprietary connection interface, in which case
> phy-mode = "internal" is what we defined to represent those.
> 

Just to let you know that I'll send a new version in a few minutes that
does not contain the requested change. I don't have the information yet,
I know it's MII compatible but nothing more.
I haven't forgotten (yet; so don't hesitate to tell me if I do) your
suggestion.

Two thoughts:
1) Doing what you suggested is rather straightforward once I have the
information so it does not impact the actual overall behaviour of the
driver (reviewable as is),

2) The current behaviour aligns with the behaviour induced by the code
snippet above, so we don't break anything or introduce any change in
behaviour. Once I have an answer, I could always send a small patch for
this if the driver gets merged before, which would also change the DT
(to add the phy-mode property).

Thanks,
Quentin