[PATCH v1 16/24] pci: Add PCI controller driver for OcteonTX / TX2

[Stefan Roese <sr@denx.de>]

Hi Simon,

On 28.07.20 21:01, Simon Glass wrote:
> Hi Stefan,
> 
> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote:
>>
>> From: Suneel Garapati <sgarapati@marvell.com>
>>
>> Adds support for PCI ECAM/PEM controllers found on OcteonTX
>> or OcteonTX2 SoC platforms.
>>
>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> ---
>>
>> Changes in v1:
>> - Change patch subject
>> - Remove inclusion of common.h
>> - Remove #ifdef's and use driver specific data instead
>> - Add comments to struct
>> - Add some helper functions to reduce code size
>> - Misc coding style changes (blank lines etc)
>> - Use debug() instead of printf() in some cases
>>
>>   drivers/pci/Kconfig        |   8 +
>>   drivers/pci/Makefile       |   1 +
>>   drivers/pci/pci_octeontx.c | 344 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 353 insertions(+)
>>   create mode 100644 drivers/pci/pci_octeontx.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>
>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>> index bc77c23f89..89cca6ffb3 100644
>> --- a/drivers/pci/Kconfig
>> +++ b/drivers/pci/Kconfig
>> @@ -149,6 +149,14 @@ config PCI_TEGRA
>>            with a total of 5 lanes. Some boards require this for Ethernet
>>            support to work (e.g. beaver, jetson-tk1).
>>
>> +config PCI_OCTEONTX
>> +       bool "OcteonTX PCI support"
>> +       depends on (ARCH_OCTEONTX || ARCH_OCTEONTX2)
>> +       help
>> +         Enable support for the OcteonTX/TX2 SoC family ECAM/PEM controllers.
>> +         These controllers provide PCI configuration access to all on-board
>> +         peripherals so it should only be disabled for testing purposes
>> +
>>   config PCI_XILINX
>>          bool "Xilinx AXI Bridge for PCI Express"
>>          depends on DM_PCI
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 6378821aaf..0529cceee7 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -45,3 +45,4 @@ obj-$(CONFIG_PCI_KEYSTONE) += pcie_dw_ti.o
>>   obj-$(CONFIG_PCIE_MEDIATEK) += pcie_mediatek.o
>>   obj-$(CONFIG_PCIE_ROCKCHIP) += pcie_rockchip.o
>>   obj-$(CONFIG_PCI_BRCMSTB) += pcie_brcmstb.o
>> +obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o
>> diff --git a/drivers/pci/pci_octeontx.c b/drivers/pci/pci_octeontx.c
>> new file mode 100644
>> index 0000000000..5c6a6f05f2
>> --- /dev/null
>> +++ b/drivers/pci/pci_octeontx.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier:    GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Marvell International Ltd.
>> + *
>> + * https://spdx.org/licenses
>> + */
>> +
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <fdtdec.h>
>> +#include <log.h>
>> +#include <malloc.h>
>> +#include <pci.h>
>> +
>> +#include <asm/io.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +enum {
> 
> comment?

Will add in next version.

>> +       OTX_ECAM,
>> +       OTX_PEM,
>> +       OTX2_PEM,
>> +};
>> +
>> +/**
>> + * struct octeontx_pci - Driver private data
>> + * @type:      Device type matched via compatible
>> + * @cfg:       Config resource
>> + * @bus:       Bus resource
>> + */
>> +struct octeontx_pci {
>> +       unsigned int type;
> 
> Is this the enum above?

Yes. Will add some comment here.

>> +
>> +       struct fdt_resource cfg;
>> +       struct fdt_resource bus;
>> +};
>> +
>> +static uintptr_t octeontx_cfg_addr(struct octeontx_pci *pcie,
>> +                                  int bus_offs, int shift_offs,
>> +                                  pci_dev_t bdf, uint offset)
>> +{
>> +       u32 bus, dev, func;
>> +       uintptr_t address;
>> +
>> +       bus = PCI_BUS(bdf) + bus_offs;
>> +       dev = PCI_DEV(bdf);
>> +       func = PCI_FUNC(bdf);
>> +
>> +       address = (bus << (20 + shift_offs)) |
>> +               (dev << (15 + shift_offs)) |
>> +               (func << (12 + shift_offs)) | offset;
>> +       address += pcie->cfg.start;
>> +
>> +       return address;
>> +}
>> +
>> +static ulong readl_size(uintptr_t addr, enum pci_size_t size)
>> +{
>> +       ulong val;
>> +
>> +       switch (size) {
>> +       case PCI_SIZE_8:
>> +               val = readb(addr);
>> +               break;
>> +       case PCI_SIZE_16:
>> +               val = readw(addr);
>> +               break;
>> +       case PCI_SIZE_32:
>> +               val = readl(addr);
>> +               break;
>> +       default:
>> +               printf("Invalid size\n");
> 
> return -EINVAL perhaps? Otherwise val is unset.

Good idea. Will update.

>> +       };
>> +
>> +       return val;
>> +}
>> +
>> +static void writel_size(uintptr_t addr, enum pci_size_t size, ulong valuep)
>> +{
>> +       switch (size) {
>> +       case PCI_SIZE_8:
>> +               writeb(valuep, addr);
>> +               break;
>> +       case PCI_SIZE_16:
>> +               writew(valuep, addr);
>> +               break;
>> +       case PCI_SIZE_32:
>> +               writel(valuep, addr);
>> +               break;
>> +       default:
>> +               printf("Invalid size\n");
>> +       };
>> +}
>> +
>> +static int octeontx_ecam_read_config(const struct udevice *bus, pci_dev_t bdf,
>> +                                    uint offset, ulong *valuep,
>> +                                    enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +
>> +       address = octeontx_cfg_addr(pcie, pcie->bus.start - hose->first_busno,
>> +                                   0, bdf, offset);
>> +       *valuep = readl_size(address, size);
>> +
>> +       debug("%02x.%02x.%02x: u%d %x -> %lx\n",
>> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), size, offset, *valuep);
>> +
>> +       return 0;
>> +}
>> +
>> +static int octeontx_ecam_write_config(struct udevice *bus, pci_dev_t bdf,
>> +                                     uint offset, ulong value,
>> +                                     enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +
>> +       address = octeontx_cfg_addr(pcie, pcie->bus.start - hose->first_busno,
>> +                                   0, bdf, offset);
>> +       writel_size(address, size, value);
>> +
>> +       debug("%02x.%02x.%02x: u%d %x <- %lx\n",
>> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), size, offset, value);
>> +
>> +       return 0;
>> +}
>> +
>> +static int octeontx_pem_read_config(const struct udevice *bus, pci_dev_t bdf,
>> +                                   uint offset, ulong *valuep,
>> +                                   enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +       u8 hdrtype;
>> +       u8 pri_bus = pcie->bus.start + 1 - hose->first_busno;
>> +       u32 bus_offs = (pri_bus << 16) | (pri_bus << 8) | (pri_bus << 0);
>> +
>> +       address = octeontx_cfg_addr(pcie, 1 - hose->first_busno, 4,
>> +                                   bdf, 0);
>> +
>> +       *valuep = pci_conv_32_to_size(~0UL, offset, size);
>> +
>> +       if (PCI_BUS(bdf) == 1 && PCI_DEV(bdf) > 0)
>> +               return 0;
> 
> Can you put this check in a function? It seems to appear everywhere.
> Also, shouldn't you return -EPERM, or similar?

Will change in next version.

>> +
>> +       *valuep = readl_size(address + offset, size);
>> +
>> +       hdrtype = readb(address + PCI_HEADER_TYPE);
>> +       if (hdrtype == PCI_HEADER_TYPE_BRIDGE &&
>> +           offset >= PCI_PRIMARY_BUS &&
>> +           offset <= PCI_SUBORDINATE_BUS &&
>> +           *valuep != pci_conv_32_to_size(~0UL, offset, size))
>> +               *valuep -= pci_conv_32_to_size(bus_offs, offset, size);
>> +
>> +       return 0;
>> +}
>> +
>> +static int octeontx_pem_write_config(struct udevice *bus, pci_dev_t bdf,
>> +                                    uint offset, ulong value,
>> +                                    enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +       u8 hdrtype;
>> +       u8 pri_bus = pcie->bus.start + 1 - hose->first_busno;
>> +       u32 bus_offs = (pri_bus << 16) | (pri_bus << 8) | (pri_bus << 0);
>> +
>> +       address = octeontx_cfg_addr(pcie, 1 - hose->first_busno, 4, bdf, 0);
>> +
>> +       hdrtype = readb(address + PCI_HEADER_TYPE);
>> +       if (hdrtype == PCI_HEADER_TYPE_BRIDGE &&
>> +           offset >= PCI_PRIMARY_BUS &&
>> +           offset <= PCI_SUBORDINATE_BUS &&
>> +           value != pci_conv_32_to_size(~0UL, offset, size))
>> +               value +=  pci_conv_32_to_size(bus_offs, offset, size);
>> +
>> +       if (PCI_BUS(bdf) == 1 && PCI_DEV(bdf) > 0)
>> +               return 0;
>> +
>> +       writel_size(address + offset, size, value);
>> +
>> +       debug("%02x.%02x.%02x: u%d %x (%lx) <- %lx\n",
>> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), size, offset,
>> +             address, value);
>> +
>> +       return 0;
>> +}
>> +
>> +static int octeontx2_pem_read_config(const struct udevice *bus, pci_dev_t bdf,
>> +                                    uint offset, ulong *valuep,
>> +                                    enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +
>> +       address = octeontx_cfg_addr(pcie, 1 - hose->first_busno, 0,
>> +                                   bdf, 0);
>> +
>> +       *valuep = pci_conv_32_to_size(~0UL, offset, size);
>> +
>> +       if (PCI_BUS(bdf) == 1 && PCI_DEV(bdf) > 0)
>> +               return 0;
>> +
>> +       *valuep = readl_size(address + offset, size);
>> +
>> +       debug("%02x.%02x.%02x: u%d %x (%lx) -> %lx\n",
>> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), size, offset,
>> +             address, *valuep);
>> +
>> +       return 0;
>> +}
>> +
>> +static int octeontx2_pem_write_config(struct udevice *bus, pci_dev_t bdf,
>> +                                     uint offset, ulong value,
>> +                                     enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       struct pci_controller *hose = dev_get_uclass_priv(bus);
>> +       uintptr_t address;
>> +
>> +       address = octeontx_cfg_addr(pcie, 1 - hose->first_busno, 0,
>> +                                   bdf, 0);
>> +
>> +       if (PCI_BUS(bdf) == 1 && PCI_DEV(bdf) > 0)
>> +               return 0;
>> +
>> +       writel_size(address + offset, size, value);
>> +
>> +       debug("%02x.%02x.%02x: u%d %x (%lx) <- %lx\n",
>> +             PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf), size, offset,
>> +             address, value);
>> +
>> +       return 0;
>> +}
>> +
>> +int pci_octeontx_read_config(const struct udevice *bus, pci_dev_t bdf,
>> +                            uint offset, ulong *valuep,
>> +                            enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       int ret = -EIO;
>> +
>> +       switch (pcie->type) {
>> +       case OTX_ECAM:
>> +               ret = octeontx_ecam_read_config(bus, bdf, offset, valuep,
>> +                                               size);
>> +               break;
>> +       case OTX_PEM:
>> +               ret = octeontx_pem_read_config(bus, bdf, offset, valuep,
>> +                                              size);
>> +               break;
>> +       case OTX2_PEM:
>> +               ret = octeontx2_pem_read_config(bus, bdf, offset, valuep,
>> +                                               size);
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +int pci_octeontx_write_config(struct udevice *bus, pci_dev_t bdf,
>> +                             uint offset, ulong value,
>> +                             enum pci_size_t size)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(bus);
>> +       int ret = -EIO;
>> +
>> +       switch (pcie->type) {
>> +       case OTX_ECAM:
>> +               ret = octeontx_ecam_write_config(bus, bdf, offset, value,
>> +                                                size);
>> +               break;
>> +       case OTX_PEM:
>> +               ret = octeontx_pem_write_config(bus, bdf, offset, value,
>> +                                               size);
>> +               break;
>> +       case OTX2_PEM:
>> +               ret = octeontx2_pem_write_config(bus, bdf, offset, value,
>> +                                                size);
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static int pci_octeontx_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +       return 0;
>> +}
>> +
>> +static int pci_octeontx_probe(struct udevice *dev)
>> +{
>> +       struct octeontx_pci *pcie = (struct octeontx_pci *)dev_get_priv(dev);
>> +       int err;
>> +
>> +       pcie->type = dev_get_driver_data(dev);
>> +
>> +       err = fdt_get_resource(gd->fdt_blob, dev->node.of_offset, "reg", 0,
>> +                              &pcie->cfg);
> 
> We really should have a livetree API for this.

Is this mandatory for this patch to get accepted? Or can I add this
to my list and post this later and port this driver to using these
livetree functions then?

BTW: Do you have an example for a similar function added to livetree,
so that I can choose the "correct" naming?

Thanks,
Stefan

>> +       if (err) {
>> +               debug("Error reading resource: %s\n", fdt_strerror(err));
>> +               return err;
>> +       }
>> +
>> +       err = fdtdec_get_pci_bus_range(gd->fdt_blob, dev->node.of_offset,
>> +                                      &pcie->bus);
> 
> and this...
> 
>> +       if (err) {
>> +               debug("Error reading resource: %s\n", fdt_strerror(err));
>> +               return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dm_pci_ops pci_octeontx_ops = {
>> +       .read_config    = pci_octeontx_read_config,
>> +       .write_config   = pci_octeontx_write_config,
>> +};
>> +
>> +static const struct udevice_id pci_octeontx_ids[] = {
>> +       { .compatible = "cavium,pci-host-thunder-ecam", .data = OTX_ECAM },
>> +       { .compatible = "cavium,pci-host-octeontx-ecam", .data = OTX_ECAM },
>> +       { .compatible = "pci-host-ecam-generic", .data = OTX_ECAM },
>> +       { .compatible = "cavium,pci-host-thunder-pem", .data = OTX_PEM },
>> +       { .compatible = "marvell,pci-host-octeontx2-pem", .data = OTX2_PEM },
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(pci_octeontx) = {
>> +       .name   = "pci_octeontx",
>> +       .id     = UCLASS_PCI,
>> +       .of_match = pci_octeontx_ids,
>> +       .ops    = &pci_octeontx_ops,
>> +       .ofdata_to_platdata = pci_octeontx_ofdata_to_platdata,
>> +       .probe  = pci_octeontx_probe,
>> +       .priv_auto_alloc_size = sizeof(struct octeontx_pci),
>> +       .flags = DM_FLAG_PRE_RELOC,
>> +};
>> --
>> 2.27.0
>>
> 
> Regards,
> Simon
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de