Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
[WIP] soc/amd/common: Use configure_scimap()
There is little point stashing the SCI trigger register configuration. Remove it to mase SCI and SMI programming more symmetrical.
Change-Id: Ie23da79546858282910db65182a6315ade506279 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/gpio_banks/gpio.c 1 file changed, 12 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/43012/1
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 1bcfc8b..141d04f 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -40,12 +40,6 @@ configure_gevent_smi(gevent_num, SMI_MODE_SMI, level); }
-struct sci_trigger_regs { - uint32_t mask; - uint32_t polarity; - uint32_t level; -}; - /* * For each general purpose event, GPE, the choice of edge/level triggered * event is represented as a single bit in SMI_SCI_LEVEL register. @@ -53,37 +47,24 @@ * In a similar fashion, polarity (rising/falling, hi/lo) of each GPE is * represented as a single bit in SMI_SCI_TRIG register. */ -static void fill_sci_trigger(uint32_t flags, int gpe, struct sci_trigger_regs *regs) +static void program_sci(uint32_t flags, int gevent_num) { - uint32_t mask = 1 << gpe; + struct sci_source sci;
- regs->mask |= mask; + sci.scimap = gevent_num; + sci.gpe = gevent_num;
if (is_gpio_event_level_triggered(flags)) - regs->level |= mask; + sci.level = SMI_SCI_LVL; else - regs->level &= ~mask; + sci.level = SMI_SCI_EDG;
if (is_gpio_event_active_high(flags)) - regs->polarity |= mask; + sci.direction = SMI_SCI_LVL_HIGH; else - regs->polarity &= ~mask; -} + sci.direction = SMI_SCI_LVL_LOW;
-/* TODO: See configure_scimap() implementations. */ -static void set_sci_trigger(const struct sci_trigger_regs *regs) -{ - uint32_t value; - - value = smi_read32(SMI_SCI_TRIG); - value &= ~regs->mask; - value |= regs->polarity; - smi_write32(SMI_SCI_TRIG, value); - - value = smi_read32(SMI_SCI_LEVEL); - value &= ~regs->mask; - value |= regs->level; - smi_write32(SMI_SCI_LEVEL, value); + configure_scimap(&sci); }
uintptr_t gpio_get_address(gpio_t gpio_num) @@ -175,7 +156,7 @@
__weak void soc_gpio_hook(uint8_t gpio, uint8_t mux) {}
-static void set_single_gpio(const struct soc_amd_gpio *g, struct sci_trigger_regs *sci_cfg) +static void set_single_gpio(const struct soc_amd_gpio *g) { static const struct soc_amd_event *gev_tbl; static size_t gev_items; @@ -206,14 +187,12 @@ if (g->flags & GPIO_FLAG_SMI) { program_smi(g->flags, gevent_num); } else if (g->flags & GPIO_FLAG_SCI) { - fill_sci_trigger(g->flags, gevent_num, sci_cfg); - soc_route_sci(gevent_num); + program_sci(g->flags, gevent_num); } }
void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { - struct sci_trigger_regs sci_trigger_cfg = { 0 }; size_t i;
/* @@ -229,7 +208,7 @@ master_switch_clr(GPIO_MASK_STS_EN | GPIO_INTERRUPT_EN);
for (i = 0; i < size; i++) - set_single_gpio(&gpio_list_ptr[i], &sci_trigger_cfg); + set_single_gpio(&gpio_list_ptr[i]);
/* * Re-enable interrupt status generation. @@ -239,9 +218,6 @@ * to be missed during boot. */ master_switch_set(GPIO_INTERRUPT_EN); - - /* Set all SCI trigger polarity (high/low) and level (edge/level). */ - set_sci_trigger(&sci_trigger_cfg); }
int gpio_interrupt_status(gpio_t gpio)
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43012
to look at the new patch set (#2).
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
[WIP] soc/amd/common: Use configure_scimap()
There is little point stashing the SCI trigger register configuration. Remove it to mase SCI and SMI programming more symmetrical.
Change-Id: Ie23da79546858282910db65182a6315ade506279 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/picasso/include/soc/smi.h M src/soc/amd/picasso/smi_util.c M src/soc/amd/stoneyridge/include/soc/smi.h M src/soc/amd/stoneyridge/smi_util.c M src/southbridge/amd/pi/hudson/soc/smi.h 6 files changed, 20 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/43012/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43012
to look at the new patch set (#3).
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
[WIP] soc/amd/common: Use configure_scimap()
There is little point stashing the SCI trigger register configuration. Remove it to mase SCI and SMI programming more symmetrical.
Change-Id: Ie23da79546858282910db65182a6315ade506279 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/picasso/include/soc/smi.h M src/soc/amd/picasso/smi_util.c M src/soc/amd/stoneyridge/include/soc/smi.h M src/soc/amd/stoneyridge/smi_util.c M src/southbridge/amd/pi/hudson/soc/smi.h 6 files changed, 20 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/43012/3
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
Patch Set 4:
With none of amd-acpimmio-alt getting reviews or moving forward, I'll abandon these later this week.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
Abandoned
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4: i'll have a look at this patch series after we can successfully boot into linux on amd/majolica; busy with getting that one to work right now. but it's on my todo list
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: [WIP] soc/amd/common: Use configure_scimap() ......................................................................
Restored
Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Felix Held. Felix Held has uploaded a new patch set (#5) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: soc/amd/common/block/gpio_banks: Use configure_scimap() ......................................................................
soc/amd/common/block/gpio_banks: Use configure_scimap()
There is no need to stash the SCI trigger register configuration and apply it at the end. Remove this to make SCI and SMI programming more symmetrical and to use available configure_scimap function instead of implementing it again, but without the additional checks. Using this function also allows removing soc_route_sci.
Change-Id: Ie23da79546858282910db65182a6315ade506279 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/smi.h M src/soc/amd/common/block/smi/smi_util.c 3 files changed, 12 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/43012/5
Attention is currently required from: Jason Glenesk, Raul Rangel, Kyösti Mälkki, Felix Held. Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: soc/amd/common/block/gpio_banks: Use configure_scimap() ......................................................................
Patch Set 5: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43012 )
Change subject: soc/amd/common/block/gpio_banks: Use configure_scimap() ......................................................................
soc/amd/common/block/gpio_banks: Use configure_scimap()
There is no need to stash the SCI trigger register configuration and apply it at the end. Remove this to make SCI and SMI programming more symmetrical and to use available configure_scimap function instead of implementing it again, but without the additional checks. Using this function also allows removing soc_route_sci.
Change-Id: Ie23da79546858282910db65182a6315ade506279 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/43012 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/gpio_banks/gpio.c M src/soc/amd/common/block/include/amdblocks/smi.h M src/soc/amd/common/block/smi/smi_util.c 3 files changed, 12 insertions(+), 46 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/common/block/gpio_banks/gpio.c b/src/soc/amd/common/block/gpio_banks/gpio.c index 1d664cb..711b779 100644 --- a/src/soc/amd/common/block/gpio_banks/gpio.c +++ b/src/soc/amd/common/block/gpio_banks/gpio.c @@ -45,12 +45,6 @@ configure_gevent_smi(gevent_num, SMI_MODE_SMI, level); }
-struct sci_trigger_regs { - uint32_t mask; - uint32_t polarity; - uint32_t level; -}; - /* * For each general purpose event, GPE, the choice of edge/level triggered * event is represented as a single bit in SMI_SCI_LEVEL register. @@ -58,37 +52,24 @@ * In a similar fashion, polarity (rising/falling, hi/lo) of each GPE is * represented as a single bit in SMI_SCI_TRIG register. */ -static void fill_sci_trigger(uint32_t flags, int gpe, struct sci_trigger_regs *regs) +static void program_sci(uint32_t flags, int gevent_num) { - uint32_t mask = 1 << gpe; + struct sci_source sci;
- regs->mask |= mask; + sci.scimap = gevent_num; + sci.gpe = gevent_num;
if (is_gpio_event_level_triggered(flags)) - regs->level |= mask; + sci.level = SMI_SCI_LVL; else - regs->level &= ~mask; + sci.level = SMI_SCI_EDG;
if (is_gpio_event_active_high(flags)) - regs->polarity |= mask; + sci.direction = SMI_SCI_LVL_HIGH; else - regs->polarity &= ~mask; -} + sci.direction = SMI_SCI_LVL_LOW;
-/* TODO: See configure_scimap() implementations. */ -static void set_sci_trigger(const struct sci_trigger_regs *regs) -{ - uint32_t value; - - value = smi_read32(SMI_SCI_TRIG); - value &= ~regs->mask; - value |= regs->polarity; - smi_write32(SMI_SCI_TRIG, value); - - value = smi_read32(SMI_SCI_LEVEL); - value &= ~regs->mask; - value |= regs->level; - smi_write32(SMI_SCI_LEVEL, value); + configure_scimap(&sci); }
uintptr_t gpio_get_address(gpio_t gpio_num) @@ -179,8 +160,7 @@ return gpio; }
-static void set_single_gpio(const struct soc_amd_gpio *g, - struct sci_trigger_regs *sci_trigger_cfg) +static void set_single_gpio(const struct soc_amd_gpio *g) { static const struct soc_amd_event *gev_tbl; static size_t gev_items; @@ -214,16 +194,12 @@ if (g->flags & GPIO_FLAG_SMI) { program_smi(g->flags, gevent_num); } else if (g->flags & GPIO_FLAG_SCI) { - fill_sci_trigger(g->flags, gevent_num, sci_trigger_cfg); - soc_route_sci(gevent_num); + program_sci(g->flags, gevent_num); } }
void program_gpios(const struct soc_amd_gpio *gpio_list_ptr, size_t size) { - struct sci_trigger_regs sci_trigger_cfg = { 0 }; - const bool can_set_smi_flags = !(CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) && - ENV_SEPARATE_VERSTAGE); size_t i;
if (!gpio_list_ptr || !size) @@ -242,7 +218,7 @@ master_switch_clr(GPIO_MASK_STS_EN | GPIO_INTERRUPT_EN);
for (i = 0; i < size; i++) - set_single_gpio(&gpio_list_ptr[i], &sci_trigger_cfg); + set_single_gpio(&gpio_list_ptr[i]);
/* * Re-enable interrupt status generation. @@ -252,10 +228,6 @@ * to be missed during boot. */ master_switch_set(GPIO_INTERRUPT_EN); - - /* Set all SCI trigger polarity (high/low) and level (edge/level). */ - if (can_set_smi_flags) - set_sci_trigger(&sci_trigger_cfg); }
int gpio_interrupt_status(gpio_t gpio) diff --git a/src/soc/amd/common/block/include/amdblocks/smi.h b/src/soc/amd/common/block/include/amdblocks/smi.h index 60602a8..b870f16 100644 --- a/src/soc/amd/common/block/include/amdblocks/smi.h +++ b/src/soc/amd/common/block/include/amdblocks/smi.h @@ -46,7 +46,6 @@ void configure_scimap(const struct sci_source *sci); void disable_gevent_smi(uint8_t gevent); void gpe_configure_sci(const struct sci_source *scis, size_t num_gpes); -void soc_route_sci(uint8_t event); void clear_all_smi_status(void); void clear_smi_sci_status(void);
diff --git a/src/soc/amd/common/block/smi/smi_util.c b/src/soc/amd/common/block/smi/smi_util.c index b763c3b..ac2f4b4 100644 --- a/src/soc/amd/common/block/smi/smi_util.c +++ b/src/soc/amd/common/block/smi/smi_util.c @@ -72,11 +72,6 @@ smi_write32(SMI_REG_SMITRIG0, reg); }
-void soc_route_sci(uint8_t event) -{ - smi_write8(SMI_SCI_MAP(event), event); -} - /** * Configure generation of SCIs. */
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)