[edk2-devel] [edk2-platforms][PATCH 1/1] Platform/RPi4: Add ACPI entry for Genet network interface

[Ard Biesheuvel]

On Sat, 8 Feb 2020 at 15:19, Pete Batard <pete at akeo.ie> wrote:
>
> On 2020.02.08 08:52, Ard Biesheuvel wrote:
> > On Mon, 3 Feb 2020 at 13:40, Pete Batard <pete at akeo.ie> wrote:
> >>
> >> The Raspberry Pi 4 platforms uses a Broadcom Genet network interface, for
> >> which we need ACPI entries in order to make it usable under Linux.
> >>
> >> This patch adds these entries, including a max-dma-burst-size DSD attribute
> >> aimed at simplifying support for Genet on distros that use older kernels,
> >> such as Debian.
> >>
> >> Note that we ran these settings through someone working for Broadcom, who
> >> okayed the proposed values including ownership of max-dma-burst-size (which
> >> we expect to also require for Device Tree usage on older kernels, hence the
> >> requirement for a designated owner).
> >>
> >> Signed-off-by: Pete Batard <pete at akeo.ie>
> >> ---
> >>   Platform/RaspberryPi/RPi4/AcpiTables/Dsdt.asl | 29 ++++++++++++++++++++
> >>   1 file changed, 29 insertions(+)
> >>
> >> diff --git a/Platform/RaspberryPi/RPi4/AcpiTables/Dsdt.asl b/Platform/RaspberryPi/RPi4/AcpiTables/Dsdt.asl
> >> index b2f1d3439211..12c3967fa9e1 100644
> >> --- a/Platform/RaspberryPi/RPi4/AcpiTables/Dsdt.asl
> >> +++ b/Platform/RaspberryPi/RPi4/AcpiTables/Dsdt.asl
> >> @@ -267,6 +267,35 @@ DefinitionBlock ("Dsdt.aml", "DSDT", 5, "MSFT", "EDK2", 2)
> >>         }
> >>       }
> >>
> >> +    Device (ETH0)
> >> +    {
> >> +      Name (_HID, "BCM6E4E")
> >> +      Name (_CID, "BCM6E4E")
> >
> > What is the point of having identical _HID and _CID? Is that a Windows thing?
> >
> >> +      Name (_UID, 0x0)
> >> +      Name (_CCA, 0x0)
> >> +      Method (_STA)
> >> +      {
> >> +        Return (0xf)
> >> +      }
> >> +      Method (_CRS, 0x0, Serialized)
> >> +      {
> >> +        Name (RBUF, ResourceTemplate ()
> >> +        {
> >> +          Memory32Fixed (ReadWrite, 0xfd580000, 0x10000, )
> >
> > We could grab that base address from a PCD as well, no?
>
> Well, we don't seem to be using PCDs in any of the .asl's we have (we
> only do so on .aslc) and, also, that couldn't really be done without the
> previous series having been integrated.
>
> I don't mind going for a PCD here, but do you really want to do that
> just for ETH0? It'll just look weird because it's going to be the only
> .asl address that we'll pick from a PCD.
>
> If we're going to move to using PCDs in .asl's, I'd rather do that
> across the board and craft a patch that addresses all, including
> Uart.asl and so on... Else, it's a bit inconsistent as far as I'm concerned.
>

That's a fair point. Let's leave it for now.

> >> +          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBD }
> >> +          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBE }
> >
> > Why not
> >
> > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 0xBD, 0xBE }
>
> Likewise: Consistency (be it ill placed or not).
>
> We've simply followed what GPIO had here.
>
> Again, I don't mind making the .asl cleaner, but if we're going to do
> so, I'd rather not be piecemeal about it, and do that in a separate
> patch that harmonises the whole thing.
>
> If you're okay with that approach, and not require a v2 for this, I can
> send a separate patch that cleans up the .asl's and uses PCDs early next
> week.
>

I'll just merge the patch as-is. These are mostly nitpicks anyway, and
the current version has been tested and is know to work (right?) so no
point in doing a  respin.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54099): https://edk2.groups.io/g/devel/message/54099
Mute This Topic: https://groups.io/mt/70946255/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-