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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8186d78e-2aa8-436e-be76-412860a3105a@collabora.com>
Date: Wed, 18 Sep 2024 10:08:02 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Fei Shao <fshao@...omium.org>
Cc: linux-pci@...r.kernel.org, ryder.lee@...iatek.com,
 jianjun.wang@...iatek.com, lpieralisi@...nel.org, kw@...ux.com,
 robh@...nel.org, bhelgaas@...gle.com, matthias.bgg@...il.com,
 linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH v2 1/2] PCI: mediatek-gen3: Add support for setting
 max-link-speed limit

Il 17/09/24 18:15, Fei Shao ha scritto:
> On Tue, Sep 17, 2024 at 5:13 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@...labora.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@...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..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
>>
>>



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ