[PATCH v2] PCI: mediatek-gen3: Disable DVFSRC voltage request

[PATCH v2] PCI: mediatek-gen3: Disable DVFSRC voltage request

[Jianjun Wang]

When the DVFSRC feature is not implemented, the MAC layer will
assert a voltage request signal when exit from the L1ss state,
but cannot receive the voltage ready signal, which will cause
the link to fail to exit the L1ss state correctly.

Disable DVFSRC voltage request by default, we need to find
a common way to enable it in the future.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
Tested-by: Qizhong Cheng <qizhong.cheng@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index f3aeb8d4eaca..79fb12fca6a9 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -79,6 +79,9 @@
 #define PCIE_ICMD_PM_REG		0x198
 #define PCIE_TURN_OFF_LINK		BIT(4)
 
+#define PCIE_MISC_CTRL_REG		0x348
+#define PCIE_DISABLE_DVFSRC_VLT_REQ	BIT(1)
+
 #define PCIE_TRANS_TABLE_BASE_REG	0x800
 #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
 #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
@@ -297,6 +300,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
 	val &= ~PCIE_INTX_ENABLE;
 	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
 
+	/* Disable DVFSRC voltage request */
+	val = readl_relaxed(port->base + PCIE_MISC_CTRL_REG);
+	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
+	writel_relaxed(val, port->base + PCIE_MISC_CTRL_REG);
+
 	/* Assert all reset signals */
 	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
 	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
-- 
2.25.1

Re: [PATCH v2] PCI: mediatek-gen3: Disable DVFSRC voltage request

[Bjorn Helgaas]

On Wed, Oct 13, 2021 at 03:53:28PM +0800, Jianjun Wang wrote:
> When the DVFSRC feature is not implemented, the MAC layer will
> assert a voltage request signal when exit from the L1ss state,
> but cannot receive the voltage ready signal, which will cause
> the link to fail to exit the L1ss state correctly.
> 
> Disable DVFSRC voltage request by default, we need to find
> a common way to enable it in the future.

Rewrap commit log to fill 75 columns.

Does "L1ss" above refer to L1.1 and L1.2?  If so, please say that
explicitly or say something like "L1 PM Substates" (the term used in
the PCIe spec) so it's clear.

This seems on the boundary of PCIe-specified things and Mediatek
implementation details, so I'm not sure what "DVFSRC," "MAC," and
"voltage request signal" mean.  Since I don't recognize those terms,
I'm guessing they are Mediatek-specific things.

But if they are things specified by the PCIe spec, please use the
exact names used in the spec.

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> Tested-by: Qizhong Cheng <qizhong.cheng@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index f3aeb8d4eaca..79fb12fca6a9 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -79,6 +79,9 @@
>  #define PCIE_ICMD_PM_REG		0x198
>  #define PCIE_TURN_OFF_LINK		BIT(4)
>  
> +#define PCIE_MISC_CTRL_REG		0x348
> +#define PCIE_DISABLE_DVFSRC_VLT_REQ	BIT(1)
> +
>  #define PCIE_TRANS_TABLE_BASE_REG	0x800
>  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
>  #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> @@ -297,6 +300,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
>  	val &= ~PCIE_INTX_ENABLE;
>  	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
>  
> +	/* Disable DVFSRC voltage request */
> +	val = readl_relaxed(port->base + PCIE_MISC_CTRL_REG);
> +	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> +	writel_relaxed(val, port->base + PCIE_MISC_CTRL_REG);
> +
>  	/* Assert all reset signals */
>  	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
>  	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> -- 
> 2.25.1
> 

Re: [PATCH v2] PCI: mediatek-gen3: Disable DVFSRC voltage request

[Bjorn Helgaas]

On Wed, Oct 13, 2021 at 01:35:17PM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 03:53:28PM +0800, Jianjun Wang wrote:
> > When the DVFSRC feature is not implemented, the MAC layer will
> > assert a voltage request signal when exit from the L1ss state,
> > but cannot receive the voltage ready signal, which will cause
> > the link to fail to exit the L1ss state correctly.
> > 
> > Disable DVFSRC voltage request by default, we need to find
> > a common way to enable it in the future.
> 
> Rewrap commit log to fill 75 columns.
> 
> Does "L1ss" above refer to L1.1 and L1.2?  If so, please say that
> explicitly or say something like "L1 PM Substates" (the term used in
> the PCIe spec) so it's clear.
> 
> This seems on the boundary of PCIe-specified things and Mediatek
> implementation details, so I'm not sure what "DVFSRC," "MAC," and
> "voltage request signal" mean.  Since I don't recognize those terms,
> I'm guessing they are Mediatek-specific things.
> 
> But if they are things specified by the PCIe spec, please use the
> exact names used in the spec.
> 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > Tested-by: Qizhong Cheng <qizhong.cheng@mediatek.com>

Krzysztof also pointed out that if this is a bug fix, we may want a
stable tag here.  And, ideally, a Fixes: tag with the specific commit
that introduced the bug.

> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index f3aeb8d4eaca..79fb12fca6a9 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -79,6 +79,9 @@
> >  #define PCIE_ICMD_PM_REG		0x198
> >  #define PCIE_TURN_OFF_LINK		BIT(4)
> >  
> > +#define PCIE_MISC_CTRL_REG		0x348
> > +#define PCIE_DISABLE_DVFSRC_VLT_REQ	BIT(1)
> > +
> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> >  #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> > @@ -297,6 +300,11 @@ static int mtk_pcie_startup_port(struct mtk_pcie_port *port)
> >  	val &= ~PCIE_INTX_ENABLE;
> >  	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> >  
> > +	/* Disable DVFSRC voltage request */
> > +	val = readl_relaxed(port->base + PCIE_MISC_CTRL_REG);
> > +	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > +	writel_relaxed(val, port->base + PCIE_MISC_CTRL_REG);
> > +
> >  	/* Assert all reset signals */
> >  	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> >  	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> > -- 
> > 2.25.1
> > 

Re: [PATCH v2] PCI: mediatek-gen3: Disable DVFSRC voltage request

[Jianjun Wang]

On Wed, 2021-10-13 at 13:35 -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 03:53:28PM +0800, Jianjun Wang wrote:
> > When the DVFSRC feature is not implemented, the MAC layer will
> > assert a voltage request signal when exit from the L1ss state,
> > but cannot receive the voltage ready signal, which will cause
> > the link to fail to exit the L1ss state correctly.
> > 
> > Disable DVFSRC voltage request by default, we need to find
> > a common way to enable it in the future.
> 
> Rewrap commit log to fill 75 columns.
> 
> Does "L1ss" above refer to L1.1 and L1.2?  If so, please say that
> explicitly or say something like "L1 PM Substates" (the term used in
> the PCIe spec) so it's clear.
> 
> This seems on the boundary of PCIe-specified things and Mediatek
> implementation details, so I'm not sure what "DVFSRC," "MAC," and
> "voltage request signal" mean.  Since I don't recognize those terms,
> I'm guessing they are Mediatek-specific things.
> 
> But if they are things specified by the PCIe spec, please use the
> exact names used in the spec.

Hi Bjorn,

Yes, the DVFSRC (dynamic voltage and frequency scaling resource
collector) is a proprietary hardware of Mediatek, which is used to
collect the requests from system and turn into the decision of minimum
Vcore voltage and minimum DRAM frequency to fulfill those requests, and
the "voltage request signal" is the hardware signal which from the PCIe
hardware to the DVFSRC module to request a specific Vcore voltage.

I will add its full name in the next version, thanks for your review.

Thanks.
> 
> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
> > Tested-by: Qizhong Cheng <qizhong.cheng@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index f3aeb8d4eaca..79fb12fca6a9 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -79,6 +79,9 @@
> >  #define PCIE_ICMD_PM_REG		0x198
> >  #define PCIE_TURN_OFF_LINK		BIT(4)
> >  
> > +#define PCIE_MISC_CTRL_REG		0x348
> > +#define PCIE_DISABLE_DVFSRC_VLT_REQ	BIT(1)
> > +
> >  #define PCIE_TRANS_TABLE_BASE_REG	0x800
> >  #define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4
> >  #define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8
> > @@ -297,6 +300,11 @@ static int mtk_pcie_startup_port(struct
> > mtk_pcie_port *port)
> >  	val &= ~PCIE_INTX_ENABLE;
> >  	writel_relaxed(val, port->base + PCIE_INT_ENABLE_REG);
> >  
> > +	/* Disable DVFSRC voltage request */
> > +	val = readl_relaxed(port->base + PCIE_MISC_CTRL_REG);
> > +	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
> > +	writel_relaxed(val, port->base + PCIE_MISC_CTRL_REG);
> > +
> >  	/* Assert all reset signals */
> >  	val = readl_relaxed(port->base + PCIE_RST_CTRL_REG);
> >  	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB |
> > PCIE_PE_RSTB;
> > -- 
> > 2.25.1
> >