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':quoted
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 */
^~~~~~~~quoted
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 [off-list ref] 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 [off-list ref]
This should not be indented.
quoted hunk
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.
quoted hunk
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?
quoted hunk
+ 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.
quoted hunk
@@ -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': >> includ=
e/linux/kern_levels.h:5:18: warning: format '%lx' expects argument of >> typ=
e 'long unsigned int', but argument 2 has type 'resource_size_t {aka long >>=
long unsigned int}' [-Wformat=3D] #define KERN_SOH "\001" /* ASCII Start Of=
Header */
=E2=80=94-
I was able to fix this small format issue. I replaced the format '%08lx' wit=
h '%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 [off-list ref] wrote:
=20
quoted
+
+ printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start);
=20
That's INFO or even DEBUG.
quoted
=20
Michael,
What do you think about this fix?
+ printk(KERN_INFO "NEMO SB600 IOB base %08llx\n",res.start);
Thanks,
Christian=
Christian Zigotzky [off-list ref] 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': >> inc=
lude/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 lo=
ng >> long unsigned int}' [-Wformat=3D] #define KERN_SOH "\001" /* ASCII St=
art Of Header */
=E2=80=94-
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?
quoted
On 3. May 2018, at 15:06, Michael Ellerman [off-list ref] wrote:
=20
quoted
+
+ printk(KERN_CRIT "NEMO SB600 IOB base %08lx\n",res.start);
=20
That's INFO or even DEBUG.
quoted
=20
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=20
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=E2=80=99t reach Darren then I will try to fix the issues but I thin=
k I need your help for fixing them.
Thanks,
Christian
On 10. Jul 2018, at 15:50, Michael Ellerman wrote:
=20
pr_info() would be nice.
=20
But I replied with lots of other comments previously.
=20
None of them were super critical, but it would be nice to get them fixed
before merging if possible.
=20
cheers
Christian Zigotzky [off-list ref] writes:
Hello Michael,
Thanks a lot for your reply. OK, first I would like to add=20
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=E2=80=99t reach Darren then I will try to fix the issues but I t=
hink
I need your help for fixing them.
Sure.
cheers