[PATCH v1 16/24] pci: Add PCI controller driver for OcteonTX / TX2
Stefan Roese
sr at denx.de
Thu Jul 30 18:25:58 CEST 2020
Hi Simon,
On 28.07.20 21:01, Simon Glass wrote:
> Hi Stefan,
>
> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr at denx.de> wrote:
>>
>> From: Suneel Garapati <sgarapati at marvell.com>
>>
>> Adds support for PCI ECAM/PEM controllers found on OcteonTX
>> or OcteonTX2 SoC platforms.
>>
>> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>
>> Signed-off-by: Stefan Roese <sr at 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 at 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
More information about the U-Boot
mailing list