| Message ID | 20240918081307.51264-2-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: mediatek-gen3: Support limiting link speed and width | expand |
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
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 > >
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)
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)
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)
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(-)