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

From: Rob Herring <robh@kernel.org>
To: Jim Quinlan <james.quinlan@broadcom.com>
Cc: linux-pci@vger.kernel.org,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Christoph Hellwig <hch@lst.de>,
	Robin Murphy <robin.murphy@arm.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	<linux-rpi-kernel@lists.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v11 05/11] PCI: brcmstb: Add bcm7278 PERST# support
Date: Thu, 10 Sep 2020 10:04:11 -0600	[thread overview]
Message-ID: <20200910160411.GA439527@bogus> (raw)
In-Reply-To: <20200824193036.6033-6-james.quinlan@broadcom.com>

On Mon, Aug 24, 2020 at 03:30:18PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> 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@broadcom.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  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
> 