Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

[Rajat Jain]

On Fri, Mar 29, 2019 at 8:53 AM Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, 2019-03-28 at 22:29 -0700, Rajat Jain wrote:
> > Hi Srinivas,
> >
> >
>
> [...]
>
> > So if everyone here thinks we should completely switch to using
> > > > the
> > > > ACPI HID "INT33A1" for attaching to the device, sure, we can do
> > > > that.
> > > > Yes, for Chromeos, we can put in a work around internally that
> > > > ensures
> > > > that shipping chromebooks (Kabylake etc) can work fine without
> > > > that
> > > > ACPI ID. What I do not know is if that will cause any regressions
> > > > outside of Chromeos. Can you discuss with Rafael, Andy, Srinivas
> > > > internally and let me know on how they'd like to proceed on this.
> > > >
> > > > The other option is to apply this patch as-is so we know that
> > > > there
> > > > is
> > > > no "functional change" and thus no possible regression (so the
> > > > driver
> > > > continues to attach to those and only those systems that s it
> > > > used
> > > > to,
> > > > before this patch). And then introduce the ACPI ID Change as a
> > > > new
> > > > independent patch.
> > >
> > > Use INT33A1 to enumerate, if this id doesn't exist then fallback to
> > > the
> > > cpuid style.

OK, I got to down to implement this and have a few questions.

The intel_pmc_driver does need some way to determine whether to choose
between spt_reg_map / cnp_reg_map / icl_reg_map. How should it choose
between these?

 1) For ACPI based enumeration, I don't think the ACPI nodes on the
current systems will have anything to offer to help the driver
differentiate between these. So it will still need to choose one of
the above maps based on the CPU model. I assume that is OK. I expect
something like:

        switch (boot_cpu_data.x86_model) {
        case INTEL_FAM6_SKYLAKE_MOBILE:
        case INTEL_FAM6_SKYLAKE_DESKTOP:
        case INTEL_FAM6_KABYLAKE_MOBILE:
        case INTEL_FAM6_KABYLAKE_DESKTOP:
                pmcdev->map = &spt_reg_map;
                break;

        case INTEL_FAM6_CANONLAKE_DESKTOP:
                pmcdev->map = &cnp_reg_map;
                break;

        case INTEL_FAM6_ICELAKE_DESKTOP:
                pmcdev->map = &icl_reg_map;
                break;
        default:
                /* Which map should we use by default? */
        }


 2) For the ACPI based enumeration, what should be the default map
chosen by the driver, for CPUs other than what is in that list today
(Or do we expect to keep adding to this switch/case for new CPU
models)?

3) For the fallback option, i.e. manually creating platform devices
using a new file, yes, we can pass a value in platform_data to
indicate which map to choose from, which is what I think should be
done. However, I am wondering if all this fallback might be an
overkill / over engineering. Do we know of any platforms that do not
have this ACPI device, and still are actually using this
intel_pmc_core driver? If not, may be we don't need this fallback
option?

Thanks,

Rajat

> > > The aim should be that we don't have to add any more
> > > CPU
> > > model to enumerate. But most probably register map will differ so
> > > we
> > > still may end up adding some CPU model relationship.
> >
> > Thanks for the guidance. Just to reconfirm my understanding of your
> > suggestion:
> >
> > Here is the suggestive code Rajneesh provided, and I intend to do it
> > similarly:
> >
> > +static const struct acpi_device_id pmc_acpi_ids[] = {
> > +             {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID
> > PNP0D80*/
> > +             { }
> > +};
> >
> > +static struct platform_driver pmc_plat_driver = {
> > +             .remove = pmc_plat_remove,
> > +             .probe = pmc_plat_probe,
> > +             .driver = {
> > +                             .name = "pmc_core_driver",
> > +                             .acpi_match_table =
> > ACPI_PTR(pmc_acpi_ids),
> > +             },
> > +};
> >
> > My understanding is that with this, the kernel would use
> > acpi_match_table to automatically create the platform_device on a
> > platform where ACPI tables contain the INT33A1 HID, and thus we don't
> > need to create the platform_device in module_init time on such
> > platforms.
> Yes. There will be /sys/bus/platform/devices/INT33A1:00.
>
>
> >  So are you saying that during init, I should check if the
> > ACPI HID INT33A1 is not present on the platform, then use the cpu_id
> > table to create the platform_device? Thus newer platforms will not
> > need an entry in the table.
> Yes. Preferably in a different file as Andy would like. So the the
> current driver only implements platform driver for INT33A1. The other driver which will enumerate on CPUID and create INT33A1 platform device  if there is no ACPI match via acpi_match_device() or similar API, for INT33A1. When you create a platform device the pmc driver will be probed.
>
> Thanks,
> Srinivas
>
>