On 09/22/2016 11:20 PM, Gabriele Paoloni wrote:
-----Original Message-----
From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
Sent: 22 September 2016 15:59
To: Gabriele Paoloni
Cc: zhichang; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
devicetree@xxxxxxxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; minyard@xxxxxxx;
linux-pci@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; John Garry;
will.deacon@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yuanzhichang;
Linuxarm; xuwei (O); linux-serial@xxxxxxxxxxxxxxx;
benh@xxxxxxxxxxxxxxxxxxx; zourongrong@xxxxxxxxx; liviu.dudau@xxxxxxx;
kantyzc@xxxxxxx
Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on
Hip06
On Thursday, September 22, 2016 2:47:14 PM CEST Gabriele Paoloni wrote:
static int of_empty_ranges_quirk(struct device_node *np)
{
if (IS_ENABLED(CONFIG_PPC)) {
@@ -503,7 +512,7 @@ static int of_translate_one(struct
device_node
*parent, struct of_bus *bus,
* This code is only enabled on powerpc. --gcl
*/
ranges = of_get_property(parent, rprop, &rlen);
- if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
+ if (ranges == NULL && !of_empty_ranges_quirk(parent) &&
!of_isa_indirect_io(parent)) {
pr_debug("OF: no ranges; cannot translate\n");
return 1;
}
I don't see what effect that would have. What do you want to
achieve with this?
If I read the code correctly adding the function above would end
up in a 1:1 mapping:
http://lxr.free-electrons.com/source/drivers/of/address.c#L513
so taddr will be assigned with the cpu address space specified
in the children nodes of LPC and we are not using a quirk function
(we are just checking that we have the indirect io assigned and
that we are on a ISA bus). Now probably there is a nit in my
code sketch where of_isa_indirect_io should be probably an
architecture
specific function...
But the point is that it would then return an incorrect address,
which in the worst case could be the same as another I/O space
if that happens to be at CPU address zero.
If we do not touch __of_address_to_resource after taddr is returned
by of_translate_address we will check for (flags & IORESOURCE_IO),
then we call pci_address_to_pio to retrieve the unique token (remember
that LPC driver will register the LPC io range to pci io_range_list).
I do not think that we can have any conflict with any other I/O space
as pci_register_io_range will guarantee that the LPC range does not
overlap with any other I/O range...
If we don't bypass the calling of pci_address_to_pio after
of_translate_address,
there should no conflict between LPC logical IO range and other logical
IO ranges
of other devices.
I guess Arnd want to skip all the translation for our LPC IO address.
But if we do it
like that, it seems we can't avoid the possible conflict with the
logical IO ranges of
PCI host bridges without any changes on the pci_register_io_range and
pci_address_to_pio.
Because two completely separate I/O spaces are created without
synchronization.
Best,
Zhichang
I think all we need from this function is to return '1' if
we hit an ISA I/O window, and that should happen for the two
interesting cases, either no 'ranges' at all, or no translation
for the range in question, so that __of_translate_address can
return OF_BAD_ADDR, and we can enter the special case
handling in the caller, that handles it like
I don't think this is very right as you may fail for different
reasons other than a missing range property, e.g:
http://lxr.free-electrons.com/source/drivers/of/address.c#L575
And even if the only failure case was a missing range if in the
future __of_translate_address had to be reworked we would again
make a wrong assumption...you get my point?
The newly introduced function would clearly have to make
some sanity checks. The idea is that treat the case of
not being able to translate a bus specific I/O address
into a CPU address literally and fall back to another method
of translating that address.
This matches my mental model of how we find the resource:
- start with the bus address
- try to translate that into a CPU address
- if we arrive at a CPU physical address for IORESOURCE_MEM, use that
- if we arrive at a CPU physical address for IORESOURCE_IO, translate
that into a Linux IORESOURCE_IO token
- if there is no valid CPU physical address, try to translate
the address into an IORESOURCE_IO using the ISA accessor
- if that fails too, give up.
If you try to fake a CPU physical address inbetween, it just
gets more confusing.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
