Re: [PATCH v11 05/11] PCI: brcmstb: Add bcm7278 PERST# support

[Rob Herring]

On Mon, Aug 24, 2020 at 03:30:18PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> 
> The PERST# bit was moved to a different register in 7278-type STB chips.
> In addition, the polarity of the bit was also changed; for other chips
> writing a 1 specified assert; for 7278-type chips, writing a 0 specifies
> assert.
> 
> Of course, PERST# is a PCIe asserted-low signal.
> 
> Signed-off-by: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> Acked-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 3d588ab7a6dd..acf2239b0251 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -83,6 +83,7 @@
>  
>  #define PCIE_MISC_PCIE_CTRL				0x4064
>  #define  PCIE_MISC_PCIE_CTRL_PCIE_L23_REQUEST_MASK	0x1
> +#define PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK		0x4
>  
>  #define PCIE_MISC_PCIE_STATUS				0x4068
>  #define  PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK		0x80
> @@ -684,9 +685,16 @@ static inline void brcm_pcie_perst_set(struct brcm_pcie *pcie, u32 val)
>  {
>  	u32 tmp;
>  
> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> -	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	if (pcie->type == BCM7278) {
> +		/* Perst bit has moved and assert value is 0 */
> +		tmp = readl(pcie->base + PCIE_MISC_PCIE_CTRL);
> +		u32p_replace_bits(&tmp, !val, PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK);
> +		writel(tmp, pcie->base +  PCIE_MISC_PCIE_CTRL);
> +	} else {
> +		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +		u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
> +		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));

Humm, now we have a mixture of a code path based on the chip and 
variables to abstract the register details. Just do a function per chip.

I have some notion to abstract out the PERST# handling from the host 
bridges. We have several cases of GPIO based handling and random 
assertion times. So having an ops function here will move in that 
direction.

> +	}
>  }
>  
>  static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> @@ -771,7 +779,10 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  
>  	/* Reset the bridge */
>  	brcm_pcie_bridge_sw_init_set(pcie, 1);
> -	brcm_pcie_perst_set(pcie, 1);

If these 2 functions are always called together, then you just need 1 
per chip function.

> +
> +	/* BCM7278 fails when PERST# is set here */
> +	if (pcie->type != BCM7278)
> +		brcm_pcie_perst_set(pcie, 1);
>  
>  	usleep_range(100, 200);
>  
> -- 
> 2.17.1
>