Additional configuration options on Armada/Cn913x

Additional configuration options on Armada/Cn913x

[Marcin Wojtas]

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

Hi,

I'd like to ask for comments before I develop the actual code - currently
we have 2 workarounds done specifically for Linux:
a. ECAM shift in PCIE
b. SPCR address space definition

Both above are not needed e.g. in FreeBSD and I was requested to add their
optional disabling. The idea is to add dedicated variables that would
optionally allow to disable the quirks, accessible via BootManager.
Questions:

- Would above be acceptable or is there a better way to handle such cases?
- In case it's fine, is there a dedicated place in the BootManager menu to
add custom switches?
- Are you aware of good examples for adding custom options?

Best regards,
Marcin

[-- Attachment #2: Type: text/html, Size: 841 bytes --]

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Ard Biesheuvel]

On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> Hi,
> 
> I'd like to ask for comments before I develop the actual code - 
> currently we have 2 workarounds done specifically for Linux:
> a. ECAM shift in PCIE
> b. SPCR address space definition

What does this mean?

> 
> Both above are not needed e.g. in FreeBSD and I was requested to add 
> their optional disabling.

Disabling ECAM shift is just a matter of exposing the iATU controls to 
the OS, right? Why do you need to disable it?

> The idea is to add dedicated variables that 
> would optionally allow to disable the quirks, accessible via 
> BootManager. Questions:
> 
> - Would above be acceptable or is there a better way to handle such cases?
> - In case it's fine, is there a dedicated place in the BootManager menu 
> to add custom switches?

Typically in a 'platform' submenu under the Device Manager

> - Are you aware of good examples for adding custom options?
> 

You could look at SynQuacer or Raspberry Pi for inspiration on how to 
create HII pages in the UiApp menu system.

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[greg]

June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:

> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> 
>> Hi,
>> I'd like to ask for comments before I develop the actual code - > currently we have 2 workarounds
>> done specifically for Linux:
>> a. ECAM shift in PCIE
>> b. SPCR address space definition
> 
> What does this mean?

The SPCR in upstream edk2 is set up to work around some Linux weirdness (?) and I have to do this:

https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1

to make it work on FreeBSD.

>> Both above are not needed e.g. in FreeBSD and I was requested to add > their optional disabling.
> 
> Disabling ECAM shift is just a matter of exposing the iATU controls to the OS, right? Why do you
> need to disable it?

I'm not sure what iATU controls are (and we don't want to do anything in the OS),
but basically the current address is shifted by 0x8000 to only expose the last device to the OS,
to work around the silicon bug (lack of some filtering thing) that causes devices to appear many many times.

But actually most modern devices (e.g. AMD RX 480, Mellanox CX2) *do not* get duplicated at all, they show up
in the first position, and this shift moves the memory way past that position and the OS sees
no PCIe devices at all. The only device that was duplicated into all slots for me was a cheap SATA card.

In my experience whether the device is duplicated seems to correlate with the "Legacy" field
in the UEFI Shell's pci command. IIRC Marcin has explained the actual technical characteristic
of these devices in some mail before. So it might actually be possible to decide whether to do the shift
automatically at runtime depending on the inserted device (?)
But a setting in the setup menu is easier to do and less magical.

I've just been running with the shift reverted:

https://github.com/myfreeweb/edk2-platforms/commit/36395be2a8707f6d396e07405eb9fe47b64cf964

to make my Radeon GPU work.

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Ard Biesheuvel]

On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
> 
>> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>>
>>> Hi,
>>> I'd like to ask for comments before I develop the actual code - > currently we have 2 workarounds
>>> done specifically for Linux:
>>> a. ECAM shift in PCIE
>>> b. SPCR address space definition
>>
>> What does this mean?
> 
> The SPCR in upstream edk2 is set up to work around some Linux weirdness (?) and I have to do this:
> 
> https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
> 
> to make it work on FreeBSD.
> 

Surely, they can't both be correct. Marcin?

>>> Both above are not needed e.g. in FreeBSD and I was requested to add > their optional disabling.
>>
>> Disabling ECAM shift is just a matter of exposing the iATU controls to the OS, right? Why do you
>> need to disable it?
> 
> I'm not sure what iATU controls are (and we don't want to do anything in the OS),
> but basically the current address is shifted by 0x8000 to only expose the last device to the OS,
> to work around the silicon bug (lack of some filtering thing) that causes devices to appear many many times.
> 
> But actually most modern devices (e.g. AMD RX 480, Mellanox CX2) *do not* get duplicated at all, they show up
> in the first position, and this shift moves the memory way past that position and the OS sees
> no PCIe devices at all. The only device that was duplicated into all slots for me was a cheap SATA card.
> 
> In my experience whether the device is duplicated seems to correlate with the "Legacy" field
> in the UEFI Shell's pci command. IIRC Marcin has explained the actual technical characteristic
> of these devices in some mail before. So it might actually be possible to decide whether to do the shift
> automatically at runtime depending on the inserted device (?)
> But a setting in the setup menu is easier to do and less magical.
> 
> I've just been running with the shift reverted:
> 
> https://github.com/myfreeweb/edk2-platforms/commit/36395be2a8707f6d396e07405eb9fe47b64cf964
> 
> to make my Radeon GPU work.
> 

OK, the shift is definitely a hack, and your assertion that 'most modern 
devices do not get duplicated at all' does not match my experience, tbh.
Are these all devices that support ARI by any chance?

The problem is that the PCIe IP is truly broken, and the lack of a root 
port means that TLPs get emitted that should never reach the device in 
the first place, and it is not the job of the device to filter these 
TLPs, or reason about their own device # in the first place.

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[greg]

June 11, 2020 5:17 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:

> On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> 
>> June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
>> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>>> Disabling ECAM shift is just a matter of exposing the iATU controls to the OS, right? Why do you
>>> need to disable it?
>> 
>> I'm not sure what iATU controls are (and we don't want to do anything in the OS),
>> but basically the current address is shifted by 0x8000 to only expose the last device to the OS,
>> to work around the silicon bug (lack of some filtering thing) that causes devices to appear many
>> many times.
>> But actually most modern devices (e.g. AMD RX 480, Mellanox CX2) *do not* get duplicated at all,
>> they show up
>> in the first position, and this shift moves the memory way past that position and the OS sees
>> no PCIe devices at all. The only device that was duplicated into all slots for me was a cheap SATA
>> card.
>> In my experience whether the device is duplicated seems to correlate with the "Legacy" field
>> in the UEFI Shell's pci command. IIRC Marcin has explained the actual technical characteristic
>> of these devices in some mail before. So it might actually be possible to decide whether to do the
>> shift
>> automatically at runtime depending on the inserted device (?)
>> But a setting in the setup menu is easier to do and less magical.
>> I've just been running with the shift reverted:
>> https://github.com/myfreeweb/edk2-platforms/commit/36395be2a8707f6d396e07405eb9fe47b64cf964
>> to make my Radeon GPU work.
> 
> OK, the shift is definitely a hack, and your assertion that 'most modern devices do not get
> duplicated at all' does not match my experience, tbh.
> Are these all devices that support ARI by any chance?

Mellanox (ConnectX-2) and Intel (82599ES and good old 82576) NICs do.

But AMD GPUs actually don't! Still the RX480 (POLARIS10) only works without the shift.

An older GPU (HD7970 I think) I've tried once was duplicated, but only literally duplicated (into two),
not into *all* slots.

> The problem is that the PCIe IP is truly broken, and the lack of a root port means that TLPs get
> emitted that should never reach the device in the first place, and it is not the job of the device
> to filter these TLPs, or reason about their own device # in the first place.

Well, it must be that AMD Radeons do reason about this :)
and that sadly breaks the offset hack.

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Ard Biesheuvel]

On 6/11/20 4:33 PM, greg@unrelenting.technology wrote:
> June 11, 2020 5:17 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
> 
>> On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
>>
>>> June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
>>> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>>>> Disabling ECAM shift is just a matter of exposing the iATU controls to the OS, right? Why do you
>>>> need to disable it?
>>>
>>> I'm not sure what iATU controls are (and we don't want to do anything in the OS),
>>> but basically the current address is shifted by 0x8000 to only expose the last device to the OS,
>>> to work around the silicon bug (lack of some filtering thing) that causes devices to appear many
>>> many times.
>>> But actually most modern devices (e.g. AMD RX 480, Mellanox CX2) *do not* get duplicated at all,
>>> they show up
>>> in the first position, and this shift moves the memory way past that position and the OS sees
>>> no PCIe devices at all. The only device that was duplicated into all slots for me was a cheap SATA
>>> card.
>>> In my experience whether the device is duplicated seems to correlate with the "Legacy" field
>>> in the UEFI Shell's pci command. IIRC Marcin has explained the actual technical characteristic
>>> of these devices in some mail before. So it might actually be possible to decide whether to do the
>>> shift
>>> automatically at runtime depending on the inserted device (?)
>>> But a setting in the setup menu is easier to do and less magical.
>>> I've just been running with the shift reverted:
>>> https://github.com/myfreeweb/edk2-platforms/commit/36395be2a8707f6d396e07405eb9fe47b64cf964
>>> to make my Radeon GPU work.
>>
>> OK, the shift is definitely a hack, and your assertion that 'most modern devices do not get
>> duplicated at all' does not match my experience, tbh.
>> Are these all devices that support ARI by any chance?
> 
> Mellanox (ConnectX-2) and Intel (82599ES and good old 82576) NICs do.
> 
> But AMD GPUs actually don't! Still the RX480 (POLARIS10) only works without the shift.
> 

Interesting. It all depends on whether the endpoint decodes the device 
field to begin with: it doesn't have to, since the root port at the 
other end should be doing the filtering.

> An older GPU (HD7970 I think) I've tried once was duplicated, but only literally duplicated (into two),
> not into *all* slots.
> 

This is because the iATU granule size is 64k, and so we are only 
exposing 64k of ECAM space to the CPU. (Other implementations of this IP 
that use smaller granule sizes don't need this shift at all)

>> The problem is that the PCIe IP is truly broken, and the lack of a root port means that TLPs get
>> emitted that should never reach the device in the first place, and it is not the job of the device
>> to filter these TLPs, or reason about their own device # in the first place.
> 
> Well, it must be that AMD Radeons do reason about this :)
> and that sadly breaks the offset hack.
> 

This is interesting. I wouldn't expect the endpoint to have any 
awareness of how the ECAM space is exposed to the CPU. How does it fail 
exactly?

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[greg]

June 11, 2020 5:38 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:

> On 6/11/20 4:33 PM, greg@unrelenting.technology wrote:
> 
>> June 11, 2020 5:17 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
>> On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
>>> 
>> 
>> June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
>> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>> Disabling ECAM shift is just a matter of exposing the iATU controls to the OS, right? Why do you
>> need to disable it?
>> 
>> I'm not sure what iATU controls are (and we don't want to do anything in the OS),
>> but basically the current address is shifted by 0x8000 to only expose the last device to the OS,
>> to work around the silicon bug (lack of some filtering thing) that causes devices to appear many
>> many times.
>> But actually most modern devices (e.g. AMD RX 480, Mellanox CX2) *do not* get duplicated at all,
>> they show up
>> in the first position, and this shift moves the memory way past that position and the OS sees
>> no PCIe devices at all. The only device that was duplicated into all slots for me was a cheap SATA
>> card.
>> In my experience whether the device is duplicated seems to correlate with the "Legacy" field
>> in the UEFI Shell's pci command. IIRC Marcin has explained the actual technical characteristic
>> of these devices in some mail before. So it might actually be possible to decide whether to do the
>> shift
>> automatically at runtime depending on the inserted device (?)
>> But a setting in the setup menu is easier to do and less magical.
>> I've just been running with the shift reverted:
>> https://github.com/myfreeweb/edk2-platforms/commit/36395be2a8707f6d396e07405eb9fe47b64cf964
>> to make my Radeon GPU work.
>>> OK, the shift is definitely a hack, and your assertion that 'most modern devices do not get
>>> duplicated at all' does not match my experience, tbh.
>>> Are these all devices that support ARI by any chance?
>> 
>> Mellanox (ConnectX-2) and Intel (82599ES and good old 82576) NICs do.
>> But AMD GPUs actually don't! Still the RX480 (POLARIS10) only works without the shift.
> 
> Interesting. It all depends on whether the endpoint decodes the device field to begin with: it
> doesn't have to, since the root port at the other end should be doing the filtering.
> 
>> An older GPU (HD7970 I think) I've tried once was duplicated, but only literally duplicated (into
>> two),
>> not into *all* slots.
> 
> This is because the iATU granule size is 64k, and so we are only exposing 64k of ECAM space to the
> CPU. (Other implementations of this IP that use smaller granule sizes don't need this shift at all)
> 
>>> The problem is that the PCIe IP is truly broken, and the lack of a root port means that TLPs get
>>> emitted that should never reach the device in the first place, and it is not the job of the device
>>> to filter these TLPs, or reason about their own device # in the first place.
>> 
>> Well, it must be that AMD Radeons do reason about this :)
>> and that sadly breaks the offset hack.
> 
> This is interesting. I wouldn't expect the endpoint to have any awareness of how the ECAM space is
> exposed to the CPU. How does it fail exactly?

As I said: it just doesn't get duplicated, so with the shift, the OS does not see it at all.

I have no idea why exactly it doesn't get duplicated.

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Mark Kettenis]

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

On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:

> 
> On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> 
>> June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com> wrote:
>> 
>> 
>>> On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>>> 
>>> 
>>>> Hi,
>>>> I'd like to ask for comments before I develop the actual code - >
>>>> currently we have 2 workarounds
>>>> done specifically for Linux:
>>>> a. ECAM shift in PCIE
>>>> b. SPCR address space definition
>>> 
>>> What does this mean?
>> 
>> The SPCR in upstream edk2 is set up to work around some Linux weirdness
>> (?) and I have to do this:
>> 
>> https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
>> 
>> 
>> to make it work on FreeBSD.
> 
> Surely, they can't both be correct. Marcin?
> 
> 

Assuming the serial port on Armada/Cn911x is the same as on Armada8k, the following DT properties would be applicable:

reg-shift = <2>;
reg-io-width = <1>

which means the registers are spaced 32-bits apart but have to be accessed using 8-bit load/store instructions.  I'd say that
means that the ACPI Generic Address Space should have RegisterBitWidth set to 32 and AccessSize set to BYTE.
In other words, I think that the current:

#define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, EFI_ACPI_5_0_BYTE, Address }

is correct.  That certainly is what works for OpenBSD.

[-- Attachment #2: Type: text/html, Size: 1744 bytes --]

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Ard Biesheuvel]

On 6/12/20 12:43 AM, Mark Kettenis via Groups.Io wrote:
> On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:
> 
>     On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> 
>         June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com>
>         wrote:
> 
>             On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> 
>                 Hi,
>                 I'd like to ask for comments before I develop the actual
>                 code - > currently we have 2 workarounds
>                 done specifically for Linux:
>                 a. ECAM shift in PCIE
>                 b. SPCR address space definition
> 
>             What does this mean?
> 
>         The SPCR in upstream edk2 is set up to work around some Linux
>         weirdness (?) and I have to do this:
> 
>         https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
> 
>         to make it work on FreeBSD.
> 
>     Surely, they can't both be correct. Marcin?
> 
> Assuming the serial port on Armada/Cn911x is the same as on Armada8k, 
> the following DT properties would be applicable:
> 
> reg-shift = <2>;
> reg-io-width = <1>
> 
> which means the registers are spaced 32-bits apart but have to be 
> accessed using 8-bit load/store instructions.  I'd say that
> means that the ACPI Generic Address Space should have RegisterBitWidth 
> set to 32 and AccessSize set to BYTE.
> In other words, I think that the current:
> 
> #define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0, 
> EFI_ACPI_5_0_BYTE, Address }
> 
> is correct.  That certainly is what works for OpenBSD.

Thanks Mark

The struct type is defined as

typedef struct {
   UINT8   AddressSpaceId;
   UINT8   RegisterBitWidth;
   UINT8   RegisterBitOffset;
   UINT8   AccessSize;
   UINT64  Address;
} EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE;

so I agree that the current definition matches a UART that requires byte 
accesses on registers that are 32 bits apart.

So why does FreeBSD deviate from this?

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Marcin Wojtas]

Hi,

I see Greg was dropped in the meantime.

pt., 12 cze 2020 o 10:45 Ard Biesheuvel <ard.biesheuvel@arm.com> napisał(a):
>
> On 6/12/20 12:43 AM, Mark Kettenis via Groups.Io wrote:
> > On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:
> >
> >     On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> >
> >         June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com>
> >         wrote:
> >
> >             On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> >
> >                 Hi,
> >                 I'd like to ask for comments before I develop the actual
> >                 code - > currently we have 2 workarounds
> >                 done specifically for Linux:
> >                 a. ECAM shift in PCIE
> >                 b. SPCR address space definition
> >
> >             What does this mean?
> >
> >         The SPCR in upstream edk2 is set up to work around some Linux
> >         weirdness (?) and I have to do this:
> >
> >         https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
> >
> >         to make it work on FreeBSD.
> >
> >     Surely, they can't both be correct. Marcin?
> >
> > Assuming the serial port on Armada/Cn911x is the same as on Armada8k,
> > the following DT properties would be applicable:
> >
> > reg-shift = <2>;
> > reg-io-width = <1>
> >
> > which means the registers are spaced 32-bits apart but have to be
> > accessed using 8-bit load/store instructions.  I'd say that
> > means that the ACPI Generic Address Space should have RegisterBitWidth
> > set to 32 and AccessSize set to BYTE.
> > In other words, I think that the current:
> >
> > #define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0,
> > EFI_ACPI_5_0_BYTE, Address }
> >
> > is correct.  That certainly is what works for OpenBSD.
>
> Thanks Mark
>
> The struct type is defined as
>
> typedef struct {
>    UINT8   AddressSpaceId;
>    UINT8   RegisterBitWidth;
>    UINT8   RegisterBitOffset;
>    UINT8   AccessSize;
>    UINT64  Address;
> } EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE;
>
> so I agree that the current definition matches a UART that requires byte
> accesses on registers that are 32 bits apart.
>
> So why does FreeBSD deviate from this?

Greg, can you please explain your concerns here?

Thanks,
Marcin

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[greg]

On June 12, 2020 3:07:21 PM UTC, Marcin Wojtas <mw@semihalf.com> wrote:
>Hi,
>
>I see Greg was dropped in the meantime.
>
>pt., 12 cze 2020 o 10:45 Ard Biesheuvel <ard.biesheuvel@arm.com> napisał(a):
>>
>> On 6/12/20 12:43 AM, Mark Kettenis via Groups.Io wrote:
>> > On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:
>> >
>> >     On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
>> >
>> >         June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com>
>> >         wrote:
>> >
>> >             On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
>> >
>> >                 Hi,
>> >                 I'd like to ask for comments before I develop the actual
>> >                 code - > currently we have 2 workarounds
>> >                 done specifically for Linux:
>> >                 a. ECAM shift in PCIE
>> >                 b. SPCR address space definition
>> >
>> >             What does this mean?
>> >
>> >         The SPCR in upstream edk2 is set up to work around some Linux
>> >         weirdness (?) and I have to do this:
>> >
>> >         https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
>> >
>> >         to make it work on FreeBSD.
>> >
>> >     Surely, they can't both be correct. Marcin?
>> >
>> > Assuming the serial port on Armada/Cn911x is the same as on Armada8k,
>> > the following DT properties would be applicable:
>> >
>> > reg-shift = <2>;
>> > reg-io-width = <1>
>> >
>> > which means the registers are spaced 32-bits apart but have to be
>> > accessed using 8-bit load/store instructions.  I'd say that
>> > means that the ACPI Generic Address Space should have RegisterBitWidth
>> > set to 32 and AccessSize set to BYTE.
>> > In other words, I think that the current:
>> >
>> > #define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0,
>> > EFI_ACPI_5_0_BYTE, Address }
>> >
>> > is correct.  That certainly is what works for OpenBSD.
>>
>> Thanks Mark
>>
>> The struct type is defined as
>>
>> typedef struct {
>>    UINT8   AddressSpaceId;
>>    UINT8   RegisterBitWidth;
>>    UINT8   RegisterBitOffset;
>>    UINT8   AccessSize;
>>    UINT64  Address;
>> } EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE;
>>
>> so I agree that the current definition matches a UART that requires byte
>> accesses on registers that are 32 bits apart.
>>
>> So why does FreeBSD deviate from this?
>
>Greg, can you please explain your concerns here?

Yeah, looking at our code again, looks like we did screw it up, seems like we are using register width as access width and vice versa :/

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Marcin Wojtas]

pt., 12 cze 2020 o 18:32 myfreeweb <greg@unrelenting.technology> napisał(a):
>
>
>
> On June 12, 2020 3:07:21 PM UTC, Marcin Wojtas <mw@semihalf.com> wrote:
> >Hi,
> >
> >I see Greg was dropped in the meantime.
> >
> >pt., 12 cze 2020 o 10:45 Ard Biesheuvel <ard.biesheuvel@arm.com> napisał(a):
> >>
> >> On 6/12/20 12:43 AM, Mark Kettenis via Groups.Io wrote:
> >> > On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:
> >> >
> >> >     On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> >> >
> >> >         June 11, 2020 4:19 PM, "Ard Biesheuvel" <ard.biesheuvel@arm.com>
> >> >         wrote:
> >> >
> >> >             On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> >> >
> >> >                 Hi,
> >> >                 I'd like to ask for comments before I develop the actual
> >> >                 code - > currently we have 2 workarounds
> >> >                 done specifically for Linux:
> >> >                 a. ECAM shift in PCIE
> >> >                 b. SPCR address space definition
> >> >
> >> >             What does this mean?
> >> >
> >> >         The SPCR in upstream edk2 is set up to work around some Linux
> >> >         weirdness (?) and I have to do this:
> >> >
> >> >         https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
> >> >
> >> >         to make it work on FreeBSD.
> >> >
> >> >     Surely, they can't both be correct. Marcin?
> >> >
> >> > Assuming the serial port on Armada/Cn911x is the same as on Armada8k,
> >> > the following DT properties would be applicable:
> >> >
> >> > reg-shift = <2>;
> >> > reg-io-width = <1>
> >> >
> >> > which means the registers are spaced 32-bits apart but have to be
> >> > accessed using 8-bit load/store instructions.  I'd say that
> >> > means that the ACPI Generic Address Space should have RegisterBitWidth
> >> > set to 32 and AccessSize set to BYTE.
> >> > In other words, I think that the current:
> >> >
> >> > #define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0,
> >> > EFI_ACPI_5_0_BYTE, Address }
> >> >
> >> > is correct.  That certainly is what works for OpenBSD.
> >>
> >> Thanks Mark
> >>
> >> The struct type is defined as
> >>
> >> typedef struct {
> >>    UINT8   AddressSpaceId;
> >>    UINT8   RegisterBitWidth;
> >>    UINT8   RegisterBitOffset;
> >>    UINT8   AccessSize;
> >>    UINT64  Address;
> >> } EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE;
> >>
> >> so I agree that the current definition matches a UART that requires byte
> >> accesses on registers that are 32 bits apart.
> >>
> >> So why does FreeBSD deviate from this?
> >
> >Greg, can you please explain your concerns here?
>
> Yeah, looking at our code again, looks like we did screw it up, seems like we are using register width as access width and vice versa :/

Ok, so I'll prepare only a switch to toggle the ECAM hack enablement.

Any chance you submit a fix to phabricator?

Best regards,
Marcin

Re: [edk2-devel] Additional configuration options on Armada/Cn913x

[Marcin Wojtas]

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

Hi Greg,

pt., 12 cze 2020 o 19:09 Marcin Wojtas <mw@semihalf.com> napisał(a):

> pt., 12 cze 2020 o 18:32 myfreeweb <greg@unrelenting.technology>
> napisał(a):
> >
> >
> >
> > On June 12, 2020 3:07:21 PM UTC, Marcin Wojtas <mw@semihalf.com> wrote:
> > >Hi,
> > >
> > >I see Greg was dropped in the meantime.
> > >
> > >pt., 12 cze 2020 o 10:45 Ard Biesheuvel <ard.biesheuvel@arm.com>
> napisał(a):
> > >>
> > >> On 6/12/20 12:43 AM, Mark Kettenis via Groups.Io wrote:
> > >> > On Thu, Jun 11, 2020 at 04:17 PM, Ard Biesheuvel wrote:
> > >> >
> > >> >     On 6/11/20 4:07 PM, greg@unrelenting.technology wrote:
> > >> >
> > >> >         June 11, 2020 4:19 PM, "Ard Biesheuvel" <
> ard.biesheuvel@arm.com>
> > >> >         wrote:
> > >> >
> > >> >             On 6/5/20 5:19 PM, Marcin Wojtas via groups.io wrote:
> > >> >
> > >> >                 Hi,
> > >> >                 I'd like to ask for comments before I develop the
> actual
> > >> >                 code - > currently we have 2 workarounds
> > >> >                 done specifically for Linux:
> > >> >                 a. ECAM shift in PCIE
> > >> >                 b. SPCR address space definition
> > >> >
> > >> >             What does this mean?
> > >> >
> > >> >         The SPCR in upstream edk2 is set up to work around some
> Linux
> > >> >         weirdness (?) and I have to do this:
> > >> >
> > >> >
> https://github.com/myfreeweb/edk2-platforms/commit/74ec98a6498e78d2ae6c861db88487bf75f2e1a1
> > >> >
> > >> >         to make it work on FreeBSD.
> > >> >
> > >> >     Surely, they can't both be correct. Marcin?
> > >> >
> > >> > Assuming the serial port on Armada/Cn911x is the same as on
> Armada8k,
> > >> > the following DT properties would be applicable:
> > >> >
> > >> > reg-shift = <2>;
> > >> > reg-io-width = <1>
> > >> >
> > >> > which means the registers are spaced 32-bits apart but have to be
> > >> > accessed using 8-bit load/store instructions.  I'd say that
> > >> > means that the ACPI Generic Address Space should have
> RegisterBitWidth
> > >> > set to 32 and AccessSize set to BYTE.
> > >> > In other words, I think that the current:
> > >> >
> > >> > #define MV_UART_AS32(Address) { EFI_ACPI_5_0_SYSTEM_MEMORY, 32, 0,
> > >> > EFI_ACPI_5_0_BYTE, Address }
> > >> >
> > >> > is correct.  That certainly is what works for OpenBSD.
> > >>
> > >> Thanks Mark
> > >>
> > >> The struct type is defined as
> > >>
> > >> typedef struct {
> > >>    UINT8   AddressSpaceId;
> > >>    UINT8   RegisterBitWidth;
> > >>    UINT8   RegisterBitOffset;
> > >>    UINT8   AccessSize;
> > >>    UINT64  Address;
> > >> } EFI_ACPI_5_0_GENERIC_ADDRESS_STRUCTURE;
> > >>
> > >> so I agree that the current definition matches a UART that requires
> byte
> > >> accesses on registers that are 32 bits apart.
> > >>
> > >> So why does FreeBSD deviate from this?
> > >
> > >Greg, can you please explain your concerns here?
> >
> > Yeah, looking at our code again, looks like we did screw it up, seems
> like we are using register width as access width and vice versa :/
>
> Ok, so I'll prepare only a switch to toggle the ECAM hack enablement.
>
> Any chance you submit a fix to phabricator?
>
>
>
I pushed a patch to review: Please review and test:
https://reviews.freebsd.org/D25373

Best regards,
Marcin

[-- Attachment #2: Type: text/html, Size: 5244 bytes --]