Re: [Patch v2] thunderbolt: Add quirk to reset downstream port

[Sanath S <sanaths2@amd.com>]

On 11/22/2023 9:13 PM, Mario Limonciello wrote:
> On 11/22/2023 00:03, Mika Westerberg wrote:
>> Hi,
>>
>> On Wed, Nov 22, 2023 at 10:36:39AM +0530, Sanath S wrote:
>>> Boot firmware on AMD's Yellow Carp and Pink Sardine allocates
>>> very minimal buses for PCIe downstream ports. This results in
>>> failure to extend the daisy chain.
>>>
>>> Add quirk to reset the downstream port to help reset the topology
>>> created by boot firmware.
>>
>> But this resets the USB4 side of ports, how does this help with the PCIe
>> side? Or this also resets the PCIe side? Please add this information to
>> the changelog too.
>
Sure, I'll add the PCIe side reset in changelog.
> IIUC the PCIe side will be implicitly reset as well.
>
>>
>> I suppose it is not possible to fix the boot firmware?
>
> It's a really difficult case to make with firmware team.  Windows and 
> Linux have a different behavior here.  The Windows CM doesn't take the 
> existing tunnels from firmware and instead always resets them.
> So Windows "isn't affected" by this problem.
>
> Furthermore there are already lots of systems out "in the wild" as 
> these are already both production silicon with shipping OEM products.
>
>>
>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>>> Signed-off-by: Sanath S <Sanath.S@amd.com>
>>> Fixes: e390909ac763 ("thunderbolt: Add vendor specific NHI quirk for 
>>> auto-clearing interrupt status")
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> Changes since v1:
>>>   - Initialize ret variable to avoid compiler warning.
>>>   - Add Fixes tag with commit id.
>>> ---
>>>
>>>   drivers/thunderbolt/quirks.c  | 14 ++++++++++++++
>>>   drivers/thunderbolt/switch.c  | 28 ++++++++++++++++++++++++++++
>>>   drivers/thunderbolt/tb.h      |  2 ++
>>>   drivers/thunderbolt/tb_regs.h |  1 +
>>>   4 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/thunderbolt/quirks.c 
>>> b/drivers/thunderbolt/quirks.c
>>> index e6bfa63b40ae..45e9d6c43e4a 100644
>>> --- a/drivers/thunderbolt/quirks.c
>>> +++ b/drivers/thunderbolt/quirks.c
>>> @@ -27,6 +27,12 @@ static void quirk_clx_disable(struct tb_switch *sw)
>>>       tb_sw_dbg(sw, "disabling CL states\n");
>>>   }
>>>   +static void quirk_amd_downstream_port_reset(struct tb_switch *sw)
>>> +{
>>> +    sw->quirks |= QUIRK_DPR;
>>> +    tb_sw_dbg(sw, "Resetting Down Stream Port\n");
>>
>> That's "resetting downstream ports\n"
>>
Ack, Will take care in v3.
>>> +}
>>> +
>>>   static void quirk_usb3_maximum_bandwidth(struct tb_switch *sw)
>>>   {
>>>       struct tb_port *port;
>>> @@ -93,6 +99,14 @@ static const struct tb_quirk tb_quirks[] = {
>>>       { 0x0438, 0x0209, 0x0000, 0x0000, quirk_clx_disable },
>>>       { 0x0438, 0x020a, 0x0000, 0x0000, quirk_clx_disable },
>>>       { 0x0438, 0x020b, 0x0000, 0x0000, quirk_clx_disable },
>>> +    /*
>>> +     * Reset Down Stream Ports on AMD USB4 Yellow Carp and
>>> +     * Pink Sardine platforms.
>>> +     */
>>> +    { 0x0438, 0x0208, 0x0000, 0x0000, 
>>> quirk_amd_downstream_port_reset },
>>> +    { 0x0438, 0x0209, 0x0000, 0x0000, 
>>> quirk_amd_downstream_port_reset },
>>> +    { 0x0438, 0x020a, 0x0000, 0x0000, 
>>> quirk_amd_downstream_port_reset },
>>> +    { 0x0438, 0x020b, 0x0000, 0x0000, 
>>> quirk_amd_downstream_port_reset },
>>>   };
>>>     /**
>>> diff --git a/drivers/thunderbolt/switch.c 
>>> b/drivers/thunderbolt/switch.c
>>> index 1e15ffa79295..1c4b1dd5f472 100644
>>> --- a/drivers/thunderbolt/switch.c
>>> +++ b/drivers/thunderbolt/switch.c
>>> @@ -1547,6 +1547,23 @@ static void tb_dump_switch(const struct tb 
>>> *tb, const struct tb_switch *sw)
>>>              regs->__unknown1, regs->__unknown4);
>>>   }
>>>   +static int tb_switch_reset_downstream_port(struct tb_switch *sw)
>>> +{
>>> +    struct tb_port *port;
>>> +    uint32_t val = 0;
>>
>> u32
>>
Ack.
>>> +    int ret = -1;
>>
>> What is -1? Please use proper error codes.
>>
Ack, It'll be ret = -ENODEV;
>>> +
>>> +    tb_switch_for_each_port(sw, port) {
>>> +        if (port->config.type == TB_TYPE_PORT) {
>>
>> You mean
>>
>>     tb_port_is_null()
>>
>> also please make it a separate function, tb_port_reset() following the
>> similar tb_port_unlock() and friends. With the matching kernel-doc and
>> everything.
>>
Sure, I'll handle this and send a v3.
Will also take case of 10ms delay as per spec.
>>> +            val = val | PORT_CS_19_DPR;
>>> +            ret = tb_port_write(port, &val, TB_CFG_PORT,
>>> +                    port->cap_usb4 + PORT_CS_19, 1);
>>
>> Since it is using cap_usb4 you probably need to make usb4_port_reset()
>> as well that gets called from tb_port_reset() (try to make it as simple
>> as possible though).
>>
>>> +            break;
>>
>> It is OK just to reset one port?
>>
As per spec, setting the DPR bit of downstream port would help us 
reconfigure

the USB4 link, So had a condition check only for downstream port.

>>> +        }
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>>   /**
>>>    * tb_switch_reset() - reconfigure route, enable and send 
>>> TB_CFG_PKG_RESET
>>>    * @sw: Switch to reset
>>> @@ -3201,6 +3218,17 @@ int tb_switch_add(struct tb_switch *sw)
>>>               return ret;
>>>       }
>>>   +    /*
>>> +     * PCIe resource allocated by boot firmware is not utilizing 
>>> all the
>>> +     * available buses, So perform reset of topology to avoid 
>>> failure in
>>> +     * extending daisy chain.
>>> +     */
>>
>> This comment should be inside the quirk, not here.
>>
Sure, I'll remove this comment as it is already mentioned in tb_quirks[]
>>> +    if (sw->quirks & QUIRK_DPR) {
>>> +        ret = tb_switch_reset_downstream_port(sw);
>>
>> And the name of the function should be tb_switch_reset_ports() or so.
>>
Ack.
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>>       ret = tb_switch_port_hotplug_enable(sw);
>>>       if (ret)
>>>           return ret;
>>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>>> index e299e53473ae..7a9ff53be67a 100644
>>> --- a/drivers/thunderbolt/tb.h
>>> +++ b/drivers/thunderbolt/tb.h
>>> @@ -23,6 +23,8 @@
>>>   #define QUIRK_FORCE_POWER_LINK_CONTROLLER        BIT(0)
>>>   /* Disable CLx if not supported */
>>>   #define QUIRK_NO_CLX                    BIT(1)
>>> +/* Reset Down Stream Port */
>>> +#define QUIRK_DPR                    BIT(2)
>>>     /**
>>>    * struct tb_nvm - Structure holding NVM information
>>> diff --git a/drivers/thunderbolt/tb_regs.h 
>>> b/drivers/thunderbolt/tb_regs.h
>>> index 87e4795275fe..d49530bc0d53 100644
>>> --- a/drivers/thunderbolt/tb_regs.h
>>> +++ b/drivers/thunderbolt/tb_regs.h
>>> @@ -389,6 +389,7 @@ struct tb_regs_port_header {
>>>   #define PORT_CS_18_CSA                BIT(22)
>>>   #define PORT_CS_18_TIP                BIT(24)
>>>   #define PORT_CS_19                0x13
>>> +#define PORT_CS_19_DPR                BIT(0)
>>>   #define PORT_CS_19_PC                BIT(3)
>>>   #define PORT_CS_19_PID                BIT(4)
>>>   #define PORT_CS_19_WOC                BIT(16)
>>> -- 
>>> 2.34.1
>