| Message ID | BLU437-SMTP183977690F008FC3A8059BBF9A0@phx.gbl |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 15 July 2015 at 02:23, Bin Meng <bmeng.cn@gmail.com> wrote: > Currently during writing MP table I/O interrupt assignment entry, we > assume the PIRQ is directly mapped to I/O APIC INTPIN#16-23, which > however is not always the case on some platforms. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/x86/include/asm/mpspec.h | 17 +++++++++++++++++ > arch/x86/lib/mpspec.c | 23 ++++++++++++++++------- > 2 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h > index efa9231..ad8eba9 100644 > --- a/arch/x86/include/asm/mpspec.h > +++ b/arch/x86/include/asm/mpspec.h > @@ -432,6 +432,23 @@ void mp_write_compat_address_space(struct mp_config_table *mc, int busid, > u32 mptable_finalize(struct mp_config_table *mc); > > /** > + * mp_determine_pci_dstirq() - Determine PCI device's int pin on the I/O APIC > + * > + * This determines a PCI device's interrupt pin number on the I/O APIC. > + * > + * This can be implemented by platform codes to handle specifal cases, which > + * do not conform to the normal chipset/board design where PIRQ[A-H] are mapped > + * directly to I/O APIC INTPIN#16-23. > + * > + * @bus: bus number of the pci device > + * @dev: device number of the pci device > + * @func: function number of the pci device > + * @pirq: PIRQ number the PCI device's interrupt pin is routed to > + * @return: interrupt pin number on the I/O APIC > + */ > +int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq); > + > +/** > * write_mp_table() - Write MP table > * > * This writes MP table at a given address. > diff --git a/arch/x86/lib/mpspec.c b/arch/x86/lib/mpspec.c > index f16fbcb..f90567c 100644 > --- a/arch/x86/lib/mpspec.c > +++ b/arch/x86/lib/mpspec.c > @@ -269,6 +269,12 @@ static bool check_dup_entry(struct mpc_config_intsrc *intsrc_base, > return (i == entry_num) ? false : true; > } > > +__weak int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq) I'd like to avoid using weak functions. Can we use device tree or driver model instead? In principle we should be able to build all of the code in and boot an image on multiple boards. These sorts of things make it harder to understand and follow the code IMO. > +{ > + /* PIRQ[A-H] are connected to I/O APIC INTPIN#16-23 */ > + return pirq + 16; > +} > + > static int mptable_add_intsrc(struct mp_config_table *mc, > int bus_isa, int apicid) > { > @@ -304,24 +310,27 @@ static int mptable_add_intsrc(struct mp_config_table *mc, > > for (i = 0; i < count; i++) { > struct pirq_routing pr; > + int bus, dev, func; > + int dstirq; > > pr.bdf = fdt_addr_to_cpu(cell[0]); > pr.pin = fdt_addr_to_cpu(cell[1]); > pr.pirq = fdt_addr_to_cpu(cell[2]); > + bus = PCI_BUS(pr.bdf); > + dev = PCI_DEV(pr.bdf); > + func = PCI_FUNC(pr.bdf); > > if (check_dup_entry(intsrc_base, intsrc_entries, > - PCI_BUS(pr.bdf), PCI_DEV(pr.bdf), pr.pin)) { > + bus, dev, pr.pin)) { > debug("found entry for bus %d device %d INT%c, skipping\n", > - PCI_BUS(pr.bdf), PCI_DEV(pr.bdf), > - 'A' + pr.pin - 1); > + bus, dev, 'A' + pr.pin - 1); > cell += sizeof(struct pirq_routing) / sizeof(u32); > continue; > } > > - /* PIRQ[A-H] are always connected to I/O APIC INTPIN#16-23 */ > - mp_write_pci_intsrc(mc, MP_INT, PCI_BUS(pr.bdf), > - PCI_DEV(pr.bdf), pr.pin, apicid, > - pr.pirq + 16); > + dstirq = mp_determine_pci_dstirq(bus, dev, func, pr.pirq); > + mp_write_pci_intsrc(mc, MP_INT, bus, dev, pr.pin, > + apicid, dstirq); > intsrc_entries++; > cell += sizeof(struct pirq_routing) / sizeof(u32); > } > -- > 1.8.2.1 > Regards, Simon
Hi Simon, On Sat, Jul 18, 2015 at 10:37 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 15 July 2015 at 02:23, Bin Meng <bmeng.cn@gmail.com> wrote: >> Currently during writing MP table I/O interrupt assignment entry, we >> assume the PIRQ is directly mapped to I/O APIC INTPIN#16-23, which >> however is not always the case on some platforms. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> arch/x86/include/asm/mpspec.h | 17 +++++++++++++++++ >> arch/x86/lib/mpspec.c | 23 ++++++++++++++++------- >> 2 files changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h >> index efa9231..ad8eba9 100644 >> --- a/arch/x86/include/asm/mpspec.h >> +++ b/arch/x86/include/asm/mpspec.h >> @@ -432,6 +432,23 @@ void mp_write_compat_address_space(struct mp_config_table *mc, int busid, >> u32 mptable_finalize(struct mp_config_table *mc); >> >> /** >> + * mp_determine_pci_dstirq() - Determine PCI device's int pin on the I/O APIC >> + * >> + * This determines a PCI device's interrupt pin number on the I/O APIC. >> + * >> + * This can be implemented by platform codes to handle specifal cases, which >> + * do not conform to the normal chipset/board design where PIRQ[A-H] are mapped >> + * directly to I/O APIC INTPIN#16-23. >> + * >> + * @bus: bus number of the pci device >> + * @dev: device number of the pci device >> + * @func: function number of the pci device >> + * @pirq: PIRQ number the PCI device's interrupt pin is routed to >> + * @return: interrupt pin number on the I/O APIC >> + */ >> +int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq); >> + >> +/** >> * write_mp_table() - Write MP table >> * >> * This writes MP table at a given address. >> diff --git a/arch/x86/lib/mpspec.c b/arch/x86/lib/mpspec.c >> index f16fbcb..f90567c 100644 >> --- a/arch/x86/lib/mpspec.c >> +++ b/arch/x86/lib/mpspec.c >> @@ -269,6 +269,12 @@ static bool check_dup_entry(struct mpc_config_intsrc *intsrc_base, >> return (i == entry_num) ? false : true; >> } >> >> +__weak int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq) > > I'd like to avoid using weak functions. Can we use device tree or > driver model instead? > As you can see in this patch http://patchwork.ozlabs.org/patch/495524/, that the return value of this function (dstirq) relies on the runtime value, so we cannot describe it in the device tree. As for driver model, I am not sure if if it suits. I will need think about it. If we do driver model, we might be able to consolidate PIRQ and MP table codes. Can we leave this to a possible future work? > In principle we should be able to build all of the code in and boot an > image on multiple boards. These sorts of things make it harder to > understand and follow the code IMO. > Yep, ideally although I don't know how practical for now :-) [snip] Regards, Bin
Hi Bin, On 18 July 2015 at 10:30, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sat, Jul 18, 2015 at 10:37 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 15 July 2015 at 02:23, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Currently during writing MP table I/O interrupt assignment entry, we >>> assume the PIRQ is directly mapped to I/O APIC INTPIN#16-23, which >>> however is not always the case on some platforms. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> arch/x86/include/asm/mpspec.h | 17 +++++++++++++++++ >>> arch/x86/lib/mpspec.c | 23 ++++++++++++++++------- >>> 2 files changed, 33 insertions(+), 7 deletions(-) >>> >>> diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h >>> index efa9231..ad8eba9 100644 >>> --- a/arch/x86/include/asm/mpspec.h >>> +++ b/arch/x86/include/asm/mpspec.h >>> @@ -432,6 +432,23 @@ void mp_write_compat_address_space(struct mp_config_table *mc, int busid, >>> u32 mptable_finalize(struct mp_config_table *mc); >>> >>> /** >>> + * mp_determine_pci_dstirq() - Determine PCI device's int pin on the I/O APIC >>> + * >>> + * This determines a PCI device's interrupt pin number on the I/O APIC. >>> + * >>> + * This can be implemented by platform codes to handle specifal cases, which >>> + * do not conform to the normal chipset/board design where PIRQ[A-H] are mapped >>> + * directly to I/O APIC INTPIN#16-23. >>> + * >>> + * @bus: bus number of the pci device >>> + * @dev: device number of the pci device >>> + * @func: function number of the pci device >>> + * @pirq: PIRQ number the PCI device's interrupt pin is routed to >>> + * @return: interrupt pin number on the I/O APIC >>> + */ >>> +int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq); >>> + >>> +/** >>> * write_mp_table() - Write MP table >>> * >>> * This writes MP table at a given address. >>> diff --git a/arch/x86/lib/mpspec.c b/arch/x86/lib/mpspec.c >>> index f16fbcb..f90567c 100644 >>> --- a/arch/x86/lib/mpspec.c >>> +++ b/arch/x86/lib/mpspec.c >>> @@ -269,6 +269,12 @@ static bool check_dup_entry(struct mpc_config_intsrc *intsrc_base, >>> return (i == entry_num) ? false : true; >>> } >>> >>> +__weak int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq) >> >> I'd like to avoid using weak functions. Can we use device tree or >> driver model instead? >> > > As you can see in this patch > http://patchwork.ozlabs.org/patch/495524/, that the return value of > this function (dstirq) relies on the runtime value, so we cannot > describe it in the device tree. As for driver model, I am not sure if > if it suits. I will need think about it. If we do driver model, we > might be able to consolidate PIRQ and MP table codes. Can we leave > this to a possible future work? OK. Please add a TODO for now. > >> In principle we should be able to build all of the code in and boot an >> image on multiple boards. These sorts of things make it harder to >> understand and follow the code IMO. >> > > Yep, ideally although I don't know how practical for now :-) > > [snip] Well Rome wasn't built in a day. Let's get this in as is while you figure it out. Regards, Simon
diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h index efa9231..ad8eba9 100644 --- a/arch/x86/include/asm/mpspec.h +++ b/arch/x86/include/asm/mpspec.h @@ -432,6 +432,23 @@ void mp_write_compat_address_space(struct mp_config_table *mc, int busid, u32 mptable_finalize(struct mp_config_table *mc); /** + * mp_determine_pci_dstirq() - Determine PCI device's int pin on the I/O APIC + * + * This determines a PCI device's interrupt pin number on the I/O APIC. + * + * This can be implemented by platform codes to handle specifal cases, which + * do not conform to the normal chipset/board design where PIRQ[A-H] are mapped + * directly to I/O APIC INTPIN#16-23. + * + * @bus: bus number of the pci device + * @dev: device number of the pci device + * @func: function number of the pci device + * @pirq: PIRQ number the PCI device's interrupt pin is routed to + * @return: interrupt pin number on the I/O APIC + */ +int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq); + +/** * write_mp_table() - Write MP table * * This writes MP table at a given address. diff --git a/arch/x86/lib/mpspec.c b/arch/x86/lib/mpspec.c index f16fbcb..f90567c 100644 --- a/arch/x86/lib/mpspec.c +++ b/arch/x86/lib/mpspec.c @@ -269,6 +269,12 @@ static bool check_dup_entry(struct mpc_config_intsrc *intsrc_base, return (i == entry_num) ? false : true; } +__weak int mp_determine_pci_dstirq(int bus, int dev, int func, int pirq) +{ + /* PIRQ[A-H] are connected to I/O APIC INTPIN#16-23 */ + return pirq + 16; +} + static int mptable_add_intsrc(struct mp_config_table *mc, int bus_isa, int apicid) { @@ -304,24 +310,27 @@ static int mptable_add_intsrc(struct mp_config_table *mc, for (i = 0; i < count; i++) { struct pirq_routing pr; + int bus, dev, func; + int dstirq; pr.bdf = fdt_addr_to_cpu(cell[0]); pr.pin = fdt_addr_to_cpu(cell[1]); pr.pirq = fdt_addr_to_cpu(cell[2]); + bus = PCI_BUS(pr.bdf); + dev = PCI_DEV(pr.bdf); + func = PCI_FUNC(pr.bdf); if (check_dup_entry(intsrc_base, intsrc_entries, - PCI_BUS(pr.bdf), PCI_DEV(pr.bdf), pr.pin)) { + bus, dev, pr.pin)) { debug("found entry for bus %d device %d INT%c, skipping\n", - PCI_BUS(pr.bdf), PCI_DEV(pr.bdf), - 'A' + pr.pin - 1); + bus, dev, 'A' + pr.pin - 1); cell += sizeof(struct pirq_routing) / sizeof(u32); continue; } - /* PIRQ[A-H] are always connected to I/O APIC INTPIN#16-23 */ - mp_write_pci_intsrc(mc, MP_INT, PCI_BUS(pr.bdf), - PCI_DEV(pr.bdf), pr.pin, apicid, - pr.pirq + 16); + dstirq = mp_determine_pci_dstirq(bus, dev, func, pr.pirq); + mp_write_pci_intsrc(mc, MP_INT, bus, dev, pr.pin, + apicid, dstirq); intsrc_entries++; cell += sizeof(struct pirq_routing) / sizeof(u32); }
Currently during writing MP table I/O interrupt assignment entry, we assume the PIRQ is directly mapped to I/O APIC INTPIN#16-23, which however is not always the case on some platforms. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- arch/x86/include/asm/mpspec.h | 17 +++++++++++++++++ arch/x86/lib/mpspec.c | 23 ++++++++++++++++------- 2 files changed, 33 insertions(+), 7 deletions(-)