[PATCH v2 0/3] PCI: dra7xx: Support PCIe x2 lane mode

[PATCH v2 0/3] PCI: dra7xx: Support PCIe x2 lane mode

[Kishon Vijay Abraham I]

Previous version of this series can be found here @ [1]

Patch series adds support to enable x2 lane mode in dra74/dra76 and
dra72 based boards in pci-dra7xx driver. It introduces new compatible
strings in order to enable x2 lane mode support.

Changes from v1:
*) Added ti prefix to syscon-lane-conf and syscon-lane-sel as
   suggested to Rob
*) Merged "PCI: dwc: dra7xx: Add support for SoC specific compatible
   strings" and "PCI: dwc: pci-dra7xx: Enable x2 mode support" into
   a single patch.
*) Fixed $subject as suggested by Bjorn
*) Added x2 lane mode support for DRA72x

The dts changes and phy changes will be sent as a separate series.

[1] -> https://lkml.org/lkml/2017/10/10/276

Kishon Vijay Abraham I (3):
  dt-bindings: PCI: dra7xx: Add SoC specific compatible strings
  dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7
  PCI: dra7xx: Enable x2 mode support for dra74x, dra76x and dra72x

 Documentation/devicetree/bindings/pci/ti-pci.txt | 14 +++-
 drivers/pci/dwc/pci-dra7xx.c                     | 90 ++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] dt-bindings: PCI: dra7xx: Add SoC specific compatible strings

[Kishon Vijay Abraham I]

Add new compatible strings for dra74x SoC (also used by dra76x) and
dra72x. This can be used to perform SoC specific configuration
(like configuring PCIe in x2 lane mode).

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/ti-pci.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 7f7af3044016..82cb875e4cec 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -1,8 +1,12 @@
 TI PCI Controllers
 
 PCIe DesignWare Controller
- - compatible: Should be "ti,dra7-pcie" for RC
-	       Should be "ti,dra7-pcie-ep" for EP
+ - compatible: Should be "ti,dra7-pcie" for RC (deprecated)
+	       Should be "ti,dra7-pcie-ep" for EP (deprecated)
+	       Should be "ti,dra746-pcie-rc" for dra74x/dra76 in RC mode
+	       Should be "ti,dra746-pcie-ep" for dra74x/dra76 in EP mode
+	       Should be "ti,dra726-pcie-rc" for dra72x in RC mode
+	       Should be "ti,dra726-pcie-ep" for dra72x in EP mode
  - phys : list of PHY specifiers (used by generic PHY framework)
  - phy-names : must be "pcie-phy0", "pcie-phy1", "pcie-phyN".. based on the
 	       number of PHYs as specified in *phys* property.
-- 
2.11.0

[PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Kishon Vijay Abraham I]

Add syscon properties required for configuring PCIe in x2 lane mode.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
index 82cb875e4cec..bfbc77ac7355 100644
--- a/Documentation/devicetree/bindings/pci/ti-pci.txt
+++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
@@ -13,6 +13,12 @@ PCIe DesignWare Controller
  - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
 	       where <X> is the instance number of the pcie from the HW spec.
  - num-lanes as specified in ../designware-pcie.txt
+ - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
+			 module and the register offset to specify 1 lane or
+			 2 lane.
+ - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
+			module and the register offset to specify lane
+			selection.
 
 HOST MODE
 =========
-- 
2.11.0

[PATCH v2 3/3] PCI: dra7xx: Enable x2 mode support for dra74x, dra76x and dra72x

[Kishon Vijay Abraham I]

dra74x/dra76x and dra72x has separate compatible strings. Add support
for these compatible strings in pci-dra7xx driver to perform syscon
configurations required to get x2 mode working.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/dwc/pci-dra7xx.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index e77a4ceed74c..3b4427c10228 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -83,11 +83,15 @@
 #define MSI_REQ_GRANT					BIT(0)
 #define MSI_VECTOR_SHIFT				7
 
+#define PCIE_1LANE_2LANE_SELECTION			BIT(13)
+#define PCIE_B1C0_MODE_SEL				BIT(2)
+
 struct dra7xx_pcie {
 	struct dw_pcie		*pci;
 	void __iomem		*base;		/* DT ti_conf */
 	int			phy_count;	/* DT phy-names count */
 	struct phy		**phy;
+	u32			*b1c0_mask;
 	int			link_gen;
 	struct irq_domain	*irq_domain;
 	enum dw_pcie_device_mode mode;
@@ -95,6 +99,7 @@ struct dra7xx_pcie {
 
 struct dra7xx_pcie_of_data {
 	enum dw_pcie_device_mode mode;
+	u32 b1co_mode_sel_mask;
 };
 
 #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
@@ -533,6 +538,26 @@ static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = {
 	.mode = DW_PCIE_EP_TYPE,
 };
 
+static const struct dra7xx_pcie_of_data dra746_pcie_rc_of_data = {
+	.b1co_mode_sel_mask = BIT(2),
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct dra7xx_pcie_of_data dra726_pcie_rc_of_data = {
+	.b1co_mode_sel_mask = GENMASK(3, 2),
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct dra7xx_pcie_of_data dra746_pcie_ep_of_data = {
+	.b1co_mode_sel_mask = BIT(2),
+	.mode = DW_PCIE_EP_TYPE,
+};
+
+static const struct dra7xx_pcie_of_data dra726_pcie_ep_of_data = {
+	.b1co_mode_sel_mask = GENMASK(3, 2),
+	.mode = DW_PCIE_EP_TYPE,
+};
+
 static const struct of_device_id of_dra7xx_pcie_match[] = {
 	{
 		.compatible = "ti,dra7-pcie",
@@ -542,6 +567,22 @@ static const struct of_device_id of_dra7xx_pcie_match[] = {
 		.compatible = "ti,dra7-pcie-ep",
 		.data = &dra7xx_pcie_ep_of_data,
 	},
+	{
+		.compatible = "ti,dra746-pcie-rc",
+		.data = &dra746_pcie_rc_of_data,
+	},
+	{
+		.compatible = "ti,dra726-pcie-rc",
+		.data = &dra726_pcie_rc_of_data,
+	},
+	{
+		.compatible = "ti,dra746-pcie-ep",
+		.data = &dra746_pcie_ep_of_data,
+	},
+	{
+		.compatible = "ti,dra726-pcie-ep",
+		.data = &dra726_pcie_ep_of_data,
+	},
 	{},
 };
 
@@ -587,6 +628,47 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev)
 	return ret;
 }
 
+static int dra7xx_pcie_configure_two_lane(struct device *dev,
+					  u32 b1co_mode_sel_mask)
+{
+	struct device_node *np = dev->of_node;
+	struct regmap *pcie_syscon;
+	unsigned int pcie_reg;
+
+	pcie_syscon = syscon_regmap_lookup_by_phandle(np,
+						      "ti,syscon-lane-conf");
+	if (IS_ERR(pcie_syscon)) {
+		dev_err(dev, "unable to get ti,syscon-lane-conf\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32_index(np, "ti,syscon-lane-conf", 1,
+				       &pcie_reg)) {
+		dev_err(dev, "couldn't get lane configuration reg offset\n");
+		return -EINVAL;
+	}
+
+	regmap_update_bits(pcie_syscon, pcie_reg, PCIE_1LANE_2LANE_SELECTION,
+			   PCIE_1LANE_2LANE_SELECTION);
+
+	pcie_syscon = syscon_regmap_lookup_by_phandle(np, "ti,syscon-lane-sel");
+	if (IS_ERR(pcie_syscon)) {
+		dev_err(dev, "unable to get ti,syscon-lane-sel\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32_index(np, "ti,syscon-lane-sel", 1,
+				       &pcie_reg)) {
+		dev_err(dev, "couldn't get lane selection reg offset\n");
+		return -EINVAL;
+	}
+
+	regmap_update_bits(pcie_syscon, pcie_reg, b1co_mode_sel_mask,
+			   PCIE_B1C0_MODE_SEL);
+
+	return 0;
+}
+
 static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 {
 	u32 reg;
@@ -608,6 +690,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	const struct dra7xx_pcie_of_data *data;
 	enum dw_pcie_device_mode mode;
+	u32 b1co_mode_sel_mask;
 
 	match = of_match_device(of_match_ptr(of_dra7xx_pcie_match), dev);
 	if (!match)
@@ -615,6 +698,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 
 	data = (struct dra7xx_pcie_of_data *)match->data;
 	mode = (enum dw_pcie_device_mode)data->mode;
+	b1co_mode_sel_mask = data->b1co_mode_sel_mask;
 
 	dra7xx = devm_kzalloc(dev, sizeof(*dra7xx), GFP_KERNEL);
 	if (!dra7xx)
@@ -673,6 +757,12 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
 	dra7xx->pci = pci;
 	dra7xx->phy_count = phy_count;
 
+	if (phy_count == 2) {
+		ret = dra7xx_pcie_configure_two_lane(dev, b1co_mode_sel_mask);
+		if (ret < 0)
+			goto err_link;
+	}
+
 	ret = dra7xx_pcie_enable_phy(dra7xx);
 	if (ret) {
 		dev_err(dev, "failed to enable phy\n");
-- 
2.11.0

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Rob Herring]

On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> Add syscon properties required for configuring PCIe in x2 lane mode.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> index 82cb875e4cec..bfbc77ac7355 100644
> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>  	       where <X> is the instance number of the pcie from the HW spec.
>   - num-lanes as specified in ../designware-pcie.txt
> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> +			 module and the register offset to specify 1 lane or
> +			 2 lane.
> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> +			module and the register offset to specify lane
> +			selection.

Adding a property for every syscon register doesn't really scale and 
doesn't work if the register layout changes.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Kishon Vijay Abraham I]

Hi Rob,

On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>> Add syscon properties required for configuring PCIe in x2 lane mode.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> index 82cb875e4cec..bfbc77ac7355 100644
>> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>>  	       where <X> is the instance number of the pcie from the HW spec.
>>   - num-lanes as specified in ../designware-pcie.txt
>> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>> +			 module and the register offset to specify 1 lane or
>> +			 2 lane.
>> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>> +			module and the register offset to specify lane
>> +			selection.
> 
> Adding a property for every syscon register doesn't really scale and 
> doesn't work if the register layout changes.

The register layout doesn't really change between silicon revisions and for new
SoCs, the phandle and the register offset for that SoC will have to be
populated again.

Having said that, I'm not aware of any other alternative here.

Thanks
Kishon

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Tony Lindgren]

* Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
> Hi Rob,
> 
> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> >> Add syscon properties required for configuring PCIe in x2 lane mode.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> >> ---
> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> index 82cb875e4cec..bfbc77ac7355 100644
> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
> >>  	       where <X> is the instance number of the pcie from the HW spec.
> >>   - num-lanes as specified in ../designware-pcie.txt
> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> >> +			 module and the register offset to specify 1 lane or
> >> +			 2 lane.
> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> >> +			module and the register offset to specify lane
> >> +			selection.
> > 
> > Adding a property for every syscon register doesn't really scale and 
> > doesn't work if the register layout changes.
> 
> The register layout doesn't really change between silicon revisions and for new
> SoCs, the phandle and the register offset for that SoC will have to be
> populated again.
> 
> Having said that, I'm not aware of any other alternative here.

Sorry I did not realize this is still open. Sounds like I need to
revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
enable PCIe x2 lane mode"), let me know if that is not the case.

Regards,

Tony

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Rob Herring]

On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Kishon Vijay Abraham I <kishon@ti.com> [171222 06:06]:
>> Hi Rob,
>>
>> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
>> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>> >> Add syscon properties required for configuring PCIe in x2 lane mode.
>> >>
>> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> index 82cb875e4cec..bfbc77ac7355 100644
>> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>> >>           where <X> is the instance number of the pcie from the HW spec.
>> >>   - num-lanes as specified in ../designware-pcie.txt
>> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>> >> +                   module and the register offset to specify 1 lane or
>> >> +                   2 lane.
>> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>> >> +                  module and the register offset to specify lane
>> >> +                  selection.
>> >
>> > Adding a property for every syscon register doesn't really scale and
>> > doesn't work if the register layout changes.
>>
>> The register layout doesn't really change between silicon revisions and for new
>> SoCs, the phandle and the register offset for that SoC will have to be
>> populated again.

And what about SoCs that don't exist yet?

>> Having said that, I'm not aware of any other alternative here.

What would you do if you had 20 different syscon registers to
configure? Add 20 properties? No, you would have per SoC functions in
the driver to handle the different cases.

> Sorry I did not realize this is still open. Sounds like I need to
> revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
> enable PCIe x2 lane mode"), let me know if that is not the case.

It's fine, I guess. Keep adding more syscon phandles and then I'll NAK
it (if I remember :)).

Rob

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Tony Lindgren]

* Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171226 17:49]:
> On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > * Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org> [171222 06:06]:
> >> Hi Rob,
> >>
> >> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
> >> > On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
> >> >> Add syscon properties required for configuring PCIe in x2 lane mode.
> >> >>
> >> >> Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
> >> >> Signed-off-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> >> >> ---
> >> >>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> index 82cb875e4cec..bfbc77ac7355 100644
> >> >> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
> >> >> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
> >> >>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
> >> >>           where <X> is the instance number of the pcie from the HW spec.
> >> >>   - num-lanes as specified in ../designware-pcie.txt
> >> >> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
> >> >> +                   module and the register offset to specify 1 lane or
> >> >> +                   2 lane.
> >> >> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
> >> >> +                  module and the register offset to specify lane
> >> >> +                  selection.
> >> >
> >> > Adding a property for every syscon register doesn't really scale and
> >> > doesn't work if the register layout changes.
> >>
> >> The register layout doesn't really change between silicon revisions and for new
> >> SoCs, the phandle and the register offset for that SoC will have to be
> >> populated again.
> 
> And what about SoCs that don't exist yet?
> 
> >> Having said that, I'm not aware of any other alternative here.
> 
> What would you do if you had 20 different syscon registers to
> configure? Add 20 properties? No, you would have per SoC functions in
> the driver to handle the different cases.

Ideally these syscon registers would be managed by some Linux
generic framework such as clock/regulator/mux/phy.

But yeah, if that does not work, then setting a SoC specific
configuration function based on the compatible value makes sense
to me.

> > Sorry I did not realize this is still open. Sounds like I need to
> > revert commit 4ece93c020e3 ("ARM: dts: dra7: Add properties to
> > enable PCIe x2 lane mode"), let me know if that is not the case.
> 
> It's fine, I guess. Keep adding more syscon phandles and then I'll NAK
> it (if I remember :)).

Already reverted, thanks for the comments.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] dt-bindings: PCI: dra7xx: Add properties to enable x2 lane in dra7

[Kishon Vijay Abraham I]

Hi,

On Tuesday 26 December 2017 11:40 PM, Tony Lindgren wrote:
> * Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [171226 17:49]:
>> On Fri, Dec 22, 2017 at 12:24 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>>> * Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org> [171222 06:06]:
>>>> Hi Rob,
>>>>
>>>> On Thursday 21 December 2017 12:27 AM, Rob Herring wrote:
>>>>> On Tue, Dec 19, 2017 at 02:28:22PM +0530, Kishon Vijay Abraham I wrote:
>>>>>> Add syscon properties required for configuring PCIe in x2 lane mode.
>>>>>>
>>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
>>>>>> Signed-off-by: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/pci/ti-pci.txt | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti-pci.txt b/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> index 82cb875e4cec..bfbc77ac7355 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pci/ti-pci.txt
>>>>>> @@ -13,6 +13,12 @@ PCIe DesignWare Controller
>>>>>>   - ti,hwmods : Name of the hwmod associated to the pcie, "pcie<X>",
>>>>>>           where <X> is the instance number of the pcie from the HW spec.
>>>>>>   - num-lanes as specified in ../designware-pcie.txt
>>>>>> + - ti,syscon-lane-conf : phandle/offset pair. Phandle to the system control
>>>>>> +                   module and the register offset to specify 1 lane or
>>>>>> +                   2 lane.
>>>>>> + - ti,syscon-lane-sel : phandle/offset pair. Phandle to the system control
>>>>>> +                  module and the register offset to specify lane
>>>>>> +                  selection.
>>>>>
>>>>> Adding a property for every syscon register doesn't really scale and
>>>>> doesn't work if the register layout changes.
>>>>
>>>> The register layout doesn't really change between silicon revisions and for new
>>>> SoCs, the phandle and the register offset for that SoC will have to be
>>>> populated again.
>>
>> And what about SoCs that don't exist yet?
>>
>>>> Having said that, I'm not aware of any other alternative here.
>>
>> What would you do if you had 20 different syscon registers to
>> configure? Add 20 properties? No, you would have per SoC functions in
>> the driver to handle the different cases.
> 
> Ideally these syscon registers would be managed by some Linux
> generic framework such as clock/regulator/mux/phy.
> 
> But yeah, if that does not work, then setting a SoC specific
> configuration function based on the compatible value makes sense
> to me.

All right. I'll add a function for syscon configuration in pci-dra7xx.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] PCI: dra7xx: Enable x2 mode support for dra74x, dra76x and dra72x

[Lorenzo Pieralisi]

On Tue, Dec 19, 2017 at 02:28:23PM +0530, Kishon Vijay Abraham I wrote:
> dra74x/dra76x and dra72x has separate compatible strings. Add support

s/has/have

> for these compatible strings in pci-dra7xx driver to perform syscon
> configurations required to get x2 mode working.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/dwc/pci-dra7xx.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index e77a4ceed74c..3b4427c10228 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -83,11 +83,15 @@
>  #define MSI_REQ_GRANT					BIT(0)
>  #define MSI_VECTOR_SHIFT				7
>  
> +#define PCIE_1LANE_2LANE_SELECTION			BIT(13)
> +#define PCIE_B1C0_MODE_SEL				BIT(2)
> +
>  struct dra7xx_pcie {
>  	struct dw_pcie		*pci;
>  	void __iomem		*base;		/* DT ti_conf */
>  	int			phy_count;	/* DT phy-names count */
>  	struct phy		**phy;
> +	u32			*b1c0_mask;

This looks unused in the current patch.

>  	int			link_gen;
>  	struct irq_domain	*irq_domain;
>  	enum dw_pcie_device_mode mode;
> @@ -95,6 +99,7 @@ struct dra7xx_pcie {
>  
>  struct dra7xx_pcie_of_data {
>  	enum dw_pcie_device_mode mode;
> +	u32 b1co_mode_sel_mask;
>  };
>  
>  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
> @@ -533,6 +538,26 @@ static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = {
>  	.mode = DW_PCIE_EP_TYPE,
>  };
>  
> +static const struct dra7xx_pcie_of_data dra746_pcie_rc_of_data = {
> +	.b1co_mode_sel_mask = BIT(2),

Nit: DT guys are more familiar than me on how this data should be
encoded but maybe bit offset + length can be better ? I do not know, I
have no problem leaving them as masks.

> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct dra7xx_pcie_of_data dra726_pcie_rc_of_data = {
> +	.b1co_mode_sel_mask = GENMASK(3, 2),
> +	.mode = DW_PCIE_RC_TYPE,
> +};
> +
> +static const struct dra7xx_pcie_of_data dra746_pcie_ep_of_data = {
> +	.b1co_mode_sel_mask = BIT(2),
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
> +static const struct dra7xx_pcie_of_data dra726_pcie_ep_of_data = {
> +	.b1co_mode_sel_mask = GENMASK(3, 2),
> +	.mode = DW_PCIE_EP_TYPE,
> +};
> +
>  static const struct of_device_id of_dra7xx_pcie_match[] = {
>  	{
>  		.compatible = "ti,dra7-pcie",
> @@ -542,6 +567,22 @@ static const struct of_device_id of_dra7xx_pcie_match[] = {
>  		.compatible = "ti,dra7-pcie-ep",
>  		.data = &dra7xx_pcie_ep_of_data,
>  	},
> +	{
> +		.compatible = "ti,dra746-pcie-rc",
> +		.data = &dra746_pcie_rc_of_data,
> +	},
> +	{
> +		.compatible = "ti,dra726-pcie-rc",
> +		.data = &dra726_pcie_rc_of_data,
> +	},
> +	{
> +		.compatible = "ti,dra746-pcie-ep",
> +		.data = &dra746_pcie_ep_of_data,
> +	},
> +	{
> +		.compatible = "ti,dra726-pcie-ep",
> +		.data = &dra726_pcie_ep_of_data,
> +	},
>  	{},
>  };
>  
> @@ -587,6 +628,47 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev)
>  	return ret;
>  }
>  
> +static int dra7xx_pcie_configure_two_lane(struct device *dev,
> +					  u32 b1co_mode_sel_mask)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct regmap *pcie_syscon;
> +	unsigned int pcie_reg;
> +
> +	pcie_syscon = syscon_regmap_lookup_by_phandle(np,
> +						      "ti,syscon-lane-conf");
> +	if (IS_ERR(pcie_syscon)) {
> +		dev_err(dev, "unable to get ti,syscon-lane-conf\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_u32_index(np, "ti,syscon-lane-conf", 1,
> +				       &pcie_reg)) {
> +		dev_err(dev, "couldn't get lane configuration reg offset\n");
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(pcie_syscon, pcie_reg, PCIE_1LANE_2LANE_SELECTION,
> +			   PCIE_1LANE_2LANE_SELECTION);

I do not know if this can create issues but the regmap should clear
those bits in the error path ?

> +
> +	pcie_syscon = syscon_regmap_lookup_by_phandle(np, "ti,syscon-lane-sel");
> +	if (IS_ERR(pcie_syscon)) {
> +		dev_err(dev, "unable to get ti,syscon-lane-sel\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_u32_index(np, "ti,syscon-lane-sel", 1,
> +				       &pcie_reg)) {
> +		dev_err(dev, "couldn't get lane selection reg offset\n");
> +		return -EINVAL;
> +	}
> +
> +	regmap_update_bits(pcie_syscon, pcie_reg, b1co_mode_sel_mask,
> +			   PCIE_B1C0_MODE_SEL);
> +
> +	return 0;
> +}
> +
>  static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  {
>  	u32 reg;
> @@ -608,6 +690,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  	const struct of_device_id *match;
>  	const struct dra7xx_pcie_of_data *data;
>  	enum dw_pcie_device_mode mode;
> +	u32 b1co_mode_sel_mask;

Again, do you need this temporary variable (given that you pass the
value below) ?

Thanks,
Lorenzo

>  	match = of_match_device(of_match_ptr(of_dra7xx_pcie_match), dev);
>  	if (!match)
> @@ -615,6 +698,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  
>  	data = (struct dra7xx_pcie_of_data *)match->data;
>  	mode = (enum dw_pcie_device_mode)data->mode;
> +	b1co_mode_sel_mask = data->b1co_mode_sel_mask;
>  
>  	dra7xx = devm_kzalloc(dev, sizeof(*dra7xx), GFP_KERNEL);
>  	if (!dra7xx)
> @@ -673,6 +757,12 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>  	dra7xx->pci = pci;
>  	dra7xx->phy_count = phy_count;
>  
> +	if (phy_count == 2) {
> +		ret = dra7xx_pcie_configure_two_lane(dev, b1co_mode_sel_mask);
> +		if (ret < 0)
> +			goto err_link;
> +	}
> +
>  	ret = dra7xx_pcie_enable_phy(dra7xx);
>  	if (ret) {
>  		dev_err(dev, "failed to enable phy\n");
> -- 
> 2.11.0
> 

Re: [PATCH v2 3/3] PCI: dra7xx: Enable x2 mode support for dra74x, dra76x and dra72x

[Kishon Vijay Abraham I]

Hi Lorenzo,

On Monday 26 February 2018 04:00 PM, Lorenzo Pieralisi wrote:
> On Tue, Dec 19, 2017 at 02:28:23PM +0530, Kishon Vijay Abraham I wrote:
>> dra74x/dra76x and dra72x has separate compatible strings. Add support
> 
> s/has/have
> 
>> for these compatible strings in pci-dra7xx driver to perform syscon
>> configurations required to get x2 mode working.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/dwc/pci-dra7xx.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index e77a4ceed74c..3b4427c10228 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -83,11 +83,15 @@
>>  #define MSI_REQ_GRANT					BIT(0)
>>  #define MSI_VECTOR_SHIFT				7
>>  
>> +#define PCIE_1LANE_2LANE_SELECTION			BIT(13)
>> +#define PCIE_B1C0_MODE_SEL				BIT(2)
>> +
>>  struct dra7xx_pcie {
>>  	struct dw_pcie		*pci;
>>  	void __iomem		*base;		/* DT ti_conf */
>>  	int			phy_count;	/* DT phy-names count */
>>  	struct phy		**phy;
>> +	u32			*b1c0_mask;
> 
> This looks unused in the current patch.

yeah, I'll remove it while sending the next revision.
> 
>>  	int			link_gen;
>>  	struct irq_domain	*irq_domain;
>>  	enum dw_pcie_device_mode mode;
>> @@ -95,6 +99,7 @@ struct dra7xx_pcie {
>>  
>>  struct dra7xx_pcie_of_data {
>>  	enum dw_pcie_device_mode mode;
>> +	u32 b1co_mode_sel_mask;
>>  };
>>  
>>  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
>> @@ -533,6 +538,26 @@ static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = {
>>  	.mode = DW_PCIE_EP_TYPE,
>>  };
>>  
>> +static const struct dra7xx_pcie_of_data dra746_pcie_rc_of_data = {
>> +	.b1co_mode_sel_mask = BIT(2),
> 
> Nit: DT guys are more familiar than me on how this data should be
> encoded but maybe bit offset + length can be better ? I do not know, I
> have no problem leaving them as masks.
> 
>> +	.mode = DW_PCIE_RC_TYPE,
>> +};
>> +
>> +static const struct dra7xx_pcie_of_data dra726_pcie_rc_of_data = {
>> +	.b1co_mode_sel_mask = GENMASK(3, 2),
>> +	.mode = DW_PCIE_RC_TYPE,
>> +};
>> +
>> +static const struct dra7xx_pcie_of_data dra746_pcie_ep_of_data = {
>> +	.b1co_mode_sel_mask = BIT(2),
>> +	.mode = DW_PCIE_EP_TYPE,
>> +};
>> +
>> +static const struct dra7xx_pcie_of_data dra726_pcie_ep_of_data = {
>> +	.b1co_mode_sel_mask = GENMASK(3, 2),
>> +	.mode = DW_PCIE_EP_TYPE,
>> +};
>> +
>>  static const struct of_device_id of_dra7xx_pcie_match[] = {
>>  	{
>>  		.compatible = "ti,dra7-pcie",
>> @@ -542,6 +567,22 @@ static const struct of_device_id of_dra7xx_pcie_match[] = {
>>  		.compatible = "ti,dra7-pcie-ep",
>>  		.data = &dra7xx_pcie_ep_of_data,
>>  	},
>> +	{
>> +		.compatible = "ti,dra746-pcie-rc",
>> +		.data = &dra746_pcie_rc_of_data,
>> +	},
>> +	{
>> +		.compatible = "ti,dra726-pcie-rc",
>> +		.data = &dra726_pcie_rc_of_data,
>> +	},
>> +	{
>> +		.compatible = "ti,dra746-pcie-ep",
>> +		.data = &dra746_pcie_ep_of_data,
>> +	},
>> +	{
>> +		.compatible = "ti,dra726-pcie-ep",
>> +		.data = &dra726_pcie_ep_of_data,
>> +	},
>>  	{},
>>  };
>>  
>> @@ -587,6 +628,47 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev)
>>  	return ret;
>>  }
>>  
>> +static int dra7xx_pcie_configure_two_lane(struct device *dev,
>> +					  u32 b1co_mode_sel_mask)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct regmap *pcie_syscon;
>> +	unsigned int pcie_reg;
>> +
>> +	pcie_syscon = syscon_regmap_lookup_by_phandle(np,
>> +						      "ti,syscon-lane-conf");
>> +	if (IS_ERR(pcie_syscon)) {
>> +		dev_err(dev, "unable to get ti,syscon-lane-conf\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_property_read_u32_index(np, "ti,syscon-lane-conf", 1,
>> +				       &pcie_reg)) {
>> +		dev_err(dev, "couldn't get lane configuration reg offset\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_update_bits(pcie_syscon, pcie_reg, PCIE_1LANE_2LANE_SELECTION,
>> +			   PCIE_1LANE_2LANE_SELECTION);
> 
> I do not know if this can create issues but the regmap should clear
> those bits in the error path ?

hmm yeah, maybe we should try to fall back to 1 lane mode.
> 
>> +
>> +	pcie_syscon = syscon_regmap_lookup_by_phandle(np, "ti,syscon-lane-sel");
>> +	if (IS_ERR(pcie_syscon)) {
>> +		dev_err(dev, "unable to get ti,syscon-lane-sel\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_property_read_u32_index(np, "ti,syscon-lane-sel", 1,
>> +				       &pcie_reg)) {
>> +		dev_err(dev, "couldn't get lane selection reg offset\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_update_bits(pcie_syscon, pcie_reg, b1co_mode_sel_mask,
>> +			   PCIE_B1C0_MODE_SEL);
>> +
>> +	return 0;
>> +}
>> +
>>  static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  {
>>  	u32 reg;
>> @@ -608,6 +690,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev)
>>  	const struct of_device_id *match;
>>  	const struct dra7xx_pcie_of_data *data;
>>  	enum dw_pcie_device_mode mode;
>> +	u32 b1co_mode_sel_mask;
> 
> Again, do you need this temporary variable (given that you pass the
> value below) ?

I used so that we don't cross the 80 line character limit below.

Thanks
Kishon