On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote: > +#include <asm/addrspace.h> > +#include "pci-sh7751.h" > + > +#define pcic_writel(val, base, reg) __raw_writel(val, base + (reg)) > +#define pcic_readl(base, reg) __raw_readl(base + (reg)) __raw_writel()/__raw_readl() has a number of problems with atomicity (the compiler may split or merge pointer dereferences), barriers and endianess. You should normally always use readl()/writel() instead. > + memset(pci_config, 0, sizeof(pci_config)); > + if (of_property_read_u32_array(np, "renesas,config", > + pci_config, SH7751_NUM_CONFIG) == 0) { > + for (i = 0; i < SH7751_NUM_CONFIG; i++) { > + r = pci_config[i * 2]; > + /* CONFIG0 is read-only, so make it a sentinel. */ > + if (r == 0) > + break; > + pcic_writel(pci_config[i * 2 + 1], pcic, r * 4); > + } > + } the config property seems a little too specific to this implementation of the driver. Instead of encoding register values in DT, I think these should either be described in named properties where needed, or hardcoded in the driver if there is only one sensible value. > +/* > + * 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. > + */ > +#define IO_REGION_BASE 0x1000 > +resource_size_t pcibios_align_resource(void *data, const struct > resource *res, > + resource_size_t size, resource_size_t align) > +{ You can't have these generic functions in a driver, as that prevents you from building more than one such driver. The logic you have here is neither architecture nor driver specific. > +static int sh7751_pci_probe(struct platform_device *pdev) > +{ > + struct resource *res, *bscres; > + void __iomem *pcic; > + void __iomem *bsc; > + u32 memory[2]; > + u16 vid, did; > + u32 word; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (IS_ERR(res)) > + return PTR_ERR(res); > + pcic = (void __iomem *)res->start; This cast is invalid, as res->start is a physical address that you would need to ioremap() to turn into an __iomem pointer. > + bscres = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + bsc = devm_ioremap_resource(&pdev->dev, bscres); > + if (IS_ERR(bsc)) > + return PTR_ERR(bsc); > + > + if (of_property_read_u32_array(pdev->dev.of_node, > + "renesas,memory", memory, 2) < 0) { > + /* > + * If no memory range is specified, > + * the entire main memory will be targeted for DMA. > + */ > + memory[0] = memory_start; > + memory[1] = memory_end - memory_start; > + } There is a generic "dma-ranges" proerty for describing which memory is visible by a bus. > diff --git a/drivers/pci/controller/pci-sh7751.h > b/drivers/pci/controller/pci-sh7751.h > new file mode 100644 > index 000000000000..540cee7095c6 > --- /dev/null > +++ b/drivers/pci/controller/pci-sh7751.h > @@ -0,0 +1,76 @@ If the header is only included from one file, just removed it and add the register definitions to the driver directly. Arnd