| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <47154bc7-5e52-4d2c-ba30-730758bf1901@collabora.com>
Date: Mon, 4 Nov 2024 12:42:43 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Jianjun Wang (王建军) <Jianjun.Wang@...iatek.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"kernel@...labora.com" <kernel@...labora.com>,
"robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"lpieralisi@...nel.org" <lpieralisi@...nel.org>,
Ryder Lee <Ryder.Lee@...iatek.com>, "fshao@...omium.org" <fshao@...omium.org>
Subject: Re: [PATCH v3 1/2] PCI: mediatek-gen3: Add support for setting
max-link-speed limit
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@...labora.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)
Powered by blists - more mailing lists