Re: [PATCH 5/5] brppt1: Update environment to support new boot concept
From: Bernhard Messerklinger
Sent: Thursday, August 25, 2022 8:54
Subject: [PATCH 5/5] brppt1: Update environment to support new boot concept
> * Drop legacy /boot/PPTImage.md5 check
> * Update device tree naming
> * Update t30args#0 root cmd line property to support latest kernel
> versions (root=/dev/mmcblk0p2 for linux < 4 and
> root=/dev/mmcblk1p2 for linux >= 4)
> * Add custom bootloader version string
> * Destroy invalid dtb at ${dtbaddr} and configuration script at
> ${cfgaddr} to ensure proper boot in warm restart case.
>
> Signed-off-by: Bernhard Messerklinger
>
> ---
>
> configs/brppt1_mmc_defconfig | 4 +++-
> include/configs/brppt1.h | 18 --
> 2 files changed, 15 insertions(+), 7 deletions(-)
Reviewed-by: Wolfgang Wallner
Re: [PATCH 4/5] include: configs: brppt1: Fix commit 0ea4fc4dcf90
From: Bernhard Messerklinger
Sent: Thursday, August 25, 2022 8:54
Subject: [PATCH 4/5] include: configs: brppt1: Fix commit 0ea4fc4dcf90
> Commit 0ea4fc4dcf90 ("board/BuR: invalidate ${dtbaddr} before cfgscr")
> destroys the boot targets b_t30lgcy#0 and b_t30lgcy#1. The reason behind
> this is, that b_t30lgcy#0 and b_t30lgcy#1 both load the for booting
> needed device trees from mmc and the cfgscr script patches those. Because
> of this, cfgscr is not allowed to destroy the previously loaded device
> tree otherwise cfgscr will fail.
> This patch moves the device trees invalidation on warm restart to the
> PREBOOT cmd to fix that issue.
>
> Fixes: 0ea4fc4dcf90 ("board/BuR: invalidate ${dtbaddr} before cfgscr")
> Signed-off-by: Bernhard Messerklinger
>
> ---
>
> configs/brppt1_mmc_defconfig | 2 +-
> include/configs/brppt1.h | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Wolfgang Wallner
Re: [PATCH 3/5] brppt1: Cleanup device tree
From: Bernhard Messerklinger Sent: Thursday, August 25, 2022 8:54 Subject: [PATCH 3/5] brppt1: Cleanup device tree > * Remove unnecessary device tree nodes which are not needed in > U-Boot directly. > * Move all U-Boot specific device tree properties to u-boot dtsi. > > Signed-off-by: Bernhard Messerklinger > > --- > > arch/arm/dts/am335x-brppt1-mmc-u-boot.dtsi | 32 > arch/arm/dts/am335x-brppt1-mmc.dts | 201 - > 2 files changed, 32 insertions(+), 201 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 2/5] brppt1: Fix SPL boot stage
From: Bernhard Messerklinger
Sent: Thursday, August 25, 2022 8:54
Subject: [PATCH 2/5] brppt1: Fix SPL boot stage
> Commit 6337d53fdf45 ("arm: dts: sync am33xx with Linux 5.9-rc7") syncs
> the am335x device tree with the latest linux kernel am335x device tree.
> That causes problems with device tree in SPL stage.
> To fix the issues CONFIG_SPL_OF_TRANSLATE must be set to handle the
> synced bus addresses correctly.
> A custom U-Boot device tree is also needed since the SPL build removes
> bus properties from bus nodes which are not explicitly marked with the
> u-boot,dm-spl or u-boot,dm-pre-reloc flag. Therefore all parent buses of
> the in the SPL needed devices must be marked with u-boot,dm-pre-reloc.
> Also since there is no driver for "ti,sysc" compatible property in SPL
> the buses marked with this compatible string must also be marked with
> compatible = "simple-bus" to make the underlying devices visible in
> SPL. Otherwise the matching device drivers aren't found and the uclass
> drivers are dropped.
>
> Signed-off-by: Bernhard Messerklinger
>
> ---
>
> arch/arm/dts/am335x-brppt1-mmc-u-boot.dtsi | 80 ++
> configs/brppt1_mmc_defconfig | 2 +-
> 2 files changed, 81 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/dts/am335x-brppt1-mmc-u-boot.dtsi
Reviewed-by: Wolfgang Wallner
Re: [PATCH 1/5] brppt1: Remove unused board variants
From: Bernhard Messerklinger Sent: Thursday, August 25, 2022 8:54 Subject: [PATCH 1/5] brppt1: Remove unused board variants > The SPI and NAND board variants never went into production. > Drop those board variants. > > Signed-off-by: Bernhard Messerklinger > > --- > > arch/arm/dts/Makefile | 2 - > arch/arm/dts/am335x-brppt1-nand.dts | 374 --- > arch/arm/dts/am335x-brppt1-spi.dts | 377 > board/BuR/brppt1/board.c | 4 +- > board/BuR/brppt1/mux.c | 39 +-- > configs/brppt1_nand_defconfig | 122 - > configs/brppt1_spi_defconfig | 130 -- > include/configs/brppt1.h | 64 + > 8 files changed, 7 insertions(+), 1105 deletions(-) > delete mode 100644 arch/arm/dts/am335x-brppt1-nand.dts > delete mode 100644 arch/arm/dts/am335x-brppt1-spi.dts > delete mode 100644 configs/brppt1_nand_defconfig > delete mode 100644 configs/brppt1_spi_defconfig Reviewed-by: Wolfgang Wallner
Re: [PATCH 0/5] Fix, update and cleanup brppt1 board
Complete series is Reviewed-by: Wolfgang Wallner From: Bernhard Messerklinger Sent: Thursday, August 25, 2022 8:53 To: u-boot@lists.denx.de Cc: Wolfgang Wallner ; Bernhard Messerklinger ; Andre Przywara ; Christian Hewitt ; Hannes Schmelzer ; Marcel Ziswiler ; Marek Vasut ; Pali Rohár ; Samuel Holland ; Simon Glass ; Ying-Chun Liu (PaulLiu) Subject: [PATCH 0/5] Fix, update and cleanup brppt1 board Drop board variants that were never produced, fix the SPL loader, and update environment. Bernhard Messerklinger (5): brppt1: Remove unused board variants brppt1: Fix SPL boot stage brppt1: Cleanup device tree include: configs: brppt1: Fix commit 0ea4fc4dcf90 brppt1: Update environment to support new boot concept arch/arm/dts/Makefile | 2 - arch/arm/dts/am335x-brppt1-mmc-u-boot.dtsi | 112 ++ arch/arm/dts/am335x-brppt1-mmc.dts | 201 --- arch/arm/dts/am335x-brppt1-nand.dts | 374 arch/arm/dts/am335x-brppt1-spi.dts | 377 - board/BuR/brppt1/board.c | 4 +- board/BuR/brppt1/mux.c | 39 +-- configs/brppt1_mmc_defconfig | 6 +- configs/brppt1_nand_defconfig | 122 --- configs/brppt1_spi_defconfig | 130 --- include/configs/brppt1.h | 83 + 11 files changed, 135 insertions(+), 1315 deletions(-) create mode 100644 arch/arm/dts/am335x-brppt1-mmc-u-boot.dtsi delete mode 100644 arch/arm/dts/am335x-brppt1-nand.dts delete mode 100644 arch/arm/dts/am335x-brppt1-spi.dts delete mode 100644 configs/brppt1_nand_defconfig delete mode 100644 configs/brppt1_spi_defconfig -- 2.37.2
Re: [PATCH] board/BuR/*: replace maintainer of BuR boards
> Since i'm leaving the company with end of june, the maintainership will > be transferred to Wolfgang Wallner. Signed-off-by: Wolfgang Wallner From: Hannes Schmelzer Sent: Wednesday, June 29, 2022 12:11 To: u-boot@lists.denx.de Cc: Hannes Schmelzer ; Wolfgang Wallner Subject: [PATCH] board/BuR/*: replace maintainer of BuR boards Since i'm leaving the company with end of june, the maintainership will be transferred to Wolfgang Wallner. Signed-off-by: Hannes Schmelzer Seriec-cc: tr...@konsulko.com Series-version : 2 Series-changes : 2 - fix typo in Wolgangs email-address --- board/BuR/brppt1/MAINTAINERS | 2 +- board/BuR/brppt2/MAINTAINERS | 2 +- board/BuR/brsmarc1/MAINTAINERS | 2 +- board/BuR/brxre1/MAINTAINERS | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/board/BuR/brppt1/MAINTAINERS b/board/BuR/brppt1/MAINTAINERS index 9eddab4..6b45508 100644 --- a/board/BuR/brppt1/MAINTAINERS +++ b/board/BuR/brppt1/MAINTAINERS @@ -1,5 +1,5 @@ BRPPT1 BOARD -M: Hannes Schmelzer +M: Wolfgang Wallner S: Maintained F: board/BuR/brppt1/ F: include/configs/brppt1.h diff --git a/board/BuR/brppt2/MAINTAINERS b/board/BuR/brppt2/MAINTAINERS index a1b5bd4..fe65188 100644 --- a/board/BuR/brppt2/MAINTAINERS +++ b/board/BuR/brppt2/MAINTAINERS @@ -1,5 +1,5 @@ BUR_PPT2 BOARD -M: Hannes Schmelzer +M: Wolfgang Wallner S: Maintained F: board/BuR/brppt2/ F: include/configs/brppt2.h diff --git a/board/BuR/brsmarc1/MAINTAINERS b/board/BuR/brsmarc1/MAINTAINERS index c6dfc20..8d1fe21 100644 --- a/board/BuR/brsmarc1/MAINTAINERS +++ b/board/BuR/brsmarc1/MAINTAINERS @@ -1,5 +1,5 @@ BRSMARC1 BOARD -M: Hannes Schmelzer +M: Wolfgang Wallner S: Maintained F: board/BuR/brsmarc1/ F: include/configs/brsmarc1.h diff --git a/board/BuR/brxre1/MAINTAINERS b/board/BuR/brxre1/MAINTAINERS index eb0fe8b..5aa3671 100644 --- a/board/BuR/brxre1/MAINTAINERS +++ b/board/BuR/brxre1/MAINTAINERS @@ -1,5 +1,5 @@ BRXRE1 BOARD -M: Hannes Schmelzer +M: Wolfgang Wallner S: Maintained F: board/BuR/brxre1/ F: include/configs/brxre1.h -- 2.7.4
[PATCH] x86: mtrr: Fix function descriptions
Fix copy/paste errors in the descriptions of mtrr_close () and mtrr_set().
Signed-off-by: Wolfgang Wallner
---
arch/x86/include/asm/mtrr.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index 3a98aacdef..384672e93f 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -103,7 +103,7 @@ struct mtrr_info {
void mtrr_open(struct mtrr_state *state, bool do_caches);
/**
- * mtrr_open() - Clean up after adjusting MTRRs, and enable them
+ * mtrr_close() - Clean up after adjusting MTRRs, and enable them
*
* This uses the structure containing information returned from mtrr_open().
*
@@ -170,7 +170,7 @@ void mtrr_read_all(struct mtrr_info *info);
int mtrr_set_valid(int cpu_select, int reg, bool valid);
/**
- * mtrr_set() - Set the valid flag for a selected MTRR and CPU(s)
+ * mtrr_set() - Set the base address and mask for a selected MTRR and CPU(s)
*
* @cpu_select: Selected CPUs (either a CPU number or MP_SELECT_...)
* @reg: MTRR register to write (0-7)
--
2.30.2
[PATCH] dt-bindings: fsp: Fix Apollo Lake FSP-S devicetree bindings
An entry is missing in the FSP-S devicetree bindings, and as a result the description for the next few following entries is off by one line. Signed-off-by: Wolfgang Wallner --- .../fsp/fsp2/apollolake/fsp-s.txt | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt index b605ed0056..dc8e3251a3 100644 --- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt +++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt @@ -258,19 +258,20 @@ Optional properties: - fsps,pcie-rp-clk-req-detect: CLKREQ# Detection - fsps,advanced-error-reportingt: Advanced Error Reporting - fsps,pme-interrupt: PME Interrupt -- fsps,fatal-error-report: URR -- fsps,no-fatal-error-report: FER -- fsps,correctable-error-report: NFER -- fsps,system-error-on-fatal-error: CER -- fsps,system-error-on-non-fatal-error: SEFE -- fsps,system-error-on-correctable-error: SENFE -- fsps,pcie-rp-speed: SECE -- fsps,physical-slot-number: PCIe Speed +- fsps,unsupported-request-report: URR +- fsps,fatal-error-report: FER +- fsps,no-fatal-error-report: NFER +- fsps,correctable-error-report: CER +- fsps,system-error-on-fatal-error: SEFE +- fsps,system-error-on-non-fatal-error: SENFE +- fsps,system-error-on-correctable-error: SECE +- fsps,pcie-rp-speed: PCIe Speed +- fsps,physical-slot-number: Physical Slot Number 0: Auto (default) 1: Gen1 2: Gen2 3: Gen3 -- fsps,pcie-rp-completion-timeout: Physical Slot Number +- fsps,pcie-rp-completion-timeout: CTO 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 (default) - fsps,enable-ptm: PTM Support - fsps,pcie-rp-aspm: ASPM -- 2.30.2
Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 devices
Hi Bin, -"Bin Meng" schrieb: - > Betreff: Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 > devices > > Hi Wolfgang, > > On Tue, Sep 22, 2020 at 10:19 PM Wolfgang Wallner > wrote: > > > > Hi Simon, > > > > -"Simon Glass" schrieb: - > > > Betreff: Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common > > > x86 devices > > > > > > Hi Wolfgang, > > > > > > On Mon, 21 Sep 2020 at 07:50, Wolfgang Wallner > > > wrote: > > > > > > > > Hi Simon, > > > > > > > > -"Simon Glass" schrieb: - > > > > > Betreff: [PATCH v3 02/57] x86: acpi: Add base asl files for common > > > > > x86 devices > > > > > > > > > > Add common x86 ASL files, taken from coreboot. > > > > > > > > > > Signed-off-by: Simon Glass > > > > > --- > > > > > > > > > > (no changes since v1) > > > > > > > > > > arch/x86/include/asm/acpi/chromeos.asl| 108 + > > > > > arch/x86/include/asm/acpi/cpu.asl | 25 > > > > > arch/x86/include/asm/acpi/cros_gnvs.asl | 29 + > > > > > arch/x86/include/asm/acpi/lpc.asl | 141 > > > > > ++ > > > > > arch/x86/include/asm/acpi/pci_osc.asl | 21 > > > > > arch/x86/include/asm/acpi/pcr.asl | 80 > > > > > arch/x86/include/asm/acpi/ramoops.asl | 32 + > > > > > arch/x86/include/asm/acpi/sleepstates.asl | 12 +- > > > > > 8 files changed, 443 insertions(+), 5 deletions(-) > > > > > create mode 100644 arch/x86/include/asm/acpi/chromeos.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/cpu.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/cros_gnvs.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/lpc.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/pci_osc.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/pcr.asl > > > > > create mode 100644 arch/x86/include/asm/acpi/ramoops.asl > > > > > > > > There are recent (~June 2020) commits in coreboot which convert most > > > > ASL files > > > > to ASL 2.0. The ASL 2.0 syntax is much easier to read IMHO, and I think > > > > it > > > > would be good to update this patch to take advantage of the recent > > > > coreboot > > > > developments. See e.g. the commit at [1]. > > > > > > Yes that looks nicer, but I have everything working and tested now so > > > don't want to go back and unpick things. This seems like something we > > > can pick up once we finally have this in the tree. The good thing is > > > that this is just a syntax change as I understand it. > > > > Fair enough. Yes, it should only be a syntax change. > > > > Regardingn testing: I haven't tested individual patches of part D, but I > > have > > tested the complete series. After applying the fix for the DSDT length my > > ApolloLake board has booted reliably with ACPI enabled. > > > > Will you add the "Tested-by" tag for this series? I lack experience in testing complete series, which is why I was not sure how this should be handled. If testing a complete series at once is enough, without testing the individual patches, then this series is: Tested-by: Wolfgang Wallner Tested on a custom ApolloLake board. > BTW: any plan to upstream your board support? Yes, that would be one of the long term goals. regards, Wolfgang
Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 devices
Hi Simon, -"Simon Glass" schrieb: - > Betreff: Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 > devices > > Hi Wolfgang, > > On Mon, 21 Sep 2020 at 07:50, Wolfgang Wallner > wrote: > > > > Hi Simon, > > > > -"Simon Glass" schrieb: - > > > Betreff: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 > > > devices > > > > > > Add common x86 ASL files, taken from coreboot. > > > > > > Signed-off-by: Simon Glass > > > --- > > > > > > (no changes since v1) > > > > > > arch/x86/include/asm/acpi/chromeos.asl| 108 + > > > arch/x86/include/asm/acpi/cpu.asl | 25 > > > arch/x86/include/asm/acpi/cros_gnvs.asl | 29 + > > > arch/x86/include/asm/acpi/lpc.asl | 141 ++ > > > arch/x86/include/asm/acpi/pci_osc.asl | 21 > > > arch/x86/include/asm/acpi/pcr.asl | 80 > > > arch/x86/include/asm/acpi/ramoops.asl | 32 + > > > arch/x86/include/asm/acpi/sleepstates.asl | 12 +- > > > 8 files changed, 443 insertions(+), 5 deletions(-) > > > create mode 100644 arch/x86/include/asm/acpi/chromeos.asl > > > create mode 100644 arch/x86/include/asm/acpi/cpu.asl > > > create mode 100644 arch/x86/include/asm/acpi/cros_gnvs.asl > > > create mode 100644 arch/x86/include/asm/acpi/lpc.asl > > > create mode 100644 arch/x86/include/asm/acpi/pci_osc.asl > > > create mode 100644 arch/x86/include/asm/acpi/pcr.asl > > > create mode 100644 arch/x86/include/asm/acpi/ramoops.asl > > > > There are recent (~June 2020) commits in coreboot which convert most ASL > > files > > to ASL 2.0. The ASL 2.0 syntax is much easier to read IMHO, and I think it > > would be good to update this patch to take advantage of the recent coreboot > > developments. See e.g. the commit at [1]. > > Yes that looks nicer, but I have everything working and tested now so > don't want to go back and unpick things. This seems like something we > can pick up once we finally have this in the tree. The good thing is > that this is just a syntax change as I understand it. Fair enough. Yes, it should only be a syntax change. Regardingn testing: I haven't tested individual patches of part D, but I have tested the complete series. After applying the fix for the DSDT length my ApolloLake board has booted reliably with ACPI enabled. I have some remaining issues that I haven't gone after yet (e.g. I see a machine check error in the log), but the board always boots. > A WFH project has been putting a few Chromebooks in my lab, and I > expect to put coral in there once everything lands. That should give > us the confidence to make minor and major changes in future. > > Regards, > SImon > > > [1] > > https://github.com/coreboot/coreboot/commit/03248033e7be6f81ad5b60ed21a60071aee32c67 > > regards, Wolfgang
Re: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 devices
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v3 02/57] x86: acpi: Add base asl files for common x86 devices > > Add common x86 ASL files, taken from coreboot. > > Signed-off-by: Simon Glass > --- > > (no changes since v1) > > arch/x86/include/asm/acpi/chromeos.asl| 108 + > arch/x86/include/asm/acpi/cpu.asl | 25 > arch/x86/include/asm/acpi/cros_gnvs.asl | 29 + > arch/x86/include/asm/acpi/lpc.asl | 141 ++ > arch/x86/include/asm/acpi/pci_osc.asl | 21 > arch/x86/include/asm/acpi/pcr.asl | 80 > arch/x86/include/asm/acpi/ramoops.asl | 32 + > arch/x86/include/asm/acpi/sleepstates.asl | 12 +- > 8 files changed, 443 insertions(+), 5 deletions(-) > create mode 100644 arch/x86/include/asm/acpi/chromeos.asl > create mode 100644 arch/x86/include/asm/acpi/cpu.asl > create mode 100644 arch/x86/include/asm/acpi/cros_gnvs.asl > create mode 100644 arch/x86/include/asm/acpi/lpc.asl > create mode 100644 arch/x86/include/asm/acpi/pci_osc.asl > create mode 100644 arch/x86/include/asm/acpi/pcr.asl > create mode 100644 arch/x86/include/asm/acpi/ramoops.asl There are recent (~June 2020) commits in coreboot which convert most ASL files to ASL 2.0. The ASL 2.0 syntax is much easier to read IMHO, and I think it would be good to update this patch to take advantage of the recent coreboot developments. See e.g. the commit at [1]. regards, Wolfgang [1] https://github.com/coreboot/coreboot/commit/03248033e7be6f81ad5b60ed21a60071aee32c67
Re: [PATCH 1/2] x86: acpi: Fix calculation of DSDT length
Hi Andy, -"Andy Shevchenko" schrieb: - > Betreff: Re: [PATCH 1/2] x86: acpi: Fix calculation of DSDT length > > On Wed, Sep 09, 2020 at 04:08:17PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 09, 2020 at 02:33:20PM +0200, Wolfgang Wallner wrote: > > > Currently, the calculation for the length of the DSDT table includes any > > > bytes that are added for alignment, but those bytes are not initialized. > > > > > > This is because the DSDT length is calculated after a call to > > > acpi_inc_align(). Split this up into the following sequence: > > > > > > * acpi_inc() > > > * Calculate DSDT length > > > * acpi_align() > > > > Perhaps Fixes: tag? > > Ah, it is against series not yet applied? No, the patch is against master. But I could not point to a specific commit that is fixed, the code in question is a result of multiple commits. Because of this I have also sent V2 without a Fixes tags. regards, Wolfgang
[PATCH v2 2/2] x86: acpi: Add memset to initialize SPCR table
Add a missing memset to acpi_create_spcr().
The other acpi_create_() functions perform a memset on their
structures, acpi_create_spcr() does not and as a result the contents of
this table are partly uninitialized (and thus random after every reset).
Fixes: commit b288cd960072 ("x86: acpi: Generate SPCR table")
Signed-off-by: Wolfgang Wallner
Reviewed-by: Andy Shevchenko
Reviewed-by: Simon Glass
---
Changes in v2:
- Removed unrelated whitespace change
- Added Reviewed-by tags
arch/x86/lib/acpi_table.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index 6b827bfa3f..c445aa6870 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -252,6 +252,8 @@ static void acpi_create_spcr(struct acpi_spcr *spcr)
int space_id;
int ret = -ENODEV;
+ memset((void *)spcr, 0, sizeof(struct acpi_spcr));
+
/* Fill out header fields */
acpi_fill_header(header, "SPCR");
header->length = sizeof(struct acpi_spcr);
--
2.28.0
[PATCH v2 1/2] x86: acpi: Fix calculation of DSDT length
Currently, the calculation for the length of the DSDT table includes any
bytes that are added for alignment, but those bytes are not initialized.
This is because the DSDT length is calculated after a call to
acpi_inc_align(). Split this up into the following sequence:
* acpi_inc()
* Calculate DSDT length
* acpi_align()
Signed-off-by: Wolfgang Wallner
Reviewed-by: Andy Shevchenko
Reviewed-by: Simon Glass
---
Changes in v2:
- Added Reviewed-by tags
arch/x86/lib/acpi_table.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index 3a93fedfc3..6b827bfa3f 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -427,7 +427,7 @@ ulong write_acpi_tables(ulong start_addr)
(char *) + sizeof(struct acpi_table_header),
dsdt->length - sizeof(struct acpi_table_header));
- acpi_inc_align(ctx, dsdt->length - sizeof(struct acpi_table_header));
+ acpi_inc(ctx, dsdt->length - sizeof(struct acpi_table_header));
/* Pack GNVS into the ACPI table area */
for (i = 0; i < dsdt->length; i++) {
@@ -450,6 +450,8 @@ ulong write_acpi_tables(ulong start_addr)
dsdt->checksum = 0;
dsdt->checksum = table_compute_checksum((void *)dsdt, dsdt->length);
+ acpi_align(ctx);
+
/*
* Fill in platform-specific global NVS variables. If this fails we
* cannot return the error but this should only happen while debugging.
--
2.28.0
Re: [PATCH] x86: Drop duplicate declaration of emulator state
Hi Simon, -"U-Boot" schrieb: - > Betreff: [PATCH] x86: Drop duplicate declaration of emulator state > > With x86 we can execute an option ROM either natively or using the x86 > emulator (if enabled with CONFIG_BIOSEMU). Both of these share the > _X86EMU_env variable, with the native code using it to hold register state > during interrupt processing. > > At present, in 32-bit U-Boot, the variable is declared twice, once in > common code and once in code only compiled with CONFIG_BIOSEMU. > > With gcc-10 this causes a 'multiple definitions' error on boards with > CONFIG_BIOSEMU. > > Drop the emulator definition, except for 64-bit builds. > > Also drop inclusion of the emulator in 64-bit U-Boot since this does not > work at present, and generally isn't needed if 32-bit code has already set > up the option ROMs. > > Reported-by: Heinrich Schuchardt > Signed-off-by: Simon Glass > --- > > drivers/bios_emulator/x86emu/sys.c | 4 > 1 file changed, 4 insertions(+) Tested-by: Wolfgang Wallner Tested by building chromebook_coral_defconfig on an Arch Linux with GCC 10.1.0 regards, Wolfgang
[PATCH] x86: fsp: Replace e-mmc with emmc in devicetree bindings
The term eMMC is used inconsistently within the FSP devicetree
bindigs (e-mmc and emmc), especially for "emmc-host-max-speed"
documentation and code disagree.
Change all eMMC instances within the FSP bindings to consistently
use "emmc". The term "emmc" is already used a lot within U-Boot,
while "e-mmc" is only used in the FSP bindings.
Signed-off-by: Wolfgang Wallner
---
arch/x86/cpu/apollolake/fsp_bindings.c | 6 +++---
doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt | 2 +-
doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/cpu/apollolake/fsp_bindings.c
b/arch/x86/cpu/apollolake/fsp_bindings.c
index bbf04b5009..319c78b95a 100644
--- a/arch/x86/cpu/apollolake/fsp_bindings.c
+++ b/arch/x86/cpu/apollolake/fsp_bindings.c
@@ -555,7 +555,7 @@ const struct fsp_binding fsp_m_bindings[] = {
}, {
.type = FSP_UINT8,
.offset = offsetof(struct fsp_m_config, e_mmc_trace_len),
- .propname = "fspm,e-mmc-trace-len",
+ .propname = "fspm,emmc-trace-len",
}, {
.type = FSP_UINT8,
.offset = offsetof(struct fsp_m_config, skip_cse_rbp),
@@ -1465,11 +1465,11 @@ const struct fsp_binding fsp_s_bindings[] = {
}, {
.type = FSP_UINT8,
.offset = offsetof(struct fsp_s_config, e_mmc_enabled),
- .propname = "fsps,e-mmc-enabled",
+ .propname = "fsps,emmc-enabled",
}, {
.type = FSP_UINT8,
.offset = offsetof(struct fsp_s_config, e_mmc_host_max_speed),
- .propname = "fsps,e-mmc-host-max-speed",
+ .propname = "fsps,emmc-host-max-speed",
}, {
.type = FSP_UINT8,
.offset = offsetof(struct fsp_s_config, ufs_enabled),
diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
index 666400e085..36936f2eb6 100644
--- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
+++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
@@ -174,7 +174,7 @@ Optional properties:
- fspm,oem-loading-base: OEM File Loading Address
- fspm,oem-file-name: OEM File Name to Load
- fspm,mrc-boot-data-ptr:
-- fspm,e-mmc-trace-len: eMMC Trace Length
+- fspm,emmc-trace-len: eMMC Trace Length
0x0: Long
0x1: Short
- fspm,skip-cse-rbp: Skip CSE RBP to support zero sized IBB
diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
index 731a310cf8..b605ed0056 100644
--- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
+++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-s.txt
@@ -318,7 +318,7 @@ Optional properties:
0x6: warm reset (default)
0xE: cold reset
- fsps,sdcard-enabled: SD Card Support (D27:F0)
-- fsps,e-mmc-enabled: SeMMC Support (D28:F0)
+- fsps,emmc-enabled: SeMMC Support (D28:F0)
- fsps,emmc-host-max-speed: eMMC Max Speed
0: HS400(default)
1: HS200
--
2.28.0
chromebook_coral.dts: multiple FSP-S entries for same parameters
Hi Simon,
chromebook_coral.dts contains multiple entries of fsps,pcie-root-port-en
and fsps,pcie-rp-hot-plug.
I don't think this is intentional, as the code parsing the FSP-S settings will
only use one of those settings.
regards, Wolfgang
-- 8< -
chromebook_coral.dts:
_s {
u-boot,dm-pre-proper;
fsps,ish-enable = <0>;
fsps,enable-sata = <0>;
fsps,pcie-root-port-en = [00 00 00 00 00 01];
fsps,pcie-rp-hot-plug = [00 00 00 00 00 01];
// ...
/* Enable WiFi */
fsps,pcie-root-port-en = [01 00 00 00 00 00];
fsps,pcie-rp-hot-plug = [00 00 00 00 00 00];
// ...
};
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: Re: [PATCH] acpi: device: Fix check for sequence number
>
> Hi Wolfgang,
>
> On Thu, 13 Aug 2020 at 01:23, Wolfgang Wallner
> wrote:
> >
> > Hi Simon,
> >
> > -"Simon Glass" schrieb: -
> > > Betreff: Re: [PATCH] acpi: device: Fix check for sequence number
> > >
> > > Hi Wolfgang,
> > >
> > > On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner
> > > wrote:
> > > >
> > > > Currently the function acpi_check_seq() checks whether dev->req_seq is
> > > > unequal to "-1", but it should actually check dev->seq. Change it to
> > > > check dev->seq.
> > > >
> > > > For req_seq the value "-1" would be a valid (meaning 'any'), while for
> > > > seq the value "-1" means 'none' and is not valid.
> > > >
> > > > Quoting the description of udevice in device.h:
> > > > * @req_seq: Requested sequence number for this device (-1 = any)
> > > > * @seq: Allocated sequence number for this device (-1 = none).
> > > > * This is set up when the device is probed and will be unique
> > > > * within the device's uclass.
> > > >
> > > > Signed-off-by: Wolfgang Wallner
> > > >
> > > > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()")
> > > >
> > > > ---
> > > >
> > > > lib/acpi/acpi_device.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > >
> > > What problem are you seeing without this patch?
> >
> > I see the following warning: "Device 'serial@18,2' has no seq".
> >
> > In my case req_seq for the UART is -1 ("any"), while seq is 0.
> > Why would we check for req_seq and not for seq in this function?
> >
> > > At present the ACPI device may not always be probed, and probing is
> > > when the sequence number is currently set up.
> >
> > In my case the UART is already probed before the ACPI tables are generated.
>
> I would expect req_seq to be set to the UART number, i.e. the value of
> the alias (uart0, uart1) that points to the node.
>
> I wonder why that doesn't work in your case?
I did not have an alias for my serial. I have added one and now it works as
expected.
I misunderstood how that code is expected to work. Thanks for the explanation,
now it makes sense.
This also means that my patch is wrong and should be dropped.
@Bin: please drop this patch.
> Are you sure that all UARTs are probed before ACPI tables are created?
> Normally U-Boot would only probe the one being used for the console.
>
> >
> > >
> > > I have been thinking about dropping req_seq and doing everything when
> > > the device is bound, but haven't dug into it in detail yet.
regards, Wolfgang
[PATCH 1/2] x86: acpi: Fix calculation of DSDT length
Currently, the calculation for the length of the DSDT table includes any
bytes that are added for alignment, but those bytes are not initialized.
This is because the DSDT length is calculated after a call to
acpi_inc_align(). Split this up into the following sequence:
* acpi_inc()
* Calculate DSDT length
* acpi_align()
Signed-off-by: Wolfgang Wallner
---
arch/x86/lib/acpi_table.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
index 3a93fedfc3..6b827bfa3f 100644
--- a/arch/x86/lib/acpi_table.c
+++ b/arch/x86/lib/acpi_table.c
@@ -427,7 +427,7 @@ ulong write_acpi_tables(ulong start_addr)
(char *) + sizeof(struct acpi_table_header),
dsdt->length - sizeof(struct acpi_table_header));
- acpi_inc_align(ctx, dsdt->length - sizeof(struct acpi_table_header));
+ acpi_inc(ctx, dsdt->length - sizeof(struct acpi_table_header));
/* Pack GNVS into the ACPI table area */
for (i = 0; i < dsdt->length; i++) {
@@ -450,6 +450,8 @@ ulong write_acpi_tables(ulong start_addr)
dsdt->checksum = 0;
dsdt->checksum = table_compute_checksum((void *)dsdt, dsdt->length);
+ acpi_align(ctx);
+
/*
* Fill in platform-specific global NVS variables. If this fails we
* cannot return the error but this should only happen while debugging.
--
2.28.0
[PATCH 2/2] x86: acpi: Add memset to initialize SPCR table
Add a missing memset to acpi_create_spcr(). The other acpi_create_() functions perform a memset on their structures, acpi_create_spcr() does not and as a result the contents of this table are partly uninitialized (and thus random after every reset). Signed-off-by: Wolfgang Wallner --- arch/x86/lib/acpi_table.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c index 6b827bfa3f..054235843e 100644 --- a/arch/x86/lib/acpi_table.c +++ b/arch/x86/lib/acpi_table.c @@ -252,6 +252,8 @@ static void acpi_create_spcr(struct acpi_spcr *spcr) int space_id; int ret = -ENODEV; + memset((void *)spcr, 0, sizeof(struct acpi_spcr)); + /* Fill out header fields */ acpi_fill_header(header, "SPCR"); header->length = sizeof(struct acpi_spcr); @@ -359,6 +361,7 @@ static void acpi_create_spcr(struct acpi_spcr *spcr) spcr->baud_rate = 0; /* Fix checksum */ + header->checksum = table_compute_checksum((void *)spcr, header->length); } -- 2.28.0
Re: [PATCH v1] cmd: acpi: Print revisions in hex format
Hi Andy, -"Andy Shevchenko" schrieb: - > Betreff: Re: [PATCH v1] cmd: acpi: Print revisions in hex format > > On Tue, Sep 08, 2020 at 05:32:08PM +0200, Wolfgang Wallner wrote: > > -"Andy Shevchenko" schrieb: - > > > On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner > > > wrote: > > > > -"Andy Shevchenko" schrieb: > > > > - > > ... > > > > > Related to "acpi list": > > > > During my recent ACPI debugging I found it very useful to have the > > > > checksum > > > > printed for each table with "acpi list". Would there be interest to > > > > have that > > > > upstream? If so I would send a patch. > > > > > > Can you elaborate what was the problem that checksum helped? > > > > Sure. I saw two strange things with the ACPI checksums: > > > > 1) The DSDT length included uninitialized bytes from alignment. This is > > described in the following link: > > > >https://lists.denx.de/pipermail/u-boot/2020-September/425378.html > > > > This was the actual bug I was looking for. > > > > 2) acpi_create_spcr() is missing a memset(). The other acpi_create_() > > functions perform a memset on their structure, acpi_create_spcr() does not > > and as a result the contents of this table are party uninitialized. > > > > I plan to send a patch for both of them. > > I'm not sure I understood how checksum pointed to uninitialized data? After adding the checksums to "acpi list" I realized that the checksums for DSDT and SPCR where different after every reset. Looking at the code each turned out to be somehow related to uninitialized memory. regards, Wolfgang PS: My mail client has somehow corrupted your last mail, and I can't open it. A colleague has forwarded it to me so I could reply. This is why I reply to my own mail.
Re: [PATCH v1] cmd: acpi: Print revisions in hex format
Hi Andy,
-"Andy Shevchenko" schrieb: -
> Betreff: Re: [PATCH v1] cmd: acpi: Print revisions in hex format
>
> On Tue, Sep 8, 2020 at 5:58 PM Wolfgang Wallner
> wrote:
> > -"Andy Shevchenko" schrieb: -
> > > Betreff: [PATCH v1] cmd: acpi: Print revisions in hex format
> > >
> > > The revisions are usually dates in hex-decimal format representing
> > > mmdd. Print them in hex to see this clearly.
> > >
> > > Before:
> > > ...
> > > FACP 000e5420 f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> > > DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> > > ...
> > > After:
> > > ...
> > > FACP 000e5420 f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> > > DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 1 INTL 20200326)
> > > ...
> > >
> > > Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> > > Cc: Wolfgang Wallner
> > > Signed-off-by: Andy Shevchenko
> > > ---
> > > cmd/acpi.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Reviewed-by: Wolfgang Wallner
> > Tested-by: Wolfgang Wallner
> > Tested on a custom Apollolake board.
>
> Thanks!
You're welcome.
>
> > Related to "acpi list":
> > During my recent ACPI debugging I found it very useful to have the checksum
> > printed for each table with "acpi list". Would there be interest to have
> > that
> > upstream? If so I would send a patch.
>
> Can you elaborate what was the problem that checksum helped?
Sure. I saw two strange things with the ACPI checksums:
1) The DSDT length included uninitialized bytes from alignment. This is
described in the following link:
https://lists.denx.de/pipermail/u-boot/2020-September/425378.html
This was the actual bug I was looking for.
2) acpi_create_spcr() is missing a memset(). The other acpi_create_()
functions perform a memset on their structure, acpi_create_spcr() does not
and as a result the contents of this table are party uninitialized.
I plan to send a patch for both of them.
Regards, Wolfgang
Re: [PATCH v1] cmd: acpi: Print revisions in hex format
Hi Andy,
-"Andy Shevchenko" schrieb: -
> Betreff: [PATCH v1] cmd: acpi: Print revisions in hex format
>
> The revisions are usually dates in hex-decimal format representing
> mmdd. Print them in hex to see this clearly.
>
> Before:
> ...
> FACP 000e5420 f4 (v06 U-BOOT U-BOOTBL 538970376 INTL 0)
> DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 65536 INTL 538968870)
> ...
> After:
> ...
> FACP 000e5420 f4 (v06 U-BOOT U-BOOTBL 20200908 INTL 0)
> DSDT 000e4780 000ba0 (v02 U-BOOT U-BOOTBL 1 INTL 20200326)
> ...
>
> Fixes: 0b885bcfd9b0 ("acpi: Add an acpi command")
> Cc: Wolfgang Wallner
> Signed-off-by: Andy Shevchenko
> ---
> cmd/acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Wolfgang Wallner
Tested-by: Wolfgang Wallner
Tested on a custom Apollolake board.
Related to "acpi list":
During my recent ACPI debugging I found it very useful to have the checksum
printed for each table with "acpi list". Would there be interest to have that
upstream? If so I would send a patch.
regards, Wolfgang
Re: [PATCH v2 08/57] x86: acpi: Support external GNVS tables
Hi Simon, Bin, -"Simon Glass" schrieb: - > Betreff: [PATCH v2 08/57] x86: acpi: Support external GNVS tables > > At present U-Boot puts a magic number in the ASL for the GNVS table and > searches for it later. > > Add a Kconfig option to use a different approach, where the ASL files > declare the table as an external symbol. U-Boot can then put it wherever > it likes, without any magic numbers or searching. > > Signed-off-by: Simon Glass > --- [snip] > diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c [snip] > acpi_inc_align(ctx, dsdt->length - sizeof(struct acpi_table_header)); > + dsdt->length = ctx->current - (void *)dsdt; While testing the latest series of ACPI patches I saw strange behavior when booting Linux. Sometimes it would boot, sometimes it would hang, sometimes it would boot but show ACPI-related errors in dmesg. Debugging showed that the reason is that the calculated length of the DSDT in the above code includes any additional bytes that were added for alignment, and those are not initialized. I gues the Linux ACPI implementation tries to decode those, and then something crashes. Changing the above two lines to the following acpi_inc(ctx, dsdt->length - sizeof(struct acpi_table_header)); dsdt->length = ctx->current - (void *)dsdt; acpi_align(ctx); fixes the issue for me. But the issue already exists in the previous code. Would it be better to send a patch which applies to the currrent master, or on top of the ACPI series? regards, Wolfgang
Re: [PATCH v2 07/16] x86: zboot: Set up a sub-command structure
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v2 07/16] x86: zboot: Set up a sub-command structure > > Add subcommands to zboot. At present there is only one called 'start' > which does the whole boot. It is the default command so is optional. > > Change the 's' string variable to const while we are here. > Signed-off-by: Simon Glass > --- > > Changes in v2: > - Fix comment about argv[0] in do_zboot_parent() > > arch/x86/lib/zimage.c | 64 +++ > 1 file changed, 58 insertions(+), 6 deletions(-) Reviewed-by: Wolfgang Wallner
x86: apl: Acessing SPI flash when booting with Coreboot/U-Boot
Hi Simon,
Since commit 609b90a6a9c0 ("x86: spi: Rewrite logic for obtaining the SPI
memory map") I have trouble accessing the SPI flash on my Apollo Lake board
when booting with Coreboot and having U-Boot as the payload.
Accessing the SPI flash returns -91 (EPROTOTYPE).
My understanding of what happens is the following:
- ich_spi_ofdata_to_platdata() calls ich_spi_get_basics(), the parameter
'can_probe' is hardcoded to true
- There is no PCH device in my devicetree (there is also no PCH driver
contained in my build)
- As can_probe is true, ich_spi_get_basics() tries to find a PCH device
and returns -EPROTOTYPE as it finds none
As far as I see the PCH is not used in the code paths relevant to Apollo Lake.
I think this behavior can not be triggered in a standalone U-Boot on Apollo
Lake,
as in this case arch_cpu_init_tpl() requires an LPC, which is below the PCH,
and so a PCH has to be part of the devicetree anyway. In this case a PCH would
be found in ich_spi_get_basics(), but it would not get used.
As a quick workaround [1], I have set can_probe hardcoded to false, and with
this change I can access the SPI flash again.
Do you have any advice on how to fix this?
How about making the value for can_probe depend on a check for
ich_version == ICHV_APL?
regards, Wolfgang
[1] Workaround patch:
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -1016,7 +1016,7 @@ static int ich_spi_ofdata_to_platdata(struct udevice *dev)
#if !CONFIG_IS_ENABLED(OF_PLATDATA)
struct ich_spi_priv *priv = dev_get_priv(dev);
- ret = ich_spi_get_basics(dev, true, >pch, >ich_version,
+ ret = ich_spi_get_basics(dev, false, >pch, >ich_version,
[PATCH] x86: mtrr: Fix parsing of "mtrr list" command
The command 'mtrr' does not recognize the 'list' subcommand any more
since the code restructuring in commit b2a76b3fe75a ("x86: mtrr:
Restructure so command execution is in one place").
The if-else parsing the command arguments does not take 'list' into
account: the if-branch is intended for no subcommands, the else-branch
is intended for the non-list subcommands (which all expect additional
arguments). Calling the 'mtrr list' subcommand leads to a "return
CMD_RET_USAGE" in the else-branch.
Fix this by changing the else-branch to explicitly checking for
if (cmd != 'l').
Fixes: b2a76b3fe75a ("x86: mtrr: Restructure so command execution is in one
place")
Signed-off-by: Wolfgang Wallner
---
cmd/x86/mtrr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/x86/mtrr.c b/cmd/x86/mtrr.c
index e118bba5a2..99efecb9d8 100644
--- a/cmd/x86/mtrr.c
+++ b/cmd/x86/mtrr.c
@@ -121,7 +121,8 @@ static int do_mtrr(struct cmd_tbl *cmdtp, int flag, int
argc,
if (argc < 1 || !cmd) {
cmd = 'l';
reg = 0;
- } else {
+ }
+ if (cmd != 'l') {
if (argc < 2)
return CMD_RET_USAGE;
reg = simple_strtoul(argv[1], NULL, 16);
--
2.28.0
Re: [PATCH 16/16] cros: Add information about booting Chrome OS on x86
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 16/16] cros: Add information about booting Chrome OS on x86 > > Recent versions of Chrome OS do not have a kernel in the root disk, to > save space. > > With the improvements to the 'zboot' command it is fairly easy to load > the kernel from the raw partition. Add instructions on how to do this. > > Signed-off-by: Simon Glass > --- > > doc/README.chromium | 32 > 1 file changed, 32 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH 14/16] x86: zboot: Allow overriding the command line
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 14/16] x86: zboot: Allow overriding the command line > > When booting Chrome OS images the command line is stored separately > from the kernel. Add a way to specify this address so that images boot > correctly. > > Also add comments to the zimage.h header. > > Signed-off-by: Simon Glass > --- > > arch/x86/include/asm/zimage.h | 30 +- > arch/x86/lib/bootm.c | 2 +- > arch/x86/lib/zimage.c | 21 - > 3 files changed, 46 insertions(+), 7 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 15/16] cros: Update chromium documentation
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 15/16] cros: Update chromium documentation > > A few things have changed since this was written about 18 months ago. > Update the README. > > Signed-off-by: Simon Glass > --- > > doc/README.chromium | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 12/16] x86: zboot: Allow setting a separate setup base address
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH 12/16] x86: zboot: Allow setting a separate setup base address
>
> At present the setup block is always obtained from the image
> automatically. In some cases it can be useful to use a setup block
> obtained elsewhere, e.g. if the image has already been unpacked. Add an
> argument to support this and update the logic to use it if provided.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/lib/zimage.c | 28 ++--
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> index 4a0a69c8f7c..b64d0196f2d 100644
> --- a/arch/x86/lib/zimage.c
> +++ b/arch/x86/lib/zimage.c
> @@ -366,6 +366,11 @@ static int do_zboot_start(struct cmd_tbl *cmdtp, int
> flag, int argc,
> state.initrd_addr = simple_strtoul(argv[3], NULL, 16);
> if (argc >= 5)
> state.initrd_size = simple_strtoul(argv[4], NULL, 16);
> + if (argc >= 6) {
> + state.base_ptr = (void *)simple_strtoul(argv[5], NULL, 16);
> + state.load_address = state.bzimage_addr;
> + state.bzimage_addr = 0;
Nit: I found it rather confusing to understand what is going in the case when
this parameter is used compared to the 'normal' operation. Maybe you could add
comments here or in do_zboot_load() when the parameter is used?
> + }
>
> return 0;
> }
> @@ -375,11 +380,20 @@ static int do_zboot_load(struct cmd_tbl *cmdtp, int
> flag, int argc,
> {
> struct boot_params *base_ptr;
>
> - base_ptr = load_zimage((void *)state.bzimage_addr, state.bzimage_size,
> -_address);
> - if (!base_ptr) {
> - puts("## Kernel loading failed ...\n");
> - return CMD_RET_FAILURE;
> + if (state.base_ptr) {
> + struct boot_params *from = (struct boot_params *)state.base_ptr;
> +
> + base_ptr = (struct boot_params *)DEFAULT_SETUP_BASE;
> + printf("Building boot_params at 0x%8.8lx\n", (ulong)base_ptr);
> + memset(base_ptr, '\0', sizeof(*base_ptr));
> + base_ptr->hdr = from->hdr;
> + } else {
> + base_ptr = load_zimage((void *)state.bzimage_addr,
> state.bzimage_size,
> +_address);
> + if (!base_ptr) {
> + puts("## Kernel loading failed ...\n");
> + return CMD_RET_FAILURE;
> + }
> }
> state.base_ptr = base_ptr;
> if (env_set_hex("zbootbase", (ulong)base_ptr) ||
> @@ -486,7 +500,7 @@ int do_zboot_parent(struct cmd_tbl *cmdtp, int flag, int
> argc,
>
> U_BOOT_CMDREP_COMPLETE(
> zboot, 8, do_zboot_parent, "Boot bzImage",
> - "[addr] [size] [initrd addr] [initrd size]\n"
> + "[addr] [size] [initrd addr] [initrd size] [setup]\n"
> " addr -The optional starting address of the bzimage.\n"
> "If not set it defaults to the environment\n"
> "variable \"fileaddr\".\n"
> @@ -494,6 +508,8 @@ U_BOOT_CMDREP_COMPLETE(
> "zero.\n"
> " initrd addr - The address of the initrd image to use, if any.\n"
> " initrd size - The size of the initrd image to use, if any.\n"
> + " setup - The address of the kernel setup region, if this\n"
> + "is not at addr\n"
> "\n"
> "Sub-commands to do part of the zboot sequence:\n"
> "\tstart [addr [arg ...]] - specify arguments\n"
> --
> 2.28.0.163.g6104cc2f0b6-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH 11/16] x86: zboot: Set environment variables for image locations
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 11/16] x86: zboot: Set environment variables for image > locations > > At present it is not possible to tell from a script where the setup block > is, or where the image was loaded to. Add environment variables for this. > > Signed-off-by: Simon Glass > --- > > README| 4 > arch/x86/lib/zimage.c | 3 +++ > 2 files changed, 7 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH 13/16] x86: zboot: Add an option to dump the setup information
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 13/16] x86: zboot: Add an option to dump the setup information > > There is a lot of information in the setup block and it is quite hard to > decode manually. Add a 'zboot dump' command to decode it into a > human-readable format. > > Signed-off-by: Simon Glass > --- > > arch/x86/include/asm/e820.h | 1 + > arch/x86/lib/zimage.c | 199 +++- > 2 files changed, 199 insertions(+), 1 deletion(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 09/16] x86: zboot: Add an 'info' subcommand
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 09/16] x86: zboot: Add an 'info' subcommand > > Add a little subcommand that prints out where the kernel was loaded and > its setup pointer. Run it by default in the normal boot. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 17 ++--- > 1 file changed, 14 insertions(+), 3 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 10/16] x86: zboot: Add an 'setup' subcommand
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 10/16] x86: zboot: Add an 'setup' subcommand > > Add a subcommand that sets up the kernel ready for execution. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 52 ++- > 1 file changed, 42 insertions(+), 10 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 07/16] x86: zboot: Set up a sub-command structure
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH 07/16] x86: zboot: Set up a sub-command structure
>
> Add subcommands to zboot. At present there is only one called 'start'
> which does the whole boot. It is the default command so is optional.
>
> Change the 's' string variable to const while we are here.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/lib/zimage.c | 64 +++
> 1 file changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> index 8651dea93b3..3d3fe6ec05f 100644
> --- a/arch/x86/lib/zimage.c
> +++ b/arch/x86/lib/zimage.c
> @@ -63,6 +63,12 @@ struct zboot_state {
> ulong load_address;
> } state;
>
> +enum {
> + ZBOOT_STATE_START = BIT(0),
> +
> + ZBOOT_STATE_COUNT = 1,
> +};
> +
> static void build_command_line(char *command_line, int auto_boot)
> {
> char *env_command_line;
> @@ -328,10 +334,11 @@ int setup_zimage(struct boot_params *setup_base, char
> *cmd_line, int auto_boot,
> return 0;
> }
>
> -int do_zboot(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +static int do_zboot_start(struct cmd_tbl *cmdtp, int flag, int argc,
> + char *const argv[])
> {
> struct boot_params *base_ptr;
> - char *s;
> + const char *s;
>
> memset(, '\0', sizeof(state));
> if (argc >= 2) {
> @@ -373,9 +380,53 @@ int do_zboot(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
> return boot_linux_kernel((ulong)base_ptr, state.load_address, false);
> }
>
> -U_BOOT_CMD(
> - zboot, 5, 0,do_zboot,
> - "Boot bzImage",
> +U_BOOT_SUBCMDS(zboot,
> + U_BOOT_CMD_MKENT(start, 8, 1, do_zboot_start, "", ""),
> +)
> +
> +int do_zboot_states(struct cmd_tbl *cmdtp, int flag, int argc,
> + char *const argv[], int state_mask)
> +{
> + int i;
> +
> + for (i = 0; i < ZBOOT_STATE_COUNT; i++) {
> + struct cmd_tbl *cmd = _subcmds[i];
> + int mask = 1 << i;
> + int ret;
> +
> + if (mask & state_mask) {
> + ret = cmd->cmd(cmd, flag, argc, argv);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int do_zboot_parent(struct cmd_tbl *cmdtp, int flag, int argc,
> + char *const argv[], int *repeatable)
> +{
> + /* determine if we have a sub command */
> + if (argc > 1) {
> + char *endp;
> +
> + simple_strtoul(argv[1], , 16);
> + /*
> + * endp pointing to nul means that argv[0] was just a valid
Do you mean argv[1] in this comment?
Because that's what is checked with simple_strtoul.
> + * number, so pass it along to the normal processing
> + */
> + if (*endp)
> + return do_zboot(cmdtp, flag, argc, argv, repeatable);
> + }
> +
> + do_zboot_states(cmdtp, flag, argc, argv, ZBOOT_STATE_START);
> +
> + return CMD_RET_FAILURE;
> +}
> +
> +U_BOOT_CMDREP_COMPLETE(
> + zboot, 8, do_zboot_parent, "Boot bzImage",
> "[addr] [size] [initrd addr] [initrd size]\n"
> " addr -The optional starting address of the bzimage.\n"
> "If not set it defaults to the environment\n"
> @@ -383,5 +434,6 @@ U_BOOT_CMD(
> " size -The optional size of the bzimage. Defaults to\n"
> "zero.\n"
> " initrd addr - The address of the initrd image to use, if any.\n"
> - " initrd size - The size of the initrd image to use, if any.\n"
> + " initrd size - The size of the initrd image to use, if any.\n",
> + complete_zboot
> );
> --
> 2.28.0.163.g6104cc2f0b6-goog
regards, Wolfgang
Re: [PATCH 05/16] x86: zboot: Correct image type
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 05/16] x86: zboot: Correct image type > > At present U-Boot sets a loader type of 8 which means LILO version 8, > according to the spec. Update it to 0x80, which means U-Boot with no > particular version. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 08/16] x86: zboot: Add a 'go' subcommand
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 08/16] x86: zboot: Add a 'go' subcommand > > Split out the code that actually boots linux into a separate sub-command. > Add base_ptr to the state to support this. > > Show an error if the boot fails, since this should not happen. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 26 +++--- > 1 file changed, 23 insertions(+), 3 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 06/16] x86: zimage: Disable interrupts just before booting
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 06/16] x86: zimage: Disable interrupts just before booting > > At present if an error occurs while setting up the boot, interrupts are > left disabled. Move this call later in the sequence to avoid this problem. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 04/16] x86: zboot: Move kernel-version code into a function
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 04/16] x86: zboot: Move kernel-version code into a function > > To help reduce the size and complexity of load_zimage(), move the code > that reads the kernel version into a separate function. Update > get_boot_protocol() to allow printing the 'Magic signature' message only > once, under control of its callers. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 43 +++ > 1 file changed, 27 insertions(+), 16 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 02/16] x86: zimage: Use a state struct to hold the state
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 02/16] x86: zimage: Use a state struct to hold the state > > At present the 'zboot' command does everything in one go. It would be > better if it supported sub-commands like bootm, so it is possible to > examine what will be booted before actually booting it. > > In preparation for this, move the 'state' of the command into a struct. > This will allow it to be shared among multiple functions in this file. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 44 --- > 1 file changed, 29 insertions(+), 15 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 01/16] x86: Update the bootparam header
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 01/16] x86: Update the bootparam header > > This header is missing a few of the newer features from the specification. > Add these as well as a link to the spec. Also use the BIT() macros where > appropriate. > > Signed-off-by: Simon Glass > --- > > arch/x86/include/asm/bootparam.h | 25 +++-- > 1 file changed, 19 insertions(+), 6 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 03/16] x86: zimage: Avoid using #ifdef
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 03/16] x86: zimage: Avoid using #ifdef > > Use IS_ENABLED() instead of #ifdef in this file. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/zimage.c | 14 +- > 1 file changed, 5 insertions(+), 9 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH 00/16] x86: zboot: Enhance the 'zboot' command
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH 00/16] x86: zboot: Enhance the 'zboot' command > > This command is currently monolithic and does not support scripts which > want to adjust the boot process. This series updates it to be more like > 'bootm', in that it has sub-commands for each stage of the boot. This > allows some stages to be adjusted or skipped. > > It also adds a way to dump out the setup block. That series is really appreciated, especially 'zboot dump' :) regards, Wolfgang
Re: [PATCH] acpi: device: Fix check for sequence number
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: Re: [PATCH] acpi: device: Fix check for sequence number
>
> Hi Wolfgang,
>
> On Thu, 30 Jul 2020 at 06:47, Wolfgang Wallner
> wrote:
> >
> > Currently the function acpi_check_seq() checks whether dev->req_seq is
> > unequal to "-1", but it should actually check dev->seq. Change it to
> > check dev->seq.
> >
> > For req_seq the value "-1" would be a valid (meaning 'any'), while for
> > seq the value "-1" means 'none' and is not valid.
> >
> > Quoting the description of udevice in device.h:
> > * @req_seq: Requested sequence number for this device (-1 = any)
> > * @seq: Allocated sequence number for this device (-1 = none).
> > * This is set up when the device is probed and will be unique
> > * within the device's uclass.
> >
> > Signed-off-by: Wolfgang Wallner
> >
> > Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()")
> >
> > ---
> >
> > lib/acpi/acpi_device.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> What problem are you seeing without this patch?
I see the following warning: "Device 'serial@18,2' has no seq".
In my case req_seq for the UART is -1 ("any"), while seq is 0.
Why would we check for req_seq and not for seq in this function?
> At present the ACPI device may not always be probed, and probing is
> when the sequence number is currently set up.
In my case the UART is already probed before the ACPI tables are generated.
>
> I have been thinking about dropping req_seq and doing everything when
> the device is bound, but haven't dug into it in detail yet.
regards, Wolfgang
Re: chromebook_coral: build failure
Hi Simon, -"Simon Glass" schrieb: - > Betreff: Re: chromebook_coral: build failure > > Hi Wolfgang, > > On Fri, 31 Jul 2020 at 05:44, Wolfgang Wallner > wrote: > > > > Hi Simon, > > > > while trying out your ACPI patches I tried to compile > chromebook_coral_defconfig > > as a reference. Building this defconfig fails for multiple > definitions > > of "_X86EMU_env": > > > > $ git checkout v2020.07 > > $ make distclean > > $ make chromebook_coral_defconfig > > $ make > > > > [...] > > LDS u-boot.lds > > LD u-boot > > ld.bfd: drivers/built-in.o:(.bss._X86EMU_env+0x0): multiple > definition of `_X86EMU_env'; > arch/x86/lib/built-in.o:(.bss._X86EMU_env+0x0): first defined here > > make: *** [Makefile:1755: u-boot] Error 1 > > > > A quick and dirty workaround was to drop "#define CONFIG_BIOSEMU" > from > > include/configs/x86-chromebook.h. With this change > chromebook_coral_defconfig > > compiles fine. > > > > Do you know what is going on here, and what might be a proper fix? > > I don't see that problem myself, [...] It seems to be triggered by a change of default flags in GCC 10. I can reliably reproduce the build error on my main machine (Arch Linux with GCC 10), but it builds fine on a Debian 10 with GCC 8. A colleague told me about "-fno-common", which was changed to be on by default in GCC 10: https://gcc.gnu.org/gcc-10/porting_to.html I can build chromebook_coral_defconfig on my Arch with GCC 10 with: make KCFLAGS="-fcommon" However, I'm not sure what the initial problem is, as I don't know the relevant code parts. Both of the following files declare a global variable "_X86EMU_env": drivers/bios_emulator/x86emu/sys.c arch/x86/lib/bios.c Should they both be part of the build for Chromebook Coral? > [...] but I do see an ACPI compile problem > now. that I rebase. I have added a patch for that and pushed the tree > to u-boot-dm/coral-working Yes, this is the reason why I wanted to compile chromebook_coral_defconfig as a reference :) I saw the ACPI compile problem when I tried your ACPI patches on my APL board and wanted to see whether I had broken something. Could you please move the fix to the beginning of the part D series when you send V2? It might make testing individual patches of that series easier if the fix is at the beginning. regards, Wolfgang
chromebook_coral: build failure
Hi Simon, while trying out your ACPI patches I tried to compile chromebook_coral_defconfig as a reference. Building this defconfig fails for multiple definitions of "_X86EMU_env": $ git checkout v2020.07 $ make distclean $ make chromebook_coral_defconfig $ make [...] LDS u-boot.lds LD u-boot ld.bfd: drivers/built-in.o:(.bss._X86EMU_env+0x0): multiple definition of `_X86EMU_env'; arch/x86/lib/built-in.o:(.bss._X86EMU_env+0x0): first defined here make: *** [Makefile:1755: u-boot] Error 1 A quick and dirty workaround was to drop "#define CONFIG_BIOSEMU" from include/configs/x86-chromebook.h. With this change chromebook_coral_defconfig compiles fine. Do you know what is going on here, and what might be a proper fix? regards, Wolfgang
[PATCH] acpi: device: Fix check for sequence number
Currently the function acpi_check_seq() checks whether dev->req_seq is
unequal to "-1", but it should actually check dev->seq. Change it to
check dev->seq.
For req_seq the value "-1" would be a valid (meaning 'any'), while for
seq the value "-1" means 'none' and is not valid.
Quoting the description of udevice in device.h:
* @req_seq: Requested sequence number for this device (-1 = any)
* @seq: Allocated sequence number for this device (-1 = none).
* This is set up when the device is probed and will be unique
* within the device's uclass.
Signed-off-by: Wolfgang Wallner
Fixes: commit fefac0b0643b ("dm: acpi: Enhance acpi_get_name()")
---
lib/acpi/acpi_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/acpi/acpi_device.c b/lib/acpi/acpi_device.c
index 3c75b6d962..76194ea1b4 100644
--- a/lib/acpi/acpi_device.c
+++ b/lib/acpi/acpi_device.c
@@ -743,12 +743,12 @@ static const char *acpi_name_from_id(enum uclass_id id)
static int acpi_check_seq(const struct udevice *dev)
{
- if (dev->req_seq == -1) {
+ if (dev->seq == -1) {
log_warning("Device '%s' has no seq\n", dev->name);
return log_msg_ret("no seq", -ENXIO);
}
- return dev->req_seq;
+ return dev->seq;
}
/* If you change this function, add test cases to dm_test_acpi_get_name() */
--
2.27.0
[PATCH] x86: irq: Fix some typos
Fix some typos in arch/x86/include/asm/irq.h.
Signed-off-by: Wolfgang Wallner
---
arch/x86/include/asm/irq.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h
index e5c916070c..bee0760c2d 100644
--- a/arch/x86/include/asm/irq.h
+++ b/arch/x86/include/asm/irq.h
@@ -12,8 +12,8 @@
* Intel interrupt router configuration mechanism
*
* There are two known ways of Intel interrupt router configuration mechanism
- * so far. On most cases, the IRQ routing configuraiton is controlled by PCI
- * configuraiton registers on the legacy bridge, normally PCI BDF(0, 31, 0).
+ * so far. On most cases, the IRQ routing configuration is controlled by PCI
+ * configuration registers on the legacy bridge, normally PCI BDF(0, 31, 0).
* On some newer platforms like BayTrail and Braswell, the IRQ routing is now
* in the IBASE register block where IBASE is memory-mapped.
*/
@@ -36,7 +36,7 @@ struct pirq_regmap {
* @link_base: link value base number
* @link_num: number of PIRQ links supported
* @has_regmap:has mapping table between PIRQ link and routing
register offset
- * @irq_mask: IRQ mask reprenting the 16 IRQs in 8259, bit N is 1 means
+ * @irq_mask: IRQ mask representing the 16 IRQs in 8259, bit N is 1 means
* IRQ N is available to be routed
* @lb_bdf:irq router's PCI bus/device/function number encoding
* @ibase: IBASE register block base address
--
2.27.0
Re: [PATCH v2 38/44] x86: Store the coreboot table address in global_data
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v2 38/44] x86: Store the coreboot table address in global_data
>
> At present this information is used to locate and parse the tables but is
> not stored. Store it so that we can display it to the user, e.g. with the
> 'bdinfo' command.
>
> Signed-off-by: Simon Glass
> Reviewed-by: Bin Meng
> ---
>
> (no changes since v1)
>
> arch/x86/cpu/coreboot/tables.c | 8 +++-
> arch/x86/cpu/i386/cpu.c| 7 ++-
> arch/x86/include/asm/global_data.h | 1 +
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/cpu/coreboot/tables.c b/arch/x86/cpu/coreboot/tables.c
> index a5d31d1dea..1594b4a8b2 100644
> --- a/arch/x86/cpu/coreboot/tables.c
> +++ b/arch/x86/cpu/coreboot/tables.c
> @@ -10,6 +10,8 @@
> #include
> #include
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> /*
> * This needs to be in the .data section so that it's copied over during
> * relocation. By default it's put in the .bss section which is simply filled
> @@ -243,6 +245,10 @@ int get_coreboot_info(struct sysinfo_t *info)
> if (addr < 0)
> return addr;
> ret = cb_parse_header((void *)addr, 0x1000, info);
> + if (!ret)
> + return -ENOENT;
> + gd->arch.coreboot_table = addr;
> + gd->flags |= GD_FLG_SKIP_LL_INIT;
Nit: Could you add to the commit message that GD_FLG_SKIP_LL_INIT is handled
differently after this commit? Currently it only describes storing the
coreboot table address.
>
> - return ret == 1 ? 0 : -ENOENT;
> + return 0;
> }
> diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> index fca3f79b69..8f342dd06e 100644
> --- a/arch/x86/cpu/i386/cpu.c
> +++ b/arch/x86/cpu/i386/cpu.c
> @@ -456,10 +456,15 @@ int x86_cpu_init_f(void)
>
> int x86_cpu_reinit_f(void)
> {
> + long addr;
> +
> setup_identity();
> setup_pci_ram_top();
> - if (locate_coreboot_table() >= 0)
> + addr = locate_coreboot_table();
> + if (addr >= 0) {
> + gd->arch.coreboot_table = addr;
> gd->flags |= GD_FLG_SKIP_LL_INIT;
> + }
>
> return 0;
> }
> diff --git a/arch/x86/include/asm/global_data.h
> b/arch/x86/include/asm/global_data.h
> index 5bc251c0dd..3e4044593c 100644
> --- a/arch/x86/include/asm/global_data.h
> +++ b/arch/x86/include/asm/global_data.h
> @@ -123,6 +123,7 @@ struct arch_global_data {
> #endif
> void *itss_priv;/* Private ITSS data pointer */
> ulong acpi_start; /* Start address of ACPI tables */
> + ulong coreboot_table; /* Address of coreboot table */
> };
>
> #endif
> --
> 2.27.0.383.g050319c2ae-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v2 28/44] i2c: designware_i2c: Support ACPI table generation
struct dw_i2c_speed_config *config)
> +{
> + switch (config->speed_mode) {
> + case IC_SPEED_MODE_HIGH:
> + acpigen_write_name(ctx, "HSCN");
> + break;
> + case IC_SPEED_MODE_FAST_PLUS:
> + acpigen_write_name(ctx, "FPCN");
> + break;
> + case IC_SPEED_MODE_FAST:
> + acpigen_write_name(ctx, "FMCN");
> + break;
> + case IC_SPEED_MODE_STANDARD:
> + default:
> + acpigen_write_name(ctx, "SSCN");
> + }
> +
> + /* Package () { scl_lcnt, scl_hcnt, sda_hold } */
> + acpigen_write_package(ctx, 3);
> + acpigen_write_word(ctx, config->scl_hcnt);
> + acpigen_write_word(ctx, config->scl_lcnt);
> + acpigen_write_dword(ctx, config->sda_hold);
> + acpigen_pop_len(ctx);
> +}
> +
> +/*
> + * Generate I2C timing information into the SSDT for the OS driver to
> consume,
> + * optionally applying override values provided by the caller.
> + */
> +static int dw_i2c_acpi_fill_ssdt(const struct udevice *dev,
> + struct acpi_ctx *ctx)
> +{
> + struct dw_i2c_speed_config config;
> + char path[ACPI_PATH_MAX];
> + u32 speeds[4];
> + uint speed;
> + int size;
> + int ret;
> +
> + /* If no device-tree node, ignore this since we assume it isn't used */
> + if (!dev_of_valid(dev))
> + return 0;
> +
> + ret = acpi_device_path(dev, path, sizeof(path));
> + if (ret)
> + return log_msg_ret("path", ret);
> +
> + size = dev_read_size(dev, "i2c,speeds");
> + if (size < 0)
> + return log_msg_ret("i2c,speeds", -EINVAL);
> +
> + size /= sizeof(u32);
> + if (size > ARRAY_SIZE(speeds))
> + return log_msg_ret("array", -E2BIG);
> +
> + ret = dev_read_u32_array(dev, "i2c,speeds", speeds, size);
> + if (ret)
> + return log_msg_ret("read", -E2BIG);
> +
> + speed = dev_read_u32_default(dev, "clock-frequency", 10);
> + acpigen_write_scope(ctx, path);
> + ret = dw_i2c_gen_speed_config(dev, speed, );
> + if (ret)
> + return log_msg_ret("config", ret);
> + dw_i2c_acpi_write_speed_config(ctx, );
> + acpigen_pop_len(ctx);
> +
> + return 0;
> +}
> +
> +struct acpi_ops dw_i2c_acpi_ops = {
> + .fill_ssdt = dw_i2c_acpi_fill_ssdt,
> +};
> +
> static const struct udevice_id designware_i2c_pci_ids[] = {
> { .compatible = "snps,designware-i2c-pci" },
> { .compatible = "intel,apl-i2c", .data = INTEL_APL },
> @@ -124,6 +217,7 @@ U_BOOT_DRIVER(i2c_designware_pci) = {
> .remove = designware_i2c_remove,
> .flags = DM_FLAG_OS_PREPARE,
> .ops= _i2c_ops,
> + ACPI_OPS_PTR(_i2c_acpi_ops)
> };
>
> static struct pci_device_id designware_pci_supported[] = {
> --
> 2.27.0.383.g050319c2ae-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v2 21/44] x86: pinctrl: Set up itss in the probe() method
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v2 21/44] x86: pinctrl: Set up itss in the probe() method > > At present the itss is probed in the ofdata_to_platdata() method. This is > incorrect since itss is a child of p2sb which itself needs to probe the > pinctrl device. This means that p2sb is effectively not probed when the > itss is probed, so we get the wrong register address from p2sb. > > Fix this by moving the itss probe to the correct place. > > Signed-off-by: Simon Glass > Reviewed-by: Bin Meng > --- > > (no changes since v1) > > drivers/pinctrl/intel/pinctrl.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v2 34/44] x86: apl: Fix save/restore of ITSS priorities
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v2 34/44] x86: apl: Fix save/restore of ITSS priorities > > The FSP-S changes the ITSS priorities. The code that tries to save it > before running FSP-S and restore it afterwards does not work as U-Boot > relocates in between the save and restore. This means that the driver > data saved before relocation is lost and the new driver just sees zeroes. > > Fix this by allocating space in the relocated memory for the ITSS data. > Save it there and access it from the driver after relocation. > > This fixes interrupt handling on coral. > > Also drop the log_msg_ret() in irq_first_device_type() since this function > can be called speculatively in places where we are not sure if there is > an interrupt controller of that type. The resulting log errors are > confusing when there is no error. > > Signed-off-by: Simon Glass > --- > > Changes in v2: > - Add mention of why log_msg_ret() is dropped > > arch/x86/cpu/apollolake/fsp_s.c| 11 +-- > arch/x86/cpu/cpu.c | 13 + > arch/x86/cpu/intel_common/itss.c | 25 +++-- > arch/x86/include/asm/global_data.h | 1 + > arch/x86/include/asm/itss.h| 2 +- > drivers/misc/irq-uclass.c | 2 +- > 6 files changed, 44 insertions(+), 10 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: Re: [PATCH v1 22/43] x86: Add support for building up an NHLT
> structure
>
> Hi Wolfgang,
>
> On Thu, 2 Jul 2020 at 02:11, Wolfgang Wallner
> wrote:
> >
> > Hi Simon,
> >
> > I dont know NHLT well enough to actually review the code, but I did compare
> > the files in this patch to the version in coreboot. Most of the changes are
> > obvious (coding style, spelling, ...), but some things are not, see the
> > remarks
> > below.
> >
> > -"Simon Glass" schrieb: -
> > > Betreff: [PATCH v1 22/43] x86: Add support for building up an NHLT
> > > structure
> > >
> > > The Intel Non-High-Definition-Audio Link Table (NHLT) table describes the
> > > audio codecs and connections in a system. Various devices can contribute
> > > information to produce the table.
> > >
> > > Add functions to allow adding to the structure that is eventually written
> > > to the ACPI tables. Also add the device-tree bindings.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v1:
> > > - Add a new patch to support building up an NHLT structure
> > >
> > > arch/x86/include/asm/acpi_nhlt.h | 314
> > > arch/x86/lib/Makefile| 1 +
> > > arch/x86/lib/acpi_nhlt.c | 482 +++
> > > 3 files changed, 797 insertions(+)
> > > create mode 100644 arch/x86/include/asm/acpi_nhlt.h
> > > create mode 100644 arch/x86/lib/acpi_nhlt.c
> > >
> > > diff --git a/arch/x86/include/asm/acpi_nhlt.h
> > > b/arch/x86/include/asm/acpi_nhlt.h
> > > new file mode 100644
> > > index 00..4d2573d5ff
> > > --- /dev/null
> > > +++ b/arch/x86/include/asm/acpi_nhlt.h
[snip]
> > > +
> > > +/*
> > > + * Serialize NHLT object to ACPI table. Take in the beginning address of
> > > where
> > > + * the table will reside and return the address of the next ACPI table.
> > > On
> > > + * error 0 will be returned. The NHLT object is no longer valid after
> > > this
> > > + * function is called.
> > > + */
> > > +uintptr_t nhlt_serialise(struct nhlt *nhlt, uintptr_t acpi_addr);
> >
> > The implementation for this functions seems to be missing in this patch.
> > In coreboot it looks like this:
> >
> > uintptr_t nhlt_serialize(struct nhlt *nhlt, uintptr_t acpi_addr)
> > {
> > return nhlt_serialize_oem_overrides(nhlt, acpi_addr, NULL, NULL, 0);
> > }
>
> Yes we don't need this at present. If we do we would likely put this
> in the device tree. Since coreboot doesn't have a proper device tree
> it uses code to call override functions to get the data, which IMO
> makes it quite hard to work out the config for a particular board
If we don't need the implementation of nhlt_serialise(), shouldn't we then
also drop its declaration in the header file?
>
> [..]
>
regards, Wolfgang
Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info for a GPIO
Hi Simon, Bin,
-"Simon Glass" schrieb: -
> Betreff: Re: [PATCH v1 25/43] x86: gpio: Add support for obtaining ACPI info
> for a GPIO
>
> Hi Bin,
>
> On Tue, 30 Jun 2020 at 01:47, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Mon, Jun 15, 2020 at 11:58 AM Simon Glass wrote:
> > >
> > > Implement the method that converts a GPIO into the form used by ACPI, so
> > > that GPIOs can be added to ACPI tables.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > Changes in v1:
> > > - Use acpi_get_path() to get device path
> > >
> > > drivers/gpio/intel_gpio.c | 34 ++
> > > 1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/gpio/intel_gpio.c b/drivers/gpio/intel_gpio.c
> > > index b4d5be97da..6a3a8c4cfa 100644
> > > --- a/drivers/gpio/intel_gpio.c
> > > +++ b/drivers/gpio/intel_gpio.c
> > > @@ -12,6 +12,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -19,6 +20,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > >
> > > static int intel_gpio_direction_input(struct udevice *dev, uint offset)
> > > @@ -128,6 +130,35 @@ static int intel_gpio_xlate(struct udevice
> > > *orig_dev, struct gpio_desc *desc,
> > > return 0;
> > > }
> > >
> > > +#if CONFIG_IS_ENABLED(ACPIGEN)
> > > +static int intel_gpio_get_acpi(const struct gpio_desc *desc,
> > > + struct acpi_gpio *gpio)
> > > +{
> > > + struct udevice *pinctrl;
> > > + int ret;
> > > +
> > > + if (!dm_gpio_is_valid(desc))
> > > + return -ENOENT;
> > > + pinctrl = dev_get_parent(desc->dev);
> > > +
> > > + memset(gpio, '\0', sizeof(*gpio));
> > > +
> > > + gpio->type = ACPI_GPIO_TYPE_IO;
> > > + gpio->pull = ACPI_GPIO_PULL_DEFAULT;
> > > + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_OUTPUT;
> > > + gpio->polarity = ACPI_GPIO_ACTIVE_HIGH;
> >
> > Is there a way to figure out these properties from DT, instead of
> > hardcoding?
>
> The answer is similar to your previous comment. But also, each pinctrl
> driver has its own settings and limitations. If we want to support
> different config for the pinctrl we would likely add it to the DT
> binding for the pinctrl driver. So far I haven't seen a need but it
> might happen with a future arch.
Could gpio->pull and gpio->polarity be set depending on desc->flags?
What is the reason for setting gpio->io_restrict to
ACPI_GPIO_IO_RESTRICT_OUTPUT?
> [..]
regards, Wolfgang
Re: [PATCH v2 20/44] x86: pinctrl: Add multi-ACPI control
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v2 20/44] x86: pinctrl: Add multi-ACPI control > > Add a Kconfig to control whether pinctrl is represented as a single ACPI > device or as multiple devices. In the latter case (the default) we should > return the pin number relative to the pinctrl device. > > Signed-off-by: Simon Glass > Reviewed-by: Bin Meng > Reviewed-by: Wolfgang Wallner > --- > > Changes in v2: > - Add help for CONFIG_INTEL_PINCTRL_MULTI_ACPI_DEVICES Thanks for adding the help text. There is a typo though, see below. > > drivers/pinctrl/intel/Kconfig | 12 > drivers/pinctrl/intel/pinctrl.c | 2 ++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig > index e62a2e0349..1acc5dabb0 100644 > --- a/drivers/pinctrl/intel/Kconfig > +++ b/drivers/pinctrl/intel/Kconfig > @@ -15,6 +15,18 @@ config INTEL_PINCTRL_IOSTANDBY > bool > default y > > +config INTEL_PINCTRL_MULTI_ACPI_DEVICES > + bool > + default y > + help > + Enable this if the pinctrl devices are modelled as multiple, > + separate ACPI devices in the ACPI tables. If enabled, the ACPI > + devices match the U-Boot pinctrl devices and the pin 'offset' is > + relatove to a particular pinctrl device. If disabled, there is a Typo: relative > + single ACPI pinctrl device which includes all U-Boot pinctrl devices > + and the pin 'offset' is in effect a global pin number. > + > + > config PINCTRL_INTEL_APL > bool "Support Intel Apollo Lake (APL)" > help > diff --git a/drivers/pinctrl/intel/pinctrl.c b/drivers/pinctrl/intel/pinctrl.c > index bf3989bf32..32ca303b27 100644 > --- a/drivers/pinctrl/intel/pinctrl.c > +++ b/drivers/pinctrl/intel/pinctrl.c > @@ -427,6 +427,8 @@ int intel_pinctrl_get_acpi_pin(struct udevice *dev, uint > offset) > const struct pad_community *comm = priv->comm; > int group; > > + if (IS_ENABLED(CONFIG_INTEL_PINCTRL_MULTI_ACPI_DEVICES)) > + return offset; > group = pinctrl_group_index(comm, offset); > > /* If pad base is not set then use GPIO number as ACPI pin number */ > -- > 2.27.0.383.g050319c2ae-goog regards, Wolfgang
Re: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
Hi Simon,
I dont know NHLT well enough to actually review the code, but I did compare
the files in this patch to the version in coreboot. Most of the changes are
obvious (coding style, spelling, ...), but some things are not, see the remarks
below.
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
>
> The Intel Non-High-Definition-Audio Link Table (NHLT) table describes the
> audio codecs and connections in a system. Various devices can contribute
> information to produce the table.
>
> Add functions to allow adding to the structure that is eventually written
> to the ACPI tables. Also add the device-tree bindings.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v1:
> - Add a new patch to support building up an NHLT structure
>
> arch/x86/include/asm/acpi_nhlt.h | 314
> arch/x86/lib/Makefile| 1 +
> arch/x86/lib/acpi_nhlt.c | 482 +++
> 3 files changed, 797 insertions(+)
> create mode 100644 arch/x86/include/asm/acpi_nhlt.h
> create mode 100644 arch/x86/lib/acpi_nhlt.c
>
> diff --git a/arch/x86/include/asm/acpi_nhlt.h
> b/arch/x86/include/asm/acpi_nhlt.h
> new file mode 100644
> index 00..4d2573d5ff
> --- /dev/null
> +++ b/arch/x86/include/asm/acpi_nhlt.h
> @@ -0,0 +1,314 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2020 Google LLC
> + *
> + * Modified from coreboot nhlt.h
> + */
> +
> +#ifndef _NHLT_H_
> +#define _NHLT_H_
> +
> +struct acpi_ctx;
> +struct nhlt;
> +struct nhlt_endpoint;
> +struct nhlt_format;
> +struct nhlt_format_config;
> +
> +/*
> + * Non HD Audio ACPI support. This table is typically used for Intel Smart
> + * Sound Technology DSP. It provides a way to encode opaque settings in
> + * the ACPI tables.
> + *
> + * While the structure fields of the NHLT structs are exposed below
> + * the SoC/chipset code should be the only other user manipulating the
> + * fields directly aside from the library itself.
> + *
> + * The NHLT table consists of endpoints which in turn contain different
> + * supporting stream formats. Each endpoint may contain a device specific
> + * configuration payload as well as each stream format.
> + *
> + * Most code should use the SoC variants of the functions because
> + * there is required logic needed to be performed by the SoC. The SoC
> + * code should be abstracting the inner details of these functions that
> + * specically apply to NHLT objects for that SoC.
> + *
> + * An example sequence:
> + *
> + * nhlt = nhlt_init()
> + * ep = nhlt_add_endpoint()
> + * nhlt_endpoint_append_config(ep)
> + * nhlt_endpoint_add_formats(ep)
> + * nhlt_soc_serialise()
> + */
> +
> +/* Obtain an nhlt object for adding endpoints. Returns NULL on error. */
> +struct nhlt *nhlt_init(void);
> +
> +/* Return the size of the NHLT table including ACPI header. */
> +size_t nhlt_current_size(struct nhlt *nhlt);
> +
> +/*
> + * Helper functions for adding NHLT devices utilizing an nhlt_endp_descriptor
> + * to drive the logic.
> + */
> +
> +struct nhlt_endp_descriptor {
> + /* NHLT endpoint types. */
> + int link;
> + int device;
> + int direction;
> + u16 vid;
> + u16 did;
> + /* Optional endpoint specific configuration data. */
> + const void *cfg;
> + size_t cfg_size;
> + /* Formats supported for endpoint. */
> + const struct nhlt_format_config *formats;
> + size_t num_formats;
> +};
> +
> +/*
> + * Add the number of endpoints described by each descriptor. The virtual bus
> + * id for each descriptor is the default value of 0.
> + * Returns < 0 on error, 0 on success.
> + */
> +int nhlt_add_endpoints(struct nhlt *nhlt,
> +const struct nhlt_endp_descriptor *epds,
> +size_t num_epds);
> +
> +/*
> + * Add the number of endpoints associated with a single NHLT SSP instance id.
> + * Each endpoint described in the endpoint descriptor array uses the provided
> + * virtual bus id. Returns < 0 on error, 0 on success.
> + */
> +int nhlt_add_ssp_endpoints(struct nhlt *nhlt, int virtual_bus_id,
> +const struct nhlt_endp_descriptor *epds,
> +size_t num_epds);
> +
> +/*
> + * Add endpoint to NHLT object. Returns NULL on error.
> + *
> + * generic nhlt_add_endpoint() is called by the SoC code to provide
> + * the specific assumptions/uses for NHLT for that platform. All fields
> + * are the NHLT enumerations found within this header file.
> + */
> +struct nhlt_endpoint *nhlt_add_endpoint(struct nhlt *nhlt, int link_type,
> + int device_type, int dir,
> + u16 vid, u16 did);
> +
> +/*
> + * Append blob of configuration to the endpoint proper. Returns 0 on
> + * success, < 0 on error. A copy of the configuration is made so any
> + * resources pointed to by config can be freed after the call.
> + */
> +int
Re: [PATCH v1 19/43] x86: pinctrl: Add multi-ACPI control
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 19/43] x86: pinctrl: Add multi-ACPI control > > Add a Kconfig to control whether pinctrl is represented as a single ACPI > device or as multiple devices. In the latter case (the default) we should > return the pin number relative to the pinctrl device. > > Signed-off-by: Simon Glass > --- > > drivers/pinctrl/intel/Kconfig | 4 > drivers/pinctrl/intel/pinctrl.c | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/pinctrl/intel/Kconfig b/drivers/pinctrl/intel/Kconfig > index e62a2e0349..05a314af88 100644 > --- a/drivers/pinctrl/intel/Kconfig > +++ b/drivers/pinctrl/intel/Kconfig > @@ -15,6 +15,10 @@ config INTEL_PINCTRL_IOSTANDBY > bool > default y > > +config INTEL_PINCTRL_MULTI_ACPI_DEVICES > + bool > + default y Nit: A description of what this option does would be helpful > + > config PINCTRL_INTEL_APL > bool "Support Intel Apollo Lake (APL)" > help > diff --git a/drivers/pinctrl/intel/pinctrl.c b/drivers/pinctrl/intel/pinctrl.c > index bf3989bf32..32ca303b27 100644 > --- a/drivers/pinctrl/intel/pinctrl.c > +++ b/drivers/pinctrl/intel/pinctrl.c > @@ -427,6 +427,8 @@ int intel_pinctrl_get_acpi_pin(struct udevice *dev, uint > offset) > const struct pad_community *comm = priv->comm; > int group; > > + if (IS_ENABLED(CONFIG_INTEL_PINCTRL_MULTI_ACPI_DEVICES)) > + return offset; > group = pinctrl_group_index(comm, offset); > > /* If pad base is not set then use GPIO number as ACPI pin number */ > -- > 2.27.0.290.gba653c62da-goog Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 23/43] x86: Add error checking for csrt table generation
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 23/43] x86: Add error checking for csrt table generation > > Generation of this table can fail, so update the function to return an > error code. > > Signed-off-by: Simon Glass > --- > > Changes in v1: > - Add new patch to add error checking for csrt table generation > > arch/x86/lib/acpi_table.c | 19 +-- > 1 file changed, 13 insertions(+), 6 deletions(-) Reviewed-by: Wolfgang Wallner
[PATCH v3 0/2] x86: p2sb: P2SB fixes
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. While at it, correct the meaning of P2SB (according to Intel's documentation P2SB stands for "Primary to Sideband Bridge"). Remark: I have resent this series as V2 as I had messed up the cover letter in V1. Changes in v3: - Replaced the term in two more places Changes in v2: - Fixed cover letter Wolfgang Wallner (2): drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge x86: p2sb: make P2SB driver depend on P2SB uclass arch/x86/Kconfig | 1 + drivers/misc/Kconfig | 12 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) -- 2.27.0
[PATCH v2 0/2] x86: p2sb: P2SB fixes
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. While at it, correct the meaning of P2SB (according to Intel's documentation P2SB stands for "Primary to Sideband Bridge"). Remark: I have resent this series as V2 as I had messed up the cover letter in V1. Changes in v3: - Replaced the term in two more places Changes in v2: - Fixed cover letter Wolfgang Wallner (2): drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge x86: p2sb: make P2SB driver depend on P2SB uclass arch/x86/Kconfig | 1 + drivers/misc/Kconfig | 12 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) -- 2.27.0
Re: [PATCH v1 37/43] x86: Store the coreboot table address in global_data
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 37/43] x86: Store the coreboot table address in global_data
>
> At present this information is used to locate and parse the tables but is
> not stored. Store it so that we can display it to the user, e.g. with the
> 'bdinfo' command.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/cpu/coreboot/tables.c | 8 +++-
> arch/x86/cpu/i386/cpu.c| 7 ++-
> arch/x86/include/asm/global_data.h | 1 +
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/cpu/coreboot/tables.c b/arch/x86/cpu/coreboot/tables.c
> index a5d31d1dea..1594b4a8b2 100644
> --- a/arch/x86/cpu/coreboot/tables.c
> +++ b/arch/x86/cpu/coreboot/tables.c
> @@ -10,6 +10,8 @@
> #include
> #include
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> /*
> * This needs to be in the .data section so that it's copied over during
> * relocation. By default it's put in the .bss section which is simply filled
> @@ -243,6 +245,10 @@ int get_coreboot_info(struct sysinfo_t *info)
> if (addr < 0)
> return addr;
> ret = cb_parse_header((void *)addr, 0x1000, info);
> + if (!ret)
> + return -ENOENT;
> + gd->arch.coreboot_table = addr;
> + gd->flags |= GD_FLG_SKIP_LL_INIT;
Why is GD_FLG_SKIP_LL_INIT now set in get_coreboot_info()?
>
> - return ret == 1 ? 0 : -ENOENT;
> + return 0;
> }
> diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> index fca3f79b69..8f342dd06e 100644
> --- a/arch/x86/cpu/i386/cpu.c
> +++ b/arch/x86/cpu/i386/cpu.c
> @@ -456,10 +456,15 @@ int x86_cpu_init_f(void)
>
> int x86_cpu_reinit_f(void)
> {
> + long addr;
> +
> setup_identity();
> setup_pci_ram_top();
> - if (locate_coreboot_table() >= 0)
> + addr = locate_coreboot_table();
> + if (addr >= 0) {
> + gd->arch.coreboot_table = addr;
> gd->flags |= GD_FLG_SKIP_LL_INIT;
> + }
>
> return 0;
> }
> diff --git a/arch/x86/include/asm/global_data.h
> b/arch/x86/include/asm/global_data.h
> index 5bc251c0dd..3e4044593c 100644
> --- a/arch/x86/include/asm/global_data.h
> +++ b/arch/x86/include/asm/global_data.h
> @@ -123,6 +123,7 @@ struct arch_global_data {
> #endif
> void *itss_priv;/* Private ITSS data pointer */
> ulong acpi_start; /* Start address of ACPI tables */
> + ulong coreboot_table; /* Address of coreboot table */
> };
>
> #endif
> --
> 2.27.0.290.gba653c62da-goog
regards, Wolfgang
Re: [PATCH v1 41/43] x86: acpi: Correct the version of the MADT
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 41/43] x86: acpi: Correct the version of the MADT
>
> Currently U-Boot implements version 2 but reports version 4. Correct it.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/lib/acpi_table.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index b6ba547b6a..6e3276c6f6 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -155,7 +155,7 @@ static void acpi_create_madt(struct acpi_madt *madt)
> /* Fill out header fields */
> acpi_fill_header(header, "APIC");
> header->length = sizeof(struct acpi_madt);
> - header->revision = 4;
> + header->revision = 2;
Nit:
Commit 91fe8b79f691 ("acpi: Add a central location for table version number")
has introduced defines for the different MADT revision numbers in
include/acpi/acpi_table.h, e.g. ACPI_MADT_REV_ACPI_3_0 for the value 2.
Could you use this instead of the hardcoded value 2?
>
> madt->lapic_addr = LAPIC_DEFAULT_BASE;
> madt->flags = ACPI_MADT_PCAT_COMPAT;
> --
> 2.27.0.290.gba653c62da-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 33/43] x86: irq: Support flags for acpi_gpe
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 33/43] x86: irq: Support flags for acpi_gpe > > This binding currently has a flags cell but it is not used. Make use of it > to create ACPI tables for interrupts. > > Signed-off-by: Simon Glass > --- > > arch/x86/cpu/acpi_gpe.c | 26 +++ > .../interrupt-controller/x86-irq.h| 14 ++ > 2 files changed, 40 insertions(+) > create mode 100644 include/dt-bindings/interrupt-controller/x86-irq.h > [snip] > diff --git a/include/dt-bindings/interrupt-controller/x86-irq.h > b/include/dt-bindings/interrupt-controller/x86-irq.h > new file mode 100644 > index 00..9e0b4612e1 > --- /dev/null > +++ b/include/dt-bindings/interrupt-controller/x86-irq.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright 2019 Google LLC > + * > + * This provides additional flags used by x86. > + */ > + > +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_X86_IRQ_H > +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_X86_IRQ_H > + > +#define X86_IRQ_TYPE_SHARED (1 << 4) > +#define X86_IRQ_TYPE_WAKE (1 << 5) Nit: BIT(4) and BIT(4) ? > + > +#endif > -- > 2.27.0.290.gba653c62da-goog Reviewed-by: Wolfgang Wallner
[PATCH v3 2/2] x86: p2sb: make P2SB driver depend on P2SB uclass
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. Signed-off-by: Wolfgang Wallner --- (no changes since v2) Changes in v2: - Fixed cover letter arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c8eae24c07..27295ef384 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -717,6 +717,7 @@ config HAVE_ITSS config HAVE_P2SB bool "Enable P2SB" + depends on P2SB help Select this to include the driver for the Primary to Sideband Bridge (P2SB) which is found on several Intel -- 2.27.0
[PATCH v2 1/2] drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge
In Intel's documentation the term P2SB stands for "Primary to Sideband Bridge". Signed-off-by: Wolfgang Wallner --- Changes in v3: - Replaced the term in two more places drivers/misc/Kconfig | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 6bb5bc77e9..b67e906a76 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -243,10 +243,10 @@ config NUVOTON_NCT6102D in the Nuvoton Super IO chips on X86 platforms. config P2SB - bool "Intel Primary-to-Sideband Bus" + bool "Intel Primary to Sideband Bridge" depends on X86 || SANDBOX help - This enables support for the Intel Primary-to-Sideband bus, + This enables support for the Intel Primary to Sideband Bridge, abbreviated to P2SB. The P2SB is used to access various peripherals such as eSPI, GPIO, through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals @@ -256,20 +256,20 @@ config P2SB devices - see pcr_readl(), etc. config SPL_P2SB - bool "Intel Primary-to-Sideband Bus in SPL" + bool "Intel Primary to Sideband Bridge in SPL" depends on SPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added in the device tree as subnodes of the p2sb. config TPL_P2SB - bool "Intel Primary-to-Sideband Bus in TPL" + bool "Intel Primary to Sideband Bridge in TPL" depends on TPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added -- 2.27.0
[PATCH v3 1/2] drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge
In Intel's documentation the term P2SB stands for "Primary to Sideband Bridge". Signed-off-by: Wolfgang Wallner --- Changes in v3: - Replaced the term in two more places drivers/misc/Kconfig | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 6bb5bc77e9..b67e906a76 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -243,10 +243,10 @@ config NUVOTON_NCT6102D in the Nuvoton Super IO chips on X86 platforms. config P2SB - bool "Intel Primary-to-Sideband Bus" + bool "Intel Primary to Sideband Bridge" depends on X86 || SANDBOX help - This enables support for the Intel Primary-to-Sideband bus, + This enables support for the Intel Primary to Sideband Bridge, abbreviated to P2SB. The P2SB is used to access various peripherals such as eSPI, GPIO, through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals @@ -256,20 +256,20 @@ config P2SB devices - see pcr_readl(), etc. config SPL_P2SB - bool "Intel Primary-to-Sideband Bus in SPL" + bool "Intel Primary to Sideband Bridge in SPL" depends on SPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added in the device tree as subnodes of the p2sb. config TPL_P2SB - bool "Intel Primary-to-Sideband Bus in TPL" + bool "Intel Primary to Sideband Bridge in TPL" depends on TPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added -- 2.27.0
[PATCH v2 2/2] x86: p2sb: make P2SB driver depend on P2SB uclass
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. Signed-off-by: Wolfgang Wallner --- (no changes since v2) Changes in v2: - Fixed cover letter arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c8eae24c07..27295ef384 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -717,6 +717,7 @@ config HAVE_ITSS config HAVE_P2SB bool "Enable P2SB" + depends on P2SB help Select this to include the driver for the Primary to Sideband Bridge (P2SB) which is found on several Intel -- 2.27.0
Antwort: Re: [PATCH v2 0/2] x86: p2sb: P2SB fixes
Hi Bin, -"Bin Meng" schrieb: - > Betreff: Re: [PATCH v2 0/2] x86: p2sb: P2SB fixes > > Hi Wolfgang, > > On Wed, Jul 1, 2020 at 5:06 PM Wolfgang Wallner > wrote: > > > > Currently it is possible to select the P2SB driver without selecting the > > P2SB uclass, which can't work. Fix this by adding a "depends on" in > > Kconfig. > > > > While at it, correct the meaning of P2SB (according to Intel's > > documentation P2SB stands for "Primary to Sideband Bridge"). > > > > Remark: I have resent this series as V2 as I had messed up the cover > > letter in V1. > > > > Changes in v2: > > - Fixed cover letter > > > > Is this series needed for v2020.07? Does this break anything? No, it is not needed for v2020.07. This patch is only a precaution to avoid creating configs that won't work. regards, Wolfgang
[PATCH v2 1/2] drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge
In Intel's documentation the term P2SB stands for "Primary to Sideband Bridge". Signed-off-by: Wolfgang Wallner --- (no changes since v1) drivers/misc/Kconfig | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 6bb5bc77e9..b6b8510e40 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -243,10 +243,10 @@ config NUVOTON_NCT6102D in the Nuvoton Super IO chips on X86 platforms. config P2SB - bool "Intel Primary-to-Sideband Bus" + bool "Intel Primary to Sideband Bridge" depends on X86 || SANDBOX help - This enables support for the Intel Primary-to-Sideband bus, + This enables support for the Intel Primary to Sideband Bridge, abbreviated to P2SB. The P2SB is used to access various peripherals such as eSPI, GPIO, through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals @@ -259,7 +259,7 @@ config SPL_P2SB bool "Intel Primary-to-Sideband Bus in SPL" depends on SPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added @@ -269,7 +269,7 @@ config TPL_P2SB bool "Intel Primary-to-Sideband Bus in TPL" depends on TPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added -- 2.27.0
[PATCH v2 2/2] x86: p2sb: make P2SB driver depend on P2SB uclass
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. Signed-off-by: Wolfgang Wallner --- Changes in v2: - Fixed cover letter arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c8eae24c07..27295ef384 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -717,6 +717,7 @@ config HAVE_ITSS config HAVE_P2SB bool "Enable P2SB" + depends on P2SB help Select this to include the driver for the Primary to Sideband Bridge (P2SB) which is found on several Intel -- 2.27.0
[PATCH v2 0/2] x86: p2sb: P2SB fixes
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. While at it, correct the meaning of P2SB (according to Intel's documentation P2SB stands for "Primary to Sideband Bridge"). Remark: I have resent this series as V2 as I had messed up the cover letter in V1. Changes in v2: - Fixed cover letter Wolfgang Wallner (2): drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge x86: p2sb: make P2SB driver depend on P2SB uclass arch/x86/Kconfig | 1 + drivers/misc/Kconfig | 8 2 files changed, 5 insertions(+), 4 deletions(-) -- 2.27.0
[PATCH 1/2] drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge
In Intel's documentation the term P2SB stands for "Primary to Sideband Bridge". Signed-off-by: Wolfgang Wallner --- drivers/misc/Kconfig | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 6bb5bc77e9..b6b8510e40 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -243,10 +243,10 @@ config NUVOTON_NCT6102D in the Nuvoton Super IO chips on X86 platforms. config P2SB - bool "Intel Primary-to-Sideband Bus" + bool "Intel Primary to Sideband Bridge" depends on X86 || SANDBOX help - This enables support for the Intel Primary-to-Sideband bus, + This enables support for the Intel Primary to Sideband Bridge, abbreviated to P2SB. The P2SB is used to access various peripherals such as eSPI, GPIO, through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals @@ -259,7 +259,7 @@ config SPL_P2SB bool "Intel Primary-to-Sideband Bus in SPL" depends on SPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added @@ -269,7 +269,7 @@ config TPL_P2SB bool "Intel Primary-to-Sideband Bus in TPL" depends on TPL && (X86 || SANDBOX) help - The Primary-to-Sideband bus is used to access various peripherals + The Primary to Sideband Bridge is used to access various peripherals through memory-mapped I/O in a large chunk of PCI space. The space is segmented into different channels and peripherals are accessed by device-specific means within those channels. Devices should be added -- 2.27.0
[PATCH 0/2] Currently it is possible to select the P2SB driver without selecting the
P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. While at it, correct the meaning of P2SB (according to Intel's documentation P2SB stands for "Primary to Sideband Bridge"). Wolfgang Wallner (2): drivers: p2sb: replace Primary-to-Sideband Bus with Primary to Sideband Bridge x86: p2sb: make P2SB driver depend on P2SB uclass arch/x86/Kconfig | 1 + drivers/misc/Kconfig | 8 2 files changed, 5 insertions(+), 4 deletions(-) -- 2.27.0
[PATCH 2/2] x86: p2sb: make P2SB driver depend on P2SB uclass
Currently it is possible to select the P2SB driver without selecting the P2SB uclass, which can't work. Fix this by adding a "depends on" in Kconfig. Signed-off-by: Wolfgang Wallner --- arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c8eae24c07..27295ef384 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -717,6 +717,7 @@ config HAVE_ITSS config HAVE_P2SB bool "Enable P2SB" + depends on P2SB help Select this to include the driver for the Primary to Sideband Bridge (P2SB) which is found on several Intel -- 2.27.0
Re: [PATCH v1 17/43] x86: pinctrl: Add a way to get the pinctrl reg address
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 17/43] x86: pinctrl: Add a way to get the pinctrl reg > address > > At present we can query the offset of a pinctrl register within the p2sb. > For ACPI we need to get the actual address of the register. Add a function > to handle this and rename the old one to more accurately reflect its > purpose. > > Signed-off-by: Simon Glass > --- > > arch/x86/include/asm/intel_pinctrl.h | 16 ++-- > drivers/gpio/intel_gpio.c| 15 +++ > drivers/misc/p2sb-uclass.c | 16 > drivers/pinctrl/intel/pinctrl.c | 11 +-- > include/p2sb.h | 9 + > 5 files changed, 51 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/intel_pinctrl.h > b/arch/x86/include/asm/intel_pinctrl.h > index e2524b089d..f39ebde539 100644 > --- a/arch/x86/include/asm/intel_pinctrl.h > +++ b/arch/x86/include/asm/intel_pinctrl.h > @@ -263,11 +263,23 @@ int pinctrl_read_pads(struct udevice *dev, ofnode node, > const char *prop, > int pinctrl_count_pads(struct udevice *dev, u32 *pads, int size); > > /** > - * intel_pinctrl_get_config_reg_addr() - Get address of the pin config > registers > + * intel_pinctrl_get_config_reg_offset() - Get offset of pin config registers > * > + * This works out the register offset of a pin within the p2sb region. > + * > + * @dev: Pinctrl device > + * @offset: GPIO offset within this device > + * @return register offset of first register within the GPIO p2sb region > + */ > +u32 intel_pinctrl_get_config_reg_offset(struct udevice *dev, uint offset); > + > +/** > + * intel_pinctrl_get_config_reg_offset() - Get address of pin config > registers Copy/Paste error: intel_pinctrl_get_config_reg_addr() > + * > + * This works out the absolute address of the registers for a pin > * @dev: Pinctrl device > * @offset: GPIO offset within this device > - * @return register offset within the GPIO p2sb region > + * @return register offset of first register within the GPIO p2sb region Copy/Paste error: should be address, not offset > */ > u32 intel_pinctrl_get_config_reg_addr(struct udevice *dev, uint offset); [snip] Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 12/43] x86: Add bindings for NHLT
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 12/43] x86: Add bindings for NHLT > > Add devicetree bindings for the Intel Non-High-Definition-Audio Link Table > (NHLT). > > Signed-off-by: Simon Glass > --- > > include/dt-bindings/sound/nhlt.h | 23 +++ > 1 file changed, 23 insertions(+) > create mode 100644 include/dt-bindings/sound/nhlt.h > > diff --git a/include/dt-bindings/sound/nhlt.h > b/include/dt-bindings/sound/nhlt.h > new file mode 100644 > index 00..c33f874966 > --- /dev/null > +++ b/include/dt-bindings/sound/nhlt.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright 2019 Google LLC > + */ > + > +#ifndef _DT_BINDINGS_SOUND_NHLT_H > +#define _DT_BINDINGS_SOUND_NHLT_H > + Nit: I like to know where numeric constants come from, so I would propose to add a comment like the following: /* See Table 2-1. NHLT Endpoint Descriptor in the NHLT Specification (0.8.1) */ > +#define NHLT_VID 0x8086 > +#define NHLT_DID_DMIC0xae20 > +#define NHLT_DID_BT 0xae30 > +#define NHLT_DID_SSP 0xae34 > + > +/* Hardware links available to use for codecs */ > +#define AUDIO_LINK_SSP0 0 > +#define AUDIO_LINK_SSP1 1 > +#define AUDIO_LINK_SSP2 2 > +#define AUDIO_LINK_SSP3 3 > +#define AUDIO_LINK_SSP4 4 > +#define AUDIO_LINK_SSP5 5 > +#define AUDIO_LINK_DMIC 6 > + > +#endif /* _DT_BINDINGS_SOUND_NHLT_H */ > -- > 2.27.0.290.gba653c62da-goog Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 11/43] acpi: mmc: Generate ACPI info for the PCI SD Card
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 11/43] acpi: mmc: Generate ACPI info for the PCI SD Card > > Write required information into the SSDT to describe the SD card > card-detect pin. Since the required GPIO properties are not present in > the device-tree binding, set them manually for now. > > Signed-off-by: Simon Glass > --- > > Changes in v1: > - Capitalise ACPI_OPS_PTR > > configs/sandbox_defconfig | 2 + > drivers/mmc/pci_mmc.c | 78 ++- > 2 files changed, 79 insertions(+), 1 deletion(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 08/43] acpi: Export functions to write sized values
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 08/43] acpi: Export functions to write sized values > > At present only acpigen_write_integer() is exported for use by other code. > But in some cases it is useful to call the specific function depending on > the size of the value. > > Export these functions and add a test. > > Signed-off-by: Simon Glass > --- > > include/acpi/acpigen.h | 46 ++ > test/dm/acpigen.c | 45 - > 2 files changed, 90 insertions(+), 1 deletion(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 07/43] dm: acpi: Add support for the NHLT table
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 07/43] dm: acpi: Add support for the NHLT table > > The Intel Non-High-Definition-Audio Link Table (NHLT) table describes the > audio codecs and connections in a system. Various devices can contribute > information to produce the table. > > Add core support for this, based on a structure which is built up through > calls to the driver. > > Signed-off-by: Simon Glass > --- > > drivers/core/acpi.c | 15 +++ > include/dm/acpi.h | 26 ++ > 2 files changed, 41 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 42/43] x86: Rename board_final_cleanup() to board_final_init()
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 42/43] x86: Rename board_final_cleanup() to > board_final_init() > > This function sounds like something that is called when U-Boot is about to > jump to Linux. In fact it is an init function. > > Rename it to reduce confusion. > > Signed-off-by: Simon Glass > --- > > arch/x86/cpu/coreboot/coreboot.c | 4 ++-- > arch/x86/cpu/cpu.c | 8 > arch/x86/cpu/efi/app.c | 2 +- > arch/x86/cpu/quark/quark.c | 2 +- > arch/x86/lib/fsp/fsp_common.c| 2 +- > 5 files changed, 9 insertions(+), 9 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 35/43] x86: Add debugging to table writing
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 35/43] x86: Add debugging to table writing > > Writing tables is currently pretty opaque. Add a bit of debugging to the > process so we can see what tables are written and where they start/end in > memory. > > Signed-off-by: Simon Glass > --- > > arch/x86/lib/tables.c | 38 -- > 1 file changed, 28 insertions(+), 10 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 39/43] x86: Update the comment about booting for FSP2
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 39/43] x86: Update the comment about booting for FSP2 > > The comment here applies only to FSP1, so update it. > > Signed-off-by: Simon Glass > --- > > arch/x86/cpu/start.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S > index 01524635e9..4ad515ce08 100644 > --- a/arch/x86/cpu/start.S > +++ b/arch/x86/cpu/start.S > @@ -124,6 +124,7 @@ car_init_ret: > #endif > #else > /* > + * Instructions for FSP1, but not FSP2: >* U-Boot enters here twice. For the first time it comes from >* car_init_done() with esp points to a temporary stack and esi >* set to zero. For the second time it comes from fsp_init_done() > -- > 2.27.0.290.gba653c62da-goog Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 26/43] i2c: designware_i2c: Add a little more debugging
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 26/43] i2c: designware_i2c: Add a little more debugging > > Add debugging for a few more values and also use log to show return values > when something goes wrong. This makes it easier to see the root cause. > > Signed-off-by: Simon Glass > --- > > Changes in v1: > - Add new patch to improve designware_i2c debugging > > drivers/i2c/designware_i2c.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 28/43] i2c: designware_i2c: Support ACPI table generation
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 28/43] i2c: designware_i2c: Support ACPI table generation
>
> Update the PCI driver to generate ACPI information so that Linux has the
> full information about each I2C bus.
>
> Signed-off-by: Simon Glass
>
> ---
>
> Changes in v1:
> - Capitalise ACPI_OPS_PTR
>
> drivers/i2c/designware_i2c.c | 25
> drivers/i2c/designware_i2c.h | 15 +
> drivers/i2c/designware_i2c_pci.c | 104 ++-
> 3 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> index 44a1f33398..630938743f 100644
> --- a/drivers/i2c/designware_i2c.c
> +++ b/drivers/i2c/designware_i2c.c
> @@ -333,6 +333,31 @@ static int _dw_i2c_set_bus_speed(struct dw_i2c *priv,
> struct i2c_regs *i2c_base,
> /* Restore back i2c now speed set */
> if (ena == IC_ENABLE_0B)
> dw_i2c_enable(i2c_base, true);
> + if (priv)
> + priv->config = config;
> + return 0;
> +}
> +
> +int dw_i2c_gen_speed_config(const struct udevice *dev, int speed_hz,
> + struct dw_i2c_speed_config *config)
> +{
> + struct dw_i2c *priv = dev_get_priv(dev);
> + ulong rate;
> + int ret;
> +
> +#if CONFIG_IS_ENABLED(CLK)
> + rate = clk_get_rate(>clk);
> + if (IS_ERR_VALUE(rate))
> + return log_msg_ret("clk", -EINVAL);
> +#else
> + rate = IC_CLK;
> +#endif
> +
> + ret = calc_bus_speed(priv, priv->regs, speed_hz, rate, config);
> + if (ret)
> + printf("%s: ret=%d\n", __func__, ret);
> + if (ret)
> + return log_msg_ret("calc_bus_speed", ret);
>
> return 0;
> }
> diff --git a/drivers/i2c/designware_i2c.h b/drivers/i2c/designware_i2c.h
> index dc9a6ccb63..d87a3bff93 100644
> --- a/drivers/i2c/designware_i2c.h
> +++ b/drivers/i2c/designware_i2c.h
> @@ -205,6 +205,7 @@ struct dw_i2c {
> #if CONFIG_IS_ENABLED(CLK)
> struct clk clk;
> #endif
> + struct dw_i2c_speed_config config;
> };
>
> extern const struct dm_i2c_ops designware_i2c_ops;
> @@ -213,4 +214,18 @@ int designware_i2c_probe(struct udevice *bus);
> int designware_i2c_remove(struct udevice *dev);
> int designware_i2c_ofdata_to_platdata(struct udevice *bus);
>
> +/**
> + * dw_i2c_gen_speed_config() - Calculate config info from requested speed1
> + *
> + * Calculate the speed config from the given @speed_hz and return it so that
> + * it can be incorporated in ACPI tables
> + *
> + * @dev: I2C bus to check
> + * @speed_hz: Requested speed in Hz
> + * @config: Returns config to use for that speed
> + * @return 0 if OK, -ve on error
> + */
> +int dw_i2c_gen_speed_config(const struct udevice *dev, int speed_hz,
> + struct dw_i2c_speed_config *config);
> +
> #endif /* __DW_I2C_H_ */
> diff --git a/drivers/i2c/designware_i2c_pci.c
> b/drivers/i2c/designware_i2c_pci.c
> index bd34ec0b47..d5108e9064 100644
> --- a/drivers/i2c/designware_i2c_pci.c
> +++ b/drivers/i2c/designware_i2c_pci.c
> @@ -9,7 +9,12 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> +#include
> +#include
> +#include
> #include "designware_i2c.h"
>
> enum {
> @@ -87,6 +92,8 @@ static int designware_i2c_pci_bind(struct udevice *dev)
> {
> char name[20];
>
> + if (dev_of_valid(dev))
> + return 0;
> /*
>* Create a unique device name for PCI type devices
>* ToDo:
> @@ -100,13 +107,107 @@ static int designware_i2c_pci_bind(struct udevice *dev)
>* be possible. We cannot use static data in drivers since they may be
>* used in SPL or before relocation.
>*/
> - dev->req_seq = gd->arch.dw_i2c_num_cards++;
> + dev->req_seq = uclass_find_next_free_req_seq(UCLASS_I2C);
> sprintf(name, "i2c_designware#%u", dev->req_seq);
> device_set_name(dev, name);
>
> return 0;
> }
>
> +/*
> + * Write ACPI object to describe speed configuration.
> + *
> + * ACPI Object: Name ("", Package () { scl_lcnt, scl_hcnt, sda_hold }
> + *
> + * SSCN: I2C_SPEED_STANDARD
> + * FMCN: I2C_SPEED_FAST
> + * FPCN: I2C_SPEED_FAST_PLUS
> + * HSCN: I2C_SPEED_HIGH
> + */
> +static void dw_i2c_acpi_write_speed_config(struct acpi_ctx *ctx,
> +struct dw_i2c_speed_config *config)
> +{
> + switch (config->speed_mode) {
> + case IC_SPEED_MODE_HIGH:
> + acpigen_write_name(ctx, "HSCN");
> + break;
> + case IC_SPEED_MODE_FAST_PLUS:
> + acpigen_write_name(ctx, "FPCN");
> + break;
> + case IC_SPEED_MODE_FAST:
> + acpigen_write_name(ctx, "FMCN");
> + break;
> + case IC_SPEED_MODE_STANDARD:
> + default:
> + acpigen_write_name(ctx, "SSCN");
> + }
> +
> + /* Package () { scl_lcnt, scl_hcnt, sda_hold } */
> + acpigen_write_package(ctx, 3);
> +
Re: [PATCH v1 32/43] pmc: Move common registers to the header file
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 32/43] pmc: Move common registers to the header file > > These registers need to be accesses from ACPI code, so move them to the > header file. > > Signed-off-by: Simon Glass > --- > > drivers/power/acpi_pmc/acpi-pmc-uclass.c | 9 - > include/power/acpi_pmc.h | 14 ++ > 2 files changed, 14 insertions(+), 9 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 27/43] i2c: Add log_ret() on error
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 27/43] i2c: Add log_ret() on error > > Add a few of these calls to make it easier to see where an error occurs, > if CONFIG_LOG_ERROR_RETURN is enabled. > > Signed-off-by: Simon Glass > --- > > drivers/i2c/i2c-uclass.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 04/43] acpi: Allow creating the GNVS to fail
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 04/43] acpi: Allow creating the GNVS to fail
>
> In some cases an internal error may prevent this from working. Update the
> function return value and report the error. At present the API for writing
> tables does not easily support reporting errors, but once it is fully
> updated to use a context pointer, this will be easier.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v1:
> - Add linux/err.h header
>
> arch/x86/cpu/baytrail/acpi.c | 4 +++-
> arch/x86/cpu/quark/acpi.c | 4 +++-
> arch/x86/cpu/tangier/acpi.c | 4 +++-
> arch/x86/include/asm/acpi_table.h | 10 +-
> arch/x86/lib/acpi_table.c | 11 +--
> 5 files changed, 27 insertions(+), 6 deletions(-)
>
[snip]
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index eeacfe9b06..27869a0e5e 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * IASL compiles the dsdt entries and writes the hex values
> @@ -443,8 +444,14 @@ ulong write_acpi_tables(ulong start_addr)
> dsdt->checksum = 0;
> dsdt->checksum = table_compute_checksum((void *)dsdt, dsdt->length);
>
> - /* Fill in platform-specific global NVS variables */
> - acpi_create_gnvs(ctx->current);
> + /*
> + * Fill in platform-specific global NVS variables. If this fails we
> + * cannot return the error but this should only happen while debugging.
> + */
> + addr = acpi_create_gnvs(ctx->current);
> + if (IS_ERR_VALUE(addr))
> + printf("Error: Gailed to create GNVS\n");
Typo: Failed
> +
> acpi_inc_align(ctx, sizeof(struct acpi_global_nvs));
>
> debug("ACPI:* FADT\n");
> --
> 2.27.0.290.gba653c62da-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 06/43] dm: core: Add a way of overriding the ACPI device path
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 06/43] dm: core: Add a way of overriding the ACPI device > path > > Some devices such as GPIO need to override the normal path that would be > generated by driver model. Add a device-tree property for this. > > Signed-off-by: Simon Glass > --- > > doc/device-tree-bindings/device.txt | 23 +++ > drivers/core/acpi.c | 19 +++ > include/dm/acpi.h | 13 + > 3 files changed, 55 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 36/43] x86: apl: Adjust FSP-M code to avoid hard-coded address
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 36/43] x86: apl: Adjust FSP-M code to avoid hard-coded
> address
>
> Update this code to calculate the address to use, rather than hard-coding
> it. Obtain the requested stack size from the FSP.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/cpu/apollolake/fsp_m.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/cpu/apollolake/fsp_m.c b/arch/x86/cpu/apollolake/fsp_m.c
> index 65461d85b8..cef937573b 100644
> --- a/arch/x86/cpu/apollolake/fsp_m.c
> +++ b/arch/x86/cpu/apollolake/fsp_m.c
> @@ -24,9 +24,11 @@ int fspm_update_config(struct udevice *dev, struct
> fspm_upd *upd)
> cache_ret = prepare_mrc_cache(upd);
> if (cache_ret && cache_ret != -ENOENT)
> return log_msg_ret("mrc", cache_ret);
> - arch->stack_base = (void *)0xfef96000;
> + arch->stack_base = (void *)(CONFIG_SYS_CAR_ADDR + CONFIG_SYS_CAR_SIZE -
> + arch->stack_size);
> arch->boot_loader_tolum_size = 0;
> - arch->boot_mode = FSP_BOOT_WITH_FULL_CONFIGURATION;
> + arch->boot_mode = cache_ret ? FSP_BOOT_WITH_FULL_CONFIGURATION :
> + FSP_BOOT_ASSUMING_NO_CONFIGURATION_CHANGES;
This change is not related to the change calculation of stack_base, and so I
think it should be a separate commit.
I see that arch-boot_mode is handled similarly in Coreboot, so I assume this
is correct, but I could not find documentation about what
FSP_BOOT_ASSUMING_NO_CONFIGURATION_CHANGES is doing differently.
Could you point me to the relevant documentation?
>
> node = dev_ofnode(dev);
> if (!ofnode_valid(node))
> --
> 2.27.0.290.gba653c62da-goog
I think both changes are correct, just that they should be separate patches.
So if you split them up both of them are:
Reviewed-by: Wolfgang Wallner
regards, Wolfgang
Re: [PATCH v1 14/43] acpi: Support writing named values
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 14/43] acpi: Support writing named values > > Allow writing named integers and strings to the generated ACPI code. > > Signed-off-by: Simon Glass > --- > > include/acpi/acpigen.h | 72 ++ > lib/acpi/acpigen.c | 49 ++ > test/dm/acpigen.c | 78 ++ > 3 files changed, 199 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 21/43] x86: pinctrl: Drop the acpi_name member
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 21/43] x86: pinctrl: Drop the acpi_name member
Nit: You are dropping the acpi_path member, not acpi_name.
>
> This is in the device tree now, so drop the unnecessary field here.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/include/asm/intel_pinctrl.h | 2 --
> drivers/pinctrl/intel/pinctrl_apl.c | 4
> 2 files changed, 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/intel_pinctrl.h
> b/arch/x86/include/asm/intel_pinctrl.h
> index 982b2514a0..e734f4a7f0 100644
> --- a/arch/x86/include/asm/intel_pinctrl.h
> +++ b/arch/x86/include/asm/intel_pinctrl.h
> @@ -99,7 +99,6 @@ struct pad_group {
> * groups exist inside a community
> *
> * @name: Community name
> - * @acpi_path: ACPI path
> * @num_gpi_regs: number of gpi registers in community
> * @max_pads_per_group: number of pads in each group; number of pads
> bit-mapped
> * in each GPI status/en and Host Own Reg
> @@ -120,7 +119,6 @@ struct pad_group {
> */
> struct pad_community {
> const char *name;
> - const char *acpi_path;
> size_t num_gpi_regs;
> size_t max_pads_per_group;
> uint first_pad;
> diff --git a/drivers/pinctrl/intel/pinctrl_apl.c
> b/drivers/pinctrl/intel/pinctrl_apl.c
> index c14176d4a7..7624a9974f 100644
> --- a/drivers/pinctrl/intel/pinctrl_apl.c
> +++ b/drivers/pinctrl/intel/pinctrl_apl.c
> @@ -75,7 +75,6 @@ static const struct pad_community apl_gpio_communities[] = {
> .gpi_smi_en_reg_0 = GPI_SMI_EN_0,
> .max_pads_per_group = GPIO_MAX_NUM_PER_GROUP,
> .name = "GPIO_GPE_N",
> - .acpi_path = "\\_SB.GPO0",
> .reset_map = rst_map,
> .num_reset_vals = ARRAY_SIZE(rst_map),
> .groups = apl_community_n_groups,
> @@ -94,7 +93,6 @@ static const struct pad_community apl_gpio_communities[] = {
> .gpi_smi_en_reg_0 = GPI_SMI_EN_0,
> .max_pads_per_group = GPIO_MAX_NUM_PER_GROUP,
> .name = "GPIO_GPE_NW",
> - .acpi_path = "\\_SB.GPO1",
> .reset_map = rst_map,
> .num_reset_vals = ARRAY_SIZE(rst_map),
> .groups = apl_community_nw_groups,
> @@ -113,7 +111,6 @@ static const struct pad_community apl_gpio_communities[]
> = {
> .gpi_smi_en_reg_0 = GPI_SMI_EN_0,
> .max_pads_per_group = GPIO_MAX_NUM_PER_GROUP,
> .name = "GPIO_GPE_W",
> - .acpi_path = "\\_SB.GPO2",
> .reset_map = rst_map,
> .num_reset_vals = ARRAY_SIZE(rst_map),
> .groups = apl_community_w_groups,
> @@ -132,7 +129,6 @@ static const struct pad_community apl_gpio_communities[]
> = {
> .gpi_smi_en_reg_0 = GPI_SMI_EN_0,
> .max_pads_per_group = GPIO_MAX_NUM_PER_GROUP,
> .name = "GPIO_GPE_SW",
> - .acpi_path = "\\_SB.GPO3",
> .reset_map = rst_map,
> .num_reset_vals = ARRAY_SIZE(rst_map),
> .groups = apl_community_sw_groups,
> --
> 2.27.0.290.gba653c62da-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 13/43] acpi: Support generation of a device
Hi Simon, -"Simon Glass" schrieb: - > > Betreff: [PATCH v1 13/43] acpi: Support generation of a device > > Allow writing an ACPI device to the generated ACPI code. > > Signed-off-by: Simon Glass > --- > > include/acpi/acpigen.h | 9 + > lib/acpi/acpigen.c | 7 +++ > test/dm/acpigen.c | 27 +++ > 3 files changed, 43 insertions(+) Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 10/43] acpi: Support generation of a generic register
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 10/43] acpi: Support generation of a generic register > > Allow writing out a generic register. > > Signed-off-by: Simon Glass > --- > > include/acpi/acpi_device.h | 1 + > include/acpi/acpigen.h | 28 +++ > lib/acpi/acpigen.c | 71 ++ > test/dm/acpigen.c | 46 > 4 files changed, 146 insertions(+) > Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 09/43] acpi: Support generation of a scope
Hi Simon,
-"Simon Glass" schrieb: -
> Betreff: [PATCH v1 09/43] acpi: Support generation of a scope
>
> Add a function to write a scope to the generated ACPI code.
>
> Signed-off-by: Simon Glass
> ---
>
> include/acpi/acpigen.h | 10 ++
> lib/acpi/acpigen.c | 7 +++
> test/dm/acpi.c | 3 +--
> test/dm/acpigen.c | 33 -
> 4 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/include/acpi/acpigen.h b/include/acpi/acpigen.h
> index 7c117c1cf6..af66c9828f 100644
> --- a/include/acpi/acpigen.h
> +++ b/include/acpi/acpigen.h
> @@ -31,6 +31,7 @@ enum {
> DWORD_PREFIX= 0x0c,
> STRING_PREFIX = 0x0d,
> QWORD_PREFIX= 0x0e,
> + SCOPE_OP= 0x10,
> BUFFER_OP = 0x11,
> PACKAGE_OP = 0x12,
> METHOD_OP = 0x14,
> @@ -258,6 +259,14 @@ void acpigen_emit_namestring(struct acpi_ctx *ctx, const
> char *namepath);
> */
> void acpigen_write_name(struct acpi_ctx *ctx, const char *namepath);
>
> +/**
> + * acpigen_write_scope() - Write a scope
> + *
> + * @ctx: ACPI context pointer
> + * @name: Scope to write (e.g. "\\_SB.ABCD")
Nit: IMHO 'path' or 'scope' wopuld be better names for this parameter, as
it is usually not a single device name.
> + */
> +void acpigen_write_scope(struct acpi_ctx *ctx, const char *name);
> +
> /**
> * acpigen_write_uuid() - Write a UUID
> *
> @@ -396,6 +405,7 @@ void acpigen_write_power_res(struct acpi_ctx *ctx, const
> char *name, uint level,
> */
> int acpigen_set_enable_tx_gpio(struct acpi_ctx *ctx, u32 tx_state_val,
> const char *dw0_name, struct acpi_gpio *gpio,
> +
> bool enable);
>
> #endif
> diff --git a/lib/acpi/acpigen.c b/lib/acpi/acpigen.c
> index e0af36f775..77d6434806 100644
> --- a/lib/acpi/acpigen.c
> +++ b/lib/acpi/acpigen.c
> @@ -257,6 +257,13 @@ void acpigen_write_name(struct acpi_ctx *ctx, const char
> *namepath)
> acpigen_emit_namestring(ctx, namepath);
> }
>
> +void acpigen_write_scope(struct acpi_ctx *ctx, const char *name)
> +{
> + acpigen_emit_byte(ctx, SCOPE_OP);
> + acpigen_write_len_f(ctx);
> + acpigen_emit_namestring(ctx, name);
> +}
> +
> static void acpigen_write_method_(struct acpi_ctx *ctx, const char *name,
> uint flags)
> {
> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
> index 7768f9514c..b94c4ba4d1 100644
> --- a/test/dm/acpi.c
> +++ b/test/dm/acpi.c
> @@ -20,9 +20,8 @@
> #include
> #include
> #include
> +#include "acpi.h"
>
> -#define ACPI_TEST_DEV_NAME "ABCD"
> -#define ACPI_TEST_CHILD_NAME "EFGH"
> #define BUF_SIZE 4096
>
> /**
> diff --git a/test/dm/acpigen.c b/test/dm/acpigen.c
> index 0322dd4f60..93dfa301d6 100644
> --- a/test/dm/acpigen.c
> +++ b/test/dm/acpigen.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include "acpi.h"
>
> #define TEST_STRING "frogmore"
> #define TEST_STRING2 "ranch"
> @@ -359,7 +360,7 @@ DM_TEST(dm_test_acpi_spi, DM_TESTF_SCAN_PDATA |
> DM_TESTF_SCAN_FDT);
> */
> static int get_length(u8 *ptr)
> {
> - if (!(*ptr & 0x80))
> + if (!(*ptr & ACPI_PKG_LEN_3_BYTES))
> return -EINVAL;
>
> return (*ptr & 0xf) | ptr[1] << 4 | ptr[2] << 12;
> @@ -906,3 +907,33 @@ static int dm_test_acpi_write_values(struct
> unit_test_state *uts)
> }
> DM_TEST(dm_test_acpi_write_values, 0);
>
> +/* Test writing a scope */
> +static int dm_test_acpi_scope(struct unit_test_state *uts)
> +{
> + char buf[ACPI_PATH_MAX];
> + struct acpi_ctx *ctx;
> + struct udevice *dev;
> + u8 *ptr;
> +
> + ut_assertok(alloc_context());
> + ptr = acpigen_get_current(ctx);
> +
> + ut_assertok(uclass_first_device_err(UCLASS_TEST_ACPI, ));
> + ut_assertok(acpi_device_path(dev, buf, sizeof(buf)));
> + acpigen_write_scope(ctx, buf);
> + acpigen_pop_len(ctx);
> +
> + ut_asserteq(SCOPE_OP, *ptr++);
> + ut_asserteq(13, get_length(ptr));
> + ptr += 3;
> + ut_asserteq(ROOT_PREFIX, *ptr++);
> + ut_asserteq(DUAL_NAME_PREFIX, *ptr++);
> + ut_asserteq_strn("_SB_" ACPI_TEST_DEV_NAME, (char *)ptr);
> + ptr += 8;
> + ut_asserteq_ptr(ptr, ctx->current);
> +
> + free_context();
> +
> + return 0;
> +}
> +DM_TEST(dm_test_acpi_scope, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> --
> 2.27.0.290.gba653c62da-goog
Reviewed-by: Wolfgang Wallner
Re: [PATCH v1 31/43] x86: apl: Hide the p2sb on exit from U-Boot
Hi Simon,
> -"Simon Glass" schrieb: -
> Betreff: [PATCH v1 31/43] x86: apl: Hide the p2sb on exit from U-Boot
>
> This confuses Linux's PCI probing so needs to be hidden when booting
> Linux. Add a remove() method to handle this.
>
> Signed-off-by: Simon Glass
> ---
>
> arch/x86/cpu/intel_common/p2sb.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/cpu/intel_common/p2sb.c
> b/arch/x86/cpu/intel_common/p2sb.c
> index ebf8f62aea..361d4c90cb 100644
> --- a/arch/x86/cpu/intel_common/p2sb.c
> +++ b/arch/x86/cpu/intel_common/p2sb.c
> @@ -153,6 +153,17 @@ static int intel_p2sb_set_hide(struct udevice *dev, bool
> hide)
> return 0;
> }
>
> +static int p2sb_remove(struct udevice *dev)
> +{
> + int ret;
> +
> + ret = intel_p2sb_set_hide(dev, true);
> + if (ret)
> + return log_msg_ret("hide", ret);
> +
> + return 0;
> +}
> +
> static int p2sb_child_post_bind(struct udevice *dev)
> {
> #if !CONFIG_IS_ENABLED(OF_PLATDATA)
> @@ -183,9 +194,12 @@ U_BOOT_DRIVER(p2sb_drv) = {
> .id = UCLASS_P2SB,
> .of_match = p2sb_ids,
> .probe = p2sb_probe,
> + .remove = p2sb_remove,
> + .ops= _ops,
Nit: should this line be part of the previous patch?
> .ofdata_to_platdata = p2sb_ofdata_to_platdata,
> .platdata_auto_alloc_size = sizeof(struct p2sb_platdata),
> .per_child_platdata_auto_alloc_size =
> sizeof(struct p2sb_child_platdata),
> .child_post_bind = p2sb_child_post_bind,
> + .flags = DM_FLAG_OS_PREPARE,
> };
> --
> 2.27.0.290.gba653c62da-goog
Reviewed-by: Wolfgang Wallner
Tested-by: Wolfgang Wallner
Tested on an Apollo Lake-based board.
Re: [PATCH v1 29/43] p2sb: Add a method to hide the bus
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 29/43] p2sb: Add a method to hide the bus > > The P2SB bus needs to be hidden in some cases so that it does not get > auto-configured by Linux. Add a method for this. > > Signed-off-by: Simon Glass > --- > > drivers/misc/p2sb-uclass.c | 10 ++ > include/p2sb.h | 25 - > 2 files changed, 34 insertions(+), 1 deletion(-) > Reviewed-by: Wolfgang Wallner Tested-by: Wolfgang Wallner Tested on an Apollo Lake-based board.
Re: [PATCH v1 30/43] x86: apl: Support set_hide() in p2sb driver
Hi Simon, -"Simon Glass" schrieb: - > Betreff: [PATCH v1 30/43] x86: apl: Support set_hide() in p2sb driver > > Add support for this new method in the driver and in the fsp-s setup. > > Signed-off-by: Simon Glass > --- > > arch/x86/cpu/apollolake/fsp_s.c | 26 +++--- > arch/x86/cpu/intel_common/p2sb.c | 30 ++ > 2 files changed, 41 insertions(+), 15 deletions(-) > Reviewed-by: Wolfgang Wallner Tested-by: Wolfgang Wallner Tested on an Apollo Lake-based board.
Re: [PATCH 6/6] x86: fsp: Support a warning message when DRAM init is slow
> + if (ret) {
> + if (ret != -ENOENT)
> + return log_msg_ret("Could not setup config", ret);
> + } else {
> + delay = 0;
> + }
> +
> + if (delay)
> + printf("SDRAM training (%d seconds)...", delay);
> + else
> + log_debug("SDRAM init...");
> bootstage_start(BOOTSTAGE_ID_ACCUM_FSP_M, "fsp-m");
> func = (fsp_memory_init_func)(hdr->img_base + hdr->fsp_mem_init);
> ret = func(, );
> bootstage_accum(BOOTSTAGE_ID_ACCUM_FSP_M);
> cpu_reinit_fpu();
> + if (delay)
> + printf("done\n");
> + else
> + log_debug("done\n");
> if (ret)
> return log_msg_ret("SDRAM init fail\n", ret);
>
> gd->arch.hob_list = hob;
> - debug("done\n");
>
> ret = fspm_done(dev);
> if (ret)
> diff --git a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
> b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
> index 647a0862d4..67407fa935 100644
> --- a/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
> +++ b/doc/device-tree-bindings/fsp/fsp2/apollolake/fsp-m.txt
> @@ -17,6 +17,9 @@ values of the FSP-M are used.
> [2] https://github.com/IntelFsp/FSP/tree/master/ApolloLakeFspBinPkg/Docs
>
> Optional properties:
> +- fspm,training-delay: Time taken to train DDR memory if there is no cached
> MRC
> +data, in seconds. This is used to show a message if possible. For Intel
> APL
> +this is typicaly 21 seconds.
1) The required time is not APL specific, I think it depends on the memory
configuration. I have tested it on an APL board with just 1 GB RAM,
and there it only takes ~6 seconds.
How about rewording it as follows:
"As an example, for Chromebook Coral this is typically 21 seconds."
2) Typo: typically
> - fspm,serial-debug-port-address: Debug Serial Port Base address
> - fspm,serial-debug-port-type: Debug Serial Port Type
>0: NONE
> --
> 2.27.0.rc0.183.gde8f92d652-goog
Reviewed-by: Wolfgang Wallner
Tested-by: Wolfgang Wallner
Tested on a custom Apollo Lake board
