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

[Pankaj Dubey]

> -----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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> > Signed-off-by: Shradha Todi <shradha.t@xxxxxxxxxxx>
> > Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
> > Cc: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > ---
> >  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
> >