[PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Andy Shevchenko]

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:
 		polarity = IOAPIC_POL_HIGH;
 
 		/* Special treatment for IRQ0 */
-- 
2.27.0

[PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()

[Andy Shevchenko]

Describe missed parameter in documentation of type1_access_ok().
Otherwise we get the following warning:

  CHECK   arch/x86/pci/intel_mid_pci.c
  CC      arch/x86/pci/intel_mid_pci.o
  arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok'

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/pci/intel_mid_pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index d8af4787e616..f855e12d7bed 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -141,6 +141,7 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
  * type1_access_ok - check whether to use type 1
  * @bus: bus number
  * @devfn: device & function in question
+ * @reg: configuration register offset
  *
  * If the bus is on a Lincroft chip and it exists, or is not on a Lincroft at
  * all, the we can go ahead with any reads & writes.  If it's on a Lincroft,
-- 
2.27.0

Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()

[Bjorn Helgaas]

On Mon, Jul 13, 2020 at 10:44:37PM +0300, Andy Shevchenko wrote:
> Describe missed parameter in documentation of type1_access_ok().
> Otherwise we get the following warning:

Would you mind including the "make" invocation that runs this
checking?  I assume it's something like "make C=2
arch/x86/pci/intel_mid_pci.o"?

I'm not in the habit of running these checks, but maybe seeing how
easy it is will help me and others get in the habit.

>   CHECK   arch/x86/pci/intel_mid_pci.c
>   CC      arch/x86/pci/intel_mid_pci.o
>   arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok'
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/pci/intel_mid_pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
> index d8af4787e616..f855e12d7bed 100644
> --- a/arch/x86/pci/intel_mid_pci.c
> +++ b/arch/x86/pci/intel_mid_pci.c
> @@ -141,6 +141,7 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
>   * type1_access_ok - check whether to use type 1
>   * @bus: bus number
>   * @devfn: device & function in question
> + * @reg: configuration register offset
>   *
>   * If the bus is on a Lincroft chip and it exists, or is not on a Lincroft at
>   * all, the we can go ahead with any reads & writes.  If it's on a Lincroft,
> -- 
> 2.27.0
> 

Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()

[Andy Shevchenko]

On Mon, Jul 13, 2020 at 02:59:07PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 13, 2020 at 10:44:37PM +0300, Andy Shevchenko wrote:
> > Describe missed parameter in documentation of type1_access_ok().
> > Otherwise we get the following warning:
> 
> Would you mind including the "make" invocation that runs this
> checking?  I assume it's something like "make C=2
> arch/x86/pci/intel_mid_pci.o"?

No, it is not sparse, it's a kernel doc validation.
I guess `make W=1` does it, but I can repeat my command line publicly again :-)
	make W=1 C=1 CF=-D__CHECK_ENDIAN__ -j64

> I'm not in the habit of running these checks, but maybe seeing how
> easy it is will help me and others get in the habit.
> 
> >   CHECK   arch/x86/pci/intel_mid_pci.c
> >   CC      arch/x86/pci/intel_mid_pci.o
> >   arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok'
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  arch/x86/pci/intel_mid_pci.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
> > index d8af4787e616..f855e12d7bed 100644
> > --- a/arch/x86/pci/intel_mid_pci.c
> > +++ b/arch/x86/pci/intel_mid_pci.c
> > @@ -141,6 +141,7 @@ static int pci_device_update_fixed(struct pci_bus *bus, unsigned int devfn,
> >   * type1_access_ok - check whether to use type 1
> >   * @bus: bus number
> >   * @devfn: device & function in question
> > + * @reg: configuration register offset
> >   *
> >   * If the bus is on a Lincroft chip and it exists, or is not on a Lincroft at
> >   * all, the we can go ahead with any reads & writes.  If it's on a Lincroft,
> > -- 
> > 2.27.0
> > 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()

[Bjorn Helgaas]

On Mon, Jul 13, 2020 at 11:03:38PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 13, 2020 at 02:59:07PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 13, 2020 at 10:44:37PM +0300, Andy Shevchenko wrote:
> > > Describe missed parameter in documentation of type1_access_ok().
> > > Otherwise we get the following warning:
> > 
> > Would you mind including the "make" invocation that runs this
> > checking?  I assume it's something like "make C=2
> > arch/x86/pci/intel_mid_pci.o"?
> 
> No, it is not sparse, it's a kernel doc validation.
> I guess `make W=1` does it, but I can repeat my command line publicly again :-)
> 	make W=1 C=1 CF=-D__CHECK_ENDIAN__ -j64

Thanks.  "make W=1 arch/x86/pci/" is enough.  I would just put this in
the commit log, e.g.,

  Otherwise "make W=1 arch/x86/pci/" produces the following warning:

    CHECK   arch/x86/pci/intel_mid_pci.c
    CC      arch/x86/pci/intel_mid_pci.o
    arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok'

Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()

[Andy Shevchenko]

On Mon, Jul 13, 2020 at 04:02:40PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 13, 2020 at 11:03:38PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 13, 2020 at 02:59:07PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jul 13, 2020 at 10:44:37PM +0300, Andy Shevchenko wrote:
> > > > Describe missed parameter in documentation of type1_access_ok().
> > > > Otherwise we get the following warning:
> > > 
> > > Would you mind including the "make" invocation that runs this
> > > checking?  I assume it's something like "make C=2
> > > arch/x86/pci/intel_mid_pci.o"?
> > 
> > No, it is not sparse, it's a kernel doc validation.
> > I guess `make W=1` does it, but I can repeat my command line publicly again :-)
> > 	make W=1 C=1 CF=-D__CHECK_ENDIAN__ -j64
> 
> Thanks.  "make W=1 arch/x86/pci/" is enough.  I would just put this in
> the commit log, e.g.,
> 
>   Otherwise "make W=1 arch/x86/pci/" produces the following warning:
> 
>     CHECK   arch/x86/pci/intel_mid_pci.c
>     CC      arch/x86/pci/intel_mid_pci.o
>     arch/x86/pci/intel_mid_pci.c:152: warning: Function parameter or member 'reg' not described in 'type1_access_ok'

Will do.

-- 
With Best Regards,
Andy Shevchenko

Re: [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Dave Hansen]

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. :)

Re: [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Bjorn Helgaas]

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
> 

Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Bjorn Helgaas]

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

Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko