Change in coreboot[master]: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null
Use either a regular null check or `config_of` to avoid bugs.
Change-Id: I36a01b898c3e62423f27c2940b5f875b73e36950 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/fadt.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/serialio.c M src/southbridge/intel/lynxpoint/usb_xhci.c 4 files changed, 7 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46665/1
diff --git a/src/southbridge/intel/lynxpoint/fadt.c b/src/southbridge/intel/lynxpoint/fadt.c index 9ae6d34..dee0601 100644 --- a/src/southbridge/intel/lynxpoint/fadt.c +++ b/src/southbridge/intel/lynxpoint/fadt.c @@ -61,7 +61,7 @@ ACPI_FADT_S4_RTC_WAKE | ACPI_FADT_PLATFORM_CLOCK;
- if (cfg->docking_supported) + if (cfg && cfg->docking_supported) fadt->flags |= ACPI_FADT_DOCKING_SUPPORTED;
fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO; diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index a161087..49a5226 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -141,7 +141,7 @@ static void pch_gpi_routing(struct device *dev) { /* Get the chip configuration */ - config_t *config = dev->chip_info; + config_t *config = config_of(dev); u32 reg32 = 0;
/* An array would be much nicer here, or some @@ -174,7 +174,7 @@ u32 reg32; const char *state; /* Get the chip configuration */ - config_t *config = dev->chip_info; + config_t *config = config_of(dev); u16 pmbase = get_pmbase(); int pwr_on = CONFIG_MAINBOARD_POWER_FAILURE_STATE; int nmi_option; @@ -282,7 +282,7 @@ /* LynxPoint LP PCH Power Management init */ static void lpt_lp_pm_init(struct device *dev) { - struct southbridge_intel_lynxpoint_config *config = dev->chip_info; + struct southbridge_intel_lynxpoint_config *config = config_of(dev); u32 data;
printk(BIOS_DEBUG, "LynxPoint LP PM init\n"); @@ -636,7 +636,7 @@ static void pch_lpc_add_io_resources(struct device *dev) { struct resource *res; - config_t *config = dev->chip_info; + config_t *config = config_of(dev);
/* Add the default claimed IO range for the LPC device. */ res = new_resource(dev, 0); diff --git a/src/southbridge/intel/lynxpoint/serialio.c b/src/southbridge/intel/lynxpoint/serialio.c index 224e0f4..7af8a77 100644 --- a/src/southbridge/intel/lynxpoint/serialio.c +++ b/src/southbridge/intel/lynxpoint/serialio.c @@ -131,7 +131,7 @@
static void serialio_init(struct device *dev) { - struct southbridge_intel_lynxpoint_config *config = dev->chip_info; + struct southbridge_intel_lynxpoint_config *config = config_of(dev); struct resource *bar0, *bar1; int sio_index = -1;
diff --git a/src/southbridge/intel/lynxpoint/usb_xhci.c b/src/southbridge/intel/lynxpoint/usb_xhci.c index 60312a4..b59b433 100644 --- a/src/southbridge/intel/lynxpoint/usb_xhci.c +++ b/src/southbridge/intel/lynxpoint/usb_xhci.c @@ -328,7 +328,7 @@ /* Reset ports that are disabled or * polling before returning to the OS. */ usb_xhci_reset_usb3(dev, 0); - } else if (config->xhci_default) { + } else if (config && config->xhci_default) { /* Route all ports to XHCI */ apm_control(APM_CNT_ROUTE_ALL_XHCI); }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
Patch Set 2:
(2 comments)
You may notice that I'm generally not convinced of the use of config_of(). I think it's really only useful for the let's-try-to-find-something-in-the-devtree-and-ignore-NULL style.
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lpc.c:
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 144: config_t *config = config_of(dev); I guess that we didn't call this for PCH-H all the time proves that it's not needed to boot. So how about
if (!config) return;
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 177: config_t *config = config_of(dev); Or guard the two statements that use it?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lpc.c:
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 144: config_t *config = config_of(dev);
I guess that we didn't call this for PCH-H all the time proves that it's […]
Since this is only called from a single place, I can eliminate the check from here completely and rely on the caller having asserted that the config is non-null.
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 250: if (pch_is_lp()) : pch_gpi_routing(dev); : : /* GPE setup based on device tree configuration */ : enable_all_gpe(config->gpe0_en_1, config->gpe0_en_2, : config->gpe0_en_3, config->gpe0_en_4); : : /* SMI setup based on device tree configuration */ : enable_alt_smi(config->alt_gp_smi_en); Just guard this part.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46665
to look at the new patch set (#3).
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null
Use either a regular null check or `config_of` to avoid bugs.
Change-Id: I36a01b898c3e62423f27c2940b5f875b73e36950 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/fadt.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/serialio.c M src/southbridge/intel/lynxpoint/usb_xhci.c 4 files changed, 28 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46665/3
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46665
to look at the new patch set (#4).
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null
Use either a regular null check or `config_of` to avoid bugs.
Change-Id: I36a01b898c3e62423f27c2940b5f875b73e36950 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/southbridge/intel/lynxpoint/fadt.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/serialio.c M src/southbridge/intel/lynxpoint/usb_xhci.c 4 files changed, 28 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/46665/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lpc.c:
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 144: config_t *config = config_of(dev);
Since this is only called from a single place, I can eliminate the check from here completely and re […]
I decided to pass in the chip config as a parameter.
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 177: config_t *config = config_of(dev);
Or guard the two statements that use it?
Done
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 250: if (pch_is_lp()) : pch_gpi_routing(dev); : : /* GPE setup based on device tree configuration */ : enable_all_gpe(config->gpe0_en_1, config->gpe0_en_2, : config->gpe0_en_3, config->gpe0_en_4); : : /* SMI setup based on device tree configuration */ : enable_alt_smi(config->alt_gp_smi_en);
Just guard this part.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/lpc.c:
https://review.coreboot.org/c/coreboot/+/46665/2/src/southbridge/intel/lynxp... PS2, Line 144: config_t *config = config_of(dev);
Since this is only called from a single place, I can eliminate the check from here completely and re […]
Ack
I wonder how I could miss it, isn't everything called from lpc_init() with the same `dev` pointer?
https://review.coreboot.org/c/coreboot/+/46665/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/serialio.c:
https://review.coreboot.org/c/coreboot/+/46665/3/src/southbridge/intel/lynxp... PS3, Line 134: struct southbridge_intel_lynxpoint_config *config = config_of(dev); Or bail out if it's NULL?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46665/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/serialio.c:
https://review.coreboot.org/c/coreboot/+/46665/3/src/southbridge/intel/lynxp... PS3, Line 134: struct southbridge_intel_lynxpoint_config *config = config_of(dev);
Or bail out if it's NULL?
I guess it's also an option, but can't readily test it.
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46665 )
Change subject: sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null ......................................................................
sb/intel/lynxpoint: Ensure that `dev->chip_info` is not null
Use either a regular null check or `config_of` to avoid bugs.
Change-Id: I36a01b898c3e62423f27c2940b5f875b73e36950 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46665 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/southbridge/intel/lynxpoint/fadt.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/serialio.c M src/southbridge/intel/lynxpoint/usb_xhci.c 4 files changed, 28 insertions(+), 26 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/southbridge/intel/lynxpoint/fadt.c b/src/southbridge/intel/lynxpoint/fadt.c index 9ae6d34..dee0601 100644 --- a/src/southbridge/intel/lynxpoint/fadt.c +++ b/src/southbridge/intel/lynxpoint/fadt.c @@ -61,7 +61,7 @@ ACPI_FADT_S4_RTC_WAKE | ACPI_FADT_PLATFORM_CLOCK;
- if (cfg->docking_supported) + if (cfg && cfg->docking_supported) fadt->flags |= ACPI_FADT_DOCKING_SUPPORTED;
fadt->x_pm1a_evt_blk.space_id = ACPI_ADDRESS_SPACE_IO; diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index 5108fa5..29cd53f 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -138,10 +138,8 @@ } }
-static void pch_gpi_routing(struct device *dev) +static void pch_gpi_routing(struct device *dev, config_t *config) { - /* Get the chip configuration */ - config_t *config = dev->chip_info; u32 reg32 = 0;
/* An array would be much nicer here, or some @@ -173,8 +171,6 @@ u16 reg16; u32 reg32; const char *state; - /* Get the chip configuration */ - config_t *config = dev->chip_info; u16 pmbase = get_pmbase(); int pwr_on = CONFIG_MAINBOARD_POWER_FAILURE_STATE; int nmi_option; @@ -243,19 +239,23 @@ reg16 &= ~(1 << 10); // Disable BIOS_PCI_EXP_EN for native PME pci_write_config16(dev, GEN_PMCON_1, reg16);
- /* - * Set the board's GPI routing on LynxPoint-H. - * This is done as part of GPIO configuration on LynxPoint-LP. - */ - if (pch_is_lp()) - pch_gpi_routing(dev); + if (dev->chip_info) { + config_t *config = dev->chip_info;
- /* GPE setup based on device tree configuration */ - enable_all_gpe(config->gpe0_en_1, config->gpe0_en_2, - config->gpe0_en_3, config->gpe0_en_4); + /* + * Set the board's GPI routing on LynxPoint-H. + * This is done as part of GPIO configuration on LynxPoint-LP. + */ + if (pch_is_lp()) + pch_gpi_routing(dev, config);
- /* SMI setup based on device tree configuration */ - enable_alt_smi(config->alt_gp_smi_en); + /* GPE setup based on device tree configuration */ + enable_all_gpe(config->gpe0_en_1, config->gpe0_en_2, + config->gpe0_en_3, config->gpe0_en_4); + + /* SMI setup based on device tree configuration */ + enable_alt_smi(config->alt_gp_smi_en); + }
/* Set up power management block and determine sleep mode */ reg32 = inl(pmbase + 0x04); // PM1_CNT @@ -345,10 +345,10 @@ /* Set RCBA CIR28 0x3A84 based on SATA port enables */ data = 0x00001005; /* Port 3 and 2 disabled */ - if ((config->sata_port_map & ((1 << 3) | (1 << 2))) == 0) + if (config && (config->sata_port_map & ((1 << 3) | (1 << 2))) == 0) data |= (1 << 24) | (1 << 26); /* Port 1 and 0 disabled */ - if ((config->sata_port_map & ((1 << 1) | (1 << 0))) == 0) + if (config && (config->sata_port_map & ((1 << 1) | (1 << 0))) == 0) data |= (1 << 20) | (1 << 18); RCBA32(0x3a84) = data;
@@ -636,7 +636,6 @@ static void pch_lpc_add_io_resources(struct device *dev) { struct resource *res; - config_t *config = dev->chip_info;
/* Add the default claimed IO range for the LPC device. */ res = new_resource(dev, 0); @@ -651,10 +650,13 @@ pch_lpc_add_io_resource(dev, get_pmbase(), 256, PMBASE);
/* LPC Generic IO Decode range. */ - pch_lpc_add_gen_io_resources(dev, config->gen1_dec, LPC_GEN1_DEC); - pch_lpc_add_gen_io_resources(dev, config->gen2_dec, LPC_GEN2_DEC); - pch_lpc_add_gen_io_resources(dev, config->gen3_dec, LPC_GEN3_DEC); - pch_lpc_add_gen_io_resources(dev, config->gen4_dec, LPC_GEN4_DEC); + if (dev->chip_info) { + config_t *config = dev->chip_info; + pch_lpc_add_gen_io_resources(dev, config->gen1_dec, LPC_GEN1_DEC); + pch_lpc_add_gen_io_resources(dev, config->gen2_dec, LPC_GEN2_DEC); + pch_lpc_add_gen_io_resources(dev, config->gen3_dec, LPC_GEN3_DEC); + pch_lpc_add_gen_io_resources(dev, config->gen4_dec, LPC_GEN4_DEC); + } }
static void pch_lpc_read_resources(struct device *dev) diff --git a/src/southbridge/intel/lynxpoint/serialio.c b/src/southbridge/intel/lynxpoint/serialio.c index 4fb84a5..199cf9f 100644 --- a/src/southbridge/intel/lynxpoint/serialio.c +++ b/src/southbridge/intel/lynxpoint/serialio.c @@ -131,7 +131,7 @@
static void serialio_init(struct device *dev) { - struct southbridge_intel_lynxpoint_config *config = dev->chip_info; + struct southbridge_intel_lynxpoint_config *config = config_of(dev); struct resource *bar0, *bar1; int sio_index = -1;
diff --git a/src/southbridge/intel/lynxpoint/usb_xhci.c b/src/southbridge/intel/lynxpoint/usb_xhci.c index a6c9eb1..6a58d41 100644 --- a/src/southbridge/intel/lynxpoint/usb_xhci.c +++ b/src/southbridge/intel/lynxpoint/usb_xhci.c @@ -328,7 +328,7 @@ /* Reset ports that are disabled or * polling before returning to the OS. */ usb_xhci_reset_usb3(dev, 0); - } else if (config->xhci_default) { + } else if (config && config->xhci_default) { /* Route all ports to XHCI */ apm_control(APM_CNT_ROUTE_ALL_XHCI); }
-
Angel Pons (Code Review) -
Nico Huber (Code Review)