Message-ID: <ZMyymUdV63g4eGaT@sunil-laptop>
Date:   Fri, 4 Aug 2023 13:41:05 +0530
From:   Sunil V L <sunilvl@...tanamicro.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-doc@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-acpi@...r.kernel.org, linux-pci@...r.kernel.org,
        Jonathan Corbet <corbet@....net>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Anup Patel <anup@...infault.org>,
        Marc Zyngier <maz@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Robert Moore <robert.moore@...el.com>,
        Haibo Xu <haibo1.xu@...el.com>,
        Andrew Jones <ajones@...tanamicro.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Atish Kumar Patra <atishp@...osinc.com>,
        Anup Patel <apatel@...tanamicro.com>
Subject: Re: [RFC PATCH v1 11/21] swnode: Add support to create early during
 boot

Hi Andy,

On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote:
> > From: Anup Patel <apatel@...tanamicro.com>
> > 
> > swnode framework can be used to create fwnode for interrupt
> > controllers.
> 
> Why? What is this for?
> Can you elaborate? This commit message is poorly written...
> 
> And why firmware node is not enough for ACPI case?
> I assume the fwnode in DT case is already provided by OF.
> 
Thanks a lot for the review!.

You are right, OF provides the fwnode for irqchip drivers. However, for
ACPI case, it is typically created using irq_domain_alloc_named_fwnode
or irq_domain_alloc_fwnode since these are not ACPI devices in the
namespace but from MADT. The fwnode created using
irq_domain_alloc_fwnode() is a simple one which doesn't support properties
similar to the one created by OF framework or software node framework.
Hence, lot of data from the MADT structures need to be cached as
separate structures in the drivers and also would need several ifdefs to
check for ACPI and some amount of code duplication is also required due
to the way DT driver gets the information vs ACPI.

The beauty of software node framework is, it supports adding properties
and also is a supported fwnode type in __irq_domain_create(). So, if we
can create the fwnode for these irqchip using software node, we can
attach the same properties and the actual irqchip driver which uses the
fwnode doesn't need to have any ACPI vs DT checks. Same driver will work
seamlessly on both DT and ACPI platforms.  But the challenge is,
currently swnode expects to be created with sysfs which won't be
available during early boot when irqchip drivers need to be probed. So,
adding support to create without dependency on sysfs help us to reuse
the same framework for irqchip use case also.

Apologies for not descriptive in the commit message. Please let us know
your feedback on this approach.

> > This helps in keeping the drivers same for both
> > DT and ACPI. To enable this, enhance the swnode framework so
> > that it can be created early during boot without dependency
> > on sysfs.
> 
> ...
> 
> > -	swnode->kobj.kset = swnode_kset;
> > +	swnode->kobj.kset = (!early) ? swnode_kset : NULL;
> 
> Too many unneeded characters. Why parentheses? Why negative check?
> 
Sure, will update in next version.

> ...
> 
> > +	if (early) {
> > +		ret = 0;
> > +		kobject_init(&swnode->kobj, &software_node_type_early);
> > +		swnode->kobj.parent = parent ? &parent->kobj : NULL;
> > +		if (node->name)
> > +			ret = kobject_set_name(&swnode->kobj,
> > +					       "%s", node->name);
> > +		else
> > +			ret = kobject_set_name(&swnode->kobj,
> > +					       "node%d", swnode->id);
> > +		if (!ret) {
> > +			spin_lock(&swnode_early_lock);
> > +			list_add_tail(&swnode->early, &swnode_early_list);
> > +			spin_unlock(&swnode_early_lock);
> > +		}
> > +	} else {
> > +		if (node->name)
> > +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> > +						   parent ? &parent->kobj : NULL,
> 
> This looks like have a duplication.
> 
> > +						   "%s", node->name);
> > +		else
> > +			ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> > +						   parent ? &parent->kobj : NULL,
> > +						   "node%d", swnode->id);
> > +	}
> 
> Maybe it's possible to refactor this piece to be more compact?
> 
The issue is, kobject_init_and_add() expects sysfs. Let me try to
compact this in next version. Thanks!
> ...
> 
> > -	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0));
> > +	return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0));
> 
> In one case you use boolean, here is unsigned int for early flag, why is the
> inconsistency added?
> 
Yeah, it should be bool. Let me fix it in next version.

> ...
> 
> > -struct fwnode_handle *
> > -fwnode_create_software_node(const struct property_entry *properties,
> > -			    const struct fwnode_handle *parent)
> > +static struct fwnode_handle *
> > +fwnode_create_software_node_common(const struct property_entry *properties,
> > +				   const struct fwnode_handle *parent,
> > +				   bool early)
> 
> Why would you need this API in early stages?
> 
Hope I answered the question above. 

Thanks!
Sunil

Powered by blists - more mailing lists