Hi Jim,
On Tue, 2020-05-19 at 16:34 -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: This is a digitally signed message part
