diff mbox series

[04/15] PCI: brcmstb: Add compatibily of other chips

Message ID 20200519203419.12369-5-james.quinlan@broadcom.com (mailing list archive)
State New, archived
Headers show
Series PCI: brcmstb: enable PCIe for STB chips | expand

Commit Message

Jim Quinlan May 19, 2020, 8:34 p.m. UTC 
From: Jim Quinlan <jquinlan@broadcom.com>

Add in compatibility strings and code for three Broadcom STB chips.
Some of the register locations, shifts, and masks are different
for certain chips, requiring the use of different constants based
on of_id.

We would like to add the following at this time to the match list
but we need to wait until the end of this patchset so that
everything works.

    { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
    { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
    { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
    { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
 1 file changed, 91 insertions(+), 12 deletions(-)

Comments

Nicolas Saenz Julienne May 20, 2020, 11:51 a.m. UTC | #1 
Hi Jim,

On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> Add in compatibility strings and code for three Broadcom STB chips.
> Some of the register locations, shifts, and masks are different
> for certain chips, requiring the use of different constants based
> on of_id.
> 
> We would like to add the following at this time to the match list
> but we need to wait until the end of this patchset so that
> everything works.
> 
>     { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
>     { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
>     { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
>     { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
>  1 file changed, 91 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index 73020b4ff090..c1cf4ea7d3d9 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -120,9 +120,8 @@
>  #define  PCIE_EXT_SLOT_SHIFT				15
>  #define  PCIE_EXT_FUNC_SHIFT				12
>  
> -#define PCIE_RGR1_SW_INIT_1				0x9210
>  #define  PCIE_RGR1_SW_INIT_1_PERST_MASK			0x1
> -#define  PCIE_RGR1_SW_INIT_1_INIT_MASK			0x2
> +#define  PCIE_RGR1_SW_INIT_1_PERST_SHIFT		0x0
>  
>  /* PCIe parameters */
>  #define BRCM_NUM_PCIE_OUT_WINS		0x4
> @@ -152,6 +151,69 @@
>  #define SSC_STATUS_SSC_MASK		0x400
>  #define SSC_STATUS_PLL_LOCK_MASK	0x800
>  
> +#define IDX_ADDR(pcie)	\
> +	(pcie->reg_offsets[EXT_CFG_INDEX])
> +#define DATA_ADDR(pcie)	\
> +	(pcie->reg_offsets[EXT_CFG_DATA])
> +#define PCIE_RGR1_SW_INIT_1(pcie) \
> +	(pcie->reg_offsets[RGR1_SW_INIT_1])
> +
> +enum {
> +	RGR1_SW_INIT_1,
> +	EXT_CFG_INDEX,
> +	EXT_CFG_DATA,
> +};
> +
> +enum {
> +	RGR1_SW_INIT_1_INIT_MASK,
> +	RGR1_SW_INIT_1_INIT_SHIFT,
> +};
> +
> +enum pcie_type {
> +	GENERIC,
> +	BCM7278,
> +};
> +
> +struct pcie_cfg_data {
> +	const int *reg_field_info;
> +	const int *offsets;
> +	const enum pcie_type type;
> +};
> +
> +static const int pcie_reg_field_info[] = {
> +	[RGR1_SW_INIT_1_INIT_MASK] = 0x2,
> +	[RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
> +};
> +
> +static const int pcie_reg_field_info_bcm7278[] = {
> +	[RGR1_SW_INIT_1_INIT_MASK] = 0x1,
> +	[RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
> +};
> +
> +static const int pcie_offsets[] = {
> +	[RGR1_SW_INIT_1] = 0x9210,
> +	[EXT_CFG_INDEX]  = 0x9000,
> +	[EXT_CFG_DATA]   = 0x9004,
> +};
> +
> +static const struct pcie_cfg_data generic_cfg = {
> +	.reg_field_info	= pcie_reg_field_info,
> +	.offsets	= pcie_offsets,
> +	.type		= GENERIC,
> +};
> +
> +static const int pcie_offset_bcm7278[] = {
> +	[RGR1_SW_INIT_1] = 0xc010,
> +	[EXT_CFG_INDEX] = 0x9000,
> +	[EXT_CFG_DATA] = 0x9004,
> +};
> +
> +static const struct pcie_cfg_data bcm7278_cfg = {
> +	.reg_field_info = pcie_reg_field_info_bcm7278,
> +	.offsets	= pcie_offset_bcm7278,
> +	.type		= BCM7278,
> +};

It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
specific structures above, and moving them to patch #15. This will keep a
clearer division between the patch introducing the infrastructure and the one
adding the support for a new device.

> +
>  struct brcm_msi {
>  	struct device		*dev;
>  	void __iomem		*base;
> @@ -176,6 +238,9 @@ struct brcm_pcie {
>  	int			gen;
>  	u64			msi_target_addr;
>  	struct brcm_msi		*msi;
> +	const int		*reg_offsets;
> +	const int		*reg_field_info;
> +	enum pcie_type		type;
>  };
>  
>  /*
> @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
>  
>  static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32
> val)
>  {
> -	u32 tmp;
> +	u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> +	u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];

I don't think you need shift here, IIUC u32p_replace_bits() will take care of
all the masking and shifting internally, moreover, you'd be able to drop the
shift entry from reg_field_info.

> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> -	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> +	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));
>  }

Regards,
Nicolas
Jim Quinlan May 20, 2020, 2:30 p.m. UTC | #2 
On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Jim,
>
> On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@broadcom.com>
> >
> > Add in compatibility strings and code for three Broadcom STB chips.
> > Some of the register locations, shifts, and masks are different
> > for certain chips, requiring the use of different constants based
> > on of_id.
> >
> > We would like to add the following at this time to the match list
> > but we need to wait until the end of this patchset so that
> > everything works.
> >
> >     { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> >     { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> >     { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> >     { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> >
> > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
> >  1 file changed, 91 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index 73020b4ff090..c1cf4ea7d3d9 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -120,9 +120,8 @@
> >  #define  PCIE_EXT_SLOT_SHIFT                         15
> >  #define  PCIE_EXT_FUNC_SHIFT                         12
> >
> > -#define PCIE_RGR1_SW_INIT_1                          0x9210
> >  #define  PCIE_RGR1_SW_INIT_1_PERST_MASK                      0x1
> > -#define  PCIE_RGR1_SW_INIT_1_INIT_MASK                       0x2
> > +#define  PCIE_RGR1_SW_INIT_1_PERST_SHIFT             0x0
> >
> >  /* PCIe parameters */
> >  #define BRCM_NUM_PCIE_OUT_WINS               0x4
> > @@ -152,6 +151,69 @@
> >  #define SSC_STATUS_SSC_MASK          0x400
> >  #define SSC_STATUS_PLL_LOCK_MASK     0x800
> >
> > +#define IDX_ADDR(pcie)       \
> > +     (pcie->reg_offsets[EXT_CFG_INDEX])
> > +#define DATA_ADDR(pcie)      \
> > +     (pcie->reg_offsets[EXT_CFG_DATA])
> > +#define PCIE_RGR1_SW_INIT_1(pcie) \
> > +     (pcie->reg_offsets[RGR1_SW_INIT_1])
> > +
> > +enum {
> > +     RGR1_SW_INIT_1,
> > +     EXT_CFG_INDEX,
> > +     EXT_CFG_DATA,
> > +};
> > +
> > +enum {
> > +     RGR1_SW_INIT_1_INIT_MASK,
> > +     RGR1_SW_INIT_1_INIT_SHIFT,
> > +};
> > +
> > +enum pcie_type {
> > +     GENERIC,
> > +     BCM7278,
> > +};
> > +
> > +struct pcie_cfg_data {
> > +     const int *reg_field_info;
> > +     const int *offsets;
> > +     const enum pcie_type type;
> > +};
> > +
> > +static const int pcie_reg_field_info[] = {
> > +     [RGR1_SW_INIT_1_INIT_MASK] = 0x2,
> > +     [RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
> > +};
> > +
> > +static const int pcie_reg_field_info_bcm7278[] = {
> > +     [RGR1_SW_INIT_1_INIT_MASK] = 0x1,
> > +     [RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
> > +};
> > +
> > +static const int pcie_offsets[] = {
> > +     [RGR1_SW_INIT_1] = 0x9210,
> > +     [EXT_CFG_INDEX]  = 0x9000,
> > +     [EXT_CFG_DATA]   = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data generic_cfg = {
> > +     .reg_field_info = pcie_reg_field_info,
> > +     .offsets        = pcie_offsets,
> > +     .type           = GENERIC,
> > +};
> > +
> > +static const int pcie_offset_bcm7278[] = {
> > +     [RGR1_SW_INIT_1] = 0xc010,
> > +     [EXT_CFG_INDEX] = 0x9000,
> > +     [EXT_CFG_DATA] = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data bcm7278_cfg = {
> > +     .reg_field_info = pcie_reg_field_info_bcm7278,
> > +     .offsets        = pcie_offset_bcm7278,
> > +     .type           = BCM7278,
> > +};
>
> It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
> specific structures above, and moving them to patch #15. This will keep a
> clearer division between the patch introducing the infrastructure and the one
> adding the support for a new device.
The problem is that one of the commits needs the 7278 type so it has
to be declared earlier.
>
> > +
> >  struct brcm_msi {
> >       struct device           *dev;
> >       void __iomem            *base;
> > @@ -176,6 +238,9 @@ struct brcm_pcie {
> >       int                     gen;
> >       u64                     msi_target_addr;
> >       struct brcm_msi         *msi;
> > +     const int               *reg_offsets;
> > +     const int               *reg_field_info;
> > +     enum pcie_type          type;
> >  };
> >
> >  /*
> > @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
> >
> >  static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32
> > val)
> >  {
> > -     u32 tmp;
> > +     u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> > +     u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
>
> I don't think you need shift here, IIUC u32p_replace_bits() will take care of
> all the masking and shifting internally, moreover, you'd be able to drop the
> shift entry from reg_field_info.
Got it.
>
> > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> > -     u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> > +     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));
> >  }
>
> Regards,
> Nicolas
>
Thanks!
Jim
Nicolas Saenz Julienne May 20, 2020, 2:41 p.m. UTC | #3 
On Wed, 2020-05-20 at 10:30 -0400, Jim Quinlan wrote:
> On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
[...]
> > > +
> > > +static const struct pcie_cfg_data bcm7278_cfg = {
> > > +     .reg_field_info = pcie_reg_field_info_bcm7278,
> > > +     .offsets        = pcie_offset_bcm7278,
> > > +     .type           = BCM7278,
> > > +};
> > 
> > It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
> > specific structures above, and moving them to patch #15. This will keep a
> > clearer division between the patch introducing the infrastructure and the
> > one
> > adding the support for a new device.
> The problem is that one of the commits needs the 7278 type so it has
> to be declared earlier.

Fair enough.

> > > +
> > >  struct brcm_msi {
> > >       struct device           *dev;
> > >       void __iomem            *base;
> > > @@ -176,6 +238,9 @@ struct brcm_pcie {
> > >       int                     gen;
> > >       u64                     msi_target_addr;
> > >       struct brcm_msi         *msi;
> > > +     const int               *reg_offsets;
> > > +     const int               *reg_field_info;
> > > +     enum pcie_type          type;
> > >  };
> > > 
> > >  /*
> > > @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
> > > 
> > >  static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie,
> > > u32
> > > val)
> > >  {
> > > -     u32 tmp;
> > > +     u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> > > +     u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
> > 
> > I don't think you need shift here, IIUC u32p_replace_bits() will take care
> > of
> > all the masking and shifting internally, moreover, you'd be able to drop the
> > shift entry from reg_field_info.
> Got it.
> > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> > > -     u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> > > +     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));
> > >  }
> > 
> > Regards,
> > Nicolas
> > 
> Thanks!
> Jim
Jim Quinlan May 21, 2020, 7:35 p.m. UTC | #4 
On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Jim,
>
> On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@broadcom.com>
> >
> > Add in compatibility strings and code for three Broadcom STB chips.
> > Some of the register locations, shifts, and masks are different
> > for certain chips, requiring the use of different constants based
> > on of_id.
> >
> > We would like to add the following at this time to the match list
> > but we need to wait until the end of this patchset so that
> > everything works.
> >
> >     { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
> >     { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> >     { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> >     { .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
> >
> > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 103 +++++++++++++++++++++++---
> >  1 file changed, 91 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index 73020b4ff090..c1cf4ea7d3d9 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -120,9 +120,8 @@
> >  #define  PCIE_EXT_SLOT_SHIFT                         15
> >  #define  PCIE_EXT_FUNC_SHIFT                         12
> >
> > -#define PCIE_RGR1_SW_INIT_1                          0x9210
> >  #define  PCIE_RGR1_SW_INIT_1_PERST_MASK                      0x1
> > -#define  PCIE_RGR1_SW_INIT_1_INIT_MASK                       0x2
> > +#define  PCIE_RGR1_SW_INIT_1_PERST_SHIFT             0x0
> >
> >  /* PCIe parameters */
> >  #define BRCM_NUM_PCIE_OUT_WINS               0x4
> > @@ -152,6 +151,69 @@
> >  #define SSC_STATUS_SSC_MASK          0x400
> >  #define SSC_STATUS_PLL_LOCK_MASK     0x800
> >
> > +#define IDX_ADDR(pcie)       \
> > +     (pcie->reg_offsets[EXT_CFG_INDEX])
> > +#define DATA_ADDR(pcie)      \
> > +     (pcie->reg_offsets[EXT_CFG_DATA])
> > +#define PCIE_RGR1_SW_INIT_1(pcie) \
> > +     (pcie->reg_offsets[RGR1_SW_INIT_1])
> > +
> > +enum {
> > +     RGR1_SW_INIT_1,
> > +     EXT_CFG_INDEX,
> > +     EXT_CFG_DATA,
> > +};
> > +
> > +enum {
> > +     RGR1_SW_INIT_1_INIT_MASK,
> > +     RGR1_SW_INIT_1_INIT_SHIFT,
> > +};
> > +
> > +enum pcie_type {
> > +     GENERIC,
> > +     BCM7278,
> > +};
> > +
> > +struct pcie_cfg_data {
> > +     const int *reg_field_info;
> > +     const int *offsets;
> > +     const enum pcie_type type;
> > +};
> > +
> > +static const int pcie_reg_field_info[] = {
> > +     [RGR1_SW_INIT_1_INIT_MASK] = 0x2,
> > +     [RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
> > +};
> > +
> > +static const int pcie_reg_field_info_bcm7278[] = {
> > +     [RGR1_SW_INIT_1_INIT_MASK] = 0x1,
> > +     [RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
> > +};
> > +
> > +static const int pcie_offsets[] = {
> > +     [RGR1_SW_INIT_1] = 0x9210,
> > +     [EXT_CFG_INDEX]  = 0x9000,
> > +     [EXT_CFG_DATA]   = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data generic_cfg = {
> > +     .reg_field_info = pcie_reg_field_info,
> > +     .offsets        = pcie_offsets,
> > +     .type           = GENERIC,
> > +};
> > +
> > +static const int pcie_offset_bcm7278[] = {
> > +     [RGR1_SW_INIT_1] = 0xc010,
> > +     [EXT_CFG_INDEX] = 0x9000,
> > +     [EXT_CFG_DATA] = 0x9004,
> > +};
> > +
> > +static const struct pcie_cfg_data bcm7278_cfg = {
> > +     .reg_field_info = pcie_reg_field_info_bcm7278,
> > +     .offsets        = pcie_offset_bcm7278,
> > +     .type           = BCM7278,
> > +};
>
> It's not essential, but if v2 is due I'd suggest factoring out the bcm2728
> specific structures above, and moving them to patch #15. This will keep a
> clearer division between the patch introducing the infrastructure and the one
> adding the support for a new device.
>
> > +
> >  struct brcm_msi {
> >       struct device           *dev;
> >       void __iomem            *base;
> > @@ -176,6 +238,9 @@ struct brcm_pcie {
> >       int                     gen;
> >       u64                     msi_target_addr;
> >       struct brcm_msi         *msi;
> > +     const int               *reg_offsets;
> > +     const int               *reg_field_info;
> > +     enum pcie_type          type;
> >  };
> >
> >  /*
> > @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
> >
> >  static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32
> > val)
> >  {
> > -     u32 tmp;
> > +     u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> > +     u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
>
> I don't think you need shift here, IIUC u32p_replace_bits() will take care of
> all the masking and shifting internally, moreover, you'd be able to drop the
> shift entry from reg_field_info.
I believe that u32p_replace_bits requires at least one of the value or
mask to be compile time constants to work and we don't have that here.
Regards,
Jim
>
> > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
> > -     u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
> > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
> > +     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));
> >  }
>
> Regards,
> Nicolas
>
Nicolas Saenz Julienne May 22, 2020, 9:17 a.m. UTC | #5 
On Thu, 2020-05-21 at 15:35 -0400, Jim Quinlan wrote:
> On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne

[...]

> > >  /*
> > > @@ -602,20 +667,21 @@ static struct pci_ops brcm_pcie_ops = {
> > > 
> > >  static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie,
> > > u32
> > > val)
> > >  {
> > > -     u32 tmp;
> > > +     u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
> > > +     u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
> > 
> > I don't think you need shift here, IIUC u32p_replace_bits() will take care
> > of
> > all the masking and shifting internally, moreover, you'd be able to drop the
> > shift entry from reg_field_info.
> I believe that u32p_replace_bits requires at least one of the value or
> mask to be compile time constants to work and we don't have that here.

Of course, sorry for the noise then.

Regards,
Nicolas
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 73020b4ff090..c1cf4ea7d3d9 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -120,9 +120,8 @@ 
 #define  PCIE_EXT_SLOT_SHIFT				15
 #define  PCIE_EXT_FUNC_SHIFT				12
 
-#define PCIE_RGR1_SW_INIT_1				0x9210
 #define  PCIE_RGR1_SW_INIT_1_PERST_MASK			0x1
-#define  PCIE_RGR1_SW_INIT_1_INIT_MASK			0x2
+#define  PCIE_RGR1_SW_INIT_1_PERST_SHIFT		0x0
 
 /* PCIe parameters */
 #define BRCM_NUM_PCIE_OUT_WINS		0x4
@@ -152,6 +151,69 @@ 
 #define SSC_STATUS_SSC_MASK		0x400
 #define SSC_STATUS_PLL_LOCK_MASK	0x800
 
+#define IDX_ADDR(pcie)	\
+	(pcie->reg_offsets[EXT_CFG_INDEX])
+#define DATA_ADDR(pcie)	\
+	(pcie->reg_offsets[EXT_CFG_DATA])
+#define PCIE_RGR1_SW_INIT_1(pcie) \
+	(pcie->reg_offsets[RGR1_SW_INIT_1])
+
+enum {
+	RGR1_SW_INIT_1,
+	EXT_CFG_INDEX,
+	EXT_CFG_DATA,
+};
+
+enum {
+	RGR1_SW_INIT_1_INIT_MASK,
+	RGR1_SW_INIT_1_INIT_SHIFT,
+};
+
+enum pcie_type {
+	GENERIC,
+	BCM7278,
+};
+
+struct pcie_cfg_data {
+	const int *reg_field_info;
+	const int *offsets;
+	const enum pcie_type type;
+};
+
+static const int pcie_reg_field_info[] = {
+	[RGR1_SW_INIT_1_INIT_MASK] = 0x2,
+	[RGR1_SW_INIT_1_INIT_SHIFT] = 0x1,
+};
+
+static const int pcie_reg_field_info_bcm7278[] = {
+	[RGR1_SW_INIT_1_INIT_MASK] = 0x1,
+	[RGR1_SW_INIT_1_INIT_SHIFT] = 0x0,
+};
+
+static const int pcie_offsets[] = {
+	[RGR1_SW_INIT_1] = 0x9210,
+	[EXT_CFG_INDEX]  = 0x9000,
+	[EXT_CFG_DATA]   = 0x9004,
+};
+
+static const struct pcie_cfg_data generic_cfg = {
+	.reg_field_info	= pcie_reg_field_info,
+	.offsets	= pcie_offsets,
+	.type		= GENERIC,
+};
+
+static const int pcie_offset_bcm7278[] = {
+	[RGR1_SW_INIT_1] = 0xc010,
+	[EXT_CFG_INDEX] = 0x9000,
+	[EXT_CFG_DATA] = 0x9004,
+};
+
+static const struct pcie_cfg_data bcm7278_cfg = {
+	.reg_field_info = pcie_reg_field_info_bcm7278,
+	.offsets	= pcie_offset_bcm7278,
+	.type		= BCM7278,
+};
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -176,6 +238,9 @@  struct brcm_pcie {
 	int			gen;
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
+	const int		*reg_offsets;
+	const int		*reg_field_info;
+	enum pcie_type		type;
 };
 
 /*
@@ -602,20 +667,21 @@  static struct pci_ops brcm_pcie_ops = {
 
 static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
 {
-	u32 tmp;
+	u32 tmp, mask =  pcie->reg_field_info[RGR1_SW_INIT_1_INIT_MASK];
+	u32 shift = pcie->reg_field_info[RGR1_SW_INIT_1_INIT_SHIFT];
 
-	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
-	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
-	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
+	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));
 }
 
 static inline void brcm_pcie_perst_set(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
-	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
+	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);
+	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 }
 
 static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
@@ -924,10 +990,17 @@  static int brcm_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id brcm_pcie_match[] = {
+	{ .compatible = "brcm,bcm2711-pcie", .data = &generic_cfg },
+	{},
+};
+
 static int brcm_pcie_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node, *msi_np;
 	struct pci_host_bridge *bridge;
+	const struct pcie_cfg_data *data;
+	const struct of_device_id *of_id;
 	struct brcm_pcie *pcie;
 	struct pci_bus *child;
 	struct resource *res;
@@ -937,9 +1010,19 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 	if (!bridge)
 		return -ENOMEM;
 
+	of_id = of_match_node(brcm_pcie_match, np);
+	if (!of_id) {
+		pr_err("failed to look up compatible string\n");
+		return -EINVAL;
+	}
+	data = of_id->data;
+
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->dev = &pdev->dev;
 	pcie->np = np;
+	pcie->reg_offsets = data->offsets;
+	pcie->reg_field_info = data->reg_field_info;
+	pcie->type = data->type;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pcie->base = devm_ioremap_resource(&pdev->dev, res);
@@ -1005,10 +1088,6 @@  static int brcm_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static const struct of_device_id brcm_pcie_match[] = {
-	{ .compatible = "brcm,bcm2711-pcie" },
-	{},
-};
 MODULE_DEVICE_TABLE(of, brcm_pcie_match);
 
 static struct platform_driver brcm_pcie_driver = {