| Message ID | 1387224459-25746-4-git-send-email-david.a.cohen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, Dec 16, 2013 at 1:07 PM, David Cohen <david.a.cohen@linux.intel.com> wrote: > This code was partially based on Mark Brown's previous work. > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com> > Signed-off-by: Fei Yang <fei.yang@intel.com> > Cc: Mark F. Brown <mark.f.brown@intel.com> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> I know this has already been merged to Linus' tree, but it looks funny to me. > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > @@ -16,3 +16,4 @@ > */ > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak)); > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak)); > +extern void * __init get_tangier_ops(void) __attribute__((weak)); We should use "__weak" instead of the gcc-specific "__attribute__((weak))". I don't think it's a good idea to use __weak on a declaration in a header file. If there are ever multiple definitions of the symbol, they are *all* made weak symbols, and one is chosen based on link order, which is error-prone. I only see one definition now, but the whole point of weak is to allow multiple definitions, so this looks like a problem waiting to happen. See 10629d711ed, for example. It look me a bit to figure out that these get_*_ops() functions are used by INTEL_MID_OPS_INIT, which constructs the name using a macro, so grep/cscope/etc. don't see any users. A comment pointing to INTEL_MID_OPS_INIT would be helpful. What's the reason for making these symbols weak? Normally we use weak to make a generic default version of a function, while allowing architectures to replace the default with their own version if necessary. But I don't see that happening here. Maybe I'm just missing it, like I missed the uses of get_tangier_ops(), et al. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote: > On Mon, Dec 16, 2013 at 1:07 PM, David Cohen > <david.a.cohen@linux.intel.com> wrote: > > This code was partially based on Mark Brown's previous work. > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com> > > Signed-off-by: Fei Yang <fei.yang@intel.com> > > Cc: Mark F. Brown <mark.f.brown@intel.com> > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > I know this has already been merged to Linus' tree, but it looks funny to me. > > > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > > @@ -16,3 +16,4 @@ > > */ > > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak)); > > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak)); > > +extern void * __init get_tangier_ops(void) __attribute__((weak)); > > We should use "__weak" instead of the gcc-specific "__attribute__((weak))". > > I don't think it's a good idea to use __weak on a declaration in a > header file. If there are ever multiple definitions of the symbol, > they are *all* made weak symbols, and one is chosen based on link > order, which is error-prone. I only see one definition now, but the > whole point of weak is to allow multiple definitions, so this looks > like a problem waiting to happen. See 10629d711ed, for example. > > It look me a bit to figure out that these get_*_ops() functions are > used by INTEL_MID_OPS_INIT, which constructs the name using a macro, > so grep/cscope/etc. don't see any users. A comment pointing to > INTEL_MID_OPS_INIT would be helpful. > > What's the reason for making these symbols weak? Normally we use weak > to make a generic default version of a function, while allowing > architectures to replace the default with their own version if > necessary. But I don't see that happening here. Maybe I'm just > missing it, like I missed the uses of get_tangier_ops(), et al. Intel mid was implemented in such way that we should select which soc to be used in compilation time. Depending on the selection, mfld.c or mrfl.c could not be compiled then some symbols wouldn't be available. But IMHO this is a bad legacy design that exists in there, so I started to rework it as you can see in this commit: commit 4cb9b00f42e07830310319a07e6c91413ee8153e Author: David Cohen <david.a.cohen@linux.intel.com> Date: Mon Dec 16 17:37:26 2013 -0800 x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID configs I'm sending more patches soon and getting rid of intel_mid_weak_decls.h file is in my TODO list. Br, David -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 27, 2014 at 6:30 PM, David Cohen <david.a.cohen@linux.intel.com> wrote: > Hi Bjorn, > > On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote: >> On Mon, Dec 16, 2013 at 1:07 PM, David Cohen >> <david.a.cohen@linux.intel.com> wrote: >> > This code was partially based on Mark Brown's previous work. >> > >> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com> >> > Signed-off-by: Fei Yang <fei.yang@intel.com> >> > Cc: Mark F. Brown <mark.f.brown@intel.com> >> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> I know this has already been merged to Linus' tree, but it looks funny to me. >> >> > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h >> > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h >> > @@ -16,3 +16,4 @@ >> > */ >> > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak)); >> > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak)); >> > +extern void * __init get_tangier_ops(void) __attribute__((weak)); >> >> We should use "__weak" instead of the gcc-specific "__attribute__((weak))". >> >> I don't think it's a good idea to use __weak on a declaration in a >> header file. If there are ever multiple definitions of the symbol, >> they are *all* made weak symbols, and one is chosen based on link >> order, which is error-prone. I only see one definition now, but the >> whole point of weak is to allow multiple definitions, so this looks >> like a problem waiting to happen. See 10629d711ed, for example. >> >> It look me a bit to figure out that these get_*_ops() functions are >> used by INTEL_MID_OPS_INIT, which constructs the name using a macro, >> so grep/cscope/etc. don't see any users. A comment pointing to >> INTEL_MID_OPS_INIT would be helpful. >> >> What's the reason for making these symbols weak? Normally we use weak >> to make a generic default version of a function, while allowing >> architectures to replace the default with their own version if >> necessary. But I don't see that happening here. Maybe I'm just >> missing it, like I missed the uses of get_tangier_ops(), et al. > > Intel mid was implemented in such way that we should select which soc to > be used in compilation time. Depending on the selection, mfld.c or > mrfl.c could not be compiled then some symbols wouldn't be available. > > But IMHO this is a bad legacy design that exists in there, so I started > to rework it as you can see in this commit: > > commit 4cb9b00f42e07830310319a07e6c91413ee8153e > Author: David Cohen <david.a.cohen@linux.intel.com> > Date: Mon Dec 16 17:37:26 2013 -0800 > > x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID > configs > > I'm sending more patches soon and getting rid of intel_mid_weak_decls.h > file is in my TODO list. Sounds good. While you're looking at it, I have similar questions about ipc_device_handler() and msic_generic_platform_data(). It's not clear to me why they should be weak. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 28, 2014 at 11:40:57AM -0700, Bjorn Helgaas wrote: > On Mon, Jan 27, 2014 at 6:30 PM, David Cohen > <david.a.cohen@linux.intel.com> wrote: > > Hi Bjorn, > > > > On Mon, Jan 27, 2014 at 05:52:30PM -0700, Bjorn Helgaas wrote: > >> On Mon, Dec 16, 2013 at 1:07 PM, David Cohen > >> <david.a.cohen@linux.intel.com> wrote: > >> > This code was partially based on Mark Brown's previous work. > >> > > >> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com> > >> > Signed-off-by: Fei Yang <fei.yang@intel.com> > >> > Cc: Mark F. Brown <mark.f.brown@intel.com> > >> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >> > >> I know this has already been merged to Linus' tree, but it looks funny to me. > >> > >> > --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > >> > +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h > >> > @@ -16,3 +16,4 @@ > >> > */ > >> > extern void * __cpuinit get_penwell_ops(void) __attribute__((weak)); > >> > extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak)); > >> > +extern void * __init get_tangier_ops(void) __attribute__((weak)); > >> > >> We should use "__weak" instead of the gcc-specific "__attribute__((weak))". > >> > >> I don't think it's a good idea to use __weak on a declaration in a > >> header file. If there are ever multiple definitions of the symbol, > >> they are *all* made weak symbols, and one is chosen based on link > >> order, which is error-prone. I only see one definition now, but the > >> whole point of weak is to allow multiple definitions, so this looks > >> like a problem waiting to happen. See 10629d711ed, for example. > >> > >> It look me a bit to figure out that these get_*_ops() functions are > >> used by INTEL_MID_OPS_INIT, which constructs the name using a macro, > >> so grep/cscope/etc. don't see any users. A comment pointing to > >> INTEL_MID_OPS_INIT would be helpful. > >> > >> What's the reason for making these symbols weak? Normally we use weak > >> to make a generic default version of a function, while allowing > >> architectures to replace the default with their own version if > >> necessary. But I don't see that happening here. Maybe I'm just > >> missing it, like I missed the uses of get_tangier_ops(), et al. > > > > Intel mid was implemented in such way that we should select which soc to > > be used in compilation time. Depending on the selection, mfld.c or > > mrfl.c could not be compiled then some symbols wouldn't be available. > > > > But IMHO this is a bad legacy design that exists in there, so I started > > to rework it as you can see in this commit: > > > > commit 4cb9b00f42e07830310319a07e6c91413ee8153e > > Author: David Cohen <david.a.cohen@linux.intel.com> > > Date: Mon Dec 16 17:37:26 2013 -0800 > > > > x86, intel-mid: Remove deprecated X86_MDFLD and X86_WANT_INTEL_MID > > configs > > > > I'm sending more patches soon and getting rid of intel_mid_weak_decls.h > > file is in my TODO list. > > Sounds good. While you're looking at it, I have similar questions > about ipc_device_handler() and msic_generic_platform_data(). It's not > clear to me why they should be weak. I'm afraid that's gargabe I missed. It supposed to be removed already. The original upstreamed patch set needed it, since all platform data were gathered in a board.c file and some of them could not be compiled. You can see it here: http://us.generation-nt.com/answer/patch-v2-00-10-rework-arch-x86-platform-mrst-intel-mid-help-212689892.html But I reworked this approach and added a sfi_device() macro to let compiler to gather all the platform data, thus board.c file doesn't exist. It means it is not necessary anymore to be weak. I can send a patch right away fixing it. Thanks for pointing that out. Br, David > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h index f8a831431fe0..e34e097b6f9d 100644 --- a/arch/x86/include/asm/intel-mid.h +++ b/arch/x86/include/asm/intel-mid.h @@ -52,6 +52,7 @@ enum intel_mid_cpu_type { /* 1 was Moorestown */ INTEL_MID_CPU_CHIP_PENWELL = 2, INTEL_MID_CPU_CHIP_CLOVERVIEW, + INTEL_MID_CPU_CHIP_TANGIER, }; extern enum intel_mid_cpu_type __intel_mid_cpu_chip; @@ -82,6 +83,7 @@ struct intel_mid_ops { #define INTEL_MID_OPS_INIT {\ DECLARE_INTEL_MID_OPS_INIT(penwell, INTEL_MID_CPU_CHIP_PENWELL), \ DECLARE_INTEL_MID_OPS_INIT(cloverview, INTEL_MID_CPU_CHIP_CLOVERVIEW), \ + DECLARE_INTEL_MID_OPS_INIT(tangier, INTEL_MID_CPU_CHIP_TANGIER) \ }; #ifdef CONFIG_X86_INTEL_MID diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c index 51384ca727ad..84b9d672843d 100644 --- a/arch/x86/pci/intel_mid_pci.c +++ b/arch/x86/pci/intel_mid_pci.c @@ -31,6 +31,7 @@ #include <asm/pci_x86.h> #include <asm/hw_irq.h> #include <asm/io_apic.h> +#include <asm/intel-mid.h> #define PCIE_CAP_OFFSET 0x100 @@ -219,7 +220,10 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) irq_attr.ioapic = mp_find_ioapic(dev->irq); irq_attr.ioapic_pin = dev->irq; irq_attr.trigger = 1; /* level */ - irq_attr.polarity = 1; /* active low */ + if (intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER) + irq_attr.polarity = 0; /* active high */ + else + irq_attr.polarity = 1; /* active low */ io_apic_set_pci_routing(&dev->dev, dev->irq, &irq_attr); return 0; diff --git a/arch/x86/platform/intel-mid/Makefile b/arch/x86/platform/intel-mid/Makefile index 78a14ba0e0db..0a8ee703b9fa 100644 --- a/arch/x86/platform/intel-mid/Makefile +++ b/arch/x86/platform/intel-mid/Makefile @@ -1,4 +1,4 @@ -obj-$(CONFIG_X86_INTEL_MID) += intel-mid.o intel_mid_vrtc.o mfld.o +obj-$(CONFIG_X86_INTEL_MID) += intel-mid.o intel_mid_vrtc.o mfld.o mrfl.o obj-$(CONFIG_EARLY_PRINTK_INTEL_MID) += early_printk_intel_mid.o # SFI specific code diff --git a/arch/x86/platform/intel-mid/intel-mid.c b/arch/x86/platform/intel-mid/intel-mid.c index 40955841bb32..1bbedc4b0f88 100644 --- a/arch/x86/platform/intel-mid/intel-mid.c +++ b/arch/x86/platform/intel-mid/intel-mid.c @@ -116,6 +116,10 @@ static void intel_mid_arch_setup(void) case 0x35: __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_CLOVERVIEW; break; + case 0x3C: + case 0x4A: + __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_TANGIER; + break; case 0x27: default: __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_PENWELL; diff --git a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h index 9ebce0447edf..a537ffc16299 100644 --- a/arch/x86/platform/intel-mid/intel_mid_weak_decls.h +++ b/arch/x86/platform/intel-mid/intel_mid_weak_decls.h @@ -16,3 +16,4 @@ */ extern void * __cpuinit get_penwell_ops(void) __attribute__((weak)); extern void * __cpuinit get_cloverview_ops(void) __attribute__((weak)); +extern void * __init get_tangier_ops(void) __attribute__((weak)); diff --git a/arch/x86/platform/intel-mid/mrfl.c b/arch/x86/platform/intel-mid/mrfl.c new file mode 100644 index 000000000000..09d10159e7b7 --- /dev/null +++ b/arch/x86/platform/intel-mid/mrfl.c @@ -0,0 +1,103 @@ +/* + * mrfl.c: Intel Merrifield platform specific setup code + * + * (C) Copyright 2013 Intel Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; version 2 + * of the License. + */ + +#include <linux/init.h> + +#include <asm/apic.h> +#include <asm/intel-mid.h> + +#include "intel_mid_weak_decls.h" + +static unsigned long __init tangier_calibrate_tsc(void) +{ + unsigned long fast_calibrate; + u32 lo, hi, ratio, fsb, bus_freq; + + /* *********************** */ + /* Compute TSC:Ratio * FSB */ + /* *********************** */ + + /* Compute Ratio */ + rdmsr(MSR_PLATFORM_INFO, lo, hi); + pr_debug("IA32 PLATFORM_INFO is 0x%x : %x\n", hi, lo); + + ratio = (lo >> 8) & 0xFF; + pr_debug("ratio is %d\n", ratio); + if (!ratio) { + pr_err("Read a zero ratio, force tsc ratio to 4 ...\n"); + ratio = 4; + } + + /* Compute FSB */ + rdmsr(MSR_FSB_FREQ, lo, hi); + pr_debug("Actual FSB frequency detected by SOC 0x%x : %x\n", + hi, lo); + + bus_freq = lo & 0x7; + pr_debug("bus_freq = 0x%x\n", bus_freq); + + if (bus_freq == 0) + fsb = FSB_FREQ_100SKU; + else if (bus_freq == 1) + fsb = FSB_FREQ_100SKU; + else if (bus_freq == 2) + fsb = FSB_FREQ_133SKU; + else if (bus_freq == 3) + fsb = FSB_FREQ_167SKU; + else if (bus_freq == 4) + fsb = FSB_FREQ_83SKU; + else if (bus_freq == 5) + fsb = FSB_FREQ_400SKU; + else if (bus_freq == 6) + fsb = FSB_FREQ_267SKU; + else if (bus_freq == 7) + fsb = FSB_FREQ_333SKU; + else { + BUG(); + pr_err("Invalid bus_freq! Setting to minimal value!\n"); + fsb = FSB_FREQ_100SKU; + } + + /* TSC = FSB Freq * Resolved HFM Ratio */ + fast_calibrate = ratio * fsb; + pr_debug("calculate tangier tsc %lu KHz\n", fast_calibrate); + + /* ************************************ */ + /* Calculate Local APIC Timer Frequency */ + /* ************************************ */ + lapic_timer_frequency = (fsb * 1000) / HZ; + + pr_debug("Setting lapic_timer_frequency = %d\n", + lapic_timer_frequency); + + /* mark tsc clocksource as reliable */ + set_cpu_cap(&boot_cpu_data, X86_FEATURE_TSC_RELIABLE); + + if (fast_calibrate) + return fast_calibrate; + + return 0; +} + +static void __init tangier_arch_setup(void) +{ + x86_platform.calibrate_tsc = tangier_calibrate_tsc; +} + +/* tangier arch ops */ +static struct intel_mid_ops tangier_ops = { + .arch_setup = tangier_arch_setup, +}; + +void * __cpuinit get_tangier_ops() +{ + return &tangier_ops; +} diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c index c84c1ca396bf..80a52288555c 100644 --- a/arch/x86/platform/intel-mid/sfi.c +++ b/arch/x86/platform/intel-mid/sfi.c @@ -443,13 +443,35 @@ static int __init sfi_parse_devs(struct sfi_table_header *table) * so we have to enable them one by one here */ ioapic = mp_find_ioapic(irq); - irq_attr.ioapic = ioapic; - irq_attr.ioapic_pin = irq; - irq_attr.trigger = 1; - irq_attr.polarity = 1; - io_apic_set_pci_routing(NULL, irq, &irq_attr); - } else + if (ioapic >= 0) { + irq_attr.ioapic = ioapic; + irq_attr.ioapic_pin = irq; + irq_attr.trigger = 1; + if (intel_mid_identify_cpu() == + INTEL_MID_CPU_CHIP_TANGIER) { + if (!strncmp(pentry->name, + "r69001-ts-i2c", 13)) + /* active low */ + irq_attr.polarity = 1; + else if (!strncmp(pentry->name, + "synaptics_3202", 14)) + /* active low */ + irq_attr.polarity = 1; + else if (irq == 41) + /* fast_int_1 */ + irq_attr.polarity = 1; + else + /* active high */ + irq_attr.polarity = 0; + } else { + /* PNW and CLV go with active low */ + irq_attr.polarity = 1; + } + io_apic_set_pci_routing(NULL, irq, &irq_attr); + } + } else { irq = 0; /* No irq */ + } dev = get_device_id(pentry->type, pentry->name);