| Message ID | 20200713194437.11325-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] x86/PCI: Get rid of custom x86 model comparison | expand |
On 7/13/20 12:44 PM, Andy Shevchenko wrote: > - switch (intel_mid_identify_cpu()) { > - case INTEL_MID_CPU_CHIP_TANGIER: > + id = x86_match_cpu(intel_mid_cpu_ids); > + if (id) > + model = id->model; > + > + switch (model) { > + case INTEL_FAM6_ATOM_SILVERMONT_MID: > polarity = IOAPIC_POL_HIGH; The diff makes it _look_ like there's a behavior change, swapping silvermont and tangier. It appears from intel_mid_arch_setup() that INTEL_FAM6_ATOM_SILVERMONT_MID and tangier are related: #define INTEL_FAM6_ATOM_SILVERMONT_MID 0x4A /* Merriefield */ ... case 0x3C: case 0x4A: __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_TANGIER; x86_platform.legacy.rtc = 1; break; But that's probably worth a note in the changelog. If you added a patch to intel_mid_arch_setup() to this series to use the intel-family.h #defines, this would be much more self explanatory. This also all makes me wonder if intel_mid_identify_cpu() is really even necessary. Does this fix any kinds of problems? It's a diffstat-challenged cleanup if not: arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) That's not usually how cleanups look. :)
On Mon, Jul 13, 2020 at 10:44:36PM +0300, Andy Shevchenko wrote: > Switch the platform code to use x86_id_table and accompanying API > instead of custom comparison against x86 CPU model. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c > index 00c62115f39c..d8af4787e616 100644 > --- a/arch/x86/pci/intel_mid_pci.c > +++ b/arch/x86/pci/intel_mid_pci.c > @@ -28,10 +28,12 @@ > #include <linux/io.h> > #include <linux/smp.h> > > +#include <asm/cpu_device_id.h> > #include <asm/segment.h> > #include <asm/pci_x86.h> > #include <asm/hw_irq.h> > #include <asm/io_apic.h> > +#include <asm/intel-family.h> > #include <asm/intel-mid.h> > > #define PCIE_CAP_OFFSET 0x100 > @@ -211,9 +213,16 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where, > where, size, value); > } > > +static const struct x86_cpu_id intel_mid_cpu_ids[] = { > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL), > + {} > +}; > + > static int intel_mid_pci_irq_enable(struct pci_dev *dev) > { > + const struct x86_cpu_id *id; > struct irq_alloc_info info; > + u16 model = 0; > int polarity; > int ret; > u8 gsi; > @@ -227,8 +236,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) > return ret; > } > > - switch (intel_mid_identify_cpu()) { > - case INTEL_MID_CPU_CHIP_TANGIER: > + id = x86_match_cpu(intel_mid_cpu_ids); > + if (id) > + model = id->model; > + > + switch (model) { > + case INTEL_FAM6_ATOM_SILVERMONT_MID: Is there a magic decoder ring somewhere that connects INTEL_MID_CPU_CHIP_TANGIER and INTEL_FAM6_ATOM_SILVERMONT_MID? I don't know how to verify that the new code is equivalent to the old. Or maybe the new code is *better* than the old, in which case the subject/commit log should mention that it's fixing or improving something. Also, there are a number of other places that check for "intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER": mrfld_pinctrl_init register_mrfld_power_btn mrfld_legacy_rtc_init mrfld_sd_init spidev_platform_data register_mid_wdt sfi_parse_devs atomisp_css_input_set_mode Maybe they should all be changed together? Or maybe this needs an explanation about why some places need intel_mid_identify_cpu() and others need x86_match_cpu()? > polarity = IOAPIC_POL_HIGH; > > /* Special treatment for IRQ0 */ > -- > 2.27.0 >
On Mon, Jul 13, 2020 at 01:59:55PM -0700, Dave Hansen wrote: > On 7/13/20 12:44 PM, Andy Shevchenko wrote: > > - switch (intel_mid_identify_cpu()) { > > - case INTEL_MID_CPU_CHIP_TANGIER: > > + id = x86_match_cpu(intel_mid_cpu_ids); > > + if (id) > > + model = id->model; > > + > > + switch (model) { > > + case INTEL_FAM6_ATOM_SILVERMONT_MID: > > polarity = IOAPIC_POL_HIGH; > > The diff makes it _look_ like there's a behavior change, swapping > silvermont and tangier. It appears from intel_mid_arch_setup() that > INTEL_FAM6_ATOM_SILVERMONT_MID and tangier are related: > > #define INTEL_FAM6_ATOM_SILVERMONT_MID 0x4A /* Merriefield */ > > ... > case 0x3C: > case 0x4A: > __intel_mid_cpu_chip = INTEL_MID_CPU_CHIP_TANGIER; > x86_platform.legacy.rtc = 1; > break; Yes. The original code is too old and was submitted without relying on existing kernel helpers. > But that's probably worth a note in the changelog. If you added a patch > to intel_mid_arch_setup() to this series to use the intel-family.h > #defines, this would be much more self explanatory. No, I'm not going to do that at all. The idea behind is to get rid of this ugly customisation. > This also all makes me wonder if intel_mid_identify_cpu() is really even > necessary. Exactly! > Does this fix any kinds of problems? It's a diffstat-challenged cleanup > if not: > arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > That's not usually how cleanups look. :) I understand. This will help to do a real clean up in the future.
On Mon, Jul 13, 2020 at 04:02:01PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 13, 2020 at 10:44:36PM +0300, Andy Shevchenko wrote: > > Switch the platform code to use x86_id_table and accompanying API > > instead of custom comparison against x86 CPU model. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c > > index 00c62115f39c..d8af4787e616 100644 > > --- a/arch/x86/pci/intel_mid_pci.c > > +++ b/arch/x86/pci/intel_mid_pci.c > > @@ -28,10 +28,12 @@ > > #include <linux/io.h> > > #include <linux/smp.h> > > > > +#include <asm/cpu_device_id.h> > > #include <asm/segment.h> > > #include <asm/pci_x86.h> > > #include <asm/hw_irq.h> > > #include <asm/io_apic.h> > > +#include <asm/intel-family.h> > > #include <asm/intel-mid.h> > > > > #define PCIE_CAP_OFFSET 0x100 > > @@ -211,9 +213,16 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where, > > where, size, value); > > } > > > > +static const struct x86_cpu_id intel_mid_cpu_ids[] = { > > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL), > > + {} > > +}; > > + > > static int intel_mid_pci_irq_enable(struct pci_dev *dev) > > { > > + const struct x86_cpu_id *id; > > struct irq_alloc_info info; > > + u16 model = 0; > > int polarity; > > int ret; > > u8 gsi; > > @@ -227,8 +236,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) > > return ret; > > } > > > > - switch (intel_mid_identify_cpu()) { > > - case INTEL_MID_CPU_CHIP_TANGIER: > > + id = x86_match_cpu(intel_mid_cpu_ids); > > + if (id) > > + model = id->model; > > + > > + switch (model) { > > + case INTEL_FAM6_ATOM_SILVERMONT_MID: > > Is there a magic decoder ring somewhere that connects > INTEL_MID_CPU_CHIP_TANGIER and INTEL_FAM6_ATOM_SILVERMONT_MID? Yes. And the idea is to get rid of it. > I don't know how to verify that the new code is equivalent to the old. > > Or maybe the new code is *better* than the old, in which case the > subject/commit log should mention that it's fixing or improving > something. > > Also, there are a number of other places that check for > "intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER": > > mrfld_pinctrl_init > register_mrfld_power_btn > mrfld_legacy_rtc_init > mrfld_sd_init > spidev_platform_data > register_mid_wdt > sfi_parse_devs > atomisp_css_input_set_mode This has been pending in Mauro's tree. > Maybe they should all be changed together? Or maybe this needs an > explanation about why some places need intel_mid_identify_cpu() and > others need x86_match_cpu()? No. The rest is subject to huge clean up (complete removal) in the future. I don't want to waste time on something which I will remove for sure.
On Tue, Jul 14, 2020 at 12:38:01PM +0300, Andy Shevchenko wrote: > On Mon, Jul 13, 2020 at 04:02:01PM -0500, Bjorn Helgaas wrote: > > On Mon, Jul 13, 2020 at 10:44:36PM +0300, Andy Shevchenko wrote: > > > Switch the platform code to use x86_id_table and accompanying API > > > instead of custom comparison against x86 CPU model. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- > > > 1 file changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c > > > index 00c62115f39c..d8af4787e616 100644 > > > --- a/arch/x86/pci/intel_mid_pci.c > > > +++ b/arch/x86/pci/intel_mid_pci.c > > > @@ -28,10 +28,12 @@ > > > #include <linux/io.h> > > > #include <linux/smp.h> > > > > > > +#include <asm/cpu_device_id.h> > > > #include <asm/segment.h> > > > #include <asm/pci_x86.h> > > > #include <asm/hw_irq.h> > > > #include <asm/io_apic.h> > > > +#include <asm/intel-family.h> > > > #include <asm/intel-mid.h> > > > > > > #define PCIE_CAP_OFFSET 0x100 > > > @@ -211,9 +213,16 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where, > > > where, size, value); > > > } > > > > > > +static const struct x86_cpu_id intel_mid_cpu_ids[] = { > > > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL), > > > + {} > > > +}; > > > + > > > static int intel_mid_pci_irq_enable(struct pci_dev *dev) > > > { > > > + const struct x86_cpu_id *id; > > > struct irq_alloc_info info; > > > + u16 model = 0; > > > int polarity; > > > int ret; > > > u8 gsi; > > > @@ -227,8 +236,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) > > > return ret; > > > } > > > > > > - switch (intel_mid_identify_cpu()) { > > > - case INTEL_MID_CPU_CHIP_TANGIER: > > > + id = x86_match_cpu(intel_mid_cpu_ids); > > > + if (id) > > > + model = id->model; > > > + > > > + switch (model) { > > > + case INTEL_FAM6_ATOM_SILVERMONT_MID: > > > > Is there a magic decoder ring somewhere that connects > > INTEL_MID_CPU_CHIP_TANGIER and INTEL_FAM6_ATOM_SILVERMONT_MID? > > Yes. And the idea is to get rid of it. OK. You don't want to even include a mention of it in the commit log to help people connect the dots and verify that this change is correct? > > I don't know how to verify that the new code is equivalent to the old. > > > > Or maybe the new code is *better* than the old, in which case the > > subject/commit log should mention that it's fixing or improving > > something. > > > > Also, there are a number of other places that check for > > "intel_mid_identify_cpu() == INTEL_MID_CPU_CHIP_TANGIER": > > > > mrfld_pinctrl_init > > register_mrfld_power_btn > > mrfld_legacy_rtc_init > > mrfld_sd_init > > spidev_platform_data > > register_mid_wdt > > sfi_parse_devs > > > atomisp_css_input_set_mode > > This has been pending in Mauro's tree. > > > Maybe they should all be changed together? Or maybe this needs an > > explanation about why some places need intel_mid_identify_cpu() and > > others need x86_match_cpu()? > > No. The rest is subject to huge clean up (complete removal) in the future. > I don't want to waste time on something which I will remove for sure. OK, I'll leave this to the x86 guys. If I were merging it I would want a little more explanation, but this isn't PCI-related at all and I'm sure they understand this better than I do. Bjorn
On Tue, Jul 14, 2020 at 02:02:41PM -0500, Bjorn Helgaas wrote: > On Tue, Jul 14, 2020 at 12:38:01PM +0300, Andy Shevchenko wrote: > > On Mon, Jul 13, 2020 at 04:02:01PM -0500, Bjorn Helgaas wrote: > > > On Mon, Jul 13, 2020 at 10:44:36PM +0300, Andy Shevchenko wrote: > > > > Switch the platform code to use x86_id_table and accompanying API > > > > instead of custom comparison against x86 CPU model. ... > > > > - case INTEL_MID_CPU_CHIP_TANGIER: > > > > + id = x86_match_cpu(intel_mid_cpu_ids); > > > > + if (id) > > > > + model = id->model; > > > > + > > > > + switch (model) { > > > > + case INTEL_FAM6_ATOM_SILVERMONT_MID: > > > > > > Is there a magic decoder ring somewhere that connects > > > INTEL_MID_CPU_CHIP_TANGIER and INTEL_FAM6_ATOM_SILVERMONT_MID? > > > > Yes. And the idea is to get rid of it. > > OK. You don't want to even include a mention of it in the commit log > to help people connect the dots and verify that this change is > correct? I think I missed it. I will update changelog for v2.
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c index 00c62115f39c..d8af4787e616 100644 --- a/arch/x86/pci/intel_mid_pci.c +++ b/arch/x86/pci/intel_mid_pci.c @@ -28,10 +28,12 @@ #include <linux/io.h> #include <linux/smp.h> +#include <asm/cpu_device_id.h> #include <asm/segment.h> #include <asm/pci_x86.h> #include <asm/hw_irq.h> #include <asm/io_apic.h> +#include <asm/intel-family.h> #include <asm/intel-mid.h> #define PCIE_CAP_OFFSET 0x100 @@ -211,9 +213,16 @@ static int pci_write(struct pci_bus *bus, unsigned int devfn, int where, where, size, value); } +static const struct x86_cpu_id intel_mid_cpu_ids[] = { + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL), + {} +}; + static int intel_mid_pci_irq_enable(struct pci_dev *dev) { + const struct x86_cpu_id *id; struct irq_alloc_info info; + u16 model = 0; int polarity; int ret; u8 gsi; @@ -227,8 +236,12 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev) return ret; } - switch (intel_mid_identify_cpu()) { - case INTEL_MID_CPU_CHIP_TANGIER: + id = x86_match_cpu(intel_mid_cpu_ids); + if (id) + model = id->model; + + switch (model) { + case INTEL_FAM6_ATOM_SILVERMONT_MID: polarity = IOAPIC_POL_HIGH; /* Special treatment for IRQ0 */
Switch the platform code to use x86_id_table and accompanying API instead of custom comparison against x86 CPU model. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/x86/pci/intel_mid_pci.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)