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

[John Garry]

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