lkml.org 
[lkml]   [2018]   [Oct]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-10-01 11:43    [W:5.332 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site