Add support to request DRAM bandwidth with Memory Interconnect
in Tegra234 SoC. The DRAM BW required for different modes depends
on speed (Gen-1/2/3/4) and width/lanes (x1/x2/x4/x8).
Suggested-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 09825b4a075e..d2513c9d3feb 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -15,6 +15,7 @@
#include <linux/gpio.h>
#include <linux/gpio/consumer.h>
#include <linux/interrupt.h>
+#include <linux/interconnect.h>
#include <linux/iopoll.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -287,6 +288,7 @@ struct tegra_pcie_dw {
unsigned int pex_rst_irq;
int ep_state;
long link_status;
+ struct icc_path *icc_path;
};
static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
@@ -309,6 +311,24 @@ struct tegra_pcie_soc {
enum dw_pcie_device_mode mode;
};
+static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
+{
+ struct dw_pcie *pci = &pcie->pci;
+ u32 val, speed, width;
+
+ val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
+
+ speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
+ width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
+
+ val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
+
+ if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
+ dev_err(pcie->dev, "can't set bw[%u]\n", val);
+
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+}
+
static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -452,14 +472,12 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
struct tegra_pcie_dw *pcie = arg;
struct dw_pcie_ep *ep = &pcie->pci.ep;
struct dw_pcie *pci = &pcie->pci;
- u32 val, speed;
+ u32 val;
if (test_and_clear_bit(0, &pcie->link_status))
dw_pcie_ep_linkup(ep);
- speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
- PCI_EXP_LNKSTA_CLS;
- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ tegra_pcie_icc_set(pcie);
if (pcie->of_data->has_ltr_req_fix)
return IRQ_HANDLED;
@@ -945,9 +963,9 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
{
- u32 val, offset, speed, tmp;
struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
struct dw_pcie_rp *pp = &pci->pp;
+ u32 val, offset, tmp;
bool retry = true;
if (pcie->of_data->mode == DW_PCIE_EP_TYPE) {
@@ -1018,9 +1036,7 @@ static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
goto retry_link;
}
- speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
- PCI_EXP_LNKSTA_CLS;
- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ tegra_pcie_icc_set(pcie);
tegra_pcie_enable_interrupts(pp);
@@ -2224,6 +2240,14 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, pcie);
+ pcie->icc_path = devm_of_icc_get(&pdev->dev, "write");
+ ret = PTR_ERR_OR_ZERO(pcie->icc_path);
+ if (ret) {
+ tegra_bpmp_put(pcie->bpmp);
+ dev_err_probe(&pdev->dev, ret, "failed to get write interconnect\n");
+ return ret;
+ }
+
switch (pcie->of_data->mode) {
case DW_PCIE_RC_TYPE:
ret = devm_request_irq(dev, pp->irq, tegra_pcie_rp_irq_handler,
--
2.17.1Capitalize subject line please, to match pcie-tegra194.c history.
On Mon, Mar 27, 2023 at 09:44:26PM +0530, Sumit Gupta wrote:
> Add support to request DRAM bandwidth with Memory Interconnect
> in Tegra234 SoC. The DRAM BW required for different modes depends
> on speed (Gen-1/2/3/4) and width/lanes (x1/x2/x4/x8).
>
> Suggested-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
> drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 09825b4a075e..d2513c9d3feb 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -15,6 +15,7 @@
> #include <linux/gpio.h>
> #include <linux/gpio/consumer.h>
> #include <linux/interrupt.h>
> +#include <linux/interconnect.h>
Almost alphabetized, swap interrupt.h and interconnect.h.
> #include <linux/iopoll.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -287,6 +288,7 @@ struct tegra_pcie_dw {
> unsigned int pex_rst_irq;
> int ep_state;
> long link_status;
> + struct icc_path *icc_path;
> };
>
> static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
> @@ -309,6 +311,24 @@ struct tegra_pcie_soc {
> enum dw_pcie_device_mode mode;
> };
>
> +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
> +{
> + struct dw_pcie *pci = &pcie->pci;
> + u32 val, speed, width;
> +
> + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
> +
> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
> +
> + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
> +
> + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
> + dev_err(pcie->dev, "can't set bw[%u]\n", val);
> +
> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
pcie_gen_freq[] is of size 4 (valid indices 0..3).
I see that you're just *moving* this code, but might as well fix it.
> +}
> +
> static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -452,14 +472,12 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
> struct tegra_pcie_dw *pcie = arg;
> struct dw_pcie_ep *ep = &pcie->pci.ep;
> struct dw_pcie *pci = &pcie->pci;
> - u32 val, speed;
> + u32 val;
>
> if (test_and_clear_bit(0, &pcie->link_status))
> dw_pcie_ep_linkup(ep);
>
> - speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
> - PCI_EXP_LNKSTA_CLS;
> - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> + tegra_pcie_icc_set(pcie);
On 28/03/23 23:23, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> Capitalize subject line please, to match pcie-tegra194.c history.
>
> On Mon, Mar 27, 2023 at 09:44:26PM +0530, Sumit Gupta wrote:
>> Add support to request DRAM bandwidth with Memory Interconnect
>> in Tegra234 SoC. The DRAM BW required for different modes depends
>> on speed (Gen-1/2/3/4) and width/lanes (x1/x2/x4/x8).
>>
>> Suggested-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>> drivers/pci/controller/dwc/pcie-tegra194.c | 40 +++++++++++++++++-----
>> 1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index 09825b4a075e..d2513c9d3feb 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -15,6 +15,7 @@
>> #include <linux/gpio.h>
>> #include <linux/gpio/consumer.h>
>> #include <linux/interrupt.h>
>> +#include <linux/interconnect.h>
>
> Almost alphabetized, swap interrupt.h and interconnect.h.
>
Ok, will swap.
>> #include <linux/iopoll.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> @@ -287,6 +288,7 @@ struct tegra_pcie_dw {
>> unsigned int pex_rst_irq;
>> int ep_state;
>> long link_status;
>> + struct icc_path *icc_path;
>> };
>>
>> static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
>> @@ -309,6 +311,24 @@ struct tegra_pcie_soc {
>> enum dw_pcie_device_mode mode;
>> };
>>
>> +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
>> +{
>> + struct dw_pcie *pci = &pcie->pci;
>> + u32 val, speed, width;
>> +
>> + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
>> +
>> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
>> +
>> + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
>> +
>> + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
>> + dev_err(pcie->dev, "can't set bw[%u]\n", val);
>> +
>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>
> Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
> speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
>
> pcie_gen_freq[] is of size 4 (valid indices 0..3).
>
> I see that you're just *moving* this code, but might as well fix it.
>
Thank you for the review.
Will include the below change in the same patch. Please let me know if
any issue.
- clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+ else
+ clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);
Thank you,
Sumit Gupta
>> +}
>> +
>> static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> @@ -452,14 +472,12 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
>> struct tegra_pcie_dw *pcie = arg;
>> struct dw_pcie_ep *ep = &pcie->pci.ep;
>> struct dw_pcie *pci = &pcie->pci;
>> - u32 val, speed;
>> + u32 val;
>>
>> if (test_and_clear_bit(0, &pcie->link_status))
>> dw_pcie_ep_linkup(ep);
>>
>> - speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
>> - PCI_EXP_LNKSTA_CLS;
>> - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>> + tegra_pcie_icc_set(pcie);
On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
> On 28/03/23 23:23, Bjorn Helgaas wrote:
> > > +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
> > > +{
> > > + struct dw_pcie *pci = &pcie->pci;
> > > + u32 val, speed, width;
> > > +
> > > + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
> > > +
> > > + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
> > > + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
> > > +
> > > + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
> > > +
> > > + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
> > > + dev_err(pcie->dev, "can't set bw[%u]\n", val);
> > > +
> > > + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> >
> > Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
> > speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
> >
> > pcie_gen_freq[] is of size 4 (valid indices 0..3).
> >
> > I see that you're just *moving* this code, but might as well fix it.
> >
> Thank you for the review.
> Will include the below change in the same patch. Please let me know if any
> issue.
>
> - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> + if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> + else
> + clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);
I didn't notice that speed is a u32, so -1 is not a possible value.
Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do
something like this:
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
if (speed >= ARRAY_SIZE(pcie_gen_freq))
speed = 0;
val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
BITS_PER_BYTE);
...
clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
On 29/03/23 22:29, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
>> On 28/03/23 23:23, Bjorn Helgaas wrote:
>>>> +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
>>>> +{
>>>> + struct dw_pcie *pci = &pcie->pci;
>>>> + u32 val, speed, width;
>>>> +
>>>> + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
>>>> +
>>>> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>>>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
>>>> +
>>>> + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
>>>> +
>>>> + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
>>>> + dev_err(pcie->dev, "can't set bw[%u]\n", val);
>>>> +
>>>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>>
>>> Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
>>> speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
>>>
>>> pcie_gen_freq[] is of size 4 (valid indices 0..3).
>>>
>>> I see that you're just *moving* this code, but might as well fix it.
>>>
>> Thank you for the review.
>> Will include the below change in the same patch. Please let me know if any
>> issue.
>>
>> - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>> + if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>> + else
>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);
>
> I didn't notice that speed is a u32, so -1 is not a possible value.
> Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do
> something like this:
>
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
> if (speed >= ARRAY_SIZE(pcie_gen_freq))
> speed = 0;
>
> val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
> BITS_PER_BYTE);
> ...
> clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
I tried this change but PCIE_SPEED2MBS_ENC gives zero when speed value
is one. The speed value ranges from "1 to 4" and for value "1",
pcie_link_speed[speed] gives '0xff'.
const unsigned char pcie_link_speed[] = {
PCI_SPEED_UNKNOWN, /* 0 */
The below change works fine. Please share if its OK to add it in patch.
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
if (!speed || speed >= ARRAY_SIZE(pcie_gen_freq))
speed = 1;
val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
BITS_PER_BYTE);
if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
dev_err(pcie->dev, "can't set bw[%u]\n", val);
clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
Thank you,
Sumit Gupta
On Wed, Mar 29, 2023 at 11:28:40PM +0530, Sumit Gupta wrote:
> On 29/03/23 22:29, Bjorn Helgaas wrote:
> > On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
> > > On 28/03/23 23:23, Bjorn Helgaas wrote:
> > > > > +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
> > > > > +{
> > > > > + struct dw_pcie *pci = &pcie->pci;
> > > > > + u32 val, speed, width;
> > > > > +
> > > > > + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
> > > > > +
> > > > > + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
> > > > > + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
> > > > > +
> > > > > + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
> > > > > +
> > > > > + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
> > > > > + dev_err(pcie->dev, "can't set bw[%u]\n", val);
> > > > > +
> > > > > + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> > > >
> > > > Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
> > > > speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
> > > >
> > > > pcie_gen_freq[] is of size 4 (valid indices 0..3).
> > > >
> > > > I see that you're just *moving* this code, but might as well fix it.
> > > >
> > > Thank you for the review.
> > > Will include the below change in the same patch. Please let me know if any
> > > issue.
> > >
> > > - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> > > + if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
> > > + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
> > > + else
> > > + clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);
> >
> > I didn't notice that speed is a u32, so -1 is not a possible value.
> > Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do
> > something like this:
> >
> > speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
> > if (speed >= ARRAY_SIZE(pcie_gen_freq))
> > speed = 0;
> >
> > val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
> > BITS_PER_BYTE);
> > ...
> > clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
>
> I tried this change but PCIE_SPEED2MBS_ENC gives zero when speed value is
> one. The speed value ranges from "1 to 4" and for value "1",
> pcie_link_speed[speed] gives '0xff'.
Oh, my fault, sorry! I thought both places indexed the same array,
but the first is pcie_link_speed[] (where all the possible values
(0..0xf) are valid indices) and the second is pcie_gen_freq[] (where
only 0..3 are valid).
> The below change works fine. Please share if its OK to add it in patch.
>
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
> if (!speed || speed >= ARRAY_SIZE(pcie_gen_freq))
> speed = 1;
>
> val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
> BITS_PER_BYTE);
So I don't think you need to clamp "speed" for indexing
pcie_link_speed[] at all.
> if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
> dev_err(pcie->dev, "can't set bw[%u]\n", val);
>
> clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
What if you added a 0th entry to pcie_gen_freq[] so you can index it
directly with the PCI_EXP_LNKSTA_CLS value the same way as
pcie_link_speed[]? Then you wouldn't need the "- 1" and only have to
worry about going off the end:
static const unsigned int pcie_gen_freq[] = {
GEN1_CORE_CLK_FREQ, /* PCI_EXP_LNKSTA_CLS == 0; undefined */
GEN1_CORE_CLK_FREQ,
GEN2_CORE_CLK_FREQ,
GEN3_CORE_CLK_FREQ,
GEN4_CORE_CLK_FREQ,
};
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
BITS_PER_BYTE);
if (speed >= ARRAY_SIZE(pcie_gen_freq))
speed = 0;
clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
Bjorn
On 30/03/23 00:47, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Mar 29, 2023 at 11:28:40PM +0530, Sumit Gupta wrote:
>> On 29/03/23 22:29, Bjorn Helgaas wrote:
>>> On Wed, Mar 29, 2023 at 02:44:34PM +0530, Sumit Gupta wrote:
>>>> On 28/03/23 23:23, Bjorn Helgaas wrote:
>>>>>> +static void tegra_pcie_icc_set(struct tegra_pcie_dw *pcie)
>>>>>> +{
>>>>>> + struct dw_pcie *pci = &pcie->pci;
>>>>>> + u32 val, speed, width;
>>>>>> +
>>>>>> + val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
>>>>>> +
>>>>>> + speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>>>>>> + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
>>>>>> +
>>>>>> + val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) / BITS_PER_BYTE);
>>>>>> +
>>>>>> + if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
>>>>>> + dev_err(pcie->dev, "can't set bw[%u]\n", val);
>>>>>> +
>>>>>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>>>>
>>>>> Array bounds violation; PCI_EXP_LNKSTA_CLS is 0x000f, so possible
>>>>> speed (CLS) values are 0..0xf and "speed - 1" values are -1..0xe.
>>>>>
>>>>> pcie_gen_freq[] is of size 4 (valid indices 0..3).
>>>>>
>>>>> I see that you're just *moving* this code, but might as well fix it.
>>>>>
>>>> Thank you for the review.
>>>> Will include the below change in the same patch. Please let me know if any
>>>> issue.
>>>>
>>>> - clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>>> + if (speed && (speed <= ARRAY_SIZE(pcie_gen_freq)))
>>>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>>>> + else
>>>> + clk_set_rate(pcie->core_clk, pcie_gen_freq[0]);
>>>
>>> I didn't notice that speed is a u32, so -1 is not a possible value.
>>> Also, it's used earlier for PCIE_SPEED2MBS_ENC(), so you could do
>>> something like this:
>>>
>>> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val) - 1;
>>> if (speed >= ARRAY_SIZE(pcie_gen_freq))
>>> speed = 0;
>>>
>>> val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
>>> BITS_PER_BYTE);
>>> ...
>>> clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
>>
>> I tried this change but PCIE_SPEED2MBS_ENC gives zero when speed value is
>> one. The speed value ranges from "1 to 4" and for value "1",
>> pcie_link_speed[speed] gives '0xff'.
>
> Oh, my fault, sorry! I thought both places indexed the same array,
> but the first is pcie_link_speed[] (where all the possible values
> (0..0xf) are valid indices) and the second is pcie_gen_freq[] (where
> only 0..3 are valid).
>
>> The below change works fine. Please share if its OK to add it in patch.
>>
>> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>> if (!speed || speed >= ARRAY_SIZE(pcie_gen_freq))
>> speed = 1;
>>
>> val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
>> BITS_PER_BYTE);
>
> So I don't think you need to clamp "speed" for indexing
> pcie_link_speed[] at all.
>
>> if (icc_set_bw(pcie->icc_path, MBps_to_icc(val), 0))
>> dev_err(pcie->dev, "can't set bw[%u]\n", val);
>>
>> clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
>
> What if you added a 0th entry to pcie_gen_freq[] so you can index it
> directly with the PCI_EXP_LNKSTA_CLS value the same way as
> pcie_link_speed[]? Then you wouldn't need the "- 1" and only have to
> worry about going off the end:
>
> static const unsigned int pcie_gen_freq[] = {
> GEN1_CORE_CLK_FREQ, /* PCI_EXP_LNKSTA_CLS == 0; undefined */
> GEN1_CORE_CLK_FREQ,
> GEN2_CORE_CLK_FREQ,
> GEN3_CORE_CLK_FREQ,
> GEN4_CORE_CLK_FREQ,
> };
>
> speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, val);
>
> val = width * (PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]) /
> BITS_PER_BYTE);
>
> if (speed >= ARRAY_SIZE(pcie_gen_freq))
> speed = 0;
> clk_set_rate(pcie->core_clk, pcie_gen_freq[speed]);
>
> Bjorn
Yes, this is working fine. Will add it in the patch.
Thank you,
Sumit Gupta
© 2016 - 2026 Red Hat, Inc.