Change in coreboot[master]: arch/x86/acpigen: Add helpers for generating _ADR

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

arch/x86/acpigen: Add helpers for generating _ADR

This change adds the following helpers:
acpigen_write_ADR: Generates _ADR object using provided 64-bit address
acpigen_write_ADR_pci: Generates _ADR object for PCI bus device.

BUG=b:153858769

Signed-off-by: Furquan Shaikh 
Change-Id: I139dfc30aa7db303c1e8bd4a8f9ee0933a60139b
---
M src/arch/x86/acpigen.c
M src/arch/x86/include/arch/acpigen.h
2 files changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/40670/1

diff --git a/src/arch/x86/acpigen.c b/src/arch/x86/acpigen.c
index 26fe08f..707f92a 100644
--- a/src/arch/x86/acpigen.c
+++ b/src/arch/x86/acpigen.c
@@ -17,6 +17,8 @@
 #include <assert.h>
 #include <console/console.h>
 #include <device/device.h>
+#include <device/pci_def.h>
+#include <device/pci_type.h>

static char *gencurrent;

@@ -1840,3 +1842,19 @@
    acpigen_emit_qword(translation);
    acpigen_emit_qword(length);
 }
+
+void acpigen_write_ADR(uint64_t adr)
+{
+	acpigen_write_name_qword("_ADR", adr);
+}
+
+void acpigen_write_ADR_pci(pci_devfn_t devfn)
+{
+	/*
+	 * _ADR for PCI Bus is encoded as follows:
+	 * [63:32] - unused
+	 * [31:16] - device #
+	 * [15:0]  - function #
+	 */
+	acpigen_write_ADR(PCI_SLOT(devfn) << 16 | PCI_FUNC(devfn));
+}
diff --git a/src/arch/x86/include/arch/acpigen.h b/src/arch/x86/include/arch/acpigen.h
index 4aba5f9..8420b4c 100644
--- a/src/arch/x86/include/arch/acpigen.h
+++ b/src/arch/x86/include/arch/acpigen.h
@@ -8,6 +8,7 @@
 #include <arch/acpi.h>
 #include <arch/acpi_device.h>
 #include <arch/acpi_pld.h>
+#include <device/pci_type.h>

/* Values that can be returned for ACPI Device _STA method */
 #define ACPI_STATUS_DEVICE_PRESENT	(1 << 0)
@@ -369,6 +370,8 @@
 void acpigen_write_return_byte(uint8_t arg);
 void acpigen_write_upc(enum acpi_upc_type type);
 void acpigen_write_pld(const struct acpi_pld *pld);
+void acpigen_write_ADR(uint64_t adr);
+void acpigen_write_ADR_pci(pci_devfn_t devfn);
 /*
  * Generate ACPI AML code for _DSM method.
  * This function takes as input uuid for the device, set of callbacks and

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 1: Code-Review+2

I just wrote one for soundwire as well, was wondering where to put it so now I will add it here.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
File src/arch/x86/include/arch/acpigen.h:

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
PS1, Line 374: void acpigen_write_ADR_pci(pci_devfn_t devfn);
I think it might be helpful to add two variants of ADR:

pci_devfn(pci_devfn_t devfn)
pci_device(const struct device *device).

One can likely call the other, but there's no need for the caller to muck around in struct device to obtain the correct parameter which the expectation that the caller already has a handle to a struct device.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 1:

(1 comment)
> Patch Set 1: Code-Review+2
>
> I just wrote one for soundwire as well, was wondering where to put it so now I will add it here.

Yeah, I realized that you might be needing this for Soundwire as well.

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
File src/arch/x86/include/arch/acpigen.h:

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
PS1, Line 374: void acpigen_write_ADR_pci(pci_devfn_t devfn);
> I think it might be helpful to add two variants of ADR: […]

That makes sense. I will add that.

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Duncan Laurie, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40670

to look at the new patch set (#2).

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

arch/x86/acpigen: Add helpers for generating _ADR

This change adds the following helpers:
acpigen_write_ADR: Generates _ADR object using provided 64-bit address
acpigen_write_ADR_pci_devfn: Generates _ADR object for PCI bus device
using devfn as input.
acpigen_write_ADR_pci_device: Generates _ADR object for PCI
bus device using struct device * as input.

BUG=b:153858769

Signed-off-by: Furquan Shaikh 
Change-Id: I139dfc30aa7db303c1e8bd4a8f9ee0933a60139b
---
M src/arch/x86/acpigen.c
M src/arch/x86/include/arch/acpigen.h
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/40670/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
File src/arch/x86/include/arch/acpigen.h:

https://review.coreboot.org/c/coreboot/+/40670/1/src/arch/x86/include/arch/a... 
PS1, Line 374: void acpigen_write_ADR_pci(pci_devfn_t devfn);
> That makes sense. I will add that.

Done

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 2: Code-Review+2

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG@10 
PS3, Line 10: acpigen_write_ADR: Generates _ADR object using provided 64-bit address
This doesn't match anything in my understanding of ACPI. Can you give
an example or pointer to the spec?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG@10 
PS3, Line 10: acpigen_write_ADR: Generates _ADR object using provided 64-bit address
> This doesn't match anything in my understanding of ACPI. Can you give […]

ACPI specification version 6.3 section 6.1.1 says that _ADR is an "integer" containing the address of the device. Integer is represented as 64-bits since ACPI 2.0.

Also, relevant change for kernel: https://patchwork.kernel.org/patch/10901037/

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40670/3//COMMIT_MSG@10 
PS3, Line 10: acpigen_write_ADR: Generates _ADR object using provided 64-bit address
> ACPI specification version 6.3 section 6.1. […]

Yes, thank you. I read "64-bit address" and subconsciously implied a
memory address, that's where I stumbled.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40670 )

Change subject: arch/x86/acpigen: Add helpers for generating _ADR
......................................................................

arch/x86/acpigen: Add helpers for generating _ADR

This change adds the following helpers:
acpigen_write_ADR: Generates _ADR object using provided 64-bit address
acpigen_write_ADR_pci_devfn: Generates _ADR object for PCI bus device
using devfn as input.
acpigen_write_ADR_pci_device: Generates _ADR object for PCI
bus device using struct device * as input.

BUG=b:153858769

Signed-off-by: Furquan Shaikh 
Change-Id: I139dfc30aa7db303c1e8bd4a8f9ee0933a60139b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40670
Reviewed-by: Nico Huber 
Reviewed-by: Aaron Durbin 
Reviewed-by: Raul Rangel 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/arch/x86/acpigen.c
M src/arch/x86/include/arch/acpigen.h
2 files changed, 28 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Aaron Durbin: Looks good to me, approved
  Raul Rangel: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/arch/x86/acpigen.c b/src/arch/x86/acpigen.c
index 26fe08f..715c38b 100644
--- a/src/arch/x86/acpigen.c
+++ b/src/arch/x86/acpigen.c
@@ -17,6 +17,8 @@
 #include <assert.h>
 #include <console/console.h>
 #include <device/device.h>
+#include <device/pci_def.h>
+#include <device/pci_type.h>

static char *gencurrent;

@@ -1840,3 +1842,25 @@
    acpigen_emit_qword(translation);
    acpigen_emit_qword(length);
 }
+
+void acpigen_write_ADR(uint64_t adr)
+{
+	acpigen_write_name_qword("_ADR", adr);
+}
+
+void acpigen_write_ADR_pci_devfn(pci_devfn_t devfn)
+{
+	/*
+	 * _ADR for PCI Bus is encoded as follows:
+	 * [63:32] - unused
+	 * [31:16] - device #
+	 * [15:0]  - function #
+	 */
+	acpigen_write_ADR(PCI_SLOT(devfn) << 16 | PCI_FUNC(devfn));
+}
+
+void acpigen_write_ADR_pci_device(const struct device *dev)
+{
+	assert(dev->path.type == DEVICE_PATH_PCI);
+	acpigen_write_ADR_pci_devfn(dev->path.pci.devfn);
+}
diff --git a/src/arch/x86/include/arch/acpigen.h b/src/arch/x86/include/arch/acpigen.h
index 4aba5f9..0eee7ff 100644
--- a/src/arch/x86/include/arch/acpigen.h
+++ b/src/arch/x86/include/arch/acpigen.h
@@ -8,6 +8,7 @@
 #include <arch/acpi.h>
 #include <arch/acpi_device.h>
 #include <arch/acpi_pld.h>
+#include <device/pci_type.h>

/* Values that can be returned for ACPI Device _STA method */
 #define ACPI_STATUS_DEVICE_PRESENT	(1 << 0)
@@ -369,6 +370,9 @@
 void acpigen_write_return_byte(uint8_t arg);
 void acpigen_write_upc(enum acpi_upc_type type);
 void acpigen_write_pld(const struct acpi_pld *pld);
+void acpigen_write_ADR(uint64_t adr);
+void acpigen_write_ADR_pci_devfn(pci_devfn_t devfn);
+void acpigen_write_ADR_pci_device(const struct device *dev);
 /*
  * Generate ACPI AML code for _DSM method.
  * This function takes as input uuid for the device, set of callbacks and