[Skiboot] [PATCH 5/6] hw/npu2, platform: Restructure OpenCAPI i2c reset/presence pins
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Thu Aug 23 12:07:21 AEST 2018
On 23/08/18 11:28, Oliver wrote:
> 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?
ACK
>
>> 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.
We could, this mostly follows tradition in npu2-opencapi.c in dealing
with register addresses but this could be made into an array if people
would rather it be one, it would be a bit cleaner
>
>> 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?
The bitmask against which we AND the value we read via i2c.
It's not actually any different from the reset pins described above so
I'm not sure why I didn't use consistent comment terminology there to
begin with...
>
>> 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?
Yeah that could be worded less confusingly I guess. But we do, at
present, apply this to both obus0 and obus3 so the lanes for ODL1 on
each obus are swapped.
>
>> 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
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com IBM Australia Limited
More information about the Skiboot
mailing list