diff mbox series

[RFC,V2,3/3] PCI: Add basic properties for dynamically generated PCI OF node

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

Commit Message

Lizhi Hou Oct. 12, 2022, 6:14 p.m. UTC
This patch addes 'reg', 'compatible' and 'device_typ' properties for
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 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h         |   3 +-
 4 files changed, 189 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pci/of_property.c

Comments

Lizhi Hou Oct. 31, 2022, 11:21 p.m. UTC | #1
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 mbox series

Patch

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)