Hello,
Some comments below...
On Mon, Aug 16, 2021 at 8:51 AM Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
>
> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>
> BCM4908 is Broadcom's 64-bit platform with Broadcom's own Brahma-B53
> CPU(s). It uses the same PCIe hardware block as STB (Set-Top-Box) family
> but in a slightly different revision & setup.
>
> Registers in BCM4908 variant are mostly the same but controller setup
> differs a bit. It requires setting few extra registers and takes
> slightly different bars setup.
>
> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 137 +++++++++++++++++++++++---
> 1 file changed, 123 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index cc30215f5a43..24bc7efcfdd5 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -51,15 +51,20 @@
> #define PCIE_RC_DL_MDIO_RD_DATA 0x1108
>
> #define PCIE_MISC_MISC_CTRL 0x4008
> +#define PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE 0x00000080
> +#define PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE 0x00000400
> +#define PCIE_MISC_MISC_CTRL_PCIE_IN_WR_COMBINE 0x00000800
> #define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000
> #define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000
> #define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000
> +#define PCIE_MISC_MISC_CTRL_BURST_ALIGN_MASK 0x00080000
>
> #define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000
> #define PCIE_MISC_MISC_CTRL_SCB1_SIZE_MASK 0x07c00000
> #define PCIE_MISC_MISC_CTRL_SCB2_SIZE_MASK 0x0000001f
> #define SCB_SIZE_MASK(x) PCIE_MISC_MISC_CTRL_SCB ## x ## _SIZE_MASK
>
> +
Extra WS
> #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c
> #define PCIE_MEM_WIN0_LO(win) \
> PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 8)
> @@ -115,6 +120,9 @@
> #define PCIE_MEM_WIN0_LIMIT_HI(win) \
> PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
>
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_OFFSET 0x40ac
> +#define PCIE_MISC_UBUS_BAR_CONFIG_ACCESS_EN 0x00000001
> +
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000
> @@ -131,6 +139,13 @@
> #define PCIE_EXT_CFG_DATA 0x8000
> #define PCIE_EXT_CFG_INDEX 0x9000
>
> +#define PCIE_CPU_INTR1_INTR_MASK_CLEAR_OFFSET 0x940c
> +#define PCIE_CPU_INTR1_PCIE_INTA_CPU_INTR 0x00000002
> +#define PCIE_CPU_INTR1_PCIE_INTB_CPU_INTR 0x00000004
> +#define PCIE_CPU_INTR1_PCIE_INTC_CPU_INTR 0x00000008
> +#define PCIE_CPU_INTR1_PCIE_INTD_CPU_INTR 0x00000010
> +#define PCIE_CPU_INTR1_PCIE_INTR_CPU_INTR 0x00000020
> +
> #define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1
> #define PCIE_RGR1_SW_INIT_1_PERST_SHIFT 0x0
>
> @@ -746,13 +761,19 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32
>
> static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
> {
> - if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
> - return;
> + if (pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> + brcm_pcie_perst_set_7278(pcie, val);
> + } else {
> + if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
> + return;
Is this necessary? Better to just assume it is there and call the
assert/deassrt routines always as they work on NULL.
>
> - if (val)
> - reset_control_assert(pcie->perst_reset);
> - else
> - reset_control_deassert(pcie->perst_reset);
> + if (val)
> + reset_control_assert(pcie->perst_reset);
> + else
> + reset_control_deassert(pcie->perst_reset);
> + }
> +
> + usleep_range(10000, 20000);
Why so long? And does it need to be long for both directions (assert/deassert)?
> }
>
> static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
> @@ -861,6 +882,86 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> return 0;
> }
>
> +static int brcm_pcie_setup_bcm4908(struct brcm_pcie *pcie)
> +{
> + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> + void __iomem *base = pcie->base;
> + struct device *dev = pcie->dev;
> + struct resource_entry *entry;
> + u32 burst_align;
> + u32 burst;
> + u32 tmp;
> + int win;
> +
> + pcie->perst_set(pcie, 0);
> +
> + msleep(500);
> +
> + if (!brcm_pcie_link_up(pcie)) {
> + dev_err(dev, "link down\n");
> + return -ENODEV;
> + }
> +
> + /* setup lgacy outband interrupts */
> + tmp = PCIE_CPU_INTR1_PCIE_INTA_CPU_INTR |
> + PCIE_CPU_INTR1_PCIE_INTB_CPU_INTR |
> + PCIE_CPU_INTR1_PCIE_INTC_CPU_INTR |
> + PCIE_CPU_INTR1_PCIE_INTD_CPU_INTR;
> + writel(tmp, base + PCIE_CPU_INTR1_INTR_MASK_CLEAR_OFFSET);
Shouldn't this be taken care of by the interrupt-map and interrupt-map
mask properties of the PCIe DT node? For example, most of our STB
chips use something like this:
interrupt-map-mask = <0x0 0x0 0x0 0x7>;
interrupt-map = <
0 0 0 1 &intc 0 44 4
0 0 0 2 &intc 0 45 4
0 0 0 3 &intc 0 46 4
0 0 0 4 &intc 0 47 4>;
If you don't have an interrupt controller DT node/driver, try looking
at the existing BRCM ones or you may have to craft your own.
> +
> + win = 0;
> + resource_list_for_each_entry(entry, &bridge->windows) {
> + struct resource *res = entry->res;
> + u64 pcie_addr;
> +
> + if (resource_type(res) != IORESOURCE_MEM)
> + continue;
> +
> + if (win >= BRCM_NUM_PCIE_OUT_WINS) {
> + dev_err(pcie->dev, "too many outbound wins\n");
> + return -EINVAL;
> + }
> +
> + tmp = 0;
> + u32p_replace_bits(&tmp, res->start / SZ_1M,
> + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK);
> + u32p_replace_bits(&tmp, res->end / SZ_1M,
> + PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK);
> + writel(tmp, base + PCIE_MEM_WIN0_BASE_LIMIT(win));
> +
> + pcie_addr = res->start - entry->offset;
> + writel(lower_32_bits(pcie_addr), pcie->base + PCIE_MEM_WIN0_LO(win));
> + writel(upper_32_bits(pcie_addr), pcie->base + PCIE_MEM_WIN0_HI(win));
Again, can you try using the PCIe DT ranges/dma-ranges node properties
instead?
> +
> + win++;
> + }
> +
> + writel(0xf, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> +
> + tmp = PCIE_MISC_UBUS_BAR_CONFIG_ACCESS_EN;
> + writel(tmp, base + PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_OFFSET);
> +
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> + u32p_replace_bits(&tmp, PCI_CLASS_BRIDGE_PCI << 8,
> + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
Isn't this already in the driver? If it is, you should at least make
it a function and call it -- the same code should not exist in
multiple places.
> +
> + /* Burst */
> + burst = 0x1; /* 128 B */
> + burst_align = 1;
> +
> + tmp = readl(base + PCIE_MISC_MISC_CTRL);if (
> + u32p_replace_bits(&tmp, burst_align, PCIE_MISC_MISC_CTRL_BURST_ALIGN_MASK);
> + u32p_replace_bits(&tmp, burst, PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_IN_WR_COMBINE);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE);
It would be better if you could use the vanilla brcm_pcie_setup()
function and do something like
if (pcie->type == BCM4908)
do_this();
else
/* ... */
> + writel(tmp, base + PCIE_MISC_MISC_CTRL);
> +
> + return 0;
> +}
> +
> static int brcm_pcie_setup(struct brcm_pcie *pcie)
> {
> struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> @@ -1284,6 +1385,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> return PTR_ERR(pcie->perst_reset);
> }
>
> + if (pcie->type == BCM4908) {
> + /* On BCM4908 we can read rev early and perst_set needs it */
> + pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
We might as well do this in all cases, no need for the 4908 qualifier.
> +
> + pcie->perst_set(pcie, 1); + writel(lower_32_bits(pcie_addr), pcie->base + PCIE_MEM_WIN0_LO(win));
> + writel(upper_32_bits(pcie_addr), pcie->base + PCIE_MEM_WIN0_HI(win));
Again, can you try using the PCIe DT ranges/dma-ranges node properties instead?
> +
> + win++;
> + }
> +
> + writel(0xf, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> +
> + tmp = PCIE_MISC_UBUS_BAR_CONFIG_ACCESS_EN;
> + writel(tmp, base + PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_OFFSET);
> +
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
> + u32p_replace_bits(&tmp, PCI_CLASS_BRIDGE_PCI << 8,
> + PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
Isn't this already in the driver? If it is, it you should at least
make it a function and call it -- the same code should not exist in
multiple places.
> +
> + /* Burst */
> + burst = 0x1; /* 128 B */
> + burst_align = 1;
> +
> + tmp = readl(base + PCIE_MISC_MISC_CTRL);if (
> + u32p_replace_bits(&tmp, burst_align, PCIE_MISC_MISC_CTRL_BURST_ALIGN_MASK);
> + u32p_replace_bits(&tmp, burst, PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_IN_WR_COMBINE);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE);
> + u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE);
It would be better if you could use the vanilla brcm_pcie_setup()
function and do something like
if (pcie->type == BCM4908)
do_this();
else
/* ... */
> + writel(tmp, base + PCIE_MISC_MISC_CTRL);
> +
b
> + }
> +
> ret = reset_control_reset(pcie->rescal);
> if (ret)
> dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> @@ -1295,16 +1403,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> return ret;
> }
>
> - ret = brcm_pcie_setup(pcie);
> - if (ret)
> - goto fail;
> + if (pcie->type == BCM4908) {
> + ret = brcm_pcie_setup_bcm4908(pcie);
> + if (ret)
> + goto fail;
> + } else {
> + ret = brcm_pcie_setup(pcie);
> + if (ret)
> + goto fail;
> + }
As Florian said, try to use the existing routine with clauses for
4908-specific code.
Regards,
Jim Quinlan, Broadcom STB
>
> pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> - if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> - dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> - ret = -ENODEV;
> - goto fail;
> - }
>
> msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
> if (pci_msi_enabled() && msi_np == pcie->np) {
> --
> 2.26.2
>
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
