| Message ID | 9178320.n4yHmfyPA3@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 22 September 2016 13:15 > To: Gabriele Paoloni > Cc: zhichang; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > kantyzc@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni > wrote: > > > > I think extending of_empty_ranges_quirk() may be a reasonable > > > solution. > > > > What do you think Arnd? > > > > > > I don't really like that idea, that quirk is meant to work around > > > broken DTs, but we can just make the DT valid and implement the > > > code properly. > > > > Ok I understand your point where it is not right to use > of_empty_ranges_quirk() > > As a quirk is used to work around broken HW or broken FW (as in this > case) > > rather than to fix code > > > > What about the following? I think adding the check you suggested next > to > > of_empty_ranges_quirk() is adding the case we need in the right point > (thus > > avoiding any duplication) > > > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct > device_node *np) > > return NULL; > > } > > > > +static inline int of_isa_indirect_io(struct device_node *np) > > +{ > > + /* > > + * check if the current node is an isa bus and if indirectio > operation > > + * are registered > > + */ > > + return (of_bus_isa_match(np) && arm64_extio_ops); > > +} > > + > > 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... > > 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? Thanks Gab > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903fe9d2..a18d96843fae 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct > device_node *dev, > if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0) > return -EINVAL; > taddr = of_translate_address(dev, addrp); > - if (taddr == OF_BAD_ADDR) > - return -EINVAL; > memset(r, 0, sizeof(struct resource)); > + > if (flags & IORESOURCE_IO) { > unsigned long port; > - port = pci_address_to_pio(taddr); > + > + if (taddr == OF_BAD_ADDR) > + port = arch_of_address_to_pio(dev, addrp) > + else > + port = pci_address_to_pio(taddr); > + > if (port == (unsigned long)-1) > return -EINVAL; > r->start = port; > r->end = port + size - 1; > } else { > + if (taddr == OF_BAD_ADDR) > + return -EINVAL; > + > r->start = taddr; > r->end = taddr + size - 1; > } > > > > Arnd
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. > > 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
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 22 September 2016 15:59 > To: Gabriele Paoloni > Cc: zhichang; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > kantyzc@163.com > 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
On 09/22/2016 11:20 PM, Gabriele Paoloni wrote: > >> -----Original Message----- >> From: Arnd Bergmann [mailto:arnd@arndb.de] >> Sent: 22 September 2016 15:59 >> To: Gabriele Paoloni >> Cc: zhichang; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; >> linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; >> will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; >> Linuxarm; xuwei (O); linux-serial@vger.kernel.org; >> benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; >> kantyzc@163.com >> 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
On 09/22/2016 08:14 PM, Arnd Bergmann wrote: > On Thursday, September 22, 2016 11:55:45 AM CEST Gabriele Paoloni wrote: >>>> I think extending of_empty_ranges_quirk() may be a reasonable >>> solution. >>>> What do you think Arnd? >>> I don't really like that idea, that quirk is meant to work around >>> broken DTs, but we can just make the DT valid and implement the >>> code properly. >> Ok I understand your point where it is not right to use of_empty_ranges_quirk() >> As a quirk is used to work around broken HW or broken FW (as in this case) >> rather than to fix code >> >> What about the following? I think adding the check you suggested next to >> of_empty_ranges_quirk() is adding the case we need in the right point (thus >> avoiding any duplication) >> >> --- a/drivers/of/address.c >> +++ b/drivers/of/address.c >> @@ -457,6 +457,15 @@ static struct of_bus *of_match_bus(struct device_node *np) >> return NULL; >> } >> >> +static inline int of_isa_indirect_io(struct device_node *np) >> +{ >> + /* >> + * check if the current node is an isa bus and if indirectio operation >> + * are registered >> + */ >> + return (of_bus_isa_match(np) && arm64_extio_ops); >> +} >> + >> 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? > > 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 > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903fe9d2..a18d96843fae 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev, > if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0) > return -EINVAL; > taddr = of_translate_address(dev, addrp); > - if (taddr == OF_BAD_ADDR) > - return -EINVAL; > memset(r, 0, sizeof(struct resource)); > + > if (flags & IORESOURCE_IO) { > unsigned long port; > - port = pci_address_to_pio(taddr); > + > + if (taddr == OF_BAD_ADDR) > + port = arch_of_address_to_pio(dev, addrp) > + else > + port = pci_address_to_pio(taddr); > + > if (port == (unsigned long)-1) > return -EINVAL; > r->start = port; > r->end = port + size - 1; > } else { > + if (taddr == OF_BAD_ADDR) > + return -EINVAL; > + > r->start = taddr; > r->end = taddr + size - 1; > } > For this patch sketch, I have a question. Do we call pci_address_to_pio in arch_of_address_to_pio to get the corresponding logical IO port for LPC?? If we don't, it seems the LPC specific IO address will conflict with PCI host bridges' logical IO. Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is normal for ISA similar devices), after arch_of_address_to_pio(), the r->start will be set as 0x100, r->end will be set as 0x3FF. And if there is one PCI host bridge who request a IO window size over 0x400 at the same time, the corresponding r->start and r->end will be set as 0x0, 0x3FF after of_address_to_resource for this host bridge. Then the IO conflict happens. cheers, Zhichang > > Arnd
On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: > For this patch sketch, I have a question. > Do we call pci_address_to_pio in arch_of_address_to_pio to get the > corresponding logical IO port > for LPC?? No, of course not, that would be silly: The argument to pci_address_to_pio() is a phys_addr_t, and we we don't have one because there is no address associated with your PIO, that is the entire point of your driver! Also, we already know the mapping because this is what the inb/outb workaround is looking at, so there is absolutely no reason to call it either. > If we don't, it seems the LPC specific IO address will conflict with PCI > host bridges' logical IO. > > Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is > normal for ISA similar > devices), after arch_of_address_to_pio(), the r->start will be set as > 0x100, r->end will be set as > 0x3FF. And if there is one PCI host bridge who request a IO window size > over 0x400 at the same > time, the corresponding r->start and r->end will be set as 0x0, 0x3FF > after of_address_to_resource > for this host bridge. Then the IO conflict happens. You would still need to reserve some space in the io_range_list to avoid possible conflicts, which is a bit ugly with the current definition of pci_register_io_range, but I'm sure can be done. One way I can think of would be to change pci_register_io_range() to just return the logical port number directly (it already knows it!), and pass an invalid physical address (e.g. #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for invalid translations. Another alternative that just occurred to me would be to move the pci_address_to_pio() call from __of_address_to_resource() into of_bus_pci_translate() and then do the special handling for the ISA/LPC bus in of_bus_isa_translate(). Arnd
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 23 September 2016 10:52 > To: zhichang.yuan > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > kantyzc@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: > > For this patch sketch, I have a question. > > Do we call pci_address_to_pio in arch_of_address_to_pio to get the > > corresponding logical IO port > > for LPC?? > > > No, of course not, that would be silly: > > The argument to pci_address_to_pio() is a phys_addr_t, and we we don't > have one because there is no address associated with your PIO, that > is the entire point of your driver! > > Also, we already know the mapping because this is what the inb/outb > workaround is looking at, so there is absolutely no reason to call it > either. Ok assume that we do not call pci_address_to_pio() for the ISA bus... The LPC driver will register its phys address range in io_range_list, then the IPMI driver probe will retrieve its physical address calling of_address_to_resource and will use the indirect io to access this address. From the perspective of the indirect IO function the input parameter is an unsigned long addr that (now) can be either: 1) an IO token coming from a legacy pci device 2) a phys address that lives on the LPC bus These are conceptually two separate address spaces (and actually they both start from 0). If the input parameter can live on different address spaces that are overlapped, even if I save the used LPC range in arm64_extio_ops->start/end there is no way for the indirect IO to tell if the input parameter is an I/O token or a phys address that belongs to LPC... Am I missing something? Thanks Gab > > > If we don't, it seems the LPC specific IO address will conflict with > PCI > > host bridges' logical IO. > > > > Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is > > normal for ISA similar > > devices), after arch_of_address_to_pio(), the r->start will be set as > > 0x100, r->end will be set as > > 0x3FF. And if there is one PCI host bridge who request a IO window > size > > over 0x400 at the same > > time, the corresponding r->start and r->end will be set as 0x0, > 0x3FF > > after of_address_to_resource > > for this host bridge. Then the IO conflict happens. > > You would still need to reserve some space in the io_range_list > to avoid possible conflicts, which is a bit ugly with the current > definition of pci_register_io_range, but I'm sure can be done. > > One way I can think of would be to change pci_register_io_range() > to just return the logical port number directly (it already > knows it!), and pass an invalid physical address (e.g. > #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for > invalid translations. > > Another alternative that just occurred to me would be to move > the pci_address_to_pio() call from __of_address_to_resource() > into of_bus_pci_translate() and then do the special handling > for the ISA/LPC bus in of_bus_isa_translate(). > > Arnd
On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote: > Hi Arnd > > > -----Original Message----- > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > Sent: 23 September 2016 10:52 > > To: zhichang.yuan > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > > kantyzc@163.com > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > > Hip06 > > > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: > > > For this patch sketch, I have a question. > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get the > > > corresponding logical IO port > > > for LPC?? > > > > > > No, of course not, that would be silly: > > > > The argument to pci_address_to_pio() is a phys_addr_t, and we we don't > > have one because there is no address associated with your PIO, that > > is the entire point of your driver! > > > > Also, we already know the mapping because this is what the inb/outb > > workaround is looking at, so there is absolutely no reason to call it > > either. > > Ok assume that we do not call pci_address_to_pio() for the ISA bus... > The LPC driver will register its phys address range in io_range_list, > then the IPMI driver probe will retrieve its physical address calling > of_address_to_resource and will use the indirect io to access this > address. > > From the perspective of the indirect IO function the input parameter > is an unsigned long addr that (now) can be either: > 1) an IO token coming from a legacy pci device > 2) a phys address that lives on the LPC bus > > These are conceptually two separate address spaces (and actually they > both start from 0). Why? Any IORESOURCE_IO address always refers to the logical I/O port range in Linux, not the physical address that is used on a bus. > If the input parameter can live on different address spaces that are > overlapped, even if I save the used LPC range in arm64_extio_ops->start/end > there is no way for the indirect IO to tell if the input parameter is > an I/O token or a phys address that belongs to LPC... The start address is the offset: if you get an address between 'start' and 'end', you subtract the 'start' from it, and use that to call the registered driver function. That works because we can safely assume that the bus address range that the LPC driver registers starts zero. Arnd
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 23 September 2016 14:43 > To: Gabriele Paoloni > Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > kantyzc@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote: > > Hi Arnd > > > > > -----Original Message----- > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > Sent: 23 September 2016 10:52 > > > To: zhichang.yuan > > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; > minyard@acm.org; > > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > > > benh@kernel.crashing.org; zourongrong@gmail.com; > liviu.dudau@arm.com; > > > kantyzc@163.com > > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > > > Hip06 > > > > > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: > > > > For this patch sketch, I have a question. > > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get > the > > > > corresponding logical IO port > > > > for LPC?? > > > > > > > > > No, of course not, that would be silly: > > > > > > The argument to pci_address_to_pio() is a phys_addr_t, and we we > don't > > > have one because there is no address associated with your PIO, that > > > is the entire point of your driver! > > > > > > Also, we already know the mapping because this is what the inb/outb > > > workaround is looking at, so there is absolutely no reason to call > it > > > either. > > > > Ok assume that we do not call pci_address_to_pio() for the ISA bus... > > The LPC driver will register its phys address range in io_range_list, > > then the IPMI driver probe will retrieve its physical address calling > > of_address_to_resource and will use the indirect io to access this > > address. > > > > From the perspective of the indirect IO function the input parameter > > is an unsigned long addr that (now) can be either: > > 1) an IO token coming from a legacy pci device > > 2) a phys address that lives on the LPC bus > > > > These are conceptually two separate address spaces (and actually they > > both start from 0). > > Why? Any IORESOURCE_IO address always refers to the logical I/O port > range in Linux, not the physical address that is used on a bus. If I read the code correctly when you get an I/O token you just add it to PCI_IOBASE. This is enough since pci_remap_iospace set the virtual address to PCI_IOBASE + the I/O token offset; so we can read/write to vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly to the respective PCI cpu address (that is set in the I/O range property of the host controller) In the patchset accessors LPC operates directly on the cpu addresses and the input parameter of the accessors can be either an IO token or a cpu address +static inline void outb(u8 value, unsigned long addr) +{ +#ifdef CONFIG_ARM64_INDIRECT_PIO + if (arm64_extio_ops && arm64_extio_ops->start <= addr && + addr <= arm64_extio_ops->end) Here below we operate on cpu address + extio_outb(value, addr); + else +#endif In the case below we have an I/O token added to PCI_IOBASE to calculate the virtual address + writeb(value, PCI_IOBASE + addr); +} My point is that if do not call pci_address_to_pio() in __of_address_to_resource for the ISA LPC exception then the accessors are called either by passing an IO token or a cpu address...and from the accessors perspective we do not know... Thanks Gab > > > If the input parameter can live on different address spaces that are > > overlapped, even if I save the used LPC range in arm64_extio_ops- > >start/end > > there is no way for the indirect IO to tell if the input parameter is > > an I/O token or a phys address that belongs to LPC... > > The start address is the offset: if you get an address between 'start' > and 'end', you subtract the 'start' from it, and use that to call > the registered driver function. That works because we can safely > assume that the bus address range that the LPC driver registers starts > zero. > > Arnd
On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote: > > > > From the perspective of the indirect IO function the input parameter > > > is an unsigned long addr that (now) can be either: > > > 1) an IO token coming from a legacy pci device > > > 2) a phys address that lives on the LPC bus > > > > > > These are conceptually two separate address spaces (and actually they > > > both start from 0). > > > > Why? Any IORESOURCE_IO address always refers to the logical I/O port > > range in Linux, not the physical address that is used on a bus. > > If I read the code correctly when you get an I/O token you just add it > to PCI_IOBASE. > This is enough since pci_remap_iospace set the virtual address to > PCI_IOBASE + the I/O token offset; so we can read/write to > vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly > to the respective PCI cpu address (that is set in the I/O range property > of the host controller) > > In the patchset accessors LPC operates directly on the cpu addresses > and the input parameter of the accessors can be either an IO token or > a cpu address > > +static inline void outb(u8 value, unsigned long addr) > +{ > +#ifdef CONFIG_ARM64_INDIRECT_PIO > + if (arm64_extio_ops && arm64_extio_ops->start <= addr && > + addr <= arm64_extio_ops->end) > > Here below we operate on cpu address > > + extio_outb(value, addr); > + else > +#endif I missed this bug earlier, this obviously needs to be arm64_extio_ops->outb(value, addr - arm64_extio_ops->start); or possibly arm64_extio_ops->outb(arm64_extio_ops, value, addr); as the outb function won't know what the offset is, but that needed to be fixed regardless. Arnd
Hi, Arnd, On 2016年09月23日 23:55, Arnd Bergmann wrote: > On Friday, September 23, 2016 2:59:55 PM CEST Gabriele Paoloni wrote: >> >>>> From the perspective of the indirect IO function the input parameter >>>> is an unsigned long addr that (now) can be either: >>>> 1) an IO token coming from a legacy pci device >>>> 2) a phys address that lives on the LPC bus >>>> >>>> These are conceptually two separate address spaces (and actually they >>>> both start from 0). >>> >>> Why? Any IORESOURCE_IO address always refers to the logical I/O port >>> range in Linux, not the physical address that is used on a bus. >> >> If I read the code correctly when you get an I/O token you just add it >> to PCI_IOBASE. >> This is enough since pci_remap_iospace set the virtual address to >> PCI_IOBASE + the I/O token offset; so we can read/write to >> vaddr = PCI_IOBASE + token as pci_remap_iospace has mapped it correctly >> to the respective PCI cpu address (that is set in the I/O range property >> of the host controller) >> >> In the patchset accessors LPC operates directly on the cpu addresses >> and the input parameter of the accessors can be either an IO token or >> a cpu address >> >> +static inline void outb(u8 value, unsigned long addr) >> +{ >> +#ifdef CONFIG_ARM64_INDIRECT_PIO >> + if (arm64_extio_ops && arm64_extio_ops->start <= addr && >> + addr <= arm64_extio_ops->end) >> >> Here below we operate on cpu address >> >> + extio_outb(value, addr); >> + else >> +#endif > > I missed this bug earlier, this obviously needs to be > > arm64_extio_ops->outb(value, addr - arm64_extio_ops->start); > > or possibly > > arm64_extio_ops->outb(arm64_extio_ops, value, addr); > > as the outb function won't know what the offset is, but > that needed to be fixed regardless. In V3, the outb is : void outb(u8 value, unsigned long addr) { if (!arm64_extio_ops || arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) writeb(value, PCI_IOBASE + addr); else if (arm64_extio_ops->pfout) arm64_extio_ops->pfout(arm64_extio_ops->devpara, addr + arm64_extio_ops->ptoffset, &value, sizeof(u8), 1); } here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address and the logical IO address, similar to the offset of primary address and secondary address in PCI bridge. But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI host bridge during its probing. cheers, Zhichang > > Arnd >
On Saturday, September 24, 2016 4:14:15 PM CEST zhichang wrote: > > In V3, the outb is : > > void outb(u8 value, unsigned long addr) > { > if (!arm64_extio_ops || arm64_extio_ops->start > addr || > arm64_extio_ops->end < addr) > writeb(value, PCI_IOBASE + addr); > else > if (arm64_extio_ops->pfout) > arm64_extio_ops->pfout(arm64_extio_ops->devpara, > addr + arm64_extio_ops->ptoffset, &value, > sizeof(u8), 1); > } > > here, arm64_extio_ops->ptoffset is the offset between the real legacy IO address > and the logical IO address, similar to the offset of primary address and > secondary address in PCI bridge. Ok, though we can probably simplify this by making the assumption that 'ptoffset' is the negative of 'start', as the bus we register should always start at port zero. > But in V3, LPC driver call pci_address_to_pio to request the logical IO as PCI > host bridge during its probing. Right, so this still needs to be fixed. Arnd
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 23 September 2016 14:43 > To: Gabriele Paoloni > Cc: zhichang.yuan; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; minyard@acm.org; > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > benh@kernel.crashing.org; zourongrong@gmail.com; liviu.dudau@arm.com; > kantyzc@163.com > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, September 23, 2016 10:23:30 AM CEST Gabriele Paoloni wrote: > > Hi Arnd > > > > > -----Original Message----- > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > Sent: 23 September 2016 10:52 > > > To: zhichang.yuan > > > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; lorenzo.pieralisi@arm.com; > minyard@acm.org; > > > linux-pci@vger.kernel.org; gregkh@linuxfoundation.org; John Garry; > > > will.deacon@arm.com; linux-kernel@vger.kernel.org; Yuanzhichang; > > > Linuxarm; xuwei (O); linux-serial@vger.kernel.org; > > > benh@kernel.crashing.org; zourongrong@gmail.com; > liviu.dudau@arm.com; > > > kantyzc@163.com > > > Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on > > > Hip06 > > > > > > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: > > > > For this patch sketch, I have a question. > > > > Do we call pci_address_to_pio in arch_of_address_to_pio to get > the > > > > corresponding logical IO port > > > > for LPC?? > > > > > > > > > No, of course not, that would be silly: > > > > > > The argument to pci_address_to_pio() is a phys_addr_t, and we we > don't > > > have one because there is no address associated with your PIO, that > > > is the entire point of your driver! > > > > > > Also, we already know the mapping because this is what the inb/outb > > > workaround is looking at, so there is absolutely no reason to call > it > > > either. > > > > Ok assume that we do not call pci_address_to_pio() for the ISA bus... > > The LPC driver will register its phys address range in io_range_list, > > then the IPMI driver probe will retrieve its physical address calling > > of_address_to_resource and will use the indirect io to access this > > address. > > > > From the perspective of the indirect IO function the input parameter > > is an unsigned long addr that (now) can be either: > > 1) an IO token coming from a legacy pci device > > 2) a phys address that lives on the LPC bus > > > > These are conceptually two separate address spaces (and actually they > > both start from 0). > > Why? Any IORESOURCE_IO address always refers to the logical I/O port > range in Linux, not the physical address that is used on a bus. > > > If the input parameter can live on different address spaces that are > > overlapped, even if I save the used LPC range in arm64_extio_ops- > >start/end > > there is no way for the indirect IO to tell if the input parameter is > > an I/O token or a phys address that belongs to LPC... > Assume that in the probe function the LPC drivers calls pci_register_io_range for the LPC cpu address range (0 to PCIBIOS_MIN_I0) and does not scan the children DT nodes. Consider for example the ipmi driver: When the reg property is read to retrieve the ipmi <<i/o port>> in http://lxr.free-electrons.com/source/drivers/char/ipmi/ipmi_si_intf.c#L2622 if we do not call pci_address_to_pio in __of_address_to_resource the input parameter of inb/outb will be the cpu address of the ipmi (not translated to a unique token id). So inb/outb at this stage can be called passing either a cpu address or a token io port. If we set arm64_extio_ops->start/end to 0 and PCIBIOS_MIN_I0 respectively we still cannot tell inside inb/outb if the passed address is a token or an LPC cpu address as the ipmi cpu address can overlap with another device I/O token... My suggestion is to call pci_address_to_pio even for devices living on the LPC bus; then in the LPC probe we set arm64_extio_ops->start/end to the I/O tokens that correspond to the LPC cpu address range (in the LPC probe function we call pci_address_to_pio after we have called pci_register_io_range); finally in inb/outb we know that we can get only an I/O token as input parameter and we check it against arm64_extio_ops->start/end to decide whether to call the LPC accessors or readb/writeb... > The start address is the offset: if you get an address between 'start' > and 'end', you subtract the 'start' from it, and use that to call > the registered driver function. That works because we can safely > assume that the bus address range that the LPC driver registers starts > zero. Sorry I cannot follow what you said here above: <<if you get an address between 'start' and 'end'>>...in which function? Thanks Gab > > Arnd
diff --git a/drivers/of/address.c b/drivers/of/address.c index 02b2903fe9d2..a18d96843fae 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -685,17 +685,24 @@ static int __of_address_to_resource(struct device_node *dev, if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0) return -EINVAL; taddr = of_translate_address(dev, addrp); - if (taddr == OF_BAD_ADDR) - return -EINVAL; memset(r, 0, sizeof(struct resource)); + if (flags & IORESOURCE_IO) { unsigned long port; - port = pci_address_to_pio(taddr); + + if (taddr == OF_BAD_ADDR) + port = arch_of_address_to_pio(dev, addrp) + else + port = pci_address_to_pio(taddr); + if (port == (unsigned long)-1) return -EINVAL; r->start = port; r->end = port + size - 1; } else { + if (taddr == OF_BAD_ADDR) + return -EINVAL; + r->start = taddr; r->end = taddr + size - 1; }