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

Rob Herring (Arm) Oct. 26, 2022, 7:39 p.m. UTC | #1
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'.

> 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.

> +	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...

> +
> +	*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.

> +	{ .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.


You are also missing 'ranges', '#address-cells, and '#size-cells' in 
bridge nodes.

Rob
Lizhi Hou Oct. 31, 2022, 11:21 p.m. UTC | #2
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)