[v1,1/2] x86/PCI: Get rid of custom x86 model comparison
diff mbox series

Message ID 20200713194437.11325-1-andriy.shevchenko@linux.intel.com
State Not Applicable
Headers show
Series
  • [v1,1/2] x86/PCI: Get rid of custom x86 model comparison
Related show

Commit Message

Andy Shevchenko July 13, 2020, 7:44 p.m. UTC
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(-)

Comments

Dave Hansen July 13, 2020, 8:59 p.m. UTC | #1
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. :)
Bjorn Helgaas July 13, 2020, 9:02 p.m. UTC | #2
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
>
Andy Shevchenko July 14, 2020, 9:35 a.m. UTC | #3
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.
Andy Shevchenko July 14, 2020, 9:38 a.m. UTC | #4
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.
Bjorn Helgaas July 14, 2020, 7:02 p.m. UTC | #5
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
Andy Shevchenko July 14, 2020, 7:29 p.m. UTC | #6
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.

Patch
diff mbox series

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 */