Message-ID:
 <OS8PR06MB7541516DD4FDEBD72A764042F25FA@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Wed, 23 Jul 2025 06:02:20 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Joel Stanley <joel@....id.au>, Andrew Jeffery <andrew@...econstruct.com.au>,
	Kevin Chen <kevin_chen@...eedtech.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>
Subject: RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700
 INTC0/INTC1 routing/protection display

> Subject: Re: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700
> INTC0/INTC1 routing/protection display
> 
> On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
> 
> The subsystem prefix is made up. See:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-su
> bmission-notes

Will prefix update to
Irqchip/aspeed-intc: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

> 
> > AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only include
> > the interrupt controller child node ("aspeed,ast2700-intc-ic"), but
> > also provide interrupt routing and register protection features.
> 
> > This patch adds debugfs entries for interrupt routing and protection
> 
> # git grep 'This patch' Documentation/process

Modify to
Adds debugfs entries for interrupt routing and protection status for AST2700 INTC0/INTC1.
> 
> > status for AST2700 INTC0/INTC1.
> >
> > - Register platform driver for "aspeed,ast2700-intc0" and
> > "aspeed,ast2700-intc1" compatible nodes.
> > - Add show_routing/show_prot callbacks for both intc0 and intc1,
> > displaying current interrupt routing and protection register status.
> > - Expose routing/protection information via debugfs for debugging  and
> > validation.
> > +
> > +struct aspeed_intc {
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	struct dentry *dbg_root;
> > +	int (*show_routing)(struct seq_file *s, void *unused);
> > +	int (*show_prot)(struct seq_file *s, void *unused); };
> 
> See the chapter about struct declarations and initializers in the documentation
> I linked to above.

Sorry, I don't see the struct "> > +	int (*show_prot)(struct seq_file *s, void *unused); };"
My original submit is following, it should ok. Am I right?

+struct aspeed_intc {
+	void __iomem *base;
+	struct device *dev;
+	struct dentry *dbg_root;
+	int (*show_routing)(struct seq_file *s, void *unused);
+	int (*show_prot)(struct seq_file *s, void *unused);
+};
https://www.spinics.net/lists/kernel/msg5776957.html
> 
> > +static int aspeed_intc1_show_prot(struct seq_file *s, void *unused) {
> > +	struct aspeed_intc *intc = s->private;
> > +	u32 prot = readl(intc->base);
> > +
> > +	seq_printf(s, "INTC1: 0x%08x\n", prot);
> > +
> > +	static const char * const prot_bits[] = {
> > +		"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP
> only",
> > +		"pprot_ssp: Protect INTC180~1D0 write by SSP only",
> > +		"pprot_tsp: Protect INTC200~250 write by TSP only",
> > +		"pprot_reg_prot: Protect INTC080~0D4 to be read only",
> > +		"pprot_regrd: Protect INTC080~0D4 to be read protected",
> > +		"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP
> only",
> > +		"pprot_regrd3: Protect INTC180~1D0 read by SSP only",
> > +		"pprot_regrd4: Protect INTC200~250 read by TSP only",
> > +		"pprot_mcu0: Protect INTC010,014 write by MCU0 only",
> > +		"pprot_regrd5: Protect INTC010,014 read by MCU0 only",
> > +		"pprot_treg: Protect INTC040~054 to be read protected"
> > +	};
> > +
> > +	for (int i = 0; i < 11; i++)
> > +		seq_printf(s, "  [%2d] %s: %s\n", i, prot_bits[i],
> > +			   (prot & BIT(i)) ? "Enable" : "Disable");
> > +	return 0;
> > +}
> 
> I really have to ask, what the value of this metric ton of string constants and
> decoding is. This is a debug interface, which is intended for developers and
> experts. As these are hardware bits, which are immutable, it's completely
> sufficient to print out the raw values here and let the engineer decode it, no?

The reason for the string decoding was to make debug output more friendly for quick diagnosis during bring-up,
If it is not accepted, I will revise the patch to remove the extra string decoding and only output the raw values.

> 
> > +static int aspeed_intc_probe(struct platform_device *pdev) {
> > +	struct aspeed_intc *intc;
> > +	struct resource *res;
> > +
> > +	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
> > +	if (!intc)
> > +		return -ENOMEM;
> > +	intc->dev = &pdev->dev;
> 
> intc->dev is not used anywhere.

Will remove it. 

> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	intc->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(intc->base))
> > +		return PTR_ERR(intc->base);
> > +
> > +	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
> {
> > +		intc->show_routing = aspeed_intc0_show_routing;
> > +		intc->show_prot    = aspeed_intc0_show_prot;
> > +	} else if (of_device_is_compatible(pdev->dev.of_node,
> "aspeed,ast2700-intc1")) {
> > +		intc->show_routing = aspeed_intc1_show_routing;
> > +		intc->show_prot    = aspeed_intc1_show_prot;
> > +	} else {
> > +		intc->show_routing = NULL;
> > +		intc->show_prot = NULL;
> 
> What's the point of creating the debugfs entry instead of bailing out?

Yes, will update to
return -ENODEV;
> 
> > +	}
> > +
> > +	platform_set_drvdata(pdev, intc);
> > +
> > +	intc->dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
> 
> Why storing this? It's just used for setting up the debugfs entry, no?

I'll update the code to use a local variable, by following.

struct dentry *dbg_root;
dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
if (dbg_root) {
    debugfs_create_file("routing", 0400, dbg_root, intc, &aspeed_intc_routing_fops);
    debugfs_create_file("protection", 0400, dbg_root, intc, &aspeed_intc_prot_fops);
}
> 
> > +	if (intc->dbg_root) {
> > +		debugfs_create_file("routing", 0400, intc->dbg_root, intc,
> > +				    &aspeed_intc_routing_fops);
> > +		debugfs_create_file("protection", 0400, intc->dbg_root, intc,
> > +				    &aspeed_intc_prot_fops);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_intc_of_match[] = {
> > +	{ .compatible = "aspeed,ast2700-intc0", },
> > +	{ .compatible = "aspeed,ast2700-intc1", },
> > +	{},
> > +};
> > +
> > +static struct platform_driver aspeed_intc_driver = {
> > +	.probe  = aspeed_intc_probe,
> > +	.driver = {
> > +		.name = "ast2700-intc",
> > +		.of_match_table = aspeed_intc_of_match,
> > +	},
> > +};
> > +builtin_platform_driver(aspeed_intc_driver);
> 
> Why has this to be builtin and not a module? It has zero dependencies on the
> existing code in this file, right?
> Just stick it into a seperate source file and make it modular with a seperate
> Kconfig switch. No point in carrying this code as built-in in multi-platform
> builds.
> 
> This whole platform driver muck is just there to expose the routing and
> protection registers in debugfs even if debugfs is disabled. Seriously?
> 
OK. I will do it in separate source c file and make it modular with Kconfig.
And also depends on DEBUG_FS
Thanks for guidance.


> It's completely disconnected from the interrupt delivery chain as far as the
> kernel is concerned, i.e. it does not provide a interrupt domain/chip. So that
> interface dumps just some register values with a lot of effort and leaves it to
> the user to decode which actual linux interrupt in the real intc-ic interrupt
> domains is affected, right?
> 
> I'm still failing to see the value of all of this. As the kernel does not even
> modify these registers, you are basically implementing a dump facility for
> decoding what the firmware put into those registers, right?
> 
> I don't have a strong opinion about it, but if it has a value, then all of this can
> be done with way smaller code by just dumping the raw register values all in
> one go. No need for two files and string encoding. That's what user space is
> for.
> 
Thanks, I will send a new version with a modular driver and a simplified debugfs interface as you suggested.

> Something like the completely uncompiled below, which I cobbled together
> quickly for illustration. You get the idea.
> 
> Thanks,
> 
>         tglx
> ---
> 
> #define INTC_TYPE_C0	0
> #define INTC_TYPE_C1	1
> 
> struct aspeed_intc {
> 	void __iomem	*base;
> 	unsigned int	type;
> };
> 
> const struct aspeed_intc_data {
> 	char		*name;
> 	unsigned int	groups;
> 	unsigned int	prot_offs;
> 	unsigned int	rout_offs;
> 	unsigned int	rout_gap;
> } aspeed_intc_data[2] = {
> 	{
> 		.name		= "INTC0",
> 		.groups		= 4,
> 		.prot_offs	= 0x40,
> 		.rout_offs	= 0x200,
> 		.rout_gap	= 0x100,
> 	},
> 	{
> 		.name		= "INTC1",
> 		.groups		= 6,
> 		.prot_offs	= 0x0,
> 		.rout_offs	= 0x80,
> 		.rout_gap	= 0x20,
> 	},
> };
> 
> static int aspeed_intc_show(struct seq_file *m, void *unused) {
> 	struct aspeed_intc *intc = m->private;
> 	const struct aspeed_intc_data *d = &aspeed_intc_data[intc->type];
> 	void __iomem *base = intc->base;
> 
> 	seq_printf(m, "%s\n", d->name)
> 	seq_printf(m, "P: 0x%08x\n", readl(base + d->prot_offs));
> 
> 	base += d->rout_offs;
> 	for (unsigned int i = 0; i < d->groups; i++, base += 4) {
> 		seq_printf(m, "R%d: 0x%08x 0x%08x 0x%08x\n", i, readl(base),
> 			   readl(base + d->rout_gap), readl(base + 2 * d->rout_gap));
> 	}
> 	return 0;
> }
> DEFINE_SHOW_ATTRIBUTE(aspeed_intc);
> 
> static int aspeed_intc_probe(struct platform_device *pdev) {
> 	struct aspeed_intc *intc;
> 	struct resource *res;
> 	struct dentry *dir;
> 
> 	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
> 	if (!intc)
> 		return -ENOMEM;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	intc->base = devm_ioremap_resource(&pdev->dev, res);
> 	if (IS_ERR(intc->base))
> 		return PTR_ERR(intc->base);
> 
> 	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
> 		intc->type = INTC_TYPE_C0;
> 	else if (of_device_is_compatible(pdev->dev.of_node,
> "aspeed,ast2700-intc1"))
> 		intc->type = INTC_TYPE_C1;
> 	else
> 		return -ENOTSUPP;
> 
> 	platform_set_drvdata(pdev, intc);
> 
> 	dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
> 	debugfs_create_file("intc_regs", 0400, dir, intc, &aspeed_intc_fops);
> 	return 0;
> }
> 
> static const struct of_device_id aspeed_intc_of_match[] = {
> 	{ .compatible	= "aspeed,ast2700-intc0", },
> 	{ .compatible	= "aspeed,ast2700-intc1", },
> 	{ },
> };
> MODULE_DEVICE_TABLE(of, aspeed_intc_of_match);
> 
> static struct platform_driver aspeed_intc_driver = {
> 	.probe  = aspeed_intc_probe,
> 	.driver = {
> 		.name		= "ast2700-intc",
> 		.of_match_table	= aspeed_intc_of_match,
> 	},
> };
> module_platform_driver(aspeed_intc_driver);

Powered by blists - more mailing lists