Message ID | cover.ff40d591b548a6da31716e6e600f11a303e0e643.1536912834.git-series.quentin.schulz@bootlin.com |
---|---|
Headers | show |
Series | mscc: ocelot: add support for SerDes muxing configuration | expand |
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>
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>
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>
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>
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>
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?
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); >
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
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
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 >
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