diff mbox series

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

Message ID 20240917091132.286582-2-angelogioacchino.delregno@collabora.com (mailing list archive)
State Superseded
Headers show
Series PCI: mediatek-gen3: Support limiting link speed and width | expand

Commit Message

AngeloGioacchino Del Regno Sept. 17, 2024, 9:11 a.m. UTC
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(-)

Comments

Fei Shao Sept. 17, 2024, 4:15 p.m. UTC | #1
On Tue, Sep 17, 2024 at 5: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>
> ---
>  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..e1d1fb39d5c6 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_MASK       GENMASK(15, 8)
> +
>  #define PCIE_SETTING_REG               0x80
> +#define PCIE_SETTING_GEN_SUPPORT_MASK  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*/

NIt: one space before ending the comment.

>         val = readl_relaxed(pcie->base + PCIE_SETTING_REG);
>         val |= PCIE_RC_MODE;
> +       if (pcie->max_link_speed) {
> +               val &= ~PCIE_SETTING_GEN_SUPPORT_MASK;
> +
> +               /* Can enable link speed support only from Gen2 onwards */
> +               if (pcie->max_link_speed >= 2)
> +                       val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT_MASK,
> +                                         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;

I guess it needs a bitwise NOT operator over the mask.
    val &= ~PCIE_CONF_LINK2_LCR2_LINK_SPEED;

Apart from that, I think appending _MASK to the name makes its usage
clearer and consistent with other masks.
(although the name gets even more lengthy...)

> +               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_MASK, val);
> +       ret = fls(val);
> +
> +       return ret > 0 ? ret : -EINVAL;
> +}
> +
>  static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>  {
> -       int err;
> +       int max_speed, err;
>
>         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%u, override to Gen%u",
> +                               max_speed, pcie->max_link_speed);

Convert max_speed to an unsigned type to avoid potential typecheck warnings?

Regards,
Fei


> +               }
> +       }
> +
>         /* Try link up */
>         err = mtk_pcie_startup_port(pcie);
>         if (err)
> --
> 2.46.0
>
>
AngeloGioacchino Del Regno Sept. 18, 2024, 8:08 a.m. UTC | #2
Il 17/09/24 18:15, Fei Shao ha scritto:
> On Tue, Sep 17, 2024 at 5: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>
>> ---
>>   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..e1d1fb39d5c6 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_MASK       GENMASK(15, 8)
>> +
>>   #define PCIE_SETTING_REG               0x80
>> +#define PCIE_SETTING_GEN_SUPPORT_MASK  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*/
> 
> NIt: one space before ending the comment.
> 

Yep. :-)

>>          val = readl_relaxed(pcie->base + PCIE_SETTING_REG);
>>          val |= PCIE_RC_MODE;
>> +       if (pcie->max_link_speed) {
>> +               val &= ~PCIE_SETTING_GEN_SUPPORT_MASK;
>> +
>> +               /* Can enable link speed support only from Gen2 onwards */
>> +               if (pcie->max_link_speed >= 2)
>> +                       val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT_MASK,
>> +                                         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;
> 
> I guess it needs a bitwise NOT operator over the mask.
>      val &= ~PCIE_CONF_LINK2_LCR2_LINK_SPEED;
> 

I have no idea how the NOT disappeared from this line :o
Nice catch.

> Apart from that, I think appending _MASK to the name makes its usage
> clearer and consistent with other masks.
> (although the name gets even more lengthy...)

Actually, the only other ones that say _MASK are the ones that I am introducing
in this commit, other than PCIE_LTSSM_STATE_MASK... all of the others do *not*
have the _MASK suffix.

At this point, for consistency, I should rather drop the _MASK suffix from the
PCIE_BASE_CFG_SPEED and PCIE_SETTING_GEN_SUPPORT definitions that I introduce
here, so I'll do exactly that for v3.

> 
>> +               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_MASK, val);
>> +       ret = fls(val);
>> +
>> +       return ret > 0 ? ret : -EINVAL;
>> +}
>> +
>>   static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>>   {
>> -       int err;
>> +       int max_speed, err;

Self-review: reorder this, e comes before m. Oops!

>>
>>          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%u, override to Gen%u",
>> +                               max_speed, pcie->max_link_speed);
> 
> Convert max_speed to an unsigned type to avoid potential typecheck warnings?
> 

I'll just change that to "[...] Gen%d, override to Gen%u", looks simply
cleaner, in the end...

Thanks!
Angelo

> Regards,
> Fei
> 
> 
>> +               }
>> +       }
>> +
>>          /* Try link up */
>>          err = mtk_pcie_startup_port(pcie);
>>          if (err)
>> --
>> 2.46.0
>>
>>
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 66ce4b5d309b..e1d1fb39d5c6 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_MASK	GENMASK(15, 8)
+
 #define PCIE_SETTING_REG		0x80
+#define PCIE_SETTING_GEN_SUPPORT_MASK	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_MASK;
+
+		/* Can enable link speed support only from Gen2 onwards */
+		if (pcie->max_link_speed >= 2)
+			val |= FIELD_PREP(PCIE_SETTING_GEN_SUPPORT_MASK,
+					  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_MASK, val);
+	ret = fls(val);
+
+	return ret > 0 ? ret : -EINVAL;
+}
+
 static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 {
-	int err;
+	int max_speed, err;
 
 	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%u, override to Gen%u",
+				max_speed, pcie->max_link_speed);
+		}
+	}
+
 	/* Try link up */
 	err = mtk_pcie_startup_port(pcie);
 	if (err)