Message-ID: <CAA93t1p915ofjgzLeW3n+tsKrDWU0wDjf7QXu1fwTao0U-bqLQ@mail.gmail.com>
Date:   Thu, 28 Mar 2019 22:29:55 -0700
From:   Rajat Jain <rajatxjain@...il.com>
To:     Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     Rajat Jain <rajatja@...gle.com>,
        "Bhardwaj, Rajneesh" <rajneesh.bhardwaj@...ux.intel.com>,
        Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        Vishwanath Somayaji <vishwanath.somayaji@...el.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Furquan Shaikh <furquan@...gle.com>,
        Evan Green <evgreen@...gle.com>,
        "Box, David E" <david.e.box@...el.com>
Subject: Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver

Hi Srinivas,


On Thu, Mar 28, 2019 at 8:41 PM Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> wrote:
>
> On Mon, 2019-03-25 at 18:41 -0700, Rajat Jain wrote:
> > Hi Rajneesh,
> >
> >
> > On Mon, Mar 25, 2019 at 3:23 AM Bhardwaj, Rajneesh
> > <rajneesh.bhardwaj@...ux.intel.com> wrote:
> > >
> > > Hi Rajat
> > >
> > > On 23-Mar-19 6:00 AM, Rajat Jain wrote:
> > > > Hi Rajneesh,
> > > >
> > > >
> > > >
> > > > On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh
> > > > <rajneesh.bhardwaj@...ux.intel.com> wrote:
> > > > > Some suggestions below
> > > > >
> > > > > On 18-Mar-19 8:36 PM, Rajat Jain wrote:
> > > > >
> > > > > On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj
> > > > > <rajneesh.bhardwaj@...el.com> wrote:
> > > > >
> > > > > On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote:
> > > > >
> > > > > Convert the intel_pmc_core driver to a platform driver. There
> > > > > is no
> > > > > functional change. Some code that tries to determine what kind
> > > > > of
> > > > > CPU this is, has been moved code is moved from pmc_core_probe()
> > > > > to
> > > > >
> > > > > Possible typo here.
> > > > >
> > > > > Ummm, you mean grammar error I guess? Sure, I will rephrase.
> > > > >
> > > > > pmc_core_init().
> > > > >
> > > > > Signed-off-by: Rajat Jain <rajatja@...gle.com>
> > > > >
> > > > > Thanks for sending this. This is certainly useful to support
> > > > > suspend-resume
> > > > > functionality for this driver which is otherwise only possible
> > > > > with PM
> > > > > notifiers otherwise and that is not desirable. Initially this
> > > > > was a PCI
> > > > > driver and after design discussion it was converted to module.
> > > > > I would like
> > > > > to consult Andy and Srinivas for their opinion about binding it
> > > > > to actual
> > > > > platform bus instead of the virtual bus as in its current form.
> > > > > In one of the
> > > > > internal versions, we used a known acpi PNP HID.
> > > > >
> > > > > Sure, if there is an established ACPI PNP HID, then we could
> > > > > bind it
> > > > > using that, on platforms where we are still developing BIOS /
> > > > > coreboot. However, this might not be possible for shipping
> > > > > systems
> > > > > (Kabylake / skylake) where there is no plan to change the BIOS.
> > > > >
> > > > > In one of our internal patches, i had used HID of power engine
> > > > > plugin. IIRC, During my testing it was working on KBL, CNL with
> > > > > UEFI BIOS but i highly recommend testing it.
> > > > >
> > > > > ---8<----8<-----
> > > > >
> > > > > +static const struct acpi_device_id pmc_acpi_ids[] = {
> > > > >
> > > > > +             {"INT33A1", 0}, /* _HID for Intel Power Engine,
> > > > > _CID PNP0D80*/
> > > > >
> > > > > +             { }
> > > > >
> > > > >   };
> > > >
> > > > We do not have this device in any of our ACPI tables today. If
> > > > Intel
> > > > can confirm that this is a well known HID to be used for
> > > > attaching
> > > > this driver, we can start putting it on our platform's ACPI going
> > > > forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I
> > > > believe we also need to have this driver attach with the device
> > > > on
> > > > older platforms (Skylake, Kabylake, Amberlake) that are already
> > > > shipping, and running a Non UEFI BIOS (that may not have this HID
> > > > since it is not published).
> > > >
> > > > Currently the intel_pmc_core driver attaches itself to the
> > > > following
> > > > table of CPU families, without regard to whether it has that HID
> > > > in
> > > > the ACPI or not:
> > > >
> > > > static const struct x86_cpu_id intel_pmc_core_ids[] = {
> > > >          INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map),
> > > >          INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map),
> > > >          INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
> > > >          INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map),
> > > >          INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map),
> > > >          INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map),
> > > >          {}
> > > > };
> > >
> > > In the past i tried one hybrid approach i.e. PCI and Platform
> > > driver at
> > > the same time. Based on that, i feel that this idea of spilling
> > > probe
> > > like this may not be the best option. The ACPI CID that i suggested
> > > is
> > > available on most Intel Core Platforms that i have worked on and i
> > > can
> > > help you in verifying it with UEFI BIOS if you want. Meanwhile,
> > > please
> > > see this https://patchwork.kernel.org/patch/9806565/ it gives some
> > > background about this ACPI ID and also points to the LPIT spec.
> > >
> > > >
> > > > So to avoid a regression, I suggest that we still maintain the
> > > > above
> > > > table (may be eliminate few entries) and always attach if the CPU
> > > > is
> > > > among the table, and if the CPU is not among the table, use the
> > > > ACPI
> > > > HID to attach. I propose to attach to at least Skylake and
> > > > Kabylake
> > > > systems using the table above, and for Canonlake and Icelake and
> > > > newer, we can rely on BIOS providing the ACPI HID. Of course I do
> > > > not
> > > > know if all non-Google Canonlake/Icelake platforms will have this
> > > > HID
> > > > in their BIOS. If we are not sure, we can include Canonlake and
> > > > Icelake also in that list, an. Please let me know what do you
> > > > think.
> > >
> > > If Coreboot firmware can not be updated for the shipping devices,
> > > then
> > > can Chromium kernel take the suggested deviation which i think gets
> > > updated OTA periodically? For upstream,  I am waiting to hear from
> > > Rafael, Andi, David and Srinivas for their inputs.
> >
> > 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. 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. 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.

Thanks,

Rajat


>
>
> Thanks,
> Srinivas
>
>
> >
> > Let me know.
> >
> > Thanks,
> >
> > Rajat
> >
> > >
> > > >
> > > > Thanks,
> > > >
> > > > Rajat
> > > >
> > > > >
> > > > >
> > > > > -builtin_pci_driver(intel_pmc_core_driver);
> > > > >
> > > > > +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),
> > > > >
> > > > > +             },
> > > > >
> > > > > +};
> > > > >
> > > > > ---
> > > > > This is rebased off
> > > > > git://git.infradead.org/linux-platform-drivers-x86.git/for-next
> > > > >
> > > > >   drivers/platform/x86/intel_pmc_core.c | 93
> > > > > ++++++++++++++++++++-------
> > > > >   1 file changed, 68 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > > > b/drivers/platform/x86/intel_pmc_core.c
> > > > > index f2c621b55f49..55578d07610c 100644
> > > > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > > > @@ -19,6 +19,7 @@
> > > > >   #include <linux/io.h>
> > > > >   #include <linux/module.h>
> > > > >   #include <linux/pci.h>
> > > > > +#include <linux/platform_device.h>
> > > > >   #include <linux/uaccess.h>
> > > > >
> > > > >   #include <asm/cpu_device_id.h>
> > > > > @@ -854,12 +855,59 @@ static const struct dmi_system_id
> > > > > pmc_core_dmi_table[]  = {
> > > > >        {}
> > > > >   };
> > > > >
> > > > > -static int __init pmc_core_probe(void)
> > > > > +static int pmc_core_probe(struct platform_device *pdev)
> > > > >   {
> > > > > -     struct pmc_dev *pmcdev = &pmc;
> > > > > +     struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > > +     int err;
> > > > > +
> > > > > +     pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > > +                               pmcdev->map->regmap_length);
> > > > > +     if (!pmcdev->regbase)
> > > > > +             return -ENOMEM;
> > > > > +
> > > > > +     mutex_init(&pmcdev->lock);
> > > > > +     pmcdev->pmc_xram_read_bit =
> > > > > pmc_core_check_read_lock_bit();
> > > > > +
> > > > > +     err = pmc_core_dbgfs_register(pmcdev);
> > > > > +     if (err < 0) {
> > > > > +             dev_warn(&pdev->dev, "debugfs register
> > > > > failed.\n");
> > > > > +             iounmap(pmcdev->regbase);
> > > > > +             return err;
> > > > > +     }
> > > > > +
> > > > > +     dmi_check_system(pmc_core_dmi_table);
> > > > > +     dev_info(&pdev->dev, " initialized\n");
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int pmc_core_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> > > > > +
> > > > > +     pmc_core_dbgfs_unregister(pmcdev);
> > > > > +     mutex_destroy(&pmcdev->lock);
> > > > > +     iounmap(pmcdev->regbase);
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static struct platform_driver pmc_core_driver = {
> > > > > +     .driver = {
> > > > > +             .name = "pmc_core",
> > > > > +     },
> > > > > +     .probe = pmc_core_probe,
> > > > > +     .remove = pmc_core_remove,
> > > > > +};
> > > > > +
> > > > > +static struct platform_device pmc_core_device = {
> > > > > +     .name           = "pmc_core",
> > > > > +};
> > > > > +
> > > > > +static int __init pmc_core_init(void)
> > > > > +{
> > > > > +     int ret;
> > > > >
> > > > > Please use reverse x-mas tree style.
> > > > >
> > > > > OK, will do.
> > > > >
> > > > >        const struct x86_cpu_id *cpu_id;
> > > > > +     struct pmc_dev *pmcdev = &pmc;
> > > > >        u64 slp_s0_addr;
> > > > > -     int err;
> > > > >
> > > > >        cpu_id = x86_match_cpu(intel_pmc_core_ids);
> > > > >        if (!cpu_id)
> > > > > @@ -880,36 +928,31 @@ static int __init pmc_core_probe(void)
> > > > >        else
> > > > >                pmcdev->base_addr = slp_s0_addr - pmcdev->map-
> > > > > >slp_s0_offset;
> > > > >
> > > > > -     pmcdev->regbase = ioremap(pmcdev->base_addr,
> > > > > -                               pmcdev->map->regmap_length);
> > > > > -     if (!pmcdev->regbase)
> > > > > -             return -ENOMEM;
> > > > > +     platform_set_drvdata(&pmc_core_device, pmcdev);
> > > > >
> > > > > -     mutex_init(&pmcdev->lock);
> > > > > -     pmcdev->pmc_xram_read_bit =
> > > > > pmc_core_check_read_lock_bit();
> > > > > +     ret = platform_device_register(&pmc_core_device);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > >
> > > > > -     err = pmc_core_dbgfs_register(pmcdev);
> > > > > -     if (err < 0) {
> > > > > -             pr_warn(" debugfs register failed.\n");
> > > > > -             iounmap(pmcdev->regbase);
> > > > > -             return err;
> > > > > -     }
> > > > > +     ret = platform_driver_register(&pmc_core_driver);
> > > > > +     if (ret)
> > > > > +             goto out_remove_dev;
> > > > >
> > > > > -     dmi_check_system(pmc_core_dmi_table);
> > > > > -     pr_info(" initialized\n");
> > > > >        return 0;
> > > > > +
> > > > > +out_remove_dev:
> > > > > +     platform_device_unregister(&pmc_core_device);
> > > > > +     return ret;
> > > > >   }
> > > > > -module_init(pmc_core_probe)
> > > > >
> > > > > -static void __exit pmc_core_remove(void)
> > > > > +static void __init pmc_core_exit(void)
> > > > >   {
> > > > > -     struct pmc_dev *pmcdev = &pmc;
> > > > > -
> > > > > -     pmc_core_dbgfs_unregister(pmcdev);
> > > > > -     mutex_destroy(&pmcdev->lock);
> > > > > -     iounmap(pmcdev->regbase);
> > > > > +     platform_driver_unregister(&pmc_core_driver);
> > > > > +     platform_device_unregister(&pmc_core_device);
> > > > >   }
> > > > > -module_exit(pmc_core_remove)
> > > > > +
> > > > > +module_init(pmc_core_init);
> > > > > +module_exit(pmc_core_exit);
> > > > >
> > > > >   MODULE_LICENSE("GPL v2");
> > > > >   MODULE_DESCRIPTION("Intel PMC Core Driver");
> > > > > --
> > > > > 2.21.0.360.g471c308f928-goog
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > > Rajneesh
>

Powered by blists - more mailing lists