On Wed, 2020-05-20 at 10:30 -0400, Jim Quinlan wrote:

On Wed, May 20, 2020 at 7:51 AM Nicolas Saenz Julienne
[off-list ref] wrote:

[...]

quoted

quoted

+
+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.

quoted

quoted

+
 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.

quoted

quoted

-     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