by Quentin Schulz

[permalink] [raw]

Subject: Re: [PATCH v3 3/5] phy: ocelot-serdes: convert to use eth phy mode and submode

Hi Grygorii,

Thanks for the patch!

On Mon, Nov 19, 2018 at 07:24:22PM -0600, Grygorii Strashko wrote:
> Convert ocelot-serdes PHY driver to use recently introduced
> PHY_MODE_ETHERNET and phy_set_mode_ext().
>
> Cc: Quentin Schulz <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>

Reviewed-by: Quentin Schulz <[email protected]>
Tested-by: Quentin Schulz <[email protected]>

Tested on top of latest master branch of net-next
(e432abfb99e5642a7e7fcaa1c8cb0e80c8fcf58e) on a PCB120 with VSC8584 PHYs
(for reference if we ever find out there is a problem with this patch).

> diff --git a/drivers/phy/mscc/phy-ocelot-serdes.c b/drivers/phy/mscc/phy-ocelot-serdes.c
> index c61a9890..77c46f6 100644
> --- a/drivers/phy/mscc/phy-ocelot-serdes.c
> +++ b/drivers/phy/mscc/phy-ocelot-serdes.c
> @@ -11,6 +11,7 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> +#include <linux/phy.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> @@ -104,20 +105,24 @@ struct serdes_mux {
> u8 idx;
> u8 port;
> enum phy_mode mode;
> + int submode;
> u32 mask;
> u32 mux;
> };
>
> -#define SERDES_MUX(_idx, _port, _mode, _mask, _mux) { \
> +#define SERDES_MUX(_idx, _port, _mode, _submode, _mask, _mux) { \
> .idx = _idx, \
> .port = _port, \
> .mode = _mode, \
> + .submode = _submode, \
> .mask = _mask, \
> .mux = _mux, \
> }
>
> -#define SERDES_MUX_SGMII(i, p, m, c) SERDES_MUX(i, p, PHY_MODE_SGMII, m, c)
> -#define SERDES_MUX_QSGMII(i, p, m, c) SERDES_MUX(i, p, PHY_MODE_QSGMII, m, c)
> +#define SERDES_MUX_SGMII(i, p, m, c) \
> + SERDES_MUX(i, p, PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_SGMII, m, c)
> +#define SERDES_MUX_QSGMII(i, p, m, c) \
> + SERDES_MUX(i, p, PHY_MODE_ETHERNET, PHY_INTERFACE_MODE_QSGMII, m, c)
>
> static const struct serdes_mux ocelot_serdes_muxes[] = {
> SERDES_MUX_SGMII(SERDES1G(0), 0, 0, 0),
> @@ -154,7 +159,7 @@ static const struct serdes_mux ocelot_serdes_muxes[] = {
> SERDES_MUX_SGMII(SERDES6G(1), 8, 0, 0),
> SERDES_MUX_SGMII(SERDES6G(2), 10, 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,
> + SERDES_MUX(SERDES6G(2), 10, PHY_MODE_PCIE, 0, HSIO_HW_CFG_PCIE_ENA,
> HSIO_HW_CFG_PCIE_ENA),
> };
>
> @@ -164,12 +169,17 @@ static int serdes_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> unsigned int i;
> int ret;
>
> + /* As of now only PHY_MODE_ETHERNET is supported */
> + if (mode != PHY_MODE_ETHERNET)
> + return -EOPNOTSUPP;
> +
> for (i = 0; i < ARRAY_SIZE(ocelot_serdes_muxes); i++) {
> if (macro->idx != ocelot_serdes_muxes[i].idx ||
> - mode != ocelot_serdes_muxes[i].mode)
> + mode != ocelot_serdes_muxes[i].mode ||
> + submode != ocelot_serdes_muxes[i].submode)
> continue;

We will most likely need to rework this to ignore the submode of the
PCIe muxing if the mode is PCIe but let?s figure this out when we add
support for PCIe muxing.

Thanks,
Quentin

Attachments:

(No filename) (3.16 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-21 07:53:22

Hi Grygorii, Kishon,

On Mon, Nov 19, 2018 at 07:24:19PM -0600, Grygorii Strashko wrote:
> Thank you for your review.
> I've not added "Tested-by"/"Acked-by" tags due to code changes in v3.
>
> As was discussed in [1] I'm posting series which introduces rework of
> phy_set_mode to accept phy mode and submode. I've dropped TI specific patches as
> this change is pretty big by itself.
>
> Patch 1 is cumulative change which refactors PHY framework code to
> support dual level PHYs mode configuration - PHY mode and PHY submode. It
> extends .set_mode() callback to support additional parameter "int submode"
> and converts all corresponding PHY drivers to support new .set_mode()
> callback declaration.
> The new extended PHY API
> int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode)
> is introduced to support dual level PHYs mode configuration and existing
> phy_set_mode() API is converted to macros, so PHY framework consumers do
> not need to be changed (~21 matches).
>
> Patches 2-4: Add new PHY's mode to be used by Ethernet PHY interface drivers or
> multipurpose PHYs like serdes and convert ocelot-serdes and mvebu-cp110-comphy
> PHY drivers to use recently introduced PHY_MODE_ETHERNET and phy_set_mode_ext().
>
> Patch 5 - removes unused, ethernet specific phy modes from enum phy_mode.
>
> Testing:
> - series tested on TI am335x/am437x/am5(dra7) paltforms.
> - other driver build tested.

I realise I'm a bit late to the party, but while working on the D-PHY
support, I noticed a few things that could be improved.

I guess the main issue is that the sub-mode is completely opaque to
the generic phy framework now. This might not be a big issue, and I
assume that it has been done that way because the net framework
already has a define for the submode it wants.

However, this creates a bunch of drawbacks at the phy framework level:

- phy_set_mode will now pass a submode of 0, all the time. This means
that the behaviour is undefined for all the modes not using the
submodes at the moment, and phy_interface_t seems to have the value
PHY_INTERFACE_MODE_NA matching 0, but I guess this could change in
the future (or the guarantee is not documented anywhere).
- on a similar note, there's no documentation for which value to pass
to phy_set_mode_ext when used with something else than a
PHY_MODE_ETHERNET.
- at the provider level, if you're supporting a phy that isn't using
the submodes, you have no way to filter out or reject any subnode,
since you have no idea what the "no submode" value is.

I guess this can be addressed by:

A) defining a generic phy framework wide unused / invalid phy submode,
that wouldn't collide with the subnode values (such as -1?), and
making phy_set_mode_ext use that.

B) moving the phy submodes definition to the generic phy headers. This
would allow to have a documented, obvious link between a mode and its
subnodes, for all the actors involved (consumer, provider, and
framework) without prior knowledge.

C) Document what the submodes expectations are

What do you think?
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (3.18 kB)
signature.asc (235.00 B)
Download all attachments