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
>
--
Florian
