Re: [PATCH v2 13/13] PCI: tegra194: Refactor code by using dw_pcie_clear_and_set_dword()

[Hans Zhang]

On 2025/6/26 04:57, Manivannan Sadhasivam wrote:
> On Wed, Jun 18, 2025 at 11:21:12PM +0800, Hans Zhang wrote:
> > Tegra194 PCIe driver contains extensive manual bit manipulation across
> > interrupt handling, ASPM configuration, and controller initialization.
> > The driver implements complex read-modify-write sequences with explicit
> > bit masking, leading to verbose and hard-to-maintain code.
> >
> > Refactor interrupt handling, ASPM setup, capability configuration, and
> > controller initialization using dw_pcie_clear_and_set_dword(). Replace
> > multi-step register modifications with single helper calls, eliminating
> > intermediate variables and reducing code size by ~100 lines. For CDMA
> > error handling, initialize the value variable to zero before setting
> > status bits.
> >
> > This comprehensive refactoring significantly improves code readability
> > and maintainability. Standardizing on the helper ensures consistent
> > register access patterns across all driver components and reduces the
> > risk of bit manipulation errors in this complex controller driver.
> >
> > Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-tegra194.c | 155 +++++++++------------
> >   1 file changed, 64 insertions(+), 91 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index 4f26086f25da..c6f5c35a4be4 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -378,9 +378,8 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
> >   			val |= APPL_CAR_RESET_OVRD_CYA_OVERRIDE_CORE_RST_N;
> >   			appl_writel(pcie, val, APPL_CAR_RESET_OVRD);
> >   
> > -			val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
> > -			val |= PORT_LOGIC_SPEED_CHANGE;
> > -			dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, val);
> > +			dw_pcie_clear_and_set_dword(pci, PCIE_LINK_WIDTH_SPEED_CONTROL,
> > +						    0, PORT_LOGIC_SPEED_CHANGE);
> >   		}
> >   	}
> >   
> > @@ -412,7 +411,7 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
> >   
> >   	if (status_l0 & APPL_INTR_STATUS_L0_CDM_REG_CHK_INT) {
> >   		status_l1 = appl_readl(pcie, APPL_INTR_STATUS_L1_18);
> > -		val = dw_pcie_readl_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS);
> > +		val = 0;
> >   		if (status_l1 & APPL_INTR_STATUS_L1_18_CDM_REG_CHK_CMPLT) {
> >   			dev_info(pci->dev, "CDM check complete\n");
> >   			val |= PCIE_PL_CHK_REG_CHK_REG_COMPLETE;
> > @@ -425,7 +424,8 @@ static irqreturn_t tegra_pcie_rp_irq_handler(int irq, void *arg)
> >   			dev_err(pci->dev, "CDM Logic error\n");
> >   			val |= PCIE_PL_CHK_REG_CHK_REG_LOGIC_ERROR;
> >   		}
> > -		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
> > +		dw_pcie_clear_and_set_dword(pci, PCIE_PL_CHK_REG_CONTROL_STATUS,
> > +					    PORT_LOGIC_SPEED_CHANGE, val);
>
> I don't know why you are clearing PORT_LOGIC_SPEED_CHANGE here which is not part
> of PCIE_PL_CHK_REG_CONTROL_STATUS. Typo?
Dear Mani,

Thank you very much for your reply and reminder.

This is copied from the first change of this patch and will be fixed in 
the next version.

Best regards,
Hans


> - Mani