| Message ID | 1665598440-47410-4-git-send-email-lizhi.hou@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Generate device tree node for pci devices | expand |
On 10/26/22 12:39, Rob Herring wrote: > On Wed, Oct 12, 2022 at 11:14:00AM -0700, Lizhi Hou wrote: >> This patch addes 'reg', 'compatible' and 'device_typ' properties for > typo > > Please read submitting-patches.rst and what it says about 'This patch'. Sure. And I will merge patch 2 and 3. > >> dynamically generated PCI device tree node >> >> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com> >> Signed-off-by: Sonal Santan <sonal.santan@amd.com> >> Signed-off-by: Max Zhen <max.zhen@amd.com> >> Signed-off-by: Brian Xu <brian.xu@amd.com> >> --- >> drivers/pci/Makefile | 1 + >> drivers/pci/of.c | 10 ++- >> drivers/pci/of_property.c | 177 ++++++++++++++++++++++++++++++++++++++ > I don't think we need a separate file here and patches 2 and 3 should be > combined. Patch 2 alone doesn't really make sense. > > >> drivers/pci/pci.h | 3 +- >> 4 files changed, 189 insertions(+), 2 deletions(-) >> create mode 100644 drivers/pci/of_property.c >> >> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile >> index 2680e4c92f0a..cc8b4e01e29d 100644 >> --- a/drivers/pci/Makefile >> +++ b/drivers/pci/Makefile >> @@ -32,6 +32,7 @@ obj-$(CONFIG_PCI_P2PDMA) += p2pdma.o >> obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o >> obj-$(CONFIG_VGA_ARB) += vgaarb.o >> obj-$(CONFIG_PCI_DOE) += doe.o >> +obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o >> >> # Endpoint library must be initialized before its users >> obj-$(CONFIG_PCI_ENDPOINT) += endpoint/ >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index 83e042f495a6..00d716589660 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -619,6 +619,7 @@ void of_pci_make_dev_node(struct pci_dev *pdev) >> { >> struct device_node *parent, *dt_node; >> const char *pci_type = "dev"; >> + struct property *props; >> char *full_name; >> >> /* >> @@ -645,10 +646,15 @@ void of_pci_make_dev_node(struct pci_dev *pdev) >> if (!full_name) >> goto failed; >> >> - dt_node = of_create_node(parent, full_name, NULL); >> + props = of_pci_props_create(pdev); >> + if (!props) >> + goto failed; >> + >> + dt_node = of_create_node(parent, full_name, props); >> if (!dt_node) >> goto failed; >> >> + of_pci_props_destroy(props); >> kfree(full_name); >> >> pdev->dev.of_node = dt_node; >> @@ -656,6 +662,8 @@ void of_pci_make_dev_node(struct pci_dev *pdev) >> return; >> >> failed: >> + if (props) >> + of_pci_props_destroy(props); >> kfree(full_name); >> } >> #endif >> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c >> new file mode 100644 >> index 000000000000..693a08323aa4 >> --- /dev/null >> +++ b/drivers/pci/of_property.c >> @@ -0,0 +1,177 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (C) 2022, Advanced Micro Devices, Inc. >> + */ >> + >> +#include <linux/pci.h> >> +#include <linux/of.h> >> +#include <linux/bitfield.h> >> +#include <linux/bits.h> >> + >> +struct of_pci_prop { >> + char *name; >> + int (*prop_val)(struct pci_dev *pdev, void **val, u32 *len); >> +}; >> + >> +struct of_pci_addr_pair { >> + __be32 phys_hi; >> + __be32 phys_mid; >> + __be32 phys_lo; >> + __be32 size_hi; >> + __be32 size_lo; >> +}; >> + >> +#define OF_PCI_ADDR_SPACE_CONFIG 0x0 >> +#define OF_PCI_ADDR_SPACE_IO 0x1 >> +#define OF_PCI_ADDR_SPACE_MEM32 0x2 >> +#define OF_PCI_ADDR_SPACE_MEM64 0x3 >> + >> +#define OF_PCI_ADDR_FIELD_SS GENMASK(25, 24) >> +#define OF_PCI_ADDR_FIELD_PREFETCH BIT(30) >> +#define OF_PCI_ADDR_FIELD_BUS GENMASK(23, 16) >> +#define OF_PCI_ADDR_FIELD_DEV GENMASK(15, 11) >> +#define OF_PCI_ADDR_FIELD_FUNC GENMASK(10, 8) >> +#define OF_PCI_ADDR_FIELD_REG GENMASK(7, 0) >> + >> +#define OF_PCI_SIZE_HI GENMASK_ULL(63, 32) >> +#define OF_PCI_SIZE_LO GENMASK_ULL(31, 0) >> + >> +#define OF_PCI_PROP_COMPAT_LEN_MAX 256 >> +static int of_pci_prop_device_type(struct pci_dev *pdev, void **val, u32 *len) >> +{ >> + if (!pci_is_bridge(pdev)) >> + return 0; >> + >> + *val = kasprintf(GFP_KERNEL, "pci"); >> + if (!*val) >> + return -ENOMEM; >> + >> + *len = strlen(*val) + 1; >> + >> + return 0; >> +} >> + >> +static int of_pci_prop_reg(struct pci_dev *pdev, void **val, u32 *len) >> +{ >> + struct of_pci_addr_pair *reg; >> + u32 reg_val, base_addr, ss; >> + resource_size_t sz; >> + int i = 1, resno; >> + >> + reg = kzalloc(sizeof(*reg) * (PCI_STD_NUM_BARS + 1), GFP_KERNEL); >> + if (!reg) >> + return -ENOMEM; >> + >> + reg_val = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, OF_PCI_ADDR_SPACE_CONFIG) | >> + FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) | >> + FIELD_PREP(OF_PCI_ADDR_FIELD_DEV, PCI_SLOT(pdev->devfn)) | >> + FIELD_PREP(OF_PCI_ADDR_FIELD_FUNC, PCI_FUNC(pdev->devfn)); >> + reg[0].phys_hi = cpu_to_be32(reg_val); >> + >> + base_addr = PCI_BASE_ADDRESS_0; >> + for (resno = PCI_STD_RESOURCES; resno <= PCI_STD_RESOURCE_END; >> + resno++, base_addr += 4) { >> + sz = pci_resource_len(pdev, resno); >> + if (!sz) >> + continue; >> + >> + if (pci_resource_flags(pdev, resno) & IORESOURCE_IO) >> + ss = OF_PCI_ADDR_SPACE_IO; >> + else if (pci_resource_flags(pdev, resno) & IORESOURCE_MEM_64) >> + ss = OF_PCI_ADDR_SPACE_MEM64; >> + else >> + ss = OF_PCI_ADDR_SPACE_MEM32; >> + >> + reg_val &= ~(OF_PCI_ADDR_FIELD_SS | OF_PCI_ADDR_FIELD_PREFETCH | >> + OF_PCI_ADDR_FIELD_REG); >> + reg_val |= FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss) | >> + FIELD_PREP(OF_PCI_ADDR_FIELD_REG, base_addr); >> + if (pci_resource_flags(pdev, resno) & IORESOURCE_PREFETCH) >> + reg_val |= OF_PCI_ADDR_FIELD_PREFETCH; >> + reg[i].phys_hi = cpu_to_be32(reg_val); >> + reg[i].size_hi = cpu_to_be32(FIELD_GET(OF_PCI_SIZE_HI, sz)); >> + reg[i].size_lo = cpu_to_be32(FIELD_GET(OF_PCI_SIZE_LO, sz)); >> + i++; >> + } >> + >> + *val = reg; >> + *len = i * sizeof(*reg); >> + >> + return 0; >> +} >> + >> +static int of_pci_prop_compatible(struct pci_dev *pdev, void **val, u32 *len) >> +{ >> + char *compat; >> + >> + compat = kzalloc(OF_PCI_PROP_COMPAT_LEN_MAX, GFP_KERNEL); > The size here looks pretty arbitrary yet we should be able to calculate > the worst case. I will remove this. > >> + if (!compat) >> + return -ENOMEM; >> + >> + *val = compat; >> + if (pdev->subsystem_vendor) { >> + compat += sprintf(compat, "pci%x,%x.%x.%x.%x", >> + pdev->vendor, pdev->device, >> + pdev->subsystem_vendor, >> + pdev->subsystem_device, >> + pdev->revision) + 1; >> + compat += sprintf(compat, "pci%x,%x.%x.%x", >> + pdev->vendor, pdev->device, >> + pdev->subsystem_vendor, >> + pdev->subsystem_device) + 1; >> + compat += sprintf(compat, "pci%x,%x", >> + pdev->subsystem_vendor, >> + pdev->subsystem_device) + 1; >> + } >> + compat += sprintf(compat, "pci%x,%x.%x", >> + pdev->vendor, pdev->device, pdev->revision) + 1; >> + compat += sprintf(compat, "pci%x,%x", pdev->vendor, pdev->device) + 1; >> + compat += sprintf(compat, "pciclass,%06x", pdev->class) + 1; >> + compat += sprintf(compat, "pciclass,%04x", pdev->class >> 8) + 1; > No checking/preventing overrunning the compat buffer? > > I don't think we need all these compatible strings. One with VID/PID and > one with the class should be sufficient. But I'm not sure offhand what > subsystem_vendor/device device is... Ok. I will keep pci%x,%x, pciclass,%06x, pciclass,%04x. > >> + >> + *len = (u32)(compat - (char *)*val); >> + >> + return 0; >> +} >> + >> +struct of_pci_prop of_pci_props[] = { >> + { .name = "device_type", .prop_val = of_pci_prop_device_type }, > This only only applies to bridge nodes. In of_pci_prop_device_type, it returns immediately for pci endpoint. To make it obvious, I will separate bridge and endpoint property arrays. > >> + { .name = "reg", .prop_val = of_pci_prop_reg }, >> + { .name = "compatible", .prop_val = of_pci_prop_compatible }, >> + {}, >> +}; >> + >> +struct property *of_pci_props_create(struct pci_dev *pdev) >> +{ >> + struct property *props, *pp; >> + void *val; >> + u32 len; >> + int i; >> + >> + props = kcalloc(ARRAY_SIZE(of_pci_props), sizeof(*props), GFP_KERNEL); >> + if (!props) >> + return NULL; >> + >> + pp = props; >> + for (i = 0; of_pci_props[i].name; i++) { >> + len = 0; >> + of_pci_props[i].prop_val(pdev, &val, &len); >> + if (!len) >> + continue; >> + props->name = of_pci_props[i].name; >> + props->value = val; >> + props->length = len; >> + props++; > This creates an array of properties and then copies each one, and it > also exposes the internals of 'struct property' which we want to make > opaque. Neither of these is great. > > I'd rather see the of_changeset API expanded to handle specific types of > properties. Something like this: > > of_changeset_add_prop_string(cset, node, "device_type", "pci"); > of_changeset_add_prop_string_array(cset, node, "compatible", compats, cnt); > of_changeset_add_prop_u32_array(cset, node, "reg", reg, cnt); > > And perhaps these functions just wrap similar non-changeset functions > that produce a struct property. > > IOW, it should be similar to the of_property_read_* APIs, but to > set/add rather than get. Okay. I will add above 3 APIs and use them to create properties. > > > You are also missing 'ranges', '#address-cells, and '#size-cells' in > bridge nodes. I will add them. Do you have more in mind that I need to add with this patch? Thanks, Lizhi > > Rob
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 2680e4c92f0a..cc8b4e01e29d 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_PCI_P2PDMA) += p2pdma.o obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o obj-$(CONFIG_VGA_ARB) += vgaarb.o obj-$(CONFIG_PCI_DOE) += doe.o +obj-$(CONFIG_PCI_DYNAMIC_OF_NODES) += of_property.o # Endpoint library must be initialized before its users obj-$(CONFIG_PCI_ENDPOINT) += endpoint/ diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 83e042f495a6..00d716589660 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -619,6 +619,7 @@ void of_pci_make_dev_node(struct pci_dev *pdev) { struct device_node *parent, *dt_node; const char *pci_type = "dev"; + struct property *props; char *full_name; /* @@ -645,10 +646,15 @@ void of_pci_make_dev_node(struct pci_dev *pdev) if (!full_name) goto failed; - dt_node = of_create_node(parent, full_name, NULL); + props = of_pci_props_create(pdev); + if (!props) + goto failed; + + dt_node = of_create_node(parent, full_name, props); if (!dt_node) goto failed; + of_pci_props_destroy(props); kfree(full_name); pdev->dev.of_node = dt_node; @@ -656,6 +662,8 @@ void of_pci_make_dev_node(struct pci_dev *pdev) return; failed: + if (props) + of_pci_props_destroy(props); kfree(full_name); } #endif diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c new file mode 100644 index 000000000000..693a08323aa4 --- /dev/null +++ b/drivers/pci/of_property.c @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2022, Advanced Micro Devices, Inc. + */ + +#include <linux/pci.h> +#include <linux/of.h> +#include <linux/bitfield.h> +#include <linux/bits.h> + +struct of_pci_prop { + char *name; + int (*prop_val)(struct pci_dev *pdev, void **val, u32 *len); +}; + +struct of_pci_addr_pair { + __be32 phys_hi; + __be32 phys_mid; + __be32 phys_lo; + __be32 size_hi; + __be32 size_lo; +}; + +#define OF_PCI_ADDR_SPACE_CONFIG 0x0 +#define OF_PCI_ADDR_SPACE_IO 0x1 +#define OF_PCI_ADDR_SPACE_MEM32 0x2 +#define OF_PCI_ADDR_SPACE_MEM64 0x3 + +#define OF_PCI_ADDR_FIELD_SS GENMASK(25, 24) +#define OF_PCI_ADDR_FIELD_PREFETCH BIT(30) +#define OF_PCI_ADDR_FIELD_BUS GENMASK(23, 16) +#define OF_PCI_ADDR_FIELD_DEV GENMASK(15, 11) +#define OF_PCI_ADDR_FIELD_FUNC GENMASK(10, 8) +#define OF_PCI_ADDR_FIELD_REG GENMASK(7, 0) + +#define OF_PCI_SIZE_HI GENMASK_ULL(63, 32) +#define OF_PCI_SIZE_LO GENMASK_ULL(31, 0) + +#define OF_PCI_PROP_COMPAT_LEN_MAX 256 +static int of_pci_prop_device_type(struct pci_dev *pdev, void **val, u32 *len) +{ + if (!pci_is_bridge(pdev)) + return 0; + + *val = kasprintf(GFP_KERNEL, "pci"); + if (!*val) + return -ENOMEM; + + *len = strlen(*val) + 1; + + return 0; +} + +static int of_pci_prop_reg(struct pci_dev *pdev, void **val, u32 *len) +{ + struct of_pci_addr_pair *reg; + u32 reg_val, base_addr, ss; + resource_size_t sz; + int i = 1, resno; + + reg = kzalloc(sizeof(*reg) * (PCI_STD_NUM_BARS + 1), GFP_KERNEL); + if (!reg) + return -ENOMEM; + + reg_val = FIELD_PREP(OF_PCI_ADDR_FIELD_SS, OF_PCI_ADDR_SPACE_CONFIG) | + FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) | + FIELD_PREP(OF_PCI_ADDR_FIELD_DEV, PCI_SLOT(pdev->devfn)) | + FIELD_PREP(OF_PCI_ADDR_FIELD_FUNC, PCI_FUNC(pdev->devfn)); + reg[0].phys_hi = cpu_to_be32(reg_val); + + base_addr = PCI_BASE_ADDRESS_0; + for (resno = PCI_STD_RESOURCES; resno <= PCI_STD_RESOURCE_END; + resno++, base_addr += 4) { + sz = pci_resource_len(pdev, resno); + if (!sz) + continue; + + if (pci_resource_flags(pdev, resno) & IORESOURCE_IO) + ss = OF_PCI_ADDR_SPACE_IO; + else if (pci_resource_flags(pdev, resno) & IORESOURCE_MEM_64) + ss = OF_PCI_ADDR_SPACE_MEM64; + else + ss = OF_PCI_ADDR_SPACE_MEM32; + + reg_val &= ~(OF_PCI_ADDR_FIELD_SS | OF_PCI_ADDR_FIELD_PREFETCH | + OF_PCI_ADDR_FIELD_REG); + reg_val |= FIELD_PREP(OF_PCI_ADDR_FIELD_SS, ss) | + FIELD_PREP(OF_PCI_ADDR_FIELD_REG, base_addr); + if (pci_resource_flags(pdev, resno) & IORESOURCE_PREFETCH) + reg_val |= OF_PCI_ADDR_FIELD_PREFETCH; + reg[i].phys_hi = cpu_to_be32(reg_val); + reg[i].size_hi = cpu_to_be32(FIELD_GET(OF_PCI_SIZE_HI, sz)); + reg[i].size_lo = cpu_to_be32(FIELD_GET(OF_PCI_SIZE_LO, sz)); + i++; + } + + *val = reg; + *len = i * sizeof(*reg); + + return 0; +} + +static int of_pci_prop_compatible(struct pci_dev *pdev, void **val, u32 *len) +{ + char *compat; + + compat = kzalloc(OF_PCI_PROP_COMPAT_LEN_MAX, GFP_KERNEL); + if (!compat) + return -ENOMEM; + + *val = compat; + if (pdev->subsystem_vendor) { + compat += sprintf(compat, "pci%x,%x.%x.%x.%x", + pdev->vendor, pdev->device, + pdev->subsystem_vendor, + pdev->subsystem_device, + pdev->revision) + 1; + compat += sprintf(compat, "pci%x,%x.%x.%x", + pdev->vendor, pdev->device, + pdev->subsystem_vendor, + pdev->subsystem_device) + 1; + compat += sprintf(compat, "pci%x,%x", + pdev->subsystem_vendor, + pdev->subsystem_device) + 1; + } + compat += sprintf(compat, "pci%x,%x.%x", + pdev->vendor, pdev->device, pdev->revision) + 1; + compat += sprintf(compat, "pci%x,%x", pdev->vendor, pdev->device) + 1; + compat += sprintf(compat, "pciclass,%06x", pdev->class) + 1; + compat += sprintf(compat, "pciclass,%04x", pdev->class >> 8) + 1; + + *len = (u32)(compat - (char *)*val); + + return 0; +} + +struct of_pci_prop of_pci_props[] = { + { .name = "device_type", .prop_val = of_pci_prop_device_type }, + { .name = "reg", .prop_val = of_pci_prop_reg }, + { .name = "compatible", .prop_val = of_pci_prop_compatible }, + {}, +}; + +struct property *of_pci_props_create(struct pci_dev *pdev) +{ + struct property *props, *pp; + void *val; + u32 len; + int i; + + props = kcalloc(ARRAY_SIZE(of_pci_props), sizeof(*props), GFP_KERNEL); + if (!props) + return NULL; + + pp = props; + for (i = 0; of_pci_props[i].name; i++) { + len = 0; + of_pci_props[i].prop_val(pdev, &val, &len); + if (!len) + continue; + props->name = of_pci_props[i].name; + props->value = val; + props->length = len; + props++; + } + + return pp; +} + +void of_pci_props_destroy(struct property *props) +{ + int i; + + for (i = 0; props[i].name; i++) + kfree(props[i].value); + kfree(props); +} diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index e0a11497b1ad..37841241ceea 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -681,7 +681,8 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br #ifdef CONFIG_PCI_DYNAMIC_OF_NODES void of_pci_make_dev_node(struct pci_dev *pdev); void of_pci_remove_node(struct pci_dev *pdev); - +struct property *of_pci_props_create(struct pci_dev *pdev); +void of_pci_props_destroy(struct property *props); #else static inline void of_pci_make_dev_node(struct pci_dev *pdev)