Message ID | 4b3d1ee02c7.3de0aa2c@auth.smtp.1and1.co.uk (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | [NEXT,1/4] powerpc/pasemi: Add PCI initialisation for Nemo board. | expand |
Hi Darren, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on powerpc/next] [also build test WARNING on v4.15-rc6 next-20180103] [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/Darren-Stevens/powerpc-pasemi-Add-PCI-initialisation-for-Nemo-board/20180103-091349 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-allmodconfig (attached as .config) compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All warnings (new ones prefixed by >>): In file included from include/linux/printk.h:7:0, from include/linux/kernel.h:14, from arch/powerpc/platforms/pasemi/pci.c:26: arch/powerpc/platforms/pasemi/pci.c: In function 'sb600_set_flag': >> include/linux/kern_levels.h:5:18: warning: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=] #define KERN_SOH "\001" /* ASCII Start Of Header */ ^ include/linux/kern_levels.h:10:19: note: in expansion of macro 'KERN_SOH' #define KERN_CRIT KERN_SOH "2" /* critical conditions */ ^~~~~~~~ >> arch/powerpc/platforms/pasemi/pci.c:137:10: note: in expansion of macro 'KERN_CRIT' printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); ^~~~~~~~~ arch/powerpc/platforms/pasemi/pci.c:137:45: note: format string is defined here printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); ~~~~^ %08llx vim +/KERN_CRIT +137 arch/powerpc/platforms/pasemi/pci.c > 26 #include <linux/kernel.h> 27 #include <linux/pci.h> 28 29 #include <asm/pci-bridge.h> 30 #include <asm/isa-bridge.h> 31 #include <asm/machdep.h> 32 33 #include <asm/ppc-pci.h> 34 35 #include "pasemi.h" 36 37 #define PA_PXP_CFA(bus, devfn, off) (((bus) << 20) | ((devfn) << 12) | (off)) 38 39 static inline int pa_pxp_offset_valid(u8 bus, u8 devfn, int offset) 40 { 41 /* Device 0 Function 0 is special: It's config space spans function 1 as 42 * well, so allow larger offset. It's really a two-function device but the 43 * second function does not probe. 44 */ 45 if (bus == 0 && devfn == 0) 46 return offset < 8192; 47 else 48 return offset < 4096; 49 } 50 51 static void volatile __iomem *pa_pxp_cfg_addr(struct pci_controller *hose, 52 u8 bus, u8 devfn, int offset) 53 { 54 return hose->cfg_data + PA_PXP_CFA(bus, devfn, offset); 55 } 56 57 static inline int is_root_port(int busno, int devfn) 58 { 59 return ((busno == 0) && (PCI_FUNC(devfn) < 4) && 60 ((PCI_SLOT(devfn) == 16) || (PCI_SLOT(devfn) == 17))); 61 } 62 63 static inline int is_5945_reg(int reg) 64 { 65 return (((reg >= 0x18) && (reg < 0x34)) || 66 ((reg >= 0x158) && (reg < 0x178))); 67 } 68 69 static int workaround_5945(struct pci_bus *bus, unsigned int devfn, 70 int offset, int len, u32 *val) 71 { 72 struct pci_controller *hose; 73 void volatile __iomem *addr, *dummy; 74 int byte; 75 u32 tmp; 76 77 if (!is_root_port(bus->number, devfn) || !is_5945_reg(offset)) 78 return 0; 79 80 hose = pci_bus_to_host(bus); 81 82 addr = pa_pxp_cfg_addr(hose, bus->number, devfn, offset & ~0x3); 83 byte = offset & 0x3; 84 85 /* Workaround bug 5945: write 0 to a dummy register before reading, 86 * and write back what we read. We must read/write the full 32-bit 87 * contents so we need to shift and mask by hand. 88 */ 89 dummy = pa_pxp_cfg_addr(hose, bus->number, devfn, 0x10); 90 out_le32(dummy, 0); 91 tmp = in_le32(addr); 92 out_le32(addr, tmp); 93 94 switch (len) { 95 case 1: 96 *val = (tmp >> (8*byte)) & 0xff; 97 break; 98 case 2: 99 if (byte == 0) 100 *val = tmp & 0xffff; 101 else 102 *val = (tmp >> 16) & 0xffff; 103 break; 104 default: 105 *val = tmp; 106 break; 107 } 108 109 return 1; 110 } 111 112 #ifdef CONFIG_PPC_PASEMI_NEMO 113 static int sb600_bus = 5; 114 static void __iomem *iob_mapbase = NULL; 115 116 static void sb600_set_flag(int bus) 117 { 118 struct resource res; 119 struct device_node *dn; 120 int err; 121 122 if (iob_mapbase == NULL) { 123 dn = of_find_compatible_node(NULL, "isa", "pasemi,1682m-iob"); 124 if (!dn) { 125 printk(KERN_CRIT "NEMO SB600 missing iob node\n"); 126 return; 127 } 128 129 err = of_address_to_resource(dn, 0, &res); 130 of_node_put(dn); 131 132 if (err) { 133 printk(KERN_CRIT "NEMO SB600 missing resource\n"); 134 return; 135 } 136 > 137 printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); 138 139 iob_mapbase = ioremap(res.start + 0x100, 0x94); 140 } 141 142 if (iob_mapbase != NULL) { 143 if (bus == sb600_bus) { 144 /* 145 * This is the SB600's bus, tell the PCI-e root port 146 * to allow non-zero devices to enumerate. 147 */ 148 out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) | 0x800); 149 } else { 150 /* 151 * Only scan device 0 on other busses 152 */ 153 out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) & ~0x800); 154 } 155 } 156 } 157 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Darren, Thanks for the patch, sorry it's taken so long to get merged. Darren Stevens <darren@stevens-zone.net> writes: > The A-Eon Amigaone X1000's Nemo motherboard has an AMD SB600 > connected to one of the PCI-e root ports on its PaSemi > Pwrficient 1628M SoC. Normally the SB600 southbridge would be > connected to a hidden PCI-e port on the system's northbridge, > and as a result doesn't fully comply with the PCI-e spec. > > Add code to relax the PCI-e detection in both the root port > and the Linux kernel allowing on board devices to be detected. > > Signed-off-by: Darren Stevens <Darren@stevens-zone.net> This should not be indented. > diff --git a/arch/powerpc/platforms/pasemi/pasemi.h b/arch/powerpc/platforms/pasemi/pasemi.h > index 329d2a6..8900dee 100644 > --- a/arch/powerpc/platforms/pasemi/pasemi.h > +++ b/arch/powerpc/platforms/pasemi/pasemi.h > @@ -3,6 +3,9 @@ > #define _PASEMI_PASEMI_H > > extern unsigned long pas_get_boot_time(void); > +#ifdef CONFIG_PPC_PASEMI_NEMO > +extern void nemo_pci_init(void); > +#endif We don't need an #ifdef around this thanks. > diff --git a/arch/powerpc/platforms/pasemi/pci.c b/arch/powerpc/platforms/pasemi/pci.c > index 5ff6108..cb0ac87 100644 > --- a/arch/powerpc/platforms/pasemi/pci.c > +++ b/arch/powerpc/platforms/pasemi/pci.c > @@ -108,6 +109,92 @@ static int workaround_5945(struct pci_bus *bus, unsigned int devfn, > return 1; > } > > +#ifdef CONFIG_PPC_PASEMI_NEMO > +static int sb600_bus = 5; That's only used once so could just be a #define, or even a literal in the code. > +static void __iomem *iob_mapbase = NULL; That's only used in sb6000_set_flag() so should be in there shouldn't it? > +static void sb600_set_flag(int bus) > +{ > + struct resource res; > + struct device_node *dn; > + int err; > + > + if (iob_mapbase == NULL) { > + dn = of_find_compatible_node(NULL, "isa", "pasemi,1682m-iob"); > + if (!dn) { > + printk(KERN_CRIT "NEMO SB600 missing iob node\n"); I'm not sure CRIT is really necessary, we tend to use ERR, but I don't mind that much. But can you use pr_err()/pr_crit() please. > + return; > + } > + > + err = of_address_to_resource(dn, 0, &res); > + of_node_put(dn); > + > + if (err) { > + printk(KERN_CRIT "NEMO SB600 missing resource\n"); > + return; > + } > + > + printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); That's INFO or even DEBUG. > + > + iob_mapbase = ioremap(res.start + 0x100, 0x94); 0x94 ? It's going to map you at least one page anyway. > + } > + > + if (iob_mapbase != NULL) { > + if (bus == sb600_bus) { > + /* > + * This is the SB600's bus, tell the PCI-e root port > + * to allow non-zero devices to enumerate. > + */ > + out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) | 0x800); Does reg #4 have a name? Or 0x800 ? > + } else { > + /* > + * Only scan device 0 on other busses > + */ > + out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) & ~0x800); > + } > + } > +} > + > +static int nemo_pxp_read_config(struct pci_bus *bus, unsigned int devfn, > + int offset, int len, u32 *val) > +{ > + struct pci_controller *hose; Can we call them host or controller or something in new code? > + void volatile __iomem *addr; Does that need to be volatile? > + hose = pci_bus_to_host(bus); > + if (!hose) > + return PCIBIOS_DEVICE_NOT_FOUND; > + > + if (!pa_pxp_offset_valid(bus->number, devfn, offset)) > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + if (workaround_5945(bus, devfn, offset, len, val)) > + return PCIBIOS_SUCCESSFUL; > + > + addr = pa_pxp_cfg_addr(hose, bus->number, devfn, offset); > + > + sb600_set_flag(bus->number); > + > + /* > + * Note: the caller has already checked that offset is > + * suitably aligned and that len is 1, 2 or 4. > + */ > + switch (len) { > + case 1: > + *val = in_8(addr); > + break; > + case 2: > + *val = in_le16(addr); > + break; > + default: > + *val = in_le32(addr); > + break; > + } > + > + return PCIBIOS_SUCCESSFUL; > +} > +#endif > + > static int pa_pxp_read_config(struct pci_bus *bus, unsigned int devfn, > int offset, int len, u32 *val) > { > @@ -178,6 +265,20 @@ static int pa_pxp_write_config(struct pci_bus *bus, unsigned int devfn, > return PCIBIOS_SUCCESSFUL; > } > > +#ifdef CONFIG_PPC_PASEMI_NEMO > +static struct pci_ops nemo_pxp_ops = { > + .read = nemo_pxp_read_config, > + .write = pa_pxp_write_config, > +}; > + > +static void __init setup_nemo_pxp(struct pci_controller *hose) > +{ > + hose->ops = &nemo_pxp_ops; > + hose->cfg_data = ioremap(0xe0000000, 0x10000000); > +} Could these go in one of the ifdef block below? Just to reduce the number of times we ifdef NEMO. > @@ -213,6 +343,28 @@ static int __init pas_add_bridge(struct device_node *dev) > return 0; > } > > +#ifdef CONFIG_PPC_PASEMI_NEMO > +void __init nemo_pci_init(void) > +{ > + struct device_node *np, *root; > + > + root = of_find_node_by_path("/"); > + if (!root) { > + printk(KERN_CRIT "pas_pci_init: can't find root " > + "of device tree\n"); TBH that can't really happen, or if it does we're not going to boot elsewhere. So the printk() is probably overkill. > + return; > + } > + > + pci_set_flags(PCI_SCAN_ALL_PCIE_DEVS); > + > + for (np = NULL; (np = of_get_next_child(root, np)) != NULL;) Does the pxp node not have a compatible property? (I may have asked that before). Normally you'd search by compatible, not name. If you have to search by name then of_get_child_by_name() should work. > + if (np->name && !strcmp(np->name, "pxp") && !nemo_add_bridge(np)) > + of_node_get(np); Why are we of_node_get()'ing here? If nemo_add_bridge() wants to keep a reference it should do that itself. > + > + of_node_put(root); > +} > +#endif > + > void __init pas_pci_init(void) > { > struct device_node *np, *root; cheers
Hi Michael, Hi All, kbuild test robot Wed, 03 Jan 2018 04:17:20 -0800 wrote: Hi Darren, Thank you for the patch! Perhaps something to improve: arch/powerpc/platforms/pasemi/pci.c: In function 'sb600_set_flag': >> include/linux/kern_levels.h:5:18: warning: format '%lx' expects argument of >> type 'long unsigned int', but argument 2 has type 'resource_size_t {aka long >> long unsigned int}' [-Wformat=] #define KERN_SOH "\001" /* ASCII Start Of Header */ —- I was able to fix this small format issue. I replaced the format '%08lx' with '%08llx'. + printk(KERN_CRIT "NEMO SB600 IOB base %08llx\n",res.start); Is this fix OK or is there a better solution? > On 3. May 2018, at 15:06, Michael Ellerman <mpe@ellerman.id.au> wrote: > >> + >> + printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); > > That's INFO or even DEBUG. >> Michael, What do you think about this fix? + printk(KERN_INFO "NEMO SB600 IOB base %08llx\n",res.start); Thanks, Christian
Christian Zigotzky <chzigotzky@xenosoft.de> writes: > Hi Michael, > Hi All, > > kbuild test robot Wed, 03 Jan 2018 04:17:20 -0800 wrote: > > Hi Darren, > > Thank you for the patch! Perhaps something to improve: > > arch/powerpc/platforms/pasemi/pci.c: In function 'sb600_set_flag': >> include/linux/kern_levels.h:5:18: warning: format '%lx' expects argument of >> type 'long unsigned int', but argument 2 has type 'resource_size_t {aka long >> long unsigned int}' [-Wformat=] #define KERN_SOH "\001" /* ASCII Start Of Header */ > > —- > > I was able to fix this small format issue. I replaced the format '%08lx' with '%08llx'. > > + printk(KERN_CRIT "NEMO SB600 IOB base %08llx\n",res.start); > > Is this fix OK or is there a better solution? > >> On 3. May 2018, at 15:06, Michael Ellerman <mpe@ellerman.id.au> wrote: >> >>> + >>> + printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); >> >> That's INFO or even DEBUG. >>> > > Michael, > > What do you think about this fix? > > + printk(KERN_INFO "NEMO SB600 IOB base %08llx\n",res.start); pr_info() would be nice. But I replied with lots of other comments previously. None of them were super critical, but it would be nice to get them fixed before merging if possible. cheers
Hello Michael, Thanks a lot for your reply. OK, first I would like to add pr_info("NEMO SB600 IOB base %08llx\n",res.start) to the Nemo patch. Is this line correct now? After that I will try to contact Darren because of your other comments. If I don’t reach Darren then I will try to fix the issues but I think I need your help for fixing them. Thanks, Christian > On 10. Jul 2018, at 15:50, Michael Ellerman wrote: > > pr_info() would be nice. > > But I replied with lots of other comments previously. > > None of them were super critical, but it would be nice to get them fixed > before merging if possible. > > cheers
Christian Zigotzky <chzigotzky@xenosoft.de> writes: > Hello Michael, > > Thanks a lot for your reply. OK, first I would like to add > > pr_info("NEMO SB600 IOB base %08llx\n",res.start) > > to the Nemo patch. Is this line correct now? Yes I think so. The type of start is resource_size_t, which can be 32 or 64-bits. But in this code it should always be 64-bit. > After that I will try to contact Darren because of your other comments. > > If I don’t reach Darren then I will try to fix the issues but I think > I need your help for fixing them. Sure. cheers
diff --git a/arch/powerpc/platforms/pasemi/pasemi.h b/arch/powerpc/platforms/pasemi/pasemi.h index 329d2a6..8900dee 100644 --- a/arch/powerpc/platforms/pasemi/pasemi.h +++ b/arch/powerpc/platforms/pasemi/pasemi.h @@ -3,6 +3,9 @@ #define _PASEMI_PASEMI_H extern unsigned long pas_get_boot_time(void); +#ifdef CONFIG_PPC_PASEMI_NEMO +extern void nemo_pci_init(void); +#endif extern void pas_pci_init(void); extern void pas_pci_irq_fixup(struct pci_dev *dev); extern void pas_pci_dma_dev_setup(struct pci_dev *dev); diff --git a/arch/powerpc/platforms/pasemi/pci.c b/arch/powerpc/platforms/pasemi/pci.c index 5ff6108..cb0ac87 100644 --- a/arch/powerpc/platforms/pasemi/pci.c +++ b/arch/powerpc/platforms/pasemi/pci.c @@ -27,6 +27,7 @@ #include <linux/pci.h> #include <asm/pci-bridge.h> +#include <asm/isa-bridge.h> #include <asm/machdep.h> #include <asm/ppc-pci.h> @@ -108,6 +109,92 @@ static int workaround_5945(struct pci_bus *bus, unsigned int devfn, return 1; } +#ifdef CONFIG_PPC_PASEMI_NEMO +static int sb600_bus = 5; +static void __iomem *iob_mapbase = NULL; + +static void sb600_set_flag(int bus) +{ + struct resource res; + struct device_node *dn; + int err; + + if (iob_mapbase == NULL) { + dn = of_find_compatible_node(NULL, "isa", "pasemi,1682m-iob"); + if (!dn) { + printk(KERN_CRIT "NEMO SB600 missing iob node\n"); + return; + } + + err = of_address_to_resource(dn, 0, &res); + of_node_put(dn); + + if (err) { + printk(KERN_CRIT "NEMO SB600 missing resource\n"); + return; + } + + printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start); + + iob_mapbase = ioremap(res.start + 0x100, 0x94); + } + + if (iob_mapbase != NULL) { + if (bus == sb600_bus) { + /* + * This is the SB600's bus, tell the PCI-e root port + * to allow non-zero devices to enumerate. + */ + out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) | 0x800); + } else { + /* + * Only scan device 0 on other busses + */ + out_le32(iob_mapbase + 4, in_le32(iob_mapbase + 4) & ~0x800); + } + } +} + +static int nemo_pxp_read_config(struct pci_bus *bus, unsigned int devfn, + int offset, int len, u32 *val) +{ + struct pci_controller *hose; + void volatile __iomem *addr; + + hose = pci_bus_to_host(bus); + if (!hose) + return PCIBIOS_DEVICE_NOT_FOUND; + + if (!pa_pxp_offset_valid(bus->number, devfn, offset)) + return PCIBIOS_BAD_REGISTER_NUMBER; + + if (workaround_5945(bus, devfn, offset, len, val)) + return PCIBIOS_SUCCESSFUL; + + addr = pa_pxp_cfg_addr(hose, bus->number, devfn, offset); + + sb600_set_flag(bus->number); + + /* + * Note: the caller has already checked that offset is + * suitably aligned and that len is 1, 2 or 4. + */ + switch (len) { + case 1: + *val = in_8(addr); + break; + case 2: + *val = in_le16(addr); + break; + default: + *val = in_le32(addr); + break; + } + + return PCIBIOS_SUCCESSFUL; +} +#endif + static int pa_pxp_read_config(struct pci_bus *bus, unsigned int devfn, int offset, int len, u32 *val) { @@ -178,6 +265,20 @@ static int pa_pxp_write_config(struct pci_bus *bus, unsigned int devfn, return PCIBIOS_SUCCESSFUL; } +#ifdef CONFIG_PPC_PASEMI_NEMO +static struct pci_ops nemo_pxp_ops = { + .read = nemo_pxp_read_config, + .write = pa_pxp_write_config, +}; + +static void __init setup_nemo_pxp(struct pci_controller *hose) +{ + hose->ops = &nemo_pxp_ops; + hose->cfg_data = ioremap(0xe0000000, 0x10000000); +} + +#endif + static struct pci_ops pa_pxp_ops = { .read = pa_pxp_read_config, .write = pa_pxp_write_config, @@ -189,6 +290,35 @@ static void __init setup_pa_pxp(struct pci_controller *hose) hose->cfg_data = ioremap(0xe0000000, 0x10000000); } +#ifdef CONFIG_PPC_PASEMI_NEMO +static int __init nemo_add_bridge(struct device_node *dev) +{ + struct pci_controller *hose; + + pr_debug("Adding PCI host bridge %pOF\n", dev); + + hose = pcibios_alloc_controller(dev); + if (!hose) + return -ENOMEM; + + hose->first_busno = 0; + hose->last_busno = 0xff; + hose->controller_ops = pasemi_pci_controller_ops; + + setup_nemo_pxp(hose); + + printk(KERN_INFO "Found PA-PXP PCI host bridge.\n"); + + /* Interpret the "ranges" property */ + pci_process_bridge_OF_ranges(hose, dev, 1); + + /* Scan for an isa bridge. */ + isa_bridge_find_early(hose); + + return 0; +} +#endif + static int __init pas_add_bridge(struct device_node *dev) { struct pci_controller *hose; @@ -213,6 +343,28 @@ static int __init pas_add_bridge(struct device_node *dev) return 0; } +#ifdef CONFIG_PPC_PASEMI_NEMO +void __init nemo_pci_init(void) +{ + struct device_node *np, *root; + + root = of_find_node_by_path("/"); + if (!root) { + printk(KERN_CRIT "pas_pci_init: can't find root " + "of device tree\n"); + return; + } + + pci_set_flags(PCI_SCAN_ALL_PCIE_DEVS); + + for (np = NULL; (np = of_get_next_child(root, np)) != NULL;) + if (np->name && !strcmp(np->name, "pxp") && !nemo_add_bridge(np)) + of_node_get(np); + + of_node_put(root); +} +#endif + void __init pas_pci_init(void) { struct device_node *np, *root;
The A-Eon Amigaone X1000's Nemo motherboard has an AMD SB600 connected to one of the PCI-e root ports on its PaSemi Pwrficient 1628M SoC. Normally the SB600 southbridge would be connected to a hidden PCI-e port on the system's northbridge, and as a result doesn't fully comply with the PCI-e spec. Add code to relax the PCI-e detection in both the root port and the Linux kernel allowing on board devices to be detected. Signed-off-by: Darren Stevens <Darren@stevens-zone.net> ---