Re: [PATCH v8 6/9] PCI: dw-rockchip: Add helper function for DDL indicator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI: dw-rockchip: rockchip_pcie_unmask_dll_indicator() naming and single use
--

commit 02422c1fdad3696875f51d4d19c596b663cc9a6c
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Add helper function for DDL indicator

This commit extracts the PCIE_CLIENT_INTR_MASK_MISC register write in
rockchip_pcie_configure_ep() into a new rockchip_pcie_unmask_dll_indicator()
helper function.

>     PCI: dw-rockchip: Add helper function for DDL indicator
>     
>     Remove code duplication and improve readability by introducing a new
>     function to setup the DLL indicator.
>     
>     Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
>     Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index c6a4649dae2e8..0e26abc509205 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -700,6 +700,16 @@ static void rockchip_pcie_set_controller_mode(struct rockchip_pcie *rockchip, u3
>  	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_SET_MODE(mode), PCIE_CLIENT_GENERAL_CON);
>  }
>  
> +static void rockchip_pcie_unmask_dll_indicator(struct rockchip_pcie *rockchip)
> +{
> +	u32 val;
> +
> +	/* unmask DLL up/down indicator and hot reset/link-down reset */
> +	val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0) |
> +	      FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
> +	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
> +}
> +
>  static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>  				      struct rockchip_pcie *rockchip)
>  {
> @@ -760,7 +770,6 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
>  {
>  	struct device *dev = &pdev->dev;
>  	int irq, ret;
> -	u32 val;
>  
>  	if (!IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_EP))
>  		return -ENODEV;
> @@ -800,10 +809,7 @@ static int rockchip_pcie_configure_ep(struct platform_device *pdev,
>  
>  	pci_epc_init_notify(rockchip->pci.ep.epc);
>  
> -	/* unmask DLL up/down indicator and hot reset/link-down reset */
> -	val = FIELD_PREP_WM16(PCIE_RDLH_LINK_UP_CHGED, 0) |
> -	      FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
> -	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
> +	rockchip_pcie_unmask_dll_indicator(rockchip);

[Severity: Low]
This isn't a bug, but is the commit message description and helper naming
accurate here?

First, there is a typo in the commit subject ("DDL" instead of "DLL").

Second, the commit message states that this change removes code duplication,
but rockchip_pcie_configure_ep() in
drivers/pci/controller/dwc/pcie-dw-rockchip.c is the only caller of
rockchip_pcie_unmask_dll_indicator().

Third, rockchip_pcie_unmask_dll_indicator() unmasks both the DLL up/down
indicator (PCIE_RDLH_LINK_UP_CHGED) and the hot reset/link-down reset
interrupt (PCIE_LINK_REQ_RST_NOT_INT) in PCIE_CLIENT_INTR_MASK_MISC, which is
not reflected in the helper's name.

Meanwhile, rockchip_pcie_configure_rc() and
rockchip_pcie_rc_reset_root_port() both open-code the write to
PCIE_CLIENT_INTR_MASK_MISC to unmask PCIE_LINK_REQ_RST_NOT_INT:

	/* unmask hot reset/link-down reset */
	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);

Could this helper be parameterized (similar to
rockchip_pcie_enable_enhanced_ltssm_control_mode()) and named to reflect
unmasking miscellaneous interrupts so that the RC paths can also use it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-rockchip-pcie-system-suspend-v8-0-0434b4f373bc@collabora.com?part=6