[PATCH v2 09/10] pci: Add driver for Broadcom STB PCIe controller
Sylwester Nawrocki
s.nawrocki at samsung.com
Fri May 8 13:46:43 CEST 2020
Hi Simon,
On 06.05.2020 16:47, Simon Glass wrote:
> On Mon, 4 May 2020 at 06:45, Sylwester Nawrocki <s.nawrocki at samsung.com> wrote:
>> ---
>> drivers/pci/Kconfig | 6 +
>> drivers/pci/Makefile | 1 +
>> drivers/pci/pcie_brcmstb.c | 594 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 601 insertions(+)
>> create mode 100644 drivers/pci/pcie_brcmstb.c
>
> A few small comments.
Thank you for time and a valuable review.
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> +config PCI_BRCMSTB
>> + bool "Broadcom STB PCIe controller"
>> + depends on DM_PCI
>> + depends on ARCH_BCM283X
>> + help
>> + Say Y here if you want to enable Broadcom STB PCIe controller support.
>
> What is STB? What features does this support? You should get a warning
> here to write at least three lines.
I'm going to change that help text to something along the lines of:
Say Y here if you want to enable support for PCIe controller
on Broadcom set-top-box (STB) SoCs.
This driver currently supports only BCM2711 SoC and RC mode
of the controller.
>> diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c
>> new file mode 100644
>> index 0000000..c6ddf92
>> --- /dev/null
>> +++ b/drivers/pci/pcie_brcmstb.c
>> @@ -0,0 +1,594 @@
>> +#include <asm/io.h>
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <dm/ofnode.h>
>> +#include <errno.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/log2.h>
>> +#include <pci.h>
>
> Check ordering of include files:
>
> https://protect2.fireeye.com/url?k=c3a0292d-9e737093-c3a1a262-0cc47a31ba82-690f9f10b3970f9d&q=1&u=https%3A%2F%2Fwww.denx.de%2Fwiki%2FU-Boot%2FCodingStyle
Thanks for the hint, it felt there was something wrong with the ordering.
>> +#define PCIE_MISC_MISC_CTRL 0x4008
>> +#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_MAX_BURST_SIZE_128 0x0
>> +#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000
>
> If you have a _MASK, don't you need a _SHIFT to allow you to read from
> the field?
I had shift definitions originally but these got removed when we started to
use the FIELD_GET macro and similar. Shifts are retrieved there from a mask
by ffs() call (find first bit set in a word). If that's not preferred in
u-boot I will switch back to using explicit shift definitions.
> Can you drop the PCIE_MISC prefix? These are very long and local to this file.
Yes, will do. It seems many of these bit field macro definitions could have
shorter names. I revisited them all again and shortened most of them.
>> +#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) * 4)
>> +/* MDIO registers */
>> +#define MDIO_RD_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 1 : 0)
>> +#define MDIO_WT_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 0 : 1)
>
> Are these two worth it? You can do this in your code> if (readl(xxx) & MDIO_DATA_DONE_MASK)
I will drop these macros, have copied them from the Linux driver without
much thought. They are not really necessary, especially when we use
readl_poll_timout().
> If you must use these, then I think true/false are better than 1/0.
>> +struct brcm_pcie {
>
> struct comment
Ok.
>> + void __iomem *base;
>> +
>> + int gen;
>> + bool ssc;
>> +};
>> +
>> +#define msleep(a) udelay((a) * 1000)
>
> This is already defined in U-Boot.
Indeed, I should have grepped for mdelay rather than msleep.
>> +/*
>> + * This is to convert the size of the inbound "BAR" region to the
>> + * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
>
> Please use a proper function comments with args and return value.
Will improve that.
>> + */
>> +static int brcm_pcie_encode_ibar_size(u64 size)
>> +{
>> + int log2_in = ilog2(size);
>> +
>> + if (log2_in >= 12 && log2_in <= 15)
>> + /* Covers 4KB to 32KB (inclusive) */
>> + return (log2_in - 12) + 0x1c;
>> + else if (log2_in >= 16 && log2_in <= 37)
>> + /* Covers 64KB to 32GB, (inclusive) */
>> + return log2_in - 15;
>> + /* Something is awry so disable */
>
> blank line always before return at end of function
>> + return 0;
>> +}
>> +/* The controller is capable of serving in both RC and EP roles */
>> +static bool brcm_pcie_rc_mode(struct brcm_pcie *pcie)
>> +{
>> + u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
>
> Consider using a struct for your registers which would reduce the size
> of this code.
I agree with all your other comments but here I have some doubts.
The register offsets used are sparse and there will be many gaps in a struct.
Anyway, I will give it a try and see how it looks.
>> + return !!FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK, val);
>
> What is FIELD_GET? We have this sort of thing rejected in the past.
It's a common macro from linux/bitfield.h for masking and shifting a bitfield.
If its use is not preferred I will switch to using explicit _SHIFTs as
I mentioned above.
> Here, since you return a bool, the !! should not be necessary.
>
>> +}
>> +
>> +static bool brcm_pcie_link_up(struct brcm_pcie *pcie)
>> +{
>> + u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
>> + u32 dla = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK, val);
>> + u32 plu = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK, val);
>
> PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK is way too long.
>
> How about STATUS_PCIE_PHYLINKUP_MASK?
Yes, that really looks much more reasonable.
>> + return dla && plu;
>> +}
>> +
>> +static int brcm_pcie_config_address(const struct udevice *udev, pci_dev_t bdf,
>
> Please use 'dev' instead of udev for the device.
Fixed.
>> + uint offset, void **paddress)
>> +{
>> + struct brcm_pcie *pcie = dev_get_priv(udev);
>> + unsigned int bus = PCI_BUS(bdf);
>> + unsigned int dev = PCI_DEV(bdf);
>> + int idx;
>> +
>> + /*
>> + * Busses 0 (host PCIe bridge) and 1 (its immediate child)
>> + * are limited to a single device each
>> + */
>> + if ((bus == (udev->seq + 1)) && dev > 0)
>> + return -ENODEV;
>> +
>> + /* Accesses to the RC go right to the RC registers if PCI device == 0 */
>> + if (bus == udev->seq) {
>> + if (PCI_DEV(bdf))
>> + return -ENODEV;
>
> You can't return that as there is a device. Perhaps ENXIO? Does this
> indicate an internal error? If so you could add a comment here as to
> how to fix it.
The function is supposed to return 0 when @bdf is valid and a non 0 value
otherwise. The pci core code calls it with a range of PCI bus/device/func
and we have to return 0 only when a PCI bus/device/function is valid.
Exact error code from this function is not propagated further as can be
seen in pci_generic_mmap_read_config(). This function only gets called
by below two read/write function callbacks.
I just followed what is done in other PCIe drivers and used ENODEV.
I'm going to change the error code to EINVAL. I have also already simplified
this function so a non-zero value is returned in one place at the beginning
only.
>> + *paddress = pcie->base + offset;
>> + return 0;
>> + }
>> +
>> + /* For devices, write to the config space index register */
>> + idx = brcm_pcie_cfg_index(bdf, 0);
>> +
>> + writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
>> + *paddress = pcie->base + PCIE_EXT_CFG_DATA + offset;
>> +
>> + return 0;
>> +}
>> +
>> +static int brcm_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong *valuep,
>> + enum pci_size_t size)
>> +{
>> + return pci_generic_mmap_read_config(bus, brcm_pcie_config_address,
>> + bdf, offset, valuep, size);
>> +}
>> +
>> +static int brcm_pcie_write_config(struct udevice *bus, pci_dev_t bdf,
>> + uint offset, ulong value,
>> + enum pci_size_t size)
>> +{
>> + return pci_generic_mmap_write_config(bus, brcm_pcie_config_address,
>> + bdf, offset, value, size);
>> +}
>> +static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val)
>> +{
>> + u32 tmp;
>> +
>> + 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);
>
> See clrsetbits_le32(), etc. Below alos.
Yes, thanks, that's really what I was missing but somehow overlooked.
>> +}
>> +/* Negative return value indicates error */
>
> This would be incorporated in a full function comment
OK, will improve that.
>> +static int brcm_pcie_mdio_read(void __iomem *base, u8 port, u8 regad, u32 *val)
>> +{
>> + int tries;
>> + u32 data;
>> +
>> + writel(brcm_pcie_mdio_form_pkt(port, regad, MDIO_CMD_READ),
>> + base + PCIE_RC_DL_MDIO_ADDR);
>> + readl(base + PCIE_RC_DL_MDIO_ADDR);
>> +
>> + data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
>> + for (tries = 0; !MDIO_RD_DONE(data) && tries < 10; tries++) {
>> + udelay(10);
>> + data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
>> + }
>> +
>> + *val = FIELD_GET(MDIO_DATA_MASK, data);
>> + return MDIO_RD_DONE(data) ? 0 : -EIO;
>> +}
>> +static int brcm_pcie_probe(struct udevice *dev)
>> +{
>> + struct udevice *ctlr = pci_get_controller(dev);
>> + struct pci_controller *hose = dev_get_uclass_priv(ctlr);
>> + struct brcm_pcie *pcie = dev_get_priv(dev);
>> + void __iomem *base = pcie->base;
>> + bool ssc_good = false;
>> + int num_out_wins = 0;
>> + u64 rc_bar2_offset, rc_bar2_size;
>> + unsigned int scb_size_val;
>> + int i, ret;
>> + u16 nlw, cls, lnksta;
>> + u32 tmp;
>> +
>> + /* Reset the bridge */
>> + brcm_pcie_bridge_sw_init_set(pcie, 1);
>> +
>> + udelay(150);
>
> Please add a comment as to how you chose the value, and below.
I'm not sure what are exact requirements for these delays, in downstream
Linux code usleep_range(100, 200) is used, so I picked 150 us value.
>> + /* Take the bridge out of reset */
>> + brcm_pcie_bridge_sw_init_set(pcie, 0);
>> +
>> + tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> + tmp &= ~PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
>> + writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
>> + /* Wait for SerDes to be stable */
>> + udelay(150);
>> + if (!brcm_pcie_link_up(pcie)) {
>> + printf("PCIe BRCM: link down\n");
>> + return -ENODEV;
>
> Again, cannot return that error
Will make it EINVAL as is done below.
>> + }
>> +
>> + if (!brcm_pcie_rc_mode(pcie)) {
>> + printf("PCIe misconfigured; is in EP mode\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (pcie->ssc) {
>> + ret = brcm_pcie_set_ssc(pcie);
>> + if (ret == 0)
>> + ssc_good = true;
>> + else
>> + printf("PCIe BRCM: failed attempt to enter SSC mode\n");
>
> Should this return an error?
In Linux driver any errors here are also ignored. AFAIU that Spread
Spectrum Clocking is not critical for the PCIe controller's operation,
it's just for EMI reduction. I don't have detailed documentation
of the controller and I'm not sure what comment could be added here.
--
Regards,
Sylwester
More information about the U-Boot
mailing list