| 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 |
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 >
> -----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 --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)