On 25/06/18 08:32, Laszlo Ersek wrote:
Hi Laszlo,
As any class defining explicit_ofw_unit_address() has explicitly requested a
specialised behaviour then it should be used in preference to the default
implementation rather than being used as a fallback.
I disagree about the last paragraph, when put like this. I don't
disagree with the *goal* of the patch, however the original
justification for explicit_ofw_unit_address() was different.
It was meant as a fallback for distinguishing sysbus devices when those
sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
MMIO/PIO-based identification was not "right", the issue was that unique
identification was impossible in the absence of such resources. Please
see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
for SysBusDeviceClass", 2015-06-23).
I don't have anything against repurposing explicit_ofw_unit_address()
like this -- as long as you check that it doesn't change behavior for
existing devices -- it's just that we shouldn't justify the new purpose
with the original intent. The original intent was different.
I suggest stating, "we can have explicit_ofw_unit_address() take
priority in a backwards-compatible manner, because no sysbus device
currently has both explicit_ofw_unit_address() and MMIO/PIO resources".
Thanks for the feedback, I'm more than happy to update the commit
message to better describe the original intent of the patch. How does
the following sound to you?
Some SysBusDevices either use sysbus_init_mmio() without
sysbus_mmio_map() or the first MMIO memory region doesn't represent the
bus address, causing a firmware device path with an invalid address to
be generated.
SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
method that can be used to override this process, but it is only
considered as a fallback option meaning that any existing MMIO memory
regions still take priority whilst determining the firmware device address.
There is currently only one user of explicit_ofw_unit_address() and that
is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
defined. This enables us to allow explicit_ofw_unit_address() to take
priority without affecting backwards compatibility, allowing the address
to be customised as required.
(Obviously checking the validity of this statement is up to you; I'm
just suggesting what I'd see as one more precise explanation.)
Yes, it seems correct to me - grep tells me the PXB device is the only
user of explicit_ofw_unit_address() in the whole code base, and there
are no sysbus_init_*() functions anywhere within pci_expander_bridge.c.
ATB,
Mark.
