[Skiboot] [PATCH 5/6] hw/npu2, platform: Restructure OpenCAPI i2c reset/presence pins
Oliver
oohall at gmail.com
Thu Aug 23 11:28:20 AEST 2018
On Fri, Aug 17, 2018 at 6:44 PM, Andrew Donnellan
<andrew.donnellan at au1.ibm.com> wrote:
> In platform_ocapi, we define i2c_{reset,presence}_odl{0,1} to specify the
> appropriate reset/presence GPIO pins for devices connected to ODL0 and ODL1
> respectively.
>
> This is obviously wrong, because a device connected to brick 2 and a device
> connected to brick 4 are going to be different devices connected to
> different I2C pins, but rather conveniently we haven't had to deal with
> systems that can use the full 4 bricks as yet. Now that we're adding
> OpenCAPI support for Witherspoon, we should change this to specify pins
> separately for all 4 bricks.
>
> Replace i2c_{reset,presence}_odl{0,1} with
> i2c_{reset,presence}_pin{2,3,4,5} and update the presence detection code,
That naming convention is amazingly bad. Can't you call it
brick2_presence or something?
> device reset code, and existing platforms accordingly.
>
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> ---
> core/platform.c | 12 ++++++++----
> hw/npu2-common.c | 9 +++++++--
> hw/npu2-opencapi.c | 10 +++++++---
> include/platform.h | 12 ++++++++----
> platforms/astbmc/zaius.c | 12 ++++++++----
> platforms/ibm-fsp/zz.c | 12 ++++++++----
> 6 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/core/platform.c b/core/platform.c
> index b32cbf5ce9d1..85b2cd8f72ee 100644
> --- a/core/platform.c
> +++ b/core/platform.c
> @@ -174,11 +174,15 @@ const struct platform_ocapi generic_ocapi = {
> .i2c_engine = 1,
> .i2c_port = 4,
> .i2c_reset_addr = 0x20,
> - .i2c_reset_odl0 = (1 << 1),
> - .i2c_reset_odl1 = (1 << 6),
> + .i2c_reset_pin2 = (1 << 1),
> + .i2c_reset_pin3 = (1 << 6),
> + .i2c_reset_pin4 = 0, /* unused */
> + .i2c_reset_pin5 = 0, /* unused */
> .i2c_presence_addr = 0x20,
> - .i2c_presence_odl0 = (1 << 2), /* bottom connector */
> - .i2c_presence_odl1 = (1 << 7), /* top connector */
> + .i2c_presence_pin2 = (1 << 2), /* bottom connector */
> + .i2c_presence_pin3 = (1 << 7), /* top connector */
> + .i2c_presence_pin4 = 0, /* unused */
> + .i2c_presence_pin5 = 0, /* unused */
> /*
> * The ZZs we typically use for BML/generic platform tend to
> * have old planars and presence detection is broken there, so
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index cceb88706981..8151df3d9ba5 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -115,11 +115,16 @@ static bool _i2c_presence_detect(struct npu2_dev *dev)
>
> switch (dev->link_index) {
> case 2:
> - data = platform.ocapi->i2c_presence_odl0;
> + data = platform.ocapi->i2c_presence_pin2;
> break;
> case 3:
> - data = platform.ocapi->i2c_presence_odl1;
> + data = platform.ocapi->i2c_presence_pin3;
> break;
> + case 4:
> + data = platform.ocapi->i2c_presence_pin4;
> + break;
> + case 5:
> + data = platform.ocapi->i2c_presence_pin5;
Can we make it an array or something rather than having switch
statements all over the place? That's probably something for another
patch though.
> default:
> OCAPIERR(dev, "presence detection on invalid link\n");
> return true;
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index fa28633bb150..ce5f99114463 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c
> @@ -816,12 +816,16 @@ static void assert_reset(struct npu2_dev *dev)
>
> switch (dev->brick_index) {
> case 2:
> - case 4:
> - pin = platform.ocapi->i2c_reset_odl0;
> + pin = platform.ocapi->i2c_reset_pin2;
> break;
> case 3:
> + pin = platform.ocapi->i2c_reset_pin3;
> + break;
> + case 4:
> + pin = platform.ocapi->i2c_reset_pin4;
> + break;
> case 5:
> - pin = platform.ocapi->i2c_reset_odl1;
> + pin = platform.ocapi->i2c_reset_pin5;
> break;
> default:
> assert(false);
> diff --git a/include/platform.h b/include/platform.h
> index efafbd2239b9..76d001547d05 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -50,11 +50,15 @@ struct platform_ocapi {
> uint8_t i2c_engine; /* I2C engine number */
> uint8_t i2c_port; /* I2C port number */
> uint8_t i2c_reset_addr; /* I2C address for reset */
> - uint8_t i2c_reset_odl0; /* I2C pin to write to reset ODL0 */
> - uint8_t i2c_reset_odl1; /* I2C pin to write to reset ODL1 */
> + uint8_t i2c_reset_pin2; /* I2C pin to write to reset brick 2 */
> + uint8_t i2c_reset_pin3; /* I2C pin to write to reset brick 3 */
> + uint8_t i2c_reset_pin4; /* I2C pin to write to reset brick 4 */
> + uint8_t i2c_reset_pin5; /* I2C pin to write to reset brick 5 */
> uint8_t i2c_presence_addr; /* I2C address for presence detection */
> - uint8_t i2c_presence_odl0; /* I2C mask for detection on ODL0 */
> - uint8_t i2c_presence_odl1; /* I2C mask for detection on ODL1 */
> + uint8_t i2c_presence_pin2; /* I2C mask for detection on brick 2 */
> + uint8_t i2c_presence_pin3; /* I2C mask for detection on brick 3 */
> + uint8_t i2c_presence_pin4; /* I2C mask for detection on brick 4 */
> + uint8_t i2c_presence_pin5; /* I2C mask for detection on brick 5 */
What is an I2C mask even?
> bool force_presence; /* don't use i2c detection */
> bool odl_phy_swap; /* Swap ODL1 to use brick 2 rather than
> * brick 1 lanes */
If we support all four bricks is this still meaningful?
> diff --git a/platforms/astbmc/zaius.c b/platforms/astbmc/zaius.c
> index 069ce5751550..95d59e8feee9 100644
> --- a/platforms/astbmc/zaius.c
> +++ b/platforms/astbmc/zaius.c
> @@ -28,11 +28,15 @@ const struct platform_ocapi zaius_ocapi = {
> .i2c_engine = 1,
> .i2c_port = 4,
> .i2c_reset_addr = 0x20,
> - .i2c_reset_odl0 = (1 << 1),
> - .i2c_reset_odl1 = (1 << 6),
> + .i2c_reset_pin2 = (1 << 1),
> + .i2c_reset_pin3 = (1 << 6),
> + .i2c_reset_pin4 = 0, /* unused */
> + .i2c_reset_pin5 = 0, /* unused */
> .i2c_presence_addr = 0x20,
> - .i2c_presence_odl0 = (1 << 2), /* bottom connector */
> - .i2c_presence_odl1 = (1 << 7), /* top connector */
> + .i2c_presence_pin2 = (1 << 2), /* bottom connector */
> + .i2c_presence_pin3 = (1 << 7), /* top connector */
> + .i2c_presence_pin4 = 0, /* unused */
> + .i2c_presence_pin5 = 0, /* unused */
> .odl_phy_swap = true,
> };
>
> diff --git a/platforms/ibm-fsp/zz.c b/platforms/ibm-fsp/zz.c
> index dc48768b8a96..800ad024314f 100644
> --- a/platforms/ibm-fsp/zz.c
> +++ b/platforms/ibm-fsp/zz.c
> @@ -32,11 +32,15 @@ const struct platform_ocapi zz_ocapi = {
> .i2c_engine = 1,
> .i2c_port = 4,
> .i2c_reset_addr = 0x20,
> - .i2c_reset_odl0 = (1 << 1),
> - .i2c_reset_odl1 = (1 << 6),
> + .i2c_reset_pin2 = (1 << 1),
> + .i2c_reset_pin3 = (1 << 6),
> + .i2c_reset_pin4 = 0, /* unused */
> + .i2c_reset_pin5 = 0, /* unused */
> .i2c_presence_addr = 0x20,
> - .i2c_presence_odl0 = (1 << 2), /* bottom connector */
> - .i2c_presence_odl1 = (1 << 7), /* top connector */
> + .i2c_presence_pin2 = (1 << 2), /* bottom connector */
> + .i2c_presence_pin3 = (1 << 7), /* top connector */
> + .i2c_presence_pin4 = 0, /* unused */
> + .i2c_presence_pin5 = 0, /* unused */
> /*
> * i2c presence detection is broken on ZZ planar < v4 so we
> * force the presence until all our systems are upgraded
> --
> git-series 0.9.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list