[PATCH 0/5] PCI: mediatek-gen3: Add MT8196 support

[PATCH 0/5] PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang]

The MT8196 is an ARM platform SoC that has the same PCIe IP as the MT8195.
It has 6 clocks like MT8195, but 2 of them are different, also it requires
additional settings in the pextpcfg registers, hence that introduce
pextpcfg in PCIe driver for these settings.

Jianjun Wang (5):
  dt-bindings: PCI: mediatek-gen3: Add MT8196 support
  PCI: mediatek-gen3: Add MT8196 support
  PCI: mediatek-gen3: Disable ASPM L0s
  PCI: mediatek-gen3: Don't reply AXI slave error
  PCI: mediatek-gen3: Keep PCIe power and clocks if suspend-to-idle

 .../bindings/pci/mediatek-pcie-gen3.yaml      |  29 ++++
 drivers/pci/controller/pcie-mediatek-gen3.c   | 129 ++++++++++++++++++
 2 files changed, 158 insertions(+)

-- 
2.46.0

[PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang]

Add compatible string and clock definition for MT8196. It has 6 clocks like
the MT8195, but 2 of them are different.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 .../bindings/pci/mediatek-pcie-gen3.yaml      | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
index f05aab2b1add..b4158a666fb6 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
@@ -51,6 +51,7 @@ properties:
               - mediatek,mt7986-pcie
               - mediatek,mt8188-pcie
               - mediatek,mt8195-pcie
+              - mediatek,mt8196-pcie
           - const: mediatek,mt8192-pcie
       - const: mediatek,mt8192-pcie
       - const: airoha,en7581-pcie
@@ -197,6 +198,34 @@ allOf:
           minItems: 1
           maxItems: 2
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - mediatek,mt8196-pcie
+    then:
+      properties:
+        clocks:
+          minItems: 6
+
+        clock-names:
+          items:
+            - const: pl_250m
+            - const: tl_26m
+            - const: peri_26m
+            - const: peri_mem
+            - const: ahb_apb
+            - const: low_power
+
+        resets:
+          minItems: 1
+          maxItems: 2
+
+        reset-names:
+          minItems: 1
+          maxItems: 2
+
   - if:
       properties:
         compatible:
-- 
2.46.0

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Krzysztof Kozlowski]

On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> +        clock-names:
> +          items:
> +            - const: pl_250m
> +            - const: tl_26m
> +            - const: peri_26m
> +            - const: peri_mem
> +            - const: ahb_apb
> +            - const: low_power
> +
> +        resets:
> +          minItems: 1
> +          maxItems: 2
> +
> +        reset-names:
> +          minItems: 1
> +          maxItems: 2

Why resets are flexible?

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> > +        clock-names:
> > +          items:
> > +            - const: pl_250m
> > +            - const: tl_26m
> > +            - const: peri_26m
> > +            - const: peri_mem
> > +            - const: ahb_apb
> > +            - const: low_power
> > +
> > +        resets:
> > +          minItems: 1
> > +          maxItems: 2
> > +
> > +        reset-names:
> > +          minItems: 1
> > +          maxItems: 2
> 
> Why resets are flexible?

There are two resets, one for MAC and another for PHY, some platforms
may only use one of them.

Would you prefer to set the number of resets to a fixed value for
specific platforms?

Thanks.

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Krzysztof Kozlowski]

On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
> On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
>>> +        clock-names:
>>> +          items:
>>> +            - const: pl_250m
>>> +            - const: tl_26m
>>> +            - const: peri_26m
>>> +            - const: peri_mem
>>> +            - const: ahb_apb
>>> +            - const: low_power
>>> +
>>> +        resets:
>>> +          minItems: 1
>>> +          maxItems: 2
>>> +
>>> +        reset-names:
>>> +          minItems: 1
>>> +          maxItems: 2
>>
>> Why resets are flexible?
> 
> There are two resets, one for MAC and another for PHY, some platforms
> may only use one of them.

Even more questions. What does it mean use? Is it there or is it not?
Platform like SoC? But this is one specific SoC, it cannot be used on
different SoC.

> 
> Would you prefer to set the number of resets to a fixed value for
> specific platforms?

Everything should be constrained to match hardware.


Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Mon, 2025-01-06 at 13:27 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
> > On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: pl_250m
> > > > +            - const: tl_26m
> > > > +            - const: peri_26m
> > > > +            - const: peri_mem
> > > > +            - const: ahb_apb
> > > > +            - const: low_power
> > > > +
> > > > +        resets:
> > > > +          minItems: 1
> > > > +          maxItems: 2
> > > > +
> > > > +        reset-names:
> > > > +          minItems: 1
> > > > +          maxItems: 2
> > > 
> > > Why resets are flexible?
> > 
> > There are two resets, one for MAC and another for PHY, some
> > platforms
> > may only use one of them.
> 
> Even more questions. What does it mean use? Is it there or is it not?

It will be used by calling the reset controller's APIs in the PCIe
controller driver. Ideally, it should be de-asserted before PCIe
initialization and should be asserted if PCIe powers down or the driver
is removed.

> Platform like SoC? But this is one specific SoC, it cannot be used on
> different SoC.

Yes, it should be SoC, each SoC have its own resets, and the number of
resets for each SoC is defined by the hardware design, most SoCs should
have one reset for MAC and one reset for PHY.

> 
> > 
> > Would you prefer to set the number of resets to a fixed value for
> > specific platforms?
> 
> Everything should be constrained to match hardware.

For MT8196, there are 2 resets. Should I use a fixed item in this case?

Thanks.

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Chen-Yu Tsai]

On Tue, Jan 7, 2025 at 4:45 PM Jianjun Wang (王建军)
<Jianjun.Wang@mediatek.com> wrote:
>
> On Mon, 2025-01-06 at 13:27 +0100, Krzysztof Kozlowski wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> >
> > On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
> > > On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > >
> > > >
> > > > On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> > > > > +        clock-names:
> > > > > +          items:
> > > > > +            - const: pl_250m
> > > > > +            - const: tl_26m
> > > > > +            - const: peri_26m
> > > > > +            - const: peri_mem
> > > > > +            - const: ahb_apb
> > > > > +            - const: low_power
> > > > > +
> > > > > +        resets:
> > > > > +          minItems: 1
> > > > > +          maxItems: 2
> > > > > +
> > > > > +        reset-names:
> > > > > +          minItems: 1
> > > > > +          maxItems: 2
> > > >
> > > > Why resets are flexible?
> > >
> > > There are two resets, one for MAC and another for PHY, some
> > > platforms
> > > may only use one of them.
> >
> > Even more questions. What does it mean use? Is it there or is it not?
>
> It will be used by calling the reset controller's APIs in the PCIe
> controller driver. Ideally, it should be de-asserted before PCIe
> initialization and should be asserted if PCIe powers down or the driver
> is removed.
>
> > Platform like SoC? But this is one specific SoC, it cannot be used on
> > different SoC.
>
> Yes, it should be SoC, each SoC have its own resets, and the number of
> resets for each SoC is defined by the hardware design, most SoCs should
> have one reset for MAC and one reset for PHY.
>
> >
> > >
> > > Would you prefer to set the number of resets to a fixed value for
> > > specific platforms?
> >
> > Everything should be constrained to match hardware.
>
> For MT8196, there are 2 resets. Should I use a fixed item in this case?

Yes. As you said, MT8196 has two resets, therefore the binding should
say it requires two resets.

So in the second part where it matches against mt8196, you should
have minItems = maxItems = 2.


ChenYu

> Thanks.
>
> >
> >
> > Best regards,
> > Krzysztof
> >

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Tue, 2025-01-07 at 17:02 +0800, Chen-Yu Tsai wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Jan 7, 2025 at 4:45 PM Jianjun Wang (王建军)
> <Jianjun.Wang@mediatek.com> wrote:
> > 
> > On Mon, 2025-01-06 at 13:27 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
> > > > On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> > > > > > +        clock-names:
> > > > > > +          items:
> > > > > > +            - const: pl_250m
> > > > > > +            - const: tl_26m
> > > > > > +            - const: peri_26m
> > > > > > +            - const: peri_mem
> > > > > > +            - const: ahb_apb
> > > > > > +            - const: low_power
> > > > > > +
> > > > > > +        resets:
> > > > > > +          minItems: 1
> > > > > > +          maxItems: 2
> > > > > > +
> > > > > > +        reset-names:
> > > > > > +          minItems: 1
> > > > > > +          maxItems: 2
> > > > > 
> > > > > Why resets are flexible?
> > > > 
> > > > There are two resets, one for MAC and another for PHY, some
> > > > platforms
> > > > may only use one of them.
> > > 
> > > Even more questions. What does it mean use? Is it there or is it
> > > not?
> > 
> > It will be used by calling the reset controller's APIs in the PCIe
> > controller driver. Ideally, it should be de-asserted before PCIe
> > initialization and should be asserted if PCIe powers down or the
> > driver
> > is removed.
> > 
> > > Platform like SoC? But this is one specific SoC, it cannot be
> > > used on
> > > different SoC.
> > 
> > Yes, it should be SoC, each SoC have its own resets, and the number
> > of
> > resets for each SoC is defined by the hardware design, most SoCs
> > should
> > have one reset for MAC and one reset for PHY.
> > 
> > > 
> > > > 
> > > > Would you prefer to set the number of resets to a fixed value
> > > > for
> > > > specific platforms?
> > > 
> > > Everything should be constrained to match hardware.
> > 
> > For MT8196, there are 2 resets. Should I use a fixed item in this
> > case?
> 
> Yes. As you said, MT8196 has two resets, therefore the binding should
> say it requires two resets.
> 
> So in the second part where it matches against mt8196, you should
> have minItems = maxItems = 2.

Got it, I'll fix it in the next version.

Thanks.

> 
> 
> ChenYu
> 
> > Thanks.
> > 
> > > 
> > > 
> > > Best regards,
> > > Krzysztof
> > > 

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Krzysztof Kozlowski]

On 07/01/2025 09:43, Jianjun Wang (王建军) wrote:
> On Mon, 2025-01-06 at 13:27 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
>>> On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
>>>> External email : Please do not click links or open attachments
>>>> until
>>>> you have verified the sender or the content.
>>>>
>>>>
>>>> On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
>>>>> +        clock-names:
>>>>> +          items:
>>>>> +            - const: pl_250m
>>>>> +            - const: tl_26m
>>>>> +            - const: peri_26m
>>>>> +            - const: peri_mem
>>>>> +            - const: ahb_apb
>>>>> +            - const: low_power
>>>>> +
>>>>> +        resets:
>>>>> +          minItems: 1
>>>>> +          maxItems: 2
>>>>> +
>>>>> +        reset-names:
>>>>> +          minItems: 1
>>>>> +          maxItems: 2
>>>>
>>>> Why resets are flexible?
>>>
>>> There are two resets, one for MAC and another for PHY, some
>>> platforms
>>> may only use one of them.
>>
>> Even more questions. What does it mean use? Is it there or is it not?
> 
> It will be used by calling the reset controller's APIs in the PCIe
> controller driver. Ideally, it should be de-asserted before PCIe
> initialization and should be asserted if PCIe powers down or the driver
> is removed.

So it is there? Then drop minItems.

> 
>> Platform like SoC? But this is one specific SoC, it cannot be used on
>> different SoC.
> 
> Yes, it should be SoC, each SoC have its own resets, and the number of
> resets for each SoC is defined by the hardware design, most SoCs should
> have one reset for MAC and one reset for PHY.

You respond with some obvious things, so this review won't work.
Properties are supposed to be constrained. Your arguments that something
else has something else, do not apply. It does not matter what something
else has.

> 
>>
>>>
>>> Would you prefer to set the number of resets to a fixed value for
>>> specific platforms?
>>
>> Everything should be constrained to match hardware.
> 
> For MT8196, there are 2 resets. Should I use a fixed item in this case?

Yes. I asked why this soc has this flexible and you speak about some
other socs.



Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Wed, 2025-01-08 at 08:16 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On 07/01/2025 09:43, Jianjun Wang (王建军) wrote:
> > On Mon, 2025-01-06 at 13:27 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On 06/01/2025 10:26, Jianjun Wang (王建军) wrote:
> > > > On Fri, 2025-01-03 at 10:10 +0100, Krzysztof Kozlowski wrote:
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > On Fri, Jan 03, 2025 at 02:00:11PM +0800, Jianjun Wang wrote:
> > > > > > +        clock-names:
> > > > > > +          items:
> > > > > > +            - const: pl_250m
> > > > > > +            - const: tl_26m
> > > > > > +            - const: peri_26m
> > > > > > +            - const: peri_mem
> > > > > > +            - const: ahb_apb
> > > > > > +            - const: low_power
> > > > > > +
> > > > > > +        resets:
> > > > > > +          minItems: 1
> > > > > > +          maxItems: 2
> > > > > > +
> > > > > > +        reset-names:
> > > > > > +          minItems: 1
> > > > > > +          maxItems: 2
> > > > > 
> > > > > Why resets are flexible?
> > > > 
> > > > There are two resets, one for MAC and another for PHY, some
> > > > platforms
> > > > may only use one of them.
> > > 
> > > Even more questions. What does it mean use? Is it there or is it
> > > not?
> > 
> > It will be used by calling the reset controller's APIs in the PCIe
> > controller driver. Ideally, it should be de-asserted before PCIe
> > initialization and should be asserted if PCIe powers down or the
> > driver
> > is removed.
> 
> So it is there? Then drop minItems.
> 
> > 
> > > Platform like SoC? But this is one specific SoC, it cannot be
> > > used on
> > > different SoC.
> > 
> > Yes, it should be SoC, each SoC have its own resets, and the number
> > of
> > resets for each SoC is defined by the hardware design, most SoCs
> > should
> > have one reset for MAC and one reset for PHY.
> 
> You respond with some obvious things, so this review won't work.
> Properties are supposed to be constrained. Your arguments that
> something
> else has something else, do not apply. It does not matter what
> something
> else has.
> 
> > 
> > > 
> > > > 
> > > > Would you prefer to set the number of resets to a fixed value
> > > > for
> > > > specific platforms?
> > > 
> > > Everything should be constrained to match hardware.
> > 
> > For MT8196, there are 2 resets. Should I use a fixed item in this
> > case?
> 
> Yes. I asked why this soc has this flexible and you speak about some
> other socs.

Got it, sorry for the noise, will fix it in the next version.

Thanks.

> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[AngeloGioacchino Del Regno]

Il 03/01/25 07:00, Jianjun Wang ha scritto:
> Add compatible string and clock definition for MT8196. It has 6 clocks like
> the MT8195, but 2 of them are different.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   .../bindings/pci/mediatek-pcie-gen3.yaml      | 29 +++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> index f05aab2b1add..b4158a666fb6 100644
> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -51,6 +51,7 @@ properties:
>                 - mediatek,mt7986-pcie
>                 - mediatek,mt8188-pcie
>                 - mediatek,mt8195-pcie
> +              - mediatek,mt8196-pcie
>             - const: mediatek,mt8192-pcie
>         - const: mediatek,mt8192-pcie
>         - const: airoha,en7581-pcie
> @@ -197,6 +198,34 @@ allOf:
>             minItems: 1
>             maxItems: 2
>   
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - mediatek,mt8196-pcie
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +
> +        clock-names:
> +          items:
> +            - const: pl_250m
> +            - const: tl_26m
> +            - const: peri_26m
> +            - const: peri_mem
> +            - const: ahb_apb

ahb_apb is a bus clock, so you can set it as

- const: bus


> +            - const: low_power

Can you please clarify what the LP clock is for?

Thanks,
Angelo

> +
> +        resets:
> +          minItems: 1
> +          maxItems: 2
> +
> +        reset-names:
> +          minItems: 1
> +          maxItems: 2
> +
>     - if:
>         properties:
>           compatible:

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 10:26 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/01/25 07:00, Jianjun Wang ha scritto:
> > Add compatible string and clock definition for MT8196. It has 6
> > clocks like
> > the MT8195, but 2 of them are different.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >   .../bindings/pci/mediatek-pcie-gen3.yaml      | 29
> > +++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > gen3.yaml
> > index f05aab2b1add..b4158a666fb6 100644
> > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> > @@ -51,6 +51,7 @@ properties:
> >                 - mediatek,mt7986-pcie
> >                 - mediatek,mt8188-pcie
> >                 - mediatek,mt8195-pcie
> > +              - mediatek,mt8196-pcie
> >             - const: mediatek,mt8192-pcie
> >         - const: mediatek,mt8192-pcie
> >         - const: airoha,en7581-pcie
> > @@ -197,6 +198,34 @@ allOf:
> >             minItems: 1
> >             maxItems: 2
> > 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - mediatek,mt8196-pcie
> > +    then:
> > +      properties:
> > +        clocks:
> > +          minItems: 6
> > +
> > +        clock-names:
> > +          items:
> > +            - const: pl_250m
> > +            - const: tl_26m
> > +            - const: peri_26m
> > +            - const: peri_mem
> > +            - const: ahb_apb
> 
> ahb_apb is a bus clock, so you can set it as
> 
> - const: bus

Agree, I'll change it to "bus" in the next version, thanks.

> 
> 
> > +            - const: low_power
> 
> Can you please clarify what the LP clock is for?

This is a power-saving clock. Its clock source consumes less power than
a regular clock, we need to keep this clock on if when entering L1.2
during suspend.

Thanks.

> 
> Thanks,
> Angelo
> 
> > +
> > +        resets:
> > +          minItems: 1
> > +          maxItems: 2
> > +
> > +        reset-names:
> > +          minItems: 1
> > +          maxItems: 2
> > +
> >     - if:
> >         properties:
> >           compatible:
> 
> 

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[AngeloGioacchino Del Regno]

Il 06/01/25 10:19, Jianjun Wang (王建军) ha scritto:
> On Fri, 2025-01-03 at 10:26 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 03/01/25 07:00, Jianjun Wang ha scritto:
>>> Add compatible string and clock definition for MT8196. It has 6
>>> clocks like
>>> the MT8195, but 2 of them are different.
>>>
>>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
>>> ---
>>>    .../bindings/pci/mediatek-pcie-gen3.yaml      | 29
>>> +++++++++++++++++++
>>>    1 file changed, 29 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-
>>> gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie-
>>> gen3.yaml
>>> index f05aab2b1add..b4158a666fb6 100644
>>> --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
>>> @@ -51,6 +51,7 @@ properties:
>>>                  - mediatek,mt7986-pcie
>>>                  - mediatek,mt8188-pcie
>>>                  - mediatek,mt8195-pcie
>>> +              - mediatek,mt8196-pcie
>>>              - const: mediatek,mt8192-pcie
>>>          - const: mediatek,mt8192-pcie
>>>          - const: airoha,en7581-pcie
>>> @@ -197,6 +198,34 @@ allOf:
>>>              minItems: 1
>>>              maxItems: 2
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - mediatek,mt8196-pcie
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 6
>>> +
>>> +        clock-names:
>>> +          items:
>>> +            - const: pl_250m
>>> +            - const: tl_26m
>>> +            - const: peri_26m
>>> +            - const: peri_mem
>>> +            - const: ahb_apb
>>
>> ahb_apb is a bus clock, so you can set it as
>>
>> - const: bus
> 
> Agree, I'll change it to "bus" in the next version, thanks.
> 
>>
>>
>>> +            - const: low_power
>>
>> Can you please clarify what the LP clock is for?
> 
> This is a power-saving clock. Its clock source consumes less power than
> a regular clock, we need to keep this clock on if when entering L1.2
> during suspend.
> 

In the driver, you are keeping all clocks ON instead.

Is this clock required to be ON when the full power ones are enabled and
the SoC is not in suspend state?

Can you please add handling for this "special" clock so that we can save power
during suspend?

Cheers,
Angelo

> Thanks.
> 
>>
>> Thanks,
>> Angelo
>>
>>> +
>>> +        resets:
>>> +          minItems: 1
>>> +          maxItems: 2
>>> +
>>> +        reset-names:
>>> +          minItems: 1
>>> +          maxItems: 2
>>> +
>>>      - if:
>>>          properties:
>>>            compatible:
>>
>>

Re: [PATCH 1/5] dt-bindings: PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Tue, 2025-01-07 at 14:04 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 06/01/25 10:19, Jianjun Wang (王建军) ha scritto:
> > On Fri, 2025-01-03 at 10:26 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > Il 03/01/25 07:00, Jianjun Wang ha scritto:
> > > > Add compatible string and clock definition for MT8196. It has 6
> > > > clocks like
> > > > the MT8195, but 2 of them are different.
> > > > 
> > > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > > > ---
> > > >    .../bindings/pci/mediatek-pcie-gen3.yaml      | 29
> > > > +++++++++++++++++++
> > > >    1 file changed, 29 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/pci/mediatek-
> > > > pcie-
> > > > gen3.yaml b/Documentation/devicetree/bindings/pci/mediatek-
> > > > pcie-
> > > > gen3.yaml
> > > > index f05aab2b1add..b4158a666fb6 100644
> > > > --- a/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > > > gen3.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-
> > > > gen3.yaml
> > > > @@ -51,6 +51,7 @@ properties:
> > > >                  - mediatek,mt7986-pcie
> > > >                  - mediatek,mt8188-pcie
> > > >                  - mediatek,mt8195-pcie
> > > > +              - mediatek,mt8196-pcie
> > > >              - const: mediatek,mt8192-pcie
> > > >          - const: mediatek,mt8192-pcie
> > > >          - const: airoha,en7581-pcie
> > > > @@ -197,6 +198,34 @@ allOf:
> > > >              minItems: 1
> > > >              maxItems: 2
> > > > 
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - mediatek,mt8196-pcie
> > > > +    then:
> > > > +      properties:
> > > > +        clocks:
> > > > +          minItems: 6
> > > > +
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: pl_250m
> > > > +            - const: tl_26m
> > > > +            - const: peri_26m
> > > > +            - const: peri_mem
> > > > +            - const: ahb_apb
> > > 
> > > ahb_apb is a bus clock, so you can set it as
> > > 
> > > - const: bus
> > 
> > Agree, I'll change it to "bus" in the next version, thanks.
> > 
> > > 
> > > 
> > > > +            - const: low_power
> > > 
> > > Can you please clarify what the LP clock is for?
> > 
> > This is a power-saving clock. Its clock source consumes less power
> > than
> > a regular clock, we need to keep this clock on if when entering
> > L1.2
> > during suspend.
> > 
> 
> In the driver, you are keeping all clocks ON instead.
> 
> Is this clock required to be ON when the full power ones are enabled
> and
> the SoC is not in suspend state?

This clock affects the exit from L1.2 via the CLKREQ# signal, and ASPM
L1.2 can be entered in the normal state. Therefore, we need to keep it
on like the other clocks.

> 
> Can you please add handling for this "special" clock so that we can
> save power
> during suspend?

When entering the L1.2 state, this clock is the only clock needed by
the PCIe HW. However, since ASPM L1.2 is controlled by HW and we don't
know when it will exit(e.g., exit by the device side), we still need to
keep other clocks on to ensure the PCIe is ready to work.

Thanks.

> 
> Cheers,
> Angelo
> 
> > Thanks.
> > 
> > > 
> > > Thanks,
> > > Angelo
> > > 
> > > > +
> > > > +        resets:
> > > > +          minItems: 1
> > > > +          maxItems: 2
> > > > +
> > > > +        reset-names:
> > > > +          minItems: 1
> > > > +          maxItems: 2
> > > > +
> > > >      - if:
> > > >          properties:
> > > >            compatible:
> > > 
> > > 
> 
> 
> 

[PATCH 2/5] PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang]

The MT8196 is an ARM platform SoC that has the same PCIe IP as the
MT8195.
However, it requires additional settings in the pextpcfg registers.
Introduce pextpcfg in PCIe driver for these settings.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 88 +++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index be52e3a123ab..ed3c0614486c 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
@@ -123,6 +124,17 @@
 #define PCIE_ATR_TLP_TYPE_MEM		PCIE_ATR_TLP_TYPE(0)
 #define PCIE_ATR_TLP_TYPE_IO		PCIE_ATR_TLP_TYPE(2)
 
+#define PCIE_RESOURCE_CTRL_REG		0xd2c
+#define PCIE_SYS_CLK_RDY_TIME_MASK	GENMASK(7, 0)
+#define PCIE_SYS_CLK_RDY_TIME_TO_10US	0xa
+
+/* PEXTPCFG Registers */
+#define PEXTP_CLOCK_CON_REG		0x20
+#define PEXTP_P0P1_LOWPOWER_CK_SEL	BIT(0)
+#define PEXTP_REQ_CTRL_0_REG		0x7c
+#define PEXTP_26M_REQ_FORCE_ON		BIT(0)
+#define PEXTP_PCIE26M_BYPASS		BIT(4)
+
 #define MAX_NUM_PHY_RESETS		3
 
 /* Time in ms needed to complete PCIe reset on EN7581 SoC */
@@ -136,10 +148,14 @@ struct mtk_gen3_pcie;
 /**
  * struct mtk_gen3_pcie_pdata - differentiate between host generations
  * @power_up: pcie power_up callback
+ * @pre_init: initialize settings before link up
+ * @cleanup: cleanup when PCIe power down
  * @phy_resets: phy reset lines SoC data.
  */
 struct mtk_gen3_pcie_pdata {
 	int (*power_up)(struct mtk_gen3_pcie *pcie);
+	int (*pre_init)(struct mtk_gen3_pcie *pcie);
+	void (*cleanup)(struct mtk_gen3_pcie *pcie);
 	struct {
 		const char *id[MAX_NUM_PHY_RESETS];
 		int num_resets;
@@ -162,6 +178,7 @@ struct mtk_msi_set {
  * struct mtk_gen3_pcie - PCIe port information
  * @dev: pointer to PCIe device
  * @base: IO mapped register base
+ * @pextpcfg: pextpcfg_ao IO mapped register base
  * @reg_base: physical register base
  * @mac_reset: MAC reset control
  * @phy_resets: PHY reset controllers
@@ -184,6 +201,7 @@ struct mtk_msi_set {
 struct mtk_gen3_pcie {
 	struct device *dev;
 	void __iomem *base;
+	void __iomem *pextpcfg;
 	phys_addr_t reg_base;
 	struct reset_control *mac_reset;
 	struct reset_control_bulk_data phy_resets[MAX_NUM_PHY_RESETS];
@@ -422,6 +440,13 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 		writel_relaxed(val, pcie->base + PCIE_CONF_LINK2_CTL_STS);
 	}
 
+	/*
+	 * The values of some registers are different in RC and EP mode. Therefore,
+	 * call soc->pre_init after the mode change in case it depends on these registers.
+	 */
+	if (pcie->soc && pcie->soc->pre_init)
+		pcie->soc->pre_init(pcie);
+
 	/* Set class code */
 	val = readl_relaxed(pcie->base + PCIE_PCI_IDS_1);
 	val &= ~GENMASK(31, 8);
@@ -848,6 +873,7 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 	int i, ret, num_resets = pcie->soc->phy_resets.num_resets;
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node;
 	struct resource *regs;
 	u32 num_lanes;
 
@@ -903,6 +929,18 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 			pcie->num_lanes = num_lanes;
        }
 
+	node = of_parse_phandle(dev->of_node, "pextpcfg", 0);
+	if (node) {
+		pcie->pextpcfg = of_iomap(node, 0);
+		of_node_put(node);
+		if (IS_ERR(pcie->pextpcfg)) {
+			dev_err(dev, "failed to get pextpcfg\n");
+			ret = PTR_ERR(pcie->pextpcfg);
+			pcie->pextpcfg = NULL;
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -1047,6 +1085,12 @@ static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
 	phy_power_off(pcie->phy);
 	phy_exit(pcie->phy);
 	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
+
+	if (pcie->soc && pcie->soc->cleanup)
+		pcie->soc->cleanup(pcie);
+
+	if (pcie->pextpcfg)
+		iounmap(pcie->pextpcfg);
 }
 
 static int mtk_pcie_get_controller_max_link_speed(struct mtk_gen3_pcie *pcie)
@@ -1277,6 +1321,49 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
 	},
 };
 
+static int mtk_pcie_mt8196_pre_init(struct mtk_gen3_pcie *pcie)
+{
+	u32 val;
+
+	/* Adjust SYS_CLK_RDY_TIME ot 10us to avoid glitch */
+	val = readl_relaxed(pcie->base + PCIE_RESOURCE_CTRL_REG);
+	val &= ~PCIE_SYS_CLK_RDY_TIME_MASK;
+	val |= PCIE_SYS_CLK_RDY_TIME_TO_10US;
+	writel_relaxed(val, pcie->base + PCIE_RESOURCE_CTRL_REG);
+
+	/* Switch to normal clock */
+	val = readl_relaxed(pcie->pextpcfg + PEXTP_CLOCK_CON_REG);
+	val &= ~PEXTP_P0P1_LOWPOWER_CK_SEL;
+	writel_relaxed(val, pcie->pextpcfg + PEXTP_CLOCK_CON_REG);
+
+	/* Force pcie_26m_req and bypass pcie_26m_ack signal */
+	val = readl_relaxed(pcie->pextpcfg + PEXTP_REQ_CTRL_0_REG);
+	val |= (PEXTP_26M_REQ_FORCE_ON | PEXTP_PCIE26M_BYPASS);
+	writel_relaxed(val, pcie->pextpcfg + PEXTP_REQ_CTRL_0_REG);
+
+	return 0;
+}
+
+static void mtk_pcie_mt8196_cleanup(struct mtk_gen3_pcie *pcie)
+{
+	u32 val;
+
+	/* Release pcie_26m_req and pcie_26m_ack signal */
+	val = readl_relaxed(pcie->pextpcfg + PEXTP_REQ_CTRL_0_REG);
+	val &= ~(PEXTP_26M_REQ_FORCE_ON | PEXTP_PCIE26M_BYPASS);
+	writel_relaxed(val, pcie->pextpcfg + PEXTP_REQ_CTRL_0_REG);
+}
+
+static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8196 = {
+	.power_up = mtk_pcie_power_up,
+	.pre_init = mtk_pcie_mt8196_pre_init,
+	.cleanup = mtk_pcie_mt8196_cleanup,
+	.phy_resets = {
+		.id[0] = "phy",
+		.num_resets = 1,
+	},
+};
+
 static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
 	.power_up = mtk_pcie_en7581_power_up,
 	.phy_resets = {
@@ -1290,6 +1377,7 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = {
 static const struct of_device_id mtk_pcie_of_match[] = {
 	{ .compatible = "airoha,en7581-pcie", .data = &mtk_pcie_soc_en7581 },
 	{ .compatible = "mediatek,mt8192-pcie", .data = &mtk_pcie_soc_mt8192 },
+	{ .compatible = "mediatek,mt8196-pcie", .data = &mtk_pcie_soc_mt8196 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_pcie_of_match);
-- 
2.46.0

Re: [PATCH 2/5] PCI: mediatek-gen3: Add MT8196 support

[Bjorn Helgaas]

On Fri, Jan 03, 2025 at 02:00:12PM +0800, Jianjun Wang wrote:
> The MT8196 is an ARM platform SoC that has the same PCIe IP as the
> MT8195.
> However, it requires additional settings in the pextpcfg registers.
> Introduce pextpcfg in PCIe driver for these settings.

Add blank lines between paragraphs.

> +	 * The values of some registers are different in RC and EP mode. Therefore,
> +	 * call soc->pre_init after the mode change in case it depends on these registers.

Wrap this to fit in 80 columns like the rest of the file.

> +	/* Adjust SYS_CLK_RDY_TIME ot 10us to avoid glitch */

s/ot/to/

Is this an erratum?  Is there any spec or erratum citation you can
include in the comment?

> +	val = readl_relaxed(pcie->base + PCIE_RESOURCE_CTRL_REG);
> +	val &= ~PCIE_SYS_CLK_RDY_TIME_MASK;
> +	val |= PCIE_SYS_CLK_RDY_TIME_TO_10US;
> +	writel_relaxed(val, pcie->base + PCIE_RESOURCE_CTRL_REG);

Re: [PATCH 2/5] PCI: mediatek-gen3: Add MT8196 support

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 13:02 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Jan 03, 2025 at 02:00:12PM +0800, Jianjun Wang wrote:
> > The MT8196 is an ARM platform SoC that has the same PCIe IP as the
> > MT8195.
> > However, it requires additional settings in the pextpcfg registers.
> > Introduce pextpcfg in PCIe driver for these settings.
> 
> Add blank lines between paragraphs.
> 
> > +      * The values of some registers are different in RC and EP
> > mode. Therefore,
> > +      * call soc->pre_init after the mode change in case it
> > depends on these registers.
> 
> Wrap this to fit in 80 columns like the rest of the file.
> 
> > +     /* Adjust SYS_CLK_RDY_TIME ot 10us to avoid glitch */
> 
> s/ot/to/
> 
> Is this an erratum?  Is there any spec or erratum citation you can
> include in the comment?

Yes, the default time for sys_clk to be ready is 3us, which is the
value we use for all other platforms.

However, for MT8196, we need to set it to 10us to avoid glitch, I'll
add more description in the comment in the next version.

Thanks.

> 
> > +     val = readl_relaxed(pcie->base + PCIE_RESOURCE_CTRL_REG);
> > +     val &= ~PCIE_SYS_CLK_RDY_TIME_MASK;
> > +     val |= PCIE_SYS_CLK_RDY_TIME_TO_10US;
> > +     writel_relaxed(val, pcie->base + PCIE_RESOURCE_CTRL_REG);
> 
> 

[PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Jianjun Wang]

Disable ASPM L0s support because it does not significantly save power
but impacts performance.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index ed3c0614486c..4bd3b39eebe2 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -84,6 +84,9 @@
 #define PCIE_MSI_SET_ENABLE_REG		0x190
 #define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
 
+#define PCIE_LOW_POWER_CTRL_REG		0x194
+#define PCIE_FORCE_DIS_L0S		BIT(8)
+
 #define PCIE_PIPE4_PIE8_REG		0x338
 #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
 #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
@@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 	val &= ~PCIE_INTX_ENABLE;
 	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
 
+	/*
+	 * Disable L0s support because it does not significantly save power
+	 * but impacts performance.
+	 */
+	val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
+	val |= PCIE_FORCE_DIS_L0S;
+	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
+
 	/* Disable DVFSRC voltage request */
 	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
 	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
-- 
2.46.0

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[AngeloGioacchino Del Regno]

Il 03/01/25 07:00, Jianjun Wang ha scritto:
> Disable ASPM L0s support because it does not significantly save power
> but impacts performance.
> 

That may be a good idea but, without numbers to support your statement, it's a bit
difficult to say.

How much power does ASPM L0s save on MediaTek SoCs, in microwatts?
How is the performance impacted, and on which specific device(s) on the PCIe bus?

Cheers,
Angelo

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index ed3c0614486c..4bd3b39eebe2 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -84,6 +84,9 @@
>   #define PCIE_MSI_SET_ENABLE_REG		0x190
>   #define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
>   
> +#define PCIE_LOW_POWER_CTRL_REG		0x194
> +#define PCIE_FORCE_DIS_L0S		BIT(8)
> +
>   #define PCIE_PIPE4_PIE8_REG		0x338
>   #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
>   #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
> @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>   	val &= ~PCIE_INTX_ENABLE;
>   	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>   
> +	/*
> +	 * Disable L0s support because it does not significantly save power
> +	 * but impacts performance.
> +	 */
> +	val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
> +	val |= PCIE_FORCE_DIS_L0S;
> +	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> +
>   	/* Disable DVFSRC voltage request */
>   	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>   	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/01/25 07:00, Jianjun Wang ha scritto:
> > Disable ASPM L0s support because it does not significantly save
> > power
> > but impacts performance.
> > 
> 
> That may be a good idea but, without numbers to support your
> statement, it's a bit
> difficult to say.
> 
> How much power does ASPM L0s save on MediaTek SoCs, in microwatts?
> How is the performance impacted, and on which specific device(s) on
> the PCIe bus?

It's hard to tell the exact number because it is difficult to measure,
and the number of entries into the L0s state may vary even in the same
test scenario.

However, we have encountered some compatibility issues when connected
with some PCIe EPs, and disabling the L0s can fix it. I think disabling
L0s might be the better way, since we usually use L1ss for power-saving 
when the link is idle.

Thanks.

> 
> Cheers,
> Angelo
> 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >   drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index ed3c0614486c..4bd3b39eebe2 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -84,6 +84,9 @@
> >   #define PCIE_MSI_SET_ENABLE_REG             0x190
> >   #define PCIE_MSI_SET_ENABLE         GENMASK(PCIE_MSI_SET_NUM - 1,
> > 0)
> > 
> > +#define PCIE_LOW_POWER_CTRL_REG              0x194
> > +#define PCIE_FORCE_DIS_L0S           BIT(8)
> > +
> >   #define PCIE_PIPE4_PIE8_REG         0x338
> >   #define PCIE_K_FINETUNE_MAX         GENMASK(5, 0)
> >   #define PCIE_K_FINETUNE_ERR         GENMASK(7, 6)
> > @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >       val &= ~PCIE_INTX_ENABLE;
> >       writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
> > 
> > +     /*
> > +      * Disable L0s support because it does not significantly save
> > power
> > +      * but impacts performance.
> > +      */
> > +     val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
> > +     val |= PCIE_FORCE_DIS_L0S;
> > +     writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> > +
> >       /* Disable DVFSRC voltage request */
> >       val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
> >       val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> 
> 

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[AngeloGioacchino Del Regno]

Il 07/01/25 03:18, Jianjun Wang (王建军) ha scritto:
> On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 03/01/25 07:00, Jianjun Wang ha scritto:
>>> Disable ASPM L0s support because it does not significantly save
>>> power
>>> but impacts performance.
>>>
>>
>> That may be a good idea but, without numbers to support your
>> statement, it's a bit
>> difficult to say.
>>
>> How much power does ASPM L0s save on MediaTek SoCs, in microwatts?
>> How is the performance impacted, and on which specific device(s) on
>> the PCIe bus?
> 
> It's hard to tell the exact number because it is difficult to measure,
> and the number of entries into the L0s state may vary even in the same
> test scenario.
> 
> However, we have encountered some compatibility issues when connected
> with some PCIe EPs, and disabling the L0s can fix it. I think disabling
> L0s might be the better way, since we usually use L1ss for power-saving
> when the link is idle.
> 

To actually decide, we should know what's actually broken, then.

Is the MediaTek controller broken, or is the device broken?
So, is it a MTK quirk, or a device quirk?

If the problem is actually device-related, then this should be handled as
a device-specific quirk, as not just MediaTek platforms would be affected
by compatibility issues.

If the MediaTek PCIe controller is at fault, instead, I agree about just
disabling L0s at the controller level - but then this shall be mentioned
in the commit message, and should have a Fixes tag as well.

Cheers,
Angelo

> Thanks.
> 
>>
>> Cheers,
>> Angelo
>>
>>> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
>>> ---
>>>    drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
>>> b/drivers/pci/controller/pcie-mediatek-gen3.c
>>> index ed3c0614486c..4bd3b39eebe2 100644
>>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
>>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>>> @@ -84,6 +84,9 @@
>>>    #define PCIE_MSI_SET_ENABLE_REG             0x190
>>>    #define PCIE_MSI_SET_ENABLE         GENMASK(PCIE_MSI_SET_NUM - 1,
>>> 0)
>>>
>>> +#define PCIE_LOW_POWER_CTRL_REG              0x194
>>> +#define PCIE_FORCE_DIS_L0S           BIT(8)
>>> +
>>>    #define PCIE_PIPE4_PIE8_REG         0x338
>>>    #define PCIE_K_FINETUNE_MAX         GENMASK(5, 0)
>>>    #define PCIE_K_FINETUNE_ERR         GENMASK(7, 6)
>>> @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct
>>> mtk_gen3_pcie *pcie)
>>>        val &= ~PCIE_INTX_ENABLE;
>>>        writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>>>
>>> +     /*
>>> +      * Disable L0s support because it does not significantly save
>>> power
>>> +      * but impacts performance.
>>> +      */
>>> +     val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
>>> +     val |= PCIE_FORCE_DIS_L0S;
>>> +     writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
>>> +
>>>        /* Disable DVFSRC voltage request */
>>>        val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>>>        val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>>
>>

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Bjorn Helgaas]

On Tue, Jan 07, 2025 at 12:44:43PM +0100, AngeloGioacchino Del Regno wrote:
> Il 07/01/25 03:18, Jianjun Wang (王建军) ha scritto:
> > On Fri, 2025-01-03 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
> > > Il 03/01/25 07:00, Jianjun Wang ha scritto:
> > > > Disable ASPM L0s support because it does not significantly save
> > > > power
> > > > but impacts performance.
> > > 
> > > That may be a good idea but, without numbers to support your
> > > statement, it's a bit
> > > difficult to say.
> > > 
> > > How much power does ASPM L0s save on MediaTek SoCs, in microwatts?
> > > How is the performance impacted, and on which specific device(s) on
> > > the PCIe bus?
> > 
> > It's hard to tell the exact number because it is difficult to measure,
> > and the number of entries into the L0s state may vary even in the same
> > test scenario.
> > 
> > However, we have encountered some compatibility issues when connected
> > with some PCIe EPs, and disabling the L0s can fix it. I think disabling
> > L0s might be the better way, since we usually use L1ss for power-saving
> > when the link is idle.
> 
> To actually decide, we should know what's actually broken, then.
> 
> Is the MediaTek controller broken, or is the device broken?
> So, is it a MTK quirk, or a device quirk?
> 
> If the problem is actually device-related, then this should be handled as
> a device-specific quirk, as not just MediaTek platforms would be affected
> by compatibility issues.
> 
> If the MediaTek PCIe controller is at fault, instead, I agree about just
> disabling L0s at the controller level - but then this shall be mentioned
> in the commit message, and should have a Fixes tag as well.

100% agreed, sorry for repeating what you just said before I finished
reading the thread!

Bjorn

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Bjorn Helgaas]

On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote:
> Disable ASPM L0s support because it does not significantly save power
> but impacts performance.

This seems like a user/administrator decision, not a driver decision.

L0s reduces power at the cost of performance for *all* PCIe devices,
although the actual numbers may vary.

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 13:15 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote:
> > Disable ASPM L0s support because it does not significantly save
> > power
> > but impacts performance.
> 
> This seems like a user/administrator decision, not a driver decision.
> 
> L0s reduces power at the cost of performance for *all* PCIe devices,
> although the actual numbers may vary.

We have encountered some compatibility issues when connected with some
PCIe EPs, these issues are probabilistic and disabling the L0s can fix
them.

Users may not be aware of these issues, so I think disabling L0s
through the driver might be the better way, since it does not
significantly save power and we usually use L1ss for power-saving when
the link is idle.

Thanks.

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Bjorn Helgaas]

On Tue, Jan 07, 2025 at 02:44:37AM +0000, Jianjun Wang (王建军) wrote:
> On Fri, 2025-01-03 at 13:15 -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote:
> > > Disable ASPM L0s support because it does not significantly save
> > > power but impacts performance.
> > 
> > This seems like a user/administrator decision, not a driver
> > decision.
> > 
> > L0s reduces power at the cost of performance for *all* PCIe
> > devices, although the actual numbers may vary.
> 
> We have encountered some compatibility issues when connected with
> some PCIe EPs, these issues are probabilistic and disabling the L0s
> can fix them.

This sounds like either a software problem in ASPM or a hardware
problem in one of the devices.  If it's a Linux ASPM issue, obviously
we should find and fix that.  If it's an endpoint hardware issue, we
should fix the driver or quirk it to avoid L0s on all platforms, not
just this one.

If it's a mediatek-gen3 hardware issue, we should disable L0s as you
do here.  But if the reason is to work around a hardware erratum, we
should describe it as such.

Justifying it as "L0s really doesn't save much power, so disable it"
is an invitation for somebody to come back and ask why L0s doesn't
work when the lspci output claims it *should* work.

> Users may not be aware of these issues, so I think disabling L0s
> through the driver might be the better way, since it does not
> significantly save power and we usually use L1ss for power-saving
> when the link is idle.

Users should not need to be aware of probabilistic behavior problems
related to ASPM.  It's *our* problem to make sure users never see
issues like that :)

Bjorn

Re: [PATCH 3/5] PCI: mediatek-gen3: Disable ASPM L0s

[Manivannan Sadhasivam]

On Fri, Jan 03, 2025 at 02:00:13PM +0800, Jianjun Wang wrote:
> Disable ASPM L0s support because it does not significantly save power
> but impacts performance.
> 

You should disable ASPM only if it is causing any functional issues to the SoC
itself. For other reasons, users will use the existing sysfs/cmdline params to
disable ASPM based on usecase if required.

- Mani

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index ed3c0614486c..4bd3b39eebe2 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -84,6 +84,9 @@
>  #define PCIE_MSI_SET_ENABLE_REG		0x190
>  #define PCIE_MSI_SET_ENABLE		GENMASK(PCIE_MSI_SET_NUM - 1, 0)
>  
> +#define PCIE_LOW_POWER_CTRL_REG		0x194
> +#define PCIE_FORCE_DIS_L0S		BIT(8)
> +
>  #define PCIE_PIPE4_PIE8_REG		0x338
>  #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
>  #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
> @@ -458,6 +461,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val &= ~PCIE_INTX_ENABLE;
>  	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>  
> +	/*
> +	 * Disable L0s support because it does not significantly save power
> +	 * but impacts performance.
> +	 */
> +	val = readl_relaxed(pcie->base + PCIE_LOW_POWER_CTRL_REG);
> +	val |= PCIE_FORCE_DIS_L0S;
> +	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> +
>  	/* Disable DVFSRC voltage request */
>  	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> -- 
> 2.46.0
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Jianjun Wang]

There are some circumstances where the EP device will not respond to
non-posted access from the root port (e.g., MMIO read). In such cases,
the root port will reply with an AXI slave error, which will be treated
as a System Error (SError), causing a kernel panic and preventing us
from obtaining any useful information for further debugging.

We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to prevent
PCIe AXI0 from replying with a slave error. Setting this bit on an older
platform that does not support this feature will have no effect.

By preventing AXI0 from replying with a slave error, we can keep the
kernel alive and debug using the information from AER.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 4bd3b39eebe2..48f83c2d91f7 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -87,6 +87,9 @@
 #define PCIE_LOW_POWER_CTRL_REG		0x194
 #define PCIE_FORCE_DIS_L0S		BIT(8)
 
+#define PCIE_AXI_IF_CTRL_REG		0x1a8
+#define PCIE_AXI0_SLV_RESP_MASK		BIT(12)
+
 #define PCIE_PIPE4_PIE8_REG		0x338
 #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
 #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
@@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 	val |= PCIE_FORCE_DIS_L0S;
 	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
 
+	/*
+	 * Prevent PCIe AXI0 from replying a slave error, as it will cause kernel panic
+	 * and prevent us from getting useful information.
+	 * Keep the kernel alive and debug using the information from AER.
+	 */
+	val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
+	val |= PCIE_AXI0_SLV_RESP_MASK;
+	writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
+
 	/* Disable DVFSRC voltage request */
 	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
 	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
-- 
2.46.0

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[AngeloGioacchino Del Regno]

Il 03/01/25 07:00, Jianjun Wang ha scritto:
> There are some circumstances where the EP device will not respond to
> non-posted access from the root port (e.g., MMIO read). In such cases,
> the root port will reply with an AXI slave error, which will be treated
> as a System Error (SError), causing a kernel panic and preventing us
> from obtaining any useful information for further debugging.
> 
> We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to prevent
> PCIe AXI0 from replying with a slave error. Setting this bit on an older
> platform that does not support this feature will have no effect.
> 
> By preventing AXI0 from replying with a slave error, we can keep the
> kernel alive and debug using the information from AER.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 4bd3b39eebe2..48f83c2d91f7 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -87,6 +87,9 @@
>   #define PCIE_LOW_POWER_CTRL_REG		0x194
>   #define PCIE_FORCE_DIS_L0S		BIT(8)
>   
> +#define PCIE_AXI_IF_CTRL_REG		0x1a8
> +#define PCIE_AXI0_SLV_RESP_MASK		BIT(12)
> +
>   #define PCIE_PIPE4_PIE8_REG		0x338
>   #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
>   #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
> @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>   	val |= PCIE_FORCE_DIS_L0S;
>   	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
>   
> +	/*
> +	 * Prevent PCIe AXI0 from replying a slave error, as it will cause kernel panic
> +	 * and prevent us from getting useful information.
> +	 * Keep the kernel alive and debug using the information from AER.
> +	 */

Isn't it safer if we set this bit at the beginning of mtk_pcie_startup_port()
instead?

Cheers,
Angelo

> +	val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> +	val |= PCIE_AXI0_SLV_RESP_MASK;
> +	writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> +
>   	/* Disable DVFSRC voltage request */
>   	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>   	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 10:29 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/01/25 07:00, Jianjun Wang ha scritto:
> > There are some circumstances where the EP device will not respond
> > to
> > non-posted access from the root port (e.g., MMIO read). In such
> > cases,
> > the root port will reply with an AXI slave error, which will be
> > treated
> > as a System Error (SError), causing a kernel panic and preventing
> > us
> > from obtaining any useful information for further debugging.
> > 
> > We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to
> > prevent
> > PCIe AXI0 from replying with a slave error. Setting this bit on an
> > older
> > platform that does not support this feature will have no effect.
> > 
> > By preventing AXI0 from replying with a slave error, we can keep
> > the
> > kernel alive and debug using the information from AER.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >   drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 4bd3b39eebe2..48f83c2d91f7 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -87,6 +87,9 @@
> >   #define PCIE_LOW_POWER_CTRL_REG             0x194
> >   #define PCIE_FORCE_DIS_L0S          BIT(8)
> > 
> > +#define PCIE_AXI_IF_CTRL_REG         0x1a8
> > +#define PCIE_AXI0_SLV_RESP_MASK              BIT(12)
> > +
> >   #define PCIE_PIPE4_PIE8_REG         0x338
> >   #define PCIE_K_FINETUNE_MAX         GENMASK(5, 0)
> >   #define PCIE_K_FINETUNE_ERR         GENMASK(7, 6)
> > @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >       val |= PCIE_FORCE_DIS_L0S;
> >       writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> > 
> > +     /*
> > +      * Prevent PCIe AXI0 from replying a slave error, as it will
> > cause kernel panic
> > +      * and prevent us from getting useful information.
> > +      * Keep the kernel alive and debug using the information from
> > AER.
> > +      */
> 
> Isn't it safer if we set this bit at the beginning of
> mtk_pcie_startup_port()
> instead?

Agree, I'll move it to the beginning of mtk_pcie_startup_port().

Thanks.

> 
> Cheers,
> Angelo
> 
> > +     val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +     val |= PCIE_AXI0_SLV_RESP_MASK;
> > +     writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +
> >       /* Disable DVFSRC voltage request */
> >       val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
> >       val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> 
> 
> 

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Bjorn Helgaas]

On Fri, Jan 03, 2025 at 02:00:14PM +0800, Jianjun Wang wrote:
> There are some circumstances where the EP device will not respond to
> non-posted access from the root port (e.g., MMIO read). In such cases,
> the root port will reply with an AXI slave error, which will be treated
> as a System Error (SError), causing a kernel panic and preventing us
> from obtaining any useful information for further debugging.
> 
> We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to prevent
> PCIe AXI0 from replying with a slave error. Setting this bit on an older
> platform that does not support this feature will have no effect.
> 
> By preventing AXI0 from replying with a slave error, we can keep the
> kernel alive and debug using the information from AER.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 4bd3b39eebe2..48f83c2d91f7 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -87,6 +87,9 @@
>  #define PCIE_LOW_POWER_CTRL_REG		0x194
>  #define PCIE_FORCE_DIS_L0S		BIT(8)
>  
> +#define PCIE_AXI_IF_CTRL_REG		0x1a8
> +#define PCIE_AXI0_SLV_RESP_MASK		BIT(12)
> +
>  #define PCIE_PIPE4_PIE8_REG		0x338
>  #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
>  #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
> @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val |= PCIE_FORCE_DIS_L0S;
>  	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
>  
> +	/*
> +	 * Prevent PCIe AXI0 from replying a slave error, as it will cause kernel panic
> +	 * and prevent us from getting useful information.
> +	 * Keep the kernel alive and debug using the information from AER.

Wrap to fit in 80 columns like the rest of the file

Add blank lines between paragraphs.

AER is an asynchronous mechanism, so if you disable the SError,
whoever issued the MMIO read to the PCIe device will receive some kind
of data.

I hope/assume that data is ~0 as on other platforms?  If so, please
confirm this in the comment and commit log.  Otherwise, the caller
will received corrupted data with no way to know that it's corrupted.

> +	 */
> +	val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> +	val |= PCIE_AXI0_SLV_RESP_MASK;
> +	writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> +
>  	/* Disable DVFSRC voltage request */
>  	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> -- 
> 2.46.0
> 

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Jianjun Wang (王建军)]

On Fri, 2025-01-03 at 13:19 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Jan 03, 2025 at 02:00:14PM +0800, Jianjun Wang wrote:
> > There are some circumstances where the EP device will not respond
> > to
> > non-posted access from the root port (e.g., MMIO read). In such
> > cases,
> > the root port will reply with an AXI slave error, which will be
> > treated
> > as a System Error (SError), causing a kernel panic and preventing
> > us
> > from obtaining any useful information for further debugging.
> > 
> > We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to
> > prevent
> > PCIe AXI0 from replying with a slave error. Setting this bit on an
> > older
> > platform that does not support this feature will have no effect.
> > 
> > By preventing AXI0 from replying with a slave error, we can keep
> > the
> > kernel alive and debug using the information from AER.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 4bd3b39eebe2..48f83c2d91f7 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -87,6 +87,9 @@
> >  #define PCIE_LOW_POWER_CTRL_REG              0x194
> >  #define PCIE_FORCE_DIS_L0S           BIT(8)
> > 
> > +#define PCIE_AXI_IF_CTRL_REG         0x1a8
> > +#define PCIE_AXI0_SLV_RESP_MASK              BIT(12)
> > +
> >  #define PCIE_PIPE4_PIE8_REG          0x338
> >  #define PCIE_K_FINETUNE_MAX          GENMASK(5, 0)
> >  #define PCIE_K_FINETUNE_ERR          GENMASK(7, 6)
> > @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >       val |= PCIE_FORCE_DIS_L0S;
> >       writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> > 
> > +     /*
> > +      * Prevent PCIe AXI0 from replying a slave error, as it will
> > cause kernel panic
> > +      * and prevent us from getting useful information.
> > +      * Keep the kernel alive and debug using the information from
> > AER.
> 
> Wrap to fit in 80 columns like the rest of the file
> 
> Add blank lines between paragraphs.
> 
> AER is an asynchronous mechanism, so if you disable the SError,
> whoever issued the MMIO read to the PCIe device will receive some
> kind
> of data.
> 
> I hope/assume that data is ~0 as on other platforms?  If so, please
> confirm this in the comment and commit log.  Otherwise, the caller
> will received corrupted data with no way to know that it's corrupted.

Yes, with this bit set, the caller will receive ~0 data if the EP does
not respond. I'll add this to the comment and commit log.

Thanks.

> 
> > +      */
> > +     val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +     val |= PCIE_AXI0_SLV_RESP_MASK;
> > +     writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +
> >       /* Disable DVFSRC voltage request */
> >       val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
> >       val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > --
> > 2.46.0
> > 

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Manivannan Sadhasivam]

On Fri, Jan 03, 2025 at 02:00:14PM +0800, Jianjun Wang wrote:
> There are some circumstances where the EP device will not respond to
> non-posted access from the root port (e.g., MMIO read). In such cases,
> the root port will reply with an AXI slave error, which will be treated

By 'reply with an AXI slave error', you meant that the root port responds to the
MMIO read by the CPU with AXI slave error? If so, please reword it as such to
avoid confusion.

> as a System Error (SError), causing a kernel panic and preventing us
> from obtaining any useful information for further debugging.
> 
> We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to prevent
> PCIe AXI0 from replying with a slave error. Setting this bit on an older
> platform that does not support this feature will have no effect.
> 

But the issue is still present on the older SoCs, isn't it? If so, please add
this info to the comments below.

- Mani

> By preventing AXI0 from replying with a slave error, we can keep the
> kernel alive and debug using the information from AER.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 4bd3b39eebe2..48f83c2d91f7 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -87,6 +87,9 @@
>  #define PCIE_LOW_POWER_CTRL_REG		0x194
>  #define PCIE_FORCE_DIS_L0S		BIT(8)
>  
> +#define PCIE_AXI_IF_CTRL_REG		0x1a8
> +#define PCIE_AXI0_SLV_RESP_MASK		BIT(12)
> +
>  #define PCIE_PIPE4_PIE8_REG		0x338
>  #define PCIE_K_FINETUNE_MAX		GENMASK(5, 0)
>  #define PCIE_K_FINETUNE_ERR		GENMASK(7, 6)
> @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val |= PCIE_FORCE_DIS_L0S;
>  	writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
>  
> +	/*
> +	 * Prevent PCIe AXI0 from replying a slave error, as it will cause kernel panic
> +	 * and prevent us from getting useful information.
> +	 * Keep the kernel alive and debug using the information from AER.
> +	 */
> +	val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> +	val |= PCIE_AXI0_SLV_RESP_MASK;
> +	writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> +
>  	/* Disable DVFSRC voltage request */
>  	val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> -- 
> 2.46.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/5] PCI: mediatek-gen3: Don't reply AXI slave error

[Jianjun Wang (王建军)]

On Mon, 2025-01-06 at 21:46 +0530, Manivannan Sadhasivam wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Jan 03, 2025 at 02:00:14PM +0800, Jianjun Wang wrote:
> > There are some circumstances where the EP device will not respond
> > to
> > non-posted access from the root port (e.g., MMIO read). In such
> > cases,
> > the root port will reply with an AXI slave error, which will be
> > treated
> 
> By 'reply with an AXI slave error', you meant that the root port
> responds to the
> MMIO read by the CPU with AXI slave error? If so, please reword it as
> such to
> avoid confusion.

Yes, I'll fix it in the next version, thanks.

> 
> > as a System Error (SError), causing a kernel panic and preventing
> > us
> > from obtaining any useful information for further debugging.
> > 
> > We have added a new bit in the PCIE_AXI_IF_CTRL_REG register to
> > prevent
> > PCIe AXI0 from replying with a slave error. Setting this bit on an
> > older
> > platform that does not support this feature will have no effect.
> > 
> 
> But the issue is still present on the older SoCs, isn't it? If so,
> please add
> this info to the comments below.

Yes, older SoCs don't have this feature, I'll add this info in the next
version.

Thanks.

> 
> - Mani
> 
> > By preventing AXI0 from replying with a slave error, we can keep
> > the
> > kernel alive and debug using the information from AER.
> > 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 4bd3b39eebe2..48f83c2d91f7 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -87,6 +87,9 @@
> >  #define PCIE_LOW_POWER_CTRL_REG              0x194
> >  #define PCIE_FORCE_DIS_L0S           BIT(8)
> > 
> > +#define PCIE_AXI_IF_CTRL_REG         0x1a8
> > +#define PCIE_AXI0_SLV_RESP_MASK              BIT(12)
> > +
> >  #define PCIE_PIPE4_PIE8_REG          0x338
> >  #define PCIE_K_FINETUNE_MAX          GENMASK(5, 0)
> >  #define PCIE_K_FINETUNE_ERR          GENMASK(7, 6)
> > @@ -469,6 +472,15 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >       val |= PCIE_FORCE_DIS_L0S;
> >       writel_relaxed(val, pcie->base + PCIE_LOW_POWER_CTRL_REG);
> > 
> > +     /*
> > +      * Prevent PCIe AXI0 from replying a slave error, as it will
> > cause kernel panic
> > +      * and prevent us from getting useful information.
> > +      * Keep the kernel alive and debug using the information from
> > AER.
> > +      */
> > +     val = readl_relaxed(pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +     val |= PCIE_AXI0_SLV_RESP_MASK;
> > +     writel_relaxed(val, pcie->base + PCIE_AXI_IF_CTRL_REG);
> > +
> >       /* Disable DVFSRC voltage request */
> >       val = readl_relaxed(pcie->base + PCIE_MISC_CTRL_REG);
> >       val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > --
> > 2.46.0
> > 
> 
> --
> மணிவண்ணன் சதாசிவம்
> 

[PATCH 5/5] PCI: mediatek-gen3: Keep PCIe power and clocks if suspend-to-idle

[Jianjun Wang]

If the target system sleep state is suspend-to-idle, the bridge is
supposed to stay in D0, and the framework will not help to restore its
configuration space, so keep its power and clocks during suspend.

It's recommended to enable L1ss support, so the link can be changed to
L1.2 state during suspend.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 48f83c2d91f7..11da68910502 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
 	int err;
 	u32 val;
 
+	/*
+	 * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in
+	 * D0, and the framework will not help to restore its configuration space, so keep it's
+	 * power and clocks during suspend.
+	 *
+	 * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during
+	 * suspend.
+	 */
+	if (pm_suspend_default_s2idle()) {
+		dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n");
+		return 0;
+	}
+
 	/* Trigger link to L2 state */
 	err = mtk_pcie_turn_off_link(pcie);
 	if (err) {
@@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev)
 	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
 	int err;
 
+	if (pm_suspend_default_s2idle()) {
+		dev_info(dev, "System enter s2idle state, no need to reinitialization\n");
+		return 0;
+	}
+
 	err = pcie->soc->power_up(pcie);
 	if (err)
 		return err;
-- 
2.46.0

Re: [PATCH 5/5] PCI: mediatek-gen3: Keep PCIe power and clocks if suspend-to-idle

[AngeloGioacchino Del Regno]

Il 03/01/25 07:00, Jianjun Wang ha scritto:
> If the target system sleep state is suspend-to-idle, the bridge is
> supposed to stay in D0, and the framework will not help to restore its
> configuration space, so keep its power and clocks during suspend.
> 
> It's recommended to enable L1ss support, so the link can be changed to
> L1.2 state during suspend.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>   drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 48f83c2d91f7..11da68910502 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
>   	int err;
>   	u32 val;
>   
> +	/*
> +	 * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in
> +	 * D0, and the framework will not help to restore its configuration space, so keep it's
> +	 * power and clocks during suspend.
> +	 *
> +	 * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during
> +	 * suspend.
> +	 */
> +	if (pm_suspend_default_s2idle()) {

AFAIK, this works only if s2idle is the default system sleep state.

...and besides, for good measure (especially in the event that in the future we
get hotplug support) you should also check if there is any active PCI-Express
device on the controller instance before deciding to leave the controller and
link UP, as PCI-Express controllers may be enabled even if there's no actually
connected device on a PCIe slot (full PCIe, or mPCIe, or M.2).

So, I suggest to:

- Add a `bool suspended` member to `struct mtk_gen3_pcie`, then

static int mtk_pcie_suspend_noirq(struct device *dev)
{
	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
	int err;
	u32 val;

	val = readl(pcie->base + PCIE_LINK_STATUS_REG);
	val &= PCIE_PORT_LINKUP;

	if (val && pm_suspend_target_state == PM_SUSPEND_TO_IDLE) {
		dev_dbg(....)
		return 0;
	}

	/* Trigger link to L2 state */
	err = mtk_pcie_turn_off_link(pcie);
	if (err) {
		dev_err(pcie->dev, "cannot enter L2 state\n");
		return err;
	}

	pcie->suspended = true;

	/* Pull down the PERST# pin */
	.... etc etc


and

static int mtk_pcie_resume_noirq(struct device *dev)
{
	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
	int err;

	if (!pcie->suspended)
		return 0;

	err = pcie->soc->power_up .... etc etc


... so you only check if we're going in s2idle in the suspend handler, as
that dictates the value of pcie->suspended :-)

Cheers,
Angelo

> +		dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n");
> +		return 0;
> +	}
> +
>   	/* Trigger link to L2 state */
>   	err = mtk_pcie_turn_off_link(pcie);
>   	if (err) {
> @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev)
>   	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
>   	int err;
>   
> +	if (pm_suspend_default_s2idle()) {
> +		dev_info(dev, "System enter s2idle state, no need to reinitialization\n");
> +		return 0;
> +	}
> +
>   	err = pcie->soc->power_up(pcie);
>   	if (err)
>   		return err;

Re: [PATCH 5/5] PCI: mediatek-gen3: Keep PCIe power and clocks if suspend-to-idle

[Bjorn Helgaas]

On Fri, Jan 03, 2025 at 02:00:15PM +0800, Jianjun Wang wrote:
> If the target system sleep state is suspend-to-idle, the bridge is
> supposed to stay in D0, and the framework will not help to restore its
> configuration space, so keep its power and clocks during suspend.
> 
> It's recommended to enable L1ss support, so the link can be changed to
> L1.2 state during suspend.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 48f83c2d91f7..11da68910502 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
>  	int err;
>  	u32 val;
>  
> +	/*
> +	 * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in
> +	 * D0, and the framework will not help to restore its configuration space, so keep it's
> +	 * power and clocks during suspend.

s/it's/its/

Where does the requirement for the bridge to stay in D0 come from?  Is
that part of the Linux power management framework?  Or is this
something specific to this SoC?

I guess "framework" refers to Linux power management, so maybe that
assumes nothing needs to be restored when resuming from
suspend-to-idle?

> +	 * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during
> +	 * suspend.

Wrap to fit in 80 columns like the rest of the file.

s/L1ss/L1SS/

Who recommends enabling L1SS support?  I suppose enabling L1SS means
setting CONFIG_PCIEASPM=y?

What causes the transition to L1.2?  Is this done by firmware or
something else outside of Linux?

> +	if (pm_suspend_default_s2idle()) {
> +		dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n");

1) I'm a bit dubious about this since there are only two callers of
pm_suspend_default_s2idle() in the rest of the kernel.  What's unique
about this device that requires this?  Is this an indication that
we're setting mtk_pcie_pm_ops incorrectly, i.e., are we using
mtk_pcie_suspend_noirq() for a callback that is *never* supposed to
power down the device?

2) The message seems like possible overkill, maybe could be dev_dbg()?
I'm not sure the user needs this information at every suspend/resume
transition.

> +		return 0;
> +	}
> +
>  	/* Trigger link to L2 state */
>  	err = mtk_pcie_turn_off_link(pcie);
>  	if (err) {
> @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev)
>  	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
>  	int err;
>  
> +	if (pm_suspend_default_s2idle()) {
> +		dev_info(dev, "System enter s2idle state, no need to reinitialization\n");
> +		return 0;
> +	}
> +
>  	err = pcie->soc->power_up(pcie);
>  	if (err)
>  		return err;
> -- 
> 2.46.0
> 

Re: [PATCH 5/5] PCI: mediatek-gen3: Keep PCIe power and clocks if suspend-to-idle

[Manivannan Sadhasivam]

On Fri, Jan 03, 2025 at 02:00:15PM +0800, Jianjun Wang wrote:
> If the target system sleep state is suspend-to-idle, the bridge is
> supposed to stay in D0, and the framework will not help to restore its
> configuration space, so keep its power and clocks during suspend.
> 
> It's recommended to enable L1ss support, so the link can be changed to
> L1.2 state during suspend.
> 
> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 48f83c2d91f7..11da68910502 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -1291,6 +1291,19 @@ static int mtk_pcie_suspend_noirq(struct device *dev)
>  	int err;
>  	u32 val;
>  
> +	/*
> +	 * If the target system sleep state is suspend-to-idle, the bridge is supposed to stay in
> +	 * D0, and the framework will not help to restore its configuration space, so keep it's
> +	 * power and clocks during suspend.
> +	 *
> +	 * It's recommended to enable L1ss support, so the link can be changed to L1.2 state during
> +	 * suspend.
> +	 */
> +	if (pm_suspend_default_s2idle()) {

I think you need:

	if (pm_suspend_target_state == PM_SUSPEND_TO_IDLE) 

here and below.

> +		dev_info(dev, "System enter s2idle state, keep PCIe power and clocks\n");

There is absolutely no reason to print this message every time system suspend
happens. Even dev_dbg() seems unnecessary to me.

- Mani

> +		return 0;
> +	}
> +
>  	/* Trigger link to L2 state */
>  	err = mtk_pcie_turn_off_link(pcie);
>  	if (err) {
> @@ -1316,6 +1329,11 @@ static int mtk_pcie_resume_noirq(struct device *dev)
>  	struct mtk_gen3_pcie *pcie = dev_get_drvdata(dev);
>  	int err;
>  
> +	if (pm_suspend_default_s2idle()) {
> +		dev_info(dev, "System enter s2idle state, no need to reinitialization\n");
> +		return 0;
> +	}
> +
>  	err = pcie->soc->power_up(pcie);
>  	if (err)
>  		return err;
> -- 
> 2.46.0
> 

-- 
மணிவண்ணன் சதாசிவம்