diff mbox series

PCI: mediatek-gen3: Avoid PCIe resetting for Airoha EN7581 SoC

Message ID 20240920-pcie-en7581-rst-fix-v1-1-1043fb63ffc9@kernel.org (mailing list archive)
State New
Headers show
Series PCI: mediatek-gen3: Avoid PCIe resetting for Airoha EN7581 SoC | expand

Commit Message

Lorenzo Bianconi Sept. 20, 2024, 8:26 a.m. UTC 
The PCIe controller available on the EN7581 SoC does not support reset
via the following lines:
- PCIE_MAC_RSTB
- PCIE_PHY_RSTB
- PCIE_BRG_RSTB
- PCIE_PE_RSTB

Introduce the reset callback in order to avoid resetting the PCIe port
for Airoha EN7581 SoC.

Tested-by: Hui Ma <hui.ma@airoha.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 16 deletions(-)


---
base-commit: f2024903cb387971abdbc6398a430e735a9b394c
change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4

Best regards,

Comments

AngeloGioacchino Del Regno Sept. 23, 2024, 9:41 a.m. UTC | #1 
Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> The PCIe controller available on the EN7581 SoC does not support reset
> via the following lines:
> - PCIE_MAC_RSTB
> - PCIE_PHY_RSTB
> - PCIE_BRG_RSTB
> - PCIE_PE_RSTB
> 
> Introduce the reset callback in order to avoid resetting the PCIe port
> for Airoha EN7581 SoC.
> 

EN7581 doesn't support pulling up/down PERST#?!
That looks definitely odd, as that signal is part of the PCI-Express CEM spec.

Besides, there's another PERST# assertion at mtk_pcie_suspend_noirq()...

Cheers,
Angelo

> Tested-by: Hui Ma <hui.ma@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>   drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
>   1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 5c19abac74e8..9cea67e92d98 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
>   /**
>    * struct mtk_gen3_pcie_pdata - differentiate between host generations
>    * @power_up: pcie power_up callback
> + * @reset: pcie reset callback
>    * @phy_resets: phy reset lines SoC data.
>    */
>   struct mtk_gen3_pcie_pdata {
>   	int (*power_up)(struct mtk_gen3_pcie *pcie);
> +	void (*reset)(struct mtk_gen3_pcie *pcie);
>   	struct {
>   		const char *id[MAX_NUM_PHY_RESETS];
>   		int num_resets;
> @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
>   	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>   }
>   
> +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* Assert all reset signals */
> +	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +
> +	/*
> +	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> +	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> +	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> +	 * for the power and clock to become stable.
> +	 */
> +	msleep(100);
> +
> +	/* De-assert reset signals */
> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +}
> +
>   static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>   {
>   	struct resource_entry *entry;
> @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>   	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>   	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
>   
> -	/* Assert all reset signals */
> -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> -
> -	/*
> -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> -	 * for the power and clock to become stable.
> -	 */
> -	msleep(100);
> -
> -	/* De-assert reset signals */
> -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	/* Reset the PCIe port if requested by the hw */
> +	if (pcie->soc->reset)
> +		pcie->soc->reset(pcie);
>   
>   	/* Check if the link is up or not */
>   	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> @@ -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
>   
>   static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
>   	.power_up = mtk_pcie_power_up,
> +	.reset = mtk_pcie_reset,
>   	.phy_resets = {
>   		.id[0] = "phy",
>   		.num_resets = 1,
> 
> ---
> base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
> Best regards,
AngeloGioacchino Del Regno Sept. 23, 2024, 11:28 a.m. UTC | #2 
Il 23/09/24 12:06, Hui Ma (马慧) ha scritto:
> Hi Angelo，
> 
>           EN7581 doesn't support pulling up/down PERST by bit3 of PCIe MAC register 0x148(PCIE_RST_CTRL_REG).
> 
>           EN7581 toggle PERST in clk_bulk_enable function called by mtk_pcie_en7581_power_up function.
> 

Hello Hui,
please don't top post.

Anyway, are those bits unexistant on EN7581, or are those used for different
functions?

If those do not exist, and setting those bits will not produce adverse effects,
it'd be possible to avoid creating a different codepath and just add a comment
saying that the setting has no effect on Airoha EN7581.

Regards,
Angelo

> 
> 
> 
> 
> 
> 
> 
> -----邮件原件-----
> 发件人: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 发送时间: 2024年9月23日 17:42
> 收件人: Lorenzo Bianconi <lorenzo@kernel.org>; Ryder Lee <Ryder.Lee@mediatek.com>; Jianjun Wang (王建军) <Jianjun.Wang@mediatek.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Rob Herring <robh@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; Matthias Brugger <matthias.bgg@gmail.com>
> 抄送: Christian Marangi <ansuelsmth@gmail.com>; linux-pci@vger.kernel.org; linux-mediatek@lists.infradead.org; linux-arm-kernel@lists.infradead.org; upstream <upstream@airoha.com>; Hui Ma (马慧) <Hui.Ma@airoha.com>
> 主题: Re: [PATCH] PCI: mediatek-gen3: Avoid PCIe resetting for Airoha EN7581 SoC
> 
> 
> 
> Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> 
>> The PCIe controller available on the EN7581 SoC does not support reset
> 
>> via the following lines:
> 
>> - PCIE_MAC_RSTB
> 
>> - PCIE_PHY_RSTB
> 
>> - PCIE_BRG_RSTB
> 
>> - PCIE_PE_RSTB
> 
>>
> 
>> Introduce the reset callback in order to avoid resetting the PCIe port
> 
>> for Airoha EN7581 SoC.
> 
>>
> 
> 
> 
> EN7581 doesn't support pulling up/down PERST#?!
> 
> That looks definitely odd, as that signal is part of the PCI-Express CEM spec.
> 
> 
> 
> Besides, there's another PERST# assertion at mtk_pcie_suspend_noirq()...
> 
> 
> 
> Cheers,
> 
> Angelo
> 
> 
> 
>> Tested-by: Hui Ma <hui.ma@airoha.com<mailto:hui.ma@airoha.com>>
> 
>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org<mailto:lorenzo@kernel.org>>
> 
>> ---
> 
>>    drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
> 
>>    1 file changed, 28 insertions(+), 16 deletions(-)
> 
>>
> 
>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> b/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> index 5c19abac74e8..9cea67e92d98 100644
> 
>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
> 
>>    /**
> 
>>     * struct mtk_gen3_pcie_pdata - differentiate between host generations
> 
>>     * @power_up: pcie power_up callback
> 
>> + * @reset: pcie reset callback
> 
>>     * @phy_resets: phy reset lines SoC data.
> 
>>     */
> 
>>    struct mtk_gen3_pcie_pdata {
> 
>>             int (*power_up)(struct mtk_gen3_pcie *pcie);
> 
>> +    void (*reset)(struct mtk_gen3_pcie *pcie);
> 
>>             struct {
> 
>>                       const char *id[MAX_NUM_PHY_RESETS];
> 
>>                       int num_resets;
> 
>> @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
> 
>>             writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
> 
>>    }
> 
>>
> 
>> +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie) {
> 
>> +    u32 val;
> 
>> +
> 
>> +    /* Assert all reset signals */
> 
>> +    val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> 
>> +    val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> 
>> +    writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> +
> 
>> +    /*
> 
>> +    * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> 
>> +    * and 2.2.1 (Initial Power-Up (G3 to S0)).
> 
>> +    * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> 
>> +    * for the power and clock to become stable.
> 
>> +    */
> 
>> +    msleep(100);
> 
>> +
> 
>> +    /* De-assert reset signals */
> 
>> +    val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> 
>> +    writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG); }
> 
>> +
> 
>>    static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> 
>>    {
> 
>>             struct resource_entry *entry;
> 
>> @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> 
>>             val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> 
>>             writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> 
>>
> 
>> -     /* Assert all reset signals */
> 
>> -     val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> 
>> -     val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> 
>> -     writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> -
> 
>> -     /*
> 
>> -     * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> 
>> -     * and 2.2.1 (Initial Power-Up (G3 to S0)).
> 
>> -     * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> 
>> -     * for the power and clock to become stable.
> 
>> -     */
> 
>> -     msleep(100);
> 
>> -
> 
>> -     /* De-assert reset signals */
> 
>> -     val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> 
>> -     writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> +    /* Reset the PCIe port if requested by the hw */
> 
>> +    if (pcie->soc->reset)
> 
>> +             pcie->soc->reset(pcie);
> 
>>
> 
>>             /* Check if the link is up or not */
> 
>>             err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val, @@
> 
>> -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
> 
>>
> 
>>    static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
> 
>>             .power_up = mtk_pcie_power_up,
> 
>> +    .reset = mtk_pcie_reset,
> 
>>             .phy_resets = {
> 
>>                       .id[0] = "phy",
> 
>>                       .num_resets = 1,
> 
>>
> 
>> ---
> 
>> base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> 
>> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
>>
> 
>> Best regards,
> 
>
Bjorn Helgaas Sept. 23, 2024, 5:10 p.m. UTC | #3 
On Fri, Sep 20, 2024 at 10:26:28AM +0200, Lorenzo Bianconi wrote:
> The PCIe controller available on the EN7581 SoC does not support reset
> via the following lines:
> - PCIE_MAC_RSTB
> - PCIE_PHY_RSTB
> - PCIE_BRG_RSTB
> - PCIE_PE_RSTB
> 
> Introduce the reset callback in order to avoid resetting the PCIe port
> for Airoha EN7581 SoC.
> 
> Tested-by: Hui Ma <hui.ma@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 5c19abac74e8..9cea67e92d98 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
>  /**
>   * struct mtk_gen3_pcie_pdata - differentiate between host generations
>   * @power_up: pcie power_up callback
> + * @reset: pcie reset callback
>   * @phy_resets: phy reset lines SoC data.
>   */
>  struct mtk_gen3_pcie_pdata {
>  	int (*power_up)(struct mtk_gen3_pcie *pcie);
> +	void (*reset)(struct mtk_gen3_pcie *pcie);
>  	struct {
>  		const char *id[MAX_NUM_PHY_RESETS];
>  		int num_resets;
> @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
>  	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>  }
>  
> +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* Assert all reset signals */
> +	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +
> +	/*
> +	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> +	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> +	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> +	 * for the power and clock to become stable.
> +	 */
> +	msleep(100);

I see you're just moving this, but it's a good chance to use
PCIE_T_PVPERL_MS.

> +
> +	/* De-assert reset signals */
> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +}
> +
>  static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  {
>  	struct resource_entry *entry;
> @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>  	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
>  
> -	/* Assert all reset signals */
> -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> -
> -	/*
> -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> -	 * for the power and clock to become stable.
> -	 */
> -	msleep(100);
> -
> -	/* De-assert reset signals */
> -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	/* Reset the PCIe port if requested by the hw */

I don't see any real "request" from the hardware.  IIUC, this is more
like "assert reset if this hardware supports it".

> +	if (pcie->soc->reset)
> +		pcie->soc->reset(pcie);
>  
>  	/* Check if the link is up or not */
>  	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> @@ -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
>  
>  static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
>  	.power_up = mtk_pcie_power_up,
> +	.reset = mtk_pcie_reset,
>  	.phy_resets = {
>  		.id[0] = "phy",
>  		.num_resets = 1,
> 
> ---
> base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
>
Lorenzo Bianconi Sept. 23, 2024, 9:29 p.m. UTC | #4 
> On Fri, Sep 20, 2024 at 10:26:28AM +0200, Lorenzo Bianconi wrote:
> > The PCIe controller available on the EN7581 SoC does not support reset
> > via the following lines:
> > - PCIE_MAC_RSTB
> > - PCIE_PHY_RSTB
> > - PCIE_BRG_RSTB
> > - PCIE_PE_RSTB
> > 
> > Introduce the reset callback in order to avoid resetting the PCIe port
> > for Airoha EN7581 SoC.
> > 
> > Tested-by: Hui Ma <hui.ma@airoha.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 5c19abac74e8..9cea67e92d98 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
> >  /**
> >   * struct mtk_gen3_pcie_pdata - differentiate between host generations
> >   * @power_up: pcie power_up callback
> > + * @reset: pcie reset callback
> >   * @phy_resets: phy reset lines SoC data.
> >   */
> >  struct mtk_gen3_pcie_pdata {
> >  	int (*power_up)(struct mtk_gen3_pcie *pcie);
> > +	void (*reset)(struct mtk_gen3_pcie *pcie);
> >  	struct {
> >  		const char *id[MAX_NUM_PHY_RESETS];
> >  		int num_resets;
> > @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
> >  	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
> >  }
> >  
> > +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie)
> > +{
> > +	u32 val;
> > +
> > +	/* Assert all reset signals */
> > +	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> > +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +
> > +	/*
> > +	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> > +	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > +	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> > +	 * for the power and clock to become stable.
> > +	 */
> > +	msleep(100);
> 
> I see you're just moving this, but it's a good chance to use
> PCIE_T_PVPERL_MS.

ack, I will add it.

> 
> > +
> > +	/* De-assert reset signals */
> > +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> > +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +}
> > +
> >  static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> >  {
> >  	struct resource_entry *entry;
> > @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> >  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> >  	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> >  
> > -	/* Assert all reset signals */
> > -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> > -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> > -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > -
> > -	/*
> > -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> > -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> > -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> > -	 * for the power and clock to become stable.
> > -	 */
> > -	msleep(100);
> > -
> > -	/* De-assert reset signals */
> > -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> > -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> > +	/* Reset the PCIe port if requested by the hw */
> 
> I don't see any real "request" from the hardware.  IIUC, this is more
> like "assert reset if this hardware supports it".

ack, %s/requested/supported. I will fix it.

Regards,
Lorenzo

> 
> > +	if (pcie->soc->reset)
> > +		pcie->soc->reset(pcie);
> >  
> >  	/* Check if the link is up or not */
> >  	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> > @@ -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
> >  
> >  static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
> >  	.power_up = mtk_pcie_power_up,
> > +	.reset = mtk_pcie_reset,
> >  	.phy_resets = {
> >  		.id[0] = "phy",
> >  		.num_resets = 1,
> > 
> > ---
> > base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> > change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> > 
> > Best regards,
> > -- 
> > Lorenzo Bianconi <lorenzo@kernel.org>
> >
Hui Ma (马慧) Sept. 24, 2024, 6:53 a.m. UTC | #5 
> Il 23/09/24 12:06, Hui Ma (马慧) ha scritto:
> > Hi Angelo，
> > 
> >           EN7581 doesn't support pulling up/down PERST by bit3 of PCIe MAC register 0x148(PCIE_RST_CTRL_REG).
> > 
> >           EN7581 toggle PERST in clk_bulk_enable function called by mtk_pcie_en7581_power_up function.
> > 
>
> Hello Hui,
> please don't top post.

> Anyway, are those bits unexistant on EN7581, or are those used for different functions?

> If those do not exist, and setting those bits will not produce adverse effects, it'd be possible to avoid creating a different codepath and just add a comment saying that the setting has no effect on Airoha EN7581.

> Regards,
> Angelo

> > 


Hi Angelo,
	Sorry for reply late reply.

	Bit3 of the register doesn't work on EN7581 because of the hardware issue.
	We have already completed PCIe RC/EP reset action before setting PCIe MAC register 0x148.
	At this moment，the device is in release status，if we toggle PCIe MAC register 0x148，PCIe will link down occasionally.

	In order to avoid creating a different codepath, we are trying another solution.



Regards,
Hui










> > 
> > 
> > 
> > 
> > 
> -----邮件原件-----
> 发件人: AngeloGioacchino Del Regno 
> <angelogioacchino.delregno@collabora.com>
> 发送时间: 2024年9月23日 17:42
> 收件人: Lorenzo Bianconi <lorenzo@kernel.org>; Ryder Lee 
> <Ryder.Lee@mediatek.com>; Jianjun Wang (王建军) 
> <Jianjun.Wang@mediatek.com>; Lorenzo Pieralisi 
> <lpieralisi@kernel.org>; Krzysztof Wilczyński <kw@linux.com>; Rob 
> Herring <robh@kernel.org>; Bjorn Helgaas <bhelgaas@google.com>; 
> Matthias Brugger <matthias.bgg@gmail.com>
> 抄送: Christian Marangi <ansuelsmth@gmail.com>; 
> linux-pci@vger.kernel.org; linux-mediatek@lists.infradead.org; 
> linux-arm-kernel@lists.infradead.org; upstream <upstream@airoha.com>; 
> Hui Ma (马慧) <Hui.Ma@airoha.com>
> 主题: Re: [PATCH] PCI: mediatek-gen3: Avoid PCIe resetting for Airoha 
> EN7581 SoC
> 
> 
> 
> Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> 
>> The PCIe controller available on the EN7581 SoC does not support 
>> reset
> 
>> via the following lines:
> 
>> - PCIE_MAC_RSTB
> 
>> - PCIE_PHY_RSTB
> 
>> - PCIE_BRG_RSTB
> 
>> - PCIE_PE_RSTB
> 
>>
> 
>> Introduce the reset callback in order to avoid resetting the PCIe 
>> port
> 
>> for Airoha EN7581 SoC.
> 
>>
> 
> 
> 
> EN7581 doesn't support pulling up/down PERST#?!
> 
> That looks definitely odd, as that signal is part of the PCI-Express CEM spec.
> 
> 
> 
> Besides, there's another PERST# assertion at mtk_pcie_suspend_noirq()...
> 
> 
> 
> Cheers,
> 
> Angelo
> 
> 
> 
>> Tested-by: Hui Ma <hui.ma@airoha.com<mailto:hui.ma@airoha.com>>
> 
>> Signed-off-by: Lorenzo Bianconi 
>> <lorenzo@kernel.org<mailto:lorenzo@kernel.org>>
> 
>> ---
> 
>>    drivers/pci/controller/pcie-mediatek-gen3.c | 44 
>> ++++++++++++++++++-----------
> 
>>    1 file changed, 28 insertions(+), 16 deletions(-)
> 
>>
> 
>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> b/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> index 5c19abac74e8..9cea67e92d98 100644
> 
>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> 
>> @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
> 
>>    /**
> 
>>     * struct mtk_gen3_pcie_pdata - differentiate between host 
>> generations
> 
>>     * @power_up: pcie power_up callback
> 
>> + * @reset: pcie reset callback
> 
>>     * @phy_resets: phy reset lines SoC data.
> 
>>     */
> 
>>    struct mtk_gen3_pcie_pdata {
> 
>>             int (*power_up)(struct mtk_gen3_pcie *pcie);
> 
>> +    void (*reset)(struct mtk_gen3_pcie *pcie);
> 
>>             struct {
> 
>>                       const char *id[MAX_NUM_PHY_RESETS];
> 
>>                       int num_resets;
> 
>> @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct 
>> mtk_gen3_pcie *pcie)
> 
>>             writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
> 
>>    }
> 
>>
> 
>> +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie) {
> 
>> +    u32 val;
> 
>> +
> 
>> +    /* Assert all reset signals */
> 
>> +    val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> 
>> +    val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | 
>> + PCIE_PE_RSTB;
> 
>> +    writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> +
> 
>> +    /*
> 
>> +    * Described in PCIe CEM specification sections 2.2 (PERST# 
>> + Signal)
> 
>> +    * and 2.2.1 (Initial Power-Up (G3 to S0)).
> 
>> +    * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> 
>> +    * for the power and clock to become stable.
> 
>> +    */
> 
>> +    msleep(100);
> 
>> +
> 
>> +    /* De-assert reset signals */
> 
>> +    val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | 
>> + PCIE_PE_RSTB);
> 
>> +    writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG); }
> 
>> +
> 
>>    static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
> 
>>    {
> 
>>             struct resource_entry *entry;
> 
>> @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct 
>> mtk_gen3_pcie *pcie)
> 
>>             val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> 
>>             writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
> 
>>
> 
>> -     /* Assert all reset signals */
> 
>> -     val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> 
>> -     val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> 
>> -     writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> -
> 
>> -     /*
> 
>> -     * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> 
>> -     * and 2.2.1 (Initial Power-Up (G3 to S0)).
> 
>> -     * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> 
>> -     * for the power and clock to become stable.
> 
>> -     */
> 
>> -     msleep(100);
> 
>> -
> 
>> -     /* De-assert reset signals */
> 
>> -     val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> 
>> -     writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> 
>> +    /* Reset the PCIe port if requested by the hw */
> 
>> +    if (pcie->soc->reset)
> 
>> +             pcie->soc->reset(pcie);
> 
>>
> 
>>             /* Check if the link is up or not */
> 
>>             err = readl_poll_timeout(pcie->base + 
>> PCIE_LINK_STATUS_REG, val, @@
> 
>> -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
> 
>>
> 
>>    static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
> 
>>             .power_up = mtk_pcie_power_up,
> 
>> +    .reset = mtk_pcie_reset,
> 
>>             .phy_resets = {
> 
>>                       .id[0] = "phy",
> 
>>                       .num_resets = 1,
> 
>>
> 
>> ---
> 
>> base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> 
>> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
>>
> 
>> Best regards,
> 
>
Bjorn Helgaas Sept. 30, 2024, 7:37 p.m. UTC | #6 
On Mon, Sep 23, 2024 at 11:41:41AM +0200, AngeloGioacchino Del Regno wrote:
> Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> > The PCIe controller available on the EN7581 SoC does not support reset
> > via the following lines:
> > - PCIE_MAC_RSTB
> > - PCIE_PHY_RSTB
> > - PCIE_BRG_RSTB
> > - PCIE_PE_RSTB
> > 
> > Introduce the reset callback in order to avoid resetting the PCIe port
> > for Airoha EN7581 SoC.
> 
> EN7581 doesn't support pulling up/down PERST#?!  That looks
> definitely odd, as that signal is part of the PCI-Express CEM spec.
> 
> Besides, there's another PERST# assertion at
> mtk_pcie_suspend_noirq()...

I agree, it doesn't smell right that this SoC doesn't have a way to
assert PERST#.

The response at
https://lore.kernel.org/r/SG2PR03MB63415DB5791C58C7EA69FF01FF682@SG2PR03MB6341.apcprd03.prod.outlook.com
suggests that maybe there's a hardware defect that means asserting
PERST# doesn't work correctly?  But surely firmware must have a way of
asserting PERST#, at least at boot time.

If this is truly a hardware defect and we really can't assert PERST#,
please say that this is a defect in the commit log so people don't
think that lack of PERST# is an acceptable thing.

Bjorn
Lorenzo Bianconi Oct. 1, 2024, 5:15 p.m. UTC | #7 
> On Mon, Sep 23, 2024 at 11:41:41AM +0200, AngeloGioacchino Del Regno wrote:
> > Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> > > The PCIe controller available on the EN7581 SoC does not support reset
> > > via the following lines:
> > > - PCIE_MAC_RSTB
> > > - PCIE_PHY_RSTB
> > > - PCIE_BRG_RSTB
> > > - PCIE_PE_RSTB
> > > 
> > > Introduce the reset callback in order to avoid resetting the PCIe port
> > > for Airoha EN7581 SoC.
> > 
> > EN7581 doesn't support pulling up/down PERST#?!  That looks
> > definitely odd, as that signal is part of the PCI-Express CEM spec.
> > 
> > Besides, there's another PERST# assertion at
> > mtk_pcie_suspend_noirq()...
> 
> I agree, it doesn't smell right that this SoC doesn't have a way to
> assert PERST#.
> 
> The response at
> https://lore.kernel.org/r/SG2PR03MB63415DB5791C58C7EA69FF01FF682@SG2PR03MB6341.apcprd03.prod.outlook.com
> suggests that maybe there's a hardware defect that means asserting
> PERST# doesn't work correctly?  But surely firmware must have a way of
> asserting PERST#, at least at boot time.
> 
> If this is truly a hardware defect and we really can't assert PERST#,
> please say that this is a defect in the commit log so people don't
> think that lack of PERST# is an acceptable thing.

Hi Bjorn,

I do not have visibility on these hw details.
@Hui: any update on it?

Regards,
Lorenzo

> 
> Bjorn
Hui Ma (马慧) Oct. 8, 2024, 10:38 a.m. UTC | #8 
> On Mon, Sep 23, 2024 at 11:41:41AM +0200, AngeloGioacchino Del Regno wrote:
> > Il 20/09/24 10:26, Lorenzo Bianconi ha scritto:
> > > The PCIe controller available on the EN7581 SoC does not support 
> > > reset via the following lines:
> > > - PCIE_MAC_RSTB
> > > - PCIE_PHY_RSTB
> > > - PCIE_BRG_RSTB
> > > - PCIE_PE_RSTB
> > > 
> > > Introduce the reset callback in order to avoid resetting the PCIe 
> > > port for Airoha EN7581 SoC.
> > 
> > EN7581 doesn't support pulling up/down PERST#?!  That looks 
> > definitely odd, as that signal is part of the PCI-Express CEM spec.
> > 
> > Besides, there's another PERST# assertion at 
> > mtk_pcie_suspend_noirq()...
> 
> I agree, it doesn't smell right that this SoC doesn't have a way to 
> assert PERST#.
> 
Hi Bjorn & Lorenzo,
	"EN7581 doesn't support pulling up/down PERST#?!"
	The described above is not exactly accurate.
	EN7581 support pulling up/down PERST#.But EN7581 doesn't support reset PCIe device by bit PCIE_PE_RSTB of PCIE_RST_CTRL_REG register.
	EN7581 used 0x1fb00088 register of EN7581 Soc to reset PCIe device instead.

	Without the patch(Avoid PCIe resetting for Airoha EN7581 SoC),the reset sequence of the current code is as follows. 
	step1.set 0x1fb00834（7581Soc reg,used to reset PCIe MAC） [release(default)->reset->release.] [this is done in reset callback]
	step2.set 0x1fb00088（7581Soc reg,used to reset PCIe device,control PERST） [reset(default)->release] [this is done in clock_enable callback]
	step3.set PCIE_MAC_RSTB（PCIe module reg,used to reset PCIe MAC) 	[release(default)->reset->release.]
	step4.set PCIE_PE_RSTB（PCIe module reg,used to reset PCIe device,control PERST,but doesn't work in EN7581） [reset(default)->release]

	With the 4 steps, we encountered PCIe link down issue occasionally. 
	The root cause of the link down issue is that the status of PERST is release after step2.
	However there is a rule that the status of PERST must be reset before do PCIe MAC reset.So step3 doesn't meet the rule.

	So we should skip step3&4 to fix the PCIe link down issue of EN7581.

Regards,
Hui

> The response at
> https://lore.kernel.org/r/SG2PR03MB63415DB5791C58C7EA69FF01FF682@SG2PR
> 03MB6341.apcprd03.prod.outlook.com
> suggests that maybe there's a hardware defect that means asserting 
> PERST# doesn't work correctly?  But surely firmware must have a way of 
> asserting PERST#, at least at boot time.
> 
> If this is truly a hardware defect and we really can't assert PERST#, 
> please say that this is a defect in the commit log so people don't 
> think that lack of PERST# is an acceptable thing.
>
>Hi Bjorn,
>
>I do not have visibility on these hw details.
>@Hui: any update on it?
>
>Regards,
>Lorenzo
>
> 
> Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 5c19abac74e8..9cea67e92d98 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -128,10 +128,12 @@  struct mtk_gen3_pcie;
 /**
  * struct mtk_gen3_pcie_pdata - differentiate between host generations
  * @power_up: pcie power_up callback
+ * @reset: pcie reset callback
  * @phy_resets: phy reset lines SoC data.
  */
 struct mtk_gen3_pcie_pdata {
 	int (*power_up)(struct mtk_gen3_pcie *pcie);
+	void (*reset)(struct mtk_gen3_pcie *pcie);
 	struct {
 		const char *id[MAX_NUM_PHY_RESETS];
 		int num_resets;
@@ -373,6 +375,28 @@  static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
 	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
 }
 
+static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie)
+{
+	u32 val;
+
+	/* Assert all reset signals */
+	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
+	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
+	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+
+	/*
+	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
+	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
+	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
+	 * for the power and clock to become stable.
+	 */
+	msleep(100);
+
+	/* De-assert reset signals */
+	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
+	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+}
+
 static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 {
 	struct resource_entry *entry;
@@ -402,22 +426,9 @@  static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
 	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
 
-	/* Assert all reset signals */
-	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
-	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
-	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
-
-	/*
-	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
-	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
-	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
-	 * for the power and clock to become stable.
-	 */
-	msleep(100);
-
-	/* De-assert reset signals */
-	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
-	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
+	/* Reset the PCIe port if requested by the hw */
+	if (pcie->soc->reset)
+		pcie->soc->reset(pcie);
 
 	/* Check if the link is up or not */
 	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
@@ -1207,6 +1218,7 @@  static const struct dev_pm_ops mtk_pcie_pm_ops = {
 
 static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
 	.power_up = mtk_pcie_power_up,
+	.reset = mtk_pcie_reset,
 	.phy_resets = {
 		.id[0] = "phy",
 		.num_resets = 1,