[Skiboot] [PATCH 2/2] platform/mihawk: Tune equalization settings for opencapi

[Frederic Barrat]

Le 08/04/2020 à 02:47, Andrew Donnellan a écrit :
> On 31/3/20 8:47 pm, Frederic Barrat wrote:
>> The Bittware 250SOC adapter on Mihawk was showing a high count of CRC
>> errors on one of the opencapi slots. The PHY team suggested new
>> equalization settings to correct the errors.
>>
>> All existing adapters have been tested on mihawk to make sure the
>> settings are compatible. However, the new settings should not be used
>> on platforms other than mihawk.
>>
>> The changes specific to mihawk are:
>> - Update the tx_ffe_pre_coeff and tx_ffe_post_coeff input parameters
>>    used during zcal
>> - turn off the tx_ffe_boost parameter through scom
>>
>> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>
>> Cc: skiboot-stable at lists.ozlabs.org # skiboot-op940.x
> 
> I can't validate the specific equalisation settings, so I'll take your 
> word that they are indeed correct.


Same for me actually. Those values are directly communicated by the hw 
team.  They've tried many different settings before reaching a conclusion.


>> ---
>>   hw/npu2-hw-procedures.c   | 25 ++++++++++++++++++-------
>>   include/npu2.h            |  2 ++
>>   platforms/astbmc/mihawk.c |  1 +
>>   3 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/npu2-hw-procedures.c b/hw/npu2-hw-procedures.c
>> index 890b25a8..95a0f9dc 100644
>> --- a/hw/npu2-hw-procedures.c
>> +++ b/hw/npu2-hw-procedures.c
>> @@ -78,6 +78,7 @@ static struct npu2_phy_reg 
>> NPU2_PHY_TX_ZCAL_DONE           = {0x3c1, 50, 1};
>>   static struct npu2_phy_reg NPU2_PHY_TX_ZCAL_ERROR           = 
>> {0x3c1, 51, 1};
>>   static struct npu2_phy_reg NPU2_PHY_TX_ZCAL_N               = 
>> {0x3c3, 48, 9};
>>   static struct npu2_phy_reg NPU2_PHY_TX_ZCAL_P               = 
>> {0x3c5, 48, 9};
>> +static struct npu2_phy_reg NPU2_PHY_TX_FFE_BOOST_EN           = 
>> {0x34b, 59, 1};
>>   static struct npu2_phy_reg NPU2_PHY_TX_PSEG_PRE_EN           = 
>> {0x34d, 51, 5};
>>   static struct npu2_phy_reg NPU2_PHY_TX_PSEG_PRE_SELECT           = 
>> {0x34d, 56, 5};
>>   static struct npu2_phy_reg NPU2_PHY_TX_NSEG_PRE_EN           = 
>> {0x34f, 51, 5};
>> @@ -498,10 +499,11 @@ static uint32_t phy_tx_zcal_wait(struct npu2_dev 
>> *ndev)
>>       return PROCEDURE_NEXT;
>>   }
>> -#define MARGIN_RATIO        (0)
>> -#define FFE_PRE_COEFF        (0)
>> -#define FFE_POST_COEFF        (0)
>> +int ffe_pre_coeff = 0;
>> +int ffe_post_coeff = 0;
> 
> Is there any way to do this without exposing global variables? (My 
> primary objection is that global variables for specific things in 
> specific subsystems is ugly and inelegant)


So those parameters are input parameters when computing the zcal and we 
need a new per-platform way of defining them. So one way or another, 
we'll end up with some kind of per-platform global, I think. And I agree 
it would be a bit better than a pure global. I went this way as a 
compromise between minimizing the impact on the "nvlink on witherspoon" 
path and giving us flexibility to hack anything for opencapi. The whole 
thing was highly empirical and feels like a hack anyway.

Another option I can think of would be to have a per-platform hook (and 
no longer in the platform_ocapi struct) at the beginning of 
phy_tx_zcal_calculate(), defaulting to the current 0 values if none are 
defined. The hook would give the values to use for the ffe_pre_coeff and 
ffe_post_coeff parameters. Probably a bit cleaner, but doesn't feel as 
easily extensible in the future if needed, I think.

As a side note, this is going to look very different on next generation 
of hardware.

   Fred