[PATCH v4 0/2] PCI: mediatek-gen3: Support limiting link speed and width

[PATCH v4 0/2] PCI: mediatek-gen3: Support limiting link speed and width

[AngeloGioacchino Del Regno]

Changes in v4:
 - Addressed comments from Jianjun Wang's review on v3

Changes in v3:
 - Addressed comments from Fei Shao's review on v2

Changes in v2:
 - Rebased on next-20240917

This series adds support for limiting the PCI-Express link speed
(or PCIe gen restriction) and link width (number of lanes) in the
pcie-mediatek-gen3 driver.

The maximum supported pcie gen is read from the controller itself,
so defining a max gen through platform data for each SoC is avoided.

Both are done by adding support for the standard devicetree properties
`max-link-speed` and `num-lanes`.

Please note that changing the bindings is not required, as those do
already allow specifying those properties for this controller.

AngeloGioacchino Del Regno (2):
  PCI: mediatek-gen3: Add support for setting max-link-speed limit
  PCI: mediatek-gen3: Add support for restricting link width

 drivers/pci/controller/pcie-mediatek-gen3.c | 75 ++++++++++++++++++++-
 1 file changed, 73 insertions(+), 2 deletions(-)

-- 
2.46.1

[PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[AngeloGioacchino Del Regno]

Add support for respecting the max-link-speed devicetree property,
forcing a maximum speed (Gen) for a PCI-Express port.

Since the MediaTek PCIe Gen3 controllers also expose the maximum
supported link speed in the PCIE_BASE_CFG register, if property
max-link-speed is specified in devicetree, validate it against the
controller capabilities and proceed setting the limitations only
if the wanted Gen is lower than the maximum one that is supported
by the controller itself (otherwise it makes no sense!).

Reviewed-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 55 ++++++++++++++++++++-
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 969f62e9cf01..c27beef75079 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -28,7 +28,11 @@
 
 #include "../pci.h"
 
+#define PCIE_BASE_CFG_REG		0x14
+#define PCIE_BASE_CFG_SPEED		GENMASK(15, 8)
+
 #define PCIE_SETTING_REG		0x80
+#define PCIE_SETTING_GEN_SUPPORT	GENMASK(14, 12)
 #define PCIE_PCI_IDS_1			0x9c
 #define PCI_CLASS(class)		(class << 8)
 #define PCIE_RC_MODE			BIT(0)
@@ -125,6 +129,9 @@
 
 struct mtk_gen3_pcie;
 
+#define PCIE_CONF_LINK2_CTL_STS		(PCIE_CFG_OFFSET_ADDR + 0xb0)
+#define PCIE_CONF_LINK2_LCR2_LINK_SPEED	GENMASK(3, 0)
+
 /**
  * struct mtk_gen3_pcie_pdata - differentiate between host generations
  * @power_up: pcie power_up callback
@@ -160,6 +167,7 @@ struct mtk_msi_set {
  * @phy: PHY controller block
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
+ * @max_link_speed: Maximum link speed (PCIe Gen) for this port
  * @irq: PCIe controller interrupt number
  * @saved_irq_state: IRQ enable state saved at suspend time
  * @irq_lock: lock protecting IRQ register access
@@ -180,6 +188,7 @@ struct mtk_gen3_pcie {
 	struct phy *phy;
 	struct clk_bulk_data *clks;
 	int num_clks;
+	u8 max_link_speed;
 
 	int irq;
 	u32 saved_irq_state;
@@ -381,11 +390,27 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 	int err;
 	u32 val;
 
-	/* Set as RC mode */
+	/* Set as RC mode and set controller PCIe Gen speed restriction, if any */
 	val = readl_relaxed(pcie->base + PCIE_SETTING_REG);
 	val |= PCIE_RC_MODE;
+	if (pcie->max_link_speed) {
+		val &= ~PCIE_SETTING_GEN_SUPPORT;
+
+		/* Can enable link speed support only from Gen2 onwards */
+		if (pcie->max_link_speed >= 2)
+			val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT,
+					  GENMASK(pcie->max_link_speed - 2, 0));
+	}
 	writel_relaxed(val, pcie->base + PCIE_SETTING_REG);
 
+	/* Set Link Control 2 (LNKCTL2) speed restriction, if any */
+	if (pcie->max_link_speed) {
+		val = readl_relaxed(pcie->base + PCIE_CONF_LINK2_CTL_STS);
+		val &= ~PCIE_CONF_LINK2_LCR2_LINK_SPEED;
+		val |= FIELD_PREP(PCIE_CONF_LINK2_LCR2_LINK_SPEED, pcie->max_link_speed);
+		writel_relaxed(val, pcie->base + PCIE_CONF_LINK2_CTL_STS);
+	}
+
 	/* Set class code */
 	val = readl_relaxed(pcie->base + PCIE_PCI_IDS_1);
 	val &= ~GENMASK(31, 8);
@@ -1004,9 +1029,21 @@ static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
 	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 }
 
+static int mtk_pcie_get_controller_max_link_speed(struct mtk_gen3_pcie *pcie)
+{
+	u32 val;
+	int ret;
+
+	val = readl_relaxed(pcie->base + PCIE_BASE_CFG_REG);
+	val = FIELD_GET(PCIE_BASE_CFG_SPEED, val);
+	ret = fls(val);
+
+	return ret > 0 ? ret : -EINVAL;
+}
+
 static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 {
-	int err;
+	int err, max_speed;
 
 	err = mtk_pcie_parse_port(pcie);
 	if (err)
@@ -1031,6 +1068,20 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 	if (err)
 		return err;
 
+	err = of_pci_get_max_link_speed(pcie->dev->of_node);
+	if (err > 0) {
+		/* Get the maximum speed supported by the controller */
+		max_speed = mtk_pcie_get_controller_max_link_speed(pcie);
+
+		/* Set max_link_speed only if the controller supports it */
+		if (max_speed >= 0 && max_speed <= err) {
+			pcie->max_link_speed = err;
+			dev_dbg(pcie->dev,
+				"Max controller link speed Gen%d, override to Gen%u",
+				max_speed, pcie->max_link_speed);
+		}
+	}
+
 	/* Try link up */
 	err = mtk_pcie_startup_port(pcie);
 	if (err)
-- 
2.46.1

[PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[AngeloGioacchino Del Regno]

Add support for restricting the port's link width by specifying
the num-lanes devicetree property in the PCIe node.

The setting is done in the GEN_SETTINGS register (in the driver
named as PCIE_SETTING_REG), where each set bit in [11:8] activates
a set of lanes (from bits 11 to 8 respectively, x16/x8/x4/x2).

Reviewed-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index c27beef75079..fa7f36a0284d 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -32,6 +32,7 @@
 #define PCIE_BASE_CFG_SPEED		GENMASK(15, 8)
 
 #define PCIE_SETTING_REG		0x80
+#define PCIE_SETTING_LINK_WIDTH		GENMASK(11, 8)
 #define PCIE_SETTING_GEN_SUPPORT	GENMASK(14, 12)
 #define PCIE_PCI_IDS_1			0x9c
 #define PCI_CLASS(class)		(class << 8)
@@ -168,6 +169,7 @@ struct mtk_msi_set {
  * @clks: PCIe clocks
  * @num_clks: PCIe clocks count for this port
  * @max_link_speed: Maximum link speed (PCIe Gen) for this port
+ * @num_lanes: Number of PCIe lanes for this port
  * @irq: PCIe controller interrupt number
  * @saved_irq_state: IRQ enable state saved at suspend time
  * @irq_lock: lock protecting IRQ register access
@@ -189,6 +191,7 @@ struct mtk_gen3_pcie {
 	struct clk_bulk_data *clks;
 	int num_clks;
 	u8 max_link_speed;
+	u8 num_lanes;
 
 	int irq;
 	u32 saved_irq_state;
@@ -401,6 +404,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
 			val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT,
 					  GENMASK(pcie->max_link_speed - 2, 0));
 	}
+	if (pcie->num_lanes) {
+		val &= ~PCIE_SETTING_LINK_WIDTH;
+
+		/* Zero means one lane, each bit activates x2/x4/x8/x16 */
+		if (pcie->num_lanes > 1)
+			val |= FIELD_PREP(PCIE_SETTING_LINK_WIDTH,
+					  GENMASK(fls(pcie->num_lanes >> 2), 0));
+	};
 	writel_relaxed(val, pcie->base + PCIE_SETTING_REG);
 
 	/* Set Link Control 2 (LNKCTL2) speed restriction, if any */
@@ -838,6 +849,7 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct resource *regs;
+	u32 num_lanes;
 
 	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
 	if (!regs)
@@ -883,6 +895,14 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
 		return pcie->num_clks;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
+	if (ret == 0) {
+		if (num_lanes == 0 || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
+			dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
+		else
+			pcie->num_lanes = num_lanes;
+	}
+
 	return 0;
 }
 
-- 
2.46.1

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[Krzysztof Wilczyński]

Hello,

> +	if (err > 0) {

You could drop > 0 here.

> +		/* Get the maximum speed supported by the controller */
> +		max_speed = mtk_pcie_get_controller_max_link_speed(pcie);
> +
> +		/* Set max_link_speed only if the controller supports it */
> +		if (max_speed >= 0 && max_speed <= err) {
> +			pcie->max_link_speed = err;
> +			dev_dbg(pcie->dev,
> +				"Max controller link speed Gen%d, override to Gen%u",
> +				max_speed, pcie->max_link_speed);
> +		}
> +	}

I wonder if this debug message would be better served as a warning to let
the user know that the speed has been overridden due to the platform
limitation.  Thoughts?

Also, there is no need to sent a new series if you fine with the
suggestions.  I will mend the code on the branch when applying.

	Krzysztof

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[AngeloGioacchino Del Regno]

Il 04/11/24 14:06, Krzysztof Wilczyński ha scritto:
> Hello,
> 
>> +	if (err > 0) {
> 
> You could drop > 0 here.
> 

I have no strong opinions about that, would be fine for me.

>> +		/* Get the maximum speed supported by the controller */
>> +		max_speed = mtk_pcie_get_controller_max_link_speed(pcie);
>> +
>> +		/* Set max_link_speed only if the controller supports it */
>> +		if (max_speed >= 0 && max_speed <= err) {
>> +			pcie->max_link_speed = err;
>> +			dev_dbg(pcie->dev,
>> +				"Max controller link speed Gen%d, override to Gen%u",
>> +				max_speed, pcie->max_link_speed);
>> +		}
>> +	}
> 
> I wonder if this debug message would be better served as a warning to let
> the user know that the speed has been overridden due to the platform
> limitation.  Thoughts?
> 
> Also, there is no need to sent a new series if you fine with the
> suggestions.  I will mend the code on the branch when applying.
> 

A warning seems to be a bit too much and would appear like something to worry
about (or something unintended)...

Perhaps a dev_info() would work better here?

Thanks,
Angelo

> 	Krzysztof

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[Krzysztof Wilczyński]

Hello,

> +	ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
> +	if (ret == 0) {
> +		if (num_lanes == 0 || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
> +			dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
> +		else
> +			pcie->num_lanes = num_lanes;
> +	}
> +
>  	return 0;
>  }

If you were to handle non-zero return value as an error here, perhaps the
property has not been set, then we could reduce the indentation here.

Something like this, perhaps?

  ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
  if (ret) {
          dev_err(dev, "Failed to read num-lanes: %d\n", ret);
          return ret;
  }
  
  if (!num_lanes || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
  	dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
  else
  	pcie->num_lanes = num_lanes;

Does this make sense here?  Thoughts?

	Krzysztof

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[Krzysztof Wilczyński]

Hello,

> > I wonder if this debug message would be better served as a warning to let
> > the user know that the speed has been overridden due to the platform
> > limitation.  Thoughts?
> > 
> > Also, there is no need to sent a new series if you fine with the
> > suggestions.  I will mend the code on the branch when applying.
> > 
> 
> A warning seems to be a bit too much and would appear like something to worry
> about (or something unintended)...
> 
> Perhaps a dev_info() would work better here?

Sounds good!  Thank you!

Will handle the necessary changes while applying the series.

	Krzysztof

Re: [PATCH v4 0/2] PCI: mediatek-gen3: Support limiting link speed and width

[Krzysztof Wilczyński]

Hello,

> Changes in v4:
>  - Addressed comments from Jianjun Wang's review on v3
> 
> Changes in v3:
>  - Addressed comments from Fei Shao's review on v2
> 
> Changes in v2:
>  - Rebased on next-20240917
> 
> This series adds support for limiting the PCI-Express link speed
> (or PCIe gen restriction) and link width (number of lanes) in the
> pcie-mediatek-gen3 driver.
> 
> The maximum supported pcie gen is read from the controller itself,
> so defining a max gen through platform data for each SoC is avoided.
> 
> Both are done by adding support for the standard devicetree properties
> `max-link-speed` and `num-lanes`.
> 
> Please note that changing the bindings is not required, as those do
> already allow specifying those properties for this controller.

Applied to controller/mediatek, thank you!

[01/02] PCI: mediatek-gen3: Add support for setting max-link-speed limit
        https://git.kernel.org/pci/pci/c/ade7da14954a

[02/02] PCI: mediatek-gen3: Add support for restricting link width
        https://git.kernel.org/pci/pci/c/6e73c5898973

	Krzysztof

Re: [PATCH v4 0/2] PCI: mediatek-gen3: Support limiting link speed and width

[Krzysztof Wilczyński]

Hello,

> > Changes in v4:
> >  - Addressed comments from Jianjun Wang's review on v3
> > 
> > Changes in v3:
> >  - Addressed comments from Fei Shao's review on v2
> > 
> > Changes in v2:
> >  - Rebased on next-20240917
> > 
> > This series adds support for limiting the PCI-Express link speed
> > (or PCIe gen restriction) and link width (number of lanes) in the
> > pcie-mediatek-gen3 driver.
> > 
> > The maximum supported pcie gen is read from the controller itself,
> > so defining a max gen through platform data for each SoC is avoided.
> > 
> > Both are done by adding support for the standard devicetree properties
> > `max-link-speed` and `num-lanes`.
> > 
> > Please note that changing the bindings is not required, as those do
> > already allow specifying those properties for this controller.
> 
> Applied to controller/mediatek, thank you!
> 
> [01/02] PCI: mediatek-gen3: Add support for setting max-link-speed limit
>         https://git.kernel.org/pci/pci/c/ade7da14954a
> 
> [02/02] PCI: mediatek-gen3: Add support for restricting link width
>         https://git.kernel.org/pci/pci/c/6e73c5898973

Angelo,

I made some small changes to the code, per the suggestions.  Let me know if
you are fine with these, or not.  Thank you!

	Krzysztof

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[Bjorn Helgaas]

On Mon, Nov 04, 2024 at 12:49:34PM +0100, AngeloGioacchino Del Regno wrote:
> Add support for respecting the max-link-speed devicetree property,
> forcing a maximum speed (Gen) for a PCI-Express port.
> 
> Since the MediaTek PCIe Gen3 controllers also expose the maximum
> supported link speed in the PCIE_BASE_CFG register, if property
> max-link-speed is specified in devicetree, validate it against the
> controller capabilities and proceed setting the limitations only
> if the wanted Gen is lower than the maximum one that is supported
> by the controller itself (otherwise it makes no sense!).
> 
> Reviewed-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Can we get an ack from Ryder and/or Jianjun, since they're listed as
supporters for this driver?

> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -28,7 +28,11 @@
>  
>  #include "../pci.h"
>  
> +#define PCIE_BASE_CFG_REG		0x14
> +#define PCIE_BASE_CFG_SPEED		GENMASK(15, 8)
> +
>  #define PCIE_SETTING_REG		0x80
> +#define PCIE_SETTING_GEN_SUPPORT	GENMASK(14, 12)
>  #define PCIE_PCI_IDS_1			0x9c
>  #define PCI_CLASS(class)		(class << 8)
>  #define PCIE_RC_MODE			BIT(0)
> @@ -125,6 +129,9 @@
>  
>  struct mtk_gen3_pcie;
>  
> +#define PCIE_CONF_LINK2_CTL_STS		(PCIE_CFG_OFFSET_ADDR + 0xb0)
> +#define PCIE_CONF_LINK2_LCR2_LINK_SPEED	GENMASK(3, 0)

Are these the same as PCI_EXP_LNKCTL2 and PCI_EXP_LNKCTL2_TLS?

It looks like you access these registers via an ioremapped MMIO access
instead of config space, but if they reach the same internal register,
please define the appropriate offset and use PCI_EXP_LNKCTL2 and
related definitions so grep can find them and we'll know how to
interpret the PCIE_CONF_LINK2_LCR2_LINK_SPEED field.

>  /**
>   * struct mtk_gen3_pcie_pdata - differentiate between host generations
>   * @power_up: pcie power_up callback
> @@ -160,6 +167,7 @@ struct mtk_msi_set {
>   * @phy: PHY controller block
>   * @clks: PCIe clocks
>   * @num_clks: PCIe clocks count for this port
> + * @max_link_speed: Maximum link speed (PCIe Gen) for this port
>   * @irq: PCIe controller interrupt number
>   * @saved_irq_state: IRQ enable state saved at suspend time
>   * @irq_lock: lock protecting IRQ register access
> @@ -180,6 +188,7 @@ struct mtk_gen3_pcie {
>  	struct phy *phy;
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +	u8 max_link_speed;
>  
>  	int irq;
>  	u32 saved_irq_state;
> @@ -381,11 +390,27 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	int err;
>  	u32 val;
>  
> -	/* Set as RC mode */
> +	/* Set as RC mode and set controller PCIe Gen speed restriction, if any */
>  	val = readl_relaxed(pcie->base + PCIE_SETTING_REG);
>  	val |= PCIE_RC_MODE;
> +	if (pcie->max_link_speed) {
> +		val &= ~PCIE_SETTING_GEN_SUPPORT;
> +
> +		/* Can enable link speed support only from Gen2 onwards */
> +		if (pcie->max_link_speed >= 2)
> +			val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT,
> +					  GENMASK(pcie->max_link_speed - 2, 0));
> +	}
>  	writel_relaxed(val, pcie->base + PCIE_SETTING_REG);
>  
> +	/* Set Link Control 2 (LNKCTL2) speed restriction, if any */
> +	if (pcie->max_link_speed) {
> +		val = readl_relaxed(pcie->base + PCIE_CONF_LINK2_CTL_STS);
> +		val &= ~PCIE_CONF_LINK2_LCR2_LINK_SPEED;
> +		val |= FIELD_PREP(PCIE_CONF_LINK2_LCR2_LINK_SPEED, pcie->max_link_speed);
> +		writel_relaxed(val, pcie->base + PCIE_CONF_LINK2_CTL_STS);
> +	}
> +
>  	/* Set class code */
>  	val = readl_relaxed(pcie->base + PCIE_PCI_IDS_1);
>  	val &= ~GENMASK(31, 8);
> @@ -1004,9 +1029,21 @@ static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie)
>  	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
>  }
>  
> +static int mtk_pcie_get_controller_max_link_speed(struct mtk_gen3_pcie *pcie)
> +{
> +	u32 val;
> +	int ret;
> +
> +	val = readl_relaxed(pcie->base + PCIE_BASE_CFG_REG);
> +	val = FIELD_GET(PCIE_BASE_CFG_SPEED, val);
> +	ret = fls(val);
> +
> +	return ret > 0 ? ret : -EINVAL;
> +}
> +
>  static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>  {
> -	int err;
> +	int err, max_speed;
>  
>  	err = mtk_pcie_parse_port(pcie);
>  	if (err)
> @@ -1031,6 +1068,20 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>  	if (err)
>  		return err;
>  
> +	err = of_pci_get_max_link_speed(pcie->dev->of_node);
> +	if (err > 0) {
> +		/* Get the maximum speed supported by the controller */
> +		max_speed = mtk_pcie_get_controller_max_link_speed(pcie);
> +
> +		/* Set max_link_speed only if the controller supports it */
> +		if (max_speed >= 0 && max_speed <= err) {
> +			pcie->max_link_speed = err;
> +			dev_dbg(pcie->dev,
> +				"Max controller link speed Gen%d, override to Gen%u",
> +				max_speed, pcie->max_link_speed);
> +		}
> +	}
> +
>  	/* Try link up */
>  	err = mtk_pcie_startup_port(pcie);
>  	if (err)
> -- 
> 2.46.1
> 

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[Bjorn Helgaas]

On Mon, Nov 04, 2024 at 12:49:35PM +0100, AngeloGioacchino Del Regno wrote:
> Add support for restricting the port's link width by specifying
> the num-lanes devicetree property in the PCIe node.
> 
> The setting is done in the GEN_SETTINGS register (in the driver
> named as PCIE_SETTING_REG), where each set bit in [11:8] activates
> a set of lanes (from bits 11 to 8 respectively, x16/x8/x4/x2).

I guess GEN_SETTINGS doesn't correspond to a register defined by the
PCIe spec, right?  The only thing in the spec that looks similar is
the Target Link Width in the Device Control 3 register, but the bit
position doesn't look like this PCIE_SETTING_LINK_WIDTH mask:

>  #define PCIE_SETTING_REG		0x80
> +#define PCIE_SETTING_LINK_WIDTH		GENMASK(11, 8)
>  #define PCIE_SETTING_GEN_SUPPORT	GENMASK(14, 12)
>  #define PCIE_PCI_IDS_1			0x9c
>  #define PCI_CLASS(class)		(class << 8)
> @@ -168,6 +169,7 @@ struct mtk_msi_set {
>   * @clks: PCIe clocks
>   * @num_clks: PCIe clocks count for this port
>   * @max_link_speed: Maximum link speed (PCIe Gen) for this port
> + * @num_lanes: Number of PCIe lanes for this port
>   * @irq: PCIe controller interrupt number
>   * @saved_irq_state: IRQ enable state saved at suspend time
>   * @irq_lock: lock protecting IRQ register access
> @@ -189,6 +191,7 @@ struct mtk_gen3_pcie {
>  	struct clk_bulk_data *clks;
>  	int num_clks;
>  	u8 max_link_speed;
> +	u8 num_lanes;
>  
>  	int irq;
>  	u32 saved_irq_state;
> @@ -401,6 +404,14 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  			val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT,
>  					  GENMASK(pcie->max_link_speed - 2, 0));
>  	}
> +	if (pcie->num_lanes) {
> +		val &= ~PCIE_SETTING_LINK_WIDTH;
> +
> +		/* Zero means one lane, each bit activates x2/x4/x8/x16 */
> +		if (pcie->num_lanes > 1)
> +			val |= FIELD_PREP(PCIE_SETTING_LINK_WIDTH,
> +					  GENMASK(fls(pcie->num_lanes >> 2), 0));
> +	};
>  	writel_relaxed(val, pcie->base + PCIE_SETTING_REG);
>  
>  	/* Set Link Control 2 (LNKCTL2) speed restriction, if any */
> @@ -838,6 +849,7 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
>  	struct device *dev = pcie->dev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct resource *regs;
> +	u32 num_lanes;
>  
>  	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");
>  	if (!regs)
> @@ -883,6 +895,14 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie)
>  		return pcie->num_clks;
>  	}
>  
> +	ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
> +	if (ret == 0) {
> +		if (num_lanes == 0 || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
> +			dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
> +		else
> +			pcie->num_lanes = num_lanes;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.46.1
> 

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[Jianjun Wang (王建军)]

On Tue, 2024-11-05 at 19:24 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Mon, Nov 04, 2024 at 12:49:35PM +0100, AngeloGioacchino Del Regno
> wrote:
> > Add support for restricting the port's link width by specifying
> > the num-lanes devicetree property in the PCIe node.
> > 
> > The setting is done in the GEN_SETTINGS register (in the driver
> > named as PCIE_SETTING_REG), where each set bit in [11:8] activates
> > a set of lanes (from bits 11 to 8 respectively, x16/x8/x4/x2).
> 
> I guess GEN_SETTINGS doesn't correspond to a register defined by the
> PCIe spec, right?  The only thing in the spec that looks similar is
> the Target Link Width in the Device Control 3 register, but the bit
> position doesn't look like this PCIE_SETTING_LINK_WIDTH mask:

Hi Bjorn,

The "GEN_SETTINGS" and "LINK_WIDTH" settings will be reflected in the
Max Link Speed and Maximum Link Width fields in the PCIe Link
Capabilities Register.

However, these registers have slight differences in hardware design:
The Max Link Speed shown in the Link Capabilities Register can be
directly changed by the "GEN_SETTINGS" register, even if the hardware
cannot support such a high speed. On the other hand, when we set the
"LINK_WIDTH", it will compare with the maximum width that the hardware
actually supports and choose the smaller one.

Thanks.

> 
> >  #define PCIE_SETTING_REG             0x80
> > +#define PCIE_SETTING_LINK_WIDTH              GENMASK(11, 8)
> >  #define PCIE_SETTING_GEN_SUPPORT     GENMASK(14, 12)
> >  #define PCIE_PCI_IDS_1                       0x9c
> >  #define PCI_CLASS(class)             (class << 8)
> > @@ -168,6 +169,7 @@ struct mtk_msi_set {
> >   * @clks: PCIe clocks
> >   * @num_clks: PCIe clocks count for this port
> >   * @max_link_speed: Maximum link speed (PCIe Gen) for this port
> > + * @num_lanes: Number of PCIe lanes for this port
> >   * @irq: PCIe controller interrupt number
> >   * @saved_irq_state: IRQ enable state saved at suspend time
> >   * @irq_lock: lock protecting IRQ register access
> > @@ -189,6 +191,7 @@ struct mtk_gen3_pcie {
> >       struct clk_bulk_data *clks;
> >       int num_clks;
> >       u8 max_link_speed;
> > +     u8 num_lanes;
> > 
> >       int irq;
> >       u32 saved_irq_state;
> > @@ -401,6 +404,14 @@ static int mtk_pcie_startup_port(struct
> > mtk_gen3_pcie *pcie)
> >                       val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT,
> >                                         GENMASK(pcie-
> > >max_link_speed - 2, 0));
> >       }
> > +     if (pcie->num_lanes) {
> > +             val &= ~PCIE_SETTING_LINK_WIDTH;
> > +
> > +             /* Zero means one lane, each bit activates
> > x2/x4/x8/x16 */
> > +             if (pcie->num_lanes > 1)
> > +                     val |= FIELD_PREP(PCIE_SETTING_LINK_WIDTH,
> > +                                       GENMASK(fls(pcie->num_lanes 
> > >> 2), 0));
> > +     };
> >       writel_relaxed(val, pcie->base + PCIE_SETTING_REG);
> > 
> >       /* Set Link Control 2 (LNKCTL2) speed restriction, if any */
> > @@ -838,6 +849,7 @@ static int mtk_pcie_parse_port(struct
> > mtk_gen3_pcie *pcie)
> >       struct device *dev = pcie->dev;
> >       struct platform_device *pdev = to_platform_device(dev);
> >       struct resource *regs;
> > +     u32 num_lanes;
> > 
> >       regs = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "pcie-mac");
> >       if (!regs)
> > @@ -883,6 +895,14 @@ static int mtk_pcie_parse_port(struct
> > mtk_gen3_pcie *pcie)
> >               return pcie->num_clks;
> >       }
> > 
> > +     ret = of_property_read_u32(dev->of_node, "num-lanes",
> > &num_lanes);
> > +     if (ret == 0) {
> > +             if (num_lanes == 0 || num_lanes > 16 || (num_lanes !=
> > 1 && num_lanes % 2))
> > +                     dev_warn(dev, "Invalid num-lanes, using
> > controller defaults\n");
> > +             else
> > +                     pcie->num_lanes = num_lanes;
> > +     }
> > +
> >       return 0;
> >  }
> > 
> > --
> > 2.46.1
> > 

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[AngeloGioacchino Del Regno]

Il 04/11/24 14:20, Krzysztof Wilczyński ha scritto:
> 
> Hello,
> 
>> +	ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
>> +	if (ret == 0) {
>> +		if (num_lanes == 0 || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
>> +			dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
>> +		else
>> +			pcie->num_lanes = num_lanes;
>> +	}
>> +
>>   	return 0;
>>   }
> 
> If you were to handle non-zero return value as an error here, perhaps the
> property has not been set, then we could reduce the indentation here.
> 
> Something like this, perhaps?
> 
>    ret = of_property_read_u32(dev->of_node, "num-lanes", &num_lanes);
>    if (ret) {
>            dev_err(dev, "Failed to read num-lanes: %d\n", ret);
>            return ret;
>    }
>    
>    if (!num_lanes || num_lanes > 16 || (num_lanes != 1 && num_lanes % 2))
>    	dev_warn(dev, "Invalid num-lanes, using controller defaults\n");
>    else
>    	pcie->num_lanes = num_lanes;
> 
> Does this make sense here?  Thoughts?
> 
> 	Krzysztof


Sorry I've just seen this email.

There's a problem here: this property has to be optional - and if you change that
to return like that, you're breaking compatibility with older device trees, which
are not specifying any "num-lanes" property.

Please remember that of_property_read_u32() returns:
  - 0 on success
  - -EINVAL if the property does not exist
  - -ENODATA or -EOVERFLOW

Please either keep the error checking like I wrote, or alternatively you can do..

ret = of_property_read_u32(...)
if (ret != -EINVAL) {
	dev_err(dev, "Failed to read num-lanes: %d\n", ret);
	return ret;
} else {
	if (num_lanes == 0 || ..... etc etc)
		dev_warn()
	else
		pcie->num_lanes = num_lanes
}

Cheers,
Angelo

Re: [PATCH v4 2/2] PCI: mediatek-gen3: Add support for restricting link width

[Krzysztof Wilczyński]

Hello,

> > Does this make sense here?  Thoughts?
[...]
> There's a problem here: this property has to be optional - and if you change that
> to return like that, you're breaking compatibility with older device trees, which
> are not specifying any "num-lanes" property.

Makes sense.  I will keep your version.  Thank you!

	Krzysztof

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[Jianjun Wang (王建军)]

Hi Krzysztof,

On Mon, 2024-11-04 at 22:25 +0900, Krzysztof Wilczyński wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hello,
> 
> > > I wonder if this debug message would be better served as a
> > > warning to let
> > > the user know that the speed has been overridden due to the
> > > platform
> > > limitation.  Thoughts?
> > > 
> > > Also, there is no need to sent a new series if you fine with the
> > > suggestions.  I will mend the code on the branch when applying.
> > > 
> > 
> > A warning seems to be a bit too much and would appear like
> > something to worry
> > about (or something unintended)...
> > 
> > Perhaps a dev_info() would work better here?
> 
> Sounds good!  Thank you!
> 
> Will handle the necessary changes while applying the series.
> 
>         Krzysztof

This patch may need more discussion. I have replied in the previous
version:

https://lore.kernel.org/linux-pci/7e220693f076c84cc1bc3d91e797580b320b4598.camel@mediatek.com/T/#m1b9f2d26a228712b6b9d02eba11d8063862772c1

Should we wait longer before applying this patch? Do you have any
suggestions? I can provide more logs or test results if needed. Sorry
for the inconvenience.

Thanks.

> 

Re: [PATCH v4 1/2] PCI: mediatek-gen3: Add support for setting max-link-speed limit

[Krzysztof Wilczyński]

Hello,

[...]
> This patch may need more discussion. I have replied in the previous
> version:
> 
> https://lore.kernel.org/linux-pci/7e220693f076c84cc1bc3d91e797580b320b4598.camel@mediatek.com/T/#m1b9f2d26a228712b6b9d02eba11d8063862772c1
> 
> Should we wait longer before applying this patch?

We can drop the series, and it's not yet too lat to do so, if you believe
this would break devices like the one you tested the changes on.

> Do you have any suggestions? I can provide more logs or test results if
> needed. Sorry for the inconvenience.

AngeloGioacchino, have you seen the reply (see the link above) from Jianjun?

	Krzysztof