| Message ID | 1585230682-24417-1-git-send-email-florinel.iordache@nxp.com |
|---|---|
| Headers | show |
| Series | net: ethernet backplane support | expand |
From: Florinel Iordache <florinel.iordache@nxp.com> Date: Thu, 26 Mar 2020 15:51:19 +0200 > +static void kr_reset_master_lane(struct kr_lane_info *krln) > +{ > + struct phy_device *bpphy = krln->bpphy; > + struct backplane_phy_info *bp_phy = bpphy->priv; > + const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops; Please use reverse christmas tree ordering for local variables. Please audit your entire submission for this issue. Thank you.
On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote: > From: Florinel Iordache <florinel.iordache@nxp.com> > Date: Thu, 26 Mar 2020 15:51:19 +0200 > > > +static void kr_reset_master_lane(struct kr_lane_info *krln) > > +{ > > + struct phy_device *bpphy = krln->bpphy; > > + struct backplane_phy_info *bp_phy = bpphy->priv; > > + const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops; > > Please use reverse christmas tree ordering for local variables. How (any why) do you suggest the first 2 entries here should be ordered?
From: Joe Perches <joe@perches.com> Date: Thu, 26 Mar 2020 11:55:17 -0700 > On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote: >> From: Florinel Iordache <florinel.iordache@nxp.com> >> Date: Thu, 26 Mar 2020 15:51:19 +0200 >> >> > +static void kr_reset_master_lane(struct kr_lane_info *krln) >> > +{ >> > + struct phy_device *bpphy = krln->bpphy; >> > + struct backplane_phy_info *bp_phy = bpphy->priv; >> > + const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops; >> >> Please use reverse christmas tree ordering for local variables. > > How (any why) do you suggest the first 2 entries here > should be ordered? You have to sometimes put assignments into the code body rather than the declarations in situations like this.
On Thu, 2020-03-26 at 12:07 -0700, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Thu, 26 Mar 2020 11:55:17 -0700 > > > On Thu, 2020-03-26 at 11:53 -0700, David Miller wrote: > >> From: Florinel Iordache <florinel.iordache@nxp.com> > >> Date: Thu, 26 Mar 2020 15:51:19 +0200 > >> > >> > +static void kr_reset_master_lane(struct kr_lane_info *krln) > >> > +{ > >> > + struct phy_device *bpphy = krln->bpphy; > >> > + struct backplane_phy_info *bp_phy = bpphy->priv; > >> > + const struct lane_io_ops *lane_ops = krln->bp_phy->bp_dev.lane_ops; > >> > >> Please use reverse christmas tree ordering for local variables. > > > > How (any why) do you suggest the first 2 entries here > > should be ordered? > > You have to sometimes put assignments into the code body rather than > the declarations in situations like this. No "why" reply given. An option is not using reverse christmas tree to both avoid ordering constraints and reduce overall line count. I think this is your own personal taste rather than an actual valuable addition for subsystem maintenance. And if this a serious requirement for the subsystem you oversee, you should add it to something like a maintainer-entry-profile with a "P:" line in MAINTAINERS. https://www.kernel.org/doc/html/latest/maintainer/maintainer-entry-profile.html
On Thu, 2020-03-26 at 15:51 +0200, Florinel Iordache wrote: > Enable backplane support for qoriq family of devices trivial notes: > diff --git a/drivers/net/phy/backplane/qoriq_backplane.c b/drivers/net/phy/backplane/qoriq_backplane.c [] > +static int qoriq_backplane_probe(struct phy_device *bpphy) > +{ > + static bool one_time_action = true; > + > + if (one_time_action) { > + one_time_action = false; > + pr_info("%s: QorIQ Backplane driver version %s\n", > + QORIQ_BACKPLANE_DRIVER_NAME, > + QORIQ_BACKPLANE_DRIVER_VERSION); > + } There is an existing mechanism for this: pr_info_once("%s: ... %s\n", etc...); [] > +static int qoriq_backplane_config_init(struct phy_device *bpphy) > +{ [] > + for (i = 0; i < bp_phy->num_lanes; i++) { [] > + ret = of_address_to_resource(lane_node, 0, &res); > + if (ret) { > + bpdev_err(bpphy, > + "could not obtain lane memory map for index=%d, ret = %d\n", > + i, ret); > + return ret; This could use the new vsprintf %pe extension: bpdev_err(bpphy, "could not obtain lane memory map for index=%d, %pe\n", i, ERR_PTR(ret)); > + ret = of_address_to_resource(serdes_node, 0, &res); > + if (ret) { > + bpdev_err(bpphy, > + "could not obtain serdes memory map, ret = %d\n", > + ret); > + return ret; %pe etc. [] > + for (i = 0; i < comp_no; i++) { > + ret = of_property_read_string_index(serdes_node, "compatible", > + i, &serdes_comp); > + if (ret == 0) { > + if (!strcasecmp(serdes_comp, "serdes-10g")) { > + serdes_type = SERDES_10G; > + break; > + } else if (!strcasecmp(serdes_comp, "serdes-28g")) { > + serdes_type = SERDES_28G; > + break; > + } > + } > + } [] > +static int qoriq_backplane_match_phy_device(struct phy_device *bpphy) > +{ [] > + for (i = 0; i < comp_no; i++) { > + ret = of_property_read_string_index(serdes_node, "compatible", > + i, &serdes_comp); > + if (ret == 0) { > + if (!strcasecmp(serdes_comp, "serdes-10g")) { > + serdes_type = SERDES_10G; > + break; > + } else if (!strcasecmp(serdes_comp, "serdes-28g")) { > + serdes_type = SERDES_28G; > + break; > + } > + } > + } [] Maybe add and use a helper function?
> > +static int qoriq_backplane_config_init(struct phy_device *bpphy) > > +{ > [] > > + for (i = 0; i < bp_phy->num_lanes; i++) { > [] > > + ret = of_address_to_resource(lane_node, 0, &res); > > + if (ret) { > > + bpdev_err(bpphy, > > + "could not obtain lane memory map for index=%d, ret = %d\n", > > + i, ret); > > + return ret; > > This could use the new vsprintf %pe extension: Hi Joe Probably a FAQ. But is there plans to extend vsprintf to take an int errno value, rather than having to do this ugly ERR_PTR(ret) every time? Format string %de ? Andrew
On Thu, 2020-03-26 at 21:13 +0100, Andrew Lunn wrote: > > > +static int qoriq_backplane_config_init(struct phy_device *bpphy) > > > +{ > > [] > > > + for (i = 0; i < bp_phy->num_lanes; i++) { > > [] > > > + ret = of_address_to_resource(lane_node, 0, &res); > > > + if (ret) { > > > + bpdev_err(bpphy, > > > + "could not obtain lane memory map for index=%d, ret = %d\n", > > > + i, ret); > > > + return ret; > > > > This could use the new vsprintf %pe extension: > > Hi Joe Hello Andrew. > Probably a FAQ. But is there plans to extend vsprintf to take an int > errno value, rather than having to do this ugly ERR_PTR(ret) every > time? Format string %de ? Uwe Kleine-König proposed one a couple times. https://lkml.org/lkml/2019/8/27/1456 Though I believe one %<type><extra> extension mechanism in vsprintf is enough. At least the %p<foo> use is unlikely to ever have desired but dropped output after an output of %p. It seems less true for %[diux].
On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > Add support for backplane kr phy connection types currently available > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > the cases for KR modes which are clause 45 compatible to correctly assign > phy_interface and phylink#supported) > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > drivers/net/phy/phylink.c | 15 ++++++++++++--- > include/linux/phy.h | 6 +++++- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index fed0c59..db1bb87 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -4,6 +4,7 @@ > * technologies such as SFP cages where the PHY is hot-pluggable. > * > * Copyright (C) 2015 Russell King > + * Copyright 2020 NXP > */ > #include <linux/ethtool.h> > #include <linux/export.h> > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode) > break; > > case PHY_INTERFACE_MODE_USXGMII: > - case PHY_INTERFACE_MODE_10GKR: We might have a backwards compatibility issue here. If i remember correctly, there are some boards out in the wild using PHY_INTERFACE_MODE_10GKR not PHY_INTERFACE_MODE_10GBASER. See e0f909bc3a242296da9ccff78277f26d4883a79d Russell, what do you say about this? Andrew
On 3/26/2020 6:51 AM, Florinel Iordache wrote: > Add support for backplane kr phy connection types currently available > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > the cases for KR modes which are clause 45 compatible to correctly assign > phy_interface and phylink#supported) > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > drivers/net/phy/phylink.c | 15 ++++++++++++--- > include/linux/phy.h | 6 +++++- > 2 files changed, 17 insertions(+), 4 deletions(-) Please remember to update Documentation/networking/phy.rst, and Documentation/ABI/testing/sysfs-class-net-phydev with these new PHY interface values.
> +static u32 le_ioread32(void __iomem *reg) > +{ > + return ioread32(reg); > +} > + > +static void le_iowrite32(u32 value, void __iomem *reg) > +{ > + iowrite32(value, reg); > +} > + > +static u32 be_ioread32(void __iomem *reg) > +{ > + return ioread32be(reg); > +} > + > +static void be_iowrite32(u32 value, void __iomem *reg) > +{ > + iowrite32be(value, reg); > +} This is very surprising to me. I've not got my head around the structure of this code yet, but i'm surprised to see memory mapped access functions in generic code. Andrew
On Fri, Mar 27, 2020 at 01:15:15AM +0100, Andrew Lunn wrote: > On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > > Add support for backplane kr phy connection types currently available > > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > > the cases for KR modes which are clause 45 compatible to correctly assign > > phy_interface and phylink#supported) > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > --- > > drivers/net/phy/phylink.c | 15 ++++++++++++--- > > include/linux/phy.h | 6 +++++- > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index fed0c59..db1bb87 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -4,6 +4,7 @@ > > * technologies such as SFP cages where the PHY is hot-pluggable. > > * > > * Copyright (C) 2015 Russell King > > + * Copyright 2020 NXP > > */ > > #include <linux/ethtool.h> > > #include <linux/export.h> > > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode) > > break; > > > > case PHY_INTERFACE_MODE_USXGMII: > > - case PHY_INTERFACE_MODE_10GKR: > > We might have a backwards compatibility issue here. If i remember > correctly, there are some boards out in the wild using > PHY_INTERFACE_MODE_10GKR not PHY_INTERFACE_MODE_10GBASER. > > See e0f909bc3a242296da9ccff78277f26d4883a79d > > Russell, what do you say about this? Yes, and that's a point that I made when I introduced 10GBASER to correct that mistake. It is way too soon to change this; it will definitely cause regressions: $ grep 10gbase-kr arch/*/boot/dts -r arch/arm64/boot/dts/marvell/cn9131-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-7040-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/cn9132-db.dts: phy-mode = "10gbase-kr"; arch/arm64/boot/dts/marvell/cn9130-db.dts: phy-mode = "10gbase-kr"; So any change to the existing PHY_INTERFACE_MODE_10GKR will likely break all these platforms.
On Thu, Mar 26, 2020 at 03:51:22PM +0200, Florinel Iordache wrote: > Add dt nodes with serdes, lanes, mdio generic description for supported > platforms: ls1046, ls1088, ls2088, lx2160. This is a prerequisite to > enable backplane on device tree for these platforms. > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 33 ++++- > arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 97 ++++++++++++- > arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi | 160 ++++++++++++++++++++- > arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 128 ++++++++++++++++- > .../boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi | 5 +- > .../boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi | 5 +- > 6 files changed, 418 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > index d4c1da3..c7d845f 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > @@ -3,7 +3,7 @@ > * Device Tree Include file for Freescale Layerscape-1046A family SoC. > * > * Copyright 2016 Freescale Semiconductor, Inc. > - * Copyright 2018 NXP > + * Copyright 2018, 2020 NXP > * > * Mingkai Hu <mingkai.hu@nxp.com> > */ > @@ -735,6 +735,37 @@ > status = "disabled"; > }; > > + serdes1: serdes@1ea0000 { > + compatible = "serdes-10g"; > + reg = <0x0 0x1ea0000 0 0x00002000>; > + reg-names = "serdes", "serdes-10g"; > + big-endian; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x00 0x1ea0000 0x00002000>; > + lane_a: lane@800 { > + compatible = "lane-10g"; > + reg = <0x800 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_b: lane@840 { > + compatible = "lane-10g"; > + reg = <0x840 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_c: lane@880 { > + compatible = "lane-10g"; > + reg = <0x880 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_d: lane@8c0 { > + compatible = "lane-10g"; > + reg = <0x8c0 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + }; > + > pcie_ep@3600000 { > compatible = "fsl,ls1046a-pcie-ep", "fsl,ls-pcie-ep"; > reg = <0x00 0x03600000 0x0 0x00100000 > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi > index 5945662..474464e 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi > @@ -2,7 +2,7 @@ > /* > * Device Tree Include file for NXP Layerscape-1088A family SoC. > * > - * Copyright 2017 NXP > + * Copyright 2017, 2020 NXP > * > * Harninder Rai <harninder.rai@nxp.com> > * > @@ -325,6 +325,69 @@ > #interrupt-cells = <2>; > }; > > + /* WRIOP0: 0x8B8_0000, E-MDIO1: 0x1_6000 */ > + emdio1: mdio@8B96000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8B96000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; /* force the driver in LE mode */ > + > + /* Not necessary on the QDS, but needed on the RDB */ > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + /* WRIOP0: 0x8B8_0000, E-MDIO2: 0x1_7000 */ > + emdio2: mdio@8B97000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8B97000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; /* force the driver in LE mode */ > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio1: mdio@0x8c07000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c07000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio2: mdio@0x8c0b000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0b000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio3: mdio@0x8c0f000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0f000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio4: mdio@0x8c13000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c13000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > ifc: ifc@2240000 { > compatible = "fsl,ifc", "simple-bus"; > reg = <0x0 0x2240000 0x0 0x20000>; > @@ -777,6 +840,38 @@ > }; > }; > }; > + > + serdes1: serdes@1ea0000 { > + compatible = "serdes-10g"; > + reg = <0x0 0x1ea0000 0 0x00002000>; > + reg-names = "serdes", "serdes-10g"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x00 0x1ea0000 0x00002000>; > + lane_a: lane@800 { > + compatible = "lane-10g"; > + reg = <0x800 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_b: lane@840 { > + compatible = "lane-10g"; > + reg = <0x840 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_c: lane@880 { > + compatible = "lane-10g"; > + reg = <0x880 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_d: lane@8c0 { > + compatible = "lane-10g"; > + reg = <0x8c0 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + }; > + > }; > > firmware { > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi > index f96d06d..e8f3026 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi > @@ -3,7 +3,7 @@ > * Device Tree Include file for Freescale Layerscape-2080A family SoC. > * > * Copyright 2016 Freescale Semiconductor, Inc. > - * Copyright 2017 NXP > + * Copyright 2017, 2020 NXP > * > * Abhimanyu Saini <abhimanyu.saini@nxp.com> > * > @@ -560,6 +560,113 @@ > #interrupt-cells = <2>; > }; > > + /* WRIOP0: 0x8B8_0000, E-MDIO1: 0x1_6000 */ > + emdio1: mdio@8B96000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8B96000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; /* force the driver in LE mode */ > + > + /* Not necessary on the QDS, but needed on the RDB */ > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + /* WRIOP0: 0x8B8_0000, E-MDIO2: 0x1_7000 */ > + emdio2: mdio@8B97000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8B97000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; /* force the driver in LE mode */ > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio1: mdio@0x8c07000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c07000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio2: mdio@0x8c0b000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0b000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio3: mdio@0x8c0f000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0f000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio4: mdio@0x8c13000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c13000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio5: mdio@0x8c17000 { > + status = "disabled"; > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c17000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio6: mdio@0x8c1b000 { > + status = "disabled"; > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c1b000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio7: mdio@0x8c1f000 { > + status = "disabled"; > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c1f000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio8: mdio@0x8c23000 { > + status = "disabled"; > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c23000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > i2c0: i2c@2000000 { > status = "disabled"; > compatible = "fsl,vf610-i2c"; > @@ -754,6 +861,57 @@ > snps,incr-burst-type-adjustment = <1>, <4>, <8>, <16>; > }; > > + serdes1: serdes@1ea0000 { > + compatible = "serdes-10g"; > + reg = <0x0 0x1ea0000 0 0x00002000>; > + reg-names = "serdes", "serdes-10g"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x00 0x1ea0000 0x00002000>; > + lane_a: lane@800 { > + compatible = "lane-10g"; > + reg = <0x800 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_b: lane@840 { > + compatible = "lane-10g"; > + reg = <0x840 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_c: lane@880 { > + compatible = "lane-10g"; > + reg = <0x880 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_d: lane@8c0 { > + compatible = "lane-10g"; > + reg = <0x8c0 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_e: lane@900 { > + compatible = "lane-10g"; > + reg = <0x900 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_f: lane@940 { > + compatible = "lane-10g"; > + reg = <0x940 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_g: lane@980 { > + compatible = "lane-10g"; > + reg = <0x980 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_h: lane@9c0 { > + compatible = "lane-10g"; > + reg = <0x9c0 0x40>; > + reg-names = "lane", "serdes-lane"; > + }; > + }; > + > ccn@4000000 { > compatible = "arm,ccn-504"; > reg = <0x0 0x04000000 0x0 0x01000000>; > diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > index e5ee559..2815908 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi > @@ -2,7 +2,7 @@ > // > // Device Tree Include file for Layerscape-LX2160A family SoC. > // > -// Copyright 2018 NXP > +// Copyright 2018, 2020 NXP > > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > @@ -947,9 +947,9 @@ > #address-cells = <1>; > #size-cells = <0>; > little-endian; > - status = "disabled"; > }; > > + /* WRIOP0: 0x8b8_0000, E-MDIO2: 0x1_7000 */ > emdio2: mdio@8b97000 { > compatible = "fsl,fman-memac-mdio"; > reg = <0x0 0x8b97000 0x0 0x1000>; > @@ -957,7 +957,129 @@ > little-endian; > #address-cells = <1>; > #size-cells = <0>; > - status = "disabled"; > + }; > + > + pcs_mdio1: mdio@0x8c07000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c07000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio2: mdio@0x8c0b000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0b000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio3: mdio@0x8c0f000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c0f000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio4: mdio@0x8c13000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c13000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio5: mdio@0x8c17000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c17000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio6: mdio@0x8c1b000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c1b000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio7: mdio@0x8c1f000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c1f000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > + pcs_mdio8: mdio@0x8c23000 { > + compatible = "fsl,fman-memac-mdio"; > + reg = <0x0 0x8c23000 0x0 0x1000>; > + device_type = "mdio"; > + little-endian; > + #address-cells = <1>; > + #size-cells = <0>; > + }; This is a subset of the entries I've posted in my PCS series... > + > + serdes1: serdes@1ea0000 { > + compatible = "serdes-28g"; > + reg = <0x0 0x1ea0000 0 0x00002000>; > + reg-names = "serdes", "serdes-28g"; > + little-endian; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x00 0x1ea0000 0x00002000>; > + lane_a: lane@800 { > + compatible = "lane-28g"; > + reg = <0x800 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_b: lane@900 { > + compatible = "lane-28g"; > + reg = <0x900 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_c: lane@a00 { > + compatible = "lane-28g"; > + reg = <0xa00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_d: lane@b00 { > + compatible = "lane-28g"; > + reg = <0xb00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_e: lane@c00 { > + compatible = "lane-28g"; > + reg = <0xc00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_f: lane@d00 { > + compatible = "lane-28g"; > + reg = <0xd00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_g: lane@e00 { > + compatible = "lane-28g"; > + reg = <0xe00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > + lane_h: lane@f00 { > + compatible = "lane-28g"; > + reg = <0xf00 0x100>; > + reg-names = "lane", "serdes-lane"; > + }; > }; > > fsl_mc: fsl-mc@80c000000 { > diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi > index dbd2fc3..d6191f1 100644 > --- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi > +++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-0.dtsi > @@ -3,6 +3,7 @@ > * QorIQ FMan v3 10g port #0 device tree > * > * Copyright 2012-2015 Freescale Semiconductor Inc. > + * Copyright 2020 NXP > * > */ > > @@ -21,7 +22,7 @@ fman@1a00000 { > fsl,fman-10g-port; > }; > > - ethernet@f0000 { > + mac9: ethernet@f0000 { > cell-index = <0x8>; > compatible = "fsl,fman-memac"; > reg = <0xf0000 0x1000>; > @@ -29,7 +30,7 @@ fman@1a00000 { > pcsphy-handle = <&pcsphy6>; > }; > > - mdio@f1000 { > + mdio9: mdio@f1000 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio"; > diff --git a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi > index 6fc5d25..1f6f28f 100644 > --- a/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi > +++ b/arch/arm64/boot/dts/freescale/qoriq-fman3-0-10g-1.dtsi > @@ -3,6 +3,7 @@ > * QorIQ FMan v3 10g port #1 device tree > * > * Copyright 2012-2015 Freescale Semiconductor Inc. > + * Copyright 2020 NXP > * > */ > > @@ -21,7 +22,7 @@ fman@1a00000 { > fsl,fman-10g-port; > }; > > - ethernet@f2000 { > + mac10: ethernet@f2000 { > cell-index = <0x9>; > compatible = "fsl,fman-memac"; > reg = <0xf2000 0x1000>; > @@ -29,7 +30,7 @@ fman@1a00000 { > pcsphy-handle = <&pcsphy7>; > }; > > - mdio@f3000 { > + mdio10: mdio@f3000 { > #address-cells = <1>; > #size-cells = <0>; > compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio"; > -- > 1.9.1 > >
> -----Original Message----- > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > Behalf Of Russell King - ARM Linux admin > Sent: Friday, March 27, 2020 2:02 PM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Florinel Iordache <florinel.iordache@nxp.com>; davem@davemloft.net; > netdev@vger.kernel.org; f.fainelli@gmail.com; hkallweit1@gmail.com; > devicetree@vger.kernel.org; linux-doc@vger.kernel.org; robh+dt@kernel.org; > mark.rutland@arm.com; kuba@kernel.org; corbet@lwn.net; > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; Madalin Bucur (OSS) > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH net-next 3/9] net: phy: add kr phy connection type > > On Fri, Mar 27, 2020 at 01:15:15AM +0100, Andrew Lunn wrote: > > On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > > > Add support for backplane kr phy connection types currently available > > > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > > > the cases for KR modes which are clause 45 compatible to correctly > assign > > > phy_interface and phylink#supported) > > > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > > --- > > > drivers/net/phy/phylink.c | 15 ++++++++++++--- > > > include/linux/phy.h | 6 +++++- > > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > > index fed0c59..db1bb87 100644 > > > --- a/drivers/net/phy/phylink.c > > > +++ b/drivers/net/phy/phylink.c > > > @@ -4,6 +4,7 @@ > > > * technologies such as SFP cages where the PHY is hot-pluggable. > > > * > > > * Copyright (C) 2015 Russell King > > > + * Copyright 2020 NXP > > > */ > > > #include <linux/ethtool.h> > > > #include <linux/export.h> > > > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, > struct fwnode_handle *fwnode) > > > break; > > > > > > case PHY_INTERFACE_MODE_USXGMII: > > > - case PHY_INTERFACE_MODE_10GKR: > > > > We might have a backwards compatibility issue here. If i remember > > correctly, there are some boards out in the wild using > > PHY_INTERFACE_MODE_10GKR not PHY_INTERFACE_MODE_10GBASER. > > > > See e0f909bc3a242296da9ccff78277f26d4883a79d > > > > Russell, what do you say about this? > > Yes, and that's a point that I made when I introduced 10GBASER to > correct that mistake. It is way too soon to change this; it will > definitely cause regressions: > > $ grep 10gbase-kr arch/*/boot/dts -r > arch/arm64/boot/dts/marvell/cn9131-db.dts: phy-mode = "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = > "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = > "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-7040-db.dts: phy-mode = "10gbase- > kr"; > arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = > "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = > "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts: phy-mode = > "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; > arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; > arch/arm64/boot/dts/marvell/cn9132-db.dts: phy-mode = "10gbase-kr"; > arch/arm64/boot/dts/marvell/cn9130-db.dts: phy-mode = "10gbase-kr"; > > So any change to the existing PHY_INTERFACE_MODE_10GKR will likely > break all these platforms. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps > up Hi Russell, I hoped a fix for those would be in by now, it's not useful to leave them like that. We have a similar situation, where all boards using XFI interfaces contain phy-connection-type="xgmii" for a long time now but that did not stop anyone from adding a warning in the Aquantia driver: + WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII, + "Your devicetree is out of date, please update it. The AQR107 family doesn't support XGMII, maybe you mean USXGMII.\n"); + Maybe we need a warning added here too, until the proper phy-mode is used for these boards, to allow for a transition period. Madalin
On Fri, Mar 27, 2020 at 12:12:37PM +0000, Madalin Bucur (OSS) wrote: > > -----Original Message----- > > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > > Behalf Of Russell King - ARM Linux admin > > Sent: Friday, March 27, 2020 2:02 PM > > To: Andrew Lunn <andrew@lunn.ch> > > Cc: Florinel Iordache <florinel.iordache@nxp.com>; davem@davemloft.net; > > netdev@vger.kernel.org; f.fainelli@gmail.com; hkallweit1@gmail.com; > > devicetree@vger.kernel.org; linux-doc@vger.kernel.org; robh+dt@kernel.org; > > mark.rutland@arm.com; kuba@kernel.org; corbet@lwn.net; > > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; Madalin Bucur (OSS) > > <madalin.bucur@oss.nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH net-next 3/9] net: phy: add kr phy connection type > > > > On Fri, Mar 27, 2020 at 01:15:15AM +0100, Andrew Lunn wrote: > > > On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > > > > Add support for backplane kr phy connection types currently available > > > > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > > > > the cases for KR modes which are clause 45 compatible to correctly > > assign > > > > phy_interface and phylink#supported) > > > > > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > > > --- > > > > drivers/net/phy/phylink.c | 15 ++++++++++++--- > > > > include/linux/phy.h | 6 +++++- > > > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > > > index fed0c59..db1bb87 100644 > > > > --- a/drivers/net/phy/phylink.c > > > > +++ b/drivers/net/phy/phylink.c > > > > @@ -4,6 +4,7 @@ > > > > * technologies such as SFP cages where the PHY is hot-pluggable. > > > > * > > > > * Copyright (C) 2015 Russell King > > > > + * Copyright 2020 NXP > > > > */ > > > > #include <linux/ethtool.h> > > > > #include <linux/export.h> > > > > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, > > struct fwnode_handle *fwnode) > > > > break; > > > > > > > > case PHY_INTERFACE_MODE_USXGMII: > > > > - case PHY_INTERFACE_MODE_10GKR: > > > > > > We might have a backwards compatibility issue here. If i remember > > > correctly, there are some boards out in the wild using > > > PHY_INTERFACE_MODE_10GKR not PHY_INTERFACE_MODE_10GBASER. > > > > > > See e0f909bc3a242296da9ccff78277f26d4883a79d > > > > > > Russell, what do you say about this? > > > > Yes, and that's a point that I made when I introduced 10GBASER to > > correct that mistake. It is way too soon to change this; it will > > definitely cause regressions: > > > > $ grep 10gbase-kr arch/*/boot/dts -r > > arch/arm64/boot/dts/marvell/cn9131-db.dts: phy-mode = "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = > > "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts: phy-mode = > > "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-7040-db.dts: phy-mode = "10gbase- > > kr"; > > arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = > > "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dts: phy-mode = > > "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts: phy-mode = > > "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; > > arch/arm64/boot/dts/marvell/armada-8040-db.dts: phy-mode = "10gbase-kr"; > > arch/arm64/boot/dts/marvell/cn9132-db.dts: phy-mode = "10gbase-kr"; > > arch/arm64/boot/dts/marvell/cn9130-db.dts: phy-mode = "10gbase-kr"; > > > > So any change to the existing PHY_INTERFACE_MODE_10GKR will likely > > break all these platforms. > > Hi Russell, > > I hoped a fix for those would be in by now, it's not useful to leave them like > that. I haven't had the time to address the ones I know about, sorry. However, there are some platforms in that list which I've no knowledge of, which I therefore can't change. > We have a similar situation, where all boards using XFI interfaces contain > phy-connection-type="xgmii" for a long time now but that did not stop anyone from > adding a warning in the Aquantia driver: > > + WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII, > + "Your devicetree is out of date, please update it. The AQR107 family doesn't support XGMII, maybe you mean USXGMII.\n"); > + > > Maybe we need a warning added here too, until the proper phy-mode is used for > these boards, to allow for a transition period. Adding a warning can only be done once the current users have been updated, otherwise it's technically introducing a regression. Plus some users may actually be correct. I never did get to the bottom of that, because that required discussion and no one seems willing to discuss it.
> > +static u32 le_ioread32(void __iomem *reg) { > > + return ioread32(reg); > > +} > > + > > +static void le_iowrite32(u32 value, void __iomem *reg) { > > + iowrite32(value, reg); > > +} > > + > > +static u32 be_ioread32(void __iomem *reg) { > > + return ioread32be(reg); > > +} > > + > > +static void be_iowrite32(u32 value, void __iomem *reg) { > > + iowrite32be(value, reg); > > +} > > This is very surprising to me. I've not got my head around the structure of this > code yet, but i'm surprised to see memory mapped access functions in generic > code. > > Andrew Hi Andrew, This is part of the framework used to automatically setup desired I/O callbacks for memory access according to device specific endianness which is specified in the specific device tree (DTS). This approach (explained below) was used to avoid the potential redundant code related to memory access LE/BE which should be similar for all devices. This portion of code is just preparing these four static IO routines for specific endianness access LE/BE which are then installed as callbacks by the generic driver on generic DT parsing routine: backplane_parse_dt according to endianness flag: + /* setup ioread/iowrite according to endianness */ + if (bp_phy->bp_dev.is_little_endian) { + bp_phy->bp_dev.io.read32 = le_ioread32; + bp_phy->bp_dev.io.write32 = le_iowrite32; + } else { + bp_phy->bp_dev.io.read32 = be_ioread32; + bp_phy->bp_dev.io.write32 = be_iowrite32; + } These io callbacks are setup in the following structure: +/* Endianness specific memory I/O operations */ struct mem_io_ops { which is part of generic structure: +/* Backplane device info */ +struct backplane_dev_info { +. . . + struct mem_io_ops io; which in the end will be used directly by the device specific code for specific memory access according to specific endianness specified in the DTS. The endianness flag must be correctly set by the device specific code before calling the generic function backplane_parse_dt, according to device specific endianness specified in the specific device tree DTS: +bp_phy->bp_dev.is_little_endian = of_property_read_bool(serdes_node, "little-endian"); This action to setup desired IO callbacks could also be performed in the specific device code but by using this framework the process is more automatic, it's reusing the logic and therefore decreasing the overall LOC required. If any specific device is doing this action by itself (which is a similar action regardless the specific device) then we will end up with a lot of redundant code. Florin.
On Fri, Mar 27, 2020 at 01:02:17PM +0000, Florinel Iordache wrote: > > > +static u32 le_ioread32(void __iomem *reg) { > > > + return ioread32(reg); > > > +} > > > + > > > +static void le_iowrite32(u32 value, void __iomem *reg) { > > > + iowrite32(value, reg); > > > +} > > > + > > > +static u32 be_ioread32(void __iomem *reg) { > > > + return ioread32be(reg); > > > +} > > > + > > > +static void be_iowrite32(u32 value, void __iomem *reg) { > > > + iowrite32be(value, reg); > > > +} > > > > This is very surprising to me. I've not got my head around the structure of this > > code yet, but i'm surprised to see memory mapped access functions in generic > > code. > > > > Andrew > > Hi Andrew, > > This is part of the framework used to automatically setup desired I/O > callbacks for memory access according to device specific endianness > which is specified in the specific device tree (DTS). > This approach (explained below) was used to avoid the potential > redundant code related to memory access LE/BE which should be > similar for all devices. All devices which are using mmio. I assume the standard does not say anything about memory mapped IO. It talks just about MDIO registers. I would expect the generic code to just have generic accessors, which could work for MMIO, yet more MDIO registers, i2c, spi, etc. So add another support file which adds an MMIO implementation of these generic access functions. Any driver which uses MMIO can pull it in. The same should be true for the DT binding. Don't assume MMIO in the generic binding. > This portion of code is just preparing these four static IO routines > for specific endianness access LE/BE Linux has a lot of MMIO accessors. Are you sure there is not one which will do the right thing, making use of cpu_le32() or cpu_be32() etc. Or are there different variants of the hardware, with some using BE registers and some using LE registers? Note, this is all about the endianness of the register, not the endianness of the cpu. cpu_le32() will be a NOP when the CPU is running LE. Andrew
> +/* Backplane custom logging */ > +#define BPDEV_LOG(name) \ > + char log_buffer[LOG_BUFFER_SIZE]; \ > + va_list args; va_start(args, msg); \ > + vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \ > + if (!bpphy->attached_dev) \ > + dev_##name(&bpphy->mdio.dev, log_buffer); \ > + else \ > + dev_##name(&bpphy->mdio.dev, "%s: %s", \ > + netdev_name(bpphy->attached_dev), log_buffer); \ > + va_end(args) > +void bpdev_err(struct phy_device *bpphy, char *msg, ...) > +{ > + BPDEV_LOG(err); > +} > +EXPORT_SYMBOL(bpdev_err); > + > +void bpdev_warn(struct phy_device *bpphy, char *msg, ...) > +{ > + BPDEV_LOG(warn); > +} > +EXPORT_SYMBOL(bpdev_warn); > + > +void bpdev_info(struct phy_device *bpphy, char *msg, ...) > +{ > + BPDEV_LOG(info); > +} > +EXPORT_SYMBOL(bpdev_info); > + > +void bpdev_dbg(struct phy_device *bpphy, char *msg, ...) > +{ > + BPDEV_LOG(dbg); > +} > +EXPORT_SYMBOL(bpdev_dbg); You are currently modelling this as a phydev. So please just use phydev_err(), phydev_info(), phydev_dbg() etc. Also, if you look at other PHY code, struct phy_device * is nearly always called phydev. Please try to be consistent with the existing code base. Andrew
> +/* Read AN Link Status */ > +static int is_an_link_up(struct phy_device *bpphy) > +{ > + struct backplane_phy_info *bp_phy = bpphy->priv; > + int ret, val = 0; > + > + mutex_lock(&bp_phy->bpphy_lock); > + > + /* Read twice because Link_Status is LL (Latched Low) bit */ > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status); > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status); > + > + mutex_unlock(&bp_phy->bpphy_lock); How does this mutex interact with phydev->lock? It appears both are trying to do the same thing, serialise access to the PHY hardware. Andrew
On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote: > Add support for backplane kr generic driver including link training > (ieee802.3ap/ba) and fixed equalization algorithm > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > drivers/net/phy/Kconfig | 2 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/backplane/Kconfig | 20 + > drivers/net/phy/backplane/Makefile | 9 + > drivers/net/phy/backplane/backplane.c | 1538 +++++++++++++++++++++++++++ > drivers/net/phy/backplane/backplane.h | 262 +++++ > drivers/net/phy/backplane/eq_fixed.c | 83 ++ > drivers/net/phy/backplane/equalization.h | 282 +++++ > drivers/net/phy/backplane/link_training.c | 1604 +++++++++++++++++++++++++++++ > drivers/net/phy/backplane/link_training.h | 34 + > 10 files changed, 3835 insertions(+) > create mode 100644 drivers/net/phy/backplane/Kconfig > create mode 100644 drivers/net/phy/backplane/Makefile > create mode 100644 drivers/net/phy/backplane/backplane.c > create mode 100644 drivers/net/phy/backplane/backplane.h > create mode 100644 drivers/net/phy/backplane/eq_fixed.c > create mode 100644 drivers/net/phy/backplane/equalization.h > create mode 100644 drivers/net/phy/backplane/link_training.c > create mode 100644 drivers/net/phy/backplane/link_training.h > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index cc7f1df..abab4e5 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -523,6 +523,8 @@ config XILINX_GMII2RGMII > the Reduced Gigabit Media Independent Interface(RGMII) between > Ethernet physical media devices and the Gigabit Ethernet controller. > > +source "drivers/net/phy/backplane/Kconfig" > + > endif # PHYLIB > > config MICREL_KS8995MA > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index 70774ab..0b867fb 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -101,3 +101,4 @@ obj-$(CONFIG_STE10XP) += ste10Xp.o > obj-$(CONFIG_TERANETICS_PHY) += teranetics.o > obj-$(CONFIG_VITESSE_PHY) += vitesse.o > obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o > +obj-$(CONFIG_ETH_BACKPLANE) += backplane/ > diff --git a/drivers/net/phy/backplane/Kconfig b/drivers/net/phy/backplane/Kconfig > new file mode 100644 > index 0000000..9ec54b5 > --- /dev/null > +++ b/drivers/net/phy/backplane/Kconfig > @@ -0,0 +1,20 @@ > +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > +config ETH_BACKPLANE > + tristate "Ethernet Backplane support" > + depends on OF_MDIO > + help > + This module provides driver support for Ethernet Operation over > + Electrical Backplanes. It includes Backplane generic > + driver including support for Link Training (IEEE802.3ap/ba). > + Based on the link quality, a signal equalization is required. > + The standard specifies that a start-up algorithm should be in place > + in order to get the link up. > + > +config ETH_BACKPLANE_FIXED > + tristate "Fixed: No Equalization algorithm" > + depends on ETH_BACKPLANE > + help > + This module provides a driver to setup fixed user configurable > + coefficient values for backplanes equalization. This means > + No Equalization algorithm is used to adapt the initial coefficients > + initially set by the user. > \ No newline at end of file > diff --git a/drivers/net/phy/backplane/Makefile b/drivers/net/phy/backplane/Makefile > new file mode 100644 > index 0000000..ded6f2d > --- /dev/null > +++ b/drivers/net/phy/backplane/Makefile > @@ -0,0 +1,9 @@ > +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > +# > +# Makefile for Ethernet Backplane driver > +# > + > +obj-$(CONFIG_ETH_BACKPLANE) += eth_backplane.o > +obj-$(CONFIG_ETH_BACKPLANE_FIXED) += eq_fixed.o > + > +eth_backplane-objs := backplane.o link_training.o > diff --git a/drivers/net/phy/backplane/backplane.c b/drivers/net/phy/backplane/backplane.c > new file mode 100644 > index 0000000..1b580bc > --- /dev/null > +++ b/drivers/net/phy/backplane/backplane.c > @@ -0,0 +1,1538 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > +/* Backplane driver > + * > + * Copyright 2015 Freescale Semiconductor, Inc. > + * Copyright 2018-2020 NXP > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mii.h> > +#include <linux/mdio.h> > +#include <linux/ethtool.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_net.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/timer.h> > +#include <linux/delay.h> > +#include <linux/workqueue.h> > +#include <linux/netdevice.h> > +#include <linux/list.h> > + > +#include "backplane.h" > +#include "link_training.h" > + > +/* KR timeouts in milliseconds */ > +#define KR_TIMEOUT_1 100 > +#define KR_TIMEOUT_2 1000 > +#define KR_DENY_RT_INTERVAL 3000 > +#define KR_LT_TIMEOUT 500 > + > +/* KR timings in interations */ > +#define KR_AN_WAIT_ITERATIONS 5 > +#define KR_TRAIN_STEP_ITERATIONS 2 > +#define CDR_LOCK_RETRY_COUNT 3 > + > +/* AN status register (Clause 45) (MMD 7): MDIO_STAT1 */ > +#define AN_LINK_UP_MASK 0x04 > + > +/* Logging buffer size */ > +#define LOG_BUFFER_SIZE 200 > + > +/* Backplane custom logging */ > +#define BPDEV_LOG(name) \ > + char log_buffer[LOG_BUFFER_SIZE]; \ > + va_list args; va_start(args, msg); \ > + vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \ > + if (!bpphy->attached_dev) \ > + dev_##name(&bpphy->mdio.dev, log_buffer); \ > + else \ > + dev_##name(&bpphy->mdio.dev, "%s: %s", \ > + netdev_name(bpphy->attached_dev), log_buffer); \ > + va_end(args) > + > +/* Backplane features */ > +__ETHTOOL_DECLARE_LINK_MODE_MASK(backplane_features) __ro_after_init; > +EXPORT_SYMBOL(backplane_features); > + > +const int backplane_common_features_array[] = { > + ETHTOOL_LINK_MODE_Backplane_BIT, > + ETHTOOL_LINK_MODE_Autoneg_BIT, > + ETHTOOL_LINK_MODE_MII_BIT, > +}; > + > +const int backplane_protocol_features_array[] = { > + ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, > + ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, > +}; > + > +/* map string key to pointer data */ > +struct spmap_node { > + struct list_head entry; > + const char *key; > + void *pdata; > +}; > + > +/* registered equalization algorithms info */ > +static LIST_HEAD(eqalg_list); > + > +/* lanes attached to an equalization algorithm */ > +static LIST_HEAD(lnalg_list); > + > +/* Backplane mutex between all KR PHY threads */ > +static struct mutex backplane_lock; > + > +static int get_backplane_speed(phy_interface_t bp_mode) > +{ > + switch (bp_mode) { > + case PHY_INTERFACE_MODE_10GKR: > + return SPEED_10000; > + case PHY_INTERFACE_MODE_40GKR4: > + return SPEED_40000; > + default: > + pr_err("%s: Unsupported backplane phy interface\n", > + BACKPLANE_DRIVER_NAME); > + return SPEED_UNKNOWN; > + } > + return SPEED_UNKNOWN; > +} > + > +static enum ethtool_link_mode_bit_indices > + get_backplane_supported_mode(phy_interface_t bp_mode) > +{ > + switch (bp_mode) { > + case PHY_INTERFACE_MODE_10GKR: > + return ETHTOOL_LINK_MODE_10000baseKR_Full_BIT; > + case PHY_INTERFACE_MODE_40GKR4: > + return ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT; > + default: > + pr_err("%s: Unsupported backplane phy interface\n", > + BACKPLANE_DRIVER_NAME); > + return ETHTOOL_LINK_MODE_Backplane_BIT; > + } > + return ETHTOOL_LINK_MODE_Backplane_BIT; > +} > + > +static int spmap_add(struct list_head *list, const char *key, void *pdata) > +{ > + struct spmap_node *node; > + > + /* create a new entry with desired key */ > + node = kzalloc(sizeof(*node), GFP_KERNEL); > + if (!node) > + return -ENOMEM; > + > + node->key = key; > + node->pdata = pdata; > + > + list_add(&node->entry, list); > + > + return 0; > +} > + > +static const struct equalization_algorithm *eq_find(const char *key) > +{ > + struct spmap_node *eqalg, *eqalg_tmp; > + > + if (!key) > + return NULL; > + > + /* search desired single key */ > + list_for_each_entry_safe(eqalg, eqalg_tmp, &eqalg_list, entry) { > + if (strcmp(eqalg->key, key) == 0) > + return (struct equalization_algorithm *)eqalg->pdata; > + } > + return NULL; > +} > + > +static void backplane_features_init(void) > +{ > + linkmode_set_bit_array(backplane_common_features_array, > + ARRAY_SIZE(backplane_common_features_array), > + backplane_features); > + > + linkmode_set_bit_array(backplane_protocol_features_array, > + ARRAY_SIZE(backplane_protocol_features_array), > + backplane_features); > +} > + > +static u32 le_ioread32(void __iomem *reg) > +{ > + return ioread32(reg); > +} > + > +static void le_iowrite32(u32 value, void __iomem *reg) > +{ > + iowrite32(value, reg); > +} > + > +static u32 be_ioread32(void __iomem *reg) > +{ > + return ioread32be(reg); > +} > + > +static void be_iowrite32(u32 value, void __iomem *reg) > +{ > + iowrite32be(value, reg); > +} > + > +static void training_status_init(struct training_status *trst) > +{ > + trst->done_training = false; > + trst->remote_tx_complete = false; > + trst->remote_tx_running = false; > + trst->sent_init = false; > + trst->lp_rx_ready = 0; > + trst->local_tx_running = false; > +} > + > +static void init_krln(struct kr_lane_info *krln, bool revert_default) > +{ > + if (revert_default) > + backplane_default_kr_lane(krln); > + > + training_status_init(&krln->trst); > + krln->state = DETECTING_LP; > + krln->an_acquired = false; > + > + krln->ld_update = 0; > + krln->prev_ld_update = 0; > + krln->ld_last_nonhold_update = 0; > + krln->lp_status = 0; > + krln->lp_last_change_status = 0; > + krln->last_lp_update_status[C_M1] = 0; > + krln->last_lp_update_status[C_Z0] = 0; > + krln->last_lp_update_status[C_P1] = 0; > + krln->ld_status = 0; > + krln->move_back_prev = false; > + krln->move_back_cnt = 0; > + krln->move_back_lp_status = 0; > + > + lt_init_ld(krln); > +} > + > +static void setup_supported_linkmode(struct phy_device *bpphy) > +{ > + struct backplane_phy_info *bp_phy = bpphy->priv; > + int i; > + > + /* Clear all supported backplane protocols features > + * and setup only the currently configured protocol > + */ > + for (i = 0; i < ARRAY_SIZE(backplane_protocol_features_array); i++) > + linkmode_clear_bit(backplane_protocol_features_array[i], > + bpphy->supported); > + > + linkmode_set_bit(get_backplane_supported_mode(bp_phy->bp_mode), > + bpphy->supported); > +} > + > +/* Read AN Link Status */ > +static int is_an_link_up(struct phy_device *bpphy) > +{ > + struct backplane_phy_info *bp_phy = bpphy->priv; > + int ret, val = 0; > + > + mutex_lock(&bp_phy->bpphy_lock); > + > + /* Read twice because Link_Status is LL (Latched Low) bit */ > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status); > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy->bp_dev.mdio.an_status); Why not just val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1); Or is your hardware not actually conformant to the standard? There has also been a lot of discussion of reading the status twice is correct or not. Don't you care the link has briefly gone down and up again? Andrew
On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote: > +static void setup_supported_linkmode(struct phy_device *bpphy) > +{ > + struct backplane_phy_info *bp_phy = bpphy->priv; I'm not sure it is a good idea to completely take over phydev->priv like this, in what is just helper code. What if the PHY driver needs memory of its own? There are a few examples of this already in other PHY drivers. Could a KR PHY contain a temperature sensor? Could it contain statistics counters which need accumulating? Andrew
On 3/26/2020 6:07 PM, Andrew Lunn wrote: >> +static u32 le_ioread32(void __iomem *reg) >> +{ >> + return ioread32(reg); >> +} >> + >> +static void le_iowrite32(u32 value, void __iomem *reg) >> +{ >> + iowrite32(value, reg); >> +} >> + >> +static u32 be_ioread32(void __iomem *reg) >> +{ >> + return ioread32be(reg); >> +} >> + >> +static void be_iowrite32(u32 value, void __iomem *reg) >> +{ >> + iowrite32be(value, reg); >> +} > > This is very surprising to me. I've not got my head around the > structure of this code yet, but i'm surprised to see memory mapped > access functions in generic code. This abstraction makes no sense whatsoever, you already have io{read,write}32{be,} to deal with the correct endian, and you can use the standard Device Tree properties 'big-endian', 'little-endian', 'native-endian' to decide which of those of to use. If you need to introduce a wrapper or indirect function calls to select the correct I/O accessor, that is fine of course.
On Fri, 2020-03-27 at 15:22 +0100, Andrew Lunn wrote: > > +/* Backplane custom logging */ > > +#define BPDEV_LOG(name) \ > > + char log_buffer[LOG_BUFFER_SIZE]; \ > > + va_list args; va_start(args, msg); \ > > + vsnprintf(log_buffer, LOG_BUFFER_SIZE - 1, msg, args); \ > > + if (!bpphy->attached_dev) \ > > + dev_##name(&bpphy->mdio.dev, log_buffer); \ > > + else \ > > + dev_##name(&bpphy->mdio.dev, "%s: %s", \ > > + netdev_name(bpphy->attached_dev), log_buffer); \ > > + va_end(args) This could also use %pV instead of an intermediate buffer. It's also bad form to use macros with required external variables.
> On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > > Add support for backplane kr phy connection types currently available > > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > > the cases for KR modes which are clause 45 compatible to correctly > > assign phy_interface and phylink#supported) > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > --- > > drivers/net/phy/phylink.c | 15 ++++++++++++--- > > include/linux/phy.h | 6 +++++- > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index fed0c59..db1bb87 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -4,6 +4,7 @@ > > * technologies such as SFP cages where the PHY is hot-pluggable. > > * > > * Copyright (C) 2015 Russell King > > + * Copyright 2020 NXP > > */ > > #include <linux/ethtool.h> > > #include <linux/export.h> > > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, struct > fwnode_handle *fwnode) > > break; > > > > case PHY_INTERFACE_MODE_USXGMII: > > - case PHY_INTERFACE_MODE_10GKR: > > We might have a backwards compatibility issue here. If i remember correctly, > there are some boards out in the wild using PHY_INTERFACE_MODE_10GKR not > PHY_INTERFACE_MODE_10GBASER. > > See e0f909bc3a242296da9ccff78277f26d4883a79d > > Russell, what do you say about this? > > Andrew Ethernet interface nomenclature is using the following terminology: e.g. 10GBase-KR: data rate (10G), modulation type (Base = Baseband), medium type (K = BacKplane), physical layer encoding scheme (R = scRambling/descRambling using 64b/66b encoding that allows for Ethernet framing at a rate of 10.3125 Gbit/s) So 10GBase-R name provide information only about the data rate and the encoding scheme without specifying the interface medium. 10GBase-R is a family of many different standards specified for several different physical medium for copper and optical fiber like for example: 10GBase-SR: Short range (over fiber) 10GBase-LR: Long reach (over fiber) 10GBase-LRM: Long reach multi-mode (over fiber) 10GBase-ER: Extended reach (over fiber) 10GBase-CR: 10G over copper 10GBase-KR: Backplane 10GBase-KR represents Ethernet operation over electrical backplanes on single lane and uses the same physical layer encoding as 10GBase-LR/ER/SR defined in IEEE802.3 Clause 49. So prior to my change, phy_interface_t provided both enumerators which is correct: PHY_INTERFACE_MODE_10GBASER PHY_INTERFACE_MODE_10GKR Perhaps PHY_INTERFACE_MODE_10GKR was not used before because Backplane support was not available and all 10GBase-R family of interfaces should be using PHY_INTERFACE_MODE_10GBASER. In case PHY_INTERFACE_MODE_10GKR was used before, this is probably incorrect and should be changed for those boards to use the correct phy connection type: PHY_INTERFACE_MODE_10GBASER. Moreover now I also introduced a new phy connection type to cover also: 40GBase-KR4 which is 40G Backplane over 4-lanes: PHY_INTERFACE_MODE_40GKR4 Prior to adding backplane support, 10GKR case was handled the same as 10GBASER in phylink_parse_mode. But now because we are adding support for backplane modes, all backplane phy connection types (like: 10GKR, 40GKR4) must be treated separately to correctly setup the supported phylink. Florin.
On Sun, Mar 29, 2020 at 08:22:10AM +0000, Florinel Iordache wrote: > > On Thu, Mar 26, 2020 at 03:51:16PM +0200, Florinel Iordache wrote: > > > Add support for backplane kr phy connection types currently available > > > (10gbase-kr, 40gbase-kr4) and the required phylink updates (cover all > > > the cases for KR modes which are clause 45 compatible to correctly > > > assign phy_interface and phylink#supported) > > > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > > --- > > > drivers/net/phy/phylink.c | 15 ++++++++++++--- > > > include/linux/phy.h | 6 +++++- > > > 2 files changed, 17 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > > index fed0c59..db1bb87 100644 > > > --- a/drivers/net/phy/phylink.c > > > +++ b/drivers/net/phy/phylink.c > > > @@ -4,6 +4,7 @@ > > > * technologies such as SFP cages where the PHY is hot-pluggable. > > > * > > > * Copyright (C) 2015 Russell King > > > + * Copyright 2020 NXP > > > */ > > > #include <linux/ethtool.h> > > > #include <linux/export.h> > > > @@ -303,7 +304,6 @@ static int phylink_parse_mode(struct phylink *pl, struct > > fwnode_handle *fwnode) > > > break; > > > > > > case PHY_INTERFACE_MODE_USXGMII: > > > - case PHY_INTERFACE_MODE_10GKR: > > > > We might have a backwards compatibility issue here. If i remember correctly, > > there are some boards out in the wild using PHY_INTERFACE_MODE_10GKR not > > PHY_INTERFACE_MODE_10GBASER. > > > > See e0f909bc3a242296da9ccff78277f26d4883a79d > > > > Russell, what do you say about this? > > > > Andrew > > Ethernet interface nomenclature is using the following terminology: > e.g. 10GBase-KR: data rate (10G), modulation type (Base = Baseband), > medium type (K = BacKplane), physical layer encoding scheme > (R = scRambling/descRambling using 64b/66b encoding that allows for > Ethernet framing at a rate of 10.3125 Gbit/s) > So 10GBase-R name provide information only about the data rate and > the encoding scheme without specifying the interface medium. > 10GBase-R is a family of many different standards specified for > several different physical medium for copper and optical fiber like > for example: > 10GBase-SR: Short range (over fiber) > 10GBase-LR: Long reach (over fiber) > 10GBase-LRM: Long reach multi-mode (over fiber) > 10GBase-ER: Extended reach (over fiber) > 10GBase-CR: 10G over copper > 10GBase-KR: Backplane > > 10GBase-KR represents Ethernet operation over electrical backplanes on > single lane and uses the same physical layer encoding as 10GBase-LR/ER/SR > defined in IEEE802.3 Clause 49. I'm not sure why NXP folk think that they have to keep explaining this to us. You do not. > So prior to my change, phy_interface_t provided both enumerators which is correct: > PHY_INTERFACE_MODE_10GBASER > PHY_INTERFACE_MODE_10GKR > Perhaps PHY_INTERFACE_MODE_10GKR was not used before because Backplane > support was not available and all 10GBase-R family of interfaces should > be using PHY_INTERFACE_MODE_10GBASER. What you are missing is that PHY_INTERFACE_MODE_10GKR was introduced first and used _incorrectly_. We are currently mid-transition to correct that mistake. While we are in transition, PHY_INTERFACE_MODE_10GKR can _not_ be used correctly, and nothing NXP or anyone else says will change that fact until the transition has been completed. In kernel land, we do not intentionally regress platforms, even if we have made a mistake.