| Message ID | 20200614215726.v1.34.I47b041ba995bc87537c2b8b03e5c5dbee96ad725@changeid |
|---|---|
| State | Accepted |
| Commit | b95611f67e709f8c98c1a31714dc941d436c0d5c |
| Headers | show |
| Series | x86: Programmatic generation of ACPI tables (Part C) | expand |
Hi Simon, On Mon, Jun 15, 2020 at 11:58 AM Simon Glass <sjg at chromium.org> wrote: > > 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. > > Signed-off-by: Simon Glass <sjg at chromium.org> > --- > > 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(-) > > diff --git a/arch/x86/cpu/apollolake/fsp_s.c b/arch/x86/cpu/apollolake/fsp_s.c > index 3a54297a28..e54b0ac104 100644 > --- a/arch/x86/cpu/apollolake/fsp_s.c > +++ b/arch/x86/cpu/apollolake/fsp_s.c > @@ -160,11 +160,6 @@ int arch_fsps_preinit(void) > ret = irq_first_device_type(X86_IRQT_ITSS, &itss); > if (ret) > return log_msg_ret("no itss", ret); > - /* > - * Snapshot the current GPIO IRQ polarities. FSP is setting a default > - * policy that doesn't honour boards' requirements > - */ > - irq_snapshot_polarities(itss); > > /* > * Clear the GPI interrupt status and enable registers. These > @@ -203,7 +198,11 @@ int arch_fsp_init_r(void) > ret = irq_first_device_type(X86_IRQT_ITSS, &itss); > if (ret) > return log_msg_ret("no itss", ret); > - /* Restore GPIO IRQ polarities back to previous settings */ > + > + /* > + * Restore GPIO IRQ polarities back to previous settings. This was > + * stored in reserve_arch() - see X86_IRQT_ITSS > + */ > irq_restore_polarities(itss); > > /* soc_init() */ > diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c > index baa7dae172..9bc243ebc8 100644 > --- a/arch/x86/cpu/cpu.c > +++ b/arch/x86/cpu/cpu.c > @@ -25,6 +25,7 @@ > #include <dm.h> > #include <errno.h> > #include <init.h> > +#include <irq.h> > #include <log.h> > #include <malloc.h> > #include <syscon.h> > @@ -274,6 +275,9 @@ int cpu_init_r(void) > #ifndef CONFIG_EFI_STUB > int reserve_arch(void) > { > + struct udevice *itss; > + int ret; > + > if (IS_ENABLED(CONFIG_ENABLE_MRC_CACHE)) > mrccache_reserve(); > > @@ -291,6 +295,15 @@ int reserve_arch(void) > fsp_save_s3_stack(); > } > } > + ret = irq_first_device_type(X86_IRQT_ITSS, &itss); > + if (!ret) { > + /* > + * Snapshot the current GPIO IRQ polarities. FSP-S is about to > + * run and will set a default policy that doesn't honour boards' > + * requirements > + */ > + irq_snapshot_polarities(itss); > + } > > return 0; > } > diff --git a/arch/x86/cpu/intel_common/itss.c b/arch/x86/cpu/intel_common/itss.c > index 963afa8f5b..fe84ebe29f 100644 > --- a/arch/x86/cpu/intel_common/itss.c > +++ b/arch/x86/cpu/intel_common/itss.c > @@ -65,14 +65,23 @@ static int snapshot_polarities(struct udevice *dev) > int i; > > reg_start = start / IRQS_PER_IPC; > - reg_end = (end + IRQS_PER_IPC - 1) / IRQS_PER_IPC; > + reg_end = DIV_ROUND_UP(end, IRQS_PER_IPC); > > + log_info("ITSS IRQ Polarities snapshot %p\n", priv->irq_snapshot); > for (i = reg_start; i < reg_end; i++) { > uint reg = PCR_ITSS_IPC0_CONF + sizeof(u32) * i; > > priv->irq_snapshot[i] = pcr_read32(dev, reg); > + log_debug(" - %d, reg %x: irq_snapshot[i] %x\n", i, reg, > + priv->irq_snapshot[i]); > } > > + /* Save the snapshot for use after relocation */ > + gd->start_addr_sp -= sizeof(*priv); > + gd->start_addr_sp &= ~0xf; > + gd->arch.itss_priv = (void *)gd->start_addr_sp; > + memcpy(gd->arch.itss_priv, priv, sizeof(*priv)); > + > return 0; > } > > @@ -91,16 +100,26 @@ static void show_polarities(struct udevice *dev, const char *msg) > static int restore_polarities(struct udevice *dev) > { > struct itss_priv *priv = dev_get_priv(dev); > + struct itss_priv *old_priv; > const int start = GPIO_IRQ_START; > const int end = GPIO_IRQ_END; > int reg_start; > int reg_end; > int i; > > + /* Get the snapshot which was stored by the pre-reloc device */ > + old_priv = gd->arch.itss_priv; > + if (!old_priv) > + return log_msg_ret("priv", -EFAULT); > + memcpy(priv->irq_snapshot, old_priv->irq_snapshot, > + sizeof(priv->irq_snapshot)); > + > show_polarities(dev, "Before"); > + log_info("priv->irq_snapshot %p\n", priv->irq_snapshot); > > reg_start = start / IRQS_PER_IPC; > - reg_end = (end + IRQS_PER_IPC - 1) / IRQS_PER_IPC; > + reg_end = DIV_ROUND_UP(end, IRQS_PER_IPC); > + > > for (i = reg_start; i < reg_end; i++) { > u32 mask; > @@ -125,6 +144,8 @@ static int restore_polarities(struct udevice *dev) > mask &= ~((1U << irq_start) - 1); > > reg = PCR_ITSS_IPC0_CONF + sizeof(u32) * i; > + log_debug(" - %d, reg %x: mask %x, irq_snapshot[i] %x\n", > + i, reg, mask, priv->irq_snapshot[i]); > pcr_clrsetbits32(dev, reg, mask, mask & priv->irq_snapshot[i]); > } > > diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h > index 0e64c8a46d..5bc251c0dd 100644 > --- a/arch/x86/include/asm/global_data.h > +++ b/arch/x86/include/asm/global_data.h > @@ -121,6 +121,7 @@ struct arch_global_data { > #ifdef CONFIG_FSP_VERSION2 > struct fsp_header *fsp_s_hdr; /* Pointer to FSP-S header */ > #endif > + void *itss_priv; /* Private ITSS data pointer */ > ulong acpi_start; /* Start address of ACPI tables */ > }; > > diff --git a/arch/x86/include/asm/itss.h b/arch/x86/include/asm/itss.h > index c75d8fe8c2..f7d3240384 100644 > --- a/arch/x86/include/asm/itss.h > +++ b/arch/x86/include/asm/itss.h > @@ -16,7 +16,7 @@ > > #define ITSS_MAX_IRQ 119 > #define IRQS_PER_IPC 32 > -#define NUM_IPC_REGS ((ITSS_MAX_IRQ + IRQS_PER_IPC - 1) / IRQS_PER_IPC) > +#define NUM_IPC_REGS DIV_ROUND_UP(ITSS_MAX_IRQ, IRQS_PER_IPC) > > /* Max PXRC registers in ITSS */ > #define MAX_PXRC_CONFIG (PCR_ITSS_PIRQH_ROUT - PCR_ITSS_PIRQA_ROUT + 1) > diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c > index 98bc79eaba..6b32b451a6 100644 > --- a/drivers/misc/irq-uclass.c > +++ b/drivers/misc/irq-uclass.c > @@ -170,7 +170,7 @@ int irq_first_device_type(enum irq_dev_t type, struct udevice **devp) > > ret = uclass_first_device_drvdata(UCLASS_IRQ, type, devp); > if (ret) > - return log_msg_ret("find", ret); > + return ret; Is this change intended? > > return 0; > } Regards, Bin
diff --git a/arch/x86/cpu/apollolake/fsp_s.c b/arch/x86/cpu/apollolake/fsp_s.c index 3a54297a28..e54b0ac104 100644 --- a/arch/x86/cpu/apollolake/fsp_s.c +++ b/arch/x86/cpu/apollolake/fsp_s.c @@ -160,11 +160,6 @@ int arch_fsps_preinit(void) ret = irq_first_device_type(X86_IRQT_ITSS, &itss); if (ret) return log_msg_ret("no itss", ret); - /* - * Snapshot the current GPIO IRQ polarities. FSP is setting a default - * policy that doesn't honour boards' requirements - */ - irq_snapshot_polarities(itss); /* * Clear the GPI interrupt status and enable registers. These @@ -203,7 +198,11 @@ int arch_fsp_init_r(void) ret = irq_first_device_type(X86_IRQT_ITSS, &itss); if (ret) return log_msg_ret("no itss", ret); - /* Restore GPIO IRQ polarities back to previous settings */ + + /* + * Restore GPIO IRQ polarities back to previous settings. This was + * stored in reserve_arch() - see X86_IRQT_ITSS + */ irq_restore_polarities(itss); /* soc_init() */ diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index baa7dae172..9bc243ebc8 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -25,6 +25,7 @@ #include <dm.h> #include <errno.h> #include <init.h> +#include <irq.h> #include <log.h> #include <malloc.h> #include <syscon.h> @@ -274,6 +275,9 @@ int cpu_init_r(void) #ifndef CONFIG_EFI_STUB int reserve_arch(void) { + struct udevice *itss; + int ret; + if (IS_ENABLED(CONFIG_ENABLE_MRC_CACHE)) mrccache_reserve(); @@ -291,6 +295,15 @@ int reserve_arch(void) fsp_save_s3_stack(); } } + ret = irq_first_device_type(X86_IRQT_ITSS, &itss); + if (!ret) { + /* + * Snapshot the current GPIO IRQ polarities. FSP-S is about to + * run and will set a default policy that doesn't honour boards' + * requirements + */ + irq_snapshot_polarities(itss); + } return 0; } diff --git a/arch/x86/cpu/intel_common/itss.c b/arch/x86/cpu/intel_common/itss.c index 963afa8f5b..fe84ebe29f 100644 --- a/arch/x86/cpu/intel_common/itss.c +++ b/arch/x86/cpu/intel_common/itss.c @@ -65,14 +65,23 @@ static int snapshot_polarities(struct udevice *dev) int i; reg_start = start / IRQS_PER_IPC; - reg_end = (end + IRQS_PER_IPC - 1) / IRQS_PER_IPC; + reg_end = DIV_ROUND_UP(end, IRQS_PER_IPC); + log_info("ITSS IRQ Polarities snapshot %p\n", priv->irq_snapshot); for (i = reg_start; i < reg_end; i++) { uint reg = PCR_ITSS_IPC0_CONF + sizeof(u32) * i; priv->irq_snapshot[i] = pcr_read32(dev, reg); + log_debug(" - %d, reg %x: irq_snapshot[i] %x\n", i, reg, + priv->irq_snapshot[i]); } + /* Save the snapshot for use after relocation */ + gd->start_addr_sp -= sizeof(*priv); + gd->start_addr_sp &= ~0xf; + gd->arch.itss_priv = (void *)gd->start_addr_sp; + memcpy(gd->arch.itss_priv, priv, sizeof(*priv)); + return 0; } @@ -91,16 +100,26 @@ static void show_polarities(struct udevice *dev, const char *msg) static int restore_polarities(struct udevice *dev) { struct itss_priv *priv = dev_get_priv(dev); + struct itss_priv *old_priv; const int start = GPIO_IRQ_START; const int end = GPIO_IRQ_END; int reg_start; int reg_end; int i; + /* Get the snapshot which was stored by the pre-reloc device */ + old_priv = gd->arch.itss_priv; + if (!old_priv) + return log_msg_ret("priv", -EFAULT); + memcpy(priv->irq_snapshot, old_priv->irq_snapshot, + sizeof(priv->irq_snapshot)); + show_polarities(dev, "Before"); + log_info("priv->irq_snapshot %p\n", priv->irq_snapshot); reg_start = start / IRQS_PER_IPC; - reg_end = (end + IRQS_PER_IPC - 1) / IRQS_PER_IPC; + reg_end = DIV_ROUND_UP(end, IRQS_PER_IPC); + for (i = reg_start; i < reg_end; i++) { u32 mask; @@ -125,6 +144,8 @@ static int restore_polarities(struct udevice *dev) mask &= ~((1U << irq_start) - 1); reg = PCR_ITSS_IPC0_CONF + sizeof(u32) * i; + log_debug(" - %d, reg %x: mask %x, irq_snapshot[i] %x\n", + i, reg, mask, priv->irq_snapshot[i]); pcr_clrsetbits32(dev, reg, mask, mask & priv->irq_snapshot[i]); } diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index 0e64c8a46d..5bc251c0dd 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -121,6 +121,7 @@ struct arch_global_data { #ifdef CONFIG_FSP_VERSION2 struct fsp_header *fsp_s_hdr; /* Pointer to FSP-S header */ #endif + void *itss_priv; /* Private ITSS data pointer */ ulong acpi_start; /* Start address of ACPI tables */ }; diff --git a/arch/x86/include/asm/itss.h b/arch/x86/include/asm/itss.h index c75d8fe8c2..f7d3240384 100644 --- a/arch/x86/include/asm/itss.h +++ b/arch/x86/include/asm/itss.h @@ -16,7 +16,7 @@ #define ITSS_MAX_IRQ 119 #define IRQS_PER_IPC 32 -#define NUM_IPC_REGS ((ITSS_MAX_IRQ + IRQS_PER_IPC - 1) / IRQS_PER_IPC) +#define NUM_IPC_REGS DIV_ROUND_UP(ITSS_MAX_IRQ, IRQS_PER_IPC) /* Max PXRC registers in ITSS */ #define MAX_PXRC_CONFIG (PCR_ITSS_PIRQH_ROUT - PCR_ITSS_PIRQA_ROUT + 1) diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c index 98bc79eaba..6b32b451a6 100644 --- a/drivers/misc/irq-uclass.c +++ b/drivers/misc/irq-uclass.c @@ -170,7 +170,7 @@ int irq_first_device_type(enum irq_dev_t type, struct udevice **devp) ret = uclass_first_device_drvdata(UCLASS_IRQ, type, devp); if (ret) - return log_msg_ret("find", ret); + return ret; return 0; }
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. Signed-off-by: Simon Glass <sjg at chromium.org> --- 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(-)