Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.

>  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>  {
> -	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> -	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +	if (val)
> +		reset_control_assert(pcie->bridge_reset);
> +	else
> +		reset_control_deassert(pcie->bridge_reset);
>  
> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> -	tmp = (tmp & ~mask) | ((val << shift) & mask);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	if (!pcie->bridge_reset) {
> +		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> +		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +
> +		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +		tmp = (tmp & ~mask) | ((val << shift) & mask);
> +		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	}

This pattern looks goofy:

  reset_control_assert(pcie->bridge_reset);
  if (!pcie->bridge_reset) {
    ...

If we're going to test pcie->bridge_reset at all, it should be first
so it's obvious what's going on and the reader doesn't have to go
verify that reset_control_assert() ignores and returns success for a
NULL pointer:

  if (pcie->bridge_reset) {
    if (val)
      reset_control_assert(pcie->bridge_reset);
    else
      reset_control_deassert(pcie->bridge_reset);

    return;
  }

  u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
  ...

Krzysztof, can you amend this on the branch?

It will also make the eventual return checking and error message
simpler because we won't have to initialize "ret" first, and we can
"return 0" directly for the legacy case.

Bjorn