Re: [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration

[Quentin Schulz <quentin.schulz@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]