* [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
@ 2020-07-13 19:44 Andy Shevchenko
2020-07-13 19:44 ` [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok() Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-13 19:44 UTC (permalink / raw)
To: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()
2020-07-13 19:44 [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Andy Shevchenko
@ 2020-07-13 19:44 ` Andy Shevchenko
2020-07-13 19:59 ` Bjorn Helgaas
2020-07-13 20:59 ` [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Dave Hansen
2020-07-13 21:02 ` Bjorn Helgaas
2 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-13 19:44 UTC (permalink / raw)
To: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()
2020-07-13 19:44 ` [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok() Andy Shevchenko
@ 2020-07-13 19:59 ` Bjorn Helgaas
2020-07-13 20:03 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2020-07-13 19:59 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()
2020-07-13 19:59 ` Bjorn Helgaas
@ 2020-07-13 20:03 ` Andy Shevchenko
2020-07-13 21:02 ` Bjorn Helgaas
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-13 20:03 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-13 19:44 [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Andy Shevchenko
2020-07-13 19:44 ` [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok() Andy Shevchenko
@ 2020-07-13 20:59 ` Dave Hansen
2020-07-14 9:35 ` Andy Shevchenko
2020-07-13 21:02 ` Bjorn Helgaas
2 siblings, 1 reply; 12+ messages in thread
From: Dave Hansen @ 2020-07-13 20:59 UTC (permalink / raw)
To: Andy Shevchenko, Bjorn Helgaas, x86, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, linux-pci
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. :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-13 19:44 [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Andy Shevchenko
2020-07-13 19:44 ` [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok() Andy Shevchenko
2020-07-13 20:59 ` [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Dave Hansen
@ 2020-07-13 21:02 ` Bjorn Helgaas
2020-07-14 9:38 ` Andy Shevchenko
2 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2020-07-13 21:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()
2020-07-13 20:03 ` Andy Shevchenko
@ 2020-07-13 21:02 ` Bjorn Helgaas
2020-07-14 9:09 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2020-07-13 21:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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'
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok()
2020-07-13 21:02 ` Bjorn Helgaas
@ 2020-07-14 9:09 ` Andy Shevchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-14 9:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-13 20:59 ` [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Dave Hansen
@ 2020-07-14 9:35 ` Andy Shevchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-14 9:35 UTC (permalink / raw)
To: Dave Hansen
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-13 21:02 ` Bjorn Helgaas
@ 2020-07-14 9:38 ` Andy Shevchenko
2020-07-14 19:02 ` Bjorn Helgaas
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-14 9:38 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-14 9:38 ` Andy Shevchenko
@ 2020-07-14 19:02 ` Bjorn Helgaas
2020-07-14 19:29 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2020-07-14 19:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison
2020-07-14 19:02 ` Bjorn Helgaas
@ 2020-07-14 19:29 ` Andy Shevchenko
0 siblings, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2020-07-14 19:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, x86, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, linux-pci
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-14 19:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 19:44 [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Andy Shevchenko
2020-07-13 19:44 ` [PATCH v1 2/2] x86/PCI: Describe @reg for type1_access_ok() Andy Shevchenko
2020-07-13 19:59 ` Bjorn Helgaas
2020-07-13 20:03 ` Andy Shevchenko
2020-07-13 21:02 ` Bjorn Helgaas
2020-07-14 9:09 ` Andy Shevchenko
2020-07-13 20:59 ` [NEEDS-REVIEW] [PATCH v1 1/2] x86/PCI: Get rid of custom x86 model comparison Dave Hansen
2020-07-14 9:35 ` Andy Shevchenko
2020-07-13 21:02 ` Bjorn Helgaas
2020-07-14 9:38 ` Andy Shevchenko
2020-07-14 19:02 ` Bjorn Helgaas
2020-07-14 19:29 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).