Date:   Fri, 8 May 2020 14:18:50 -0500
From:   Jeremy Linton <jeremy.linton@....com>
To:     Andrew Lunn <andrew@...n.ch>,
        Calvin Johnson <calvin.johnson@....nxp.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        linux.cj@...il.com, Florian Fainelli <f.fainelli@...il.com>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Florin Laurentiu Chiculita <florinlaurentiu.chiculita@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Varun Sethi <V.Sethi@....com>,
        "Rajesh V . Bikkina" <rajesh.bikkina@....com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Diana Madalina Craciun <diana.craciun@....com>,
        netdev <netdev@...r.kernel.org>, Marcin Wojtas <mw@...ihalf.com>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        Makarand Pawagi <makarand.pawagi@....com>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Pankaj Bansal <pankaj.bansal@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

Hi,

On 5/8/20 1:13 PM, Andrew Lunn wrote:
>>> It does have a numeric version defined for EISA types. OTOH I suspect that
>>> your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
>>> be ideal to parse it. Instead the normal ACPI model of exactly matching the
>>> complete string in the phy driver might be more appropriate.
>>
>> IMO, it should be fine to parse the string to extract the phy_id. Is there any
>> reason why we cannot do this?
> 
> Some background here, about what the PHY core does.
> 
> PHYs have two ID registers. This contains vendor, device, and often
> revision of the PHY. Only the vendor part is standardised, vendors can
> decide how to use the device part, but it is common for the lowest
> nibble to be revision. The core will read these ID registers, and then
> go through all the PHY drivers registered and ask them if they support
> this ID. The drivers provide a table of IDs and masks. The mask is
> applied, and then if the ID matches, the driver is used. The mask
> allows the revision to be ignored, etc.
> 
> There is a very small number of devices where the vendor messed up,
> and did not put valid contents in the ID registers. In such cases, we
> can read the IDs from device tree. These are then used in exactly the
> same way as if they were read from the device.
>

Is that the case here?

Also, how much of this was caused by uboot being deficient, and failing 
to do vendor specific setup? AKA like what has been happening with the 
mac addresses, where it turns out the difference between a chip being 
used on x86 vs arm has frequently been that no one bothered to port all 
the option rom functionality to uboot (and in some cases edk2).

> If you want the ACPI model to be used, an exact match on the string,
> you are going to have to modify the core and the drivers. They
> currently don't have any string, and have no idea about different
> revisions which are out in the wild.

Right, not pretty. But _DSD should never be used to provide functionally 
provided elsewhere in the spec, and with ACPI the attempt is to make the 
firmware less linux focused, and more generic.

> 
>>> Similarly to how I suspect the next patch's use of "compatible" isn't ideal
>>> either, because whether a device is c45 or not, should tend to be fixed to a
>>> particular vendor/device implementation and not a firmware provided
>>> property.
> 
> Not exactly true. It is the combination of can the bus master do C45
> and can the device do C45. Unfortunately, we have no knowledge of the
> bus masters capabilities, if it can do C45. And many MDIO drivers will
> do a C22 transaction when asked to perform a C45 transaction. All new
> submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
> C45 is not supported. But we cannot rely on that. Too much history >
>>
>> I tend to agree with you on this. Even for DT, ideal case, IMO should be:
>>
>> 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
>> registers 2 and 3
>> 2) if not found scan for c45 devices <= looks like this is missing in Linux
>> 3) look for phy_id from compatible string.
> 
> It is somewhat more complex, in that there are a small number of
> devices which will respond to both C22 and C45. Generally, you want to
> use C45 if supported. So you would want to do the C45 scan first. But
> then the earlier problem comes to play, you have no idea if the bus
> master actually correctly supports C45.

But this shouldn't this be implied by the mdio vendor/model? AKA, you 
wouldn't have a part with a given _HID() where the capabilities would 
change.You would only have a situation where the phy's capabilities 
change, but they must in the end support whatever is provided by the master.

> 
> Given the issues, we assume all bus masters and PHY devices are C22
> unless DT says the bus master and PHY combination is compatible with
> C45.

How much of this can be simplified for ACPI buy ignoring the legacy and 
putting some guides around the ACPI/platform requirements?


Thanks,

Powered by blists - more mailing lists