Message-ID: <aXCQ9jcpheMub8u3@smile.fi.intel.com>
Date: Wed, 21 Jan 2026 10:40:22 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Michal Simek <michal.simek@....com>
Cc: Mark Brown <broonie@...nel.org>,
	Abdurrahman Hussain <abdurrahman@...thop.ai>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] spi: xilinx: use device property accessors.

On Wed, Jan 21, 2026 at 09:11:04AM +0100, Michal Simek wrote:
> On 1/20/26 22:38, Andy Shevchenko wrote:
> > On Tue, Jan 20, 2026 at 11:28:50PM +0200, Andy Shevchenko wrote:
> > > On Tue, Jan 20, 2026 at 09:04:58PM +0000, Mark Brown wrote:
> > > > On Tue, Jan 20, 2026 at 11:11:44AM -0800, Abdurrahman Hussain wrote:
> > > > > > On Jan 20, 2026, at 10:45 AM, Mark Brown <broonie@...nel.org> wrote:
> > > > > > On Mon, Jan 19, 2026 at 04:20:06PM -0800, Abdurrahman Hussain wrote:
> > > > 
> > > > Let me once more renew my plea:
> > > > 
> > > > > > To repeat once again:
> > > > 
> > > > > > | Please fix your mail client to word wrap within paragraphs at something
> > > > > > | substantially less than 80 columns.  Doing this makes your messages much
> > > > > > | easier to read and reply to.
> > > > 
> > > > > > To drivers that are used on ACPI systems, yes.  Many devices wouldn't be
> > > > > > used on ACPI systems, or would be expected to be exposed differently
> > > > > > (for example, hidden behind AML).
> > > > 
> > > > > This is not for a normal off the shelf server. In our case we are building an embedded
> > > > > switch with an AMD CPU and Xilinx FPGAs that happens to use EDK2 based BIOS and ACPI.
> > > > 
> > > > Sure, AFAICT it's mostly a PCI card with a bunch of stuff on it from a
> > > > software point of view.
> > > > 
> > > > > > > I am just trying to get this 2-line small change merged so we can start using the standard spi-xilinx driver today. I am not trying to boil the ocean.
> > > > 
> > > > > > I mean, adding a HID wouldn't take substantially more code.
> > > > 
> > > > > We could, but we don’t own the Xilinx IP blocks. Are we not justified in using PRP0001
> > > > > hack until the driver owner adds the HIDs? Wasn’t PRP0001 created as an escape hatch for
> > > > > these kind of scenarios?
> > > > 
> > > > No, it's more there for the cases where embedded ACPI systems need to
> > > > import non-trivial DT bindings so they can avoid having to respecify
> > > > things that ACPI really doesn't cope with or for local hacks.  See
> > > > Andy's reply earlier in the thread:
> > > > 
> > > >     https://lore.kernel.org/r/aW9JihlsjnJ-uBul@black.igk.intel.com
> > > > 
> > > > AFAICT for ACPI the HID assigment is a bit of a free for all in practice
> > > > - board vendors generally seem perfectly happy to just pick something if
> > > > the silicon vendor didn't do something.  Just look at all the parts with
> > > > INTxxxx IDs!  That said Michal is on the thread so hopefully that's not
> > > 
> > > INTxxxx was a (historical) mistake, but look at the correct one INTCxxxx
> > > which has listed several components Intel doesn't own. Because ACPI HID
> > > it's not only about the component in use, it's also about integration of
> > > that component in the platform environment. Hence it might require (platform)
> > > specific quirks.
> > 
> > Btw, you can look at the MIPI I3C HCI case. MIPI as an owner allocated generic
> > ID (which is usually represented as _CID in ACPI), AMD allocated their own one
> > _HID (compatible with _CID) exactly for the purpose of having platform quirks.
> > 
> > TL;DR: if you are 100% sure you have no HW integration issues, requirements, etc
> > and everything works as is, you can use Xilinx allocated id (assuming it will come)
> > for the given IP and use it directly as _HID, otherwise use that as _CID and
> > allocate your own for _HID.
> 
> I got in touch with respective AMD team about ACPI ID allocation. I also
> don't think it is going to be a problem to get unique one. And I expect
> device properties should pretty much match DT property names to be able to
> use the same device_property_read* helper function to read them. Andy:
> Correct?

Yes, the main issue I see is the enumeration by PRP0001 instead of by ACPI
_HID. Also note, whatever ACPI has supported in _CRS (resources) should be
used that way (memory, interrupts, DMA channels, etc).

> I am still trying to wrap my head around all these possible solutions for
> this problem or if we can solve it in a more generic way.
> 
> I can't see any problem with patches which are switching from
> of_property_read* to device_property_read.

Technically there is no issue with that, indeed.

> If driver also works without IRQ
> I think it is fine to make irq optional (which is 2/3 patch in this series).
> The patch 1/3 is the same as mine I sent in past
> https://lore.kernel.org/all/a527f5adffc6efe4c1ad2ccc40e1e095d73efe74.1749027112.git.michal.simek@amd.com/
> and it was rejected by Rob.

I think Rob is correct. But what's the problem to describe IRQ natively in _CRS
in ACPI via Interrupt() or GpioInt() resource? You mean there is no actual wire
connection to it?

> Andrew: thanks for sharing link to pcie driver with DT which is one solution
> which requires writing one PCIe driver and that's pretty much it.
> Obviously enable DT on x86 but that's not a problem for product based on
> embedded x86. But it can be a problem for standard distributions.
> 
> Then this ACPI way where it is not clear to me yet how to get information
> about clocks. The i2c Abdurrahman's patch is making clk optional
> https://lore.kernel.org/r/20260115002846.25389-1-abdurrahman@nexthop.ai
> But input_clk is used in reinit code that's why maybe it is working for the
> first time but in case of error not sure if driver really works properly.
> On PCIe clock for spi/i2c is likely only one and ACPI device property can be
> created to pass that information and call devm_clk_get_enabled() only in DT
> case.

On ACPI the clock are usually managed by firmware, but in new specification
we also have ClockInput() resource. I dunno if there is still a gap between
ACPICA and CCF to integrate that.

> And then you have DFL (drivers/fpga/dfl*) and aux bus
> (drivers/misc/keba/cp500.c) which pretty much targets similar setup.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists