Change in coreboot[master]: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Hello Usha P,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to review the following change.
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current Pin-Ctrl kernel driver expects coreboot to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation in updated to consume community based GPIO ACPI device, updaing the current ACPI code to comply with pin-ctrl driver requirement.
BUG=150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Signed-off-by: Usha P usha.p@intel.com Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/1
diff --git a/src/soc/intel/tigerlake/acpi/gpio.asl b/src/soc/intel/tigerlake/acpi/gpio.asl index 0378b52..90f4632 100644 --- a/src/soc/intel/tigerlake/acpi/gpio.asl +++ b/src/soc/intel/tigerlake/acpi/gpio.asl @@ -12,145 +12,9 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ -#include <soc/gpio_defs.h> -#include <soc/irq.h> -#include <soc/pcr_ids.h> -#include <intelblocks/gpio.h> -#include "gpio_op.asl"
-Device (GCM0) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 0) - Name (_DDN, "GPIO Controller Community 0") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM0) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { - CreateDWordField (^RBUF, ^COM0._BAS, BAS0) - BAS0 = ^^PCRB (PID_GPIOCOM0) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} - -Device (GCM1) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 1) - Name (_DDN, "GPIO Controller Community 1") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM1) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { - CreateDWordField (^RBUF, ^COM1._BAS, BAS1) - BAS1 = ^^PCRB (PID_GPIOCOM1) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} - -Device (GCM4) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 4) - Name (_DDN, "GPIO Controller Community 4") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM4) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { - CreateDWordField (^RBUF, ^COM4._BAS, BAS4) - BAS4 = ^^PCRB (PID_GPIOCOM4) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} - -Device (GCM5) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 5) - Name (_DDN, "GPIO Controller Community 5") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM5) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { - CreateDWordField (^RBUF, ^COM5._BAS, BAS5) - BAS5 = ^^PCRB (PID_GPIOCOM5) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} - -/* - * Get GPIO DW0 Address - * Arg0 - GPIO Number - */ -Method (GADD, 1, NotSerialized) -{ - /* GPIO Community 0 */ - If (Arg0 >= GPIO_COM0_START && Arg0 <= GPIO_COM0_END) - { - Local0 = PID_GPIOCOM0 - Local1 = Arg0 - GPIO_COM0_START - } - /* GPIO Community 1 */ - If (Arg0 >= GPIO_COM1_START && Arg0 <= GPIO_COM1_END) - { - Local0 = PID_GPIOCOM1 - Local1 = Arg0 - GPIO_COM1_START - } - /* GPIO Community 2 */ - If (Arg0 >= GPIO_COM2_START && Arg0 <= GPIO_COM2_END) - { - Local0 = PID_GPIOCOM2 - Local1 = Arg0 - GPIO_COM2_START - } - /* GPIO Community 4 */ - If (Arg0 >= GPIO_COM4_START && Arg0 <= GPIO_COM4_END) - { - Local0 = PID_GPIOCOM4 - Local1 = Arg0 - GPIO_COM4_START - } - /* GPIO Community 05*/ - If (Arg0 >= GPIO_COM5_START && Arg0 <= GPIO_COM5_END) - { - Local0 = PID_GPIOCOM5 - Local1 = Arg0 - GPIO_COM5_START - } - - Local2 = PCRB(Local0) + PAD_CFG_BASE + (Local1 * 16) - Return (Local2) -} +#if CONFIG(SOC_INTEL_TIGERLAKE) + #include "gpio_tgl.asl" +#elif CONFIG(SOC_INTEL_JASPERLAKE) + #include "gpio_jsl.asl" +#endif diff --git a/src/soc/intel/tigerlake/acpi/gpio_jsl.asl b/src/soc/intel/tigerlake/acpi/gpio_jsl.asl new file mode 100644 index 0000000..504b776 --- /dev/null +++ b/src/soc/intel/tigerlake/acpi/gpio_jsl.asl @@ -0,0 +1,117 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 The coreboot project Authors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <soc/gpio_defs.h> +#include <soc/irq.h> +#include <soc/pcr_ids.h> +#include <intelblocks/gpio.h> +#include "gpio_op.asl" + +Device (GPIO) +{ + Name (_HID, CROS_GPIO_NAME) + Name (_UID, 0) + Name (_DDN, "GPIO Controller") + + Name (RBUF, ResourceTemplate() + { + Memory32Fixed (ReadWrite, 0, 0, COM0) + Memory32Fixed (ReadWrite, 0, 0, COM1) + Memory32Fixed (ReadWrite, 0, 0, COM2) + Memory32Fixed (ReadWrite, 0, 0, COM4) + Memory32Fixed (ReadWrite, 0, 0, COM5) + Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) + { GPIO_IRQ14 } + }) + + Method (_CRS, 0, NotSerialized) + { + /* GPIO Community 0 */ + CreateDWordField (^RBUF, ^COM0._BAS, BAS0) + CreateDWordField (^RBUF, ^COM0._LEN, LEN0) + Store (^^PCRB (PID_GPIOCOM0), BAS0) + Store (GPIO_BASE_SIZE, LEN0) + + /* GPIO Community 1 */ + CreateDWordField (^RBUF, ^COM1._BAS, BAS1) + CreateDWordField (^RBUF, ^COM1._LEN, LEN1) + Store (^^PCRB (PID_GPIOCOM1), BAS1) + Store (GPIO_BASE_SIZE, LEN1) + + /* GPIO Community 2 */ + CreateDWordField (^RBUF, ^COM2._BAS, BAS2) + CreateDWordField (^RBUF, ^COM2._LEN, LEN2) + Store (^^PCRB (PID_GPIOCOM2), BAS2) + Store (GPIO_BASE_SIZE, LEN2) + + /* GPIO Community 4 */ + CreateDWordField (^RBUF, ^COM4._BAS, BAS4) + CreateDWordField (^RBUF, ^COM4._LEN, LEN4) + Store (^^PCRB (PID_GPIOCOM4), BAS4) + Store (GPIO_BASE_SIZE, LEN4) + + /* GPIO Community 5 */ + CreateDWordField (^RBUF, ^COM5._BAS, BAS5) + CreateDWordField (^RBUF, ^COM5._LEN, LEN5) + Store (^^PCRB (PID_GPIOCOM5), BAS5) + Store (GPIO_BASE_SIZE, LEN5) + + Return (RBUF) + } + + Method (_STA, 0, NotSerialized) + { + Return (0xF) + } +} +/* + * Get GPIO DW0 Address + * Arg0 - GPIO Number + */ +Method (GADD, 1, NotSerialized) +{ + /* GPIO Community 0 */ + If (Arg0 >= GPIO_COM0_START && Arg0 <= GPIO_COM0_END) + { + Local0 = PID_GPIOCOM0 + Local1 = Arg0 - GPIO_COM0_START + } + /* GPIO Community 1 */ + If (Arg0 >= GPIO_COM1_START && Arg0 <= GPIO_COM1_END) + { + Local0 = PID_GPIOCOM1 + Local1 = Arg0 - GPIO_COM1_START + } + /* GPIO Community 2 */ + If (Arg0 >= GPIO_COM2_START && Arg0 <= GPIO_COM2_END) + { + Local0 = PID_GPIOCOM2 + Local1 = Arg0 - GPIO_COM2_START + } + /* GPIO Community 4 */ + If (Arg0 >= GPIO_COM4_START && Arg0 <= GPIO_COM4_END) + { + Local0 = PID_GPIOCOM4 + Local1 = Arg0 - GPIO_COM4_START + } + /* GPIO Community 05*/ + If (Arg0 >= GPIO_COM5_START && Arg0 <= GPIO_COM5_END) + { + Local0 = PID_GPIOCOM5 + Local1 = Arg0 - GPIO_COM5_START + } + + Local2 = PCRB(Local0) + PAD_CFG_BASE + (Local1 * 16) + Return (Local2) +} diff --git a/src/soc/intel/tigerlake/acpi/gpio_tgl.asl b/src/soc/intel/tigerlake/acpi/gpio_tgl.asl new file mode 100644 index 0000000..0378b52 --- /dev/null +++ b/src/soc/intel/tigerlake/acpi/gpio_tgl.asl @@ -0,0 +1,156 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 The coreboot project Authors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <soc/gpio_defs.h> +#include <soc/irq.h> +#include <soc/pcr_ids.h> +#include <intelblocks/gpio.h> +#include "gpio_op.asl" + +Device (GCM0) +{ + Name (_HID, CROS_GPIO_NAME) + Name (_UID, 0) + Name (_DDN, "GPIO Controller Community 0") + + Name (RBUF, ResourceTemplate() + { + Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM0) + Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) + { GPIO_IRQ14 } + }) + Method (_CRS, 0, NotSerialized) + { + CreateDWordField (^RBUF, ^COM0._BAS, BAS0) + BAS0 = ^^PCRB (PID_GPIOCOM0) + Return (^RBUF) + } + Method (_STA) + { + Return (0xF) + } +} + +Device (GCM1) +{ + Name (_HID, CROS_GPIO_NAME) + Name (_UID, 1) + Name (_DDN, "GPIO Controller Community 1") + + Name (RBUF, ResourceTemplate() + { + Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM1) + Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) + { GPIO_IRQ14 } + }) + Method (_CRS, 0, NotSerialized) + { + CreateDWordField (^RBUF, ^COM1._BAS, BAS1) + BAS1 = ^^PCRB (PID_GPIOCOM1) + Return (^RBUF) + } + Method (_STA) + { + Return (0xF) + } +} + +Device (GCM4) +{ + Name (_HID, CROS_GPIO_NAME) + Name (_UID, 4) + Name (_DDN, "GPIO Controller Community 4") + + Name (RBUF, ResourceTemplate() + { + Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM4) + Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) + { GPIO_IRQ14 } + }) + Method (_CRS, 0, NotSerialized) + { + CreateDWordField (^RBUF, ^COM4._BAS, BAS4) + BAS4 = ^^PCRB (PID_GPIOCOM4) + Return (^RBUF) + } + Method (_STA) + { + Return (0xF) + } +} + +Device (GCM5) +{ + Name (_HID, CROS_GPIO_NAME) + Name (_UID, 5) + Name (_DDN, "GPIO Controller Community 5") + + Name (RBUF, ResourceTemplate() + { + Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM5) + Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) + { GPIO_IRQ14 } + }) + Method (_CRS, 0, NotSerialized) + { + CreateDWordField (^RBUF, ^COM5._BAS, BAS5) + BAS5 = ^^PCRB (PID_GPIOCOM5) + Return (^RBUF) + } + Method (_STA) + { + Return (0xF) + } +} + +/* + * Get GPIO DW0 Address + * Arg0 - GPIO Number + */ +Method (GADD, 1, NotSerialized) +{ + /* GPIO Community 0 */ + If (Arg0 >= GPIO_COM0_START && Arg0 <= GPIO_COM0_END) + { + Local0 = PID_GPIOCOM0 + Local1 = Arg0 - GPIO_COM0_START + } + /* GPIO Community 1 */ + If (Arg0 >= GPIO_COM1_START && Arg0 <= GPIO_COM1_END) + { + Local0 = PID_GPIOCOM1 + Local1 = Arg0 - GPIO_COM1_START + } + /* GPIO Community 2 */ + If (Arg0 >= GPIO_COM2_START && Arg0 <= GPIO_COM2_END) + { + Local0 = PID_GPIOCOM2 + Local1 = Arg0 - GPIO_COM2_START + } + /* GPIO Community 4 */ + If (Arg0 >= GPIO_COM4_START && Arg0 <= GPIO_COM4_END) + { + Local0 = PID_GPIOCOM4 + Local1 = Arg0 - GPIO_COM4_START + } + /* GPIO Community 05*/ + If (Arg0 >= GPIO_COM5_START && Arg0 <= GPIO_COM5_END) + { + Local0 = PID_GPIOCOM5 + Local1 = Arg0 - GPIO_COM5_START + } + + Local2 = PCRB(Local0) + PAD_CFG_BASE + (Local1 * 16) + Return (Local2) +}
Hello build bot (Jenkins), Usha P, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current Pin-Ctrl kernel driver expects coreboot to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation in updated to consume community based GPIO ACPI device, updaing the current ACPI code to comply with pin-ctrl driver requirement.
BUG=150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Justin TerAvest, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current Pin-Ctrl kernel driver expects coreboot to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation in updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/3
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... PS3, Line 121: Method (GADD, 1, NotSerialized) This ACPI method looks identical between the two. Can it be retained here and just the GPIO devices moved to the concerned ASL. Is there a downside?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39470/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/3//COMMIT_MSG@14 PS3, Line 14: 150154277 Nit: b:150154277
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Justin TerAvest, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current Pin-Ctrl kernel driver expects coreboot to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation in updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/4
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39470/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/3//COMMIT_MSG@14 PS3, Line 14: 150154277
Nit: b:150154277
Done
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... PS3, Line 121: Method (GADD, 1, NotSerialized)
This ACPI method looks identical between the two. […]
No downside, can be done. Just that it would be easy to pick the changes , when we do a jsl soc split. Please let me know your thoughts, I can make that change
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
Looks good overall, especially given that we are planning to split Jasper Lake as a separate SoC. Please address the nits.
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG@9 PS5, Line 9: Pin-Ctrl Nit: pin-ctrl
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG@10 PS5, Line 10: in Nit: is
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-Ctrl kernel driver expects coreboot to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/6
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG@9 PS5, Line 9: Pin-Ctrl
Nit: pin-ctrl
Done
https://review.coreboot.org/c/coreboot/+/39470/5//COMMIT_MSG@10 PS5, Line 10: in
Nit: is
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/39470/3/src/soc/intel/tigerlake/acp... PS3, Line 121: Method (GADD, 1, NotSerialized)
No downside, can be done. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: coreboot the firmware
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: kernel driver Which version?
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: pin-Ctrl pin-ctrl
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@11 PS6, Line 11: updating update
Aamir Bohra has uploaded a new patch set (#7) to the change originally created by Aamir Bohra. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-ctrl kernel(v5.4) driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/tigerlake/acpi/gpio.asl A src/soc/intel/tigerlake/acpi/gpio_jsl.asl A src/soc/intel/tigerlake/acpi/gpio_tgl.asl 3 files changed, 278 insertions(+), 141 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/7
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 7: Code-Review+2
Ronak Kanabar has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Removed Code-Review+2 by Ronak Kanabar ronak.kanabar@intel.com
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-ctrl kernel(v5.4) driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 36 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/8
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#9).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-ctrl kernel(v5.4) driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 35 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/9
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#10).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-ctrl kernel v5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 35 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/10
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: kernel driver
Which version?
Done
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: coreboot
the firmware
Done
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@9 PS6, Line 9: pin-Ctrl
pin-ctrl
Done
https://review.coreboot.org/c/coreboot/+/39470/6//COMMIT_MSG@11 PS6, Line 11: updating
update
Done
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#11).
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernelv5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 35 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/11
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39470/11/src/soc/intel/jasperlake/a... File src/soc/intel/jasperlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/39470/11/src/soc/intel/jasperlake/a... PS11, Line 43: Store (^^PCRB (PID_GPIOCOM0), BAS0) Can we use ASL 2.0 throughout to keep it consistent with GADD method?
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#12).
Change subject: soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake ......................................................................
soc/intel/tigerlake: Publish single GPIO ACPI device for Jasper Lake
Current pin-ctrl kernel(v5.4) driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, updating the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 32 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/12
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#13).
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernelv5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 32 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/13
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#14).
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernel v5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 32 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/14
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#15).
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernel v5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 31 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/15
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39470/11/src/soc/intel/jasperlake/a... File src/soc/intel/jasperlake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/39470/11/src/soc/intel/jasperlake/a... PS11, Line 43: Store (^^PCRB (PID_GPIOCOM0), BAS0)
Can we use ASL 2. […]
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 15: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Justin TerAvest, Ronak Kanabar, Usha P, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39470
to look at the new patch set (#16).
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernel v5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Signed-off-by: Aamir Bohra aamir.bohra@intel.com Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 31 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/39470/16
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 16: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 16: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
soc/intel/jasperlake: Publish single GPIO ACPI device
Current pin-ctrl kernel v5.4 driver expects the firmware to publish single GPIO ACPI device. Until kernel pin-ctrl driver implementation is updated to consume community based GPIO ACPI device, update the current ACPI code to comply with pin-ctrl driver requirement.
BUG=b:150154277 TEST=Verify intel pin-ctrl driver can successfully load in OS
Signed-off-by: Aamir Bohra aamir.bohra@intel.com Change-Id: Ifcc92adaee550182ab405541ea85019f31bb8658 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39470 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/jasperlake/acpi/gpio.asl 1 file changed, 31 insertions(+), 69 deletions(-)
Approvals: build bot (Jenkins): Verified Subrata Banik: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/jasperlake/acpi/gpio.asl b/src/soc/intel/jasperlake/acpi/gpio.asl index 2b4aff0..f1e4498 100644 --- a/src/soc/intel/jasperlake/acpi/gpio.asl +++ b/src/soc/intel/jasperlake/acpi/gpio.asl @@ -1,107 +1,69 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* This file is part of the coreboot project. */ + +#include <intelblocks/gpio.h> #include <soc/gpio_defs.h> #include <soc/irq.h> #include <soc/pcr_ids.h> -#include <intelblocks/gpio.h> #include "gpio_op.asl"
-Device (GCM0) +Device (GPIO) { Name (_HID, CROS_GPIO_NAME) Name (_UID, 0) - Name (_DDN, "GPIO Controller Community 0") + Name (_DDN, "GPIO Controller")
Name (RBUF, ResourceTemplate() { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM0) + Memory32Fixed (ReadWrite, 0, 0, COM0) + Memory32Fixed (ReadWrite, 0, 0, COM1) + Memory32Fixed (ReadWrite, 0, 0, COM2) + Memory32Fixed (ReadWrite, 0, 0, COM4) + Memory32Fixed (ReadWrite, 0, 0, COM5) Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) { GPIO_IRQ14 } }) + Method (_CRS, 0, NotSerialized) { + /* GPIO Community 0 */ CreateDWordField (^RBUF, ^COM0._BAS, BAS0) + CreateDWordField (^RBUF, ^COM0._LEN, LEN0) BAS0 = ^^PCRB (PID_GPIOCOM0) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} + LEN0 = GPIO_BASE_SIZE
-Device (GCM1) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 1) - Name (_DDN, "GPIO Controller Community 1") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM1) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { + /* GPIO Community 1 */ CreateDWordField (^RBUF, ^COM1._BAS, BAS1) + CreateDWordField (^RBUF, ^COM1._LEN, LEN1) BAS1 = ^^PCRB (PID_GPIOCOM1) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} + LEN1 = GPIO_BASE_SIZE
-Device (GCM4) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 4) - Name (_DDN, "GPIO Controller Community 4") + /* GPIO Community 2 */ + CreateDWordField (^RBUF, ^COM2._BAS, BAS2) + CreateDWordField (^RBUF, ^COM2._LEN, LEN2) + BAS2 = ^^PCRB (PID_GPIOCOM2) + LEN2 = GPIO_BASE_SIZE
- Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM4) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { + /* GPIO Community 4 */ CreateDWordField (^RBUF, ^COM4._BAS, BAS4) + CreateDWordField (^RBUF, ^COM4._LEN, LEN4) BAS4 = ^^PCRB (PID_GPIOCOM4) - Return (^RBUF) - } - Method (_STA) - { - Return (0xF) - } -} + LEN4 = GPIO_BASE_SIZE
-Device (GCM5) -{ - Name (_HID, CROS_GPIO_NAME) - Name (_UID, 5) - Name (_DDN, "GPIO Controller Community 5") - - Name (RBUF, ResourceTemplate() - { - Memory32Fixed (ReadWrite, 0, GPIO_BASE_SIZE, COM5) - Interrupt (ResourceConsumer, Level, ActiveLow, Shared,,, GIRQ) - { GPIO_IRQ14 } - }) - Method (_CRS, 0, NotSerialized) - { + /* GPIO Community 5 */ CreateDWordField (^RBUF, ^COM5._BAS, BAS5) + CreateDWordField (^RBUF, ^COM5._LEN, LEN5) BAS5 = ^^PCRB (PID_GPIOCOM5) - Return (^RBUF) + LEN5 = GPIO_BASE_SIZE + + Return (RBUF) } - Method (_STA) + + Method (_STA, 0, NotSerialized) { Return (0xF) } } - /* * Get GPIO DW0 Address * Arg0 - GPIO Number
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39470 )
Change subject: soc/intel/jasperlake: Publish single GPIO ACPI device ......................................................................
Patch Set 17:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2248 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2247 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2246
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aamir Bohra (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Paul Menzel (Code Review)
-
Ronak Kanabar (Code Review)
-
Subrata Banik (Code Review)