diff mbox series

[RFC,1/6] ACPI/IORT: Set PMCG device parent

Message ID 1569854031-237636-2-git-send-email-john.garry@huawei.com
State New
Headers show
Series SMMUv3 PMCG IMP DEF event support | expand

Commit Message

John Garry Sept. 30, 2019, 2:33 p.m. UTC
In the IORT, a PMCG node includes a node reference to its associated
device.

Set the PMCG platform device parent device for future referencing.

For now, we only consider setting for when the associated component is an
SMMUv3.

Signed-off-by: John Garry <john.garry@huawei.com>

---
 drivers/acpi/arm64/iort.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Hanjun Guo Oct. 15, 2019, 2:35 a.m. UTC | #1
Hi John,

On 2019/9/30 22:33, John Garry wrote:
> In the IORT, a PMCG node includes a node reference to its associated

> device.

> 

> Set the PMCG platform device parent device for future referencing.

> 

> For now, we only consider setting for when the associated component is an

> SMMUv3.

> 

> Signed-off-by: John Garry <john.garry@huawei.com>

> ---

>  drivers/acpi/arm64/iort.c | 34 ++++++++++++++++++++++++++++++++--

>  1 file changed, 32 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c

> index 8569b79e8b58..0b687520c3e7 100644

> --- a/drivers/acpi/arm64/iort.c

> +++ b/drivers/acpi/arm64/iort.c

> @@ -1455,7 +1455,7 @@ static __init const struct iort_dev_config *iort_get_dev_cfg(

>   * Returns: 0 on success, <0 failure


...

>   */

>  static int __init iort_add_platform_device(struct acpi_iort_node *node,

> -					   const struct iort_dev_config *ops)

> +					   const struct iort_dev_config *ops, struct device *parent)


Since you added a input for this function, could you please update
the comments of this function as well?

>  {

>  	struct fwnode_handle *fwnode;

>  	struct platform_device *pdev;

> @@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,

>  	if (!pdev)

>  		return -ENOMEM;

>  

> +	pdev->dev.parent = parent;

> +

>  	if (ops->dev_set_proximity) {

>  		ret = ops->dev_set_proximity(&pdev->dev, node);

>  		if (ret)

> @@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)

>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }

>  #endif

>  

> +static int iort_fwnode_match(struct device *dev, const void *fwnode)

> +{

> +	return dev->fwnode == fwnode;

> +}

> +

>  static void __init iort_init_platform_devices(void)

>  {

>  	struct acpi_iort_node *iort_node, *iort_end;

> @@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)

>  				iort_table->length);

>  

>  	for (i = 0; i < iort->node_count; i++) {

> +		struct device *parent = NULL;

> +

>  		if (iort_node >= iort_end) {

>  			pr_err("iort node pointer overflows, bad table\n");

>  			return;

>  		}

>  

> +		/* Fixme: handle parent declared in IORT after PMCG */

> +		if (iort_node->type == ACPI_IORT_NODE_PMCG) {

> +			struct acpi_iort_node *iort_assoc_node;

> +			struct acpi_iort_pmcg *pmcg;

> +			u32 node_reference;

> +

> +			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;

> +

> +			node_reference = pmcg->node_reference;

> +			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,

> +				 node_reference);

> +

> +			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {

> +				struct fwnode_handle *assoc_fwnode;

> +

> +				assoc_fwnode = iort_get_fwnode(iort_assoc_node);

> +

> +				parent = bus_find_device(&platform_bus_type, NULL,

> +				      assoc_fwnode, iort_fwnode_match);

> +			}

> +		}


How about using a function to include those new added code to make this
function (iort_init_platform_devices()) a bit cleaner?

>  		iort_enable_acs(iort_node);

>  

>  		ops = iort_get_dev_cfg(iort_node);

> @@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)

>  

>  			iort_set_fwnode(iort_node, fwnode);

>  

> -			ret = iort_add_platform_device(iort_node, ops);

> +			ret = iort_add_platform_device(iort_node, ops, parent);


This function is called if ops is valid, so retrieve the parent
can be done before this function I think.

Thanks
Hanjun
John Garry Oct. 15, 2019, 9:07 a.m. UTC | #2
Hi Hanjun,

Thanks for checking this.

>>   */

>>  static int __init iort_add_platform_device(struct acpi_iort_node *node,

>> -					   const struct iort_dev_config *ops)

>> +					   const struct iort_dev_config *ops, struct device *parent)

>

> Since you added a input for this function, could you please update

> the comments of this function as well?


Right, that can be updated. Indeed, the current comment omit the @ops 
argument also.

>

>>  {

>>  	struct fwnode_handle *fwnode;

>>  	struct platform_device *pdev;

>> @@ -1466,6 +1466,8 @@ static int __init iort_add_platform_device(struct acpi_iort_node *node,

>>  	if (!pdev)

>>  		return -ENOMEM;

>>

>> +	pdev->dev.parent = parent;

>> +

>>  	if (ops->dev_set_proximity) {

>>  		ret = ops->dev_set_proximity(&pdev->dev, node);

>>  		if (ret)

>> @@ -1573,6 +1575,11 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)

>>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }

>>  #endif

>>

>> +static int iort_fwnode_match(struct device *dev, const void *fwnode)

>> +{

>> +	return dev->fwnode == fwnode;

>> +}

>> +

>>  static void __init iort_init_platform_devices(void)

>>  {

>>  	struct acpi_iort_node *iort_node, *iort_end;

>> @@ -1594,11 +1601,34 @@ static void __init iort_init_platform_devices(void)

>>  				iort_table->length);

>>

>>  	for (i = 0; i < iort->node_count; i++) {

>> +		struct device *parent = NULL;

>> +

>>  		if (iort_node >= iort_end) {

>>  			pr_err("iort node pointer overflows, bad table\n");

>>  			return;

>>  		}

>>

>> +		/* Fixme: handle parent declared in IORT after PMCG */

>> +		if (iort_node->type == ACPI_IORT_NODE_PMCG) {

>> +			struct acpi_iort_node *iort_assoc_node;

>> +			struct acpi_iort_pmcg *pmcg;

>> +			u32 node_reference;

>> +

>> +			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;

>> +

>> +			node_reference = pmcg->node_reference;

>> +			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,

>> +				 node_reference);

>> +

>> +			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {

>> +				struct fwnode_handle *assoc_fwnode;

>> +

>> +				assoc_fwnode = iort_get_fwnode(iort_assoc_node);

>> +

>> +				parent = bus_find_device(&platform_bus_type, NULL,

>> +				      assoc_fwnode, iort_fwnode_match);

>> +			}

>> +		}

>

> How about using a function to include those new added code to make this

> function (iort_init_platform_devices()) a bit cleaner?

>


Can do. But I still need to add code to deal with the scenario of the 
parent PMCG not being an SMMU, which is supported in the spec as I recall.

Note that I would not have FW to test that, so maybe can omit support 
for now as long as there's no regression.

>>  		iort_enable_acs(iort_node);

>>

>>  		ops = iort_get_dev_cfg(iort_node);

>> @@ -1609,7 +1639,7 @@ static void __init iort_init_platform_devices(void)

>>

>>  			iort_set_fwnode(iort_node, fwnode);

>>

>> -			ret = iort_add_platform_device(iort_node, ops);

>> +			ret = iort_add_platform_device(iort_node, ops, parent);

>

> This function is called if ops is valid, so retrieve the parent

> can be done before this function I think.


Ah, yes

Thanks,
John

>


> Thanks

> Hanjun

>

>

> .

>
diff mbox series

Patch

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 8569b79e8b58..0b687520c3e7 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1455,7 +1455,7 @@  static __init const struct iort_dev_config *iort_get_dev_cfg(
  * Returns: 0 on success, <0 failure
  */
 static int __init iort_add_platform_device(struct acpi_iort_node *node,
-					   const struct iort_dev_config *ops)
+					   const struct iort_dev_config *ops, struct device *parent)
 {
 	struct fwnode_handle *fwnode;
 	struct platform_device *pdev;
@@ -1466,6 +1466,8 @@  static int __init iort_add_platform_device(struct acpi_iort_node *node,
 	if (!pdev)
 		return -ENOMEM;
 
+	pdev->dev.parent = parent;
+
 	if (ops->dev_set_proximity) {
 		ret = ops->dev_set_proximity(&pdev->dev, node);
 		if (ret)
@@ -1573,6 +1575,11 @@  static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
 static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
 #endif
 
+static int iort_fwnode_match(struct device *dev, const void *fwnode)
+{
+	return dev->fwnode == fwnode;
+}
+
 static void __init iort_init_platform_devices(void)
 {
 	struct acpi_iort_node *iort_node, *iort_end;
@@ -1594,11 +1601,34 @@  static void __init iort_init_platform_devices(void)
 				iort_table->length);
 
 	for (i = 0; i < iort->node_count; i++) {
+		struct device *parent = NULL;
+
 		if (iort_node >= iort_end) {
 			pr_err("iort node pointer overflows, bad table\n");
 			return;
 		}
 
+		/* Fixme: handle parent declared in IORT after PMCG */
+		if (iort_node->type == ACPI_IORT_NODE_PMCG) {
+			struct acpi_iort_node *iort_assoc_node;
+			struct acpi_iort_pmcg *pmcg;
+			u32 node_reference;
+
+			pmcg = (struct acpi_iort_pmcg *)iort_node->node_data;
+
+			node_reference = pmcg->node_reference;
+			iort_assoc_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+				 node_reference);
+
+			if (iort_assoc_node->type == ACPI_IORT_NODE_SMMU_V3) {
+				struct fwnode_handle *assoc_fwnode;
+
+				assoc_fwnode = iort_get_fwnode(iort_assoc_node);
+
+				parent = bus_find_device(&platform_bus_type, NULL,
+				      assoc_fwnode, iort_fwnode_match);
+			}
+		}
 		iort_enable_acs(iort_node);
 
 		ops = iort_get_dev_cfg(iort_node);
@@ -1609,7 +1639,7 @@  static void __init iort_init_platform_devices(void)
 
 			iort_set_fwnode(iort_node, fwnode);
 
-			ret = iort_add_platform_device(iort_node, ops);
+			ret = iort_add_platform_device(iort_node, ops, parent);
 			if (ret) {
 				iort_delete_fwnode(iort_node);
 				acpi_free_fwnode_static(fwnode);