> -----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... > > > > 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
