Change in ...coreboot[master]: soc/intel/cannonlake: Add required FSP UPD changes for CML
Hello Subrata Banik,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to review the following change.
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h M src/soc/intel/cannonlake/romstage/fsp_params.c 4 files changed, 62 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/1
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index cb9ad38..f30116b 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -3,7 +3,7 @@ * * Copyright (C) 2007-2008 coresystems GmbH * Copyright (C) 2014 Google Inc. - * Copyright (C) 2017-2018 Intel Corporation. + * Copyright (C) 2017-2019 Intel Corporation. * * 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 @@ -36,6 +36,8 @@ #include <soc/gpio_defs.h> #endif
+#define SOC_INTEL_UART_DEV_MAX 3 + struct soc_intel_cannonlake_config {
/* Common struct containing soc config data required by common code */ @@ -101,7 +103,7 @@ * For CNL, options are as following * When enabled, memory will be training at three different frequencies. * 0:Disabled, 1:FixedLow, 2:FixedMid, 3:FixedHigh, 4:Enabled - * For WHL/CFL options are as following + * For WHL/CFL/CML options are as following * When enabled, memory will be training at two different frequencies. * 0:Disabled, 1:FixedLow, 2:FixedHigh, 3:Enabled*/ enum { @@ -286,6 +288,19 @@ DebugConsent_XDP, /* XDP/Mipi60 */ DebugConsent_USB2_DBC, } DebugConsent; +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) + /* + * SerialIO device mode selection: + * PchSerialIoDisabled, + * PchSerialIoPci, + * PchSerialIoHidden, + * PchSerialIoLegacyUart, + * PchSerialIoSkipInit + */ + uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX]; + uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; + uint8_t SerialIoUartMode[SOC_INTEL_UART_DEV_MAX]; +#else /* * SerialIO device mode selection: * @@ -310,7 +325,7 @@ * PchSerialIoHidden */ uint8_t SerialIoDevMode[PchSerialIoIndexMAX]; - +#endif /* GPIO SD card detect pin */ unsigned int sdcard_cd_gpio;
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 866d9c8..ecd22d4 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2018 Intel Corporation. + * Copyright (C) 2018-2019 Intel Corporation. * * 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 @@ -34,6 +34,16 @@ }
const config_t *config = dev->chip_info; +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) + for (int i = 0; i < CONFIG_SOC_INTEL_I2C_DEV_MAX; i++) + params->SerialIoI2cMode[i] = config->SerialIoI2cMode[i]; + + for (int i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++) + params->SerialIoSpiMode[i] = config->SerialIoGSpiMode[i]; + + for (int i = 0; i < SOC_INTEL_UART_DEV_MAX; i++) + params->SerialIoUartMode[i] = config->SerialIoUartMode[i]; +#else const int SerialIoDev[] = { PCH_DEVFN_I2C0, PCH_DEVFN_I2C1, @@ -60,6 +70,7 @@ config->SerialIoDevMode[i] == PchSerialIoHidden) params->SerialIoDevMode[i] = config->SerialIoDevMode[i]; } +#endif }
/* UPD parameters to be initialized before SiliconInit */ @@ -165,8 +176,11 @@
/* Enable CNVi Wifi if enabled in device tree */ dev = dev_find_slot(0, PCH_DEVFN_CNViWIFI); +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) + params->CnviMode = dev->enabled; +#else params->PchCnviMode = dev->enabled; - +#endif /* PCI Express */ for (i = 0; i < ARRAY_SIZE(config->PcieClkSrcUsage); i++) { if (config->PcieClkSrcUsage[i] == 0) diff --git a/src/soc/intel/cannonlake/include/soc/serialio.h b/src/soc/intel/cannonlake/include/soc/serialio.h index e152770..fad7283 100644 --- a/src/soc/intel/cannonlake/include/soc/serialio.h +++ b/src/soc/intel/cannonlake/include/soc/serialio.h @@ -2,7 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2013 Google Inc. - * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2017-2019 Intel Corporation. * * 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 @@ -22,8 +22,33 @@ PchSerialIoPci, PchSerialIoAcpi, PchSerialIoHidden, +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) + PchSerialIoSkipInit +#endif } PCH_SERIAL_IO_MODE;
+#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) +enum { + PchSerialIoIndexI2C0, + PchSerialIoIndexI2C1, + PchSerialIoIndexI2C2, + PchSerialIoIndexI2C3, + PchSerialIoIndexI2C4, + PchSerialIoIndexI2C5 +}; + +enum { + PchSerialIoIndexGSPI0, + PchSerialIoIndexGSPI1, + PchSerialIoIndexGSPI2 +}; + +enum { + PchSerialIoIndexUART0, + PchSerialIoIndexUART1, + PchSerialIoIndexUART2 +}; +#else typedef enum { PchSerialIoIndexI2C0, PchSerialIoIndexI2C1, @@ -39,5 +64,6 @@ PchSerialIoIndexUART2, PchSerialIoIndexMAX } PCH_SERIAL_IO_CONTROLLER; +#endif
#endif diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index b8b2c17..c436e50 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2018 Intel Corp. + * Copyright (C) 2018-2019 Intel Corp. * * 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
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31284/2/src/soc/intel/cannonlake/romstage/fs... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31284/2/src/soc/intel/cannonlake/romstage/fs... PS2, Line 4: * Copyright (C) 2018-2019 Intel Corp. unrelated. you didn't change anything in this file
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h@302 PS4, Line 302: SOC_INTEL_UART_DEV_MAX better to define in SOC Kconfig, with rest of the LPSS controllers
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h@302 PS4, Line 302: SOC_INTEL_UART_DEV_MAX
better to define in SOC Kconfig, with rest of the LPSS controllers
Can you please check this: https://review.coreboot.org/#/c/coreboot/+/31213/
Furquan is looking at that way..
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/4/src/soc/intel/cannonlake/chip.h@302 PS4, Line 302: SOC_INTEL_UART_DEV_MAX
Can you please check this: […]
Ok, I see this has been already discussed :-)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/chip.h@39 PS5, Line 39: SOC_INTEL_UART_DEV_MAX SOC_INTEL_CML_UART_DEV_MAX?
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 39: params->SerialIoI2cMode[i] = config->SerialIoI2cMode[i]; Just use memcpy?
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 42: params->SerialIoSpiMode[i] = config->SerialIoGSpiMode[i]; same here?
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 45: params->SerialIoUartMode[i] = config->SerialIoUartMode[i]; same here.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 59 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/6
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/chip.h@39 PS5, Line 39: SOC_INTEL_UART_DEV_MAX
SOC_INTEL_CML_UART_DEV_MAX?
Done
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 39: params->SerialIoI2cMode[i] = config->SerialIoI2cMode[i];
Just use memcpy?
Done
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 42: params->SerialIoSpiMode[i] = config->SerialIoGSpiMode[i];
same here?
Done
https://review.coreboot.org/#/c/31284/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 45: params->SerialIoUartMode[i] = config->SerialIoUartMode[i];
same here.
Done
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 56 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 38: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, CONFIG_SOC_INTEL_I2C_DEV_MAX); line over 80 characters
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 39: memcpy(params->SerialIoSpiMode, config->SerialIoGSpiMode, CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX); line over 80 characters
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 40: memcpy(params->SerialIoUartMode, config->SerialIoUartMode, SOC_INTEL_CML_UART_DEV_MAX); line over 80 characters
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h@304 PS7, Line 304: uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX]; I think this file is a perfect place to abstract over the unnecessary differences in the CFL/CML UPD headers. You can keep this file as is and limit the changes to `fsp_params.c`. This way, our devicetree format would stay stable and we'd avoid the #if here (it's generally discouraged).
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 37: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) Please use a C `if` statement instead.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 38: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, CONFIG_SOC_INTEL_I2C_DEV_MAX); The original code below was explicitly written to allow the use of a devicetree-native `off` for the disabled state. Please maintain that functionality.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 184: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) C `if` please.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
PS7: I have a feeling that I commented on this on another change before. If this has to be in sync with FSP, please mention it in a comment. But I'd prefer not to draw the interface changes in FSP into the coreboot code. That one version of FSP names the same thing dif- ferently than another doesn't have to mean that we have to jump through hoops in coreboot.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... PS7, Line 26: PchSerialIoSkipInit AIUI, the last two are only available for UARTs (at least that's what the comments in FSP headers suggest). Maybe a comment on each value, when it is valid (serial i/o type, fsp version), would be better here than the #if.
And for the future, it would be nice if the FSP headers would ship such enums. Than we wouldn't have to keep things in sync manually, and without all the maintenance burden, would get everything ready much faster ;)
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h@304 PS7, Line 304: uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX];
I think this file is a perfect place to abstract over […]
Yes, It an make device tree struct constant but it'll be add more complexity in fsp_param.c. Also I feel that it would make the code difficult to understand. This change will provide clear picture of what changes are coming into new fsp.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 37: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
Please use a C `if` statement instead.
If we use C if statement, compilation would fail since fsp paramaters wouldn't be included in headers.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 38: memcpy(params->SerialIoI2cMode, config->SerialIoI2cMode, CONFIG_SOC_INTEL_I2C_DEV_MAX);
The original code below was explicitly written to allow the […]
Ack
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 184: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
C `if` please.
Compilation would fail since FSP header file will have only one of the parameter.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... PS7, Line 26: PchSerialIoSkipInit
AIUI, the last two are only available for UARTs (at least that's […]
It'll be difficult to add FSP version details since these changes are coming up with new CML FSP changes. I can surely pass your feedback regarding enum changes to our FSP team.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(3 comments)
Please don't draw every cosmetic change in FSP UPDs into our devicetrees. It makes things messy and in- compatible.
I don't object to such changes in general. But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/chip.h@304 PS7, Line 304: uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX];
Yes, It an make device tree struct constant but it'll be add more complexity in fsp_param.c.
Try to see it as a courtesy to Intel's customers. If you abstract here, they don't have to relearn everything with every FSP release. There might be even cases where somebody reuses a board design and only changes the SoC. In that case it would be very nice to keep the devicetree compatible.
Also I feel that it would make the code difficult to understand. This change will provide clear picture of what changes are coming into new fsp.
I see it much different. If you implement it here, you have a common place where you can see what changed in the FSP interface without technical reason. That would provide a clear picture of the extra burden brought in by the unstable FSP interface. I mean, it's just the names that changed, not the semantics. Sometimes it seems that some Intel employees do such changes just to show that they really worked on something (without thinking on the implications for the teams that have to incorporate FSP). Or maybe it's just Intel's way to slow their customer's projects down.
So please ask your FSP team fellows to align the UPDs for all FSPs that are supported here by soc/intel/cannonlake/. And remind them that such cosmetic changes create a lot of work for naught. Maybe there is still a chance that we don't have to drag around the #if mess here forever?
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/fsp_params.... PS7, Line 37: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
If we use C if statement, compilation would fail since fsp paramaters wouldn't be included in header […]
Ack, forgot about that.
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/7/src/soc/intel/cannonlake/include/soc... PS7, Line 26: PchSerialIoSkipInit
It'll be difficult to add FSP version details since these changes are coming up with new CML FSP cha […]
Yes, that's why I said "for the future", I know we can't fix it right now. Intel's FSP development process just slows things down, I've long accepted that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
(3 comments)
Please don't draw every cosmetic change in FSP UPDs into our devicetrees. It makes things messy and in- compatible.
I don't object to such changes in general. But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer.
CNL code base is there since 2016-17 time if i'm not wrong and we are in 2019, still making use of CNL code base for adding close possible platforms. Because coreboot touches very less Silicon programming and leave remaining on FSP. Consider the fact that intel FSP has wide range of customer from different OS background since CNL till now, every one not using this coreboot code base and FSP is not ideally "only" written to support coreboot hence expectation is wrong that FSP would consider all coreboot platform change and make an unified FSP header file so that all coming platform going to consume the same. I don't have problem with setting the expectation but just highlighting the reality.
And more over if you have concern that "your" devicetree is populated with FSP changes then we can come up with better planning in future and clean up that code. Anyway we are targeting for common code 2.0 soon FYI.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7: Code-Review-2
Please don't draw every cosmetic change in FSP UPDs into our devicetrees. It makes things messy and in- compatible.
I don't object to such changes in general. But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer.
CNL code base is there since 2016-17 time if i'm not wrong and we are in 2019, still making use of CNL code base for adding close possible platforms. Because coreboot touches very less Silicon programming and leave remaining on FSP. Consider the fact that intel FSP has wide range of customer from different OS background since CNL till now, every one not using this coreboot code base and FSP is not ideally "only" written to support coreboot hence expectation is wrong that FSP would consider all coreboot platform change and make an unified FSP header file so that all coming platform going to consume the same. I don't have problem with setting the expectation but just highlighting the reality.
You know what, I don't care. Intel wants FSP in coreboot instead of open-source code. So I'm asking Intel not to make a mess of it. I know that makes your job harder than it should be. The design and development process of FSP are not targeting open-source integration so it's pretty hard not to make a mess. But that's not due to decisions made by the coreboot community, is it?
And more over if you have concern that "your" devicetree is populated with FSP changes then we can come up with better planning in future and clean up that code.
Well, this change contains unnecessary changes to a core- boot API (`chip.h`) and the mentioned regression in `fsp_ params.c`. I don't see why we'd have to wait for that future?
Anyway we are targeting for common code 2.0 soon FYI.
Will you take advice from the coreboot community for that? Please start a ML discussion if so.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
Please don't draw every cosmetic change in FSP UPDs into our devicetrees. It makes things messy and in- compatible.
I don't object to such changes in general. But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer.
CNL code base is there since 2016-17 time if i'm not wrong and we
are in 2019, still making use of CNL code base for adding close possible platforms. Because coreboot touches very less Silicon programming and leave remaining on FSP. Consider the fact that intel FSP has wide range of customer from different OS background since CNL till now, every one not using this coreboot code base and FSP is not ideally "only" written to support coreboot hence expectation is wrong that FSP would consider all coreboot platform change and make an unified FSP header file so that all coming platform going to consume the same. I don't have problem with setting the expectation but just highlighting the reality.
You know what, I don't care. Intel wants FSP in coreboot instead of open-source code. So I'm asking Intel not to make a mess of it. I know that makes your job harder than it should be. The design and development process of FSP are not targeting open-source integration so it's pretty hard not to make a mess. But that's not due to decisions made by the coreboot community, is it?
And more over if you have concern that "your" devicetree is
populated with FSP changes then we can come up with better planning in future and clean up that code.
Well, this change contains unnecessary changes to a core- boot API (`chip.h`) and the mentioned regression in `fsp_ params.c`. I don't see why we'd have to wait for that future?
Anyway we are targeting for common code 2.0 soon FYI.
Will you take advice from the coreboot community for that? Please start a ML discussion if so.
@Nico, i'm requesting you to remove your personal battle between coreboot and FSP and consider your -2, this patch is nowhere eligible for -2, when i'm giving you complete inside and proposing to change the design ASAP. For now we are in middle of product and creating roadblock like this is against good development, do you want to should not push patch into coreboot.org? i think thats your only intention too me. This is going out of control for now.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
@Nico, i'm requesting you to remove your personal battle between coreboot and FSP and consider your -2, this patch is nowhere eligible for -2, when i'm giving you complete inside and proposing to change the design ASAP.
Subrata, -2 simply means the change should not be merged *as is*. When the author already agreed to change it, how is it not eligible? When I give a -2, I reconsider it on every following revision of the change. Even if it was just an update to the commit message to better reflect the changes.
For now we are in middle of product and creating roadblock like this is against good development,
When is anybody not in the middle of a product? With your argument the only right time to write propper code is during vacation? I don't see how this is a roadblock. You seem to have plenty of time to argue about 10 lines of code but zero time to change them???
do you want to should not push patch into coreboot.org? i think thats your only intention too me. This is going out of control for now.
Nope, rather the opposite. I appreciate your contributions to coreboot.org. I wouldn't spent hours of review every day if I wouldn't care about it.
That I gave the -2 was rather symbolic. You were talking about a potential future clean-up while it seemed to me that Maulik already agreed to update this change. I just want to make sure you're not whipping him not to update.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 7:
@Nico, i'm requesting you to remove your personal battle between
coreboot and FSP and consider your -2, this patch is nowhere eligible for -2, when i'm giving you complete inside and proposing to change the design ASAP.
Subrata, -2 simply means the change should not be merged *as is*. When the author already agreed to change it, how is it not eligible? When I give a -2, I reconsider it on every following revision of the change. Even if it was just an update to the commit message to better reflect the changes.
[Subrata] Nice to know this.
For now we are in middle of product and creating roadblock like
this is against good development,
When is anybody not in the middle of a product? With your argument the only right time to write propper code is during vacation? I don't see how this is a roadblock. You seem to have plenty of time to argue about 10 lines of code but zero time to change them???
[Subrata] If i had zero time to change then then wy should i let this patch open for 7 days to hear lots of things from you. I'm only requesting you to be reasonable and giving proper picture against your above comment
"But please ask your FSP team to do such cleanups to all FSP plat- forms in parallel so FSP users don't have to suffer."
My only intention to let you know that this time its not possible to make changes in FSP and its old code base but seems you "don't care about that". thats okay.
And i can point out many patch in cb.org those are not for sure meant for any product ? those are improvement. There is clearly difference between working on datelines vs wish to make things perfect.
do you want to should not push patch into coreboot.org? i think
thats your only intention too me. This is going out of control for now.
Nope, rather the opposite. I appreciate your contributions to coreboot.org. I wouldn't spent hours of review every day if I wouldn't care about it.
That I gave the -2 was rather symbolic. You were talking about a potential future clean-up while it seemed to me that Maulik already agreed to update this change. I just want to make sure you're not whipping him not to update.
[Subrata] I don't understand which part Maulik agrees to update in code based on your comment. i do care about this change as author of this CL.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 98 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/8
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
Patch Set 7:
Subrata, -2 simply means the change should not be merged *as is*.
[Subrata] Nice to know this.
Gerrit's semantics: -2 prevents a change from going in, while -1 is advisory only. Therefore for "should not be merged as is", -2 is the right action to take.
And yes, we expect developers who hand out -2 to track changes made to such CLs so they can remove it once the concerns are resolved, see https://doc.coreboot.org/getting_started/gerrit_guidelines.html?highlight=-2...
But more on this entire mess by email.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... PS9, Line 20: typedef enum { : PchSerialIoDisabled, : PchSerialIoPci, : PchSerialIoAcpi, : PchSerialIoHidden, : #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : PchSerialIoSkipInit : #endif : } PCH_SERIAL_IO_MODE; @Maulik: lets redefine this as below
#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) typedef enum { PchSerialIoDisabled, PchSerialIoPci, PchSerialIoHidden, PchSerialIoLegacyUart, PchSerialIoSkipInit } PCH_SERIAL_IO_MODE; #else typedef enum { PchSerialIoDisabled, PchSerialIoPci, PchSerialIoAcpi, PchSerialIoHidden, } PCH_SERIAL_IO_MODE; #endif
as I could see its been different in overall enum field.
Note: SerialIoSkipInit is no more only applicable for UART as part in FSP because you can use the same UPD value in order to skip I2C and SPI controller programming as well if decide to make the same in coreboot.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) Can we split this into separate functions to make it easier to read:
#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) parse_devicetree_cml(config); #else parse_devicetree_cnl_common(config); #endif
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
Can we split this into separate functions to make it easier to read: […]
good idea Furquan.
Also i have one open.
can we create something like soc/soctree.cb to move all FSP UPD related changes from devicetree.cb to soctree.cb ?
in that sense we might not need to copy those common FSP UPD across different mb/devicetree.cb
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... PS9, Line 20: typedef enum { : PchSerialIoDisabled, : PchSerialIoPci, : PchSerialIoAcpi, : PchSerialIoHidden, : #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : PchSerialIoSkipInit : #endif : } PCH_SERIAL_IO_MODE;
@Maulik: lets redefine this as below […]
Note above is just a proposal that I'm working internally to see if we can implement the same to reduce duplicate effort
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
good idea Furquan. […]
But those UPDs could have different values for different mainboards. How would common soctree.cb work?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
But those UPDs could have different values for different mainboards. How would common soctree. […]
we can take 2 steps path.
1. Common UPDs across all mainboards can move to soc/soctree.cb 2. UPDs are mb specific might still exist in mb/devicetree.cb
#2 is against what Nico is trying to achieve so I'm not sure if we can have other plan.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 129 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/10
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
we can take 2 steps path. […]
Done
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/include/soc... PS9, Line 20: typedef enum { : PchSerialIoDisabled, : PchSerialIoPci, : PchSerialIoAcpi, : PchSerialIoHidden, : #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : PchSerialIoSkipInit : #endif : } PCH_SERIAL_IO_MODE;
Note above is just a proposal that I'm working internally to see if we can implement the same to red […]
Done
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
@Furquan, I have made the changes as per comment. Can you please review it?
Also, TODO: check if we can move soc related UPD from device tree to common soc file and override board specific UPDs only from mb.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h@295 PS10, Line 295: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : /* : * SerialIO device mode selection: : * PchSerialIoDisabled, : * PchSerialIoPci, : * PchSerialIoHidden, : * PchSerialIoLegacyUart, : * PchSerialIoSkipInit : */ : uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX]; : uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; : uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX]; : #else Do we really need this? Can't we just have the same SerialIoDevMode[] and then handle it correctly within chip.c? It will avoid the #if here as well as in serialio.h for PchSerialIo*Index.
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
we can take 2 steps path. […]
I don't see a very good use-case for why we would need a soctree.cb. If the UPDs are common across all mainboards, why do we even need a config file in that case? Maybe it needs a wider discussion. If you have specific examples of what you think would be good candidates, you can start a CL or email thread to discuss it further.
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/fsp_params... PS10, Line 56: continue; don't need the continue here, you could just use else below. But I think this is being done for consistency with the _cnl_common function, so its okay either ways.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
I don't see a very good use-case for why we would need a soctree.cb. If the UPDs are common across all mainboards, why do we even need a config file in that case?
[Subrata] Few reasons: 1. Why do mainboard/devicetree.cb will have SOC related configuration. 2. Why do we want to make duplicate SOC configuration copy into all mb/devicetree.cb. I have made some study in existing coreboot code and looks like 80% FSP UPD exist in devicetree.cb are duplicated across all MB. 3. Fix it once and use it across, something like common code model, Hope you remember recent hatch PO issue related to SAGV UPD value. Looks like someone didn;t copy it correctly in mainboard devicetree.cb hence we need to suffer. But assume if this would be a common soc config then once its working for 1 mb, the same should work for all. 4. Code readability: Hope you have seen lots of hard coding in fsp_param.c file while assigning UPDs, we could refer to single config file like soc_devicetree.cb to make better readability.
Maybe it needs a wider discussion. If you have specific examples of what you think would be good candidates, you can start a CL or email thread to discuss it further.
[Subrata] We are working on proposal side and initial POC patch sets to see how it works. so far we are seeing some sconfig limitations with multiple .cb file between mainboards and soc. but i believe we can fix those.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h@295 PS10, Line 295: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : /* : * SerialIO device mode selection: : * PchSerialIoDisabled, : * PchSerialIoPci, : * PchSerialIoHidden, : * PchSerialIoLegacyUart, : * PchSerialIoSkipInit : */ : uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX]; : uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; : uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX]; : #else
Do we really need this? Can't we just have the same SerialIoDevMode[] and then handle it correctly w […]
we can do that i believe so that we can use old config structure as below.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/9/src/soc/intel/cannonlake/fsp_params.... PS9, Line 38: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
I don't see a very good use-case for why we would need a soctree.cb. If the UPDs are common across all mainboards, why do we even need a config file in that case?
[Subrata] Few reasons:
- Why do mainboard/devicetree.cb will have SOC related configuration.
I agree, we shouldn't have settings in mainboard/ that are not board specific. However, it may be hard to decide which settings that are. Sometimes, all boards already in the coreboot tree may just use the same setting by coincidence. It is hard to predict if the next board that will be added might need a different set- ting.
- Why do we want to make duplicate SOC configuration copy into all mb/devicetree.cb. I have made some study in existing coreboot code and looks like 80% FSP UPD exist in devicetree.cb are duplicated across all MB.
Yes, wold be really nice if we could reduce that.
- Code readability: Hope you have seen lots of hard coding in fsp_param.c file while assigning UPDs, we could refer to single config file like soc_devicetree.cb to make better readability.
I dare to object here. Whether the code in some `fsp_param.c` is better readable than some configuration file, depends much on your choice of local variable names and style in either file. For instance:
fsps->SomeSettingWithLongName = 0; fsps->AnotherSetting = { 2, 3 };
vs
register "SomeSettingWithLongName" = "0" register "AnotherSetting" = "{ 2, 3 }"
It doesn't make much of a difference, does it? Ofc. we could also make up a better configuration syntax, but I'm not sure if it's worth it.
If you keep the SoC specific UPD settings in C, you also avoid all the added declaration clutter (e.g. entry in chip.h) and maintenance burden added by that. So my proposal: Just reduce the entries in `chip.h` to the minimum existing boards need. If we add another board that needs a new entry, add it on demand. If we care, we could even implement the new entry in a way that it doesn't need to be added to existing boards (i.e. on the default 0 for that entry, set the old static value).
Ugh, so much written already. Furquan is right, we should continue on ML in case. It doesn't affect this change here, anyway.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h M src/soc/intel/cannonlake/romstage/fsp_params.c 4 files changed, 114 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/11
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/10/src/soc/intel/cannonlake/chip.h@295 PS10, Line 295: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) : /* : * SerialIO device mode selection: : * PchSerialIoDisabled, : * PchSerialIoPci, : * PchSerialIoHidden, : * PchSerialIoLegacyUart, : * PchSerialIoSkipInit : */ : uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX]; : uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; : uint8_t SerialIoUartMode[SOC_INTEL_CML_UART_DEV_MAX]; : #else
we can do that i believe so that we can use old config structure as below.
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/fsp_params... PS11, Line 69: config->SerialIoDevMode[SerialIoDevOffset];
80?
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/romstage/f... PS11, Line 109: remove ?
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 113 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/12
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/fsp_params... PS11, Line 69: config->SerialIoDevMode[SerialIoDevOffset];
80?
Done
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31284/11/src/soc/intel/cannonlake/romstage/f... PS11, Line 109:
remove ?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 47: dev_offset can also be just maintained across the three loops. Then you don't have to increment it in every iteration:
int i; int dev_offset = 0;
for (i = 0; i < CONFIG_SOC_INTEL_I2C_DEV_MAX; i++, dev_offset++) { }
for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, dev_offset++) { }
...
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 62: DevOffset dev_offset
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 78: DevOffset dev_offset
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#13).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 101 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/13
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 47:
dev_offset can also be just maintained across the three loops. […]
Done
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 62: DevOffset
dev_offset
Done
https://review.coreboot.org/#/c/31284/12/src/soc/intel/cannonlake/fsp_params... PS12, Line 78: DevOffset
dev_offset
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 13: Code-Review+1
(4 comments)
some minor nits.
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 28: SerialIoDev just use serial_io_dev
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 59: for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, dev_offset++) { line greater than 80?
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 65: params->SerialIoSpiMode[i] = config->SerialIoDevMode[dev_offset]; line greater than 80?
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 74: params->SerialIoUartMode[i] = config->SerialIoDevMode[dev_offset]; line greater than 80?
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 105 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... PS14, Line 59: for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, \ Avoid unnecessary line continuations
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 28: SerialIoDev
just use serial_io_dev
Done
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 59: for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, dev_offset++) {
line greater than 80?
Done
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 65: params->SerialIoSpiMode[i] = config->SerialIoDevMode[dev_offset];
line greater than 80?
Done
https://review.coreboot.org/#/c/31284/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 74: params->SerialIoUartMode[i] = config->SerialIoDevMode[dev_offset];
line greater than 80?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... PS14, Line 59: for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, \
Avoid unnecessary line continuations
As indicated, you don't need a line continuation here.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#15).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 105 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/15
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/14/src/soc/intel/cannonlake/fsp_params... PS14, Line 59: for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, \
As indicated, you don't need a line continuation here.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 15: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 78: config->SerialIoDevMode[dev_offset]; Here, you can both disable the device via `register "SerialIoDevMode"` in dt, and via `device pci ... off`. That differs from the existing logic, see below.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 95: params->SerialIoDevMode[i] = config->SerialIoDevMode[i]; Here, mode is "Pci" when device is set to `on` and config says `Disabled`. It seems intentional (allows a default "Pci" mode w/o mentioning the config in dt). Although, it doesn't match the `enum` name.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 110: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) Nit, if both implementations would share their name, this #if wouldn't be necessary.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif We have still `PchSerialIoHidden` two times with different meaning.
This and the other commented problem could be solved easily by a coreboot specific enum instead of an FSP specific one.
For instance,
enum { serial_io_default = 0, /* default if not mentioned in dt, same as pci */ serial_io_pci, serial_io_hidden_acpi, serial_io_hidden_legacy_uart, };
deliberately ignores `SkipInit` as that doesn't sound like something to decide in the dt. Code could work like this:
if (config->serial_io_mode == serial_io_default) params->SerialIoDevMode = serial_io_pci; else params->SerialIoDevMode = config->serial_io_mode;
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#16).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 112 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/16
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(4 comments)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 78: config->SerialIoDevMode[dev_offset];
Here, you can both disable the device via `register […]
True. Existing logic assigns default value as SerialIoPci in case no value assigned in dt. Also logic checks that value assigned are valid ones. I have updated new code to check validity as well as assign default value in case no values assigned in dt.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 95: params->SerialIoDevMode[i] = config->SerialIoDevMode[i];
Here, mode is "Pci" when device is set to `on` and config […]
True. Existing logic assigns default value as SerialIoPci in case no value assigned in dt. Also logic checks that value assigned are valid ones. I have updated new code to check validity as well as assign default value in case no values assigned in dt.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 110: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
Nit, if both implementations would share their name, this #if wouldn't be necessary.
Done
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
We have still `PchSerialIoHidden` two times with different […]
PchSerialIoHidden has 2 different value but does the same job. It'll communicate to fsp that SerialIo is hidden from pci bus. I feel that making enum changes in coreboot will create one more layer between device tree and fsp_param.c. Wouldn't it be simpler if we can pass values directly from dt to FSP? This way developer are also aware of what value they are passing to FSP.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(4 comments)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 95: params->SerialIoDevMode[i] = config->SerialIoDevMode[i];
True. Existing logic assigns default value as SerialIoPci in case no value assigned in dt. […]
In this version a `0` / PchSerialIoDisabled / unset was ignored but in the new version it is copied.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/fsp_params... PS15, Line 110: #if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE)
Done
Nice, thank you :)
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... PS16, Line 58: if (config->SerialIoDevMode[dev_offset] <= PchSerialIoSkipInit) A value of 0 (i.e. unset) is also <= PchSerialIoSkipInit and would override the Pci default from above.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
PchSerialIoHidden has 2 different value but does the same job. […]
What about PchSerialIoLegacyUart then? does it do the same job, too? If so, why are there two different values?
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
What about PchSerialIoLegacyUart then? does it do the same […]
PchSerialIoLegacyUart was introduced from CML FSP code and it is used to communicate to FSP that UART port should be used as a Legacy UART and not in LPSS mode. Note that value of enum is different because FSP code base is different across CNL and CML platforms. Since FSP code itself has change in enum value, coreboot need to keep that in sync with FSP.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
PchSerialIoLegacyUart was introduced from CML FSP code and it is used to communicate to FSP that UAR […]
I still doubt that anything changed (beside the additional `SkipInit`). We have for CML:
0:SerialIoUartDisabled, 1:SerialIoUartPci, 2:SerialIoUartHidden, 3:SerialIoUartCom, 4:SerialIoUartSkipInit
and for CNL/CFL:
0:Disabled, 1:PCI Mode, 2:Acpi mode, 3:Hidden mode (Legacy UART mode)
Maybe my interpretation is wrong, but in "Acpi mode", I'd expect the PCI device to be hidden (much like `SerialIoUartHidden`), both have a value of 2. Legacy serial ports were also called COM ports, hence "Hidden mode (Legacy UART mode)" and `SerialIoUartCom` seem the same to me, too, both with a value of 3.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... PS16, Line 58: if (config->SerialIoDevMode[dev_offset] <= PchSerialIoSkipInit)
A value of 0 (i.e. unset) is also <= PchSerialIoSkipInit and would […]
Is the concern here that uninitialized values in DT could result in unwanted side-effects by setting SerialIoXXXXMode incorrectly? It is possible to handle that case by simply adding a dummy index 0 value:
enum { PchSerialIoNotInitialized, PchSerialIoDisabled, ..., PchSerialIoMax, };
and then just subtract 1 when setting the UPD.
if (!dev || !dev->enabled) { ... = PchSerialIoDisabled; continue; }
if (config->SerialIoDevMode[dev_offset] == PchSerialIoNotInitialized || config->SerialIoDevMode[dev_offset] >= PchSerialIoMax) { ... = PchSerialIoPci; continue; }
... = config->SerialIoDevMode[dev_offset] - 1;
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
deliberately ignores `SkipInit` as that doesn't sound like
something to decide in the dt.
There can be cases where this will have to be done in DT. Mainboards might require certain blocks to be left as is and not initialized by FSP e.g. UART that is initialized by coreboot and doesn't need to be reinitialized by FSP. So, it will have to be done in DT.
Or consider the case where function 0 needs to be kept enabled even though it is unused just because you don't want function 1..n to be disabled. However, the GPIOs for function 0 are not used for native functions, but some other purpose and you don't want FSP to re-initialize or disable that block.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... PS16, Line 58: if (config->SerialIoDevMode[dev_offset] <= PchSerialIoSkipInit)
Is the concern here that uninitialized values in DT could result in unwanted side-effects by setting […]
I'm fine with any solution that either maintains the current behavior or makes dt writing more convenient.
A change in behavior should be mentioned in the commit message.
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/serialio.h:
https://review.coreboot.org/#/c/31284/15/src/soc/intel/cannonlake/include/so... PS15, Line 35: #endif
deliberately ignores `SkipInit` as that doesn't sound like […]
Wrt `SkipInit` I meant, I'd rather handle such things in soc code. i.e. if coreboot implements alternative code and that code is used, having to set `SkipInit` in the dt would be redundant. A function 0 shouldn't be hidden case, can be handled in soc code, too.
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/16/src/soc/intel/cannonlake/fsp_params... PS16, Line 58: if (config->SerialIoDevMode[dev_offset] <= PchSerialIoSkipInit)
I'm fine with any solution that either maintains the current behavior […]
Yes Furquan, I think this was concern that some random value might get filled into UPD value. I have done changes as per your suggestion. I'll also capture this into commit message.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#17).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC. Also this patch tries to create common parse logic for CometLake as well as cannonlake SOC.
We parse device tree parameters for PCI devices and fill values in FSP UPDs. We fill UPDs based on pci device config as well as SerialIoDev config of devicetree. For PCI devices, if PCI device is disabled from devicetree, we'll assign disable value to FSP UPD. In case devicetree doesn't fill this parameter or value is invalid in SerialIoDev config, default mode will be set to PCI. In case of valid value, we'll fill the same value into FSP UPD.
BUG=none BRANCH=none TEST=check if CML board boots and proper UPD values are filled.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 99 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 17:
(4 comments)
https://review.coreboot.org/#/c/31284/17/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/17/src/soc/intel/cannonlake/fsp_params... PS17, Line 43: static uint8_t get_param_value (const config_t *config, uint32_t dev_offset) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/31284/17/src/soc/intel/cannonlake/fsp_params... PS17, Line 48: if (!dev || !dev->enabled) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/31284/17/src/soc/intel/cannonlake/fsp_params... PS17, Line 53: (config->SerialIoDevMode[dev_offset] == PchSerialIoNotInitialized)){ space required before the open brace '{'
https://review.coreboot.org/#/c/31284/17/src/soc/intel/cannonlake/fsp_params... PS17, Line 88: for (int i = 0; i < ARRAY_SIZE(serial_io_dev); i++) { braces {} are not necessary for single statement blocks
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#18).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC. Also this patch tries to create common parse logic for CometLake as well as cannonlake SOC.
We parse device tree parameters for PCI devices and fill values in FSP UPDs. We fill UPDs based on pci device config as well as SerialIoDev config of devicetree. For PCI devices, if PCI device is disabled from devicetree, we'll assign disable value to FSP UPD. In case devicetree doesn't fill this parameter or value is invalid in SerialIoDev config, default mode will be set to PCI. In case of valid value, we'll fill the same value into FSP UPD.
BUG=none BRANCH=none TEST=check if CML board boots and proper UPD values are filled.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 96 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/18
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 18: Code-Review+1
Looks good but I have still doubts about the enum.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 18: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/31284/18/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/31284/18/src/soc/intel/cannonlake/chip.h@109 PS18, Line 109: enum { It would be good to mention somewhere what PchSerialIoNotInitialized is and what that option does.
https://review.coreboot.org/#/c/31284/18/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31284/18/src/soc/intel/cannonlake/fsp_params... PS18, Line 58: ( Not required.
Hello Naresh Solanki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Aamir Bohra, Patrick Rudolph, Duncan Laurie, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, V Sowmya, Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31284
to look at the new patch set (#19).
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC. Also this patch tries to create common parse logic for CometLake as well as cannonlake SOC.
We parse device tree parameters for PCI devices and fill values in FSP UPDs. We fill UPDs based on pci device config as well as SerialIoDev config of devicetree. For PCI devices, if PCI device is disabled from devicetree, we'll assign disable value to FSP UPD. In case devicetree doesn't fill this parameter or value is invalid in SerialIoDev config, default mode will be set to PCI. In case of valid value, we'll fill the same value into FSP UPD.
BUG=none BRANCH=none TEST=check if CML board boots and proper UPD values are filled.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 108 insertions(+), 30 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31284/19
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 19: Code-Review+2
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
Patch Set 19: Code-Review+2
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31284 )
Change subject: soc/intel/cannonlake: Add required FSP UPD changes for CML ......................................................................
soc/intel/cannonlake: Add required FSP UPD changes for CML
This patch adds required FSP UPD changes for CometLake SoC. Also this patch tries to create common parse logic for CometLake as well as cannonlake SOC.
We parse device tree parameters for PCI devices and fill values in FSP UPDs. We fill UPDs based on pci device config as well as SerialIoDev config of devicetree. For PCI devices, if PCI device is disabled from devicetree, we'll assign disable value to FSP UPD. In case devicetree doesn't fill this parameter or value is invalid in SerialIoDev config, default mode will be set to PCI. In case of valid value, we'll fill the same value into FSP UPD.
BUG=none BRANCH=none TEST=check if CML board boots and proper UPD values are filled.
Change-Id: Ib92b660409ab01d70358042b2ed29b8bf9cab26d Signed-off-by: Subrata Banik subrata.banik@intel.com Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31284 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: V Sowmya v.sowmya@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/include/soc/serialio.h 3 files changed, 108 insertions(+), 30 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 3e4bafc..b4d78f3 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -3,7 +3,7 @@ * * Copyright (C) 2007-2008 coresystems GmbH * Copyright (C) 2014 Google Inc. - * Copyright (C) 2017-2018 Intel Corporation. + * Copyright (C) 2017-2019 Intel Corporation. * * 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 @@ -36,6 +36,8 @@ #include <soc/gpio_defs.h> #endif
+#define SOC_INTEL_CML_UART_DEV_MAX 3 + struct soc_intel_cannonlake_config {
/* Common struct containing soc config data required by common code */ @@ -101,7 +103,7 @@ * For CNL, options are as following * When enabled, memory will be training at three different frequencies. * 0:Disabled, 1:FixedLow, 2:FixedMid, 3:FixedHigh, 4:Enabled - * For WHL/CFL options are as following + * For WHL/CFL/CML options are as following * When enabled, memory will be training at two different frequencies. * 0:Disabled, 1:FixedLow, 2:FixedHigh, 3:Enabled*/ enum { @@ -308,10 +310,30 @@ * PchSerialIoIndexUART2 * * Mode select: + * For Cannonlake PCH following values are supported: + * PchSerialIoNotInitialized * PchSerialIoDisabled * PchSerialIoPci * PchSerialIoAcpi * PchSerialIoHidden + * PchSerialIoMax + * + * For Cometlake following values are supported: + * PchSerialIoNotInitialized + * PchSerialIoDisabled, + * PchSerialIoPci, + * PchSerialIoHidden, + * PchSerialIoLegacyUart, + * PchSerialIoSkipInit, + * PchSerialIoMax + * + * NOTE: + * PchSerialIoNotInitialized is not an option provided by FSP, this + * option is default selected in case devicetree doesn't fill this param + * In case PchSerialIoNotInitialized is selected or an invalid value is + * provided from devicetree, coreboot will configure device into PCI + * mode by default. + * */ uint8_t SerialIoDevMode[PchSerialIoIndexMAX];
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 318b8a2..e3a2310 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2018 Intel Corporation. + * Copyright (C) 2018-2019 Intel Corporation. * * 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 @@ -25,6 +25,69 @@ #include <soc/ramstage.h> #include <string.h>
+static const int serial_io_dev[] = { + PCH_DEVFN_I2C0, + PCH_DEVFN_I2C1, + PCH_DEVFN_I2C2, + PCH_DEVFN_I2C3, + PCH_DEVFN_I2C4, + PCH_DEVFN_I2C5, + PCH_DEVFN_GSPI0, + PCH_DEVFN_GSPI1, + PCH_DEVFN_GSPI2, + PCH_DEVFN_UART0, + PCH_DEVFN_UART1, + PCH_DEVFN_UART2 +}; + +static uint8_t get_param_value(const config_t *config, uint32_t dev_offset) +{ + struct device *dev; + + dev = dev_find_slot(0, serial_io_dev[dev_offset]); + if (!dev || !dev->enabled) + return PchSerialIoDisabled; + + if ((config->SerialIoDevMode[dev_offset] >= PchSerialIoMax) || + (config->SerialIoDevMode[dev_offset] == PchSerialIoNotInitialized)) + return PchSerialIoPci; + + /* + * Correct Enum index starts from 1, so subtract 1 while returning value + */ + return config->SerialIoDevMode[dev_offset] - 1; +} + +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) +static void parse_devicetree_param(const config_t *config, FSP_S_CONFIG *params) +{ + uint32_t dev_offset = 0; + uint32_t i = 0; + + for (i = 0; i < CONFIG_SOC_INTEL_I2C_DEV_MAX; i++, dev_offset++) { + params->SerialIoI2cMode[i] = + get_param_value(config, dev_offset); + } + + for (i = 0; i < CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX; i++, + dev_offset++) { + params->SerialIoSpiMode[i] = + get_param_value(config, dev_offset); + } + + for (i = 0; i < SOC_INTEL_CML_UART_DEV_MAX; i++, dev_offset++) { + params->SerialIoUartMode[i] = + get_param_value(config, dev_offset); + } +} +#else +static void parse_devicetree_param(const config_t *config, FSP_S_CONFIG *params) +{ + for (int i = 0; i < ARRAY_SIZE(serial_io_dev); i++) + params->SerialIoDevMode[i] = get_param_value(config, i); +} +#endif + static void parse_devicetree(FSP_S_CONFIG *params) { struct device *dev = SA_DEV_ROOT; @@ -34,32 +97,8 @@ }
const config_t *config = dev->chip_info; - const int SerialIoDev[] = { - PCH_DEVFN_I2C0, - PCH_DEVFN_I2C1, - PCH_DEVFN_I2C2, - PCH_DEVFN_I2C3, - PCH_DEVFN_I2C4, - PCH_DEVFN_I2C5, - PCH_DEVFN_GSPI0, - PCH_DEVFN_GSPI1, - PCH_DEVFN_GSPI2, - PCH_DEVFN_UART0, - PCH_DEVFN_UART1, - PCH_DEVFN_UART2 - };
- for (int i = 0; i < ARRAY_SIZE(SerialIoDev); i++) { - dev = dev_find_slot(0, SerialIoDev[i]); - if (!dev->enabled) { - params->SerialIoDevMode[i] = PchSerialIoDisabled; - continue; - } - params->SerialIoDevMode[i] = PchSerialIoPci; - if (config->SerialIoDevMode[i] == PchSerialIoAcpi || - config->SerialIoDevMode[i] == PchSerialIoHidden) - params->SerialIoDevMode[i] = config->SerialIoDevMode[i]; - } + parse_devicetree_param(config, params); }
/* UPD parameters to be initialized before SiliconInit */ @@ -175,8 +214,11 @@
/* Enable CNVi Wifi if enabled in device tree */ dev = dev_find_slot(0, PCH_DEVFN_CNViWIFI); +#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) + params->CnviMode = dev->enabled; +#else params->PchCnviMode = dev->enabled; - +#endif /* PCI Express */ for (i = 0; i < ARRAY_SIZE(config->PcieClkSrcUsage); i++) { if (config->PcieClkSrcUsage[i] == 0) diff --git a/src/soc/intel/cannonlake/include/soc/serialio.h b/src/soc/intel/cannonlake/include/soc/serialio.h index e152770..6c95356 100644 --- a/src/soc/intel/cannonlake/include/soc/serialio.h +++ b/src/soc/intel/cannonlake/include/soc/serialio.h @@ -2,7 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2013 Google Inc. - * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2017-2019 Intel Corporation. * * 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 @@ -17,12 +17,26 @@ #ifndef _SERIALIO_H_ #define _SERIALIO_H_
+#if IS_ENABLED(CONFIG_SOC_INTEL_COMETLAKE) typedef enum { + PchSerialIoNotInitialized, + PchSerialIoDisabled, + PchSerialIoPci, + PchSerialIoHidden, + PchSerialIoLegacyUart, + PchSerialIoSkipInit, + PchSerialIoMax, +} PCH_SERIAL_IO_MODE; +#else +typedef enum { + PchSerialIoNotInitialized, PchSerialIoDisabled, PchSerialIoPci, PchSerialIoAcpi, PchSerialIoHidden, + PchSerialIoMax, } PCH_SERIAL_IO_MODE; +#endif
typedef enum { PchSerialIoIndexI2C0,
-
Aamir Bohra (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Maulik V Vaghela (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)