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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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)