Re: [PATCH v15 1/9] LIB: Introduce a generic PIO mapping method

[John Garry]

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@xxxxxxxxxxxxx>
> >
> > 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