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

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

[AngeloGioacchino Del Regno]

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.0

[PATCH v3 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!).

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 66ce4b5d309b..8d4b045633da 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		0x10b0
+#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.0

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

[Fei Shao]

Hi Angelo,

On Wed, Sep 18, 2024 at 4:13 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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!).
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

I confirmed that v3 addressed all my comments in v2 (with some in a
more sensible approach), so
Reviewed-by: Fei Shao <fshao@chromium.org>

Regards,
Fei

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

[Manivannan Sadhasivam]

On Wed, Sep 18, 2024 at 10:13:06AM +0200, 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!).
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 66ce4b5d309b..8d4b045633da 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		0x10b0
> +#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.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Jianjun Wang (王建军)]

Hi Angelo,

Thanks for your patch.

On Wed, 2024-09-18 at 10:13 +0200, 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!).
> 
> 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 66ce4b5d309b..8d4b045633da 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		0x10b0

Maybe it's better to use: (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;

Do we need to set it to max_speed? Since the hardware only supports
speeds lower than max_speed.

Thanks.

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

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

[AngeloGioacchino Del Regno]

Il 04/11/24 09:22, Jianjun Wang (王建军) ha scritto:
> Hi Angelo,
> 
> Thanks for your patch.
> 
> On Wed, 2024-09-18 at 10:13 +0200, 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!).
>>
>> 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 66ce4b5d309b..8d4b045633da 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		0x10b0
> 
> Maybe it's better to use: (PCIE_CFG_OFFSET_ADDR + 0xb0).
> 

Makes sense.

>> +#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;
> 
> Do we need to set it to max_speed? Since the hardware only supports
> speeds lower than max_speed.
> 

The controller's default value is already "max speed" (all Gen supported)
so... no, we take action only if DT says to apply a limit.

Cheers,
Angelo

> Thanks.
> 
>> +			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)

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

[Jianjun Wang (王建军)]

On Mon, 2024-11-04 at 12:42 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 04/11/24 09:22, Jianjun Wang (王建军) ha scritto:
> > Hi Angelo,
> > 
> > Thanks for your patch.
> > 
> > On Wed, 2024-09-18 at 10:13 +0200, 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!).
> > > 
> > > 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 66ce4b5d309b..8d4b045633da 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             0x10b0
> > 
> > Maybe it's better to use: (PCIE_CFG_OFFSET_ADDR + 0xb0).
> > 
> 
> Makes sense.
> 
> > > +#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;
> > 
> > Do we need to set it to max_speed? Since the hardware only supports
> > speeds lower than max_speed.
> > 
> 
> The controller's default value is already "max speed" (all Gen
> supported)
> so... no, we take action only if DT says to apply a limit.

Hi Angelo,

I confirmed with our designer and did some tests on the MT8195
platform. We cannot override the speed if DT sets it higher than what
the hardware supports.

The max link speed in the PCIE_SETTING_REG will be reflected in the
corresponding field of the Link Capabilities Register. If we set it
higher than what the hardware supports, it will show the wrong value
when using "lspci -vv", and the PCIe link will drop to Gen1 since the
hardware does not support such a high speed.

The MT8195 PCIe0 only support up to Gen3 speed. With this patch, I set
"max-link-speed = <4>" in DT and connected a Gen4 supported device, the
output of "lspci -vv" will be as follows:
...
LnkCap: Port #1, Speed 16GT/s, Width x2, ASPM L0s L1, Exit Latency L0s
<2us, L1 <8us
...
LnkSta: Speed 2.5GT/s (downgraded), Width x2 (ok)
...

Thanks.

> 
> Cheers,
> Angelo
> 
> > Thanks.
> > 
> > > +                    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)
> 
> 

[PATCH v3 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).

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 8d4b045633da..8dd2e5135b01 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(pcie->num_lanes >> 1, 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.0

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

[Fei Shao]

On Wed, Sep 18, 2024 at 4:13 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> 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).
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Fei Shao <fshao@chromium.org>

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

[Manivannan Sadhasivam]

On Wed, Sep 18, 2024 at 10:13:07AM +0200, 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).
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 8d4b045633da..8dd2e5135b01 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(pcie->num_lanes >> 1, 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.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Jianjun Wang (王建军)]

Hi Angelo,

Thanks for your patch.

On Wed, 2024-09-18 at 10:13 +0200, 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).
> 
> 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 8d4b045633da..8dd2e5135b01 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(pcie->num_lanes >> 1,
> 0));

It should be GENMASK(fls(pcie->num_lanes) - 2, 0).

Thanks.

> +	};
>  	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;
>  }
>  

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

[AngeloGioacchino Del Regno]

Il 04/11/24 09:56, Jianjun Wang (王建军) ha scritto:
> Hi Angelo,
> 
> Thanks for your patch.
> 
> On Wed, 2024-09-18 at 10:13 +0200, 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).
>>
>> 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 8d4b045633da..8dd2e5135b01 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(pcie->num_lanes >> 1,
>> 0));
> 
> It should be GENMASK(fls(pcie->num_lanes) - 2, 0).
> 

You're right in that there's a mistake in that one, and I see it now,
but I don't get why this should be "fls(...) - 2".

The datasheet says that "LinkWidths" is

Bit 8 = x2 supported
Bit 9 = x4 supported
Bit 10 = x8 supported
Bit 11 = x16 supported

pcie->num_lanes can be set to either 2, 4, 8 or 16.

2>>2 = 0   -> fls(0) == 0 (after field_prep/genmask: bit 8)
4>>2 = 1   -> fls(1) == 1 (after field_prep/genmask: bit 9 to 8)
8>>2 = 2   -> fls(2) == 2 (after field_prep/genmask: bit 10 to 8)
16>>2 = 4  -> fls(4) == 3 (after field_prep/genmask: bit 11 to 8)

So, this should be
	GENMASK(fls(pcie->num_lanes >> 2), 0)

Right? :-)

In which case, should I send a new version, or can you fix that while
applying? I'd really appreciate the latter due to lack of time.

Cheers,
Angelo

> Thanks.
> 
>> +	};
>>   	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;
>>   }
>>