Bernardo Perez Priego has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h M src/soc/intel/cannonlake/romstage/fsp_params.c 3 files changed, 20 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/1
diff --git a/src/mainboard/google/drallion/variants/drallion/gpio.c b/src/mainboard/google/drallion/variants/drallion/gpio.c index ebe5ee2..fc2c4dc 100644 --- a/src/mainboard/google/drallion/variants/drallion/gpio.c +++ b/src/mainboard/google/drallion/variants/drallion/gpio.c @@ -15,6 +15,10 @@
#include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <gpio.h> + +/* Sensor detection pin */ +#define SENSOR_DET_360 GPP_H5
/* Pad configuration in ramstage */ static const struct pad_config gpio_table[] = { @@ -277,3 +281,9 @@ *num = ARRAY_SIZE(cros_gpios); return cros_gpios; } + +int get_sensor_detect(void) +{ + gpio_input(SENSOR_DET_360); + return gpio_get(SENSOR_DET_360); +} diff --git a/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h b/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h index 20cfbb8..6b7b64a 100644 --- a/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h +++ b/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h @@ -32,10 +32,14 @@ #define GPIO_MEM_CONFIG_3 GPP_F15 #define GPIO_MEM_CONFIG_4 GPP_F16
+#define IS_ISH_DEVICE_ENABLED (get_sensor_detect() == 0) + const struct pad_config *variant_gpio_table(size_t *num); const struct pad_config *variant_early_gpio_table(size_t *num);
struct cros_gpio; const struct cros_gpio *variant_cros_gpios(size_t *num);
+int get_sensor_detect(void); + #endif diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 3ba997d..1b673d9 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -23,9 +23,14 @@ #include <soc/pci_devs.h> #include <soc/romstage.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <variant/gpio.h>
#include "../chip.h"
+#ifndef IS_ISH_DEVICE_ENABLED +#define IS_ISH_DEVICE_ENABLED (1) +#endif + static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, const config_t *config) { unsigned int i; @@ -82,7 +87,7 @@ if (!dev) m_cfg->PchIshEnable = 0; else - m_cfg->PchIshEnable = dev->enabled; + m_cfg->PchIshEnable = dev->enabled && IS_ISH_DEVICE_ENABLED;
/* If HDA is enabled, enable HDA elements */ dev = pcidev_path_on_root(PCH_DEVFN_HDA);
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#2).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/chromeos.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/vendorcode/google/chromeos/chromeos.h 3 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/2
Selma Bensaid has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 2:
Please use https://partnerissuetracker.corp.google.com/issues/140748790 for tracking this change.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35303/2//COMMIT_MSG@10 PS2, Line 10: add bug like this, BUG=b:140415892
AndreX Andraos has uploaded a new patch set (#3) to the change originally created by Bernardo Perez Priego. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790 Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/chromeos.c M src/soc/intel/cannonlake/romstage/fsp_params.c M src/vendorcode/google/chromeos/chromeos.h 3 files changed, 17 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 3: Code-Review-1
No need for weak. Please use mainboard_memory_init_params()
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... PS3, Line 28: /* Sensor detection pin */ : #define SENSOR_DET_360 GPP_H5 Put this in gpio.h
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... PS3, Line 134: gpio_input(SENSOR_DET_360); I think this should be set up in gpio.c
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... PS3, Line 134: gpio_input(SENSOR_DET_360);
I think this should be set up in gpio. […]
I left comment in GPIO change CL.
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, AndreX Andraos, Varun Joshi, Patrick Rudolph, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#4).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/romstage.c M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35303/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/4/src/mainboard/google/dralli... PS4, Line 61: FSP_M_CONFIG *fsp_m_cfg = &memupd->FspmConfig; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35303/4/src/mainboard/google/dralli... PS4, Line 61: FSP_M_CONFIG *fsp_m_cfg = &memupd->FspmConfig; please, no spaces at the start of a line
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, AndreX Andraos, Varun Joshi, Patrick Rudolph, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#5).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/romstage.c M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/5
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, AndreX Andraos, Varun Joshi, Patrick Rudolph, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#6).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/romstage.c M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 3 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 62: FSP_M_CONFIG *fsp_m_cfg = &memupd->FspmConfig; need consistent spacing around '*' (ctx:WxV)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 64: PchIshEnable This is already set in soc/intel/cannonlake/fsp_params.c based on dev->enabled. So, the only change you really need here is:
dev->enabled = is_ish_device_enabled(); where dev would be the ISH device.
However, looking at the flow in SoC, it looks like mainboard_memory_init_params() is called after soc_memory_init_params() is called. You will have to fix the order there to ensure that the mainboard call happens before.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 64: PchIshEnable
This is already set in soc/intel/cannonlake/fsp_params.c based on dev->enabled. […]
Isn't the devicetree read-only in romstage? How could you write dev->enable here?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 61: #if CONFIG(BOARD_GOOGLE_DRALLION) I prefer remove the sarien_cml and arcade_cml after drallion proto board came out. And we don't need to guard here.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 64: PchIshEnable
Isn't the devicetree read-only in romstage? […]
Oops. Yes, you are right. It won't be possible to set dev->enabled here. Scratch my comment.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 61: #if CONFIG(BOARD_GOOGLE_DRALLION)
I prefer remove the sarien_cml and arcade_cml after drallion proto board came out. […]
You can also add variant_memory_init_params(memupd) here and implement it only in variants/drallion. That will prevent having to update this later.
Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 61: #if CONFIG(BOARD_GOOGLE_DRALLION)
You can also add variant_memory_init_params(memupd) here and implement it only in variants/drallion. […]
- Does this mean that it will be OK to declare variant_memory_init_params as __weak?
- How about implementing a function pointer?
I would need to create one common header file at google/drallion folder, to declare the pointer and assign to NULL. And then specific drallion variant will set it to corresponding implementation.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 61: #if CONFIG(BOARD_GOOGLE_DRALLION)
- Does this mean that it will be OK to declare variant_memory_init_params as __weak? […]
Use of __weak should be totally fine. That is how we are handling variant_* specific functions in other mainboards. Example from Hatch: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
Hello Thejaswani Putta, Patrick Rudolph, EricR Lai, AndreX Andraos, Varun Joshi, Patrick Rudolph, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#7).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/romstage.c M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h M src/soc/intel/cannonlake/include/soc/romstage.h 4 files changed, 26 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/gpio.c:
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 291: FSP_M_CONFIG *fsp_m_cfg = &mupd->FspmConfig; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 292: if (fsp_m_cfg->PchIshEnable) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 292: if (fsp_m_cfg->PchIshEnable) suspect code indent for conditional statements (7, 15)
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 293: fsp_m_cfg->PchIshEnable = is_ish_device_enabled(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 293: fsp_m_cfg->PchIshEnable = is_ish_device_enabled(); please, no spaces at the start of a line
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/gpio.c:
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 283: int This can now be static. It is not used outside this file.
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... PS7, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd); You don't need this here. It can live in variants.h in mainboard/google/drallion.
Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... PS7, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd);
You don't need this here. It can live in variants.h in mainboard/google/drallion.
If I do not put it here, I will have to put it in every drallion's variant.h file, otherwise compiler complains that function is not defined. Any other suggestion?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... PS7, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd);
If I do not put it here, I will have to put it in every drallion's variant. […]
1. Create a baseboard/ directory under drallion/variants/ 2. Place a variants.h file under drallion/variants/baseboard/include/baseboard/ 3. Update Makefile.inc under drallion/ to include the newly added baseboard/ and CPPFLAGS_COMMON accordingly.
Hatch or octopus would be good examples to check.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/gpio.c:
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... PS8, Line 291: FSP_M_CONFIG *fsp_m_cfg = &mupd->FspmConfig; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... PS8, Line 292: if (fsp_m_cfg->PchIshEnable) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... PS8, Line 292: if (fsp_m_cfg->PchIshEnable) suspect code indent for conditional statements (7, 15)
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... PS8, Line 293: fsp_m_cfg->PchIshEnable = is_ish_device_enabled(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35303/8/src/mainboard/google/dralli... PS8, Line 293: fsp_m_cfg->PchIshEnable = is_ish_device_enabled(); please, no spaces at the start of a line
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 8: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/8/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/8/src/soc/intel/cannonlake/in... PS8, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd); no need for that as you call it from mainboard_memory_init_params
Hello Patrick Rudolph, Varun Joshi, Patrick Rudolph, Patrick Rudolph, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Thejaswani Putta, EricR Lai, AndreX Andraos, Mathew King,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#9).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/9
Hello Patrick Rudolph, Varun Joshi, Patrick Rudolph, Patrick Rudolph, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Thejaswani Putta, EricR Lai, AndreX Andraos, Mathew King, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#10).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/10
Hello Patrick Rudolph, Varun Joshi, Patrick Rudolph, Patrick Rudolph, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Thejaswani Putta, EricR Lai, AndreX Andraos, Mathew King, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#11).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/11
Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... PS7, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd);
- Create a baseboard/ directory under drallion/variants/ […]
Thanks for the guidelines, I have finished implementation.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 11: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 11: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/11/src/mainboard/google/drall... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/11/src/mainboard/google/drall... PS11, Line 22: nit: Maybe we could change the function name, ISH is not related to memory. We can use variant_mainboard_update_fspm/upd_params etc..
Hello Patrick Rudolph, Varun Joshi, Patrick Rudolph, Patrick Rudolph, Selma Bensaid, Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Thejaswani Putta, EricR Lai, AndreX Andraos, Mathew King, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35303
to look at the new patch set (#12).
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/12
Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35303/11/src/mainboard/google/drall... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/11/src/mainboard/google/drall... PS11, Line 22:
nit: Maybe we could change the function name, ISH is not related to memory. […]
Done
Thejaswani Putta has uploaded a new patch set (#13) to the change originally created by Bernardo Perez Priego. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/13
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 16:
@Intel, there was some overwrite from Patch 12 to Patch 13. Please change it back.
EricR Lai has uploaded a new patch set (#17) to the change originally created by Bernardo Perez Priego. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35303/17
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 17: Code-Review+2
Honor Furquan and Patrick CR+2.
Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 18: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
Patch Set 18:
(7 comments)
Please remember to mark comments "done" when you're done, or we can't submit the change.
https://review.coreboot.org/c/coreboot/+/35303/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35303/2//COMMIT_MSG@10 PS2, Line 10:
add bug like this, BUG=b:140415892
Done
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/chromeos.c:
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... PS3, Line 28: /* Sensor detection pin */ : #define SENSOR_DET_360 GPP_H5
Put this in gpio. […]
Done
https://review.coreboot.org/c/coreboot/+/35303/3/src/mainboard/google/dralli... PS3, Line 134: gpio_input(SENSOR_DET_360);
I left comment in GPIO change CL.
Done
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... File src/mainboard/google/drallion/romstage.c:
https://review.coreboot.org/c/coreboot/+/35303/6/src/mainboard/google/dralli... PS6, Line 61: #if CONFIG(BOARD_GOOGLE_DRALLION)
Use of __weak should be totally fine. […]
Done
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/gpio.c:
https://review.coreboot.org/c/coreboot/+/35303/7/src/mainboard/google/dralli... PS7, Line 283: int
This can now be static. It is not used outside this file.
Done
https://review.coreboot.org/c/coreboot/+/35303/8/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/8/src/soc/intel/cannonlake/in... PS8, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd);
no need for that as you call it from mainboard_memory_init_params
Done
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/romstage.h:
https://review.coreboot.org/c/coreboot/+/35303/7/src/soc/intel/cannonlake/in... PS7, Line 24: void variant_mainboard_memory_init_params(FSPM_UPD *mupd);
Thanks for the guidelines, I have finished implementation.
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35303 )
Change subject: mb/google/drallion: Enable 360 sensor detection ......................................................................
mb/google/drallion: Enable 360 sensor detection
Implementing logic to detect SKU model and enable ISH accordignly.
BUG=b:140748790
Change-Id: I22fafb43dce6545851883be556a02d65a01fc386 Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35303 Reviewed-by: Mathew King mathewk@chromium.org Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/drallion/Makefile.inc M src/mainboard/google/drallion/romstage.c A src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/drallion/variants/drallion/gpio.c M src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h 5 files changed, 50 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified EricR Lai: Looks good to me, approved Mathew King: Looks good to me, approved
diff --git a/src/mainboard/google/drallion/Makefile.inc b/src/mainboard/google/drallion/Makefile.inc index e7c90bb..ae8251e 100644 --- a/src/mainboard/google/drallion/Makefile.inc +++ b/src/mainboard/google/drallion/Makefile.inc @@ -34,6 +34,9 @@ romstage-y += ec.c verstage-y += ec.c
+subdirs-y += variants/baseboard +CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/baseboard/include + subdirs-y += variants/$(VARIANT_DIR) CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/$(VARIANT_DIR)/include
diff --git a/src/mainboard/google/drallion/romstage.c b/src/mainboard/google/drallion/romstage.c index 20eee7f..c9f009e 100644 --- a/src/mainboard/google/drallion/romstage.c +++ b/src/mainboard/google/drallion/romstage.c @@ -16,6 +16,9 @@ #include <ec/google/wilco/romstage.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> +#include <baseboard/variants.h> + +void __weak variant_mainboard_post_init_params(FSPM_UPD *mupd) {}
static const struct cnl_mb_cfg memcfg = { /* Access memory info through SMBUS. */ @@ -57,6 +60,8 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { + variant_mainboard_post_init_params(memupd); + wilco_ec_romstage_init();
cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); diff --git a/src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h new file mode 100644 index 0000000..1edd660 --- /dev/null +++ b/src/mainboard/google/drallion/variants/baseboard/include/baseboard/variants.h @@ -0,0 +1,23 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef BASEBOARD_VARIANTS_H +#define BASEBOARD_VARIANTS_H + +#include <fsp/api.h> + +void variant_mainboard_post_init_params(FSPM_UPD *mupd); + +#endif /* BASEBOARD_VARIANTS_H */ diff --git a/src/mainboard/google/drallion/variants/drallion/gpio.c b/src/mainboard/google/drallion/variants/drallion/gpio.c index 064f96c..5fba04b 100644 --- a/src/mainboard/google/drallion/variants/drallion/gpio.c +++ b/src/mainboard/google/drallion/variants/drallion/gpio.c @@ -15,6 +15,9 @@
#include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <gpio.h> +#include <soc/romstage.h> +#include <baseboard/variants.h>
/* Pad configuration in ramstage */ static const struct pad_config gpio_table[] = { @@ -271,3 +274,16 @@ *num = ARRAY_SIZE(cros_gpios); return cros_gpios; } + +static int is_ish_device_enabled(void) +{ + gpio_input(SENSOR_DET_360); + return gpio_get(SENSOR_DET_360) == 0; +} + +void variant_mainboard_post_init_params(FSPM_UPD *mupd) +{ + FSP_M_CONFIG *fsp_m_cfg = &mupd->FspmConfig; + if (fsp_m_cfg->PchIshEnable) + fsp_m_cfg->PchIshEnable = is_ish_device_enabled(); +} diff --git a/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h b/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h index 20cfbb8..251b40e 100644 --- a/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h +++ b/src/mainboard/google/drallion/variants/drallion/include/variant/gpio.h @@ -25,6 +25,9 @@ /* Recovery mode */ #define GPIO_REC_MODE GPP_E8
+/* Sensor detection pin */ +#define SENSOR_DET_360 GPP_H5 + /* Memory configuration board straps */ #define GPIO_MEM_CONFIG_0 GPP_F12 #define GPIO_MEM_CONFIG_1 GPP_F13
-
AndreX Andraos (Code Review) -
Bernardo Perez Priego (Code Review)
-
build bot (Jenkins) (Code Review)
-
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Mathew King (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Selma Bensaid (Code Review)
-
Thejaswani Putta (Code Review)