Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Sun, Mar 8, 2015 at 12:50 PM, Guenter Roeck wrote: > On 03/08/2015 12:30 PM, Andy Lutomirski wrote: > [ ... ] > >>> One other question: from my reading of the spec, it should be possible to augment this driver to expose a temporate sensor subdevice that shows recent cached temperatures from HW DIMM measurements. They would be redundant with the jc42 outputs, but it would be safe to use them even on systems without safe SMBUS arbitration. Should I do that as a followup later on? >>> >>> Without thinking too much about it, this should be a separate driver, >>> and I think it might actually be more valuable (since less risky) >>> than this entire patch set. >> >> >> The main problem there is that they'll fight over the PCI ids. >> > That is why we have mfd drivers. While that most likely won't be a > solution here (and I don't suggest it), you can use the same idea. > If can not be instantiated as pci driver, the alternative would be > to instantiate it as platform driver and let it get its pci > information from the parent device. > Hmm. It could make sense to have an Intel chipset driver that instantiates sub-devices for thermal monitoring, smbus, and EDAC. It might be a bit of a cleanup for sb-edac, too. Anyway, this isn't currently necessary. Let's deal with it when we get there. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/08/2015 12:30 PM, Andy Lutomirski wrote: [ ... ] One other question: from my reading of the spec, it should be possible to augment this driver to expose a temporate sensor subdevice that shows recent cached temperatures from HW DIMM measurements. They would be redundant with the jc42 outputs, but it would be safe to use them even on systems without safe SMBUS arbitration. Should I do that as a followup later on? Without thinking too much about it, this should be a separate driver, and I think it might actually be more valuable (since less risky) than this entire patch set. The main problem there is that they'll fight over the PCI ids. That is why we have mfd drivers. While that most likely won't be a solution here (and I don't suggest it), you can use the same idea. If can not be instantiated as pci driver, the alternative would be to instantiate it as platform driver and let it get its pci information from the parent device. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Sun, Mar 8, 2015 at 8:39 AM, Guenter Roeck wrote:
> On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
>>
>> On Mar 7, 2015 6:39 AM, "Guenter Roeck" wrote:
>>>
>>>
>>> On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
Sandy Bridge Xeon and Extreme chips have integrated memory
controllers with (rather limited) onboard SMBUS masters. This
driver gives access to the bus.
There are various groups working on standardizing a way to arbitrate
access to the bus between the OS, SMM firmware, a BMC, hardware
thermal control, etc. In the mean time, running this driver is
unsafe except under special circumstances. Nonetheless, this driver
has real users.
As a compromise, the driver will refuse to load unless
i2c_imc.allow_unsafe_access=Y. When safe access becomes available,
we can leave this option as a way for legacy users to run the
driver, and we'll allow the driver to load by default if safe bus
access is available.
Signed-off-by: Andy Lutomirski
---
>
> [ ... ]
>
+
+ if (imc_wait_not_busy(priv, chan, ) != 0) {
+ /* Timeout. TODO: Reset the controller? */
+ ret = -EIO;
>>>
>>>
>>>
>>> timeout -> -ETIMEDOUT ?
>>
>>
>> OK
>>
> Actually, I just realized that imc_wait_not_busy returns a valid error code.
> Given that, some static analysis checkers (and now me) will ask you
> why you don't just use the error code from imc_wait_not_busy.
> This applies to other calls to the same function as well.
I changed it the other way. One if the imc_wait_not_busy callers is
trying to get access to the bus in the first place. If that fails,
then I think that -EBUSY is appropriate -- we're failing because the
thing is in use, not because our transaction timed out. If the other
caller of imc_wait_not_busy fails, then our transaction timed out. So
I'll make imc_wait_not_busy return bool. This ends up simplifying the
code a bit, too.
>
>>>
>>>
+ dev_err(>pci_dev->dev, "controller is wedged\n");
>>>
>>>
>>>
>>> If this happens, it will presumably happen all the time and the message
>>> will
>>> pollute the log. Is the message really necessary ?
>>
>>
>> I'd rather log something to help diagnose. Would rate-limiting it be
>> okay?
>>
> It would still pollute the log because it doesn't happen that often.
> A message once a second still fills the log.
>
> If it is for diagnose/debugging, why not dev_dbg ?
>
OK. I demoted some other log lines, too.
>>>
>>>
+ goto out_release;
+ }
+
+ /*
+* Be paranoid: try to detect races. This will only detect
races
+* against BIOS, not against hardware. (I've never seen this
happen.)
+*/
+ pci_read_config_dword(priv->pci_dev, SMBCMD(chan), _cmd);
+ pci_read_config_dword(priv->pci_dev, SMBCNTL(chan),
_cntl);
+ if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) ||
+ ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) {
+ WARN(1, "iMC SMBUS raced against firmware");
+ dev_emerg(>pci_dev->dev,
>>>
>>>
>>>
>>> Is a stack trace and dev_emerg really warranted here ?
>>>
>>
>> If this happens, something's very wrong and the user should stop using
>> the driver. We could potentially write the wrong address, and, if we
>> manage to screw up thermal management, we could potentially corrupt
>> data for to an inappropriate refresh interval.
>>
>> IOW, I want to hear about it if this happens.
>>
> Ok, that explains the WARN. Still not an "emergency", though.
>
It's dev_err now.
> Coding style suggests
>
> if (!(stat & SMBSTAT_RDO)) {
> dev_err();
> ret - -EIO;
> goto out_release;
> }
>
> and
>
> if (!(stat & SMBSTAT_WOD)) {
> dev_err();
> ret = -EIO;
> goto out_release;
> }
Done. dev_dbg, too -- I think that either all of these errors should
be dev_dbg or none should be.
> On a side note, I am a bit confused about the note "same bug as in the read
> case".
> Do you want to say that RDO is sometimes/often set in the write case ?
> If so, it might make more sense to just say it.
Yes, fixed.
>
> [ ... ]
>
+static void imc_free_channel(struct imc_priv *priv, int i)
+{
+ struct imc_channel *ch = >channels[i];
+
+ /* This can recurse into imc_smbus_xfer. */
>>>
>>>
>>>
>>> So ?
>>
>>
>> It needs to happen before mutex_destroy. I improved the comment.
>>
> Seems to me obvious that a mutex would be destroyed last in cleanup.
>
It wasn't to me. I'll remove the comment.
>> I want to be ready for future hardware that might support more than
>> two channels.
>>
> Not my call to make, but I am a bit wary of future hardware support which
> may
> never materialize. I prefer writing code liks this for the
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
On Mar 7, 2015 6:39 AM, "Guenter Roeck" wrote:
On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
Sandy Bridge Xeon and Extreme chips have integrated memory
controllers with (rather limited) onboard SMBUS masters. This
driver gives access to the bus.
There are various groups working on standardizing a way to arbitrate
access to the bus between the OS, SMM firmware, a BMC, hardware
thermal control, etc. In the mean time, running this driver is
unsafe except under special circumstances. Nonetheless, this driver
has real users.
As a compromise, the driver will refuse to load unless
i2c_imc.allow_unsafe_access=Y. When safe access becomes available,
we can leave this option as a way for legacy users to run the
driver, and we'll allow the driver to load by default if safe bus
access is available.
Signed-off-by: Andy Lutomirski
---
[ ... ]
+
+ if (imc_wait_not_busy(priv, chan, ) != 0) {
+ /* Timeout. TODO: Reset the controller? */
+ ret = -EIO;
timeout -> -ETIMEDOUT ?
OK
Actually, I just realized that imc_wait_not_busy returns a valid error code.
Given that, some static analysis checkers (and now me) will ask you
why you don't just use the error code from imc_wait_not_busy.
This applies to other calls to the same function as well.
+ dev_err(>pci_dev->dev, "controller is wedged\n");
If this happens, it will presumably happen all the time and the message will
pollute the log. Is the message really necessary ?
I'd rather log something to help diagnose. Would rate-limiting it be okay?
It would still pollute the log because it doesn't happen that often.
A message once a second still fills the log.
If it is for diagnose/debugging, why not dev_dbg ?
+ goto out_release;
+ }
+
+ /*
+* Be paranoid: try to detect races. This will only detect races
+* against BIOS, not against hardware. (I've never seen this happen.)
+*/
+ pci_read_config_dword(priv->pci_dev, SMBCMD(chan), _cmd);
+ pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), _cntl);
+ if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) ||
+ ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) {
+ WARN(1, "iMC SMBUS raced against firmware");
+ dev_emerg(>pci_dev->dev,
Is a stack trace and dev_emerg really warranted here ?
If this happens, something's very wrong and the user should stop using
the driver. We could potentially write the wrong address, and, if we
manage to screw up thermal management, we could potentially corrupt
data for to an inappropriate refresh interval.
IOW, I want to hear about it if this happens.
Ok, that explains the WARN. Still not an "emergency", though.
+ "Access to channel %d raced: cmd 0x%08X->0x%08X, cntl
0x%08X->0x%08X\n",
+ chan, cmd, final_cmd, cntl, final_cntl);
+ atomic_set(_raced, 1);
+ ret = -EIO;
+ goto out_release;
+ }
+
+ if (stat & SMBSTAT_SBE) {
+ /*
+* Clear the error to re-enable TSOD polling. The docs say
+* that, as long as SBE is set, TSOD polling won't happen.
+* The docs also say that writing zero to this bit (which is
+* the only writable bit in the whole register) will clear
+* the error. Empirically, writing 0 does not clear SBE, but
+* it's probably still good to do the write in compliance with
+* the spec. (TSOD polling still happens and seems to
+* clear SBE on its own.)
+*/
+ pci_write_config_dword(priv->pci_dev, SMBSTAT(chan), 0);
+ ret = -ENXIO;
+ goto out_release;
+ }
+
+ if (read_write == I2C_SMBUS_READ) {
+ if (stat & SMBSTAT_RDO) {
+ /*
+* The iMC SMBUS controller thinks of SMBUS
+* words as being big-endian (MSB first).
+* Linux treats them as little-endian, so we need
+* to swap them.
+*
+* Note: the controller will often (always?) set
+* WOD here. This is probably a bug.
+*/
+ if (size == I2C_SMBUS_WORD_DATA)
+ data->word = swab16(stat & SMBSTAT_RDATA_MASK);
+ else
+ data->byte = stat & 0xFF;
+ ret = 0;
ret is already guaranteed to be 0 here.
+ } else {
+ dev_err(>pci_dev->dev,
+ "Unexpected read status 0x%08X\n", stat);
+ ret = -EIO;
+ }
+ } else {
+ if (stat & SMBSTAT_WOD) {
+
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Mar 7, 2015 6:39 AM, "Guenter Roeck" wrote: > > On 03/06/2015 06:50 PM, Andy Lutomirski wrote: >> >> Sandy Bridge Xeon and Extreme chips have integrated memory >> controllers with (rather limited) onboard SMBUS masters. This >> driver gives access to the bus. >> >> There are various groups working on standardizing a way to arbitrate >> access to the bus between the OS, SMM firmware, a BMC, hardware >> thermal control, etc. In the mean time, running this driver is >> unsafe except under special circumstances. Nonetheless, this driver >> has real users. >> >> As a compromise, the driver will refuse to load unless >> i2c_imc.allow_unsafe_access=Y. When safe access becomes available, >> we can leave this option as a way for legacy users to run the >> driver, and we'll allow the driver to load by default if safe bus >> access is available. >> >> Signed-off-by: Andy Lutomirski >> --- > > > Please consider running your patch through checkpatch --strict, or at least > checkpatch. > [ I won't comment on the checkpatch problems below ] > > >> drivers/i2c/busses/Kconfig | 18 ++ >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-imc.c | 583 >> +++ >> 3 files changed, 602 insertions(+) >> create mode 100644 drivers/i2c/busses/i2c-imc.c >> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index ab838d9e28b6..d6b9ce164fbf 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -149,6 +149,24 @@ config I2C_ISMT >> This driver can also be built as a module. If so, the module will >> be >> called i2c-ismt. >> >> +config I2C_IMC >> + tristate "Intel iMC (LGA 2011) SMBus Controller" >> + depends on PCI && X86 >> + select I2C_DIMM_BUS >> + help >> + If you say yes to this option, support will be included for the >> Intel >> + Integrated Memory Controller SMBus host controller interface. This >> + controller is found on LGA 2011 Xeons and Core i7 Extremes. >> + >> + There are currently no systems on which the kernel knows that it >> can >> + safely enable this driver. For now, you need to pass this driver a >> + scary module parameter, and you should only pass that parameter if >> you >> + have a special motherboard and know exactly what you are doing. >> + Special motherboards include the Supermicro X9DRH-iF-NV. >> + >> + This driver can also be built as a module. If so, the module will >> be >> + called i2c-imc. >> + >> config I2C_PIIX4 >> tristate "Intel PIIX4 and compatible >> (ATI/AMD/Serverworks/Broadcom/SMSC)" >> depends on PCI >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 56388f658d2f..4287c891e782 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o >> obj-$(CONFIG_I2C_I801)+= i2c-i801.o >> obj-$(CONFIG_I2C_ISCH)+= i2c-isch.o >> obj-$(CONFIG_I2C_ISMT)+= i2c-ismt.o >> +obj-$(CONFIG_I2C_IMC) += i2c-imc.o >> obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o >> obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o >> obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o >> diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c >> new file mode 100644 >> index ..2dbf171114c6 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-imc.c >> @@ -0,0 +1,583 @@ >> +/* >> + * Copyright (c) 2013 Andrew Lutomirski >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 >> + * as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that 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. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* >> + * The datasheet can be found here, for example: >> + * >> http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf >> + * >> + * There seem to be quite a few bugs or spec errors, though: >> + * >> + * - A successful transaction sets WOD and RDO. >> + * >> + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). >> + * >> + * - Erratum BT109, which says: >> + * >> + * The processor may not complete SMBus (System
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/08/2015 12:30 PM, Andy Lutomirski wrote: [ ... ] One other question: from my reading of the spec, it should be possible to augment this driver to expose a temporate sensor subdevice that shows recent cached temperatures from HW DIMM measurements. They would be redundant with the jc42 outputs, but it would be safe to use them even on systems without safe SMBUS arbitration. Should I do that as a followup later on? Without thinking too much about it, this should be a separate driver, and I think it might actually be more valuable (since less risky) than this entire patch set. The main problem there is that they'll fight over the PCI ids. That is why we have mfd drivers. While that most likely won't be a solution here (and I don't suggest it), you can use the same idea. If can not be instantiated as pci driver, the alternative would be to instantiate it as platform driver and let it get its pci information from the parent device. Guenter -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Sun, Mar 8, 2015 at 8:39 AM, Guenter Roeck li...@roeck-us.net wrote:
On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
On Mar 7, 2015 6:39 AM, Guenter Roeck li...@roeck-us.net wrote:
On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
Sandy Bridge Xeon and Extreme chips have integrated memory
controllers with (rather limited) onboard SMBUS masters. This
driver gives access to the bus.
There are various groups working on standardizing a way to arbitrate
access to the bus between the OS, SMM firmware, a BMC, hardware
thermal control, etc. In the mean time, running this driver is
unsafe except under special circumstances. Nonetheless, this driver
has real users.
As a compromise, the driver will refuse to load unless
i2c_imc.allow_unsafe_access=Y. When safe access becomes available,
we can leave this option as a way for legacy users to run the
driver, and we'll allow the driver to load by default if safe bus
access is available.
Signed-off-by: Andy Lutomirski l...@amacapital.net
---
[ ... ]
+
+ if (imc_wait_not_busy(priv, chan, stat) != 0) {
+ /* Timeout. TODO: Reset the controller? */
+ ret = -EIO;
timeout - -ETIMEDOUT ?
OK
Actually, I just realized that imc_wait_not_busy returns a valid error code.
Given that, some static analysis checkers (and now me) will ask you
why you don't just use the error code from imc_wait_not_busy.
This applies to other calls to the same function as well.
I changed it the other way. One if the imc_wait_not_busy callers is
trying to get access to the bus in the first place. If that fails,
then I think that -EBUSY is appropriate -- we're failing because the
thing is in use, not because our transaction timed out. If the other
caller of imc_wait_not_busy fails, then our transaction timed out. So
I'll make imc_wait_not_busy return bool. This ends up simplifying the
code a bit, too.
+ dev_err(priv-pci_dev-dev, controller is wedged\n);
If this happens, it will presumably happen all the time and the message
will
pollute the log. Is the message really necessary ?
I'd rather log something to help diagnose. Would rate-limiting it be
okay?
It would still pollute the log because it doesn't happen that often.
A message once a second still fills the log.
If it is for diagnose/debugging, why not dev_dbg ?
OK. I demoted some other log lines, too.
+ goto out_release;
+ }
+
+ /*
+* Be paranoid: try to detect races. This will only detect
races
+* against BIOS, not against hardware. (I've never seen this
happen.)
+*/
+ pci_read_config_dword(priv-pci_dev, SMBCMD(chan), final_cmd);
+ pci_read_config_dword(priv-pci_dev, SMBCNTL(chan),
final_cntl);
+ if (((cmd ^ final_cmd) SMBCMD_OUR_BITS) ||
+ ((cntl ^ final_cntl) SMBCNTL_OUR_BITS)) {
+ WARN(1, iMC SMBUS raced against firmware);
+ dev_emerg(priv-pci_dev-dev,
Is a stack trace and dev_emerg really warranted here ?
If this happens, something's very wrong and the user should stop using
the driver. We could potentially write the wrong address, and, if we
manage to screw up thermal management, we could potentially corrupt
data for to an inappropriate refresh interval.
IOW, I want to hear about it if this happens.
Ok, that explains the WARN. Still not an emergency, though.
It's dev_err now.
Coding style suggests
if (!(stat SMBSTAT_RDO)) {
dev_err();
ret - -EIO;
goto out_release;
}
and
if (!(stat SMBSTAT_WOD)) {
dev_err();
ret = -EIO;
goto out_release;
}
Done. dev_dbg, too -- I think that either all of these errors should
be dev_dbg or none should be.
On a side note, I am a bit confused about the note same bug as in the read
case.
Do you want to say that RDO is sometimes/often set in the write case ?
If so, it might make more sense to just say it.
Yes, fixed.
[ ... ]
+static void imc_free_channel(struct imc_priv *priv, int i)
+{
+ struct imc_channel *ch = priv-channels[i];
+
+ /* This can recurse into imc_smbus_xfer. */
So ?
It needs to happen before mutex_destroy. I improved the comment.
Seems to me obvious that a mutex would be destroyed last in cleanup.
It wasn't to me. I'll remove the comment.
I want to be ready for future hardware that might support more than
two channels.
Not my call to make, but I am a bit wary of future hardware support which
may
never materialize. I prefer writing code liks this for the current use case.
The time to optimize the code for the future hardware is if and when the
future
hardware materializes.
In general, I am also in favor of the guidance in the coding style document,
which suggests to have a single error exit and handle any necessary cleanup
there.
In this
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Sun, Mar 8, 2015 at 12:50 PM, Guenter Roeck li...@roeck-us.net wrote: On 03/08/2015 12:30 PM, Andy Lutomirski wrote: [ ... ] One other question: from my reading of the spec, it should be possible to augment this driver to expose a temporate sensor subdevice that shows recent cached temperatures from HW DIMM measurements. They would be redundant with the jc42 outputs, but it would be safe to use them even on systems without safe SMBUS arbitration. Should I do that as a followup later on? Without thinking too much about it, this should be a separate driver, and I think it might actually be more valuable (since less risky) than this entire patch set. The main problem there is that they'll fight over the PCI ids. That is why we have mfd drivers. While that most likely won't be a solution here (and I don't suggest it), you can use the same idea. If can not be instantiated as pci driver, the alternative would be to instantiate it as platform driver and let it get its pci information from the parent device. Hmm. It could make sense to have an Intel chipset driver that instantiates sub-devices for thermal monitoring, smbus, and EDAC. It might be a bit of a cleanup for sb-edac, too. Anyway, this isn't currently necessary. Let's deal with it when we get there. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On Mar 7, 2015 6:39 AM, Guenter Roeck li...@roeck-us.net wrote: On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Sandy Bridge Xeon and Extreme chips have integrated memory controllers with (rather limited) onboard SMBUS masters. This driver gives access to the bus. There are various groups working on standardizing a way to arbitrate access to the bus between the OS, SMM firmware, a BMC, hardware thermal control, etc. In the mean time, running this driver is unsafe except under special circumstances. Nonetheless, this driver has real users. As a compromise, the driver will refuse to load unless i2c_imc.allow_unsafe_access=Y. When safe access becomes available, we can leave this option as a way for legacy users to run the driver, and we'll allow the driver to load by default if safe bus access is available. Signed-off-by: Andy Lutomirski l...@amacapital.net --- Please consider running your patch through checkpatch --strict, or at least checkpatch. [ I won't comment on the checkpatch problems below ] drivers/i2c/busses/Kconfig | 18 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-imc.c | 583 +++ 3 files changed, 602 insertions(+) create mode 100644 drivers/i2c/busses/i2c-imc.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index ab838d9e28b6..d6b9ce164fbf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate Intel iMC (LGA 2011) SMBus Controller + depends on PCI X86 + select I2C_DIMM_BUS + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC) depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f658d2f..4287c891e782 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o obj-$(CONFIG_I2C_I801)+= i2c-i801.o obj-$(CONFIG_I2C_ISCH)+= i2c-isch.o obj-$(CONFIG_I2C_ISMT)+= i2c-ismt.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c new file mode 100644 index ..2dbf171114c6 --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME : fmt + +#include linux/module.h +#include linux/kernel.h +#include linux/delay.h +#include linux/pci.h +#include linux/i2c.h + +/* + * The datasheet can be found here, for example: + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf + * + * There seem to be quite a few bugs or spec errors, though: + * + * - A successful transaction sets WOD and RDO. + * + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). + * + * - Erratum BT109, which says: + * + * The processor may not complete SMBus (System Management Bus) + * transactions targeting the TSOD (Temperature Sensor On DIMM) + * when Package C-States are enabled. Due to this
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/08/2015 07:03 AM, Andy Lutomirski wrote:
On Mar 7, 2015 6:39 AM, Guenter Roeck li...@roeck-us.net wrote:
On 03/06/2015 06:50 PM, Andy Lutomirski wrote:
Sandy Bridge Xeon and Extreme chips have integrated memory
controllers with (rather limited) onboard SMBUS masters. This
driver gives access to the bus.
There are various groups working on standardizing a way to arbitrate
access to the bus between the OS, SMM firmware, a BMC, hardware
thermal control, etc. In the mean time, running this driver is
unsafe except under special circumstances. Nonetheless, this driver
has real users.
As a compromise, the driver will refuse to load unless
i2c_imc.allow_unsafe_access=Y. When safe access becomes available,
we can leave this option as a way for legacy users to run the
driver, and we'll allow the driver to load by default if safe bus
access is available.
Signed-off-by: Andy Lutomirski l...@amacapital.net
---
[ ... ]
+
+ if (imc_wait_not_busy(priv, chan, stat) != 0) {
+ /* Timeout. TODO: Reset the controller? */
+ ret = -EIO;
timeout - -ETIMEDOUT ?
OK
Actually, I just realized that imc_wait_not_busy returns a valid error code.
Given that, some static analysis checkers (and now me) will ask you
why you don't just use the error code from imc_wait_not_busy.
This applies to other calls to the same function as well.
+ dev_err(priv-pci_dev-dev, controller is wedged\n);
If this happens, it will presumably happen all the time and the message will
pollute the log. Is the message really necessary ?
I'd rather log something to help diagnose. Would rate-limiting it be okay?
It would still pollute the log because it doesn't happen that often.
A message once a second still fills the log.
If it is for diagnose/debugging, why not dev_dbg ?
+ goto out_release;
+ }
+
+ /*
+* Be paranoid: try to detect races. This will only detect races
+* against BIOS, not against hardware. (I've never seen this happen.)
+*/
+ pci_read_config_dword(priv-pci_dev, SMBCMD(chan), final_cmd);
+ pci_read_config_dword(priv-pci_dev, SMBCNTL(chan), final_cntl);
+ if (((cmd ^ final_cmd) SMBCMD_OUR_BITS) ||
+ ((cntl ^ final_cntl) SMBCNTL_OUR_BITS)) {
+ WARN(1, iMC SMBUS raced against firmware);
+ dev_emerg(priv-pci_dev-dev,
Is a stack trace and dev_emerg really warranted here ?
If this happens, something's very wrong and the user should stop using
the driver. We could potentially write the wrong address, and, if we
manage to screw up thermal management, we could potentially corrupt
data for to an inappropriate refresh interval.
IOW, I want to hear about it if this happens.
Ok, that explains the WARN. Still not an emergency, though.
+ Access to channel %d raced: cmd 0x%08X-0x%08X, cntl
0x%08X-0x%08X\n,
+ chan, cmd, final_cmd, cntl, final_cntl);
+ atomic_set(imc_raced, 1);
+ ret = -EIO;
+ goto out_release;
+ }
+
+ if (stat SMBSTAT_SBE) {
+ /*
+* Clear the error to re-enable TSOD polling. The docs say
+* that, as long as SBE is set, TSOD polling won't happen.
+* The docs also say that writing zero to this bit (which is
+* the only writable bit in the whole register) will clear
+* the error. Empirically, writing 0 does not clear SBE, but
+* it's probably still good to do the write in compliance with
+* the spec. (TSOD polling still happens and seems to
+* clear SBE on its own.)
+*/
+ pci_write_config_dword(priv-pci_dev, SMBSTAT(chan), 0);
+ ret = -ENXIO;
+ goto out_release;
+ }
+
+ if (read_write == I2C_SMBUS_READ) {
+ if (stat SMBSTAT_RDO) {
+ /*
+* The iMC SMBUS controller thinks of SMBUS
+* words as being big-endian (MSB first).
+* Linux treats them as little-endian, so we need
+* to swap them.
+*
+* Note: the controller will often (always?) set
+* WOD here. This is probably a bug.
+*/
+ if (size == I2C_SMBUS_WORD_DATA)
+ data-word = swab16(stat SMBSTAT_RDATA_MASK);
+ else
+ data-byte = stat 0xFF;
+ ret = 0;
ret is already guaranteed to be 0 here.
+ } else {
+ dev_err(priv-pci_dev-dev,
+ Unexpected read status 0x%08X\n, stat);
+ ret = -EIO;
+ }
+ } else {
+
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Sandy Bridge Xeon and Extreme chips have integrated memory controllers with (rather limited) onboard SMBUS masters. This driver gives access to the bus. There are various groups working on standardizing a way to arbitrate access to the bus between the OS, SMM firmware, a BMC, hardware thermal control, etc. In the mean time, running this driver is unsafe except under special circumstances. Nonetheless, this driver has real users. As a compromise, the driver will refuse to load unless i2c_imc.allow_unsafe_access=Y. When safe access becomes available, we can leave this option as a way for legacy users to run the driver, and we'll allow the driver to load by default if safe bus access is available. Signed-off-by: Andy Lutomirski --- Please consider running your patch through checkpatch --strict, or at least checkpatch. [ I won't comment on the checkpatch problems below ] drivers/i2c/busses/Kconfig | 18 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-imc.c | 583 +++ 3 files changed, 602 insertions(+) create mode 100644 drivers/i2c/busses/i2c-imc.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index ab838d9e28b6..d6b9ce164fbf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate "Intel iMC (LGA 2011) SMBus Controller" + depends on PCI && X86 + select I2C_DIMM_BUS + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f658d2f..4287c891e782 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o obj-$(CONFIG_I2C_I801)+= i2c-i801.o obj-$(CONFIG_I2C_ISCH)+= i2c-isch.o obj-$(CONFIG_I2C_ISMT)+= i2c-ismt.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c new file mode 100644 index ..2dbf171114c6 --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include +#include +#include +#include + +/* + * The datasheet can be found here, for example: + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf + * + * There seem to be quite a few bugs or spec errors, though: + * + * - A successful transaction sets WOD and RDO. + * + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). + * + * - Erratum BT109, which says: + * + * The processor may not complete SMBus (System Management Bus) + * transactions targeting the TSOD (Temperature Sensor On DIMM) + * when Package C-States are enabled. Due to this erratum, if the + * processor transitions into a Package C-State while an SMBus + * transaction with the TSOD is in process, the processor will + * suspend receipt of the transaction. The transaction completes + * while the processor is in a Package C-State.
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
Just two nits.
Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]:
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -149,6 +149,24 @@ config I2C_ISMT
> This driver can also be built as a module. If so, the module will be
> called i2c-ismt.
>
> +config I2C_IMC
> + tristate "Intel iMC (LGA 2011) SMBus Controller"
> + depends on PCI && X86
> + select I2C_DIMM_BUS
The pedant in me can't help but notice that I2C_DIMM_BUS itself is added
in patch 2/2. And so is the call of i2c_scan_dimm_bus() in i2c-imc.c
that apparently requires this select. So that select isn't really needed
in this patch but in 2/2.
> + help
> + If you say yes to this option, support will be included for the Intel
> + Integrated Memory Controller SMBus host controller interface. This
> + controller is found on LGA 2011 Xeons and Core i7 Extremes.
> +
> + There are currently no systems on which the kernel knows that it can
> + safely enable this driver. For now, you need to pass this driver a
> + scary module parameter, and you should only pass that parameter if you
> + have a special motherboard and know exactly what you are doing.
> + Special motherboards include the Supermicro X9DRH-iF-NV.
> +
> + This driver can also be built as a module. If so, the module will be
> + called i2c-imc.
> +
> config I2C_PIIX4
> tristate "Intel PIIX4 and compatible
> (ATI/AMD/Serverworks/Broadcom/SMSC)"
> depends on PCI
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-imc.c
> @@ -0,0 +1,583 @@
> +/*
> + * Copyright (c) 2013 Andrew Lutomirski
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
So you probably want to use
MODULE_LICENSE("GPL v2");
here.
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
On 03/06/2015 06:50 PM, Andy Lutomirski wrote: Sandy Bridge Xeon and Extreme chips have integrated memory controllers with (rather limited) onboard SMBUS masters. This driver gives access to the bus. There are various groups working on standardizing a way to arbitrate access to the bus between the OS, SMM firmware, a BMC, hardware thermal control, etc. In the mean time, running this driver is unsafe except under special circumstances. Nonetheless, this driver has real users. As a compromise, the driver will refuse to load unless i2c_imc.allow_unsafe_access=Y. When safe access becomes available, we can leave this option as a way for legacy users to run the driver, and we'll allow the driver to load by default if safe bus access is available. Signed-off-by: Andy Lutomirski l...@amacapital.net --- Please consider running your patch through checkpatch --strict, or at least checkpatch. [ I won't comment on the checkpatch problems below ] drivers/i2c/busses/Kconfig | 18 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-imc.c | 583 +++ 3 files changed, 602 insertions(+) create mode 100644 drivers/i2c/busses/i2c-imc.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index ab838d9e28b6..d6b9ce164fbf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate Intel iMC (LGA 2011) SMBus Controller + depends on PCI X86 + select I2C_DIMM_BUS + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC) depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f658d2f..4287c891e782 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o obj-$(CONFIG_I2C_I801)+= i2c-i801.o obj-$(CONFIG_I2C_ISCH)+= i2c-isch.o obj-$(CONFIG_I2C_ISMT)+= i2c-ismt.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c new file mode 100644 index ..2dbf171114c6 --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME : fmt + +#include linux/module.h +#include linux/kernel.h +#include linux/delay.h +#include linux/pci.h +#include linux/i2c.h + +/* + * The datasheet can be found here, for example: + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf + * + * There seem to be quite a few bugs or spec errors, though: + * + * - A successful transaction sets WOD and RDO. + * + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). + * + * - Erratum BT109, which says: + * + * The processor may not complete SMBus (System Management Bus) + * transactions targeting the TSOD (Temperature Sensor On DIMM) + * when Package C-States are enabled. Due to this erratum, if the + * processor transitions into a Package C-State while an SMBus + * transaction with the TSOD is in process, the processor will + * suspend receipt of the
Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
Just two nits. Andy Lutomirski schreef op vr 06-03-2015 om 18:50 [-0800]: --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate Intel iMC (LGA 2011) SMBus Controller + depends on PCI X86 + select I2C_DIMM_BUS The pedant in me can't help but notice that I2C_DIMM_BUS itself is added in patch 2/2. And so is the call of i2c_scan_dimm_bus() in i2c-imc.c that apparently requires this select. So that select isn't really needed in this patch but in 2/2. + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC) depends on PCI --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ This states the license is GPL v2. +MODULE_LICENSE(GPL); So you probably want to use MODULE_LICENSE(GPL v2); here. Paul Bolle -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
Sandy Bridge Xeon and Extreme chips have integrated memory controllers with (rather limited) onboard SMBUS masters. This driver gives access to the bus. There are various groups working on standardizing a way to arbitrate access to the bus between the OS, SMM firmware, a BMC, hardware thermal control, etc. In the mean time, running this driver is unsafe except under special circumstances. Nonetheless, this driver has real users. As a compromise, the driver will refuse to load unless i2c_imc.allow_unsafe_access=Y. When safe access becomes available, we can leave this option as a way for legacy users to run the driver, and we'll allow the driver to load by default if safe bus access is available. Signed-off-by: Andy Lutomirski --- drivers/i2c/busses/Kconfig | 18 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-imc.c | 583 +++ 3 files changed, 602 insertions(+) create mode 100644 drivers/i2c/busses/i2c-imc.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index ab838d9e28b6..d6b9ce164fbf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate "Intel iMC (LGA 2011) SMBus Controller" + depends on PCI && X86 + select I2C_DIMM_BUS + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f658d2f..4287c891e782 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o obj-$(CONFIG_I2C_I801) += i2c-i801.o obj-$(CONFIG_I2C_ISCH) += i2c-isch.o obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o obj-$(CONFIG_I2C_NFORCE2_S4985)+= i2c-nforce2-s4985.o obj-$(CONFIG_I2C_PIIX4)+= i2c-piix4.o diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c new file mode 100644 index ..2dbf171114c6 --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include +#include +#include +#include + +/* + * The datasheet can be found here, for example: + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf + * + * There seem to be quite a few bugs or spec errors, though: + * + * - A successful transaction sets WOD and RDO. + * + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). + * + * - Erratum BT109, which says: + * + * The processor may not complete SMBus (System Management Bus) + * transactions targeting the TSOD (Temperature Sensor On DIMM) + * when Package C-States are enabled. Due to this erratum, if the + * processor transitions into a Package C-State while an SMBus + * transaction with the TSOD is in process, the processor will + * suspend receipt of the transaction. The transaction completes + * while the processor is in a Package C-State. Upon exiting + * Package C-State, the processor will attempt to resume the + * SMBus transaction, detect a protocol violation, and log an + * error. + * + * The description notwithstanding, I've seen
[PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips
Sandy Bridge Xeon and Extreme chips have integrated memory controllers with (rather limited) onboard SMBUS masters. This driver gives access to the bus. There are various groups working on standardizing a way to arbitrate access to the bus between the OS, SMM firmware, a BMC, hardware thermal control, etc. In the mean time, running this driver is unsafe except under special circumstances. Nonetheless, this driver has real users. As a compromise, the driver will refuse to load unless i2c_imc.allow_unsafe_access=Y. When safe access becomes available, we can leave this option as a way for legacy users to run the driver, and we'll allow the driver to load by default if safe bus access is available. Signed-off-by: Andy Lutomirski l...@amacapital.net --- drivers/i2c/busses/Kconfig | 18 ++ drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-imc.c | 583 +++ 3 files changed, 602 insertions(+) create mode 100644 drivers/i2c/busses/i2c-imc.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index ab838d9e28b6..d6b9ce164fbf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -149,6 +149,24 @@ config I2C_ISMT This driver can also be built as a module. If so, the module will be called i2c-ismt. +config I2C_IMC + tristate Intel iMC (LGA 2011) SMBus Controller + depends on PCI X86 + select I2C_DIMM_BUS + help + If you say yes to this option, support will be included for the Intel + Integrated Memory Controller SMBus host controller interface. This + controller is found on LGA 2011 Xeons and Core i7 Extremes. + + There are currently no systems on which the kernel knows that it can + safely enable this driver. For now, you need to pass this driver a + scary module parameter, and you should only pass that parameter if you + have a special motherboard and know exactly what you are doing. + Special motherboards include the Supermicro X9DRH-iF-NV. + + This driver can also be built as a module. If so, the module will be + called i2c-imc. + config I2C_PIIX4 tristate Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC) depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 56388f658d2f..4287c891e782 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o obj-$(CONFIG_I2C_I801) += i2c-i801.o obj-$(CONFIG_I2C_ISCH) += i2c-isch.o obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o +obj-$(CONFIG_I2C_IMC) += i2c-imc.o obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o obj-$(CONFIG_I2C_NFORCE2_S4985)+= i2c-nforce2-s4985.o obj-$(CONFIG_I2C_PIIX4)+= i2c-piix4.o diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c new file mode 100644 index ..2dbf171114c6 --- /dev/null +++ b/drivers/i2c/busses/i2c-imc.c @@ -0,0 +1,583 @@ +/* + * Copyright (c) 2013 Andrew Lutomirski l...@amacapital.net + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME : fmt + +#include linux/module.h +#include linux/kernel.h +#include linux/delay.h +#include linux/pci.h +#include linux/i2c.h + +/* + * The datasheet can be found here, for example: + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf + * + * There seem to be quite a few bugs or spec errors, though: + * + * - A successful transaction sets WOD and RDO. + * + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). + * + * - Erratum BT109, which says: + * + * The processor may not complete SMBus (System Management Bus) + * transactions targeting the TSOD (Temperature Sensor On DIMM) + * when Package C-States are enabled. Due to this erratum, if the + * processor transitions into a Package C-State while an SMBus + * transaction with the TSOD is in process, the processor will + * suspend receipt of the transaction. The transaction completes + * while the processor is in a Package C-State. Upon exiting + * Package C-State, the processor will attempt to resume the + * SMBus transaction, detect a protocol
