diff mbox series

[1/2] mowgli: Limit slot1 to Gen3 by default

Message ID 20201030022230.21949-1-Lulu_Su@wistron.com
State Accepted
Headers show
Series [1/2] mowgli: Limit slot1 to Gen3 by default | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (233ade2f6ffdfa406100276784eb447d062fe8e7)

Commit Message

Lulu Su Oct. 30, 2020, 2:22 a.m. UTC
From: LuluTHSu <Lulu_Su@wistron.com>

Refer to the spec. of mowgli, limit the slot to Gen3 speed.
For mowgli platform spec.

Cc: skiboot-stable@lists.ozlabs.org
Signed-off-by: LuluTHSu <Lulu_Su@wistron.com>
---
 hw/phb4.c                 | 21 +++++++++++++++++++++
 include/phb4.h            |  1 +
 platforms/astbmc/mowgli.c | 16 ++++++++++++++++
 3 files changed, 38 insertions(+)

Comments

Frederic Barrat Nov. 9, 2020, 4:36 p.m. UTC | #1
On 30/10/2020 03:22, Lulu Su wrote:
> From: LuluTHSu<Lulu_Su@wistron.com>
> 
> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
> For mowgli platform spec.
> 
> Cc:skiboot-stable@lists.ozlabs.org
> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
> ---
>   hw/phb4.c                 | 21 +++++++++++++++++++++
>   include/phb4.h            |  1 +
>   platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>   3 files changed, 38 insertions(+)
> 
> diff --git a/hw/phb4.c b/hw/phb4.c
> index 17a233f..de10bb0 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -2991,6 +2991,27 @@ static unsigned int phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>   	return max_link_speed;
>   }
>   
> +/*
> + * Has the same effect as the ibm,max-link-speed property.
> + * i.e. sets the default link speed, while allowing NVRAM
> + * overrides, etc to still take effect.
> + */
> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max)
> +{
> +	uint64_t scr;
> +	int max;
> +
> +	/* take into account nvram settings, etc */
> +	if (pcie_max_link_speed)
> +		max = pcie_max_link_speed;
> +	else
> +		max = new_max;
> +	
> +	scr = phb4_read_reg(p, PHB_PCIE_SCR);
> +	scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
> +	phb4_write_reg(p, PHB_PCIE_SCR, scr);
> +}
> +


This patch has already been merged, but I think there's a problem here. 
This should work to limit the link speed at boot, when the link is 
trained for the first time. However, since we're writing directly in the 
register, skiboot won't memorize the limit and on the next link 
reset/retrain (due to a EEH recovery or PCI hotplug for example), then 
that is lost. And mowgli_setup_phb() won't be called either on that path.

It seems there's an easier way of handling it: phb4_get_max_link_speed() 
will honor any limit found in the "ibm,max-link-speed" property of the 
PHB in the device tree. So it should be enough to just set/add that 
property for PHB 0. I'm not sure what control you have on the device 
tree generation on that platform, but worse case, you could add a fixup 
in mowgli.c, for example when probe() is called.

   Fred
Vasant Hegde Dec. 2, 2020, 7:03 a.m. UTC | #2
On 11/9/20 10:06 PM, Frederic Barrat wrote:
> 
> 
> On 30/10/2020 03:22, Lulu Su wrote:
>> From: LuluTHSu<Lulu_Su@wistron.com>
>>
>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>> For mowgli platform spec.
>>
>> Cc:skiboot-stable@lists.ozlabs.org
>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>> ---
>>   hw/phb4.c                 | 21 +++++++++++++++++++++
>>   include/phb4.h            |  1 +
>>   platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>   3 files changed, 38 insertions(+)
>>
>> diff --git a/hw/phb4.c b/hw/phb4.c
>> index 17a233f..de10bb0 100644
>> --- a/hw/phb4.c
>> +++ b/hw/phb4.c
>> @@ -2991,6 +2991,27 @@ static unsigned int phb4_get_max_link_speed(struct phb4 
>> *p, struct dt_node *np)
>>       return max_link_speed;
>>   }
>> +/*
>> + * Has the same effect as the ibm,max-link-speed property.
>> + * i.e. sets the default link speed, while allowing NVRAM
>> + * overrides, etc to still take effect.
>> + */
>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max)
>> +{
>> +    uint64_t scr;
>> +    int max;
>> +
>> +    /* take into account nvram settings, etc */
>> +    if (pcie_max_link_speed)
>> +        max = pcie_max_link_speed;
>> +    else
>> +        max = new_max;
>> +
>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr);
>> +}
>> +
> 
> 
> This patch has already been merged, but I think there's a problem here. This 
> should work to limit the link speed at boot, when the link is trained for the 
> first time. However, since we're writing directly in the register, skiboot won't 
> memorize the limit and on the next link reset/retrain (due to a EEH recovery or 
> PCI hotplug for example), then that is lost. And mowgli_setup_phb() won't be 
> called either on that path.


Lulu,

Can you guys fix this issue?

-Vasant
Lulu Su Dec. 8, 2020, 11:50 a.m. UTC | #3
Hi Vasant,

Thank you for your reminder. I have a question for your opinion:
Finally, there is a branch used by mowgli in ibm-op-release/op-build: release-mowgli,
Maybe I can refer to the previous approach https://github.com/ibm-op-release/op-build/blob/release-mowgli/openpower/configs/mihawk_defconfig#L3 ,
build a dedicated mowgli-patches, and add the modification of phb4_get_max_link_speed(), set max_link_speed to GEN3.
What do you think?



Hi Frederic,

Thanks for your advice,
I tried to make some modifications (attached files) and verified through EEH, the max-link-speed of this modification will not be lost,
Do you have any suggestions for this modification?

But as you mentioned, there is an easier way to achieve this.
I would like to ask how to set/add this property (ibm, max-link-speed) for PHB 0?

If there is any problem, please feel free to contact me.
Thank you very much.

Best regards,
Lulu Su

-----Original Message-----
From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> 
Sent: Wednesday, December 2, 2020 3:04 PM
To: Frederic Barrat <fbarrat@linux.ibm.com>; Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; skiboot@lists.ozlabs.org; Stewart Smith <stewart@linux.vnet.ibm.com>
Cc: Oliver OHalloran <oliveroh@au1.ibm.com>; skiboot-stable@lists.ozlabs.org; Klaus Heinrich Kiwi <klausk@br.ibm.com>
Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by default

On 11/9/20 10:06 PM, Frederic Barrat wrote:
> 
> 
> On 30/10/2020 03:22, Lulu Su wrote:
>> From: LuluTHSu<Lulu_Su@wistron.com>
>>
>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>> For mowgli platform spec.
>>
>> Cc:skiboot-stable@lists.ozlabs.org
>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>> ---
>>   hw/phb4.c                 | 21 +++++++++++++++++++++
>>   include/phb4.h            |  1 +
>>   platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>   3 files changed, 38 insertions(+)
>>
>> diff --git a/hw/phb4.c b/hw/phb4.c
>> index 17a233f..de10bb0 100644
>> --- a/hw/phb4.c
>> +++ b/hw/phb4.c
>> @@ -2991,6 +2991,27 @@ static unsigned int 
>> phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>>       return max_link_speed;
>>   }
>> +/*
>> + * Has the same effect as the ibm,max-link-speed property.
>> + * i.e. sets the default link speed, while allowing NVRAM
>> + * overrides, etc to still take effect.
>> + */
>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max) {
>> +    uint64_t scr;
>> +    int max;
>> +
>> +    /* take into account nvram settings, etc */
>> +    if (pcie_max_link_speed)
>> +        max = pcie_max_link_speed;
>> +    else
>> +        max = new_max;
>> +
>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr); }
>> +
> 
> 
> This patch has already been merged, but I think there's a problem 
> here. This should work to limit the link speed at boot, when the link 
> is trained for the first time. However, since we're writing directly 
> in the register, skiboot won't memorize the limit and on the next link 
> reset/retrain (due to a EEH recovery or PCI hotplug for example), then 
> that is lost. And mowgli_setup_phb() won't be called either on that path.


Lulu,

Can you guys fix this issue?

-Vasant
Frederic Barrat Dec. 8, 2020, 6:33 p.m. UTC | #4
Hi,

Attached is a patch for you to try. You will also need to revert the 
previous patch (5262cdd1b99f77bca5951fc8132f9795ef0c2b87), since we no 
longer need the phb4_set_dt_max_link_speed() function.

   Fred


On 08/12/2020 12:50, Lulu_Su@wistron.com wrote:
> Hi Vasant,
> 
> Thank you for your reminder. I have a question for your opinion:
> Finally, there is a branch used by mowgli in ibm-op-release/op-build: release-mowgli,
> Maybe I can refer to the previous approach https://github.com/ibm-op-release/op-build/blob/release-mowgli/openpower/configs/mihawk_defconfig#L3 ,
> build a dedicated mowgli-patches, and add the modification of phb4_get_max_link_speed(), set max_link_speed to GEN3.
> What do you think?
> 
> 
> 
> Hi Frederic,
> 
> Thanks for your advice,
> I tried to make some modifications (attached files) and verified through EEH, the max-link-speed of this modification will not be lost,
> Do you have any suggestions for this modification?
> 
> But as you mentioned, there is an easier way to achieve this.
> I would like to ask how to set/add this property (ibm, max-link-speed) for PHB 0?
> 
> If there is any problem, please feel free to contact me.
> Thank you very much.
> 
> Best regards,
> Lulu Su
> 
> -----Original Message-----
> From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Sent: Wednesday, December 2, 2020 3:04 PM
> To: Frederic Barrat <fbarrat@linux.ibm.com>; Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; skiboot@lists.ozlabs.org; Stewart Smith <stewart@linux.vnet.ibm.com>
> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>; skiboot-stable@lists.ozlabs.org; Klaus Heinrich Kiwi <klausk@br.ibm.com>
> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by default
> 
> On 11/9/20 10:06 PM, Frederic Barrat wrote:
>>
>>
>> On 30/10/2020 03:22, Lulu Su wrote:
>>> From: LuluTHSu<Lulu_Su@wistron.com>
>>>
>>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>>> For mowgli platform spec.
>>>
>>> Cc:skiboot-stable@lists.ozlabs.org
>>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>>> ---
>>>    hw/phb4.c                 | 21 +++++++++++++++++++++
>>>    include/phb4.h            |  1 +
>>>    platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>>    3 files changed, 38 insertions(+)
>>>
>>> diff --git a/hw/phb4.c b/hw/phb4.c
>>> index 17a233f..de10bb0 100644
>>> --- a/hw/phb4.c
>>> +++ b/hw/phb4.c
>>> @@ -2991,6 +2991,27 @@ static unsigned int
>>> phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>>>        return max_link_speed;
>>>    }
>>> +/*
>>> + * Has the same effect as the ibm,max-link-speed property.
>>> + * i.e. sets the default link speed, while allowing NVRAM
>>> + * overrides, etc to still take effect.
>>> + */
>>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max) {
>>> +    uint64_t scr;
>>> +    int max;
>>> +
>>> +    /* take into account nvram settings, etc */
>>> +    if (pcie_max_link_speed)
>>> +        max = pcie_max_link_speed;
>>> +    else
>>> +        max = new_max;
>>> +
>>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr); }
>>> +
>>
>>
>> This patch has already been merged, but I think there's a problem
>> here. This should work to limit the link speed at boot, when the link
>> is trained for the first time. However, since we're writing directly
>> in the register, skiboot won't memorize the limit and on the next link
>> reset/retrain (due to a EEH recovery or PCI hotplug for example), then
>> that is lost. And mowgli_setup_phb() won't be called either on that path.
> 
> 
> Lulu,
> 
> Can you guys fix this issue?
> 
> -Vasant
> 
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------
> This email contains confidential or legally privileged information and is for the sole use of its intended recipient.
> Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
> If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
> ---------------------------------------------------------------------------------------------------------------------------------------------------------------
>
Lulu Su Dec. 9, 2020, 5:26 a.m. UTC | #5
Hi Frederic,

The attached patch has been modified according to what you mentioned. 
And use EEH to verify, the result is the same as expected.
If you think there is no problem, I will upload this patch to Skiboot. 
Thank you very much for your methods and suggestions.

	Lulu
-----Original Message-----
From: Frederic Barrat <fbarrat@linux.ibm.com> 
Sent: Wednesday, December 9, 2020 2:33 AM
To: Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; hegdevasant@linux.vnet.ibm.com; skiboot@lists.ozlabs.org; stewart@linux.vnet.ibm.com
Cc: oliveroh@au1.ibm.com; skiboot-stable@lists.ozlabs.org; klausk@br.ibm.com
Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by default

Hi,

Attached is a patch for you to try. You will also need to revert the previous patch (5262cdd1b99f77bca5951fc8132f9795ef0c2b87), since we no longer need the phb4_set_dt_max_link_speed() function.

   Fred


On 08/12/2020 12:50, Lulu_Su@wistron.com wrote:
> Hi Vasant,
> 
> Thank you for your reminder. I have a question for your opinion:
> Finally, there is a branch used by mowgli in ibm-op-release/op-build: 
> release-mowgli, Maybe I can refer to the previous approach 
> https://github.com/ibm-op-release/op-build/blob/release-mowgli/openpower/configs/mihawk_defconfig#L3 , build a dedicated mowgli-patches, and add the modification of phb4_get_max_link_speed(), set max_link_speed to GEN3.
> What do you think?
> 
> 
> 
> Hi Frederic,
> 
> Thanks for your advice,
> I tried to make some modifications (attached files) and verified 
> through EEH, the max-link-speed of this modification will not be lost, Do you have any suggestions for this modification?
> 
> But as you mentioned, there is an easier way to achieve this.
> I would like to ask how to set/add this property (ibm, max-link-speed) for PHB 0?
> 
> If there is any problem, please feel free to contact me.
> Thank you very much.
> 
> Best regards,
> Lulu Su
> 
> -----Original Message-----
> From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Sent: Wednesday, December 2, 2020 3:04 PM
> To: Frederic Barrat <fbarrat@linux.ibm.com>; Lulu Su/WHQ/Wistron 
> <Lulu_Su@wistron.com>; skiboot@lists.ozlabs.org; Stewart Smith 
> <stewart@linux.vnet.ibm.com>
> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>; 
> skiboot-stable@lists.ozlabs.org; Klaus Heinrich Kiwi 
> <klausk@br.ibm.com>
> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by 
> default
> 
> On 11/9/20 10:06 PM, Frederic Barrat wrote:
>>
>>
>> On 30/10/2020 03:22, Lulu Su wrote:
>>> From: LuluTHSu<Lulu_Su@wistron.com>
>>>
>>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>>> For mowgli platform spec.
>>>
>>> Cc:skiboot-stable@lists.ozlabs.org
>>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>>> ---
>>>    hw/phb4.c                 | 21 +++++++++++++++++++++
>>>    include/phb4.h            |  1 +
>>>    platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>>    3 files changed, 38 insertions(+)
>>>
>>> diff --git a/hw/phb4.c b/hw/phb4.c
>>> index 17a233f..de10bb0 100644
>>> --- a/hw/phb4.c
>>> +++ b/hw/phb4.c
>>> @@ -2991,6 +2991,27 @@ static unsigned int 
>>> phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>>>        return max_link_speed;
>>>    }
>>> +/*
>>> + * Has the same effect as the ibm,max-link-speed property.
>>> + * i.e. sets the default link speed, while allowing NVRAM
>>> + * overrides, etc to still take effect.
>>> + */
>>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max) {
>>> +    uint64_t scr;
>>> +    int max;
>>> +
>>> +    /* take into account nvram settings, etc */
>>> +    if (pcie_max_link_speed)
>>> +        max = pcie_max_link_speed;
>>> +    else
>>> +        max = new_max;
>>> +
>>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr); }
>>> +
>>
>>
>> This patch has already been merged, but I think there's a problem 
>> here. This should work to limit the link speed at boot, when the link 
>> is trained for the first time. However, since we're writing directly 
>> in the register, skiboot won't memorize the limit and on the next 
>> link reset/retrain (due to a EEH recovery or PCI hotplug for 
>> example), then that is lost. And mowgli_setup_phb() won't be called either on that path.
> 
> 
> Lulu,
> 
> Can you guys fix this issue?
> 
> -Vasant
> 
> ----------------------------------------------------------------------
> ----------------------------------------------------------------------
> ------------------- This email contains confidential or legally 
> privileged information and is for the sole use of its intended recipient.
> Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
> If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
> ----------------------------------------------------------------------
> ----------------------------------------------------------------------
> -------------------
>
Frederic Barrat Dec. 9, 2020, 7:29 a.m. UTC | #6
Hi Lulu,

The resulting patch looks good to me. But please submit it as 2 separate 
patches: patch 1/2 for the revert and patch 2/2 for the code change. It 
makes it easier to track what is going on.

   Fred


On 09/12/2020 06:26, Lulu_Su@wistron.com wrote:
> Hi Frederic,
> 
> The attached patch has been modified according to what you mentioned.
> And use EEH to verify, the result is the same as expected.
> If you think there is no problem, I will upload this patch to Skiboot.
> Thank you very much for your methods and suggestions.
> 
> 	Lulu
> -----Original Message-----
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> Sent: Wednesday, December 9, 2020 2:33 AM
> To: Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; hegdevasant@linux.vnet.ibm.com; skiboot@lists.ozlabs.org; stewart@linux.vnet.ibm.com
> Cc: oliveroh@au1.ibm.com; skiboot-stable@lists.ozlabs.org; klausk@br.ibm.com
> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by default
> 
> Hi,
> 
> Attached is a patch for you to try. You will also need to revert the previous patch (5262cdd1b99f77bca5951fc8132f9795ef0c2b87), since we no longer need the phb4_set_dt_max_link_speed() function.
> 
>     Fred
> 
> 
> On 08/12/2020 12:50, Lulu_Su@wistron.com wrote:
>> Hi Vasant,
>>
>> Thank you for your reminder. I have a question for your opinion:
>> Finally, there is a branch used by mowgli in ibm-op-release/op-build:
>> release-mowgli, Maybe I can refer to the previous approach
>> https://github.com/ibm-op-release/op-build/blob/release-mowgli/openpower/configs/mihawk_defconfig#L3 , build a dedicated mowgli-patches, and add the modification of phb4_get_max_link_speed(), set max_link_speed to GEN3.
>> What do you think?
>>
>>
>>
>> Hi Frederic,
>>
>> Thanks for your advice,
>> I tried to make some modifications (attached files) and verified
>> through EEH, the max-link-speed of this modification will not be lost, Do you have any suggestions for this modification?
>>
>> But as you mentioned, there is an easier way to achieve this.
>> I would like to ask how to set/add this property (ibm, max-link-speed) for PHB 0?
>>
>> If there is any problem, please feel free to contact me.
>> Thank you very much.
>>
>> Best regards,
>> Lulu Su
>>
>> -----Original Message-----
>> From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> Sent: Wednesday, December 2, 2020 3:04 PM
>> To: Frederic Barrat <fbarrat@linux.ibm.com>; Lulu Su/WHQ/Wistron
>> <Lulu_Su@wistron.com>; skiboot@lists.ozlabs.org; Stewart Smith
>> <stewart@linux.vnet.ibm.com>
>> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>;
>> skiboot-stable@lists.ozlabs.org; Klaus Heinrich Kiwi
>> <klausk@br.ibm.com>
>> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by
>> default
>>
>> On 11/9/20 10:06 PM, Frederic Barrat wrote:
>>>
>>>
>>> On 30/10/2020 03:22, Lulu Su wrote:
>>>> From: LuluTHSu<Lulu_Su@wistron.com>
>>>>
>>>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>>>> For mowgli platform spec.
>>>>
>>>> Cc:skiboot-stable@lists.ozlabs.org
>>>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>>>> ---
>>>>     hw/phb4.c                 | 21 +++++++++++++++++++++
>>>>     include/phb4.h            |  1 +
>>>>     platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>>>     3 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/hw/phb4.c b/hw/phb4.c
>>>> index 17a233f..de10bb0 100644
>>>> --- a/hw/phb4.c
>>>> +++ b/hw/phb4.c
>>>> @@ -2991,6 +2991,27 @@ static unsigned int
>>>> phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>>>>         return max_link_speed;
>>>>     }
>>>> +/*
>>>> + * Has the same effect as the ibm,max-link-speed property.
>>>> + * i.e. sets the default link speed, while allowing NVRAM
>>>> + * overrides, etc to still take effect.
>>>> + */
>>>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max) {
>>>> +    uint64_t scr;
>>>> +    int max;
>>>> +
>>>> +    /* take into account nvram settings, etc */
>>>> +    if (pcie_max_link_speed)
>>>> +        max = pcie_max_link_speed;
>>>> +    else
>>>> +        max = new_max;
>>>> +
>>>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>>>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>>>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr); }
>>>> +
>>>
>>>
>>> This patch has already been merged, but I think there's a problem
>>> here. This should work to limit the link speed at boot, when the link
>>> is trained for the first time. However, since we're writing directly
>>> in the register, skiboot won't memorize the limit and on the next
>>> link reset/retrain (due to a EEH recovery or PCI hotplug for
>>> example), then that is lost. And mowgli_setup_phb() won't be called either on that path.
>>
>>
>> Lulu,
>>
>> Can you guys fix this issue?
>>
>> -Vasant
>>
>> ----------------------------------------------------------------------
>> ----------------------------------------------------------------------
>> ------------------- This email contains confidential or legally
>> privileged information and is for the sole use of its intended recipient.
>> Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
>> If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
>> ----------------------------------------------------------------------
>> ----------------------------------------------------------------------
>> -------------------
>>
Lulu Su Dec. 9, 2020, 8:38 a.m. UTC | #7
Hi Frederic,

Okay, I modified this patch as you said and resubmitted it. 
Thank you very much.

 Lulu Su

-----Original Message-----
From: Frederic Barrat <fbarrat@linux.ibm.com> 
Sent: Wednesday, December 9, 2020 3:30 PM
To: Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; hegdevasant@linux.vnet.ibm.com; skiboot@lists.ozlabs.org; stewart@linux.vnet.ibm.com
Cc: skiboot-stable@lists.ozlabs.org; klausk@br.ibm.com
Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by default

Hi Lulu,

The resulting patch looks good to me. But please submit it as 2 separate
patches: patch 1/2 for the revert and patch 2/2 for the code change. It makes it easier to track what is going on.

   Fred


On 09/12/2020 06:26, Lulu_Su@wistron.com wrote:
> Hi Frederic,
> 
> The attached patch has been modified according to what you mentioned.
> And use EEH to verify, the result is the same as expected.
> If you think there is no problem, I will upload this patch to Skiboot.
> Thank you very much for your methods and suggestions.
> 
> 	Lulu
> -----Original Message-----
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> Sent: Wednesday, December 9, 2020 2:33 AM
> To: Lulu Su/WHQ/Wistron <Lulu_Su@wistron.com>; 
> hegdevasant@linux.vnet.ibm.com; skiboot@lists.ozlabs.org; 
> stewart@linux.vnet.ibm.com
> Cc: oliveroh@au1.ibm.com; skiboot-stable@lists.ozlabs.org; 
> klausk@br.ibm.com
> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by 
> default
> 
> Hi,
> 
> Attached is a patch for you to try. You will also need to revert the previous patch (5262cdd1b99f77bca5951fc8132f9795ef0c2b87), since we no longer need the phb4_set_dt_max_link_speed() function.
> 
>     Fred
> 
> 
> On 08/12/2020 12:50, Lulu_Su@wistron.com wrote:
>> Hi Vasant,
>>
>> Thank you for your reminder. I have a question for your opinion:
>> Finally, there is a branch used by mowgli in ibm-op-release/op-build:
>> release-mowgli, Maybe I can refer to the previous approach
>> https://github.com/ibm-op-release/op-build/blob/release-mowgli/openpower/configs/mihawk_defconfig#L3 , build a dedicated mowgli-patches, and add the modification of phb4_get_max_link_speed(), set max_link_speed to GEN3.
>> What do you think?
>>
>>
>>
>> Hi Frederic,
>>
>> Thanks for your advice,
>> I tried to make some modifications (attached files) and verified 
>> through EEH, the max-link-speed of this modification will not be lost, Do you have any suggestions for this modification?
>>
>> But as you mentioned, there is an easier way to achieve this.
>> I would like to ask how to set/add this property (ibm, max-link-speed) for PHB 0?
>>
>> If there is any problem, please feel free to contact me.
>> Thank you very much.
>>
>> Best regards,
>> Lulu Su
>>
>> -----Original Message-----
>> From: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> Sent: Wednesday, December 2, 2020 3:04 PM
>> To: Frederic Barrat <fbarrat@linux.ibm.com>; Lulu Su/WHQ/Wistron 
>> <Lulu_Su@wistron.com>; skiboot@lists.ozlabs.org; Stewart Smith 
>> <stewart@linux.vnet.ibm.com>
>> Cc: Oliver OHalloran <oliveroh@au1.ibm.com>; 
>> skiboot-stable@lists.ozlabs.org; Klaus Heinrich Kiwi 
>> <klausk@br.ibm.com>
>> Subject: Re: [Skiboot] [PATCH 1/2] mowgli: Limit slot1 to Gen3 by 
>> default
>>
>> On 11/9/20 10:06 PM, Frederic Barrat wrote:
>>>
>>>
>>> On 30/10/2020 03:22, Lulu Su wrote:
>>>> From: LuluTHSu<Lulu_Su@wistron.com>
>>>>
>>>> Refer to the spec. of mowgli, limit the slot to Gen3 speed.
>>>> For mowgli platform spec.
>>>>
>>>> Cc:skiboot-stable@lists.ozlabs.org
>>>> Signed-off-by: LuluTHSu<Lulu_Su@wistron.com>
>>>> ---
>>>>     hw/phb4.c                 | 21 +++++++++++++++++++++
>>>>     include/phb4.h            |  1 +
>>>>     platforms/astbmc/mowgli.c | 16 ++++++++++++++++
>>>>     3 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/hw/phb4.c b/hw/phb4.c
>>>> index 17a233f..de10bb0 100644
>>>> --- a/hw/phb4.c
>>>> +++ b/hw/phb4.c
>>>> @@ -2991,6 +2991,27 @@ static unsigned int 
>>>> phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
>>>>         return max_link_speed;
>>>>     }
>>>> +/*
>>>> + * Has the same effect as the ibm,max-link-speed property.
>>>> + * i.e. sets the default link speed, while allowing NVRAM
>>>> + * overrides, etc to still take effect.
>>>> + */
>>>> +void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max) {
>>>> +    uint64_t scr;
>>>> +    int max;
>>>> +
>>>> +    /* take into account nvram settings, etc */
>>>> +    if (pcie_max_link_speed)
>>>> +        max = pcie_max_link_speed;
>>>> +    else
>>>> +        max = new_max;
>>>> +
>>>> +    scr = phb4_read_reg(p, PHB_PCIE_SCR);
>>>> +    scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
>>>> +    phb4_write_reg(p, PHB_PCIE_SCR, scr); }
>>>> +
>>>
>>>
>>> This patch has already been merged, but I think there's a problem 
>>> here. This should work to limit the link speed at boot, when the 
>>> link is trained for the first time. However, since we're writing 
>>> directly in the register, skiboot won't memorize the limit and on 
>>> the next link reset/retrain (due to a EEH recovery or PCI hotplug 
>>> for example), then that is lost. And mowgli_setup_phb() won't be called either on that path.
>>
>>
>> Lulu,
>>
>> Can you guys fix this issue?
>>
>> -Vasant
>>
>> ---------------------------------------------------------------------
>> -
>> ---------------------------------------------------------------------
>> -
>> ------------------- This email contains confidential or legally 
>> privileged information and is for the sole use of its intended recipient.
>> Any unauthorized review, use, copying or distribution of this email or the content of this email is strictly prohibited.
>> If you are not the intended recipient, you may reply to the sender and should delete this e-mail immediately.
>> ---------------------------------------------------------------------
>> -
>> ---------------------------------------------------------------------
>> -
>> -------------------
>>
diff mbox series

Patch

diff --git a/hw/phb4.c b/hw/phb4.c
index 17a233f..de10bb0 100644
--- a/hw/phb4.c
+++ b/hw/phb4.c
@@ -2991,6 +2991,27 @@  static unsigned int phb4_get_max_link_speed(struct phb4 *p, struct dt_node *np)
 	return max_link_speed;
 }
 
+/*
+ * Has the same effect as the ibm,max-link-speed property.
+ * i.e. sets the default link speed, while allowing NVRAM
+ * overrides, etc to still take effect.
+ */
+void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max)
+{
+	uint64_t scr;
+	int max;
+
+	/* take into account nvram settings, etc */
+	if (pcie_max_link_speed) 
+		max = pcie_max_link_speed;
+	else 
+		max = new_max;
+	
+	scr = phb4_read_reg(p, PHB_PCIE_SCR);
+	scr = SETFIELD(PHB_PCIE_SCR_MAXLINKSPEED, scr, max);
+	phb4_write_reg(p, PHB_PCIE_SCR, scr);
+}
+
 static void phb4_assert_perst(struct pci_slot *slot, bool assert)
 {
 	struct phb4 *p = phb_to_phb4(slot->phb);
diff --git a/include/phb4.h b/include/phb4.h
index abba2d9..47a46b7 100644
--- a/include/phb4.h
+++ b/include/phb4.h
@@ -259,4 +259,5 @@  static inline int phb4_get_opal_id(unsigned int chip_id, unsigned int index)
 
 void phb4_pec2_dma_engine_realloc(struct phb4 *p);
 
+void phb4_set_dt_max_link_speed(struct phb4 *p, int new_max);
 #endif /* __PHB4_H */
diff --git a/platforms/astbmc/mowgli.c b/platforms/astbmc/mowgli.c
index 9bfe7a4..0246bff 100644
--- a/platforms/astbmc/mowgli.c
+++ b/platforms/astbmc/mowgli.c
@@ -12,6 +12,8 @@ 
 #include <psi.h>
 #include <npu-regs.h>
 #include <secvar.h>
+#include <pci.h>
+#include <phb4.h>
 
 #include "astbmc.h"
 
@@ -71,6 +73,19 @@  static int mowgli_secvar_init(void)
 	return secvar_main(secboot_tpm_driver, edk2_compatible_v1);
 }
 
+/*
+ * Limit PHB0/(pec0) to gen3 speeds.
+ */
+static void mowgli_setup_phb(struct phb *phb, unsigned int __unused index)
+{
+	struct phb4 *p = phb_to_phb4(phb);
+
+ 	if (p->pec == 0) {
+		phb4_set_dt_max_link_speed(p, 3);
+		prlog(PR_DEBUG, "Mowgli: Force the PHB%d Speed to Gen3.\n", p->pec);
+	} 
+}
+
 DECLARE_PLATFORM(mowgli) = {
 	.name			= "Mowgli",
 	.probe			= mowgli_probe,
@@ -80,6 +95,7 @@  DECLARE_PLATFORM(mowgli) = {
 	.bmc			= &bmc_plat_ast2500_openbmc,
 	.pci_get_slot_info	= slot_table_get_slot_info,
 	.pci_probe_complete	= check_all_slot_table,
+	.pci_setup_phb 		= mowgli_setup_phb,
 	.cec_power_down         = astbmc_ipmi_power_down,
 	.cec_reboot             = astbmc_ipmi_reboot,
 	.elog_commit		= ipmi_elog_commit,