lkml.org 
[lkml]   [2020]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()
    Thanks Andrew and Jeremy for the detailed discussion!

    On Fri, May 08, 2020 at 08:13:01PM +0200, 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.
    >
    > 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.

    I don't think ACPI mandates that OS driver use exact string match and not parse
    the string.

    First of all, I would suggest that we use "compatible" property instead of _CID.
    Not sure of a reason why we cannot. This will simplify implementation of fwnode
    APIs.

    Already I've pointed out couple of ASL files in tianocore where they are already
    used.
    one eg:https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280

    Even if we use _CID, I'm not sure we are prohibited from extracting characters
    (phy_id) from it.
    If we decide to use _CID, then we need to define somewhere and standardize
    exactly how we are going to use it. I'm not sure where we can do this.

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

    Makes sense to me.

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

    Makes sense to me.

    \
     
     \ /
      Last update: 2020-05-11 07:54    [W:3.134 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site