diff mbox series

[v7,2/5] PCI: dwc: add support to handle ZRX-DC Compliant PHYs

Message ID 1610033323-10560-3-git-send-email-shradha.t@samsung.com (mailing list archive)
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series Add support to handle ZRX-DC Compliant PHYs | expand

Commit Message

Shradha Todi Jan. 7, 2021, 3:28 p.m. UTC
From: Pankaj Dubey <pankaj.dubey@samsung.com>

Many platforms use DesignWare controller but the PHY can be different in
different platforms. If the PHY is compliant is to ZRX-DC specification it
helps in low power consumption during power states.

If current data rate is 8.0 GT/s or higher and PHY is not compliant to
ZRX-DC specification, then after every 100ms link should transition to
recovery state during the low power states.

DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.

Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant variable to
specify this property to the controller.

Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
 2 files changed, 10 insertions(+)

Comments

Bjorn Helgaas Jan. 7, 2021, 6:44 p.m. UTC | #1
Capitalize subject to match the rest of the series.

"Add support to handle .." is redundant; "Add support for ..." would
be equivalent and shorter.

But this patch actually doesn't add anything at all by itself, since
it checks pci->phy_zrxdc_compliant but never sets it.

On Thu, Jan 07, 2021 at 08:58:40PM +0530, Shradha Todi wrote:
> From: Pankaj Dubey <pankaj.dubey@samsung.com>
> 
> Many platforms use DesignWare controller but the PHY can be different in
> different platforms. If the PHY is compliant is to ZRX-DC specification it
> helps in low power consumption during power states.

s/is to/to/

Even with that, this sentence doesn't quite parse correctly.  Do you
mean something like this?

  If the PHY is compliant to the ZRX-DC specification, it reduces
  power consumption in low power Link states.

(I assume this is related to Link power states (L0, L1, etc), not
device power states (D0, D3hot, etc)).

> If current data rate is 8.0 GT/s or higher and PHY is not compliant to
> ZRX-DC specification, then after every 100ms link should transition to
> recovery state during the low power states.

Not sure this makes sense.  If the Link is in a low power state for 10
seconds, it must transition to the Recovery state every 100ms during
that 10 seconds, i.e., 100 times?

> DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
> GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.
> 
> Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant variable to
> specify this property to the controller.

If this is a DesignWare-generic register and the "phy-zrxdc-compliant"
property can be used by any DesignWare-based driver, why isn't the
code to look for it in the DesignWare-generic part?

Is there a link to the ZRX-DC specification you can mention somewhere
in this series?

> Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
>  drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 645fa18..74590c7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -722,4 +722,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		       PCIE_PL_CHK_REG_CHK_REG_START;
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
> +
> +	if (pci->phy_zrxdc_compliant) {
> +		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
> +		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
> +		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
> +	}
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0207840..8b905a2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -74,6 +74,9 @@
>  #define PCIE_MSI_INTR0_MASK		0x82C
>  #define PCIE_MSI_INTR0_STATUS		0x830
>  
> +#define PCIE_PORT_GEN3_RELATED		0x890
> +#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
> +
>  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
>  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
>  
> @@ -273,6 +276,7 @@ struct dw_pcie {
>  	u8			n_fts[2];
>  	bool			iatu_unroll_enabled: 1;
>  	bool			io_cfg_atu_shared: 1;
> +	bool			phy_zrxdc_compliant;

I raise my eyebrows a little at "bool xx : 1".  I think it's probably
*correct*, but "unsigned int xx : 1" is the overwhelming favorite and
I doubt bool gives any advantage.

  $ git grep -E "int\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
  3129
  $ git grep -E "bool\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
  637

pcie-designware.h is the only user in drivers/pci.  But you're
following the existing style in the file, which is good.

>  };
>  
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> -- 
> 2.7.4
>
Pankaj Dubey Jan. 12, 2021, 1:52 a.m. UTC | #2
> -----Original Message-----
<snip>
> Subject: Re: [PATCH v7 2/5] PCI: dwc: add support to handle ZRX-DC
> Compliant PHYs
> 
> Capitalize subject to match the rest of the series.
> 
> "Add support to handle .." is redundant; "Add support for ..." would be
> equivalent and shorter.

OK 

> 
> But this patch actually doesn't add anything at all by itself, since it
checks pci-
> >phy_zrxdc_compliant but never sets it.

OK, we will reword the commit message as  "configure controller to handle
ZRX-DC compliant PHYs"

> 
> On Thu, Jan 07, 2021 at 08:58:40PM +0530, Shradha Todi wrote:
> > From: Pankaj Dubey <pankaj.dubey@samsung.com>
> >
> > Many platforms use DesignWare controller but the PHY can be different
> > in different platforms. If the PHY is compliant is to ZRX-DC
> > specification it helps in low power consumption during power states.
> 
> s/is to/to/

OK

> 
> Even with that, this sentence doesn't quite parse correctly.  Do you mean
> something like this?
> 
>   If the PHY is compliant to the ZRX-DC specification, it reduces
>   power consumption in low power Link states.
> 
> (I assume this is related to Link power states (L0, L1, etc), not device
power
> states (D0, D3hot, etc)).
> 

Yes, we are talking about Link power states. We rephrase the commit
description to make it more clear.

> > If current data rate is 8.0 GT/s or higher and PHY is not compliant to
> > ZRX-DC specification, then after every 100ms link should transition to
> > recovery state during the low power states.
> 
> Not sure this makes sense.  If the Link is in a low power state for 10
seconds,
> it must transition to the Recovery state every 100ms during that 10
seconds,
> i.e., 100 times?
> 

According to SNPS DesignWare data-book, the link will transition into
recovery state every 100ms, which means that yes, 100 times in 10 seconds.
But what we are trying to say here is that if the PHY is ZRX-DC compliant,
then the controller does not need to do this and we can thus save power
consumption.

As per the DesignWare data-book, the controller keeps this bit set to '1' by
default ("This bit enables a 100ms timer which can trigger exit from L1.")
and we are trying to reset this bit to '0' in order to not perform the
constant recovery and hence save power.

> > DesignWare controller provides GEN3_ZRXDC_NONCOMPL field in
> > GEN3_RELATED_OFF to specify about ZRX-DC compliant PHY.
> >
> > Platforms with ZRX-DC compliant PHY can set phy_zrxdc_compliant
> > variable to specify this property to the controller.
> 
> If this is a DesignWare-generic register and the "phy-zrxdc-compliant"
> property can be used by any DesignWare-based driver, why isn't the code to
> look for it in the DesignWare-generic part?
> 

Do you mean why this property is part of PHY node instead of DesignWare
controller?

> Is there a link to the ZRX-DC specification you can mention somewhere in
this
> series?
> 

I don't know if there is any separate ZRX-DC specification exists which can
be pointed out here, but we have implementation note in PCIe specification
Rev 4.0 which says as:
"Ports that meet the ZRX-DC specification for 2.5 GT/s while in the L1.Idle
state and are therefore not required to implement the 100 ms timeout and
transition to Recovery should avoid implementing it, since it will reduce
the power savings expected from the L1 state." 

We have captured same in cover-letter of this patch series.

> > Signed-off-by: Anvesh Salveru <anvesh.salveru@gmail.com>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > Cc: Jingoo Han <jingoohan1@gmail.com>
> > Cc: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 6 ++++++
> > drivers/pci/controller/dwc/pcie-designware.h | 4 ++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > b/drivers/pci/controller/dwc/pcie-designware.c
> > index 645fa18..74590c7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -722,4 +722,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
> >  		       PCIE_PL_CHK_REG_CHK_REG_START;
> >  		dw_pcie_writel_dbi(pci,
> PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> >  	}
> > +
> > +	if (pci->phy_zrxdc_compliant) {
> > +		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
> > +		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
> > +		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
> > +	}
> >  }
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 0207840..8b905a2 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -74,6 +74,9 @@
> >  #define PCIE_MSI_INTR0_MASK		0x82C
> >  #define PCIE_MSI_INTR0_STATUS		0x830
> >
> > +#define PCIE_PORT_GEN3_RELATED		0x890
> > +#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
> > +
> >  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
> >  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
> >
> > @@ -273,6 +276,7 @@ struct dw_pcie {
> >  	u8			n_fts[2];
> >  	bool			iatu_unroll_enabled: 1;
> >  	bool			io_cfg_atu_shared: 1;
> > +	bool			phy_zrxdc_compliant;
> 
> I raise my eyebrows a little at "bool xx : 1".  I think it's probably
*correct*, but
> "unsigned int xx : 1" is the overwhelming favorite and I doubt bool gives
any
> advantage.
> 
>   $ git grep -E "int\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
>   3129
>   $ git grep -E "bool\s+\S+\s*:\s*1" | egrep "^\S*\.[ch]" | wc -l
>   637
> 
> pcie-designware.h is the only user in drivers/pci.  But you're following
the
> existing style in the file, which is good.

No, we didn't follow existing style, we will update this in next version.

Thanks for review.

Pankaj Dubey
> 
> >  };
> >
> >  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie,
> > pp)
> > --
> > 2.7.4
> >
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 645fa18..74590c7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -722,4 +722,10 @@  void dw_pcie_setup(struct dw_pcie *pci)
 		       PCIE_PL_CHK_REG_CHK_REG_START;
 		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
 	}
+
+	if (pci->phy_zrxdc_compliant) {
+		val = dw_pcie_readl_dbi(pci, PCIE_PORT_GEN3_RELATED);
+		val &= ~PORT_LOGIC_GEN3_ZRXDC_NONCOMPL;
+		dw_pcie_writel_dbi(pci, PCIE_PORT_GEN3_RELATED, val);
+	}
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 0207840..8b905a2 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -74,6 +74,9 @@ 
 #define PCIE_MSI_INTR0_MASK		0x82C
 #define PCIE_MSI_INTR0_STATUS		0x830
 
+#define PCIE_PORT_GEN3_RELATED		0x890
+#define PORT_LOGIC_GEN3_ZRXDC_NONCOMPL	BIT(0)
+
 #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
 #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
 
@@ -273,6 +276,7 @@  struct dw_pcie {
 	u8			n_fts[2];
 	bool			iatu_unroll_enabled: 1;
 	bool			io_cfg_atu_shared: 1;
+	bool			phy_zrxdc_compliant;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)