[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bf00d184-137d-a2fa-b202-f18ce131fdd6@huawei.com>
Date: Fri, 2 Mar 2018 10:33:26 +0000
From: John Garry <john.garry@...wei.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
<mika.westerberg@...ux.intel.com>, <rafael@...nel.org>,
<lorenzo.pieralisi@....com>, <rjw@...ysocki.net>,
<hanjun.guo@...aro.org>, <robh+dt@...nel.org>,
<bhelgaas@...gle.com>, <arnd@...db.de>, <mark.rutland@....com>,
<olof@...om.net>, <dann.frazier@...onical.com>,
<andy.shevchenko@...il.com>, <robh@...nel.org>
CC: <joe@...ches.com>, <benh@...nel.crashing.org>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-acpi@...r.kernel.org>, <linuxarm@...wei.com>,
<minyard@....org>, <devicetree@...r.kernel.org>,
<linux-arch@...r.kernel.org>, <rdunlap@...radead.org>,
<gregkh@...uxfoundation.org>, <akpm@...ux-foundation.org>,
<frowand.list@...il.com>, <agraf@...e.de>
Subject: Re: [PATCH v15 1/9] LIB: Introduce a generic PIO mapping method
On 01/03/2018 19:11, Andy Shevchenko wrote:
> On Tue, 2018-02-27 at 00:40 +0800, John Garry wrote:
>> From: Zhichang Yuan <yuanzhichang@...ilicon.com>
>>
>> In commit 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
>> pci_pio_to_address()"), a new I/O space management was supported. With
>> that driver, the I/O ranges configured for PCI/PCIe hosts on some
>> architectures can be mapped to logical PIO, converted easily between
>> CPU address and the corresponding logicial PIO. Based on this, PCI
>> I/O devices can be accessed in a memory read/write way through the
>> unified in/out accessors.
>>
>> But on some archs/platforms, there are bus hosts which access I/O
>> peripherals with host-local I/O port addresses rather than memory
>> addresses after memory-mapped.
>>
>> To support those devices, a more generic I/O mapping method is
>> introduced
>> here. Through this patch, both the CPU addresses and the host-local
>> port
>> can be mapped into the logical PIO space with different logical/fake
>> PIOs.
>> After this, all the I/O accesses to either PCI MMIO devices or host-
>> local
>> I/O peripherals can be unified into the existing I/O accessors defined
>> in
>> asm-generic/io.h and be redirected to the right device-specific hooks
>> based on the input logical PIO.
>>
>
> A bit more small comments.
>
>
Hi Andy,
>> +#ifndef __LINUX_LOGIC_PIO_H
>> +#define __LINUX_LOGIC_PIO_H
>> +
>
>> +#ifdef __KERNEL__
>
> Hmm... How the header in include/linux/ can be non-kernel?
I did doubt the legitimacy of this. I think it should be ok to remove.
>
>> + list_for_each_entry_rcu(range, &io_range_list, list) {
>
>> + if (range->flags == LOGIC_PIO_CPU_MMIO &&
>> + new_range->flags ==
>> LOGIC_PIO_CPU_MMIO) {
>
> It's rather fancy indentation.
Right, I should align these.
>
>> + if (start >= range->hw_start + range->size ||
>> + end < range->hw_start)
>
> Misses {}
Right, I will fix.
>
>> + allocated_mmio_size += range->size;
>> + else {
>> + ret = -EFAULT;
>> + goto end_register;
>> + }
>
>> + if (allocated_mmio_size + new_range->size - 1 >
>> + MMIO_UPPER_LIMIT) {
>
> Fancy indentation.
>
>> + if (allocated_mmio_size + SZ_64K - 1 >
>> + MMIO_UPPER_LIMIT) {
>
> Ditto.
Will fix both
>
>> + if (allocated_iio_size + new_range->size - 1 >
>> + IO_SPACE_LIMIT) {
>
> While this is nothing wrong, like above I would consider to check how
> long it is and consider putting on one line.
Fine. Some symbol names can be shortened to aid this.
>
>> +/**
>> + * logic_pio_to_hwaddr - translate logical PIO to HW address
>> + * @pio: logical PIO value
>> + *
>> + * Returns HW address if valid, -1 otherwise
>
> It can't return -1 when the return type is unsigned.
>
>> + *
>> + * Translate the input logical pio to the corresponding hardware
>> address.
>> + * The input pio should be unique in the whole logical PIO space.
>> + */
>> +resource_size_t logic_pio_to_hwaddr(unsigned long pio)
>> +{
>> + struct logic_pio_hwaddr *range;
>
>> + resource_size_t hwaddr = (resource_size_t)-1;
>
> Ditto.
ok, I should have changed this to ~0 also.
>
>> + return hwaddr;
>> +}
>
>> + list_for_each_entry_rcu(range, &io_range_list, list) {
>> + if (range->flags != LOGIC_PIO_CPU_MMIO)
>> + continue;
>
>> + if (addr >= range->hw_start &&
>> + addr < range->hw_start + range->size)
>
> Perhaps you may copy'n'paste in_range() macro from fs/ext* files and use
> in this module. At some point it would be good to make it generic.
OK, I should be able to copy. It would not be good to include
fs/ufs/util.h, but a generic helper would be useful.
>
>> + return addr - range->hw_start +
>> + range->io_start;
>
>
>> + }
>
>
Thanks very much,
John
Powered by blists - more mailing lists