diff mbox series

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

Message ID 20200331094735.82874-2-fbarrat@linux.ibm.com
State New
Headers show
Series [1/2] npu2-hw-procedures: Add platform hook to tune PHY setup | expand

Commit Message

Frederic Barrat March 31, 2020, 9:47 a.m. UTC
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@linux.ibm.com>
Cc: skiboot-stable@lists.ozlabs.org # skiboot-op940.x
---
 hw/npu2-hw-procedures.c   | 25 ++++++++++++++++++-------
 include/npu2.h            |  2 ++
 platforms/astbmc/mihawk.c |  1 +
 3 files changed, 21 insertions(+), 7 deletions(-)

Comments

Andrew Donnellan April 8, 2020, 12:47 a.m. UTC | #1
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@linux.ibm.com>
> Cc: skiboot-stable@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.

> ---
>   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)
Frederic Barrat April 8, 2020, 9:40 a.m. UTC | #2
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@linux.ibm.com>
>> Cc: skiboot-stable@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
Frederic Barrat April 9, 2020, 2:05 p.m. UTC | #3
>>> -#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.


Andrew, so do you think we can live with it or do you see a better 
option (proposal above or else)?

   Fred
Andrew Donnellan April 15, 2020, 2:15 a.m. UTC | #4
On 10/4/20 12:05 am, Frederic Barrat wrote:
> 
>>>> -#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.
> 
> 
> Andrew, so do you think we can live with it or do you see a better 
> option (proposal above or else)?

Throw it in the platform_ocapi struct alongside all the other platform 
specific opencapi parameters?
diff mbox series

Patch

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;
+
+#define MARGIN_RATIO		(0)
 #define PRE_WIDTH		(5)
 #define POST_WIDTH		(7)
 #define MAIN_WIDTH		(7)
@@ -565,8 +567,8 @@  static uint32_t phy_tx_zcal_calculate(struct npu2_dev *ndev)
 		return PROCEDURE_COMPLETE | PROCEDURE_FAILED;
 
 	p_value = zcal_p - TOTAL_X2_MAX;
-	p_precursor_select = (p_value * FFE_PRE_COEFF)/128;
-	p_postcursor_select = (p_value * FFE_POST_COEFF)/128;
+	p_precursor_select = (p_value * ffe_pre_coeff)/128;
+	p_postcursor_select = (p_value * ffe_post_coeff)/128;
 	margin_pu_select = (p_value * MARGIN_RATIO)/256;
 
 	if (p_value % 2) {
@@ -587,8 +589,8 @@  static uint32_t phy_tx_zcal_calculate(struct npu2_dev *ndev)
 	}
 
 	n_value = zcal_n - TOTAL_X2_MAX;
-	n_precursor_select = (n_value * FFE_PRE_COEFF)/128;
-	n_postcursor_select = (n_value * FFE_POST_COEFF)/128;
+	n_precursor_select = (n_value * ffe_pre_coeff)/128;
+	n_postcursor_select = (n_value * ffe_post_coeff)/128;
 	margin_pd_select = (p_value * MARGIN_RATIO)/256;
 
 	if (n_value % 2) {
@@ -1065,3 +1067,12 @@  void npu2_opencapi_phy_prbs31(struct npu2_dev *dev)
 {
 	phy_write(dev, &NPU2_PHY_TX_DRV_DATA_PATTERN_GCRMSG, 0xD);
 }
+
+void mihawk_phy_setup(struct npu2_dev *dev)
+{
+	/* Tune equalization settings */
+	NPU2DEVINF(dev, "Enabling Mihawk-specific PHY setup\n");
+	phy_write(dev, &NPU2_PHY_TX_FFE_BOOST_EN, 0);
+	ffe_pre_coeff = 0x3;
+	ffe_post_coeff = 0x14;
+}
diff --git a/include/npu2.h b/include/npu2.h
index eb7c4558..cab1e7ac 100644
--- a/include/npu2.h
+++ b/include/npu2.h
@@ -243,6 +243,8 @@  int64_t npu2_freeze_status(struct phb *phb __unused,
 			   uint16_t *severity __unused);
 void npu2_dump_scoms(int chip_id);
 
+void mihawk_phy_setup(struct npu2_dev *dev);
+
 int64_t npu2_init_context(struct phb *phb, uint64_t msr, uint64_t bdf);
 int64_t npu2_destroy_context(struct phb *phb, uint64_t bdf);
 int64_t npu2_map_lpar(struct phb *phb, uint64_t bdf, uint64_t lparid,
diff --git a/platforms/astbmc/mihawk.c b/platforms/astbmc/mihawk.c
index 8971b407..813ab406 100644
--- a/platforms/astbmc/mihawk.c
+++ b/platforms/astbmc/mihawk.c
@@ -157,6 +157,7 @@  static const struct platform_ocapi mihawk_ocapi = {
         .i2c_presence_brick5 = 0, /* unused */
         .odl_phy_swap        = true,
 	.ocapi_slot_label    = mihawk_ocapi_slot_label,
+	.phy_setup           = mihawk_phy_setup,
 };
 
 static const struct slot_table_entry P1E1A_x8_PLX8748_RiserA_down[] = {