Message-ID: <CAPv3WKeSoErRWZYV3X6+kDd9VZAqQmjA7KvC14_Zs8RbbLXjDg@mail.gmail.com>
Date:   Tue, 21 Jun 2022 01:25:42 +0200
From:   Marcin Wojtas <mw@...ihalf.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Len Brown <lenb@...nel.org>, vivien.didelot@...il.com,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Grzegorz Bernacki <gjb@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>,
        Tomasz Nowicki <tn@...ihalf.com>,
        Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@....com>,
        upstream@...ihalf.com
Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

pon., 20 cze 2022 o 21:47 Andrew Lunn <andrew@...n.ch> napisał(a):
>
> > +In DSDT/SSDT the scope of switch device is extended by the front-panel
> > +and one or more so called 'CPU' switch ports. Additionally
> > +subsequent MDIO busses with attached PHYs can be described.
>
> Humm, dsa.yaml says nothing about MDIO busses with attached PHYs.
> That is up to each individual DSA drivers binding.
>
> Please spilt this into a generic DSA binding, similar to dsa.yaml, and
> a Marvell specific binding, similar to marvell.txt. It might be you
> also need a generic MDIO binding, since the marvell device is just an
> MDIO device, and inherits some of its properties from MDIO.
>
> > +
> > +This document presents the switch description with the required subnodes
> > +and _DSD properties.
> > +
> > +These properties are defined in accordance with the "Device
> > +Properties UUID For _DSD" [dsd-guide] document and the
> > +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device
> > +Data Descriptors containing them.
> > +
> > +Switch device
> > +=============
> > +
> > +The switch device is represented as a child node of the MDIO bus.
> > +It must comprise the _HID (and optionally _CID) field, so to allow matching
> > +with appropriate driver via ACPI ID. The other obligatory field is
> > +_ADR with the device address on the MDIO bus [adr]. Below example
> > +shows 'SWI0' switch device at address 0x4 on the 'SMI0' bus.
> >
> > +.. code-block:: none
> > +
> > +    Scope (\_SB.SMI0)
> > +    {
> > +        Name (_HID, "MRVL0100")
> > +        Name (_UID, 0x00)
> > +        Method (_STA)
> > +        {
> > +            Return (0xF)
> > +        }
> > +        Name (_CRS, ResourceTemplate ()
> > +        {
> > +            Memory32Fixed (ReadWrite,
> > +                0xf212a200,
> > +                0x00000010,
>
> What do these magic numbers mean?
>
> > +                )
> > +        })
> > +        Device (SWI0)
> > +        {
> > +            Name (_HID, "MRVL0120")
> > +            Name (_UID, 0x00)
> > +            Name (_ADR, 0x4)
> > +            <...>
> > +        }
>
> I guess it is not normal for ACPI, but could you add some comments
> which explain this. In DT we have
>
>     properties:
>       reg:
>         minimum: 0
>         maximum: 31
>         description:
>           The ID number for the device.
>
> which i guess what this _ADR property is, but it would be nice if it
> actually described what it is supposed to mean. You have a lot of
> undocumented properties here.
>
>
> > +label
> > +-----
> > +A property with a string value describing port's name in the OS. In case the
> > +port is connected to the MAC ('CPU' port), its value should be set to "cpu".
>
> Each port is a MAC, so "is connected to the MAC" is a bit
> meaningless. "CPU Port" is well defined in DSA, and is a DSA concept,
> not a DT concept, so you might as well just use it here.
>
> > +
> > +phy-handle
> > +----------
> > +For each MAC node, a device property "phy-handle" is used to reference
> > +the PHY that is registered on an MDIO bus. This is mandatory for
> > +network interfaces that have PHYs connected to MAC via MDIO bus.
>
> It is not mandatory. The DSA core will assume that port 0 has a PHY
> using address 0, port 1 has a PHY using address 1, etc. You only need
> a phy-handle when this assumption does not work.
>
> > +See [phy] for more details.
> > +
> > +ethernet
> > +--------
> > +A property valid for the so called 'CPU' port and should comprise a reference
> > +to the MAC object declared in the DSDT/SSDT.
>
> Is "MAC" an ACPI term? Because this does not seem very descriptive to
> me. DT says:
>
>       Should be a phandle to a valid Ethernet device node.  This host
>       device is what the switch port is connected to
>
> > +
> > +fixed-link
> > +----------
> > +The 'fixed-link' is described by a data-only subnode of the
> > +port, which is linked in the _DSD package via
> > +hierarchical data extension (UUID dbb8e3e6-5886-4ba6-8795-1319f52a966b
> > +in accordance with [dsd-guide] "_DSD Implementation Guide" document).
> > +The subnode should comprise a required property ("speed") and
> > +possibly the optional ones - complete list of parameters and
> > +their values are specified in [ethernet-controller].
>
> You appear to be cut/pasting
> Documentation/firmware-guide/acpi/dsd/phy.txt. Please just reference
> it.
>
> > +Below example comprises MDIO bus ('SMI0') with a PHY at address 0x0 ('PHY0')
> > +and a switch ('SWI0') at 0x4. The so called 'CPU' port ('PRT5') is connected to
> > +the SoC's MAC (\_SB.PP20.ETH2). 'PRT2' port is configured as 1G fixed-link.
>
> This is ACPI, so it is less likely to be a SoC. The hosts CPU port
> could well be an external PCIe device for example. Yes, there are AMD
> devices with built in MACs, but in the ACPI world, they don't happen
> so often.
>
> I assume you have 3 different 'compatible' strings for the nv88e6xxx
> driver? You should document them somewhere and say how they map to
> different marvell switches,
>

Thank you for the remarks, I'll address all after the consensus about
the binding is established.

Best regards,
Marcin

Powered by blists - more mailing lists