| Message ID | 1478576829-112707-4-git-send-email-yuanzhichang@hisilicon.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi zhichang.yuan,
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on v4.9-rc4 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/zhichang-yuan/ARM64-LPC-legacy-ISA-I-O-support/20161108-114742
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All warnings (new ones prefixed by >>):
drivers/bus/hisi_lpc.c: In function 'hisilpc_probe':
>> drivers/bus/hisi_lpc.c:439:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=]
dev_err(&pdev->dev, "ioremap memory FAIL(%d)!\n",
^
vim +439 drivers/bus/hisi_lpc.c
423 {
424 struct resource *iores;
425 struct hisilpc_dev *lpcdev;
426 int ret;
427
428 dev_info(&pdev->dev, "probing hslpc...\n");
429
430 lpcdev = devm_kzalloc(&pdev->dev,
431 sizeof(struct hisilpc_dev), GFP_KERNEL);
432 if (!lpcdev)
433 return -ENOMEM;
434
435 spin_lock_init(&lpcdev->cycle_lock);
436 iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
437 lpcdev->membase = devm_ioremap_resource(&pdev->dev, iores);
438 if (IS_ERR(lpcdev->membase)) {
> 439 dev_err(&pdev->dev, "ioremap memory FAIL(%d)!\n",
440 PTR_ERR(lpcdev->membase));
441 return PTR_ERR(lpcdev->membase);
442 }
443 /*
444 * The first PCIBIOS_MIN_IO is reserved specifically for indirectIO.
445 * It will separate indirectIO range from pci host bridge to
446 * avoid the possible PIO conflict.
447 * Set the indirectIO range directly here.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > + /* > + * The first PCIBIOS_MIN_IO is reserved specifically for indirectIO. > + * It will separate indirectIO range from pci host bridge to > + * avoid the possible PIO conflict. > + * Set the indirectIO range directly here. > + */ > + lpcdev->io_ops.start = 0; > + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > + lpcdev->io_ops.devpara = lpcdev; > + lpcdev->io_ops.pfin = hisilpc_comm_in; > + lpcdev->io_ops.pfout = hisilpc_comm_out; > + lpcdev->io_ops.pfins = hisilpc_comm_ins; > + lpcdev->io_ops.pfouts = hisilpc_comm_outs; I have to look at patch 2 in more detail again, after missing a few review rounds. I'm still a bit skeptical about hardcoding a logical I/O port range here, and would hope that we can just go through the same assignment of logical port ranges that we have for PCI buses, decoupling the bus addresses from the linux-internal ones. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 08 November 2016 16:25 > To: Yuanzhichang > Cc: catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org; > bhelgaas@google.com; mark.rutland@arm.com; olof@lixom.net; linux-arm- > kernel@lists.infradead.org; lorenzo.pieralisi@arm.com; linux- > kernel@vger.kernel.org; Linuxarm; devicetree@vger.kernel.org; linux- > pci@vger.kernel.org; linux-serial@vger.kernel.org; minyard@acm.org; > benh@kernel.crashing.org; liviu.dudau@arm.com; zourongrong@gmail.com; > John Garry; Gabriele Paoloni; zhichang.yuan02@gmail.com; > kantyzc@163.com; xuwei (O) > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > > + /* > > + * The first PCIBIOS_MIN_IO is reserved specifically for > indirectIO. > > + * It will separate indirectIO range from pci host bridge to > > + * avoid the possible PIO conflict. > > + * Set the indirectIO range directly here. > > + */ > > + lpcdev->io_ops.start = 0; > > + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > > + lpcdev->io_ops.devpara = lpcdev; > > + lpcdev->io_ops.pfin = hisilpc_comm_in; > > + lpcdev->io_ops.pfout = hisilpc_comm_out; > > + lpcdev->io_ops.pfins = hisilpc_comm_ins; > > + lpcdev->io_ops.pfouts = hisilpc_comm_outs; > > I have to look at patch 2 in more detail again, after missing a few > review > rounds. I'm still a bit skeptical about hardcoding a logical I/O port > range here, and would hope that we can just go through the same > assignment of logical port ranges that we have for PCI buses, > decoupling > the bus addresses from the linux-internal ones. The point here is that we want to avoid any conflict/overlap between the LPC I/O space and the PCI I/O space. With the assignment above we make sure that LPC never interfere with PCI I/O space. Thanks Gab > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote: > > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > > > + /* > > > + * The first PCIBIOS_MIN_IO is reserved specifically for > > indirectIO. > > > + * It will separate indirectIO range from pci host bridge to > > > + * avoid the possible PIO conflict. > > > + * Set the indirectIO range directly here. > > > + */ > > > + lpcdev->io_ops.start = 0; > > > + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > > > + lpcdev->io_ops.devpara = lpcdev; > > > + lpcdev->io_ops.pfin = hisilpc_comm_in; > > > + lpcdev->io_ops.pfout = hisilpc_comm_out; > > > + lpcdev->io_ops.pfins = hisilpc_comm_ins; > > > + lpcdev->io_ops.pfouts = hisilpc_comm_outs; > > > > I have to look at patch 2 in more detail again, after missing a few > > review > > rounds. I'm still a bit skeptical about hardcoding a logical I/O port > > range here, and would hope that we can just go through the same > > assignment of logical port ranges that we have for PCI buses, > > decoupling > > the bus addresses from the linux-internal ones. > > The point here is that we want to avoid any conflict/overlap between > the LPC I/O space and the PCI I/O space. With the assignment above > we make sure that LPC never interfere with PCI I/O space. But we already abstract the PCI I/O space using dynamic registration. There is no need to hardcode the logical address for ISA, though I think we can hardcode the bus address to start at zero here. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Arnd, On 2016/11/10 5:34, Arnd Bergmann wrote: > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote: >>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: >>>> + /* >>>> + * The first PCIBIOS_MIN_IO is reserved specifically for >>> indirectIO. >>>> + * It will separate indirectIO range from pci host bridge to >>>> + * avoid the possible PIO conflict. >>>> + * Set the indirectIO range directly here. >>>> + */ >>>> + lpcdev->io_ops.start = 0; >>>> + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; >>>> + lpcdev->io_ops.devpara = lpcdev; >>>> + lpcdev->io_ops.pfin = hisilpc_comm_in; >>>> + lpcdev->io_ops.pfout = hisilpc_comm_out; >>>> + lpcdev->io_ops.pfins = hisilpc_comm_ins; >>>> + lpcdev->io_ops.pfouts = hisilpc_comm_outs; >>> >>> I have to look at patch 2 in more detail again, after missing a few >>> review >>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port >>> range here, and would hope that we can just go through the same >>> assignment of logical port ranges that we have for PCI buses, >>> decoupling >>> the bus addresses from the linux-internal ones. >> >> The point here is that we want to avoid any conflict/overlap between >> the LPC I/O space and the PCI I/O space. With the assignment above >> we make sure that LPC never interfere with PCI I/O space. > > But we already abstract the PCI I/O space using dynamic registration. > There is no need to hardcode the logical address for ISA, though > I think we can hardcode the bus address to start at zero here. Do you means that we can pick up the maximal I/O address from all children's device resources?? Thanks, Zhichang > > Arnd > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote: > On 2016/11/10 5:34, Arnd Bergmann wrote: > > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote: > >>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > >>>> + /* > >>>> + * The first PCIBIOS_MIN_IO is reserved specifically for > >>> indirectIO. > >>>> + * It will separate indirectIO range from pci host bridge to > >>>> + * avoid the possible PIO conflict. > >>>> + * Set the indirectIO range directly here. > >>>> + */ > >>>> + lpcdev->io_ops.start = 0; > >>>> + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > >>>> + lpcdev->io_ops.devpara = lpcdev; > >>>> + lpcdev->io_ops.pfin = hisilpc_comm_in; > >>>> + lpcdev->io_ops.pfout = hisilpc_comm_out; > >>>> + lpcdev->io_ops.pfins = hisilpc_comm_ins; > >>>> + lpcdev->io_ops.pfouts = hisilpc_comm_outs; > >>> > >>> I have to look at patch 2 in more detail again, after missing a few > >>> review > >>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port > >>> range here, and would hope that we can just go through the same > >>> assignment of logical port ranges that we have for PCI buses, > >>> decoupling > >>> the bus addresses from the linux-internal ones. > >> > >> The point here is that we want to avoid any conflict/overlap between > >> the LPC I/O space and the PCI I/O space. With the assignment above > >> we make sure that LPC never interfere with PCI I/O space. > > > > But we already abstract the PCI I/O space using dynamic registration. > > There is no need to hardcode the logical address for ISA, though > > I think we can hardcode the bus address to start at zero here. > > Do you means that we can pick up the maximal I/O address from all children's > device resources?? The driver should not look at the resources of its children, just register a range of addresses dynamically, as I suggested in an earlier review. Your current version has if (arm64_extio_ops->pfout) \ arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ addr, value, sizeof(type)); \ Instead, just subtract the start of the range from the logical port number to transform it back into a bus-local port number: if (arm64_extio_ops->pfout) \ arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ addr - arm64_extio_ops->start, value, sizeof(type)); \ We know that the ISA/LPC bus can only have up to 65536 ports, so you can register all of those, or possibly limit it further to 1024 or 4096 ports, whichever matches the bus implementation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Arnd, On 2016/11/10 17:12, Arnd Bergmann wrote: > On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote: >> On 2016/11/10 5:34, Arnd Bergmann wrote: >>> On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote: >>>>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: >>>>>> + /* >>>>>> + * The first PCIBIOS_MIN_IO is reserved specifically for >>>>> indirectIO. >>>>>> + * It will separate indirectIO range from pci host bridge to >>>>>> + * avoid the possible PIO conflict. >>>>>> + * Set the indirectIO range directly here. >>>>>> + */ >>>>>> + lpcdev->io_ops.start = 0; >>>>>> + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; >>>>>> + lpcdev->io_ops.devpara = lpcdev; >>>>>> + lpcdev->io_ops.pfin = hisilpc_comm_in; >>>>>> + lpcdev->io_ops.pfout = hisilpc_comm_out; >>>>>> + lpcdev->io_ops.pfins = hisilpc_comm_ins; >>>>>> + lpcdev->io_ops.pfouts = hisilpc_comm_outs; >>>>> >>>>> I have to look at patch 2 in more detail again, after missing a few >>>>> review >>>>> rounds. I'm still a bit skeptical about hardcoding a logical I/O port >>>>> range here, and would hope that we can just go through the same >>>>> assignment of logical port ranges that we have for PCI buses, >>>>> decoupling >>>>> the bus addresses from the linux-internal ones. >>>> >>>> The point here is that we want to avoid any conflict/overlap between >>>> the LPC I/O space and the PCI I/O space. With the assignment above >>>> we make sure that LPC never interfere with PCI I/O space. >>> >>> But we already abstract the PCI I/O space using dynamic registration. >>> There is no need to hardcode the logical address for ISA, though >>> I think we can hardcode the bus address to start at zero here. >> >> Do you means that we can pick up the maximal I/O address from all children's >> device resources?? > > The driver should not look at the resources of its children, just > register a range of addresses dynamically, as I suggested in an > earlier review. > Sorry! I can't catch your idea yet:( When to register the I/O range? Is it done just after the successfully of_translate_address() during the children scanning? If yes, when a child is scanning, there is no range data in arm64_extio_ops. The addr_is_indirect_io() calling in of_get_isa_indirect_io() don't need. All we can check is just whether the address to be translated is IO and is under a parent device which has no 'ranges' property. > > Your current version has > > if (arm64_extio_ops->pfout) \ > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > addr, value, sizeof(type)); \ > > Instead, just subtract the start of the range from the logical > port number to transform it back into a bus-local port number: > > if (arm64_extio_ops->pfout) \ > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > addr - arm64_extio_ops->start, value, sizeof(type)); \ > I think there is some information needed sync. In the old patch-set, we don't bypass the pci_address_to_pio() after successfully of_translate_address(). In this way, we don't need to reserve any PIO space for our LPC since the logical port are from the same mapping algorithm. Based on this way, the port number in the device resource is logical one, then we need to subtract the start of the resource to get back the bus-local port. From V3, we don't apply the mapping based on pci_address_to_pio(), the of_translate_address() return the bus-local port directly and store into relevant device resource. So, in the current arm64_extio_ops->pfout(), the reverse translation don't need anymore. The input "addr" is bus-local port now. Thanks, Zhichang > We know that the ISA/LPC bus can only have up to 65536 ports, > so you can register all of those, or possibly limit it further to > 1024 or 4096 ports, whichever matches the bus implementation. > > Arnd > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 10 November 2016 09:12 > To: linux-arm-kernel@lists.infradead.org > Cc: Yuanzhichang; mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; Gabriele Paoloni; minyard@acm.org; linux- > pci@vger.kernel.org; benh@kernel.crashing.org; John Garry; > will.deacon@arm.com; linux-kernel@vger.kernel.org; xuwei (O); Linuxarm; > zourongrong@gmail.com; robh+dt@kernel.org; kantyzc@163.com; linux- > serial@vger.kernel.org; catalin.marinas@arm.com; olof@lixom.net; > liviu.dudau@arm.com; bhelgaas@google.com; zhichang.yuan02@gmail.com > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, November 10, 2016 2:40:26 PM CET zhichang.yuan wrote: > > On 2016/11/10 5:34, Arnd Bergmann wrote: > > > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni > wrote: > > >>> On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > > >>>> + /* > > >>>> + * The first PCIBIOS_MIN_IO is reserved specifically for > > >>> indirectIO. > > >>>> + * It will separate indirectIO range from pci host > bridge to > > >>>> + * avoid the possible PIO conflict. > > >>>> + * Set the indirectIO range directly here. > > >>>> + */ > > >>>> + lpcdev->io_ops.start = 0; > > >>>> + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > > >>>> + lpcdev->io_ops.devpara = lpcdev; > > >>>> + lpcdev->io_ops.pfin = hisilpc_comm_in; > > >>>> + lpcdev->io_ops.pfout = hisilpc_comm_out; > > >>>> + lpcdev->io_ops.pfins = hisilpc_comm_ins; > > >>>> + lpcdev->io_ops.pfouts = hisilpc_comm_outs; > > >>> > > >>> I have to look at patch 2 in more detail again, after missing a > few > > >>> review > > >>> rounds. I'm still a bit skeptical about hardcoding a logical I/O > port > > >>> range here, and would hope that we can just go through the same > > >>> assignment of logical port ranges that we have for PCI buses, > > >>> decoupling > > >>> the bus addresses from the linux-internal ones. > > >> > > >> The point here is that we want to avoid any conflict/overlap > between > > >> the LPC I/O space and the PCI I/O space. With the assignment above > > >> we make sure that LPC never interfere with PCI I/O space. > > > > > > But we already abstract the PCI I/O space using dynamic > registration. > > > There is no need to hardcode the logical address for ISA, though > > > I think we can hardcode the bus address to start at zero here. > > > > Do you means that we can pick up the maximal I/O address from all > children's > > device resources?? > > The driver should not look at the resources of its children, just > register a range of addresses dynamically, as I suggested in an > earlier review. Where should we get the range from? For LPC we know that it is going Work on anything that is not used by PCI I/O space, and this is why we use [0, PCIBIOS_MIN_IO] > > > Your current version has > > if (arm64_extio_ops->pfout) \ > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > addr, value, sizeof(type)); \ > > Instead, just subtract the start of the range from the logical > port number to transform it back into a bus-local port number: These accessors do not operate on IO tokens: If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) addr is not going to be an I/O token; in fact patch 2/3 imposes that the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO we have free physical addresses that the accessors can operate on. Thanks Gab > > if (arm64_extio_ops->pfout) \ > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > addr - arm64_extio_ops->start, value, > sizeof(type)); \ > > We know that the ISA/LPC bus can only have up to 65536 ports, > so you can register all of those, or possibly limit it further to > 1024 or 4096 ports, whichever matches the bus implementation. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: > > Where should we get the range from? For LPC we know that it is going > Work on anything that is not used by PCI I/O space, and this is > why we use [0, PCIBIOS_MIN_IO] It should be allocated the same way we allocate PCI config space segments. This is currently done with the io_range list in drivers/pci/pci.c, which isn't perfect but could be extended if necessary. Based on what others commented here, I'd rather make the differences between ISA/LPC and PCI I/O ranges smaller than larger. > > Your current version has > > > > if (arm64_extio_ops->pfout) \ > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > addr, value, sizeof(type)); \ > > > > Instead, just subtract the start of the range from the logical > > port number to transform it back into a bus-local port number: > > These accessors do not operate on IO tokens: > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > addr is not going to be an I/O token; in fact patch 2/3 imposes that > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO > we have free physical addresses that the accessors can operate on. Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to the logical I/O tokens, the purpose of that macro is really meant for allocating PCI I/O port numbers within the address space of one bus. Note that it's equally likely that whichever next platform needs non-mapped I/O access like this actually needs them for PCI I/O space, and that will use it on addresses registered to a PCI host bridge. If we separate the two steps: a) assign a range of logical I/O port numbers to a bus b) register a set of helpers for redirecting logical I/O port to a helper function then I think the code will get cleaner and more flexible. It should actually then be able to replace the powerpc specific implementation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Arnd, On 2016/11/11 0:07, Arnd Bergmann wrote: > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: >> >> Where should we get the range from? For LPC we know that it is going >> Work on anything that is not used by PCI I/O space, and this is >> why we use [0, PCIBIOS_MIN_IO] > > It should be allocated the same way we allocate PCI config space > segments. This is currently done with the io_range list in > drivers/pci/pci.c, which isn't perfect but could be extended > if necessary. Based on what others commented here, I'd rather > make the differences between ISA/LPC and PCI I/O ranges smaller > than larger. > >>> Your current version has >>> >>> if (arm64_extio_ops->pfout) \ >>> arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ >>> addr, value, sizeof(type)); \ >>> >>> Instead, just subtract the start of the range from the logical >>> port number to transform it back into a bus-local port number: >> >> These accessors do not operate on IO tokens: >> >> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) >> addr is not going to be an I/O token; in fact patch 2/3 imposes that >> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO >> we have free physical addresses that the accessors can operate on. > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to > the logical I/O tokens, the purpose of that macro is really meant > for allocating PCI I/O port numbers within the address space of > one bus. > > Note that it's equally likely that whichever next platform needs > non-mapped I/O access like this actually needs them for PCI I/O space, > and that will use it on addresses registered to a PCI host bridge. > > If we separate the two steps: > > a) assign a range of logical I/O port numbers to a bus > b) register a set of helpers for redirecting logical I/O > port to a helper function > It seems that we need to add a new bus and the corresponding resource management which can also cover current PCI pio mapping, is it right? Thanks, Zhichang > then I think the code will get cleaner and more flexible. > It should actually then be able to replace the powerpc > specific implementation. > > Arnd > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On Thu, Nov 10, 2016 at 05:07:21PM +0100, Arnd Bergmann wrote: > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: > > > > Where should we get the range from? For LPC we know that it is going > > Work on anything that is not used by PCI I/O space, and this is > > why we use [0, PCIBIOS_MIN_IO] > > It should be allocated the same way we allocate PCI config space > segments. This is currently done with the io_range list in > drivers/pci/pci.c, which isn't perfect but could be extended > if necessary. Based on what others commented here, I'd rather > make the differences between ISA/LPC and PCI I/O ranges smaller > than larger. > > > > Your current version has > > > > > > if (arm64_extio_ops->pfout) \ > > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > > addr, value, sizeof(type)); \ > > > > > > Instead, just subtract the start of the range from the logical > > > port number to transform it back into a bus-local port number: > > > > These accessors do not operate on IO tokens: > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > > addr is not going to be an I/O token; in fact patch 2/3 imposes that > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO > > we have free physical addresses that the accessors can operate on. > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to > the logical I/O tokens, the purpose of that macro is really meant > for allocating PCI I/O port numbers within the address space of > one bus. > > Note that it's equally likely that whichever next platform needs > non-mapped I/O access like this actually needs them for PCI I/O space, > and that will use it on addresses registered to a PCI host bridge. > > If we separate the two steps: > > a) assign a range of logical I/O port numbers to a bus Except that currently when we add ranges to io_range_list we don't have a bus number yet, because the parsing happens before the host bridge has been created. Maybe register_io_range() can take a bus number as an argument, but I'm not sure how we are going to use that in pci_pio_to_address() or pci_address_to_pio(). Best regards, Liviu > b) register a set of helpers for redirecting logical I/O > port to a helper function > > then I think the code will get cleaner and more flexible. > It should actually then be able to replace the powerpc > specific implementation. > > Arnd
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 10 November 2016 16:07 > To: Gabriele Paoloni > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; > mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; > bhelgaas@googl e.com; zhichang.yuan02@gmail.com > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: > > > > Where should we get the range from? For LPC we know that it is going > > Work on anything that is not used by PCI I/O space, and this is > > why we use [0, PCIBIOS_MIN_IO] > > It should be allocated the same way we allocate PCI config space > segments. This is currently done with the io_range list in > drivers/pci/pci.c, which isn't perfect but could be extended > if necessary. Based on what others commented here, I'd rather > make the differences between ISA/LPC and PCI I/O ranges smaller > than larger. I am not sure this would make sense... IMHO all the mechanism around io_range_list is needed to provide the "mapping" between I/O tokens and physical CPU addresses. Currently the available tokens range from 0 to IO_SPACE_LIMIT. As you know the I/O memory accessors operate on whatever __of_address_to_resource sets into the resource (start, end). With this special device in place we cannot know if a resource is assigned with an I/O token or a physical address, unless we forbid the I/O tokens to be in a specific range. So this is why we are changing the offsets of all the functions handling io_range_list (to make sure that a range is forbidden to the tokens and is available to the physical addresses). We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) because this is the maximum physical I/O range that a non PCI device can operate on and because we believe this does not impose much restriction on the available I/O token range; that now is [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. So we believe that the chosen forbidden range can accommodate any special ISA bus device with no much constraint on the rest of I/O tokens... > > > > Your current version has > > > > > > if (arm64_extio_ops->pfout) \ > > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > > addr, value, sizeof(type)); \ > > > > > > Instead, just subtract the start of the range from the logical > > > port number to transform it back into a bus-local port number: > > > > These accessors do not operate on IO tokens: > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > > addr is not going to be an I/O token; in fact patch 2/3 imposes that > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > PCIBIOS_MIN_IO > > we have free physical addresses that the accessors can operate on. > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to > the logical I/O tokens, the purpose of that macro is really meant > for allocating PCI I/O port numbers within the address space of > one bus. As I mentioned above, special devices operate on CPU addresses directly, not I/O tokens. For them there is no way to distinguish.... > > Note that it's equally likely that whichever next platform needs > non-mapped I/O access like this actually needs them for PCI I/O space, > and that will use it on addresses registered to a PCI host bridge. Ok so here you are talking about a platform that has got an I/O range under the PCI host controller, right? And this I/O range cannot be directly memory mapped but needs special redirections for the I/O tokens, right? In this scenario registering the I/O ranges with the forbidden range implemented by the current patch would still allow to redirect I/O tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO So effectively the special PCI host controller 1) knows the physical range that needs special redirection 2) register such range 3) uses pci_pio_to_address() to retrieve the IO tokens for the special accessors 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3) So to be honest I think this patch can fit well both with special PCI controllers that need I/O tokens redirection and with special non-PCI controllers that need non-PCI I/O physical address redirection... Thanks (and sorry for the long reply but I didn't know how to make the explanation shorter :) ) Gab > > If we separate the two steps: > > a) assign a range of logical I/O port numbers to a bus > b) register a set of helpers for redirecting logical I/O > port to a helper function > > then I think the code will get cleaner and more flexible. > It should actually then be able to replace the powerpc > specific implementation. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote: > Hi Arnd > > > -----Original Message----- > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > Sent: 10 November 2016 16:07 > > To: Gabriele Paoloni > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > > Hip06 > > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: > > > > > > Where should we get the range from? For LPC we know that it is going > > > Work on anything that is not used by PCI I/O space, and this is > > > why we use [0, PCIBIOS_MIN_IO] > > > > It should be allocated the same way we allocate PCI config space > > segments. This is currently done with the io_range list in > > drivers/pci/pci.c, which isn't perfect but could be extended > > if necessary. Based on what others commented here, I'd rather > > make the differences between ISA/LPC and PCI I/O ranges smaller > > than larger. Gabriele, > > I am not sure this would make sense... > > IMHO all the mechanism around io_range_list is needed to provide the > "mapping" between I/O tokens and physical CPU addresses. > > Currently the available tokens range from 0 to IO_SPACE_LIMIT. > > As you know the I/O memory accessors operate on whatever > __of_address_to_resource sets into the resource (start, end). > > With this special device in place we cannot know if a resource is > assigned with an I/O token or a physical address, unless we forbid > the I/O tokens to be in a specific range. > > So this is why we are changing the offsets of all the functions > handling io_range_list (to make sure that a range is forbidden to > the tokens and is available to the physical addresses). > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) > because this is the maximum physical I/O range that a non PCI device > can operate on and because we believe this does not impose much > restriction on the available I/O token range; that now is > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. > So we believe that the chosen forbidden range can accommodate > any special ISA bus device with no much constraint on the rest > of I/O tokens... Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you actually need another variable for "reserving" an area in the I/O space that can be used for physical addresses rather than I/O tokens. The one good example for using PCIBIOS_MIN_IO is when your platform/architecture does not support legacy ISA operations *at all*. In that case someone sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range so that it doesn't get used. With Zhichang's patch you now start forcing those platforms to have a valid address below PCIBIOS_MIN_IO. For the general case you also have to bear in mind that PCIBIOS_MIN_IO could be zero. In that case, what is your "forbidden" range? [0, 0) ? So it makes sense to add a new #define that should only be defined by those architectures/ platforms that want to reserve on top of PCIBIOS_MIN_IO another region where I/O tokens can't be generated for. Best regards, Liviu > > > > > > > Your current version has > > > > > > > > if (arm64_extio_ops->pfout) \ > > > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > > > addr, value, sizeof(type)); \ > > > > > > > > Instead, just subtract the start of the range from the logical > > > > port number to transform it back into a bus-local port number: > > > > > > These accessors do not operate on IO tokens: > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > > > addr is not going to be an I/O token; in fact patch 2/3 imposes that > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > > PCIBIOS_MIN_IO > > > we have free physical addresses that the accessors can operate on. > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to > > the logical I/O tokens, the purpose of that macro is really meant > > for allocating PCI I/O port numbers within the address space of > > one bus. > > As I mentioned above, special devices operate on CPU addresses directly, > not I/O tokens. For them there is no way to distinguish.... > > > > > Note that it's equally likely that whichever next platform needs > > non-mapped I/O access like this actually needs them for PCI I/O space, > > and that will use it on addresses registered to a PCI host bridge. > > Ok so here you are talking about a platform that has got an I/O range > under the PCI host controller, right? > And this I/O range cannot be directly memory mapped but needs special > redirections for the I/O tokens, right? > > In this scenario registering the I/O ranges with the forbidden range > implemented by the current patch would still allow to redirect I/O > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO > > So effectively the special PCI host controller > 1) knows the physical range that needs special redirection > 2) register such range > 3) uses pci_pio_to_address() to retrieve the IO tokens for the > special accessors > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3) > > So to be honest I think this patch can fit well both with > special PCI controllers that need I/O tokens redirection and with > special non-PCI controllers that need non-PCI I/O physical > address redirection... > > Thanks (and sorry for the long reply but I didn't know how > to make the explanation shorter :) ) > > Gab > > > > > If we separate the two steps: > > > > a) assign a range of logical I/O port numbers to a bus > > b) register a set of helpers for redirecting logical I/O > > port to a helper function > > > > then I think the code will get cleaner and more flexible. > > It should actually then be able to replace the powerpc > > specific implementation. > > > > Arnd
Hi Liviu > -----Original Message----- > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com] > Sent: 11 November 2016 14:46 > To: Gabriele Paoloni > Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang; > mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com; > zhichang.yuan02@gmail.com > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote: > > Hi Arnd > > > > > -----Original Message----- > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > Sent: 10 November 2016 16:07 > > > To: Gabriele Paoloni > > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux- > pci@vger.kernel.org; > > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; > > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > > > Hip06 > > > > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni > wrote: > > > > > > > > Where should we get the range from? For LPC we know that it is > going > > > > Work on anything that is not used by PCI I/O space, and this is > > > > why we use [0, PCIBIOS_MIN_IO] > > > > > > It should be allocated the same way we allocate PCI config space > > > segments. This is currently done with the io_range list in > > > drivers/pci/pci.c, which isn't perfect but could be extended > > > if necessary. Based on what others commented here, I'd rather > > > make the differences between ISA/LPC and PCI I/O ranges smaller > > > than larger. > > Gabriele, > > > > > I am not sure this would make sense... > > > > IMHO all the mechanism around io_range_list is needed to provide the > > "mapping" between I/O tokens and physical CPU addresses. > > > > Currently the available tokens range from 0 to IO_SPACE_LIMIT. > > > > As you know the I/O memory accessors operate on whatever > > __of_address_to_resource sets into the resource (start, end). > > > > With this special device in place we cannot know if a resource is > > assigned with an I/O token or a physical address, unless we forbid > > the I/O tokens to be in a specific range. > > > > So this is why we are changing the offsets of all the functions > > handling io_range_list (to make sure that a range is forbidden to > > the tokens and is available to the physical addresses). > > > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) > > because this is the maximum physical I/O range that a non PCI device > > can operate on and because we believe this does not impose much > > restriction on the available I/O token range; that now is > > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. > > So we believe that the chosen forbidden range can accommodate > > any special ISA bus device with no much constraint on the rest > > of I/O tokens... > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you > actually need another variable for "reserving" an area in the I/O space > that can be used for physical addresses rather than I/O tokens. > > The one good example for using PCIBIOS_MIN_IO is when your > platform/architecture > does not support legacy ISA operations *at all*. In that case someone > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range > so that it doesn't get used. With Zhichang's patch you now start > forcing > those platforms to have a valid address below PCIBIOS_MIN_IO. But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be used by PCI controllers only...so if you have a special bus device using an I/O range in this case should be a PCI controller...i.e. I would expect it to fall back into the case of I/O tokens redirection rather than physical addresses redirection (as mentioned below from my previous reply). What do you think? Thanks Gab > > For the general case you also have to bear in mind that PCIBIOS_MIN_IO > could > be zero. In that case, what is your "forbidden" range? [0, 0) ? So it > makes > sense to add a new #define that should only be defined by those > architectures/ > platforms that want to reserve on top of PCIBIOS_MIN_IO another region > where I/O tokens can't be generated for. > > Best regards, > Liviu > > > > > > > > > > > Your current version has > > > > > > > > > > if (arm64_extio_ops->pfout) > \ > > > > > arm64_extio_ops->pfout(arm64_extio_ops- > >devpara,\ > > > > > addr, value, sizeof(type)); > \ > > > > > > > > > > Instead, just subtract the start of the range from the logical > > > > > port number to transform it back into a bus-local port number: > > > > > > > > These accessors do not operate on IO tokens: > > > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > > > > addr is not going to be an I/O token; in fact patch 2/3 imposes > that > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > > > PCIBIOS_MIN_IO > > > > we have free physical addresses that the accessors can operate > on. > > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer > to > > > the logical I/O tokens, the purpose of that macro is really meant > > > for allocating PCI I/O port numbers within the address space of > > > one bus. > > > > As I mentioned above, special devices operate on CPU addresses > directly, > > not I/O tokens. For them there is no way to distinguish.... > > > > > > > > Note that it's equally likely that whichever next platform needs > > > non-mapped I/O access like this actually needs them for PCI I/O > space, > > > and that will use it on addresses registered to a PCI host bridge. > > > > Ok so here you are talking about a platform that has got an I/O range > > under the PCI host controller, right? > > And this I/O range cannot be directly memory mapped but needs special > > redirections for the I/O tokens, right? > > > > In this scenario registering the I/O ranges with the forbidden range > > implemented by the current patch would still allow to redirect I/O > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO > > > > So effectively the special PCI host controller > > 1) knows the physical range that needs special redirection > > 2) register such range > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the > > special accessors > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3) > > > > So to be honest I think this patch can fit well both with > > special PCI controllers that need I/O tokens redirection and with > > special non-PCI controllers that need non-PCI I/O physical > > address redirection... > > > > Thanks (and sorry for the long reply but I didn't know how > > to make the explanation shorter :) ) > > > > Gab > > > > > > > > If we separate the two steps: > > > > > > a) assign a range of logical I/O port numbers to a bus > > > b) register a set of helpers for redirecting logical I/O > > > port to a helper function > > > > > > then I think the code will get cleaner and more flexible. > > > It should actually then be able to replace the powerpc > > > specific implementation. > > > > > > Arnd > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
Hi, Liviu, On 11/11/2016 10:45 PM, liviu.dudau@arm.com wrote: > On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote: >> Hi Arnd >> >>> -----Original Message----- >>> From: Arnd Bergmann [mailto:arnd@arndb.de] >>> Sent: 10 November 2016 16:07 >>> To: Gabriele Paoloni >>> Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; >>> mark.rutland@arm.com; devicetree@vger.kernel.org; >>> lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; >>> benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- >>> kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; >>> robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; >>> catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; >>> bhelgaas@googl e.com; zhichang.yuan02@gmail.com >>> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on >>> Hip06 >>> >>> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote: >>>> >>>> Where should we get the range from? For LPC we know that it is going >>>> Work on anything that is not used by PCI I/O space, and this is >>>> why we use [0, PCIBIOS_MIN_IO] >>> >>> It should be allocated the same way we allocate PCI config space >>> segments. This is currently done with the io_range list in >>> drivers/pci/pci.c, which isn't perfect but could be extended >>> if necessary. Based on what others commented here, I'd rather >>> make the differences between ISA/LPC and PCI I/O ranges smaller >>> than larger. > > Gabriele, > >> >> I am not sure this would make sense... >> >> IMHO all the mechanism around io_range_list is needed to provide the >> "mapping" between I/O tokens and physical CPU addresses. >> >> Currently the available tokens range from 0 to IO_SPACE_LIMIT. >> >> As you know the I/O memory accessors operate on whatever >> __of_address_to_resource sets into the resource (start, end). >> >> With this special device in place we cannot know if a resource is >> assigned with an I/O token or a physical address, unless we forbid >> the I/O tokens to be in a specific range. >> >> So this is why we are changing the offsets of all the functions >> handling io_range_list (to make sure that a range is forbidden to >> the tokens and is available to the physical addresses). >> >> We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) >> because this is the maximum physical I/O range that a non PCI device >> can operate on and because we believe this does not impose much >> restriction on the available I/O token range; that now is >> [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. >> So we believe that the chosen forbidden range can accommodate >> any special ISA bus device with no much constraint on the rest >> of I/O tokens... > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you > actually need another variable for "reserving" an area in the I/O space > that can be used for physical addresses rather than I/O tokens. > I think selecting PCIBIOS_MIN_IO as the separator of mapped and non-mapped I/O range probably is not so reasonable. PCIBIOS_MIN_IN is specific to PCI devices, it seems as the recommended minimal start I/O address when assigning the pci device I/O region. It is probably not defined in some platforms/architectures when no PCI is needed there. That is why my patch caused some compile error on some archs; But more important thing is that the PCIBIOS_MIN_IO has different value on different platforms/architectures. On Arm64, it is 4K currently, but in other archs, it is not true. And the maximum LPC I/O address should be 64K theoretically, although for compatible ISA, 2K is enough. So, It means using PCIBIOS_MIN_IO on arm64 can match our I/O reservation require. But we can not make this indirectIO work well on other architectures. I am thinking Arnd's suggestion. But I worry about I haven't completely understood his idea. What about create a new bus host for LPC/ISA whose I/O range can be 64KB? This LPC/ISA I/O range works similar to PCI host bridge's I/O window, all the downstream devices under LPC/ISA should request I/O from that root resource. But it seems Arnd want this root resource registered dynamically, I am not sure how to do... Anyway, if we have this root I/O resource, we don't need any new macro or variable for the LPC/ISA I/O reservation. Hope my thought is right. Best, Zhichang > The one good example for using PCIBIOS_MIN_IO is when your platform/architecture > does not support legacy ISA operations *at all*. In that case someone > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range > so that it doesn't get used. With Zhichang's patch you now start forcing > those platforms to have a valid address below PCIBIOS_MIN_IO. > > For the general case you also have to bear in mind that PCIBIOS_MIN_IO could > be zero. In that case, what is your "forbidden" range? [0, 0) ? So it makes > sense to add a new #define that should only be defined by those architectures/ > platforms that want to reserve on top of PCIBIOS_MIN_IO another region > where I/O tokens can't be generated for. > > Best regards, > Liviu > >> >>> >>>>> Your current version has >>>>> >>>>> if (arm64_extio_ops->pfout) \ >>>>> arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ >>>>> addr, value, sizeof(type)); \ >>>>> >>>>> Instead, just subtract the start of the range from the logical >>>>> port number to transform it back into a bus-local port number: >>>> >>>> These accessors do not operate on IO tokens: >>>> >>>> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) >>>> addr is not going to be an I/O token; in fact patch 2/3 imposes that >>>> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to >>> PCIBIOS_MIN_IO >>>> we have free physical addresses that the accessors can operate on. >>> >>> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to >>> the logical I/O tokens, the purpose of that macro is really meant >>> for allocating PCI I/O port numbers within the address space of >>> one bus. >> >> As I mentioned above, special devices operate on CPU addresses directly, >> not I/O tokens. For them there is no way to distinguish.... >> >>> >>> Note that it's equally likely that whichever next platform needs >>> non-mapped I/O access like this actually needs them for PCI I/O space, >>> and that will use it on addresses registered to a PCI host bridge. >> >> Ok so here you are talking about a platform that has got an I/O range >> under the PCI host controller, right? >> And this I/O range cannot be directly memory mapped but needs special >> redirections for the I/O tokens, right? >> >> In this scenario registering the I/O ranges with the forbidden range >> implemented by the current patch would still allow to redirect I/O >> tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO >> >> So effectively the special PCI host controller >> 1) knows the physical range that needs special redirection >> 2) register such range >> 3) uses pci_pio_to_address() to retrieve the IO tokens for the >> special accessors >> 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3) >> >> So to be honest I think this patch can fit well both with >> special PCI controllers that need I/O tokens redirection and with >> special non-PCI controllers that need non-PCI I/O physical >> address redirection... >> >> Thanks (and sorry for the long reply but I didn't know how >> to make the explanation shorter :) ) >> >> Gab >> >>> >>> If we separate the two steps: >>> >>> a) assign a range of logical I/O port numbers to a bus >>> b) register a set of helpers for redirecting logical I/O >>> port to a helper function >>> >>> then I think the code will get cleaner and more flexible. >>> It should actually then be able to replace the powerpc >>> specific implementation. >>> >>> Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 11, 2016 at 03:53:53PM +0000, Gabriele Paoloni wrote: > Hi Liviu Hi Gabriele, > > > -----Original Message----- > > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com] > > Sent: 11 November 2016 14:46 > > To: Gabriele Paoloni > > Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang; > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com; > > zhichang.yuan02@gmail.com > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > > Hip06 > > > > On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote: > > > Hi Arnd > > > > > > > -----Original Message----- > > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > Sent: 10 November 2016 16:07 > > > > To: Gabriele Paoloni > > > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux- > > pci@vger.kernel.org; > > > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > > > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > > > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > > > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; > > > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com > > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > > > > Hip06 > > > > > > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni > > wrote: > > > > > > > > > > Where should we get the range from? For LPC we know that it is > > going > > > > > Work on anything that is not used by PCI I/O space, and this is > > > > > why we use [0, PCIBIOS_MIN_IO] > > > > > > > > It should be allocated the same way we allocate PCI config space > > > > segments. This is currently done with the io_range list in > > > > drivers/pci/pci.c, which isn't perfect but could be extended > > > > if necessary. Based on what others commented here, I'd rather > > > > make the differences between ISA/LPC and PCI I/O ranges smaller > > > > than larger. > > > > Gabriele, > > > > > > > > I am not sure this would make sense... > > > > > > IMHO all the mechanism around io_range_list is needed to provide the > > > "mapping" between I/O tokens and physical CPU addresses. > > > > > > Currently the available tokens range from 0 to IO_SPACE_LIMIT. > > > > > > As you know the I/O memory accessors operate on whatever > > > __of_address_to_resource sets into the resource (start, end). > > > > > > With this special device in place we cannot know if a resource is > > > assigned with an I/O token or a physical address, unless we forbid > > > the I/O tokens to be in a specific range. > > > > > > So this is why we are changing the offsets of all the functions > > > handling io_range_list (to make sure that a range is forbidden to > > > the tokens and is available to the physical addresses). > > > > > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) > > > because this is the maximum physical I/O range that a non PCI device > > > can operate on and because we believe this does not impose much > > > restriction on the available I/O token range; that now is > > > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. > > > So we believe that the chosen forbidden range can accommodate > > > any special ISA bus device with no much constraint on the rest > > > of I/O tokens... > > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and you > > actually need another variable for "reserving" an area in the I/O space > > that can be used for physical addresses rather than I/O tokens. > > > > The one good example for using PCIBIOS_MIN_IO is when your > > platform/architecture > > does not support legacy ISA operations *at all*. In that case someone > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O range > > so that it doesn't get used. With Zhichang's patch you now start > > forcing > > those platforms to have a valid address below PCIBIOS_MIN_IO. > > But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be used > by PCI controllers only... Nope, that is not what it means. It means that PCI devices can see I/O addresses on the bus that start from 0. There never was any usage for non-PCI controllers when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and what I think is not the right thing (and not enough anyway). > so if you have a special bus device using > an I/O range in this case should be a PCI controller... That has always been the case. It is this series that wants to introduce the new meaning. > i.e. I would > expect it to fall back into the case of I/O tokens redirection rather than > physical addresses redirection (as mentioned below from my previous reply). > What do you think? I think you have looked too much at the code *with* Zhichang's patches applied. Take a step back and look at how PCIBIOS_MIN_IO is used now, before you apply the patches. It is all about PCI addresses and there is no notion of non-PCI busses using PCI framework. Only platforms and architectures that try to work around some legacy standards (ISA) or HW restrictions. Best regards, Liviu > > Thanks > > Gab > > > > > > For the general case you also have to bear in mind that PCIBIOS_MIN_IO > > could > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So it > > makes > > sense to add a new #define that should only be defined by those > > architectures/ > > platforms that want to reserve on top of PCIBIOS_MIN_IO another region > > where I/O tokens can't be generated for. > > > > Best regards, > > Liviu > > > > > > > > > > > > > > > Your current version has > > > > > > > > > > > > if (arm64_extio_ops->pfout) > > \ > > > > > > arm64_extio_ops->pfout(arm64_extio_ops- > > >devpara,\ > > > > > > addr, value, sizeof(type)); > > \ > > > > > > > > > > > > Instead, just subtract the start of the range from the logical > > > > > > port number to transform it back into a bus-local port number: > > > > > > > > > > These accessors do not operate on IO tokens: > > > > > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr) > > > > > addr is not going to be an I/O token; in fact patch 2/3 imposes > > that > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > > > > PCIBIOS_MIN_IO > > > > > we have free physical addresses that the accessors can operate > > on. > > > > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer > > to > > > > the logical I/O tokens, the purpose of that macro is really meant > > > > for allocating PCI I/O port numbers within the address space of > > > > one bus. > > > > > > As I mentioned above, special devices operate on CPU addresses > > directly, > > > not I/O tokens. For them there is no way to distinguish.... > > > > > > > > > > > Note that it's equally likely that whichever next platform needs > > > > non-mapped I/O access like this actually needs them for PCI I/O > > space, > > > > and that will use it on addresses registered to a PCI host bridge. > > > > > > Ok so here you are talking about a platform that has got an I/O range > > > under the PCI host controller, right? > > > And this I/O range cannot be directly memory mapped but needs special > > > redirections for the I/O tokens, right? > > > > > > In this scenario registering the I/O ranges with the forbidden range > > > implemented by the current patch would still allow to redirect I/O > > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO > > > > > > So effectively the special PCI host controller > > > 1) knows the physical range that needs special redirection > > > 2) register such range > > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the > > > special accessors > > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in 3) > > > > > > So to be honest I think this patch can fit well both with > > > special PCI controllers that need I/O tokens redirection and with > > > special non-PCI controllers that need non-PCI I/O physical > > > address redirection... > > > > > > Thanks (and sorry for the long reply but I didn't know how > > > to make the explanation shorter :) ) > > > > > > Gab > > > > > > > > > > > If we separate the two steps: > > > > > > > > a) assign a range of logical I/O port numbers to a bus > > > > b) register a set of helpers for redirecting logical I/O > > > > port to a helper function > > > > > > > > then I think the code will get cleaner and more flexible. > > > > It should actually then be able to replace the powerpc > > > > specific implementation. > > > > > > > > Arnd > > > > -- > > ==================== > > | I would like to | > > | fix the world, | > > | but they're not | > > | giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯
Hi Liviu > -----Original Message----- > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com] > Sent: 11 November 2016 18:16 > To: Gabriele Paoloni > Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; Yuanzhichang; > mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com; > zhichang.yuan02@gmail.com > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Fri, Nov 11, 2016 at 03:53:53PM +0000, Gabriele Paoloni wrote: > > Hi Liviu > > Hi Gabriele, > > > > > > -----Original Message----- > > > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com] > > > Sent: 11 November 2016 14:46 > > > To: Gabriele Paoloni > > > Cc: Arnd Bergmann; linux-arm-kernel@lists.infradead.org; > Yuanzhichang; > > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux- > pci@vger.kernel.org; > > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > > > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > > > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > > > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@googl e.com; > > > zhichang.yuan02@gmail.com > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > > > Hip06 > > > > > > On Fri, Nov 11, 2016 at 01:39:35PM +0000, Gabriele Paoloni wrote: > > > > Hi Arnd > > > > > > > > > -----Original Message----- > > > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > > Sent: 10 November 2016 16:07 > > > > > To: Gabriele Paoloni > > > > > Cc: linux-arm-kernel@lists.infradead.org; Yuanzhichang; > > > > > mark.rutland@arm.com; devicetree@vger.kernel.org; > > > > > lorenzo.pieralisi@arm.com; minyard@acm.org; linux- > > > pci@vger.kernel.org; > > > > > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; > linux- > > > > > kernel@vger.kernel.org; xuwei (O); Linuxarm; > zourongrong@gmail.com; > > > > > robh+dt@kernel.org; kantyzc@163.com; linux- > serial@vger.kernel.org; > > > > > catalin.marinas@arm.com; olof@lixom.net; liviu.dudau@arm.com; > > > > > bhelgaas@googl e.com; zhichang.yuan02@gmail.com > > > > > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver > implementation on > > > > > Hip06 > > > > > > > > > > On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni > > > wrote: > > > > > > > > > > > > Where should we get the range from? For LPC we know that it > is > > > going > > > > > > Work on anything that is not used by PCI I/O space, and this > is > > > > > > why we use [0, PCIBIOS_MIN_IO] > > > > > > > > > > It should be allocated the same way we allocate PCI config > space > > > > > segments. This is currently done with the io_range list in > > > > > drivers/pci/pci.c, which isn't perfect but could be extended > > > > > if necessary. Based on what others commented here, I'd rather > > > > > make the differences between ISA/LPC and PCI I/O ranges smaller > > > > > than larger. > > > > > > Gabriele, > > > > > > > > > > > I am not sure this would make sense... > > > > > > > > IMHO all the mechanism around io_range_list is needed to provide > the > > > > "mapping" between I/O tokens and physical CPU addresses. > > > > > > > > Currently the available tokens range from 0 to IO_SPACE_LIMIT. > > > > > > > > As you know the I/O memory accessors operate on whatever > > > > __of_address_to_resource sets into the resource (start, end). > > > > > > > > With this special device in place we cannot know if a resource is > > > > assigned with an I/O token or a physical address, unless we > forbid > > > > the I/O tokens to be in a specific range. > > > > > > > > So this is why we are changing the offsets of all the functions > > > > handling io_range_list (to make sure that a range is forbidden to > > > > the tokens and is available to the physical addresses). > > > > > > > > We have chosen this forbidden range to be [0, PCIBIOS_MIN_IO) > > > > because this is the maximum physical I/O range that a non PCI > device > > > > can operate on and because we believe this does not impose much > > > > restriction on the available I/O token range; that now is > > > > [PCIBIOS_MIN_IO, IO_SPACE_LIMIT]. > > > > So we believe that the chosen forbidden range can accommodate > > > > any special ISA bus device with no much constraint on the rest > > > > of I/O tokens... > > > > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and > you > > > actually need another variable for "reserving" an area in the I/O > space > > > that can be used for physical addresses rather than I/O tokens. > > > > > > The one good example for using PCIBIOS_MIN_IO is when your > > > platform/architecture > > > does not support legacy ISA operations *at all*. In that case > someone > > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O > range > > > so that it doesn't get used. With Zhichang's patch you now start > > > forcing > > > those platforms to have a valid address below PCIBIOS_MIN_IO. > > > > But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be > used > > by PCI controllers only... > > Nope, that is not what it means. It means that PCI devices can see I/O > addresses > on the bus that start from 0. There never was any usage for non-PCI > controllers So I am a bit confused... From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps It seems that ISA buses operate on cpu I/O address range [0, 0xFFF]. I thought that was the reason why for most architectures we have PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers usually use [0, PCIBIOS_MIN_IO - 1] ) For those architectures whose PCIBIOS_MIN_IO != 0x1000 probably they are not fully compliant or they cannot fully support an ISA controller...? As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO) to allow special ISA controllers to use that range with special accessors. Having a variable threshold would make life much more difficult as there would be a probe dependency between the PCI controller and the special ISA one (PCI to wait for the special ISA device to be probed and set the right threshold value from DT or ACPI table). Instead using PCIBIOS_MIN_IO is easier and should not impose much constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to the PCI controller for I/O tokens... Thanks Gab > when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and > what > I think is not the right thing (and not enough anyway). > > > so if you have a special bus device using > > an I/O range in this case should be a PCI controller... > > That has always been the case. It is this series that wants to > introduce the > new meaning. > > > i.e. I would > > expect it to fall back into the case of I/O tokens redirection rather > than > > physical addresses redirection (as mentioned below from my previous > reply). > > What do you think? > > I think you have looked too much at the code *with* Zhichang's patches > applied. > Take a step back and look at how PCIBIOS_MIN_IO is used now, before you > apply > the patches. It is all about PCI addresses and there is no notion of > non-PCI > busses using PCI framework. Only platforms and architectures that try > to work > around some legacy standards (ISA) or HW restrictions. > > Best regards, > Liviu > > > > > Thanks > > > > Gab > > > > > > > > > > For the general case you also have to bear in mind that > PCIBIOS_MIN_IO > > > could > > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So > it > > > makes > > > sense to add a new #define that should only be defined by those > > > architectures/ > > > platforms that want to reserve on top of PCIBIOS_MIN_IO another > region > > > where I/O tokens can't be generated for. > > > > > > Best regards, > > > Liviu > > > > > > > > > > > > > > > > > > > Your current version has > > > > > > > > > > > > > > if (arm64_extio_ops->pfout) > > > \ > > > > > > > arm64_extio_ops->pfout(arm64_extio_ops- > > > >devpara,\ > > > > > > > addr, value, sizeof(type)); > > > \ > > > > > > > > > > > > > > Instead, just subtract the start of the range from the > logical > > > > > > > port number to transform it back into a bus-local port > number: > > > > > > > > > > > > These accessors do not operate on IO tokens: > > > > > > > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < > addr) > > > > > > addr is not going to be an I/O token; in fact patch 2/3 > imposes > > > that > > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > > > > > PCIBIOS_MIN_IO > > > > > > we have free physical addresses that the accessors can > operate > > > on. > > > > > > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to > refer > > > to > > > > > the logical I/O tokens, the purpose of that macro is really > meant > > > > > for allocating PCI I/O port numbers within the address space of > > > > > one bus. > > > > > > > > As I mentioned above, special devices operate on CPU addresses > > > directly, > > > > not I/O tokens. For them there is no way to distinguish.... > > > > > > > > > > > > > > Note that it's equally likely that whichever next platform > needs > > > > > non-mapped I/O access like this actually needs them for PCI I/O > > > space, > > > > > and that will use it on addresses registered to a PCI host > bridge. > > > > > > > > Ok so here you are talking about a platform that has got an I/O > range > > > > under the PCI host controller, right? > > > > And this I/O range cannot be directly memory mapped but needs > special > > > > redirections for the I/O tokens, right? > > > > > > > > In this scenario registering the I/O ranges with the forbidden > range > > > > implemented by the current patch would still allow to redirect > I/O > > > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO > > > > > > > > So effectively the special PCI host controller > > > > 1) knows the physical range that needs special redirection > > > > 2) register such range > > > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the > > > > special accessors > > > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in > 3) > > > > > > > > So to be honest I think this patch can fit well both with > > > > special PCI controllers that need I/O tokens redirection and with > > > > special non-PCI controllers that need non-PCI I/O physical > > > > address redirection... > > > > > > > > Thanks (and sorry for the long reply but I didn't know how > > > > to make the explanation shorter :) ) > > > > > > > > Gab > > > > > > > > > > > > > > If we separate the two steps: > > > > > > > > > > a) assign a range of logical I/O port numbers to a bus > > > > > b) register a set of helpers for redirecting logical I/O > > > > > port to a helper function > > > > > > > > > > then I think the code will get cleaner and more flexible. > > > > > It should actually then be able to replace the powerpc > > > > > specific implementation. > > > > > > > > > > Arnd > > > > > > -- > > > ==================== > > > | I would like to | > > > | fix the world, | > > > | but they're not | > > > | giving me the | > > > \ source code! / > > > --------------- > > > ¯\_(ツ)_/¯ > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯
On Wed, 09 Nov 2016 22:34:38 +0100 Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote: > > > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote: > > > > + /* > > > > + * The first PCIBIOS_MIN_IO is reserved specifically for > > > indirectIO. > > > > + * It will separate indirectIO range from pci host bridge to > > > > + * avoid the possible PIO conflict. > > > > + * Set the indirectIO range directly here. > > > > + */ > > > > + lpcdev->io_ops.start = 0; > > > > + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; > > > > + lpcdev->io_ops.devpara = lpcdev; > > > > + lpcdev->io_ops.pfin = hisilpc_comm_in; > > > > + lpcdev->io_ops.pfout = hisilpc_comm_out; > > > > + lpcdev->io_ops.pfins = hisilpc_comm_ins; > > > > + lpcdev->io_ops.pfouts = hisilpc_comm_outs; > > > > > > I have to look at patch 2 in more detail again, after missing a few > > > review > > > rounds. I'm still a bit skeptical about hardcoding a logical I/O port > > > range here, and would hope that we can just go through the same > > > assignment of logical port ranges that we have for PCI buses, > > > decoupling > > > the bus addresses from the linux-internal ones. > > > > The point here is that we want to avoid any conflict/overlap between > > the LPC I/O space and the PCI I/O space. With the assignment above > > we make sure that LPC never interfere with PCI I/O space. > > But we already abstract the PCI I/O space using dynamic registration. > There is no need to hardcode the logical address for ISA, though > I think we can hardcode the bus address to start at zero here. Pedantically ISA starts at 0x100. The LPC may start at 0x00 as it also covers motherboard devices (0x00-0xFF). It is also possible that the 'LPC' space is only partially routed to the PCI bridges because some if it magially disappears on CPU die (at least on x86) and has done since the era of socket 7 (eg the Cyrix 6x86 doesn't route 0x22/0x23 out of the CPU). Assuming LPC starts at 0 ought to be ok given the PCI root bridge shouldn't see the transactions. The LPC or it's equivalent may also not be routed via the PCI bridges at all, so you could have an LPC mapping that is unused or partially used with another bus actually getting some classes of LPC traffic - on x86 at least. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote: > Hi Liviu > [snip] > > > > > > > > Your idea is a good one, however you are abusing PCIBIOS_MIN_IO and > > you > > > > actually need another variable for "reserving" an area in the I/O > > space > > > > that can be used for physical addresses rather than I/O tokens. > > > > > > > > The one good example for using PCIBIOS_MIN_IO is when your > > > > platform/architecture > > > > does not support legacy ISA operations *at all*. In that case > > someone > > > > sets the PCIBIOS_MIN_IO to a non-zero value to reserve that I/O > > range > > > > so that it doesn't get used. With Zhichang's patch you now start > > > > forcing > > > > those platforms to have a valid address below PCIBIOS_MIN_IO. > > > > > > But if PCIBIOS_MIN_IO is 0 then it means that all I/O space is to be > > used > > > by PCI controllers only... > > > > Nope, that is not what it means. It means that PCI devices can see I/O > > addresses > > on the bus that start from 0. There never was any usage for non-PCI > > controllers > > So I am a bit confused... > From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > It seems that ISA buses operate on cpu I/O address range [0, 0xFFF]. > I thought that was the reason why for most architectures we have > PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers > usually use [0, PCIBIOS_MIN_IO - 1] ) First of all, cpu I/O addresses is an x86-ism. ARM architectures and others have no separate address space for I/O, it is all merged into one unified address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could mean that we don't care about ISA I/O because the platform does not support having an ISA bus (e.g.). > > For those architectures whose PCIBIOS_MIN_IO != 0x1000 probably > they are not fully compliant or they cannot fully support an ISA > controller...? Exactly. Not fully compliant is a bit strong, as ISA is a legacy feature and when it comes to PCI-e you are allowed to ignore it. Having PCIBIOS_MIN_IO != 0x1000 is a way to signal that you don't fully support ISA. > > As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO) > to allow special ISA controllers to use that range with special > accessors. > Having a variable threshold would make life much more difficult > as there would be a probe dependency between the PCI controller and > the special ISA one (PCI to wait for the special ISA device to be > probed and set the right threshold value from DT or ACPI table). > > Instead using PCIBIOS_MIN_IO is easier and should not impose much > constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to > the PCI controller for I/O tokens... What I am suggesting is to leave PCIBIOS_MIN_IO alone which still reserves space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will reserve space for your direct address I/O on top of PCIBIOS_MIN_IO. Best regards, Liviu > > Thanks > > Gab > > > when PCIBIOS_MIN_IO != 0. That is what Zhichang is trying to do now and > > what > > I think is not the right thing (and not enough anyway). > > > > > so if you have a special bus device using > > > an I/O range in this case should be a PCI controller... > > > > That has always been the case. It is this series that wants to > > introduce the > > new meaning. > > > > > i.e. I would > > > expect it to fall back into the case of I/O tokens redirection rather > > than > > > physical addresses redirection (as mentioned below from my previous > > reply). > > > What do you think? > > > > I think you have looked too much at the code *with* Zhichang's patches > > applied. > > Take a step back and look at how PCIBIOS_MIN_IO is used now, before you > > apply > > the patches. It is all about PCI addresses and there is no notion of > > non-PCI > > busses using PCI framework. Only platforms and architectures that try > > to work > > around some legacy standards (ISA) or HW restrictions. > > > > Best regards, > > Liviu > > > > > > > > Thanks > > > > > > Gab > > > > > > > > > > > > > > For the general case you also have to bear in mind that > > PCIBIOS_MIN_IO > > > > could > > > > be zero. In that case, what is your "forbidden" range? [0, 0) ? So > > it > > > > makes > > > > sense to add a new #define that should only be defined by those > > > > architectures/ > > > > platforms that want to reserve on top of PCIBIOS_MIN_IO another > > region > > > > where I/O tokens can't be generated for. > > > > > > > > Best regards, > > > > Liviu > > > > > > > > > > > > > > > > > > > > > > > Your current version has > > > > > > > > > > > > > > > > if (arm64_extio_ops->pfout) > > > > \ > > > > > > > > arm64_extio_ops->pfout(arm64_extio_ops- > > > > >devpara,\ > > > > > > > > addr, value, sizeof(type)); > > > > \ > > > > > > > > > > > > > > > > Instead, just subtract the start of the range from the > > logical > > > > > > > > port number to transform it back into a bus-local port > > number: > > > > > > > > > > > > > > These accessors do not operate on IO tokens: > > > > > > > > > > > > > > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < > > addr) > > > > > > > addr is not going to be an I/O token; in fact patch 2/3 > > imposes > > > > that > > > > > > > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to > > > > > > PCIBIOS_MIN_IO > > > > > > > we have free physical addresses that the accessors can > > operate > > > > on. > > > > > > > > > > > > Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to > > refer > > > > to > > > > > > the logical I/O tokens, the purpose of that macro is really > > meant > > > > > > for allocating PCI I/O port numbers within the address space of > > > > > > one bus. > > > > > > > > > > As I mentioned above, special devices operate on CPU addresses > > > > directly, > > > > > not I/O tokens. For them there is no way to distinguish.... > > > > > > > > > > > > > > > > > Note that it's equally likely that whichever next platform > > needs > > > > > > non-mapped I/O access like this actually needs them for PCI I/O > > > > space, > > > > > > and that will use it on addresses registered to a PCI host > > bridge. > > > > > > > > > > Ok so here you are talking about a platform that has got an I/O > > range > > > > > under the PCI host controller, right? > > > > > And this I/O range cannot be directly memory mapped but needs > > special > > > > > redirections for the I/O tokens, right? > > > > > > > > > > In this scenario registering the I/O ranges with the forbidden > > range > > > > > implemented by the current patch would still allow to redirect > > I/O > > > > > tokens as long as arm64_extio_ops->start >= PCIBIOS_MIN_IO > > > > > > > > > > So effectively the special PCI host controller > > > > > 1) knows the physical range that needs special redirection > > > > > 2) register such range > > > > > 3) uses pci_pio_to_address() to retrieve the IO tokens for the > > > > > special accessors > > > > > 4) sets arm64_extio_ops->start/end to the IO tokens retrieved in > > 3) > > > > > > > > > > So to be honest I think this patch can fit well both with > > > > > special PCI controllers that need I/O tokens redirection and with > > > > > special non-PCI controllers that need non-PCI I/O physical > > > > > address redirection... > > > > > > > > > > Thanks (and sorry for the long reply but I didn't know how > > > > > to make the explanation shorter :) ) > > > > > > > > > > Gab > > > > > > > > > > > > > > > > > If we separate the two steps: > > > > > > > > > > > > a) assign a range of logical I/O port numbers to a bus > > > > > > b) register a set of helpers for redirecting logical I/O > > > > > > port to a helper function > > > > > > > > > > > > then I think the code will get cleaner and more flexible. > > > > > > It should actually then be able to replace the powerpc > > > > > > specific implementation. > > > > > > > > > > > > Arnd
On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote: > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote: > > > Nope, that is not what it means. It means that PCI devices can see I/O > > > addresses > > > on the bus that start from 0. There never was any usage for non-PCI > > > controllers > > > > So I am a bit confused... > > From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > It seems that ISA buses operate on cpu I/O address range [0, 0xFFF]. > > I thought that was the reason why for most architectures we have > > PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers > > usually use [0, PCIBIOS_MIN_IO - 1] ) > > First of all, cpu I/O addresses is an x86-ism. ARM architectures and others > have no separate address space for I/O, it is all merged into one unified > address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could mean > that we don't care about ISA I/O because the platform does not support having > an ISA bus (e.g.). I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you cannot have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is different from having an LPC master outside of PCI, as that lives in its own domain and has a separately addressable I/O space. > > As said before this series forbid IO tokens to be in [0, PCIBIOS_MIN_IO) > > to allow special ISA controllers to use that range with special > > accessors. > > Having a variable threshold would make life much more difficult > > as there would be a probe dependency between the PCI controller and > > the special ISA one (PCI to wait for the special ISA device to be > > probed and set the right threshold value from DT or ACPI table). > > > > Instead using PCIBIOS_MIN_IO is easier and should not impose much > > constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to > > the PCI controller for I/O tokens... > > What I am suggesting is to leave PCIBIOS_MIN_IO alone which still reserves > space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will reserve > space for your direct address I/O on top of PCIBIOS_MIN_IO. The PCIBIOS_MIN_DIRECT_IO name still suggests having something related to PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple concepts here that are not the same but that are somewhat related: a) keeping PCI devices from allocating low I/O ports on the PCI bus that would conflict with ISA devices behind a bridge of the same bus. b) reserving the low 0x0-0xfff range of the Linux-internal I/O space abstraction to a particular LPC or PCI domain to make legacy device drivers work that hardcode a particular port number. c) Redirecting inb/outb to call a domain-specific accessor function rather than doing the normal MMIO window for an LPC master or more generally any arbitrary LPC or PCI domain that has a nonstandard I/O space. [side note: actually if we generalized this, we could avoid assigning an MMIO range for the I/O space on the pci-mvebu driver, and that would help free up some other remapping windows] I think there is no need to change a) here, we have PCIBIOS_MIN_IO today and even if we don't need it, there is no obvious downside. I would also argue that we can ignore b) for the discussion of the HiSilicon LPC driver, we just need to assign some range of logical addresses to each domain. That means solving c) is the important problem here, and it shouldn't be so hard. We can do this either with a single special domain as in the v5 patch series, or by generalizing it so that any I/O space mapping gets looked up through the device pointer of the bus master. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[found this old mail in my drafts folder, might as well send it now] On Thursday, November 10, 2016 8:36:24 PM CET zhichang.yuan wrote: > Sorry! I can't catch your idea yet:( > > When to register the I/O range? Is it done just after the successfully > of_translate_address() during the children scanning? No, you do it when first finding the bus itself, just like we do for PCI host bridges. > If yes, when a child is scanning, there is no range data in arm64_extio_ops. The > addr_is_indirect_io() calling in of_get_isa_indirect_io() don't need. All we can > check is just whether the address to be translated is IO and is under a parent > device which has no 'ranges' property. The children should only be scanned after the I/O range has been registered for the parent. > > Your current version has > > > > if (arm64_extio_ops->pfout) \ > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > addr, value, sizeof(type)); \ > > > > Instead, just subtract the start of the range from the logical > > port number to transform it back into a bus-local port number: > > > > if (arm64_extio_ops->pfout) \ > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\ > > addr - arm64_extio_ops->start, value, sizeof(type)); \ > > > I think there is some information needed sync. > In the old patch-set, we don't bypass the pci_address_to_pio() after > successfully of_translate_address(). In this way, we don't need to reserve any > PIO space for our LPC since the logical port are from the same mapping > algorithm. Based on this way, the port number in the device resource is logical > one, then we need to subtract the start of the resource to get back the > bus-local port. > > From V3, we don't apply the mapping based on pci_address_to_pio(), the > of_translate_address() return the bus-local port directly and store into > relevant device resource. So, in the current arm64_extio_ops->pfout(), the > reverse translation don't need anymore. The input "addr" is bus-local port now. Ok, so this would have to be changed again: If we want to support multiple bus domains, of_translate_address() must translate between the bus specific address and the general Linux I/O port number. Even without doing that, it seems nicer to not overlap the range of the first PCI host bridge. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd many thanks for your help here > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 18 November 2016 10:18 > To: liviu.dudau@arm.com > Cc: Gabriele Paoloni; linux-arm-kernel@lists.infradead.org; > Yuanzhichang; mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@go ogle.com; > zhichang.yuan02@gmail.com; Jason Gunthorpe; Thomas Petazzoni > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote: > > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote: > > > > Nope, that is not what it means. It means that PCI devices can > see I/O > > > > addresses > > > > on the bus that start from 0. There never was any usage for non- > PCI > > > > controllers > > > > > > So I am a bit confused... > > > From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > It seems that ISA buses operate on cpu I/O address range [0, > 0xFFF]. > > > I thought that was the reason why for most architectures we have > > > PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers > > > usually use [0, PCIBIOS_MIN_IO - 1] ) > > > > First of all, cpu I/O addresses is an x86-ism. ARM architectures and > others > > have no separate address space for I/O, it is all merged into one > unified > > address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could > mean > > that we don't care about ISA I/O because the platform does not > support having > > an ISA bus (e.g.). > > I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you > cannot > have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is > different > from having an LPC master outside of PCI, as that lives in its own > domain > and has a separately addressable I/O space. Yes correct so if we go for the single domain solution arch that have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC unless we also redefine PCIBIOS_MIN_IO, right? > > > > As said before this series forbid IO tokens to be in [0, > PCIBIOS_MIN_IO) > > > to allow special ISA controllers to use that range with special > > > accessors. > > > Having a variable threshold would make life much more difficult > > > as there would be a probe dependency between the PCI controller and > > > the special ISA one (PCI to wait for the special ISA device to be > > > probed and set the right threshold value from DT or ACPI table). > > > > > > Instead using PCIBIOS_MIN_IO is easier and should not impose much > > > constraint as [PCIBIOS_MIN_IO, IO_SPACE_LIMIT] is available to > > > the PCI controller for I/O tokens... > > > > What I am suggesting is to leave PCIBIOS_MIN_IO alone which still > reserves > > space for ISA controller and add a PCIBIOS_MIN_DIRECT_IO that will > reserve > > space for your direct address I/O on top of PCIBIOS_MIN_IO. > > The PCIBIOS_MIN_DIRECT_IO name still suggests having something related > to > PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple > concepts here that are not the same but that are somewhat related: > > a) keeping PCI devices from allocating low I/O ports on the PCI bus > that would conflict with ISA devices behind a bridge of the > same bus. > > b) reserving the low 0x0-0xfff range of the Linux-internal I/O > space abstraction to a particular LPC or PCI domain to make > legacy device drivers work that hardcode a particular port > number. > > c) Redirecting inb/outb to call a domain-specific accessor function > rather than doing the normal MMIO window for an LPC master or > more generally any arbitrary LPC or PCI domain that has a > nonstandard I/O space. > [side note: actually if we generalized this, we could avoid > assigning an MMIO range for the I/O space on the pci-mvebu > driver, and that would help free up some other remapping > windows] > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO > today and even if we don't need it, there is no obvious downside. > I would also argue that we can ignore b) for the discussion of > the HiSilicon LPC driver, we just need to assign some range > of logical addresses to each domain. > > That means solving c) is the important problem here, and it > shouldn't be so hard. We can do this either with a single > special domain as in the v5 patch series, or by generalizing it > so that any I/O space mapping gets looked up through the device > pointer of the bus master. I am not very on the "generalized" multi-domain solution... Currently the IO accessors prototypes have an unsigned long addr as input parameter. If we live in a multi-domain IO system how can we distinguish inside the accessor which domain addr belongs to? Thanks Gab > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote: > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com wrote: > > > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote: > > > > > Nope, that is not what it means. It means that PCI devices can > > see I/O > > > > > addresses > > > > > on the bus that start from 0. There never was any usage for non- > > PCI > > > > > controllers > > > > > > > > So I am a bit confused... > > > > From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > It seems that ISA buses operate on cpu I/O address range [0, > > 0xFFF]. > > > > I thought that was the reason why for most architectures we have > > > > PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA controllers > > > > usually use [0, PCIBIOS_MIN_IO - 1] ) > > > > > > First of all, cpu I/O addresses is an x86-ism. ARM architectures and > > others > > > have no separate address space for I/O, it is all merged into one > > unified > > > address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 could > > mean > > > that we don't care about ISA I/O because the platform does not > > support having > > > an ISA bus (e.g.). > > > > I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that you > > cannot > > have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is > > different > > from having an LPC master outside of PCI, as that lives in its own > > domain > > and has a separately addressable I/O space. > > Yes correct so if we go for the single domain solution arch that > have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC > unless we also redefine PCIBIOS_MIN_IO, right? This is what I was referring to below as the difference between a) and b): Setting PCIBIOS_MIN_IO=0 means you cannot have LPC behind PCI, but it shouldn't stop you from having a separate LPC bridge. > > The PCIBIOS_MIN_DIRECT_IO name still suggests having something related > > to > > PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple > > concepts here that are not the same but that are somewhat related: > > > > a) keeping PCI devices from allocating low I/O ports on the PCI bus > > that would conflict with ISA devices behind a bridge of the > > same bus. > > > > b) reserving the low 0x0-0xfff range of the Linux-internal I/O > > space abstraction to a particular LPC or PCI domain to make > > legacy device drivers work that hardcode a particular port > > number. > > > > c) Redirecting inb/outb to call a domain-specific accessor function > > rather than doing the normal MMIO window for an LPC master or > > more generally any arbitrary LPC or PCI domain that has a > > nonstandard I/O space. > > [side note: actually if we generalized this, we could avoid > > assigning an MMIO range for the I/O space on the pci-mvebu > > driver, and that would help free up some other remapping > > windows] > > > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO > > today and even if we don't need it, there is no obvious downside. > > I would also argue that we can ignore b) for the discussion of > > the HiSilicon LPC driver, we just need to assign some range > > of logical addresses to each domain. > > > > That means solving c) is the important problem here, and it > > shouldn't be so hard. We can do this either with a single > > special domain as in the v5 patch series, or by generalizing it > > so that any I/O space mapping gets looked up through the device > > pointer of the bus master. > > I am not very on the "generalized" multi-domain solution... > Currently the IO accessors prototypes have an unsigned long addr > as input parameter. If we live in a multi-domain IO system > how can we distinguish inside the accessor which domain addr > belongs to? The easiest change compared to the v5 code would be to walk a linked list of 'struct extio_ops' structures rather than assuming there is only ever one of them. I think one of the earlier versions actually did this. Another option the IA64 approach mentioned in another subthread today, looking up the operations based on an index from the upper bits of the port number. If we do this, we probably want to do that for all PIO access and replace the entire virtual address remapping logic with that. I think Bjorn in the past argued in favor of such an approach, while I advocated the current scheme for simplicity based on how every I/O space these days is just memory mapped (which now turned out to be false, both on powerpc and arm64). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 18 November 2016 12:24 > To: Gabriele Paoloni > Cc: liviu.dudau@arm.com; linux-arm-kernel@lists.infradead.org; > Yuanzhichang; mark.rutland@arm.com; devicetree@vger.kernel.org; > lorenzo.pieralisi@arm.com; minyard@acm.org; linux-pci@vger.kernel.org; > benh@kernel.crashing.org; John Garry; will.deacon@arm.com; linux- > kernel@vger.kernel.org; xuwei (O); Linuxarm; zourongrong@gmail.com; > robh+dt@kernel.org; kantyzc@163.com; linux-serial@vger.kernel.org; > catalin.marinas@arm.com; olof@lixom.net; bhelgaas@go og le.com; > zhichang.yuan02@gmail.com; Jason Gunthorpe; Thomas Petazzoni > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote: > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > On Monday, November 14, 2016 11:26:25 AM CET liviu.dudau@arm.com > wrote: > > > > On Mon, Nov 14, 2016 at 08:26:42AM +0000, Gabriele Paoloni wrote: > > > > > > Nope, that is not what it means. It means that PCI devices > can > > > see I/O > > > > > > addresses > > > > > > on the bus that start from 0. There never was any usage for > non- > > > PCI > > > > > > controllers > > > > > > > > > > So I am a bit confused... > > > > > From http://www.firmware.org/1275/bindings/isa/isa0_4d.ps > > > > > It seems that ISA buses operate on cpu I/O address range [0, > > > 0xFFF]. > > > > > I thought that was the reason why for most architectures we > have > > > > > PCIBIOS_MIN_IO equal to 0x1000 (so I thought that ISA > controllers > > > > > usually use [0, PCIBIOS_MIN_IO - 1] ) > > > > > > > > First of all, cpu I/O addresses is an x86-ism. ARM architectures > and > > > others > > > > have no separate address space for I/O, it is all merged into > one > > > unified > > > > address space. So, on arm/arm64 for example, PCIBIOS_MIN_IO = 0 > could > > > mean > > > > that we don't care about ISA I/O because the platform does not > > > support having > > > > an ISA bus (e.g.). > > > > > > I think to be more specific, PCIBIOS_MIN_IO=0 would indicate that > you > > > cannot > > > have a PCI-to-ISA or PCI-to-LPC bridge in any PCI domain. This is > > > different > > > from having an LPC master outside of PCI, as that lives in its own > > > domain > > > and has a separately addressable I/O space. > > > > Yes correct so if we go for the single domain solution arch that > > have PCIBIOS_MIN_IO=0 cannot support special devices such as LPC > > unless we also redefine PCIBIOS_MIN_IO, right? > > This is what I was referring to below as the difference between > a) and b): Setting PCIBIOS_MIN_IO=0 means you cannot have LPC > behind PCI, but it shouldn't stop you from having a separate > LPC bridge. > > > > The PCIBIOS_MIN_DIRECT_IO name still suggests having something > related > > > to > > > PCIBIOS_MIN_IO, but it really isn't. We are talking about multiple > > > concepts here that are not the same but that are somewhat related: > > > > > > a) keeping PCI devices from allocating low I/O ports on the PCI bus > > > that would conflict with ISA devices behind a bridge of the > > > same bus. > > > > > > b) reserving the low 0x0-0xfff range of the Linux-internal I/O > > > space abstraction to a particular LPC or PCI domain to make > > > legacy device drivers work that hardcode a particular port > > > number. > > > > > > c) Redirecting inb/outb to call a domain-specific accessor function > > > rather than doing the normal MMIO window for an LPC master or > > > more generally any arbitrary LPC or PCI domain that has a > > > nonstandard I/O space. > > > [side note: actually if we generalized this, we could avoid > > > assigning an MMIO range for the I/O space on the pci-mvebu > > > driver, and that would help free up some other remapping > > > windows] > > > > > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO > > > today and even if we don't need it, there is no obvious downside. > > > I would also argue that we can ignore b) for the discussion of > > > the HiSilicon LPC driver, we just need to assign some range > > > of logical addresses to each domain. > > > > > > That means solving c) is the important problem here, and it > > > shouldn't be so hard. We can do this either with a single > > > special domain as in the v5 patch series, or by generalizing it > > > so that any I/O space mapping gets looked up through the device > > > pointer of the bus master. > > > > I am not very on the "generalized" multi-domain solution... > > Currently the IO accessors prototypes have an unsigned long addr > > as input parameter. If we live in a multi-domain IO system > > how can we distinguish inside the accessor which domain addr > > belongs to? > > The easiest change compared to the v5 code would be to walk > a linked list of 'struct extio_ops' structures rather than > assuming there is only ever one of them. I think one of the > earlier versions actually did this. Right but if my understanding is correct if we live in a multi- domain I/O space world when you have an input addr in the I/O accessors this addr can be duplicated (for example for the standard PCI IO domain and for our special LPC domain). So effectively even if you walk a linked list there is a problem of disambiguation...am I right? > > Another option the IA64 approach mentioned in another subthread > today, looking up the operations based on an index from the > upper bits of the port number. If we do this, we probably > want to do that for all PIO access and replace the entire > virtual address remapping logic with that. I think Bjorn > in the past argued in favor of such an approach, while I > advocated the current scheme for simplicity based on how > every I/O space these days is just memory mapped (which now > turned out to be false, both on powerpc and arm64). This seems really complex...I am a bit worried that possibly we end up in having the maintainers saying that it is not worth to re-invent the world just for this special LPC device... To be honest with you I would keep things simple for this LPC and introduce more complex reworks later if more devices need to be introduced. What if we stick on a single domain now where we introduce a reserved threshold for the IO space (say INDIRECT_MAX_IO). We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h" So effectively this threshold can change according to the architecture and so far we only define it for ARM64 as we need it for ARM64... Thoughts? Thanks again Gab > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote: > From: Arnd Bergmann [mailto:arnd@arndb.de] > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni wrote: > > > > I think there is no need to change a) here, we have PCIBIOS_MIN_IO > > > > today and even if we don't need it, there is no obvious downside. > > > > I would also argue that we can ignore b) for the discussion of > > > > the HiSilicon LPC driver, we just need to assign some range > > > > of logical addresses to each domain. > > > > > > > > That means solving c) is the important problem here, and it > > > > shouldn't be so hard. We can do this either with a single > > > > special domain as in the v5 patch series, or by generalizing it > > > > so that any I/O space mapping gets looked up through the device > > > > pointer of the bus master. > > > > > > I am not very on the "generalized" multi-domain solution... > > > Currently the IO accessors prototypes have an unsigned long addr > > > as input parameter. If we live in a multi-domain IO system > > > how can we distinguish inside the accessor which domain addr > > > belongs to? > > > > The easiest change compared to the v5 code would be to walk > > a linked list of 'struct extio_ops' structures rather than > > assuming there is only ever one of them. I think one of the > > earlier versions actually did this. > > Right but if my understanding is correct if we live in a multi- > domain I/O space world when you have an input addr in the I/O > accessors this addr can be duplicated (for example for the standard > PCI IO domain and for our special LPC domain). > So effectively even if you walk a linked list there is a problem > of disambiguation...am I right? No, unlike the PCI memory space, the PIO addresses are not usually distinct, i.e. every PCI bus has its own 64K I/O addresses starting at zero. We linearize them into the Linux I/O space using the per-domain io_offset value. For the ISA/LPC spaces there are only 4k of addresses, they the bus addresses always overlap, but we can trivially figure out the bus address from Linux I/O port number by subtracting the start of the range. > > Another option the IA64 approach mentioned in another subthread > > today, looking up the operations based on an index from the > > upper bits of the port number. If we do this, we probably > > want to do that for all PIO access and replace the entire > > virtual address remapping logic with that. I think Bjorn > > in the past argued in favor of such an approach, while I > > advocated the current scheme for simplicity based on how > > every I/O space these days is just memory mapped (which now > > turned out to be false, both on powerpc and arm64). > > This seems really complex...I am a bit worried that possibly > we end up in having the maintainers saying that it is not worth > to re-invent the world just for this special LPC device... It would clearly be a rather invasive change, but the end-result isn't necessarily more complex than what we have today, as we'd kill off the crazy pci_pio_to_address() and pci_address_to_pio() hacks in address translation. > To be honest with you I would keep things simple for this > LPC and introduce more complex reworks later if more devices > need to be introduced. > > What if we stick on a single domain now where we introduce a > reserved threshold for the IO space (say INDIRECT_MAX_IO). I said having a single domain is fine, but I still don't like the idea of reserving low port numbers for this hack, it would mean that the numbers change for everyone else. > We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and > we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h" > > So effectively this threshold can change according to the > architecture and so far we only define it for ARM64 as we need > it for ARM64... I liked the idea of having it done in asm-generic/io.h (in an ifdef) and lib/*.c under an as someone suggested earlier. There is nothing ARM64 specific in the implementation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 18 November 2016 13:43 > To: linux-arm-kernel@lists.infradead.org > Cc: Gabriele Paoloni; mark.rutland@arm.com; benh@kernel.crashing.org; > catalin.marinas@arm.com; liviu.dudau@arm.com; Linuxarm; > lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux- > serial@vger.kernel.org; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John > Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og > le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; Thomas Petazzoni; > linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote: > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni > wrote: > > > > > I think there is no need to change a) here, we have > PCIBIOS_MIN_IO > > > > > today and even if we don't need it, there is no obvious > downside. > > > > > I would also argue that we can ignore b) for the discussion of > > > > > the HiSilicon LPC driver, we just need to assign some range > > > > > of logical addresses to each domain. > > > > > > > > > > That means solving c) is the important problem here, and it > > > > > shouldn't be so hard. We can do this either with a single > > > > > special domain as in the v5 patch series, or by generalizing it > > > > > so that any I/O space mapping gets looked up through the device > > > > > pointer of the bus master. > > > > > > > > I am not very on the "generalized" multi-domain solution... > > > > Currently the IO accessors prototypes have an unsigned long addr > > > > as input parameter. If we live in a multi-domain IO system > > > > how can we distinguish inside the accessor which domain addr > > > > belongs to? > > > > > > The easiest change compared to the v5 code would be to walk > > > a linked list of 'struct extio_ops' structures rather than > > > assuming there is only ever one of them. I think one of the > > > earlier versions actually did this. > > > > Right but if my understanding is correct if we live in a multi- > > domain I/O space world when you have an input addr in the I/O > > accessors this addr can be duplicated (for example for the standard > > PCI IO domain and for our special LPC domain). > > So effectively even if you walk a linked list there is a problem > > of disambiguation...am I right? > > No, unlike the PCI memory space, the PIO addresses are not > usually distinct, i.e. every PCI bus has its own 64K I/O > addresses starting at zero. We linearize them into the > Linux I/O space using the per-domain io_offset value. I am going to summarize my understanding here below: It seems to me that what is linearized is the virtual address space associated to the IO address space. This address space goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT). The I/O accessors perform rd/wr operation on this address space using a port IO token. Each token map into a cpu physical address range Each cpu physical address range maps to a bus address range if the bus controller specifies a range property. Devices under a bus controller specify the bus addresses that they operate on in their reg property. So each device can use the same bus addresses under two separate bus controllers as long as the bus controller use the range properties to map the same bus range to different cpu address range. > > For the ISA/LPC spaces there are only 4k of addresses, they > the bus addresses always overlap, but we can trivially > figure out the bus address from Linux I/O port number > by subtracting the start of the range. Are you saying that our LPC controller should specify a range property to map bus addresses into a cpu address range? > > > > Another option the IA64 approach mentioned in another subthread > > > today, looking up the operations based on an index from the > > > upper bits of the port number. If we do this, we probably > > > want to do that for all PIO access and replace the entire > > > virtual address remapping logic with that. I think Bjorn > > > in the past argued in favor of such an approach, while I > > > advocated the current scheme for simplicity based on how > > > every I/O space these days is just memory mapped (which now > > > turned out to be false, both on powerpc and arm64). > > > > This seems really complex...I am a bit worried that possibly > > we end up in having the maintainers saying that it is not worth > > to re-invent the world just for this special LPC device... > > It would clearly be a rather invasive change, but the > end-result isn't necessarily more complex than what we > have today, as we'd kill off the crazy pci_pio_to_address() > and pci_address_to_pio() hacks in address translation. I have to look better into this...can you provide me a reference to the Bjorn argument in favor of this approach? > > > To be honest with you I would keep things simple for this > > LPC and introduce more complex reworks later if more devices > > need to be introduced. > > > > What if we stick on a single domain now where we introduce a > > reserved threshold for the IO space (say INDIRECT_MAX_IO). > > I said having a single domain is fine, but I still don't > like the idea of reserving low port numbers for this hack, > it would mean that the numbers change for everyone else. I don't get this much...I/O tokens that are passed to the I/O accessors are not fixed anyway and they vary depending on the order of adding ranges to io_range_list...so I don't see a big issue with this... > > > We define INDIRECT_MAX_IO as 0 in "include/linux/extio.h" and > > we define INDIRECT_MAX_IO as 0x1000 in "arch/arm64/include/asm/io.h" > > > > So effectively this threshold can change according to the > > architecture and so far we only define it for ARM64 as we need > > it for ARM64... > > I liked the idea of having it done in asm-generic/io.h (in an ifdef) > and lib/*.c under an as someone suggested earlier. There is nothing > ARM64 specific in the implementation. Correct and ideally if the INDIRECT_MAX_IO approach was taken then we should define INDIRECT_MAX_IO for any architecture that can support the special LPC devices. We would define it for ARM64 just because LPC is used in our case under ARM64; the idea was to leave other architecture to define their own ones as needed...but probably this is wrong and we should have defined this for all the architectures. Many thanks Gab > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote: > From: Arnd Bergmann [mailto:arnd@arndb.de] > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni wrote: > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni > > > > The easiest change compared to the v5 code would be to walk > > > > a linked list of 'struct extio_ops' structures rather than > > > > assuming there is only ever one of them. I think one of the > > > > earlier versions actually did this. > > > > > > Right but if my understanding is correct if we live in a multi- > > > domain I/O space world when you have an input addr in the I/O > > > accessors this addr can be duplicated (for example for the standard > > > PCI IO domain and for our special LPC domain). > > > So effectively even if you walk a linked list there is a problem > > > of disambiguation...am I right? > > > > No, unlike the PCI memory space, the PIO addresses are not > > usually distinct, i.e. every PCI bus has its own 64K I/O > > addresses starting at zero. We linearize them into the > > Linux I/O space using the per-domain io_offset value. > > I am going to summarize my understanding here below: > > It seems to me that what is linearized is the virtual address > space associated to the IO address space. This address space > goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT). > > The I/O accessors perform rd/wr operation on this address > space using a port IO token. > > Each token map into a cpu physical address range > Each cpu physical address range maps to a bus address range > if the bus controller specifies a range property. > > Devices under a bus controller specify the bus addresses that > they operate on in their reg property. > > So each device can use the same bus addresses under two > separate bus controllers as long as the bus controller > use the range properties to map the same bus range to different > cpu address range. Sounds all correct to me so far, yes. > > For the ISA/LPC spaces there are only 4k of addresses, they > > the bus addresses always overlap, but we can trivially > > figure out the bus address from Linux I/O port number > > by subtracting the start of the range. > > Are you saying that our LPC controller should specify a > range property to map bus addresses into a cpu address range? No. There is not CPU address associated with it, because it's not memory mapped. Instead, we need to associate a bus address with a logical Linux port number, both in of_address_to_resource and in inb()/outb(). > > > > Another option the IA64 approach mentioned in another subthread > > > > today, looking up the operations based on an index from the > > > > upper bits of the port number. If we do this, we probably > > > > want to do that for all PIO access and replace the entire > > > > virtual address remapping logic with that. I think Bjorn > > > > in the past argued in favor of such an approach, while I > > > > advocated the current scheme for simplicity based on how > > > > every I/O space these days is just memory mapped (which now > > > > turned out to be false, both on powerpc and arm64). > > > > > > This seems really complex...I am a bit worried that possibly > > > we end up in having the maintainers saying that it is not worth > > > to re-invent the world just for this special LPC device... > > > > It would clearly be a rather invasive change, but the > > end-result isn't necessarily more complex than what we > > have today, as we'd kill off the crazy pci_pio_to_address() > > and pci_address_to_pio() hacks in address translation. > > I have to look better into this...can you provide me a reference > to the Bjorn argument in favor of this approach? The thread seems to have been pci: Introduce pci_register_io_range() helper function, e.g. in https://lkml.org/lkml/2014/7/8/969 > > > To be honest with you I would keep things simple for this > > > LPC and introduce more complex reworks later if more devices > > > need to be introduced. > > > > > > What if we stick on a single domain now where we introduce a > > > reserved threshold for the IO space (say INDIRECT_MAX_IO). > > > > I said having a single domain is fine, but I still don't > > like the idea of reserving low port numbers for this hack, > > it would mean that the numbers change for everyone else. > > I don't get this much...I/O tokens that are passed to the I/O > accessors are not fixed anyway and they vary depending on the order > of adding ranges to io_range_list...so I don't see a big issue > with this... On machines with a legacy devices behind the PCI bridge, there may still be a reason to have the low I/O port range reserved for the primary bus, e.g. to get a VGA text console to work. On powerpc, this is called the "primary" PCI host, i.e. the only one that is allowed to have an ISA bridge. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 18 November 2016 16:35 > To: Gabriele Paoloni > Cc: linux-arm-kernel@lists.infradead.org; mark.rutland@arm.com; > benh@kernel.crashing.org; catalin.marinas@arm.com; liviu.dudau@arm.com; > Linuxarm; lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux- > serial@vger.kernel.org; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John > Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og > le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; T homas Petazzoni; > linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote: > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni > wrote: > > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni > > > > > The easiest change compared to the v5 code would be to walk > > > > > a linked list of 'struct extio_ops' structures rather than > > > > > assuming there is only ever one of them. I think one of the > > > > > earlier versions actually did this. > > > > > > > > Right but if my understanding is correct if we live in a multi- > > > > domain I/O space world when you have an input addr in the I/O > > > > accessors this addr can be duplicated (for example for the > standard > > > > PCI IO domain and for our special LPC domain). > > > > So effectively even if you walk a linked list there is a problem > > > > of disambiguation...am I right? > > > > > > No, unlike the PCI memory space, the PIO addresses are not > > > usually distinct, i.e. every PCI bus has its own 64K I/O > > > addresses starting at zero. We linearize them into the > > > Linux I/O space using the per-domain io_offset value. > > > > I am going to summarize my understanding here below: > > > > It seems to me that what is linearized is the virtual address > > space associated to the IO address space. This address space > > goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT). > > > > The I/O accessors perform rd/wr operation on this address > > space using a port IO token. > > > > Each token map into a cpu physical address range > > Each cpu physical address range maps to a bus address range > > if the bus controller specifies a range property. > > > > Devices under a bus controller specify the bus addresses that > > they operate on in their reg property. > > > > So each device can use the same bus addresses under two > > separate bus controllers as long as the bus controller > > use the range properties to map the same bus range to different > > cpu address range. > > Sounds all correct to me so far, yes. > > > > For the ISA/LPC spaces there are only 4k of addresses, they > > > the bus addresses always overlap, but we can trivially > > > figure out the bus address from Linux I/O port number > > > by subtracting the start of the range. > > > > Are you saying that our LPC controller should specify a > > range property to map bus addresses into a cpu address range? > > No. There is not CPU address associated with it, because it's > not memory mapped. > > Instead, we need to associate a bus address with a logical > Linux port number, both in of_address_to_resource and > in inb()/outb(). I think this is effectively what we are doing so far with patch 2/3. The problem with this patch is that we are carving out a "forbidden" IO tokens range that goes from 0 to PCIBIOS_MIN_IO. I think that the proper solution would be to have the LPC driver to set the carveout threshold used in pci_register_io_range(), pci_pio_to_address(), pci_address_to_pio(), but this would impose a probe dependency on the LPC itself that should be probed before the PCI controller (or before any other devices calling these functions...) > > > > > > Another option the IA64 approach mentioned in another subthread > > > > > today, looking up the operations based on an index from the > > > > > upper bits of the port number. If we do this, we probably > > > > > want to do that for all PIO access and replace the entire > > > > > virtual address remapping logic with that. I think Bjorn > > > > > in the past argued in favor of such an approach, while I > > > > > advocated the current scheme for simplicity based on how > > > > > every I/O space these days is just memory mapped (which now > > > > > turned out to be false, both on powerpc and arm64). > > > > > > > > This seems really complex...I am a bit worried that possibly > > > > we end up in having the maintainers saying that it is not worth > > > > to re-invent the world just for this special LPC device... > > > > > > It would clearly be a rather invasive change, but the > > > end-result isn't necessarily more complex than what we > > > have today, as we'd kill off the crazy pci_pio_to_address() > > > and pci_address_to_pio() hacks in address translation. > > > > I have to look better into this...can you provide me a reference > > to the Bjorn argument in favor of this approach? > > The thread seems to have been pci: Introduce pci_register_io_range() > helper function, e.g. in https://lkml.org/lkml/2014/7/8/969 Ok many thanks I am going to look at it > > > > > To be honest with you I would keep things simple for this > > > > LPC and introduce more complex reworks later if more devices > > > > need to be introduced. > > > > > > > > What if we stick on a single domain now where we introduce a > > > > reserved threshold for the IO space (say INDIRECT_MAX_IO). > > > > > > I said having a single domain is fine, but I still don't > > > like the idea of reserving low port numbers for this hack, > > > it would mean that the numbers change for everyone else. > > > > I don't get this much...I/O tokens that are passed to the I/O > > accessors are not fixed anyway and they vary depending on the order > > of adding ranges to io_range_list...so I don't see a big issue > > with this... > > On machines with a legacy devices behind the PCI bridge, > there may still be a reason to have the low I/O port range > reserved for the primary bus, e.g. to get a VGA text console > to work. > > On powerpc, this is called the "primary" PCI host, i.e. the > only one that is allowed to have an ISA bridge. Yes but 1) isn't the PCI controller range property that defines how IO bus address map into physical CPU addresses? 2) How can you guarantee that the cpu range associated with this IO bus range is the first to be registered in pci_register_io_range()? ( i.e. are you saying that they are just relying on the fact that it is the only IO range in the system and by chance the IO tokens and corresponding bus addresses are the same? ) Thanks Gab > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote: > > On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote: > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni > > wrote: > > > > For the ISA/LPC spaces there are only 4k of addresses, they > > > > the bus addresses always overlap, but we can trivially > > > > figure out the bus address from Linux I/O port number > > > > by subtracting the start of the range. > > > > > > Are you saying that our LPC controller should specify a > > > range property to map bus addresses into a cpu address range? > > > > No. There is not CPU address associated with it, because it's > > not memory mapped. > > > > Instead, we need to associate a bus address with a logical > > Linux port number, both in of_address_to_resource and > > in inb()/outb(). > > I think this is effectively what we are doing so far with patch 2/3. > The problem with this patch is that we are carving out a "forbidden" > IO tokens range that goes from 0 to PCIBIOS_MIN_IO. > > I think that the proper solution would be to have the LPC driver to > set the carveout threshold used in pci_register_io_range(), > pci_pio_to_address(), pci_address_to_pio(), but this would impose > a probe dependency on the LPC itself that should be probed before > the PCI controller (or before any other devices calling these > functions...) Why do you think the order matters? My point was that we should be able to register any region of logical port numbers for any bus here. > > > > > To be honest with you I would keep things simple for this > > > > > LPC and introduce more complex reworks later if more devices > > > > > need to be introduced. > > > > > > > > > > What if we stick on a single domain now where we introduce a > > > > > reserved threshold for the IO space (say INDIRECT_MAX_IO). > > > > > > > > I said having a single domain is fine, but I still don't > > > > like the idea of reserving low port numbers for this hack, > > > > it would mean that the numbers change for everyone else. > > > > > > I don't get this much...I/O tokens that are passed to the I/O > > > accessors are not fixed anyway and they vary depending on the order > > > of adding ranges to io_range_list...so I don't see a big issue > > > with this... > > > > On machines with a legacy devices behind the PCI bridge, > > there may still be a reason to have the low I/O port range > > reserved for the primary bus, e.g. to get a VGA text console > > to work. > > > > On powerpc, this is called the "primary" PCI host, i.e. the > > only one that is allowed to have an ISA bridge. > > Yes but > 1) isn't the PCI controller range property that defines how IO bus address > map into physical CPU addresses? Correct, but the DT knows nothing about logical port numbers in Linux. > 2) How can you guarantee that the cpu range associated with this > IO bus range is the first to be registered in pci_register_io_range()? > ( i.e. are you saying that they are just relying on the fact that it is the > only IO range in the system and by chance the IO tokens and corresponding > bus addresses are the same? ) To clarify: the special properties of having the first 0x1000 logical port numbers go to a particular physical bus are very obscure. I think it's more important to not change the behavior for existing systems that might rely on it than for new systems that have no such legacy. The ipmi and uart drivers in particular will get the port numbers filled in their platform device from the DT bus scanning, so they don't care at all about having the same numeric value for port numbers on the bus and logical numbers, but other drivers might rely on particular ports to be mapped on a specific PCI host, especially when those drivers are used only on systems that don't have more than one PCI domain. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: 23 November 2016 14:16 > To: Gabriele Paoloni > Cc: linux-arm-kernel@lists.infradead.org; mark.rutland@arm.com; > benh@kernel.crashing.org; catalin.marinas@arm.com; liviu.dudau@arm.com; > Linuxarm; lorenzo.pieralisi@arm.com; xuwei (O); Jason Gunthorpe; linux- > serial@vger.kernel.org; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; minyard@acm.org; will.deacon@arm.com; John > Garry; zourongrong@gmail.com; robh+dt@kernel.org; bhelgaas@go og > le.com; kantyzc@163.com; zhichang.yuan02@gmail.com; T homas Petazzoni; > linux-kernel@vger.kernel.org; Yuanzhichang; olof@lixom.net > Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on > Hip06 > > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote: > > > On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote: > > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni > > > wrote: > > > > > For the ISA/LPC spaces there are only 4k of addresses, they > > > > > the bus addresses always overlap, but we can trivially > > > > > figure out the bus address from Linux I/O port number > > > > > by subtracting the start of the range. > > > > > > > > Are you saying that our LPC controller should specify a > > > > range property to map bus addresses into a cpu address range? > > > > > > No. There is not CPU address associated with it, because it's > > > not memory mapped. > > > > > > Instead, we need to associate a bus address with a logical > > > Linux port number, both in of_address_to_resource and > > > in inb()/outb(). > > > > I think this is effectively what we are doing so far with patch 2/3. > > The problem with this patch is that we are carving out a "forbidden" > > IO tokens range that goes from 0 to PCIBIOS_MIN_IO. > > > > I think that the proper solution would be to have the LPC driver to > > set the carveout threshold used in pci_register_io_range(), > > pci_pio_to_address(), pci_address_to_pio(), but this would impose > > a probe dependency on the LPC itself that should be probed before > > the PCI controller (or before any other devices calling these > > functions...) > > Why do you think the order matters? My point was that we should > be able to register any region of logical port numbers for any > bus here. Maybe I have not followed well so let's roll back to your previous comment... "we need to associate a bus address with a logical Linux port number, both in of_address_to_resource and in inb()/outb()" Actually of_address_to_resource() returns the port number to used in inb/outb(); inb() and outb() add the port number to PCI_IOBASE to rd/wr to the right virtual address. Our LPC cannot operate on the virtual address and it operates on a bus address range that for LPC is also equal to the cpu address range and goes from 0 to 0x1000. Now as I understand it is risky and not appropriate to reserve the logical port numbers from 0 to 0x1000 or to whatever other upper bound because existing systems may rely on these port numbers retrieved by __of_address_to_resource(). In this scenario I think the best thing to do would be in the probe function of the LPC driver: 1) call pci_register_io_range() passing [0, 0x1000] (that is the range for LPC) 2) retrieve the logical port numbers associated to the LPC range by calling pci_address_to_pio() for 0 and 0x1000 and assign them to extio_ops_node->start and extio_ops_node->end 3) implement the LPC accessors to operate on the logical ports associated to the LPC range (in practice in the accessors implementation we will call pci_pio_to_address to retrieve the cpu address to operate on) What do you think? Thanks Gab > > > > > > > > To be honest with you I would keep things simple for this > > > > > > LPC and introduce more complex reworks later if more devices > > > > > > need to be introduced. > > > > > > > > > > > > What if we stick on a single domain now where we introduce a > > > > > > reserved threshold for the IO space (say INDIRECT_MAX_IO). > > > > > > > > > > I said having a single domain is fine, but I still don't > > > > > like the idea of reserving low port numbers for this hack, > > > > > it would mean that the numbers change for everyone else. > > > > > > > > I don't get this much...I/O tokens that are passed to the I/O > > > > accessors are not fixed anyway and they vary depending on the > order > > > > of adding ranges to io_range_list...so I don't see a big issue > > > > with this... > > > > > > On machines with a legacy devices behind the PCI bridge, > > > there may still be a reason to have the low I/O port range > > > reserved for the primary bus, e.g. to get a VGA text console > > > to work. > > > > > > On powerpc, this is called the "primary" PCI host, i.e. the > > > only one that is allowed to have an ISA bridge. > > > > Yes but > > 1) isn't the PCI controller range property that defines how IO bus > address > > map into physical CPU addresses? > > Correct, but the DT knows nothing about logical port numbers in Linux. > > > 2) How can you guarantee that the cpu range associated with this > > IO bus range is the first to be registered in > pci_register_io_range()? > > ( i.e. are you saying that they are just relying on the fact that > it is the > > only IO range in the system and by chance the IO tokens and > corresponding > > bus addresses are the same? ) > > To clarify: the special properties of having the first 0x1000 logical > port numbers go to a particular physical bus are very obscure. I think > it's more important to not change the behavior for existing systems > that might rely on it than for new systems that have no such legacy. > > The ipmi and uart drivers in particular will get the port numbers > filled > in their platform device from the DT bus scanning, so they don't care > at all about having the same numeric value for port numbers on the bus > and logical numbers, but other drivers might rely on particular ports > to be mapped on a specific PCI host, especially when those drivers > are used only on systems that don't have more than one PCI domain. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 23, 2016 3:22:33 PM CET Gabriele Paoloni wrote: > From: Arnd Bergmann [mailto:arnd@arndb.de] > > On Friday, November 18, 2016 5:03:11 PM CET Gabriele Paoloni wrote: > > > I think this is effectively what we are doing so far with patch 2/3. > > > The problem with this patch is that we are carving out a "forbidden" > > > IO tokens range that goes from 0 to PCIBIOS_MIN_IO. > > > > > > I think that the proper solution would be to have the LPC driver to > > > set the carveout threshold used in pci_register_io_range(), > > > pci_pio_to_address(), pci_address_to_pio(), but this would impose > > > a probe dependency on the LPC itself that should be probed before > > > the PCI controller (or before any other devices calling these > > > functions...) > > > > Why do you think the order matters? My point was that we should > > be able to register any region of logical port numbers for any > > bus here. > > Maybe I have not followed well so let's roll back to your previous > comment... > > "we need to associate a bus address with a logical Linux port number, > both in of_address_to_resource and in inb()/outb()" > > Actually of_address_to_resource() returns the port number to used > in inb/outb(); inb() and outb() add the port number to PCI_IOBASE > to rd/wr to the right virtual address. Correct. > Our LPC cannot operate on the virtual address and it operates on > a bus address range that for LPC is also equal to the cpu address > range and goes from 0 to 0x1000. There is no "cpu address" here, otherwise this is correct. > Now as I understand it is risky and not appropriate to reserve > the logical port numbers from 0 to 0x1000 or to whatever other > upper bound because existing systems may rely on these port numbers > retrieved by __of_address_to_resource(). Right again. > In this scenario I think the best thing to do would be > in the probe function of the LPC driver: > 1) call pci_register_io_range() passing [0, 0x1000] (that is the > range for LPC) pci_register_io_range() takes a physical address, not a port number, so that would not be appropriate as you say above. We can however add a variant that reserves a range of port numbers in io_range_list for an indirect access method. > 2) retrieve the logical port numbers associated to the LPC range > by calling pci_address_to_pio() for 0 and 0x1000 and assign > them to extio_ops_node->start and extio_ops_node->end Again, calling pci_address_to_pio() doesn't seem right here, because we don't have a phys_addr_t address > 3) implement the LPC accessors to operate on the logical ports > associated to the LPC range (in practice in the accessors > implementation we will call pci_pio_to_address to retrieve > the cpu address to operate on) Please don't proliferate the use of pci_pio_to_address/pci_address_to_pio here, computing the physical address from the logical address is trivial, you just need to subtract the start of the range that you already use when matching the port number range. The only thing we need here is to make of_address_to_resource() return the correct logical port number that was registered for a given host device when asked to translate an address that does not have a CPU address associated with it. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/MAINTAINERS b/MAINTAINERS index ccae35b..4c7a350 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5729,6 +5729,14 @@ F: include/uapi/linux/if_hippi.h F: net/802/hippi.c F: drivers/net/hippi/ +HISILICON LPC BUS DRIVER +M: Zhichang Yuan <yuanzhichang@hisilicon.com> +L: linux-arm-kernel@lists.infradead.org +W: http://www.hisilicon.com +S: Maintained +F: drivers/bus/hisi_lpc.c +F: Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt + HISILICON NETWORK SUBSYSTEM DRIVER M: Yisen Zhuang <yisen.zhuang@huawei.com> M: Salil Mehta <salil.mehta@huawei.com> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 7875105..4fa8ab4 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -64,6 +64,14 @@ config BRCMSTB_GISB_ARB arbiter. This driver provides timeout and target abort error handling and internal bus master decoding. +config HISILICON_LPC + bool "Workaround for nonstandard ISA I/O space on Hisilicon Hip0X" + depends on (ARCH_HISI || COMPILE_TEST) && ARM64 + select ARM64_INDIRECT_PIO + help + Driver needed for some legacy ISA devices attached to Low-Pin-Count + on Hisilicon Hip0X SoC. + config IMX_WEIM bool "Freescale EIM DRIVER" depends on ARCH_MXC diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index c6cfa6b..10b4983 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_ARM_CCI) += arm-cci.o obj-$(CONFIG_ARM_CCN) += arm-ccn.o obj-$(CONFIG_BRCMSTB_GISB_ARB) += brcmstb_gisb.o +obj-$(CONFIG_HISILICON_LPC) += hisi_lpc.o obj-$(CONFIG_IMX_WEIM) += imx-weim.o obj-$(CONFIG_MIPS_CDMM) += mips_cdmm.o obj-$(CONFIG_MVEBU_MBUS) += mvebu-mbus.o diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c new file mode 100644 index 0000000..47dc081 --- /dev/null +++ b/drivers/bus/hisi_lpc.c @@ -0,0 +1,501 @@ +/* + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved. + * Author: Zhichang Yuan <yuanzhichang@hisilicon.com> + * Author: Zou Rongrong <zourongrong@huawei.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/acpi.h> +#include <linux/console.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/pci.h> +#include <linux/serial_8250.h> +#include <linux/slab.h> + +/* + * setting this bit means each IO operation will target to different port address; + * 0 means repeatly IO operations will be sticked on the same port, such as BT; + */ +#define FG_INCRADDR_LPC 0x02 + +struct lpc_cycle_para { + unsigned int opflags; + unsigned int csize; /* the data length of each operation */ +}; + +struct hisilpc_dev { + spinlock_t cycle_lock; + void __iomem *membase; + struct extio_ops io_ops; +}; + + +/* The maximum continous operations*/ +#define LPC_MAX_OPCNT 16 +/* only support IO data unit length is four at maximum */ +#define LPC_MAX_DULEN 4 +#if LPC_MAX_DULEN > LPC_MAX_OPCNT +#error "LPC.. MAX_DULEN must be not bigger than MAX_OPCNT!" +#endif + +#define LPC_REG_START 0x00 /* start a new LPC cycle */ +#define LPC_REG_OP_STATUS 0x04 /* the current LPC status */ +#define LPC_REG_IRQ_ST 0x08 /* interrupt enable&status */ +#define LPC_REG_OP_LEN 0x10 /* how many LPC cycles each start */ +#define LPC_REG_CMD 0x14 /* command for the required LPC cycle */ +#define LPC_REG_ADDR 0x20 /* LPC target address */ +#define LPC_REG_WDATA 0x24 /* data to be written */ +#define LPC_REG_RDATA 0x28 /* data coming from peer */ + + +/* The command register fields*/ +#define LPC_CMD_SAMEADDR 0x08 +#define LPC_CMD_TYPE_IO 0x00 +#define LPC_CMD_WRITE 0x01 +#define LPC_CMD_READ 0x00 +/* the bit attribute is W1C. 1 represents OK. */ +#define LPC_STAT_BYIRQ 0x02 + +#define LPC_STATUS_IDLE 0x01 +#define LPC_OP_FINISHED 0x02 + +#define START_WORK 0x01 + +/* + * The minimal waiting interval... Suggest it is not less than 10. + * Bigger value probably will lower the performance. + */ +#define LPC_NSEC_PERWAIT 100 +/* + * The maximum waiting time is about 128us. + * The fastest IO cycle time is about 390ns, but the worst case will wait + * for extra 256 lpc clocks, so (256 + 13) * 30ns = 8 us. The maximum + * burst cycles is 16. So, the maximum waiting time is about 128us under + * worst case. + * choose 1300 as the maximum. + */ +#define LPC_MAX_WAITCNT 1300 +/* About 10us. This is specfic for single IO operation, such as inb. */ +#define LPC_PEROP_WAITCNT 100 + + +static inline int wait_lpc_idle(unsigned char *mbase, + unsigned int waitcnt) { + u32 opstatus; + + while (waitcnt--) { + ndelay(LPC_NSEC_PERWAIT); + opstatus = readl(mbase + LPC_REG_OP_STATUS); + if (opstatus & LPC_STATUS_IDLE) + return (opstatus & LPC_OP_FINISHED) ? 0 : (-EIO); + } + return -ETIME; +} + +/** + * hisilpc_target_in - trigger a series of lpc cycles to read required data + * from target periperal. + * @pdev: pointer to hisi lpc device + * @para: some paramerters used to control the lpc I/O operations + * @ptaddr: the lpc I/O target port address + * @buf: where the read back data is stored + * @opcnt: how many I/O operations required in this calling + * + * only one byte data is read each I/O operation. + * + * Returns 0 on success, non-zero on fail. + * + */ +static int hisilpc_target_in(struct hisilpc_dev *lpcdev, + struct lpc_cycle_para *para, + unsigned long ptaddr, unsigned char *buf, + unsigned long opcnt) +{ + unsigned long cnt_per_trans; + unsigned int cmd_word; + unsigned int waitcnt; + int ret; + + if (!buf || !opcnt || !para || !para->csize || !lpcdev) + return -EINVAL; + + if (opcnt > LPC_MAX_OPCNT) + return -EINVAL; + + cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_READ; + waitcnt = (LPC_PEROP_WAITCNT); + if (!(para->opflags & FG_INCRADDR_LPC)) { + cmd_word |= LPC_CMD_SAMEADDR; + waitcnt = LPC_MAX_WAITCNT; + } + + ret = 0; + cnt_per_trans = (para->csize == 1) ? opcnt : para->csize; + for (; opcnt && !ret; cnt_per_trans = para->csize) { + unsigned long flags; + + /* whole operation must be atomic */ + spin_lock_irqsave(&lpcdev->cycle_lock, flags); + + writel(cnt_per_trans, lpcdev->membase + LPC_REG_OP_LEN); + + writel(cmd_word, lpcdev->membase + LPC_REG_CMD); + + writel(ptaddr, lpcdev->membase + LPC_REG_ADDR); + + writel(START_WORK, lpcdev->membase + LPC_REG_START); + + /* whether the operation is finished */ + ret = wait_lpc_idle(lpcdev->membase, waitcnt); + if (!ret) { + opcnt -= cnt_per_trans; + for (; cnt_per_trans--; buf++) + *buf = readl(lpcdev->membase + LPC_REG_RDATA); + } + + spin_unlock_irqrestore(&lpcdev->cycle_lock, flags); + } + + return ret; +} + +/** + * hisilpc_target_out - trigger a series of lpc cycles to write required data + * to target periperal. + * @pdev: pointer to hisi lpc device + * @para: some paramerters used to control the lpc I/O operations + * @ptaddr: the lpc I/O target port address + * @buf: where the data to be written is stored + * @opcnt: how many I/O operations required + * + * only one byte data is read each I/O operation. + * + * Returns 0 on success, non-zero on fail. + * + */ +static int hisilpc_target_out(struct hisilpc_dev *lpcdev, + struct lpc_cycle_para *para, + unsigned long ptaddr, + const unsigned char *buf, + unsigned long opcnt) +{ + unsigned long cnt_per_trans; + unsigned int cmd_word; + unsigned int waitcnt; + int ret; + + if (!buf || !opcnt || !para || !lpcdev) + return -EINVAL; + + if (opcnt > LPC_MAX_OPCNT) + return -EINVAL; + /* default is increasing address */ + cmd_word = LPC_CMD_TYPE_IO | LPC_CMD_WRITE; + waitcnt = (LPC_PEROP_WAITCNT); + if (!(para->opflags & FG_INCRADDR_LPC)) { + cmd_word |= LPC_CMD_SAMEADDR; + waitcnt = LPC_MAX_WAITCNT; + } + + ret = 0; + cnt_per_trans = (para->csize == 1) ? opcnt : para->csize; + for (; opcnt && !ret; cnt_per_trans = para->csize) { + unsigned long flags; + + spin_lock_irqsave(&lpcdev->cycle_lock, flags); + + writel(cnt_per_trans, lpcdev->membase + LPC_REG_OP_LEN); + opcnt -= cnt_per_trans; + for (; cnt_per_trans--; buf++) + writel(*buf, lpcdev->membase + LPC_REG_WDATA); + + writel(cmd_word, lpcdev->membase + LPC_REG_CMD); + + writel(ptaddr, lpcdev->membase + LPC_REG_ADDR); + + writel(START_WORK, lpcdev->membase + LPC_REG_START); + + /* whether the operation is finished */ + ret = wait_lpc_idle(lpcdev->membase, waitcnt); + + spin_unlock_irqrestore(&lpcdev->cycle_lock, flags); + } + + return ret; +} + +/** + * hisilpc_comm_in - read/input the data from the I/O peripheral through LPC. + * @devobj: pointer to the device information relevant to LPC controller. + * @ptaddr: the target I/O port address. + * @dlen: the data length required to read from the target I/O port. + * + * when succeed, the data read back is stored in buffer pointed by inbuf. + * For inb, return the data read from I/O or -1 when error occur. + */ +static u64 hisilpc_comm_in(void *devobj, unsigned long ptaddr, size_t dlen) +{ + struct hisilpc_dev *lpcdev; + struct lpc_cycle_para iopara; + u32 rd_data; + unsigned char *newbuf; + int ret = 0; + + if (!devobj || !dlen || dlen > LPC_MAX_DULEN || (dlen & (dlen - 1))) + return -1; + + /* the local buffer must be enough for one data unit */ + if (sizeof(rd_data) < dlen) + return -1; + + newbuf = (unsigned char *)&rd_data; + + lpcdev = (struct hisilpc_dev *)devobj; + + iopara.opflags = FG_INCRADDR_LPC; + iopara.csize = dlen; + + ret = hisilpc_target_in(lpcdev, &iopara, ptaddr, newbuf, dlen); + if (ret) + return -1; + + return le32_to_cpu(rd_data); +} + +/** + * hisilpc_comm_out - write/output the data whose maximal length is four bytes to + * the I/O peripheral through LPC. + * @devobj: pointer to the device information relevant to LPC controller. + * @outval: a value to be outputed from caller, maximum is four bytes. + * @ptaddr: the target I/O port address. + * @dlen: the data length required writing to the target I/O port . + * + * This function is corresponding to out(b,w,l) only + * + */ +static void hisilpc_comm_out(void *devobj, unsigned long ptaddr, + u32 outval, size_t dlen) +{ + struct hisilpc_dev *lpcdev; + struct lpc_cycle_para iopara; + const unsigned char *newbuf; + + if (!devobj || !dlen || dlen > LPC_MAX_DULEN) + return; + + if (sizeof(outval) < dlen) + return; + + outval = cpu_to_le32(outval); + + newbuf = (const unsigned char *)&outval; + lpcdev = (struct hisilpc_dev *)devobj; + + iopara.opflags = FG_INCRADDR_LPC; + iopara.csize = dlen; + + hisilpc_target_out(lpcdev, &iopara, ptaddr, newbuf, dlen); +} + +/** + * hisilpc_comm_ins - read/input the data in buffer to the I/O peripheral + * through LPC, it corresponds to ins(b,w,l) + * @devobj: pointer to the device information relevant to LPC controller. + * @ptaddr: the target I/O port address. + * @inbuf: a buffer where read/input data bytes are stored. + * @dlen: the data length required writing to the target I/O port. + * @count: how many data units whose length is dlen will be read. + * + */ +static u64 hisilpc_comm_ins(void *devobj, unsigned long ptaddr, + void *inbuf, size_t dlen, unsigned int count) +{ + struct hisilpc_dev *lpcdev; + struct lpc_cycle_para iopara; + unsigned char *newbuf; + unsigned int loopcnt, cntleft; + unsigned int max_perburst; + int ret = 0; + + if (!devobj || !inbuf || !count || !dlen || + dlen > LPC_MAX_DULEN || (dlen & (dlen - 1))) + return -1; + + iopara.opflags = 0; + if (dlen > 1) + iopara.opflags |= FG_INCRADDR_LPC; + iopara.csize = dlen; + + lpcdev = (struct hisilpc_dev *)devobj; + newbuf = (unsigned char *)inbuf; + /* + * ensure data stream whose length is multiple of dlen to be processed + * each IO input + */ + max_perburst = LPC_MAX_OPCNT & (~(dlen - 1)); + cntleft = count * dlen; + do { + loopcnt = (cntleft >= max_perburst) ? max_perburst : cntleft; + ret = hisilpc_target_in(lpcdev, &iopara, ptaddr, newbuf, + loopcnt); + if (ret) + break; + newbuf += loopcnt; + cntleft -= loopcnt; + } while (cntleft); + + return ret; +} + +/** + * hisilpc_comm_outs - write/output the data in buffer to the I/O peripheral + * through LPC, it corresponds to outs(b,w,l) + * @devobj: pointer to the device information relevant to LPC controller. + * @ptaddr: the target I/O port address. + * @outbuf: a buffer where write/output data bytes are stored. + * @dlen: the data length required writing to the target I/O port . + * @count: how many data units whose length is dlen will be written. + * + */ +static void hisilpc_comm_outs(void *devobj, unsigned long ptaddr, + const void *outbuf, size_t dlen, unsigned int count) +{ + struct hisilpc_dev *lpcdev; + struct lpc_cycle_para iopara; + const unsigned char *newbuf; + unsigned int loopcnt, cntleft; + unsigned int max_perburst; + int ret = 0; + + if (!devobj || !outbuf || !count || !dlen || + dlen > LPC_MAX_DULEN || (dlen & (dlen - 1))) + return; + + iopara.opflags = 0; + if (dlen > 1) + iopara.opflags |= FG_INCRADDR_LPC; + iopara.csize = dlen; + + lpcdev = (struct hisilpc_dev *)devobj; + newbuf = (unsigned char *)outbuf; + /* + * ensure data stream whose lenght is multiple of dlen to be processed + * each IO input + */ + max_perburst = LPC_MAX_OPCNT & (~(dlen - 1)); + cntleft = count * dlen; + do { + loopcnt = (cntleft >= max_perburst) ? max_perburst : cntleft; + ret = hisilpc_target_out(lpcdev, &iopara, ptaddr, newbuf, + loopcnt); + if (ret) + break; + newbuf += loopcnt; + cntleft -= loopcnt; + } while (cntleft); +} + +/** + * hisilpc_probe - the probe callback function for hisi lpc device, + * will finish all the intialization. + * @pdev: the platform device corresponding to hisi lpc + * + * Returns 0 on success, non-zero on fail. + * + */ +static int hisilpc_probe(struct platform_device *pdev) +{ + struct resource *iores; + struct hisilpc_dev *lpcdev; + int ret; + + dev_info(&pdev->dev, "probing hslpc...\n"); + + lpcdev = devm_kzalloc(&pdev->dev, + sizeof(struct hisilpc_dev), GFP_KERNEL); + if (!lpcdev) + return -ENOMEM; + + spin_lock_init(&lpcdev->cycle_lock); + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); + lpcdev->membase = devm_ioremap_resource(&pdev->dev, iores); + if (IS_ERR(lpcdev->membase)) { + dev_err(&pdev->dev, "ioremap memory FAIL(%d)!\n", + PTR_ERR(lpcdev->membase)); + return PTR_ERR(lpcdev->membase); + } + /* + * The first PCIBIOS_MIN_IO is reserved specifically for indirectIO. + * It will separate indirectIO range from pci host bridge to + * avoid the possible PIO conflict. + * Set the indirectIO range directly here. + */ + lpcdev->io_ops.start = 0; + lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1; + lpcdev->io_ops.devpara = lpcdev; + lpcdev->io_ops.pfin = hisilpc_comm_in; + lpcdev->io_ops.pfout = hisilpc_comm_out; + lpcdev->io_ops.pfins = hisilpc_comm_ins; + lpcdev->io_ops.pfouts = hisilpc_comm_outs; + + platform_set_drvdata(pdev, lpcdev); + + arm64_set_extops(&lpcdev->io_ops); + + /* + * The children scanning is only for dts mode. For ACPI children, + * the corresponding devices had be created during acpi scanning. + */ + ret = 0; + if (!has_acpi_companion(&pdev->dev)) + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, + &pdev->dev); + + if (!ret) + dev_info(&pdev->dev, "hslpc end probing. range[0x%lx - %lx]\n", + arm64_extio_ops->start, arm64_extio_ops->end); + else + dev_info(&pdev->dev, "hslpc probing is fail(%d)\n", ret); + + return ret; +} + +static const struct of_device_id hisilpc_of_match[] = { + { + .compatible = "hisilicon,hip06-lpc", + }, + {}, +}; + +static const struct acpi_device_id hisilpc_acpi_match[] = { + {"HISI0191", }, + {}, +}; + +static struct platform_driver hisilpc_driver = { + .driver = { + .name = "hisi_lpc", + .of_match_table = hisilpc_of_match, + .acpi_match_table = hisilpc_acpi_match, + }, + .probe = hisilpc_probe, +}; + + +builtin_platform_driver(hisilpc_driver);