| Message ID | 20190313222124.229371-1-rajatja@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
| Series | [1/2] platform/x86: intel_pmc_core: Convert to a platform_driver | expand |
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. > pmc_core_init(). > > Signed-off-by: Rajat Jain <rajatja@google.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. > --- > 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. > 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 >
On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.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@google.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. > > > --- > > 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
Hi Rajneesh, On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh <rajneesh.bhardwaj@linux.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@intel.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@google.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), {} }; 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. 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
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@linux.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@intel.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@google.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. > > 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
Hi Rajneesh, On Mon, Mar 25, 2019 at 3:23 AM Bhardwaj, Rajneesh <rajneesh.bhardwaj@linux.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@linux.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@intel.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@google.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 it used to, before this patch). And then introduce the ACPI ID Change as a new independent patch. 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
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@linux.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@linux.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@intel.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@google.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, 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
Hi Srinivas, On Thu, Mar 28, 2019 at 8:41 PM Srinivas Pandruvada <srinivas.pandruvada@linux.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@linux.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@linux.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@intel.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@google.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 >
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. 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
On Fri, Mar 29, 2019 at 8:53 AM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 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 > >
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; 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");
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 pmc_core_init(). Signed-off-by: Rajat Jain <rajatja@google.com> --- 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(-)