Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Ard Biesheuvel]

On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote:
> Hi,
>
> This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> First three patches introduce fwnode helpers for obtaining PHY
> information from nodes and also MDIO fwnode API for registering
> the bus with its PHY/devices.
>
> Following patches update code of the mvmdio and mvpp2 drivers
> to support ACPI tables handling. The latter is done in 4 steps,
> as can be seen in the commits. For the details, please
> refer to the commit messages.
>
> Drivers operation was tested on top of the net-next branch
> with both DT and ACPI. Although for compatibility reasons
> with older platforms, the mvmdio driver keeps using
> of_ MDIO registering, new fwnode_ one proved to fully work
> with DT as well (tested on MacchiatoBin board).
>
> mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> on a public branch:
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> It was compiled together with the most recent Tianocore EDK2 revision.
> Please refer to the firmware build instruction on MacchiatoBin board:
> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>
> Above support configures 1G to use its PHY normally. 10G can work now
> only with the link interrupt mode. Somehow reading of the
> string property in fwnode_mdiobus_child_is_phy works only with
> DT and cannot cope with 10G PHY nodes as in:
> https://pastebin.com/3JnYpU0A
>
> Above root cause will be further checked. In the meantime I will
> appreciate any comments or remarks for the kernel patches.
>

Hi Marcin,

I have added linux-acpi and Graeme to cc. I think it makes sense to
discuss the way you describe the device topology before looking at the
patches in more detail.

In particular, I would like to request feedback on the use of
[redundant] 'reg' properties and the use of _DSD + compatible to
describe PHYs. Usually, we try to avoid this, given that it is
essentially a ACPI encapsulated DT dialect that is difficult to
support in drivers unless they are based on DT to begin with. Also,
non-standard _DSD properties require a vendor prefix, it is not
freeform.

For reference, the ACPI description is here (starting at line 175)
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl

-- 
Ard.

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

Hi Ard

2017-12-18 10:40 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote:
>> Hi,
>>
>> This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
>> First three patches introduce fwnode helpers for obtaining PHY
>> information from nodes and also MDIO fwnode API for registering
>> the bus with its PHY/devices.
>>
>> Following patches update code of the mvmdio and mvpp2 drivers
>> to support ACPI tables handling. The latter is done in 4 steps,
>> as can be seen in the commits. For the details, please
>> refer to the commit messages.
>>
>> Drivers operation was tested on top of the net-next branch
>> with both DT and ACPI. Although for compatibility reasons
>> with older platforms, the mvmdio driver keeps using
>> of_ MDIO registering, new fwnode_ one proved to fully work
>> with DT as well (tested on MacchiatoBin board).
>>
>> mvpp2/mvmdio driver can work with the ACPI representation, as exposed
>> on a public branch:
>> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
>> It was compiled together with the most recent Tianocore EDK2 revision.
>> Please refer to the firmware build instruction on MacchiatoBin board:
>> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>>
>> Above support configures 1G to use its PHY normally. 10G can work now
>> only with the link interrupt mode. Somehow reading of the
>> string property in fwnode_mdiobus_child_is_phy works only with
>> DT and cannot cope with 10G PHY nodes as in:
>> https://pastebin.com/3JnYpU0A
>>
>> Above root cause will be further checked. In the meantime I will
>> appreciate any comments or remarks for the kernel patches.
>>
>
> Hi Marcin,
>
> I have added linux-acpi and Graeme to cc. I think it makes sense to
> discuss the way you describe the device topology before looking at the
> patches in more detail.
>

Thanks. Tomasz Nowicki immediately pointed this to me off the lists.

> In particular, I would like to request feedback on the use of
> [redundant] 'reg' properties and the use of _DSD + compatible to
> describe PHYs. Usually, we try to avoid this, given that it is
> essentially a ACPI encapsulated DT dialect that is difficult to
> support in drivers unless they are based on DT to begin with. Also,
> non-standard _DSD properties require a vendor prefix, it is not
> freeform.
>

Already a lot of drivers use both DT + ACPI. Some have IMO very
fanciful bindings in both, mostly handled within the drivers with
custom functions. OF_ - only drivers can use of_mdio_ helper routines,
that assume a certain hierarchy:
MDIO device node with PHYs as children, which are referenced in the
ports node. I believe such approach could fit ACPI description too.
I'm aware however that my code is pretty much DT transposed into ACPI
environment and I'm of course open to discussion, how to do it in the
most proper way.

My main goal is to provide an fwnode-based glue code, that can be used
among the NIC/MDIO drivers  (+ phylink) without multiple ifs
differentiating between ACPI/OF. What I sent has single calls in
mvpp2/mvmdio with a common bottom layers, but I don't see a problem,
that, e.g. when iterating over PHY subnodes, in case of OF
'reg'/'compatible' are used, whereas with ACPI some specific _ADR/_CID
objects.

I'd appreaciate any feedback.

Best regards,
Marcin

> For reference, the ACPI description is here (starting at line 175)
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Graeme Gregory]

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

On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote:
> > Hi,
> >
> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> > First three patches introduce fwnode helpers for obtaining PHY
> > information from nodes and also MDIO fwnode API for registering
> > the bus with its PHY/devices.
> >
> > Following patches update code of the mvmdio and mvpp2 drivers
> > to support ACPI tables handling. The latter is done in 4 steps,
> > as can be seen in the commits. For the details, please
> > refer to the commit messages.
> >
> > Drivers operation was tested on top of the net-next branch
> > with both DT and ACPI. Although for compatibility reasons
> > with older platforms, the mvmdio driver keeps using
> > of_ MDIO registering, new fwnode_ one proved to fully work
> > with DT as well (tested on MacchiatoBin board).
> >
> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> > on a public branch:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> > It was compiled together with the most recent Tianocore EDK2 revision.
> > Please refer to the firmware build instruction on MacchiatoBin board:
> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
> >
> > Above support configures 1G to use its PHY normally. 10G can work now
> > only with the link interrupt mode. Somehow reading of the
> > string property in fwnode_mdiobus_child_is_phy works only with
> > DT and cannot cope with 10G PHY nodes as in:
> > https://pastebin.com/3JnYpU0A
> >
> > Above root cause will be further checked. In the meantime I will
> > appreciate any comments or remarks for the kernel patches.
> >
> 
> Hi Marcin,
> 
> I have added linux-acpi and Graeme to cc. I think it makes sense to
> discuss the way you describe the device topology before looking at the
> patches in more detail.
> 
> In particular, I would like to request feedback on the use of
> [redundant] 'reg' properties and the use of _DSD + compatible to
> describe PHYs. Usually, we try to avoid this, given that it is
> essentially a ACPI encapsulated DT dialect that is difficult to
> support in drivers unless they are based on DT to begin with. Also,
> non-standard _DSD properties require a vendor prefix, it is not
> freeform.
> 
> For reference, the ACPI description is here (starting at line 175)
> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
> 
So the representation of PHY's with _DSD was kind of formalised here

http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf

This is already in use in the kernel, and that DSDT seems to be along the same
lines. So seems ok in that manner.

The "reg", 0 property seems a little odd, it would probably make more
sense to check for the lack of ranges passed in in ACPI manner _CRS.

Graeme


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

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

Hi Graeme,

2018-01-03 12:00 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>:
> On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
>> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote:
>> > Hi,
>> >
>> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
>> > First three patches introduce fwnode helpers for obtaining PHY
>> > information from nodes and also MDIO fwnode API for registering
>> > the bus with its PHY/devices.
>> >
>> > Following patches update code of the mvmdio and mvpp2 drivers
>> > to support ACPI tables handling. The latter is done in 4 steps,
>> > as can be seen in the commits. For the details, please
>> > refer to the commit messages.
>> >
>> > Drivers operation was tested on top of the net-next branch
>> > with both DT and ACPI. Although for compatibility reasons
>> > with older platforms, the mvmdio driver keeps using
>> > of_ MDIO registering, new fwnode_ one proved to fully work
>> > with DT as well (tested on MacchiatoBin board).
>> >
>> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
>> > on a public branch:
>> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
>> > It was compiled together with the most recent Tianocore EDK2 revision.
>> > Please refer to the firmware build instruction on MacchiatoBin board:
>> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
>> >
>> > Above support configures 1G to use its PHY normally. 10G can work now
>> > only with the link interrupt mode. Somehow reading of the
>> > string property in fwnode_mdiobus_child_is_phy works only with
>> > DT and cannot cope with 10G PHY nodes as in:
>> > https://pastebin.com/3JnYpU0A
>> >
>> > Above root cause will be further checked. In the meantime I will
>> > appreciate any comments or remarks for the kernel patches.
>> >
>>
>> Hi Marcin,
>>
>> I have added linux-acpi and Graeme to cc. I think it makes sense to
>> discuss the way you describe the device topology before looking at the
>> patches in more detail.
>>
>> In particular, I would like to request feedback on the use of
>> [redundant] 'reg' properties and the use of _DSD + compatible to
>> describe PHYs. Usually, we try to avoid this, given that it is
>> essentially a ACPI encapsulated DT dialect that is difficult to
>> support in drivers unless they are based on DT to begin with. Also,
>> non-standard _DSD properties require a vendor prefix, it is not
>> freeform.
>>
>> For reference, the ACPI description is here (starting at line 175)
>> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
>>
> So the representation of PHY's with _DSD was kind of formalised here
>
> http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
>
> This is already in use in the kernel, and that DSDT seems to be along the same
> lines. So seems ok in that manner.
>
> The "reg", 0 property seems a little odd, it would probably make more
> sense to check for the lack of ranges passed in in ACPI manner _CRS.
>

I already agreed with 'reg' being awkward in the later emails.
Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?

Thanks,
Marcin

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> I already agreed with 'reg' being awkward in the later emails.
> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?

Also, how do you specify which MDIO bus the PHY is on. To fully
specify a PHY, you need both bits of information.

In DT, the phy-handle phandle can point to any PHY anywhere in the
system. This is particularly important when a Ethernet device has two
MDIO busses.

     Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

Hi Andrew,

2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>> I already agreed with 'reg' being awkward in the later emails.
>> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
>
> Also, how do you specify which MDIO bus the PHY is on. To fully
> specify a PHY, you need both bits of information.
>
> In DT, the phy-handle phandle can point to any PHY anywhere in the
> system. This is particularly important when a Ethernet device has two
> MDIO busses.
>

For now, my local MDIO bus description is pretty DT-like, i.e. master
bus with children PHYs:
        Device (MDIO)
        {
            Name (_HID, "MRVL0100")                             //
_HID: Hardware ID
            Name (_UID, 0x00)                                   //
_UID: Unique ID
            Name (_CRS, ResourceTemplate ()
            {
                Memory32Fixed (ReadWrite,
                    0xf212a200,                                 // Address Base
                    0x00000010,                                 //
Address Length
                    )
            })
            Device (GPHY)
            {
              Name (_ADR, 0x0)
            }
        }

        Device (XSMI)
        {
            Name (_HID, "MRVL0101")                             //
_HID: Hardware ID
            Name (_UID, 0x00)                                   //
_UID: Unique ID
            Name (_CRS, ResourceTemplate ()
            {
                Memory32Fixed (ReadWrite,
                    0xf212a600,                                 // Address Base
                    0x00000010,                                 //
Address Length
                    )
            })
            Device (PHY0)
            {
              Name (_ADR, 0x0)
              Name (_CID, "ethernet-phy-ieee802.3-c45")
            }
            Device (PHY8)
            {
              Name (_ADR, 0x8)
              Name (_CID, "ethernet-phy-ieee802.3-c45")
            }
        }

Which is referenced in the port's node:

Package () { "phy", Package (){\_SB.XSMI.PHY0}},

I'm studying an alternatives with graphs, as suggested by Tomasz
Nowicki, but to me above is pretty natural and not complicated.

Best regards,
Marcin

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote:
> Hi Andrew,
> 
> 2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> >> I already agreed with 'reg' being awkward in the later emails.
> >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> >
> > Also, how do you specify which MDIO bus the PHY is on. To fully
> > specify a PHY, you need both bits of information.
> >
> > In DT, the phy-handle phandle can point to any PHY anywhere in the
> > system. This is particularly important when a Ethernet device has two
> > MDIO busses.
> >
> 
> For now, my local MDIO bus description is pretty DT-like, i.e. master
> bus with children PHYs:
>         Device (MDIO)
>         {
>             Name (_HID, "MRVL0100")                             //
> _HID: Hardware ID
>             Name (_UID, 0x00)                                   //
> _UID: Unique ID
>             Name (_CRS, ResourceTemplate ()
>             {
>                 Memory32Fixed (ReadWrite,
>                     0xf212a200,                                 // Address Base
>                     0x00000010,                                 //
> Address Length
>                     )
>             })
>             Device (GPHY)
>             {
>               Name (_ADR, 0x0)
>             }
>         }
> 
>         Device (XSMI)
>         {
>             Name (_HID, "MRVL0101")                             //
> _HID: Hardware ID
>             Name (_UID, 0x00)                                   //
> _UID: Unique ID
>             Name (_CRS, ResourceTemplate ()
>             {
>                 Memory32Fixed (ReadWrite,
>                     0xf212a600,                                 // Address Base
>                     0x00000010,                                 //
> Address Length
>                     )
>             })
>             Device (PHY0)
>             {
>               Name (_ADR, 0x0)
>               Name (_CID, "ethernet-phy-ieee802.3-c45")
>             }
>             Device (PHY8)
>             {
>               Name (_ADR, 0x8)
>               Name (_CID, "ethernet-phy-ieee802.3-c45")
>             }
>         }
> 
> Which is referenced in the port's node:
> 
> Package () { "phy", Package (){\_SB.XSMI.PHY0}},

Hi Marcin

This reference looks good, giving both the bus and the PHY on the bus.

I assume you can use references like this within the Device (PHY8)
node? You need to be able to reference a GPIO used for resetting the
PHY. And you also need to reference a GPIO at the Device (MDIO) level
for resetting all the PHYs on the MDIO bus.

    Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

2018-01-03 14:33 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote:
>> Hi Andrew,
>>
>> 2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>> >> I already agreed with 'reg' being awkward in the later emails.
>> >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
>> >
>> > Also, how do you specify which MDIO bus the PHY is on. To fully
>> > specify a PHY, you need both bits of information.
>> >
>> > In DT, the phy-handle phandle can point to any PHY anywhere in the
>> > system. This is particularly important when a Ethernet device has two
>> > MDIO busses.
>> >
>>
>> For now, my local MDIO bus description is pretty DT-like, i.e. master
>> bus with children PHYs:
>>         Device (MDIO)
>>         {
>>             Name (_HID, "MRVL0100")                             //
>> _HID: Hardware ID
>>             Name (_UID, 0x00)                                   //
>> _UID: Unique ID
>>             Name (_CRS, ResourceTemplate ()
>>             {
>>                 Memory32Fixed (ReadWrite,
>>                     0xf212a200,                                 // Address Base
>>                     0x00000010,                                 //
>> Address Length
>>                     )
>>             })
>>             Device (GPHY)
>>             {
>>               Name (_ADR, 0x0)
>>             }
>>         }
>>
>>         Device (XSMI)
>>         {
>>             Name (_HID, "MRVL0101")                             //
>> _HID: Hardware ID
>>             Name (_UID, 0x00)                                   //
>> _UID: Unique ID
>>             Name (_CRS, ResourceTemplate ()
>>             {
>>                 Memory32Fixed (ReadWrite,
>>                     0xf212a600,                                 // Address Base
>>                     0x00000010,                                 //
>> Address Length
>>                     )
>>             })
>>             Device (PHY0)
>>             {
>>               Name (_ADR, 0x0)
>>               Name (_CID, "ethernet-phy-ieee802.3-c45")
>>             }
>>             Device (PHY8)
>>             {
>>               Name (_ADR, 0x8)
>>               Name (_CID, "ethernet-phy-ieee802.3-c45")
>>             }
>>         }
>>
>> Which is referenced in the port's node:
>>
>> Package () { "phy", Package (){\_SB.XSMI.PHY0}},
>
> Hi Marcin
>
> This reference looks good, giving both the bus and the PHY on the bus.
>
> I assume you can use references like this within the Device (PHY8)
> node?

Yes.

> You need to be able to reference a GPIO used for resetting the
> PHY. And you also need to reference a GPIO at the Device (MDIO) level
> for resetting all the PHYs on the MDIO bus.
>

Yes, for full support of PHYs the GPIO must be supported, as well as
the PHY's IRQs.

Best regards,
Marcin

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Graeme Gregory]

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

On Wed, Jan 03, 2018 at 12:12:06PM +0100, Marcin Wojtas wrote:
> Hi Graeme,
> 
> 2018-01-03 12:00 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>:
> > On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote:
> >> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote:
> >> > Hi,
> >> >
> >> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers.
> >> > First three patches introduce fwnode helpers for obtaining PHY
> >> > information from nodes and also MDIO fwnode API for registering
> >> > the bus with its PHY/devices.
> >> >
> >> > Following patches update code of the mvmdio and mvpp2 drivers
> >> > to support ACPI tables handling. The latter is done in 4 steps,
> >> > as can be seen in the commits. For the details, please
> >> > refer to the commit messages.
> >> >
> >> > Drivers operation was tested on top of the net-next branch
> >> > with both DT and ACPI. Although for compatibility reasons
> >> > with older platforms, the mvmdio driver keeps using
> >> > of_ MDIO registering, new fwnode_ one proved to fully work
> >> > with DT as well (tested on MacchiatoBin board).
> >> >
> >> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed
> >> > on a public branch:
> >> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip
> >> > It was compiled together with the most recent Tianocore EDK2 revision.
> >> > Please refer to the firmware build instruction on MacchiatoBin board:
> >> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II
> >> >
> >> > Above support configures 1G to use its PHY normally. 10G can work now
> >> > only with the link interrupt mode. Somehow reading of the
> >> > string property in fwnode_mdiobus_child_is_phy works only with
> >> > DT and cannot cope with 10G PHY nodes as in:
> >> > https://pastebin.com/3JnYpU0A
> >> >
> >> > Above root cause will be further checked. In the meantime I will
> >> > appreciate any comments or remarks for the kernel patches.
> >> >
> >>
> >> Hi Marcin,
> >>
> >> I have added linux-acpi and Graeme to cc. I think it makes sense to
> >> discuss the way you describe the device topology before looking at the
> >> patches in more detail.
> >>
> >> In particular, I would like to request feedback on the use of
> >> [redundant] 'reg' properties and the use of _DSD + compatible to
> >> describe PHYs. Usually, we try to avoid this, given that it is
> >> essentially a ACPI encapsulated DT dialect that is difficult to
> >> support in drivers unless they are based on DT to begin with. Also,
> >> non-standard _DSD properties require a vendor prefix, it is not
> >> freeform.
> >>
> >> For reference, the ACPI description is here (starting at line 175)
> >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
> >>
> > So the representation of PHY's with _DSD was kind of formalised here
> >
> > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
> >
> > This is already in use in the kernel, and that DSDT seems to be along the same
> > lines. So seems ok in that manner.
> >
> > The "reg", 0 property seems a little odd, it would probably make more
> > sense to check for the lack of ranges passed in in ACPI manner _CRS.
> >
> 
> I already agreed with 'reg' being awkward in the later emails.
> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> 
Ah it is an actual address, then yes _ADR is probably more appropriate.

Graeme


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

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> > I already agreed with 'reg' being awkward in the later emails.
> > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > 
> Ah it is an actual address, then yes _ADR is probably more appropriate.

Newbie ACPI question. What is the definition of an address?

In this cause, we are talking about an address of a device on an MDIO
bus. It takes a value between 0 and 31.

How are IC2 device addresses represented in ACPI? MDIO devices and I2C
devices are pretty similar. So it would make sense to use the same as
what I2C uses.

     Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Graeme Gregory]

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

On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > I already agreed with 'reg' being awkward in the later emails.
> > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > > 
> > Ah it is an actual address, then yes _ADR is probably more appropriate.
> 
> Newbie ACPI question. What is the definition of an address?
> 
> In this cause, we are talking about an address of a device on an MDIO
> bus. It takes a value between 0 and 31.
> 
> How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> devices are pretty similar. So it would make sense to use the same as
> what I2C uses.
> 
Too big (and has table) to sensibly quote, but defined in ACPI spec

6.1.1 _ADR (Address)

Ive never though been quite sure if that is just an example list of
address types or its supposed to be canonical (in which case some ECRs
are needed to the spec).

Thanks

Graeme


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

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote:
> On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > > I already agreed with 'reg' being awkward in the later emails.
> > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > > > 
> > > Ah it is an actual address, then yes _ADR is probably more appropriate.
> > 
> > Newbie ACPI question. What is the definition of an address?
> > 
> > In this cause, we are talking about an address of a device on an MDIO
> > bus. It takes a value between 0 and 31.
> > 
> > How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> > devices are pretty similar. So it would make sense to use the same as
> > what I2C uses.
> > 
> Too big (and has table) to sensibly quote, but defined in ACPI spec
> 
> 6.1.1 _ADR (Address)
> 
> Ive never though been quite sure if that is just an example list of
> address types or its supposed to be canonical (in which case some ECRs
> are needed to the spec).

Hi Graeme

I took a quick look at version 6.2, and noticed i2c devices use
_ADR(). So using it for MDIO seems O.K.

However, i2c, spi and uart devices all seem to be described using
GenericSerialBus. Maybe the correct way to describe MDIO devices is to
also use GenericSerialBus?

     Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Graeme Gregory]

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

On Mon, Jan 08, 2018 at 03:53:12PM +0100, Andrew Lunn wrote:
> On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote:
> > On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote:
> > > > > I already agreed with 'reg' being awkward in the later emails.
> > > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus?
> > > > > 
> > > > Ah it is an actual address, then yes _ADR is probably more appropriate.
> > > 
> > > Newbie ACPI question. What is the definition of an address?
> > > 
> > > In this cause, we are talking about an address of a device on an MDIO
> > > bus. It takes a value between 0 and 31.
> > > 
> > > How are IC2 device addresses represented in ACPI? MDIO devices and I2C
> > > devices are pretty similar. So it would make sense to use the same as
> > > what I2C uses.
> > > 
> > Too big (and has table) to sensibly quote, but defined in ACPI spec
> > 
> > 6.1.1 _ADR (Address)
> > 
> > Ive never though been quite sure if that is just an example list of
> > address types or its supposed to be canonical (in which case some ECRs
> > are needed to the spec).
> 
> Hi Graeme
> 
> I took a quick look at version 6.2, and noticed i2c devices use
> _ADR(). So using it for MDIO seems O.K.
> 
> However, i2c, spi and uart devices all seem to be described using
> GenericSerialBus. Maybe the correct way to describe MDIO devices is to
> also use GenericSerialBus?
>
I am not familiar with MDIO, but if its similar or a specific
implementation of a serial bus that does sound sane!

Graeme


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

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

w> I am not familiar with MDIO, but if its similar or a specific
> implementation of a serial bus that does sound sane!

It is a two wire serial bus. A good overview can be found here:

https://www.totalphase.com/support/articles/200349206-MDIO-Background

	Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

Hi Andrew,



2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> w> I am not familiar with MDIO, but if its similar or a specific
>> implementation of a serial bus that does sound sane!
>

Thanks for digging, I will check if and how we can use
GenericSerialBus with MDIO.

Best regards,
Marcin

> It is a two wire serial bus. A good overview can be found here:
>
> https://www.totalphase.com/support/articles/200349206-MDIO-Background
>

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Graeme Gregory]

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

On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> Hi Andrew,
> 
> 
> 
> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> > w> I am not familiar with MDIO, but if its similar or a specific
> >> implementation of a serial bus that does sound sane!
> >
> 
> Thanks for digging, I will check if and how we can use
> GenericSerialBus with MDIO.
> 
Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
across similar on other ARM boards.

Graeme


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

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>:
> On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
>> Hi Andrew,
>>
>>
>>
>> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>> > w> I am not familiar with MDIO, but if its similar or a specific
>> >> implementation of a serial bus that does sound sane!
>> >
>>
>> Thanks for digging, I will check if and how we can use
>> GenericSerialBus with MDIO.
>>
> Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> across similar on other ARM boards.
>

I'm looking forward to their feedback, however, what I've noticed,
each driver handles mdio/phys on its own, not using any generic
solution, which is what I need to actually avoid :)

Best regards,
Marcin

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote:
> 2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>:
> > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> >> Hi Andrew,
> >>
> >>
> >>
> >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> >> > w> I am not familiar with MDIO, but if its similar or a specific
> >> >> implementation of a serial bus that does sound sane!
> >> >
> >>
> >> Thanks for digging, I will check if and how we can use
> >> GenericSerialBus with MDIO.
> >>
> > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> > across similar on other ARM boards.
> >
> 
> I'm looking forward to their feedback, however, what I've noticed,
> each driver handles mdio/phys on its own, not using any generic
> solution, which is what I need to actually avoid :)

Agreed. Lets define it once for all drivers using phylib/phylink.

	Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Lorenzo Pieralisi]

[+cc Mika]

On Tue, Jan 09, 2018 at 02:00:12PM +0100, Andrew Lunn wrote:
> On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote:
> > 2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>:
> > > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote:
> > >> Hi Andrew,
> > >>
> > >>
> > >>
> > >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> > >> > w> I am not familiar with MDIO, but if its similar or a specific
> > >> >> implementation of a serial bus that does sound sane!
> > >> >
> > >>
> > >> Thanks for digging, I will check if and how we can use
> > >> GenericSerialBus with MDIO.
> > >>
> > > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come
> > > across similar on other ARM boards.
> > >
> > 
> > I'm looking forward to their feedback, however, what I've noticed,
> > each driver handles mdio/phys on its own, not using any generic
> > solution, which is what I need to actually avoid :)
> 
> Agreed. Lets define it once for all drivers using phylib/phylink.

To start with, I am not entirely familiar with MDIO, apologies in
advance. Building something on top of GenericSerialBus sounds correct
but if I am not mistaken you would need a new bus type in the ACPI
specs.

I CC'ed Mika since he is more familiar with handling these bits of ACPI
specs - I wonder whether this is a problem that cropped up on x86
systems too.

I do not think there is one and only answer but there must be a single
set of bindings and if the ACPI specs already cater for some of them
we have to reuse them.

Please take some time to ensure the solution you are pushing is widely
deployable.

Thanks,
Lorenzo

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> I CC'ed Mika since he is more familiar with handling these bits of ACPI
> specs - I wonder whether this is a problem that cropped up on x86
> systems too.

Hi Lorenzo

There is nothing about MDIO, PHYs, Ethernet switches, etc in version
6.2 of the spec. If x86 has this, it must be after 6.2 was released.
I would not be too surprised if x86 has none of this. If you look at
the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
are all PCI devices, and hide all this.

> I do not think there is one and only answer but there must be a single
> set of bindings and if the ACPI specs already cater for some of them
> we have to reuse them.

Agreed. Due diligence so far suggests there is nothing already
defined. But im a newbie to ACPI, so could be looking in the wrong
place. I really hope there is somebody like Rob Herring, the DT
maintainer, who keeps an eye on all ACPI talk and would tell us if we
are heading off in the wrong direction.

    Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Marcin Wojtas]

Hi Mika,

2018-01-18 14:00 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>> I CC'ed Mika since he is more familiar with handling these bits of ACPI
>> specs - I wonder whether this is a problem that cropped up on x86
>> systems too.
>
> Hi Lorenzo
>
> There is nothing about MDIO, PHYs, Ethernet switches, etc in version
> 6.2 of the spec. If x86 has this, it must be after 6.2 was released.
> I would not be too surprised if x86 has none of this. If you look at
> the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
> are all PCI devices, and hide all this.
>
>> I do not think there is one and only answer but there must be a single
>> set of bindings and if the ACPI specs already cater for some of them
>> we have to reuse them.
>
> Agreed. Due diligence so far suggests there is nothing already
> defined. But im a newbie to ACPI, so could be looking in the wrong
> place. I really hope there is somebody like Rob Herring, the DT
> maintainer, who keeps an eye on all ACPI talk and would tell us if we
> are heading off in the wrong direction.
>

My initial approach with MDIO bus with PHYs as child nodes was super
easy to describe and handle in Linux - please refer to:
- typical representation of mdio bus with the phys -
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8
- my patches in the initial series
However I guess it would be more proper to use the
GenericSerialBus-based description, as advised in ACPI Spec. The
question is, whether we should define new type of a bus or not
(MdioSerialBus, similar to e.g. I2cSerialBus).

Since I have a code that can be tested and easily modified to use
different ACPI approaches with real platform MDIO controller
(mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
some time to prepare a proof of concept based on GenericSerialBus.
Please expect some RFC patches hopefully right after the coming merge
window is closed.

Of course, if I come up on some ACPI - specific implementation
questions, I won't hesitate to ask in this thred. I will also
appreciate any hints. For now my two main concerns are:
- The PHY address on the mdio bus - should it be put into _CRS ->
GenericSerialBus() field or separate _ADR? I'd lean towards first
option.
- The PHY type - in Linux it's resolved basing on two generic
compatible strings
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8).
I'd put it as a sort of ID into GenericSerialBus(). If you agree - any
specific filed that you would try to use?

Do I understand correctly that the MDIO controller node should
comprise OperationRegion() definition of the GenericSerialBus?

I'm looking forward to your feedback.

Thanks,
Marcin

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> Since I have a code that can be tested and easily modified to use
> different ACPI approaches with real platform MDIO controller
> (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
> some time to prepare a proof of concept based on GenericSerialBus.
> Please expect some RFC patches hopefully right after the coming merge
> window is closed.

It would also be interesting to know how the standardisation process
works.

I'm sure these is a FAQs, so maybe somebody could point us towards it.

Do we need to submit a proposed extension to the ACPI standard at the
same time as the patch? Should we not accept the code into Linux until
the proposal has been accepted? Or can we accept the code
provisionally, with the understanding that if the ACPI committee
rejects the extension, or suggest alternations, we can take the code
out of Linux without having to worry about backwards compatibility?

Thanks
      Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Mika Westerberg]

On Fri, Jan 19, 2018 at 07:07:29PM +0100, Marcin Wojtas wrote:
> Hi Mika,

Hi,

> 2018-01-18 14:00 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> >> I CC'ed Mika since he is more familiar with handling these bits of ACPI
> >> specs - I wonder whether this is a problem that cropped up on x86
> >> systems too.
> >
> > Hi Lorenzo
> >
> > There is nothing about MDIO, PHYs, Ethernet switches, etc in version
> > 6.2 of the spec. If x86 has this, it must be after 6.2 was released.
> > I would not be too surprised if x86 has none of this. If you look at
> > the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They
> > are all PCI devices, and hide all this.
> >
> >> I do not think there is one and only answer but there must be a single
> >> set of bindings and if the ACPI specs already cater for some of them
> >> we have to reuse them.
> >
> > Agreed. Due diligence so far suggests there is nothing already
> > defined. But im a newbie to ACPI, so could be looking in the wrong
> > place. I really hope there is somebody like Rob Herring, the DT
> > maintainer, who keeps an eye on all ACPI talk and would tell us if we
> > are heading off in the wrong direction.
> >
> 
> My initial approach with MDIO bus with PHYs as child nodes was super
> easy to describe and handle in Linux - please refer to:
> - typical representation of mdio bus with the phys -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8
> - my patches in the initial series
> However I guess it would be more proper to use the
> GenericSerialBus-based description, as advised in ACPI Spec. The
> question is, whether we should define new type of a bus or not
> (MdioSerialBus, similar to e.g. I2cSerialBus).

I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
would be to follow what SDIO is doing, e.g have the PHY devices listed
below the MDIO controller and use _ADR to describe their "address" on
that bus. You can see how _ADR applies to SDIO bus from ACPI spec.

Of course ACPI spec should then be updated accordingly to describe what
_ADR means for devices on MDIO bus.

> Since I have a code that can be tested and easily modified to use
> different ACPI approaches with real platform MDIO controller
> (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find
> some time to prepare a proof of concept based on GenericSerialBus.
> Please expect some RFC patches hopefully right after the coming merge
> window is closed.
> 
> Of course, if I come up on some ACPI - specific implementation
> questions, I won't hesitate to ask in this thred. I will also
> appreciate any hints. For now my two main concerns are:
> - The PHY address on the mdio bus - should it be put into _CRS ->
> GenericSerialBus() field or separate _ADR? I'd lean towards first
> option.
> - The PHY type - in Linux it's resolved basing on two generic
> compatible strings
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8).
> I'd put it as a sort of ID into GenericSerialBus(). If you agree - any
> specific filed that you would try to use?

If you go with the SDIO way then each PHY is described as normal ACPI
device and you can use ACPI _HID/_CID to match the device to the
corresponding driver.

> Do I understand correctly that the MDIO controller node should
> comprise OperationRegion() definition of the GenericSerialBus?

I don't think OpRegions are useful in this case because they are mainly
used to allow BIOS AML code to access the hardware through OS driver.

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
> would be to follow what SDIO is doing, e.g have the PHY devices listed
> below the MDIO controller and use _ADR to describe their "address" on
> that bus. You can see how _ADR applies to SDIO bus from ACPI spec.

Hi Mika

SDIO is not a serial bus, well it can be in its simplest form, but
high speed implementations have 4 data lines. So i can understand them
not using GenericSerialBus.

MDIO is a serial bus, very similar to SPI, I2C, and UART.

> If you go with the SDIO way then each PHY is described as normal ACPI
> device and you can use ACPI _HID/_CID to match the device to the
> corresponding driver.

Just some background here. If you have a plain PHY as a device on an
MDIO bus, you don't need to match it to a driver within ACPI.
Registers 2 and 3 contain a vendor and product ID. That is what it
used to match the device to the driver.

What you might need to know is the protocol to talk on the bus. Most
devices use clause 22 protocol. A few devices are clause 45. 22 is the
default in Linux, and you need to indicate if 45 should be used. You
can also indicate 22.

It gets more complex when the device on the bus is not a PHY. It is a
generic bus, you can connect anything to it. Ethernet switches can be
on the bus. They generally cannot be identified using registers 2 and
3. So you do need to match the device to the driver. Most do have ID
registers, so the driver can work out what specific device is on the
bus. However, Marvell moved the ID registers on there newer generation
of devices, so we need to give the driver a hint where to look. So in
device tree, we have two different compatible string.

Broadcom really do use it as a generic bus. They have their USB PHYs
and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to
match the device to the driver, as normal.

We need to ensure what we define for ACPI has the same level of
flexibility.

	Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Mika Westerberg]

On Sun, Jan 21, 2018 at 02:08:40AM +0100, Andrew Lunn wrote:
> > I'm not familiar with MDIO bus but an alternative to GeneriSerialBus
> > would be to follow what SDIO is doing, e.g have the PHY devices listed
> > below the MDIO controller and use _ADR to describe their "address" on
> > that bus. You can see how _ADR applies to SDIO bus from ACPI spec.
> 
> Hi Mika
> 
> SDIO is not a serial bus, well it can be in its simplest form, but
> high speed implementations have 4 data lines. So i can understand them
> not using GenericSerialBus.

Well, I think SDIO is more of a serial bus pretty much the same way than
SPI but it can support more data lines as needed (in the same way than
SPI). But I'm not an expert in SDIO.

However, that's not the point here :-) SATA (which is definitely a serial
bus) uses the same mechanism and not GenericSerialBus.

> MDIO is a serial bus, very similar to SPI, I2C, and UART.
> 
> > If you go with the SDIO way then each PHY is described as normal ACPI
> > device and you can use ACPI _HID/_CID to match the device to the
> > corresponding driver.
> 
> Just some background here. If you have a plain PHY as a device on an
> MDIO bus, you don't need to match it to a driver within ACPI.
> Registers 2 and 3 contain a vendor and product ID. That is what it
> used to match the device to the driver.
> 
> What you might need to know is the protocol to talk on the bus. Most
> devices use clause 22 protocol. A few devices are clause 45. 22 is the
> default in Linux, and you need to indicate if 45 should be used. You
> can also indicate 22.
> 
> It gets more complex when the device on the bus is not a PHY. It is a
> generic bus, you can connect anything to it. Ethernet switches can be
> on the bus. They generally cannot be identified using registers 2 and
> 3. So you do need to match the device to the driver. Most do have ID
> registers, so the driver can work out what specific device is on the
> bus. However, Marvell moved the ID registers on there newer generation
> of devices, so we need to give the driver a hint where to look. So in
> device tree, we have two different compatible string.
> 
> Broadcom really do use it as a generic bus. They have their USB PHYs
> and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to
> match the device to the driver, as normal.

OK, thanks for the explanation.

> We need to ensure what we define for ACPI has the same level of
> flexibility.

Right. So if you need to have some additional "parameters" with the
connection, then I suppose you may want to go with the GenericSerialBus
route. However, looking at the sample device tree description:

        davinci_mdio: ethernet@5c030000 {
                compatible = "ti,davinci_mdio";
                reg = <0x5c030000 0x1000>;
                #address-cells = <1>;
                #size-cells = <0>;

                reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
                reset-delay-us = <2>;

                ethphy0: ethernet-phy@1 {
                        reg = <1>;
                };

                ethphy1: ethernet-phy@3 {
                        reg = <3>;
                };
        };

would pretty much translate directly to this in ACPI if you don't need
any additional attributes:

	Device (ETH0) {
		Name (_ADR, /* PCI address of the NIC */)

		Device (PHY0) {
			Name (_ADR, 1)
			...
		} 

		Device (PHY1) {
			Name (_ADR, 3)
			...
		} 
	}

which looks pretty simple to me. You can also use _DSM and _DSD here to
pass information (like the protocol number) for the PHY devices to Linux.

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> Right. So if you need to have some additional "parameters" with the
> connection, then I suppose you may want to go with the GenericSerialBus
> route. However, looking at the sample device tree description:
> 
>         davinci_mdio: ethernet@5c030000 {
>                 compatible = "ti,davinci_mdio";
>                 reg = <0x5c030000 0x1000>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>                 reset-delay-us = <2>;
> 
>                 ethphy0: ethernet-phy@1 {
>                         reg = <1>;
>                 };
> 
>                 ethphy1: ethernet-phy@3 {
>                         reg = <3>;
>                 };
>         };
> 
> would pretty much translate directly to this in ACPI if you don't need
> any additional attributes:
> 
> 	Device (ETH0) {
> 		Name (_ADR, /* PCI address of the NIC */)
> 
> 		Device (PHY0) {
> 			Name (_ADR, 1)
> 			...
> 		} 
> 
> 		Device (PHY1) {
> 			Name (_ADR, 3)
> 			...
> 		} 
> 	}
> 
> which looks pretty simple to me. You can also use _DSM and _DSD here to
> pass information (like the protocol number) for the PHY devices to Linux.

I'm not particularly worried about that simple case. Other than, i
don't want people to think that is all that is required. 

For a more full example, take a look at vf610-zii-dev-rev-b.dts. The
Freescale FEC Ethernet controller provides the base MDIO device,
mdio1. On top of this is an MDIO mux, using a few GPIO lines to
enable/disable 3 child MDIO busses. Each of these busses has an
Ethernet Switch. The Ethernet switch exports up to two MDIO busses,
and on these busses are Ethernet PHYs which are embedded inside the
switch. The Ethernet switches are also interrupt controllers, with the
PHYs having interrupt properties which point back to the interrupt
controller in the switch.

So i'm interested in an ACPI proposal which supports this board.

	   Andrew

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Ard Biesheuvel]

On 21 January 2018 at 16:13, Andrew Lunn <andrew@lunn.ch> wrote:
>> Right. So if you need to have some additional "parameters" with the
>> connection, then I suppose you may want to go with the GenericSerialBus
>> route. However, looking at the sample device tree description:
>>
>>         davinci_mdio: ethernet@5c030000 {
>>                 compatible = "ti,davinci_mdio";
>>                 reg = <0x5c030000 0x1000>;
>>                 #address-cells = <1>;
>>                 #size-cells = <0>;
>>
>>                 reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>                 reset-delay-us = <2>;
>>
>>                 ethphy0: ethernet-phy@1 {
>>                         reg = <1>;
>>                 };
>>
>>                 ethphy1: ethernet-phy@3 {
>>                         reg = <3>;
>>                 };
>>         };
>>
>> would pretty much translate directly to this in ACPI if you don't need
>> any additional attributes:
>>
>>       Device (ETH0) {
>>               Name (_ADR, /* PCI address of the NIC */)
>>
>>               Device (PHY0) {
>>                       Name (_ADR, 1)
>>                       ...
>>               }
>>
>>               Device (PHY1) {
>>                       Name (_ADR, 3)
>>                       ...
>>               }
>>       }
>>
>> which looks pretty simple to me. You can also use _DSM and _DSD here to
>> pass information (like the protocol number) for the PHY devices to Linux.
>
> I'm not particularly worried about that simple case. Other than, i
> don't want people to think that is all that is required.
>
> For a more full example, take a look at vf610-zii-dev-rev-b.dts. The
> Freescale FEC Ethernet controller provides the base MDIO device,
> mdio1. On top of this is an MDIO mux, using a few GPIO lines to
> enable/disable 3 child MDIO busses. Each of these busses has an
> Ethernet Switch. The Ethernet switch exports up to two MDIO busses,
> and on these busses are Ethernet PHYs which are embedded inside the
> switch. The Ethernet switches are also interrupt controllers, with the
> PHYs having interrupt properties which point back to the interrupt
> controller in the switch.
>
> So i'm interested in an ACPI proposal which supports this board.
>

However interesting as an example, I'm not convinced this is what we
should aim for.

ACPI is not a replacement for DT, and it is unlikely that people would
be interested in running ACPI-only distros such as RHEL on their
Ethernet switch. DT is excellent at describing this, and there is no
need to replace it.

ACPI is about firmware abstractions: you don't need to describe every
stacked interrupt controller in minute detail to the OS if the
firmware configures it sufficiently. That way, the OS does not need to
know all these details, and vendors can update their hardware without
having to update the software as well. (Or that is the idea at least,
how that works out in practice on arm64 systems remains to be seen)

Taking the Marvell 8040 as an example: the ACPI description does not
expose the ICU interrupt controllers to the OS, but the firmware
configures them and describes their configured state as ordinary GIC
interrupts.

Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI
forum, and only members (who are all under a contract regarding
reasonable and non-discriminatory (RAND) licensing terms to IP they
own that is covered by the spec) can contribute. Individuals can
become members for free AFAIR, but that doesn't mean individuals are
typically interested in getting involved in a process that is rather
tedious and bureaucratic.

So my suggestion would be to find out to what extent the latest
version of the spec can describe non-trivial MDIO topologies, and
hopefully that includes the mvpp2 on the Marvell 8040.

Re: [net-next: PATCH 0/8] Armada 7k/8k PP2 ACPI support

[Andrew Lunn]

> However interesting as an example, I'm not convinced this is what we
> should aim for.
> 
> ACPI is not a replacement for DT, and it is unlikely that people would
> be interested in running ACPI-only distros such as RHEL on their
> Ethernet switch. DT is excellent at describing this, and there is no
> need to replace it.

I however do know of somebody who is building an industrial wireless
access point, with a Marvell Ethernet switch. They have chosen to use
an Intel CPU, and want to run CentOS on it, because that is what they
know.

So they will be using ACPI whatever, for the basic board support. They
then have a choice of either ACPI for the switch, or having device
tree as well as ACPI for the switch. And it will be interesting to see
how that works. Can DT reference GPIOs and I2C busses in ACPI?

> Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI
> forum, and only members (who are all under a contract regarding
> reasonable and non-discriminatory (RAND) licensing terms to IP they
> own that is covered by the spec) can contribute. Individuals can
> become members for free AFAIR, but that doesn't mean individuals are
> typically interested in getting involved in a process that is rather
> tedious and bureaucratic.

And this is a bit what i'm worried about. How you represent MDIO
busses, PHYs, Ethernet switches is implemented once in the core Linux
code. So i would be interested in knowing what happens if the Linux
community defines and implements something, boards start using it, but
a year later the tedious and bureaucratic process rejects it,
re-writes it, in an incompatible way.

	     Andrew