From: "David E. Box" <[email protected]>
Current Intel SOC cores use a MailBox Interface (MBI) to provide access to unit
devices connected to the system fabric. This driver implements access to this
interface on BayTrail platforms. This is a requirement for drivers that need
access to unit registers on the platform (e.g. accessing the PUNIT for power
management features such as RAPL). Serialized access is handled by all exported
routines with spinlocks.
The API includes 3 functions for access to unit registers:
int bt_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
int bt_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr)
int bt_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
port: indicating the unit being accessed
opcode: the read or write port specific opcode
offset: the register offset within the port
mdr: the register data to be read, written, or modified
mask: bit locations in mdr to change
Returns nonzero on error
Note: GPU code handles access to the GFX unit. Therefore access to that unit
with this driver is disallowed to avoid conflicts.
Signed-off-by: David E. Box <[email protected]>
---
Applies to Linux 3.12
v5: Converted from pnpacpi/mmio driver to pci/config_space driver as
necessitated by firmware change.
v4: Define driver as platform specific to BayTrail as some platforms cannot
enumerate the MBI using ACPI as noted by Bin Gao <[email protected]>
Renamed register macros and API funcitons to platform specific names.
Changed dependency to PNPACPI as sugessted by Rafael Wysocki <[email protected]>
v3: Converted to PNP ACPI driver as sugessted by Rafael Wysocki <[email protected]>
Removed config visibility to user as suggested by Andi Kleen <[email protected]>
v2: Made modular since there was no longer a reason not to
Moved to x86 platform as suggested by Mathhew Garrett <[email protected]>
Added probe to init to cause driver load to fail if device not detected.
drivers/platform/x86/Kconfig | 10 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel_baytrail.c | 225 +++++++++++++++++++++++++++++++++
drivers/platform/x86/intel_baytrail.h | 90 +++++++++++++
4 files changed, 326 insertions(+)
create mode 100644 drivers/platform/x86/intel_baytrail.c
create mode 100644 drivers/platform/x86/intel_baytrail.h
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b51a746..6e199a5 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -819,4 +819,14 @@ config PVPANIC
a paravirtualized device provided by QEMU; it lets a virtual machine
(guest) communicate panic events to the host.
+config INTEL_BAYTRAIL_MBI
+ tristate
+ depends on PCI
+ ---help---
+ Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
+ Interface. This is a requirement for systems that need to configure
+ the PUNIT for power management features such as RAPL. Register
+ addresses and r/w opcodes are defined in
+ drivers/platform/x86/intel_baytrail.c.
+
endif # X86_PLATFORM_DEVICES
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5dbe193..b3d4cfd 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -55,3 +55,4 @@ obj-$(CONFIG_INTEL_RST) += intel-rst.o
obj-$(CONFIG_INTEL_SMARTCONNECT) += intel-smartconnect.o
obj-$(CONFIG_PVPANIC) += pvpanic.o
+obj-$(CONFIG_INTEL_BAYTRAIL_MBI) += intel_baytrail.o
diff --git a/drivers/platform/x86/intel_baytrail.c b/drivers/platform/x86/intel_baytrail.c
new file mode 100644
index 0000000..fcb49cc
--- /dev/null
+++ b/drivers/platform/x86/intel_baytrail.c
@@ -0,0 +1,225 @@
+/*
+ * Baytrail IOSF-SB MailBox Interface Driver
+ * Copyright (c) 2013, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ *
+ * The IOSF-SB is a fabric bus available on Atom based SOC's that uses a
+ * mailbox interface (MBI) to communicate with mutiple devices. This
+ * driver implements BayTrail-specific access to this interface.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/spinlock.h>
+#include <linux/pci.h>
+
+#include "intel_baytrail.h"
+
+static DEFINE_SPINLOCK(iosf_mbi_lock);
+
+static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
+{
+ return (op << 24) | (port << 16) | (offset << 8) | BT_MBI_ENABLE;
+}
+
+static struct pci_dev *mbi_pdev; /* one mbi device */
+
+static int iosf_mbi_pci_read_mdr(u32 mcrx, u32 mcr, u32 *mdr)
+{
+ int result;
+
+ if (!mbi_pdev)
+ return -ENODEV;
+
+ if (mcrx) {
+ result = pci_write_config_dword(mbi_pdev, BT_MBI_MCRX_OFFSET,
+ mcrx);
+ if (result < 0)
+ goto fail_read;
+ }
+
+ result = pci_write_config_dword(mbi_pdev, BT_MBI_MCR_OFFSET, mcr);
+ if (result < 0)
+ goto fail_read;
+
+ result = pci_read_config_dword(mbi_pdev, BT_MBI_MDR_OFFSET, mdr);
+ if (result < 0)
+ goto fail_read;
+
+ return 0;
+
+fail_read:
+ dev_err(&mbi_pdev->dev, "PCI config access failed with %d\n", result);
+ return result;
+}
+
+static int iosf_mbi_pci_write_mdr(u32 mcrx, u32 mcr, u32 mdr)
+{
+ int result;
+
+ if (!mbi_pdev)
+ return -ENODEV;
+
+ result = pci_write_config_dword(mbi_pdev, BT_MBI_MDR_OFFSET, mdr);
+ if (result < 0)
+ goto fail_write;
+
+ if (mcrx) {
+ result = pci_write_config_dword(mbi_pdev, BT_MBI_MCRX_OFFSET,
+ mcrx);
+ if (result < 0)
+ goto fail_write;
+ }
+
+ result = pci_write_config_dword(mbi_pdev, BT_MBI_MCR_OFFSET, mcr);
+ if (result < 0)
+ goto fail_write;
+
+ return 0;
+
+fail_write:
+ dev_err(&mbi_pdev->dev, "PCI config access failed with %d\n", result);
+ return result;
+}
+
+int bt_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
+{
+ u32 mcr, mcrx;
+ unsigned long flags;
+ int ret;
+
+ /*Access to the GFX unit is handled by GPU code */
+ if (port == BT_MBI_UNIT_GFX) {
+ WARN_ON(1);
+ return -EPERM;
+ }
+
+ mcr = iosf_mbi_form_mcr(opcode, port, offset & BT_MBI_MASK_LO);
+ mcrx = offset & BT_MBI_MASK_HI;
+
+ spin_lock_irqsave(&iosf_mbi_lock, flags);
+ ret = iosf_mbi_pci_read_mdr(mcrx, mcr, mdr);
+ spin_unlock_irqrestore(&iosf_mbi_lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL(bt_mbi_read);
+
+int bt_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr)
+{
+ u32 mcr, mcrx;
+ unsigned long flags;
+ int ret;
+
+ /*Access to the GFX unit is handled by GPU code */
+ if (port == BT_MBI_UNIT_GFX) {
+ WARN_ON(1);
+ return -EPERM;
+ }
+
+ mcr = iosf_mbi_form_mcr(opcode, port, offset & BT_MBI_MASK_LO);
+ mcrx = offset & BT_MBI_MASK_HI;
+
+ spin_lock_irqsave(&iosf_mbi_lock, flags);
+ ret = iosf_mbi_pci_write_mdr(mcrx, mcr, mdr);
+ spin_unlock_irqrestore(&iosf_mbi_lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL(bt_mbi_write);
+
+int bt_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
+{
+ u32 mcr, mcrx;
+ u32 value;
+ unsigned long flags;
+ int ret;
+
+ /*Access to the GFX unit is handled by GPU code */
+ if (port == BT_MBI_UNIT_GFX) {
+ WARN_ON(1);
+ return -EPERM;
+ }
+
+ mcr = iosf_mbi_form_mcr(opcode, port, offset & BT_MBI_MASK_LO);
+ mcrx = offset & BT_MBI_MASK_HI;
+
+ spin_lock_irqsave(&iosf_mbi_lock, flags);
+
+ /* Read current mdr value */
+ ret = iosf_mbi_pci_read_mdr(mcrx, mcr & BT_MBI_RD_MASK, &value);
+ if (ret < 0) {
+ spin_unlock_irqrestore(&iosf_mbi_lock, flags);
+ return ret;
+ }
+
+ /* Apply mask */
+ value &= ~mask;
+ mdr &= mask;
+ value |= mdr;
+
+ /* Write back */
+ ret = iosf_mbi_pci_write_mdr(mcrx, mcr | BT_MBI_WR_MASK, value);
+
+ spin_unlock_irqrestore(&iosf_mbi_lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL(bt_mbi_modify);
+
+static int iosf_mbi_probe(struct pci_dev *pdev,
+ const struct pci_device_id *unused)
+{
+ int ret;
+
+ ret = pci_enable_device(pdev);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "error: could not enable device\n");
+ return ret;
+ }
+
+ mbi_pdev = pci_dev_get(pdev);
+ return 0;
+}
+
+static DEFINE_PCI_DEVICE_TABLE(iosf_mbi_pci_ids) = {
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0F00) },
+ { 0, },
+};
+MODULE_DEVICE_TABLE(pci, iosf_mbi_pci_ids);
+
+static struct pci_driver iosf_mbi_pci_driver = {
+ .name = "iosf_mbi_pci",
+ .probe = iosf_mbi_probe,
+ .id_table = iosf_mbi_pci_ids,
+};
+
+static int __init bt_mbi_init(void)
+{
+ return pci_register_driver(&iosf_mbi_pci_driver);
+}
+
+static void __exit bt_mbi_exit(void)
+{
+ pci_unregister_driver(&iosf_mbi_pci_driver);
+ if (mbi_pdev) {
+ pci_dev_put(mbi_pdev);
+ mbi_pdev = NULL;
+ }
+}
+
+module_init(bt_mbi_init);
+module_exit(bt_mbi_exit);
+
+MODULE_AUTHOR("David E. Box <[email protected]>");
+MODULE_DESCRIPTION("BayTrail Mailbox Interface accessor");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel_baytrail.h b/drivers/platform/x86/intel_baytrail.h
new file mode 100644
index 0000000..8bcc311
--- /dev/null
+++ b/drivers/platform/x86/intel_baytrail.h
@@ -0,0 +1,90 @@
+/*
+ * intel_baytrail.h: MailBox access support for Intel BayTrail platforms
+ */
+
+#ifndef INTEL_BAYTRAIL_MBI_SYMS_H
+#define INTEL_BAYTRAIL_MBI_SYMS_H
+
+#define BT_MBI_MCR_OFFSET 0xD0
+#define BT_MBI_MDR_OFFSET 0xD4
+#define BT_MBI_MCRX_OFFSET 0xD8
+
+#define BT_MBI_RD_MASK 0xFEFFFFFF
+#define BT_MBI_WR_MASK 0X01000000
+
+#define BT_MBI_MASK_HI 0xFFFFFF00
+#define BT_MBI_MASK_LO 0x000000FF
+#define BT_MBI_ENABLE 0xF0
+
+/* BT-SB unit access methods */
+#define BT_MBI_UNIT_AUNIT 0x00
+#define BT_MBI_UNIT_SMC 0x01
+#define BT_MBI_UNIT_CPU 0x02
+#define BT_MBI_UNIT_BUNIT 0x03
+#define BT_MBI_UNIT_PMC 0x04
+#define BT_MBI_UNIT_GFX 0x06
+#define BT_MBI_UNIT_SMI 0x0C
+#define BT_MBI_UNIT_USB 0x43
+#define BT_MBI_UNIT_SATA 0xA3
+#define BT_MBI_UNIT_PCIE 0xA6
+
+/* Read/write opcodes */
+#define BT_MBI_AUNIT_READ 0x10
+#define BT_MBI_AUNIT_WRITE 0x11
+#define BT_MBI_SMC_READ 0x10
+#define BT_MBI_SMC_WRITE 0x11
+#define BT_MBI_CPU_READ 0x10
+#define BT_MBI_CPU_WRITE 0x11
+#define BT_MBI_BUNIT_READ 0x10
+#define BT_MBI_BUNIT_WRITE 0x11
+#define BT_MBI_PMC_READ 0x06
+#define BT_MBI_PMC_WRITE 0x07
+#define BT_MBI_GFX_READ 0x00
+#define BT_MBI_GFX_WRITE 0x01
+#define BT_MBI_SMIO_READ 0x06
+#define BT_MBI_SMIO_WRITE 0x07
+#define BT_MBI_USB_READ 0x06
+#define BT_MBI_USB_WRITE 0x07
+#define BT_MBI_SATA_READ 0x00
+#define BT_MBI_SATA_WRITE 0x01
+#define BT_MBI_PCIE_READ 0x00
+#define BT_MBI_PCIE_WRITE 0x01
+
+/**
+ * bt_mbi_read() - MailBox Interface read command
+ * @port: port indicating subunit being accessed
+ * @opcode: port specific read or write opcode
+ * @offset: register address offset
+ * @mdr: register data to be read
+ *
+ * Locking is handled by spinlock - cannot sleep.
+ * Return: Nonzero on error
+ */
+int bt_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr);
+
+/**
+ * bt_mbi_write() - MailBox unmasked write command
+ * @port: port indicating subunit being accessed
+ * @opcode: port specific read or write opcode
+ * @offset: register address offset
+ * @mdr: register data to be written
+ *
+ * Locking is handled by spinlock - cannot sleep.
+ * Return: Nonzero on error
+ */
+int bt_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
+
+/**
+ * bt_mbi_modify() - MailBox masked write command
+ * @port: port indicating subunit being accessed
+ * @opcode: port specific read or write opcode
+ * @offset: register address offset
+ * @mdr: register data being modified
+ * @mask: mask indicating bits in mdr to be modified
+ *
+ * Locking is handled by spinlock - cannot sleep.
+ * Return: Nonzero on error
+ */
+int bt_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
+
+#endif /* INTEL_BAYTRAIL_MBI_SYMS_H */
--
1.7.10.4
On 01/07/14 10:03, David E. Box wrote:
> From: "David E. Box" <[email protected]>
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index b51a746..6e199a5 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -819,4 +819,14 @@ config PVPANIC
> a paravirtualized device provided by QEMU; it lets a virtual machine
> (guest) communicate panic events to the host.
>
> +config INTEL_BAYTRAIL_MBI
> + tristate
Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
Doesn't it need a prompt string?
How did you enable it and test it?
> + depends on PCI
> + ---help---
> + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> + Interface. This is a requirement for systems that need to configure
> + the PUNIT for power management features such as RAPL. Register
> + addresses and r/w opcodes are defined in
Think of users reading this. At least change "r/w" to read/write.
What is IOSF? does it matter here?
PUNIT? RAPL?
> + drivers/platform/x86/intel_baytrail.c.
> +
> endif # X86_PLATFORM_DEVICES
--
~Randy
On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
> On 01/07/14 10:03, David E. Box wrote:
> > From: "David E. Box" <[email protected]>
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index b51a746..6e199a5 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -819,4 +819,14 @@ config PVPANIC
> > a paravirtualized device provided by QEMU; it lets a virtual machine
> > (guest) communicate panic events to the host.
> >
> > +config INTEL_BAYTRAIL_MBI
> > + tristate
>
> Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
> Doesn't it need a prompt string?
> How did you enable it and test it?
>
It is not displayed on purpose. The driver isn't exposed to user space.
It was tested by adding the option to .config, both as module and built-in.
> > + depends on PCI
> > + ---help---
> > + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> > + Interface. This is a requirement for systems that need to configure
> > + the PUNIT for power management features such as RAPL. Register
> > + addresses and r/w opcodes are defined in
>
> Think of users reading this. At least change "r/w" to read/write.
> What is IOSF? does it matter here?
> PUNIT? RAPL?
>
If you don't know what the IOSF Sideband is you probably shouldn't be enabling
this feature. Users of this driver (of which for now there are few) will be
kernel space drivers that should have ample knowledge of the registers this
interface provides access to.
Dave
On 01/07/14 10:48, David E. Box wrote:
> On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
>> On 01/07/14 10:03, David E. Box wrote:
>>> From: "David E. Box" <[email protected]>
>>>
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index b51a746..6e199a5 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -819,4 +819,14 @@ config PVPANIC
>>> a paravirtualized device provided by QEMU; it lets a virtual machine
>>> (guest) communicate panic events to the host.
>>>
>>> +config INTEL_BAYTRAIL_MBI
>>> + tristate
>>
>> Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
>> Doesn't it need a prompt string?
>> How did you enable it and test it?
>>
>
> It is not displayed on purpose. The driver isn't exposed to user space.
> It was tested by adding the option to .config, both as module and built-in.
>
>>> + depends on PCI
>>> + ---help---
>>> + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
>>> + Interface. This is a requirement for systems that need to configure
>>> + the PUNIT for power management features such as RAPL. Register
>>> + addresses and r/w opcodes are defined in
>>
>> Think of users reading this. At least change "r/w" to read/write.
>> What is IOSF? does it matter here?
>> PUNIT? RAPL?
>>
>
> If you don't know what the IOSF Sideband is you probably shouldn't be enabling
> this feature. Users of this driver (of which for now there are few) will be
> kernel space drivers that should have ample knowledge of the registers this
> interface provides access to.
I see. Thanks.
--
~Randy
On Tuesday, January 07, 2014 10:48:05 AM David E. Box wrote:
> On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
> > On 01/07/14 10:03, David E. Box wrote:
> > > From: "David E. Box" <[email protected]>
> > >
> > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > index b51a746..6e199a5 100644
> > > --- a/drivers/platform/x86/Kconfig
> > > +++ b/drivers/platform/x86/Kconfig
> > > @@ -819,4 +819,14 @@ config PVPANIC
> > > a paravirtualized device provided by QEMU; it lets a virtual machine
> > > (guest) communicate panic events to the host.
> > >
> > > +config INTEL_BAYTRAIL_MBI
> > > + tristate
> >
> > Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
> > Doesn't it need a prompt string?
> > How did you enable it and test it?
> >
>
> It is not displayed on purpose. The driver isn't exposed to user space.
> It was tested by adding the option to .config, both as module and built-in.
>
> > > + depends on PCI
> > > + ---help---
> > > + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> > > + Interface. This is a requirement for systems that need to configure
> > > + the PUNIT for power management features such as RAPL. Register
> > > + addresses and r/w opcodes are defined in
> >
> > Think of users reading this. At least change "r/w" to read/write.
> > What is IOSF? does it matter here?
> > PUNIT? RAPL?
> >
>
> If you don't know what the IOSF Sideband is you probably shouldn't be enabling
> this feature.
What about generic x86 distro kernels? They won't know in advance whether or
not they will need this feature.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Tue, Jan 07, 2014 at 09:46:57PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, January 07, 2014 10:48:05 AM David E. Box wrote:
> > On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
> > > On 01/07/14 10:03, David E. Box wrote:
> > > > From: "David E. Box" <[email protected]>
> > > >
> > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > index b51a746..6e199a5 100644
> > > > --- a/drivers/platform/x86/Kconfig
> > > > +++ b/drivers/platform/x86/Kconfig
> > > > @@ -819,4 +819,14 @@ config PVPANIC
> > > > a paravirtualized device provided by QEMU; it lets a virtual machine
> > > > (guest) communicate panic events to the host.
> > > >
> > > > +config INTEL_BAYTRAIL_MBI
> > > > + tristate
> > >
> > > Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
> > > Doesn't it need a prompt string?
> > > How did you enable it and test it?
> > >
> >
> > It is not displayed on purpose. The driver isn't exposed to user space.
> > It was tested by adding the option to .config, both as module and built-in.
> >
> > > > + depends on PCI
> > > > + ---help---
> > > > + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> > > > + Interface. This is a requirement for systems that need to configure
> > > > + the PUNIT for power management features such as RAPL. Register
> > > > + addresses and r/w opcodes are defined in
> > >
> > > Think of users reading this. At least change "r/w" to read/write.
> > > What is IOSF? does it matter here?
> > > PUNIT? RAPL?
> > >
> >
> > If you don't know what the IOSF Sideband is you probably shouldn't be enabling
> > this feature.
>
> What about generic x86 distro kernels? They won't know in advance whether or
> not they will need this feature.
Ok, I spoke with other developers and I apparently misunderstood the context
here. Distro's enable these features and this is too detailed for them to know
what to do with it. How about simply "Required to enable platform specific power
managemnet features on Baytrail"?
KISS is easier said than done.
Dave
On Tuesday, January 07, 2014 01:43:00 PM David E. Box wrote:
> On Tue, Jan 07, 2014 at 09:46:57PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, January 07, 2014 10:48:05 AM David E. Box wrote:
> > > On Tue, Jan 07, 2014 at 10:15:03AM -0800, Randy Dunlap wrote:
> > > > On 01/07/14 10:03, David E. Box wrote:
> > > > > From: "David E. Box" <[email protected]>
> > > > >
> > > > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > > > > index b51a746..6e199a5 100644
> > > > > --- a/drivers/platform/x86/Kconfig
> > > > > +++ b/drivers/platform/x86/Kconfig
> > > > > @@ -819,4 +819,14 @@ config PVPANIC
> > > > > a paravirtualized device provided by QEMU; it lets a virtual machine
> > > > > (guest) communicate panic events to the host.
> > > > >
> > > > > +config INTEL_BAYTRAIL_MBI
> > > > > + tristate
> > > >
> > > > Is this kconfig option displayed when you run menuconfig/nconfig/xconfig/gconfig etc.?
> > > > Doesn't it need a prompt string?
> > > > How did you enable it and test it?
> > > >
> > >
> > > It is not displayed on purpose. The driver isn't exposed to user space.
> > > It was tested by adding the option to .config, both as module and built-in.
> > >
> > > > > + depends on PCI
> > > > > + ---help---
> > > > > + Needed on Baytrail platforms for access to the IOSF Sideband Mailbox
> > > > > + Interface. This is a requirement for systems that need to configure
> > > > > + the PUNIT for power management features such as RAPL. Register
> > > > > + addresses and r/w opcodes are defined in
> > > >
> > > > Think of users reading this. At least change "r/w" to read/write.
> > > > What is IOSF? does it matter here?
> > > > PUNIT? RAPL?
> > > >
> > >
> > > If you don't know what the IOSF Sideband is you probably shouldn't be enabling
> > > this feature.
> >
> > What about generic x86 distro kernels? They won't know in advance whether or
> > not they will need this feature.
>
> Ok, I spoke with other developers and I apparently misunderstood the context
> here. Distro's enable these features and this is too detailed for them to know
> what to do with it. How about simply "Required to enable platform specific power
> managemnet features on Baytrail"?
>
> KISS is easier said than done.
Well, I personally think that this code should go into arch/x86/ as library code
needed to access IOSF Sideband on some platforms. I probably would make modules
depending on it select it, so for example if the RAPL driver is going to be
built, your driver should be build either and rather unconditionally in that
case.
So the rule should be "if something *may* need it, build it" in my opinion.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 01/07/2014 04:11 PM, Rafael J. Wysocki wrote:
>>
>> Ok, I spoke with other developers and I apparently misunderstood the context
>> here. Distro's enable these features and this is too detailed for them to know
>> what to do with it. How about simply "Required to enable platform specific power
>> managemnet features on Baytrail"?
>>
>> KISS is easier said than done.
>
> Well, I personally think that this code should go into arch/x86/ as library code
> needed to access IOSF Sideband on some platforms. I probably would make modules
> depending on it select it, so for example if the RAPL driver is going to be
> built, your driver should be build either and rather unconditionally in that
> case.
>
> So the rule should be "if something *may* need it, build it" in my opinion.
>
I thought we were targeting this for drivers/x86? However, perhaps with
power management tied in that doesn't make too much sense.
-hpa
On Wed, Jan 08, 2014 at 01:11:22AM +0100, Rafael J. Wysocki wrote:
> Well, I personally think that this code should go into arch/x86/ as library code
> needed to access IOSF Sideband on some platforms.
I don't disagree. However for the record this is not the first time it has been
discussed and I already moved it from arch/x86 to platform on the suggestion of
several maintainers. I will keep the interface generic except that it has to be
stated that it will only support those platforms that can enumerate this device
using PCI. Plus I learned there are others who plan to patch when it gets
accepted to support other platforms using different methods of
enumeration/communication. I would thik this probably cements its placement in
arch/x86.
> I probably would make modules
> depending on it select it, so for example if the RAPL driver is going to be
> built, your driver should be build either and rather unconditionally in that
> case.
>
Wouldn't such a dependency be handled by the RAPL driver et al. How can I force
modules to build this driver? Reverse dependency? Also the biggest consumer of
the driver doesn't have code upstream yet.
> So the rule should be "if something *may* need it, build it" in my opinion.
You suggest that even though non-IOSF systems don't need this driver to enable
RAPL, it should build anyway since it's a dependency for systems that do have
IOSF? Even if this is what you suggest I'd prefer the owner of the driver that
has the dependency be the one to patch this driver to make in build in that
case. I do not know all who would use it.
Dave Box
On Tuesday, January 07, 2014 09:27:17 PM David E. Box wrote:
> On Wed, Jan 08, 2014 at 01:11:22AM +0100, Rafael J. Wysocki wrote:
> > Well, I personally think that this code should go into arch/x86/ as library code
> > needed to access IOSF Sideband on some platforms.
>
> I don't disagree. However for the record this is not the first time it has been
> discussed and I already moved it from arch/x86 to platform on the suggestion of
> several maintainers. I will keep the interface generic except that it has to be
> stated that it will only support those platforms that can enumerate this device
> using PCI. Plus I learned there are others who plan to patch when it gets
> accepted to support other platforms using different methods of
> enumeration/communication. I would thik this probably cements its placement in
> arch/x86.
I think so.
> > I probably would make modules
> > depending on it select it, so for example if the RAPL driver is going to be
> > built, your driver should be build either and rather unconditionally in that
> > case.
> >
>
> Wouldn't such a dependency be handled by the RAPL driver et al. How can I force
> modules to build this driver? Reverse dependency? Also the biggest consumer of
> the driver doesn't have code upstream yet.
The modules depending on your driver can do
select INTEL_BAYTRAIL_MBI
in their Kconfig entries. That will cause it to be built.
> > So the rule should be "if something *may* need it, build it" in my opinion.
>
> You suggest that even though non-IOSF systems don't need this driver to enable
> RAPL, it should build anyway since it's a dependency for systems that do have
> IOSF? Even if this is what you suggest I'd prefer the owner of the driver that
> has the dependency be the one to patch this driver to make in build in that
> case. I do not know all who would use it.
Simply, don't enable it by default and let its users do the above.
Then, they won't have to do any #ifdef CONFIG_INTEL_BAYTRAIL_MBI stuff in their
code (I'd change the name of the CONFIG_ thing if it goes beyond BayTrail).
Of course, it will be compiled for generic x86 kernels this way, but since
they have to assume they may run on anything, they'll have to build it anyway.
The only "problematic" case will be kernels compiled by users of systems that
don't use IOSF Sideband and do use things like RAPL, but I really wouldn't
try to optimize for that case.
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.