lkml.org 
[lkml]   [2020]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 04/15] PCI: brcmstb: Add compatibily of other chips
    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
    >

    \
     
     \ /
      Last update: 2020-05-21 21:35    [W:2.440 / U:0.904 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site