linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] Add support to handle ZRX-DC Compliant PHYs
       [not found] <CGME20191121032031epcas5p29659e014c9ff4564b24c9b1457d6b0b7@epcas5p2.samsung.com>
@ 2019-11-21  3:20 ` Anvesh Salveru
       [not found]   ` <CGME20191121032036epcas5p1ec12cabed1104c131a3cab202a180c21@epcas5p1.samsung.com>
       [not found]   ` <CGME20191121032041epcas5p433066ebc6a07b73a1949da26e55e9b2f@epcas5p4.samsung.com>
  0 siblings, 2 replies; 7+ messages in thread
From: Anvesh Salveru @ 2019-11-21  3:20 UTC (permalink / raw)
  To: linux-kernel, linux-pci
  Cc: jingoohan1, gustavo.pimentel, pankaj.dubey, lorenzo.pieralisi,
	andrew.murray, bhelgaas, kishon, robh+dt, mark.rutland,
	Anvesh Salveru

According the PCI Express base specification when PHY does not meet
ZRX-DC specification, after every 100ms timeout the link should
transition to recovery state when the link is in low power states. 

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.

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

We need to get the PHY property in controller driver. So, we are
proposing a new method phy_property_present() in the phy driver.

Platform specific phy drivers should implement the callback for
property_present which will return true if the property exists in
the PHY.

static bool xxxx_phy_property_present(struct phy *phy, const char *property)
{
       struct device *dev = &phy->dev;

       return of_property_read_bool(dev->of_node, property);
}

And controller platform driver should populate the phy_zrxdc_compliant
flag, which will be used by generic DesignWare driver.

pci->phy_zrxdc_compliant = phy_property_present(xxxx_ctrl->phy, "phy-zrxdc-compliant");

Patchset v2 can be found at:
 - 1/2: https://lkml.org/lkml/2019/11/11/672
 - 2/2: https://lkml.org/lkml/2019/10/28/285

Changes w.r.t v2:
 - Addressed review comments
 - Rebased on latest linus/master

Changes w.r.t v3:
 - Added linux-pci@vger.kernel.org as pointed by Gustavo, Sorry for annoying.

Anvesh Salveru (2):
  phy: core: add phy_property_present method
  PCI: dwc: add support to handle ZRX-DC Compliant PHYs

 drivers/pci/controller/dwc/pcie-designware.c |  6 ++++++
 drivers/pci/controller/dwc/pcie-designware.h |  4 ++++
 drivers/phy/phy-core.c                       | 26 ++++++++++++++++++++++++++
 include/linux/phy/phy.h                      |  8 ++++++++
 4 files changed, 44 insertions(+)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/2] phy: core: add phy_property_present method
       [not found]   ` <CGME20191121032036epcas5p1ec12cabed1104c131a3cab202a180c21@epcas5p1.samsung.com>
@ 2019-11-21  3:20     ` Anvesh Salveru
  2019-11-21 16:08       ` Andrew Murray
  0 siblings, 1 reply; 7+ messages in thread
From: Anvesh Salveru @ 2019-11-21  3:20 UTC (permalink / raw)
  To: linux-kernel, linux-pci
  Cc: jingoohan1, gustavo.pimentel, pankaj.dubey, lorenzo.pieralisi,
	andrew.murray, bhelgaas, kishon, robh+dt, mark.rutland,
	Anvesh Salveru

In some platforms, we need information of phy properties in
the controller drivers. This patch adds a new phy_property_present()
method which can be used to check if some property exists in PHY
or not.

In case of DesignWare PCIe controller, we need to write into controller
register to specify about ZRX-DC compliance property of the PHY, which
reduces the power consumption during lower power states.

Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
---
 drivers/phy/phy-core.c  | 26 ++++++++++++++++++++++++++
 include/linux/phy/phy.h |  8 ++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b04f4fe..0a62eca 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -420,6 +420,32 @@ int phy_calibrate(struct phy *phy)
 EXPORT_SYMBOL_GPL(phy_calibrate);
 
 /**
+ * phy_property_present() - checks if the property is present in PHY
+ * @phy: the phy returned by phy_get()
+ * @property: name of the property to check
+ *
+ * Used to check if the given property is present in PHY. PHY drivers
+ * can implement this callback function to expose PHY properties to
+ * controller drivers.
+ *
+ * Returns: true if property exists, false otherwise
+ */
+bool phy_property_present(struct phy *phy, const char *property)
+{
+	bool ret;
+
+	if (!phy || !phy->ops->property_present)
+		return false;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->property_present(phy, property);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_property_present);
+
+/**
  * phy_configure() - Changes the phy parameters
  * @phy: the phy returned by phy_get()
  * @opts: New configuration to apply
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 15032f14..3dd8f3c 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -61,6 +61,7 @@ union phy_configure_opts {
  * @reset: resetting the phy
  * @calibrate: calibrate the phy
  * @release: ops to be performed while the consumer relinquishes the PHY
+ * @property_present: check if some property is present in PHY
  * @owner: the module owner containing the ops
  */
 struct phy_ops {
@@ -103,6 +104,7 @@ struct phy_ops {
 	int	(*reset)(struct phy *phy);
 	int	(*calibrate)(struct phy *phy);
 	void	(*release)(struct phy *phy);
+	bool	(*property_present)(struct phy *phy, const char *property);
 	struct module *owner;
 };
 
@@ -217,6 +219,7 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
 }
 int phy_reset(struct phy *phy);
 int phy_calibrate(struct phy *phy);
+bool phy_property_present(struct phy *phy, const char *property);
 static inline int phy_get_bus_width(struct phy *phy)
 {
 	return phy->attrs.bus_width;
@@ -354,6 +357,11 @@ static inline int phy_calibrate(struct phy *phy)
 	return -ENOSYS;
 }
 
+static inline bool phy_property_present(struct phy *phy, const char *property)
+{
+	return false;
+}
+
 static inline int phy_configure(struct phy *phy,
 				union phy_configure_opts *opts)
 {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant PHYs
       [not found]   ` <CGME20191121032041epcas5p433066ebc6a07b73a1949da26e55e9b2f@epcas5p4.samsung.com>
@ 2019-11-21  3:20     ` Anvesh Salveru
  2019-11-21 13:55       ` Gustavo Pimentel
  0 siblings, 1 reply; 7+ messages in thread
From: Anvesh Salveru @ 2019-11-21  3:20 UTC (permalink / raw)
  To: linux-kernel, linux-pci
  Cc: jingoohan1, gustavo.pimentel, pankaj.dubey, lorenzo.pieralisi,
	andrew.murray, bhelgaas, kishon, robh+dt, mark.rutland,
	Anvesh Salveru

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.s@samsung.com>
Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.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 820488d..36a01b7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -556,4 +556,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 5a18e94..f43f986 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -60,6 +60,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_ATU_VIEWPORT		0x900
 #define PCIE_ATU_REGION_INBOUND		BIT(31)
 #define PCIE_ATU_REGION_OUTBOUND	0
@@ -249,6 +252,7 @@ struct dw_pcie {
 	void __iomem		*atu_base;
 	u32			num_viewport;
 	u8			iatu_unroll_enabled;
+	bool			phy_zrxdc_compliant;
 	struct pcie_port	pp;
 	struct dw_pcie_ep	ep;
 	const struct dw_pcie_ops *ops;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant PHYs
  2019-11-21  3:20     ` [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant PHYs Anvesh Salveru
@ 2019-11-21 13:55       ` Gustavo Pimentel
  2019-11-22  2:35         ` Pankaj Dubey
  0 siblings, 1 reply; 7+ messages in thread
From: Gustavo Pimentel @ 2019-11-21 13:55 UTC (permalink / raw)
  To: Anvesh Salveru, linux-kernel, linux-pci
  Cc: jingoohan1, pankaj.dubey, lorenzo.pieralisi, andrew.murray,
	bhelgaas, kishon, robh+dt, mark.rutland

On Thu, Nov 21, 2019 at 3:20:8, Anvesh Salveru <anvesh.s@samsung.com> 
wrote:

> 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.s@samsung.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.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 820488d..36a01b7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -556,4 +556,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 5a18e94..f43f986 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -60,6 +60,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_ATU_VIEWPORT		0x900
>  #define PCIE_ATU_REGION_INBOUND		BIT(31)
>  #define PCIE_ATU_REGION_OUTBOUND	0
> @@ -249,6 +252,7 @@ struct dw_pcie {
>  	void __iomem		*atu_base;
>  	u32			num_viewport;
>  	u8			iatu_unroll_enabled;
> +	bool			phy_zrxdc_compliant;

Typically is used u8 instead of bool, due to platform compatibility.
I'd guess that checkpatch script should have reported this. Did you use 
it?

>  	struct pcie_port	pp;
>  	struct dw_pcie_ep	ep;
>  	const struct dw_pcie_ops *ops;
> -- 
> 2.7.4



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] phy: core: add phy_property_present method
  2019-11-21  3:20     ` [PATCH v4 1/2] phy: core: add phy_property_present method Anvesh Salveru
@ 2019-11-21 16:08       ` Andrew Murray
  2019-11-22  2:42         ` Pankaj Dubey
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Murray @ 2019-11-21 16:08 UTC (permalink / raw)
  To: Anvesh Salveru
  Cc: linux-kernel, linux-pci, jingoohan1, gustavo.pimentel,
	pankaj.dubey, lorenzo.pieralisi, bhelgaas, kishon, robh+dt,
	mark.rutland

On Thu, Nov 21, 2019 at 08:50:07AM +0530, Anvesh Salveru wrote:
> In some platforms, we need information of phy properties in
> the controller drivers. This patch adds a new phy_property_present()
> method which can be used to check if some property exists in PHY
> or not.
> 
> In case of DesignWare PCIe controller, we need to write into controller
> register to specify about ZRX-DC compliance property of the PHY, which
> reduces the power consumption during lower power states.
> 
> Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> ---
>  drivers/phy/phy-core.c  | 26 ++++++++++++++++++++++++++
>  include/linux/phy/phy.h |  8 ++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index b04f4fe..0a62eca 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -420,6 +420,32 @@ int phy_calibrate(struct phy *phy)
>  EXPORT_SYMBOL_GPL(phy_calibrate);
>  
>  /**
> + * phy_property_present() - checks if the property is present in PHY
> + * @phy: the phy returned by phy_get()
> + * @property: name of the property to check
> + *
> + * Used to check if the given property is present in PHY. PHY drivers
> + * can implement this callback function to expose PHY properties to
> + * controller drivers.
> + *
> + * Returns: true if property exists, false otherwise
> + */
> +bool phy_property_present(struct phy *phy, const char *property)
> +{
> +	bool ret;
> +
> +	if (!phy || !phy->ops->property_present)
> +		return false;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->property_present(phy, property);

I don't understand why it is necessary to require every phy driver to
implement this. Why can't the phy-core driver look up the device node
of the given phy?

Thanks,

Andrew Murray

> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_property_present);
> +
> +/**
>   * phy_configure() - Changes the phy parameters
>   * @phy: the phy returned by phy_get()
>   * @opts: New configuration to apply
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 15032f14..3dd8f3c 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -61,6 +61,7 @@ union phy_configure_opts {
>   * @reset: resetting the phy
>   * @calibrate: calibrate the phy
>   * @release: ops to be performed while the consumer relinquishes the PHY
> + * @property_present: check if some property is present in PHY
>   * @owner: the module owner containing the ops
>   */
>  struct phy_ops {
> @@ -103,6 +104,7 @@ struct phy_ops {
>  	int	(*reset)(struct phy *phy);
>  	int	(*calibrate)(struct phy *phy);
>  	void	(*release)(struct phy *phy);
> +	bool	(*property_present)(struct phy *phy, const char *property);
>  	struct module *owner;
>  };
>  
> @@ -217,6 +219,7 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
>  }
>  int phy_reset(struct phy *phy);
>  int phy_calibrate(struct phy *phy);
> +bool phy_property_present(struct phy *phy, const char *property);
>  static inline int phy_get_bus_width(struct phy *phy)
>  {
>  	return phy->attrs.bus_width;
> @@ -354,6 +357,11 @@ static inline int phy_calibrate(struct phy *phy)
>  	return -ENOSYS;
>  }
>  
> +static inline bool phy_property_present(struct phy *phy, const char *property)
> +{
> +	return false;
> +}
> +
>  static inline int phy_configure(struct phy *phy,
>  				union phy_configure_opts *opts)
>  {
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant PHYs
  2019-11-21 13:55       ` Gustavo Pimentel
@ 2019-11-22  2:35         ` Pankaj Dubey
  0 siblings, 0 replies; 7+ messages in thread
From: Pankaj Dubey @ 2019-11-22  2:35 UTC (permalink / raw)
  To: 'Gustavo Pimentel', 'Anvesh Salveru',
	linux-kernel, linux-pci
  Cc: jingoohan1, lorenzo.pieralisi, andrew.murray, bhelgaas, kishon,
	robh+dt, mark.rutland



> -----Original Message-----
> From: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
> Sent: Thursday, November 21, 2019 7:25 PM
> To: Anvesh Salveru <anvesh.s@samsung.com>; linux-kernel@vger.kernel.org;
> linux-pci@vger.kernel.org
> Cc: jingoohan1@gmail.com; pankaj.dubey@samsung.com;
> lorenzo.pieralisi@arm.com; andrew.murray@arm.com; bhelgaas@google.com;
> kishon@ti.com; robh+dt@kernel.org; mark.rutland@arm.com
> Subject: RE: [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant
> PHYs
> 
> On Thu, Nov 21, 2019 at 3:20:8, Anvesh Salveru <anvesh.s@samsung.com>
> wrote:
> 
> > 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.s@samsung.com>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.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 820488d..36a01b7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -556,4 +556,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 5a18e94..f43f986 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -60,6 +60,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_ATU_VIEWPORT		0x900
> >  #define PCIE_ATU_REGION_INBOUND		BIT(31)
> >  #define PCIE_ATU_REGION_OUTBOUND	0
> > @@ -249,6 +252,7 @@ struct dw_pcie {
> >  	void __iomem		*atu_base;
> >  	u32			num_viewport;
> >  	u8			iatu_unroll_enabled;
> > +	bool			phy_zrxdc_compliant;
> 
> Typically is used u8 instead of bool, due to platform compatibility.
> I'd guess that checkpatch script should have reported this. Did you use it?

Checkpatch didn't report any error/warning. 
We used bool here as phy_zrxdc_compliant will store the value returned by of_property_read_bool API. 
I can see many places in drivers/pci/ where this API is used the value is stored in bool itself.

> 
> >  	struct pcie_port	pp;
> >  	struct dw_pcie_ep	ep;
> >  	const struct dw_pcie_ops *ops;
> > --
> > 2.7.4
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v4 1/2] phy: core: add phy_property_present method
  2019-11-21 16:08       ` Andrew Murray
@ 2019-11-22  2:42         ` Pankaj Dubey
  0 siblings, 0 replies; 7+ messages in thread
From: Pankaj Dubey @ 2019-11-22  2:42 UTC (permalink / raw)
  To: 'Andrew Murray', 'Anvesh Salveru'
  Cc: linux-kernel, linux-pci, jingoohan1, gustavo.pimentel,
	lorenzo.pieralisi, bhelgaas, kishon, robh+dt, mark.rutland



> -----Original Message-----
> From: Andrew Murray <andrew.murray@arm.com>
> Sent: Thursday, November 21, 2019 9:39 PM
> To: Anvesh Salveru <anvesh.s@samsung.com>
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org;
> jingoohan1@gmail.com; gustavo.pimentel@synopsys.com;
> pankaj.dubey@samsung.com; lorenzo.pieralisi@arm.com;
> bhelgaas@google.com; kishon@ti.com; robh+dt@kernel.org;
> mark.rutland@arm.com
> Subject: Re: [PATCH v4 1/2] phy: core: add phy_property_present method
> 
> On Thu, Nov 21, 2019 at 08:50:07AM +0530, Anvesh Salveru wrote:
> > In some platforms, we need information of phy properties in the
> > controller drivers. This patch adds a new phy_property_present()
> > method which can be used to check if some property exists in PHY or
> > not.
> >
> > In case of DesignWare PCIe controller, we need to write into
> > controller register to specify about ZRX-DC compliance property of the
> > PHY, which reduces the power consumption during lower power states.
> >
> > Signed-off-by: Anvesh Salveru <anvesh.s@samsung.com>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> > ---
> >  drivers/phy/phy-core.c  | 26 ++++++++++++++++++++++++++
> > include/linux/phy/phy.h |  8 ++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index
> > b04f4fe..0a62eca 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> > @@ -420,6 +420,32 @@ int phy_calibrate(struct phy *phy)
> > EXPORT_SYMBOL_GPL(phy_calibrate);
> >
> >  /**
> > + * phy_property_present() - checks if the property is present in PHY
> > + * @phy: the phy returned by phy_get()
> > + * @property: name of the property to check
> > + *
> > + * Used to check if the given property is present in PHY. PHY drivers
> > + * can implement this callback function to expose PHY properties to
> > + * controller drivers.
> > + *
> > + * Returns: true if property exists, false otherwise  */ bool
> > +phy_property_present(struct phy *phy, const char *property) {
> > +	bool ret;
> > +
> > +	if (!phy || !phy->ops->property_present)
> > +		return false;
> > +
> > +	mutex_lock(&phy->mutex);
> > +	ret = phy->ops->property_present(phy, property);
> 
> I don't understand why it is necessary to require every phy driver to
implement
> this. Why can't the phy-core driver look up the device node of the given
phy?
> 

No specific reason.

We just went ahead and implemented this similar to other API in phy-core.c
file where it redirects call to platform specific phy driver. As  you
pointed out in this case, it makes sense to keep it in phy-core driver
itself, as platform specific phy driver is not going to do anything which is
really dependent on the PHY. 
We will wait for further review comments on this patch, and then will take
up your suggestion in next patchset.

Thanks for review.
Pankaj Dubey
> Thanks,
> 
> Andrew Murray
> 
> > +	mutex_unlock(&phy->mutex);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_property_present);
> > +
> > +/**
> >   * phy_configure() - Changes the phy parameters
> >   * @phy: the phy returned by phy_get()
> >   * @opts: New configuration to apply
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index
> > 15032f14..3dd8f3c 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -61,6 +61,7 @@ union phy_configure_opts {
> >   * @reset: resetting the phy
> >   * @calibrate: calibrate the phy
> >   * @release: ops to be performed while the consumer relinquishes the
> > PHY
> > + * @property_present: check if some property is present in PHY
> >   * @owner: the module owner containing the ops
> >   */
> >  struct phy_ops {
> > @@ -103,6 +104,7 @@ struct phy_ops {
> >  	int	(*reset)(struct phy *phy);
> >  	int	(*calibrate)(struct phy *phy);
> >  	void	(*release)(struct phy *phy);
> > +	bool	(*property_present)(struct phy *phy, const char *property);
> >  	struct module *owner;
> >  };
> >
> > @@ -217,6 +219,7 @@ static inline enum phy_mode phy_get_mode(struct
> > phy *phy)  }  int phy_reset(struct phy *phy);  int
> > phy_calibrate(struct phy *phy);
> > +bool phy_property_present(struct phy *phy, const char *property);
> >  static inline int phy_get_bus_width(struct phy *phy)  {
> >  	return phy->attrs.bus_width;
> > @@ -354,6 +357,11 @@ static inline int phy_calibrate(struct phy *phy)
> >  	return -ENOSYS;
> >  }
> >
> > +static inline bool phy_property_present(struct phy *phy, const char
> > +*property) {
> > +	return false;
> > +}
> > +
> >  static inline int phy_configure(struct phy *phy,
> >  				union phy_configure_opts *opts)
> >  {
> > --
> > 2.7.4
> >


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-11-22  2:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20191121032031epcas5p29659e014c9ff4564b24c9b1457d6b0b7@epcas5p2.samsung.com>
2019-11-21  3:20 ` [PATCH v4 0/2] Add support to handle ZRX-DC Compliant PHYs Anvesh Salveru
     [not found]   ` <CGME20191121032036epcas5p1ec12cabed1104c131a3cab202a180c21@epcas5p1.samsung.com>
2019-11-21  3:20     ` [PATCH v4 1/2] phy: core: add phy_property_present method Anvesh Salveru
2019-11-21 16:08       ` Andrew Murray
2019-11-22  2:42         ` Pankaj Dubey
     [not found]   ` <CGME20191121032041epcas5p433066ebc6a07b73a1949da26e55e9b2f@epcas5p4.samsung.com>
2019-11-21  3:20     ` [PATCH v4 2/2] PCI: dwc: add support to handle ZRX-DC Compliant PHYs Anvesh Salveru
2019-11-21 13:55       ` Gustavo Pimentel
2019-11-22  2:35         ` Pankaj Dubey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).