Re: [PATCH v2 12/17] sh: Add PCI host bridge driver for SH7751

[Yoshinori Sato]

On Mon, 13 Jun 2016 17:38:28 +0900,
Arnd Bergmann wrote:
> 
> On Sunday, June 12, 2016 3:54:30 PM CEST Yoshinori Sato wrote:
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/sh7751-pci.txt
> > +		ranges = <0x02000000 0x00000000 0xfd000000 0xfd000000 0x00000000 0x01000000>,
> > +		         <0x01000000 0x00000000 0xfe240000 0x00000000 0x00000000 0x00040000>;
> > +		reg = <0xfe200000 0x0400>,
> > +		      <0x0c000000 0x04000000>,
> > +		      <0xff800000 0x0030>;
> > +		#interrupt-cells = <1>;
> > +		interrupt-map-mask = <0x1800 0 7>;
> > +		interrupt-map = <0x0000 0 1 &cpldintc evt2irq(0x2a0) 0
> > +		                 0x0000 0 2 &cpldintc evt2irq(0x2c0) 0
> > +		                 0x0000 0 3 &cpldintc evt2irq(0x2e0) 0
> > +		                 0x0000 0 4 &cpldintc evt2irq(0x300) 0
> > +
> > +		                 0x0800 0 1 &cpldintc evt2irq(0x2c0) 0
> > +		                 0x0800 0 2 &cpldintc evt2irq(0x2e0) 0
> > +		                 0x0800 0 3 &cpldintc evt2irq(0x300) 0
> > +		                 0x0800 0 4 &cpldintc evt2irq(0x2a0) 0
> 
> Is this not the default swizzling? I would guess that you can just
> list the interrupt once.
> 
> The formatting is slightly inconsistent here, my recommendation is
> to always enclose each entry in '< >' so you have a comma-separated
> list.

OK.
I'll fix this.

> > +
> > +#define pcic_writel(val, reg) __raw_writel(val, pci_reg_base + (reg))
> > +#define pcic_readl(reg) __raw_readl(pci_reg_base + (reg))
> 
> We generally try not to use __raw_*() accessors in drivers, because
> they are not portable, lack barriers and atomicity, and don't work
> when you change endianess.

OK.
It cpied old style driver.
Update ioread/write.

> > +unsigned long PCIBIOS_MIN_IO;
> > +unsigned long PCIBIOS_MIN_MEM;
> 
> These should probably be in architecture code, otherwise you cannot
> build more than one such driver into the kernel.
> 
> > +DEFINE_RAW_SPINLOCK(pci_config_lock);
> 
> This seems unnecessary, the config operations are always called
> under a spinlock already.

OK.
remove it.

> > +static __initconst const struct fixups {
> > +	char *compatible;
> > +	void (*fixup)(void __iomem *, void __iomem *);
> > +} fixup_list[] = {
> > +	{
> > +		.compatible = "iodata,landisk",
> > +		.fixup = landisk_fixup,
> > +	},
> > +};
> 
> Just move the fixup pointer into the existing of match table
> as the .data pointer, or add a structure to which .data
> points.

OK.

> > +/*
> > + * Functions for accessing PCI configuration space with type 1 accesses
> > + */
> > +static int sh4_pci_read(struct pci_bus *bus, unsigned int devfn,
> > +			   int where, int size, u32 *val)
> > +{
> > +	struct gen_pci *pci = bus->sysdata;
> > +	void __iomem *pci_reg_base;
> > +	unsigned long flags;
> > +	u32 data;
> > +
> > +	pci_reg_base = ioremap(pci->cfg.res.start,
> > +			       pci->cfg.res.end - pci->cfg.res.start);
> 
> You cannot call normally ioremap from pci_read, because it
> gets called under a spinlock and ioremap can normally sleep
> (that might be architecture specific).
> 
> Better map it a probe time, or use fixmap.

OK.

> > +	/*
> > +	 * PCIPDR may only be accessed as 32 bit words,
> > +	 * so we must do byte alignment by hand
> > +	 */
> > +	raw_spin_lock_irqsave(&pci_config_lock, flags);
> > +	pcic_writel(CONFIG_CMD(bus, devfn, where), SH4_PCIPAR);
> > +	data = pcic_readl(SH4_PCIPDR);
> > +	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> 
> This is shorter to express this using a 'map' function
> in pci_ops combined with pci_generic_config_read32
> and pci_generic_config_write32.

OK.

> > +/*
> > + * Called after each bus is probed, but before its children
> > + * are examined.
> > + */
> > +void pcibios_fixup_bus(struct pci_bus *bus)
> > +{
> > +}
> 
> This doesn't really belong into the driver.

OK.

> > +/*
> > + * We need to avoid collisions with `mirrored' VGA ports
> > + * and other strange ISA hardware, so we always want the
> > + * addresses to be allocated in the 0x000-0x0ff region
> > + * modulo 0x400.
> > + */
> > +resource_size_t pcibios_align_resource(void *data,
> > +				       const struct resource *res,
> > +				       resource_size_t size,
> > +				       resource_size_t align)
> > +{
> > +	resource_size_t start = res->start;
> > +
> > +	return start;
> > +}
> 
> The comment does not match what the function does, you should
> change one or the other. Also move it to the same file as
> pcibios_fixup_bus.

OK.
This part copied old driver.
I will cleanup.

> > +int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
> > +			enum pci_mmap_state mmap_state, int write_combine)
> > +{
> > +	/*
> > +	 * I/O space can be accessed via normal processor loads and stores on
> > +	 * this platform but for now we elect not to do this and portable
> > +	 * drivers should not do this anyway.
> > +	 */
> > +
> > +	if (mmap_state == pci_mmap_io)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Ignore write-combine; for now only return uncached mappings.
> > +	 */
> > +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > +
> > +	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> > +			       vma->vm_end - vma->vm_start,
> > +			       vma->vm_page_prot);
> > +}
> 
> Do you actually need HAVE_PCI_MMAP? Maybe you can just unset that
> and remove the function here.

Not set this.
Remove function.

> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	pci_reg_base = ioremap(res->start, res->end - res->start);
> > +	if (IS_ERR(pci_reg_base))
> > +		return PTR_ERR(pci_reg_base);
> > +
> > +	wres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	if (IS_ERR(wres))
> > +		return PTR_ERR(wres);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > +	bcr = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(bcr))
> 
> 
> You map three distinct memory resources here, but your
> binding just describes one as "reg: base address and
> length of the PCI controller registers".

OK.

> If there are multiple register ranges, please list each
> one in the binding and document in which order they
> are expected, or use named resources and list the required
> names.
>

This controller have single register area.
I will fix dts.

Thanks.

> 	Arnd

-- 
Yoshinori Sato
<ysato@xxxxxxxxxxxxxxxxxxxx>