* [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
@ 2025-06-11 16:31 Hans Zhang
2025-06-11 18:38 ` Frank Li
2025-06-11 18:43 ` Frank Li
0 siblings, 2 replies; 7+ messages in thread
From: Hans Zhang @ 2025-06-11 16:31 UTC (permalink / raw)
To: lpieralisi, bhelgaas, mani, kwilczynski
Cc: robh, jingoohan1, linux-pci, linux-kernel, Hans Zhang
i.MX6 PCIe driver contains multiple read-modify-write sequences for
link training and speed configuration. These operations manually handle
bit masking and shifting to update specific fields in control registers,
particularly for link capabilities and speed change initiation.
Refactor link capability configuration and speed change handling using
dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
by encapsulating bit clear/set operations and eliminates intermediate
variables. For speed change control, replace explicit bit manipulation
with direct register updates through the helper.
Adopting the standard interface reduces code complexity in link training
paths and ensures consistent handling of speed-related bits. The change
also prepares the driver for future enhancements to Gen3 link training
by centralizing bit manipulation logic.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5a38cfaf989b..3004e432f013 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
struct imx_pcie *imx_pcie = to_imx_pcie(pci);
struct device *dev = pci->dev;
u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
- u32 tmp;
int ret;
if (!(imx_pcie->drvdata->flags &
@@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
* bus will not be detected at all. This happens with PCIe switches.
*/
dw_pcie_dbi_ro_wr_en(pci);
- tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
- tmp &= ~PCI_EXP_LNKCAP_SLS;
- tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
- dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+ dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
+ PCI_EXP_LNKCAP_SLS,
+ PCI_EXP_LNKCAP_SLS_2_5GB);
dw_pcie_dbi_ro_wr_dis(pci);
/* Start LTSSM. */
@@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
/* Allow faster modes after the link is up */
dw_pcie_dbi_ro_wr_en(pci);
- tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
- tmp &= ~PCI_EXP_LNKCAP_SLS;
- tmp |= pci->max_link_speed;
- dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+ dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
+ PCI_EXP_LNKCAP_SLS,
+ pci->max_link_speed);
/*
* Start Directed Speed Change so the best possible
* speed both link partners support can be negotiated.
*/
- tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
- tmp |= PORT_LOGIC_SPEED_CHANGE;
- dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
+ dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
+ 0, PORT_LOGIC_SPEED_CHANGE);
dw_pcie_dbi_ro_wr_dis(pci);
ret = imx_pcie_wait_for_speed_change(imx_pcie);
@@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
struct imx_pcie *imx_pcie = to_imx_pcie(pci);
- u32 val;
if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
/*
@@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
* to 0.
*/
dw_pcie_dbi_ro_wr_en(pci);
- val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
- val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
- dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
+ dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
+ GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
dw_pcie_dbi_ro_wr_dis(pci);
}
}
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 16:31 [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword() Hans Zhang
@ 2025-06-11 18:38 ` Frank Li
2025-06-11 18:45 ` Frank Li
2025-06-12 0:46 ` Hans Zhang
2025-06-11 18:43 ` Frank Li
1 sibling, 2 replies; 7+ messages in thread
From: Frank Li @ 2025-06-11 18:38 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
Subject should be
PCI: dwc: imx6: Refactor code by using dw_pcie_clear_and_set_dword()
tag "imx6:" should after "dwc:"
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> i.MX6 PCIe driver contains multiple read-modify-write sequences for
> link training and speed configuration. These operations manually handle
> bit masking and shifting to update specific fields in control registers,
> particularly for link capabilities and speed change initiation.
>
> Refactor link capability configuration and speed change handling using
> dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
> by encapsulating bit clear/set operations and eliminates intermediate
> variables. For speed change control, replace explicit bit manipulation
> with direct register updates through the helper.
>
> Adopting the standard interface reduces code complexity in link training
> paths and ensures consistent handling of speed-related bits. The change
> also prepares the driver for future enhancements to Gen3 link training
> by centralizing bit manipulation logic.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 5a38cfaf989b..3004e432f013 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> struct device *dev = pci->dev;
> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> - u32 tmp;
> int ret;
>
> if (!(imx_pcie->drvdata->flags &
> @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> * bus will not be detected at all. This happens with PCIe switches.
> */
> dw_pcie_dbi_ro_wr_en(pci);
> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> - tmp &= ~PCI_EXP_LNKCAP_SLS;
> - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> + PCI_EXP_LNKCAP_SLS,
> + PCI_EXP_LNKCAP_SLS_2_5GB);
> dw_pcie_dbi_ro_wr_dis(pci);
>
> /* Start LTSSM. */
> @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>
> /* Allow faster modes after the link is up */
> dw_pcie_dbi_ro_wr_en(pci);
> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> - tmp &= ~PCI_EXP_LNKCAP_SLS;
> - tmp |= pci->max_link_speed;
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> + PCI_EXP_LNKCAP_SLS,
> + pci->max_link_speed);
>
> /*
> * Start Directed Speed Change so the best possible
> * speed both link partners support can be negotiated.
> */
> - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> - tmp |= PORT_LOGIC_SPEED_CHANGE;
> - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
> + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
> + 0, PORT_LOGIC_SPEED_CHANGE);
> dw_pcie_dbi_ro_wr_dis(pci);
>
> ret = imx_pcie_wait_for_speed_change(imx_pcie);
> @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> - u32 val;
>
> if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
> /*
> @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> * to 0.
> */
> dw_pcie_dbi_ro_wr_en(pci);
> - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
> + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
> + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
> dw_pcie_dbi_ro_wr_dis(pci);
> }
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 18:38 ` Frank Li
@ 2025-06-11 18:45 ` Frank Li
2025-06-12 0:50 ` Hans Zhang
2025-06-12 0:46 ` Hans Zhang
1 sibling, 1 reply; 7+ messages in thread
From: Frank Li @ 2025-06-11 18:45 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On Wed, Jun 11, 2025 at 02:38:14PM -0400, Frank Li wrote:
> On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
>
> Subject should be
>
> PCI: dwc: imx6: Refactor code by using dw_pcie_clear_and_set_dword()
>
> tag "imx6:" should after "dwc:"
>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
I sent out too quick, please don't applied my review tag at next version
because I found a issue, see:
https://lore.kernel.org/linux-pci/aEnONwJUSEMdMAUD@lizhi-Precision-Tower-5810/
Frank
>
> > i.MX6 PCIe driver contains multiple read-modify-write sequences for
> > link training and speed configuration. These operations manually handle
> > bit masking and shifting to update specific fields in control registers,
> > particularly for link capabilities and speed change initiation.
> >
> > Refactor link capability configuration and speed change handling using
> > dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
> > by encapsulating bit clear/set operations and eliminates intermediate
> > variables. For speed change control, replace explicit bit manipulation
> > with direct register updates through the helper.
> >
> > Adopting the standard interface reduces code complexity in link training
> > paths and ensures consistent handling of speed-related bits. The change
> > also prepares the driver for future enhancements to Gen3 link training
> > by centralizing bit manipulation logic.
> >
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
> > 1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 5a38cfaf989b..3004e432f013 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> > struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> > struct device *dev = pci->dev;
> > u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > - u32 tmp;
> > int ret;
> >
> > if (!(imx_pcie->drvdata->flags &
> > @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> > * bus will not be detected at all. This happens with PCIe switches.
> > */
> > dw_pcie_dbi_ro_wr_en(pci);
> > - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> > - tmp &= ~PCI_EXP_LNKCAP_SLS;
> > - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> > - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> > + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> > + PCI_EXP_LNKCAP_SLS,
> > + PCI_EXP_LNKCAP_SLS_2_5GB);
> > dw_pcie_dbi_ro_wr_dis(pci);
> >
> > /* Start LTSSM. */
> > @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> >
> > /* Allow faster modes after the link is up */
> > dw_pcie_dbi_ro_wr_en(pci);
> > - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> > - tmp &= ~PCI_EXP_LNKCAP_SLS;
> > - tmp |= pci->max_link_speed;
> > - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> > + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> > + PCI_EXP_LNKCAP_SLS,
> > + pci->max_link_speed);
> >
> > /*
> > * Start Directed Speed Change so the best possible
> > * speed both link partners support can be negotiated.
> > */
> > - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > - tmp |= PORT_LOGIC_SPEED_CHANGE;
> > - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
> > + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
> > + 0, PORT_LOGIC_SPEED_CHANGE);
> > dw_pcie_dbi_ro_wr_dis(pci);
> >
> > ret = imx_pcie_wait_for_speed_change(imx_pcie);
> > @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> > {
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> > - u32 val;
> >
> > if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
> > /*
> > @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> > * to 0.
> > */
> > dw_pcie_dbi_ro_wr_en(pci);
> > - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> > - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> > - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
> > + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
> > + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
> > dw_pcie_dbi_ro_wr_dis(pci);
> > }
> > }
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 18:45 ` Frank Li
@ 2025-06-12 0:50 ` Hans Zhang
0 siblings, 0 replies; 7+ messages in thread
From: Hans Zhang @ 2025-06-12 0:50 UTC (permalink / raw)
To: Frank Li
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On 2025/6/12 02:45, Frank Li wrote:
> On Wed, Jun 11, 2025 at 02:38:14PM -0400, Frank Li wrote:
>> On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
>>
>> Subject should be
>>
>> PCI: dwc: imx6: Refactor code by using dw_pcie_clear_and_set_dword()
>>
>> tag "imx6:" should after "dwc:"
>>
>> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> I sent out too quick, please don't applied my review tag at next version
> because I found a issue, see:
>
> https://lore.kernel.org/linux-pci/aEnONwJUSEMdMAUD@lizhi-Precision-Tower-5810/
>
Dear Frank,
Thank you very much for your reply.
I have replied to you in the previous email. Bjorn also raised a
question as to why the entire series is not under
0000-cover-letter.patch. I will reply to Bjorn in the email and try to
solve this problem.
Best regards,
Hans
> Frank
>>
>>> i.MX6 PCIe driver contains multiple read-modify-write sequences for
>>> link training and speed configuration. These operations manually handle
>>> bit masking and shifting to update specific fields in control registers,
>>> particularly for link capabilities and speed change initiation.
>>>
>>> Refactor link capability configuration and speed change handling using
>>> dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
>>> by encapsulating bit clear/set operations and eliminates intermediate
>>> variables. For speed change control, replace explicit bit manipulation
>>> with direct register updates through the helper.
>>>
>>> Adopting the standard interface reduces code complexity in link training
>>> paths and ensures consistent handling of speed-related bits. The change
>>> also prepares the driver for future enhancements to Gen3 link training
>>> by centralizing bit manipulation logic.
>>>
>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>> ---
>>> drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
>>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
>>> index 5a38cfaf989b..3004e432f013 100644
>>> --- a/drivers/pci/controller/dwc/pci-imx6.c
>>> +++ b/drivers/pci/controller/dwc/pci-imx6.c
>>> @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>>> struct device *dev = pci->dev;
>>> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>> - u32 tmp;
>>> int ret;
>>>
>>> if (!(imx_pcie->drvdata->flags &
>>> @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>>> * bus will not be detected at all. This happens with PCIe switches.
>>> */
>>> dw_pcie_dbi_ro_wr_en(pci);
>>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>>> - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>>> + PCI_EXP_LNKCAP_SLS,
>>> + PCI_EXP_LNKCAP_SLS_2_5GB);
>>> dw_pcie_dbi_ro_wr_dis(pci);
>>>
>>> /* Start LTSSM. */
>>> @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>>>
>>> /* Allow faster modes after the link is up */
>>> dw_pcie_dbi_ro_wr_en(pci);
>>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>>> - tmp |= pci->max_link_speed;
>>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>>> + PCI_EXP_LNKCAP_SLS,
>>> + pci->max_link_speed);
>>>
>>> /*
>>> * Start Directed Speed Change so the best possible
>>> * speed both link partners support can be negotiated.
>>> */
>>> - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>>> - tmp |= PORT_LOGIC_SPEED_CHANGE;
>>> - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>>> + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
>>> + 0, PORT_LOGIC_SPEED_CHANGE);
>>> dw_pcie_dbi_ro_wr_dis(pci);
>>>
>>> ret = imx_pcie_wait_for_speed_change(imx_pcie);
>>> @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>>> {
>>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>>> - u32 val;
>>>
>>> if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
>>> /*
>>> @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>>> * to 0.
>>> */
>>> dw_pcie_dbi_ro_wr_en(pci);
>>> - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>>> - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>>> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
>>> + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
>>> + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
>>> dw_pcie_dbi_ro_wr_dis(pci);
>>> }
>>> }
>>> --
>>> 2.25.1
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 18:38 ` Frank Li
2025-06-11 18:45 ` Frank Li
@ 2025-06-12 0:46 ` Hans Zhang
1 sibling, 0 replies; 7+ messages in thread
From: Hans Zhang @ 2025-06-12 0:46 UTC (permalink / raw)
To: Frank Li
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On 2025/6/12 02:38, Frank Li wrote:
> On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
>
> Subject should be
>
> PCI: dwc: imx6: Refactor code by using dw_pcie_clear_and_set_dword()
>
> tag "imx6:" should after "dwc:"
>
Dear Frank,
Thank you very much for your reply and suggestions. Will change.
Best regards,
Hans
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
>> i.MX6 PCIe driver contains multiple read-modify-write sequences for
>> link training and speed configuration. These operations manually handle
>> bit masking and shifting to update specific fields in control registers,
>> particularly for link capabilities and speed change initiation.
>>
>> Refactor link capability configuration and speed change handling using
>> dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
>> by encapsulating bit clear/set operations and eliminates intermediate
>> variables. For speed change control, replace explicit bit manipulation
>> with direct register updates through the helper.
>>
>> Adopting the standard interface reduces code complexity in link training
>> paths and ensures consistent handling of speed-related bits. The change
>> also prepares the driver for future enhancements to Gen3 link training
>> by centralizing bit manipulation logic.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
>> index 5a38cfaf989b..3004e432f013 100644
>> --- a/drivers/pci/controller/dwc/pci-imx6.c
>> +++ b/drivers/pci/controller/dwc/pci-imx6.c
>> @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>> struct device *dev = pci->dev;
>> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> - u32 tmp;
>> int ret;
>>
>> if (!(imx_pcie->drvdata->flags &
>> @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>> * bus will not be detected at all. This happens with PCIe switches.
>> */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>> - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>> + PCI_EXP_LNKCAP_SLS,
>> + PCI_EXP_LNKCAP_SLS_2_5GB);
>> dw_pcie_dbi_ro_wr_dis(pci);
>>
>> /* Start LTSSM. */
>> @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>>
>> /* Allow faster modes after the link is up */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>> - tmp |= pci->max_link_speed;
>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>> + PCI_EXP_LNKCAP_SLS,
>> + pci->max_link_speed);
>>
>> /*
>> * Start Directed Speed Change so the best possible
>> * speed both link partners support can be negotiated.
>> */
>> - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>> - tmp |= PORT_LOGIC_SPEED_CHANGE;
>> - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>> + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
>> + 0, PORT_LOGIC_SPEED_CHANGE);
>> dw_pcie_dbi_ro_wr_dis(pci);
>>
>> ret = imx_pcie_wait_for_speed_change(imx_pcie);
>> @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>> - u32 val;
>>
>> if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
>> /*
>> @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>> * to 0.
>> */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>> - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
>> + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
>> + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
>> dw_pcie_dbi_ro_wr_dis(pci);
>> }
>> }
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 16:31 [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword() Hans Zhang
2025-06-11 18:38 ` Frank Li
@ 2025-06-11 18:43 ` Frank Li
2025-06-12 0:47 ` Hans Zhang
1 sibling, 1 reply; 7+ messages in thread
From: Frank Li @ 2025-06-11 18:43 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
> i.MX6 PCIe driver contains multiple read-modify-write sequences for
> link training and speed configuration. These operations manually handle
> bit masking and shifting to update specific fields in control registers,
> particularly for link capabilities and speed change initiation.
>
> Refactor link capability configuration and speed change handling using
> dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
> by encapsulating bit clear/set operations and eliminates intermediate
> variables. For speed change control, replace explicit bit manipulation
> with direct register updates through the helper.
>
> Adopting the standard interface reduces code complexity in link training
> paths and ensures consistent handling of speed-related bits. The change
> also prepares the driver for future enhancements to Gen3 link training
> by centralizing bit manipulation logic.
>
Sorry, where define dw_pcie_clear_and_set_dword() ?
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 5a38cfaf989b..3004e432f013 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> struct device *dev = pci->dev;
> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> - u32 tmp;
> int ret;
>
> if (!(imx_pcie->drvdata->flags &
> @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
> * bus will not be detected at all. This happens with PCIe switches.
> */
> dw_pcie_dbi_ro_wr_en(pci);
> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> - tmp &= ~PCI_EXP_LNKCAP_SLS;
> - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> + PCI_EXP_LNKCAP_SLS,
> + PCI_EXP_LNKCAP_SLS_2_5GB);
> dw_pcie_dbi_ro_wr_dis(pci);
>
> /* Start LTSSM. */
> @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>
> /* Allow faster modes after the link is up */
> dw_pcie_dbi_ro_wr_en(pci);
> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> - tmp &= ~PCI_EXP_LNKCAP_SLS;
> - tmp |= pci->max_link_speed;
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
> + PCI_EXP_LNKCAP_SLS,
> + pci->max_link_speed);
>
> /*
> * Start Directed Speed Change so the best possible
> * speed both link partners support can be negotiated.
> */
> - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> - tmp |= PORT_LOGIC_SPEED_CHANGE;
> - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
> + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
> + 0, PORT_LOGIC_SPEED_CHANGE);
supposed 3 args is mask. 0 should be PORT_LOGIC_SPEED_CHANGE.
Frank
> dw_pcie_dbi_ro_wr_dis(pci);
>
> ret = imx_pcie_wait_for_speed_change(imx_pcie);
> @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
> - u32 val;
>
> if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
> /*
> @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
> * to 0.
> */
> dw_pcie_dbi_ro_wr_en(pci);
> - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
> + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
> + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
> dw_pcie_dbi_ro_wr_dis(pci);
> }
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword()
2025-06-11 18:43 ` Frank Li
@ 2025-06-12 0:47 ` Hans Zhang
0 siblings, 0 replies; 7+ messages in thread
From: Hans Zhang @ 2025-06-12 0:47 UTC (permalink / raw)
To: Frank Li
Cc: lpieralisi, bhelgaas, mani, kwilczynski, robh, jingoohan1,
linux-pci, linux-kernel
On 2025/6/12 02:43, Frank Li wrote:
> On Thu, Jun 12, 2025 at 12:31:21AM +0800, Hans Zhang wrote:
>> i.MX6 PCIe driver contains multiple read-modify-write sequences for
>> link training and speed configuration. These operations manually handle
>> bit masking and shifting to update specific fields in control registers,
>> particularly for link capabilities and speed change initiation.
>>
>> Refactor link capability configuration and speed change handling using
>> dw_pcie_clear_and_set_dword(). The helper simplifies LNKCAP modification
>> by encapsulating bit clear/set operations and eliminates intermediate
>> variables. For speed change control, replace explicit bit manipulation
>> with direct register updates through the helper.
>>
>> Adopting the standard interface reduces code complexity in link training
>> paths and ensures consistent handling of speed-related bits. The change
>> also prepares the driver for future enhancements to Gen3 link training
>> by centralizing bit manipulation logic.
>>
>
> Sorry, where define dw_pcie_clear_and_set_dword() ?
>
Dear Frank,
Thank you very much for your reply.
Please see the following link:
https://patchwork.kernel.org/project/linux-pci/patch/20250611163057.860353-1-18255117159@163.com/
Best regards,
Hans
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/controller/dwc/pci-imx6.c | 26 ++++++++++----------------
>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
>> index 5a38cfaf989b..3004e432f013 100644
>> --- a/drivers/pci/controller/dwc/pci-imx6.c
>> +++ b/drivers/pci/controller/dwc/pci-imx6.c
>> @@ -941,7 +941,6 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>> struct device *dev = pci->dev;
>> u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> - u32 tmp;
>> int ret;
>>
>> if (!(imx_pcie->drvdata->flags &
>> @@ -956,10 +955,9 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>> * bus will not be detected at all. This happens with PCIe switches.
>> */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>> - tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>> + PCI_EXP_LNKCAP_SLS,
>> + PCI_EXP_LNKCAP_SLS_2_5GB);
>> dw_pcie_dbi_ro_wr_dis(pci);
>>
>> /* Start LTSSM. */
>> @@ -972,18 +970,16 @@ static int imx_pcie_start_link(struct dw_pcie *pci)
>>
>> /* Allow faster modes after the link is up */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
>> - tmp &= ~PCI_EXP_LNKCAP_SLS;
>> - tmp |= pci->max_link_speed;
>> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
>> + dw_pcie_clear_and_set_dword(pci, offset + PCI_EXP_LNKCAP,
>> + PCI_EXP_LNKCAP_SLS,
>> + pci->max_link_speed);
>>
>> /*
>> * Start Directed Speed Change so the best possible
>> * speed both link partners support can be negotiated.
>> */
>> - tmp = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>> - tmp |= PORT_LOGIC_SPEED_CHANGE;
>> - dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
>> + dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
>> + 0, PORT_LOGIC_SPEED_CHANGE);
>
> supposed 3 args is mask. 0 should be PORT_LOGIC_SPEED_CHANGE.
>
> Frank
>
>> dw_pcie_dbi_ro_wr_dis(pci);
>>
>> ret = imx_pcie_wait_for_speed_change(imx_pcie);
>> @@ -1295,7 +1291,6 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> struct imx_pcie *imx_pcie = to_imx_pcie(pci);
>> - u32 val;
>>
>> if (imx_pcie->drvdata->flags & IMX_PCIE_FLAG_8GT_ECN_ERR051586) {
>> /*
>> @@ -1310,9 +1305,8 @@ static void imx_pcie_host_post_init(struct dw_pcie_rp *pp)
>> * to 0.
>> */
>> dw_pcie_dbi_ro_wr_en(pci);
>> - val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
>> - val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
>> - dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
>> + dw_pcie_clear_and_set_dword(pci, GEN3_RELATED_OFF,
>> + GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL, 0);
>> dw_pcie_dbi_ro_wr_dis(pci);
>> }
>> }
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-12 0:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-11 16:31 [PATCH 04/13] PCI: dwc: Refactor imx6 to use dw_pcie_clear_and_set_dword() Hans Zhang
2025-06-11 18:38 ` Frank Li
2025-06-11 18:45 ` Frank Li
2025-06-12 0:50 ` Hans Zhang
2025-06-12 0:46 ` Hans Zhang
2025-06-11 18:43 ` Frank Li
2025-06-12 0:47 ` Hans Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®