Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller

From: Andrew Armenia
Date: Fri Jun 15 2012 - 09:44:22 EST

Next message: Marcelo Tosatti: "Re: WARNING: at arch/x86/kernel/kvmclock.c:127"
Previous message: Srinidhi Kasagar: "Re: Fwd: [PATCH 1/3] i2c: Add Device Tree support to the NomadikI2C driver"
In reply to: Jean Delvare: "Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller"
Next in thread: Andrew Armenia: "[PATCH 2/3] i2c-piix4: separate registration and probing code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jun 15, 2012 at 4:31 AM, Jean Delvare <khali@xxxxxxxxxxxx> wrote:
> Hi Andrew,
>
> For next time... When you post or repost a patch series, please always
> start a new discussion thread (don't use Reply), otherwise discussion
> threading becomes very hard to follow, both in e-mail clients and
> online list archives.
>
> On Wed, 13 Jun 2012 12:59:09 -0400, Andrew Armenia wrote:
>> Some AMD chipsets, such as the SP5100, have an auxiliary SMBus
>> controller with a second set of registers. This patch adds
>> support for this auxiliary controller.
>>
>> Tested on ASUS KCMA-D8 motherboard.
>>
>> Signed-off-by: Andrew Armenia <andrew@xxxxxxxxxxxxxxxx>
>> ---
>> Documentation/i2c/busses/i2c-piix4 | 13 +++++--
>> drivers/i2c/busses/Kconfig | 6 +++-
>> drivers/i2c/busses/i2c-piix4.c | 69 ++++++++++++++++++++++++++++++++++--
>> 3 files changed, 82 insertions(+), 6 deletions(-)
>
> A few issues remaining in this patch:
>
>> diff --git a/Documentation/i2c/busses/i2c-piix4 b/Documentation/i2c/busses/i2c-piix4
>> index 475bb4a..474779e 100644
>> --- a/Documentation/i2c/busses/i2c-piix4
>> +++ b/Documentation/i2c/busses/i2c-piix4
>> @@ -8,12 +8,17 @@ Supported adapters:
>> Datasheet: Only available via NDA from ServerWorks
>> * ATI IXP200, IXP300, IXP400, SB600, SB700 and SB800 southbridges
>> Datasheet: Not publicly available
>> + SB700 register reference available at:
>> + http://support.amd.com/us/Embedded_TechDocs/43009_sb7xx_rrg_pub_1.00.pdf
>> + * AMD SP5100 (SB700 derivative found on some server mainboards)
>> + Datasheet: Publicly available at the AMD website
>> + http://support.amd.com/us/Embedded_TechDocs/44413.pdf
>> * AMD Hudson-2
>> Datasheet: Not publicly available
>> * Standard Microsystems (SMSC) SLC90E66 (Victory66) southbridge
>> Datasheet: Publicly available at the SMSC website http://www.smsc.com
>>
>> -Authors:
>> +Authors:
>
> Never include white-space changes in a non-cleanup patch, they make
> review and backporting harder.
>
>> Frodo Looijaard <frodol@xxxxxx>
>> Philip Edelbrock <phil@xxxxxxxxxxxxx>
>>
>> @@ -32,7 +37,7 @@ Description
>>
>> The PIIX4 (properly known as the 82371AB) is an Intel chip with a lot of
>> functionality. Among other things, it implements the PCI bus. One of its
>> -minor functions is implementing a System Management Bus. This is a true
>> +minor functions is implementing a System Management Bus. This is a true
>> SMBus - you can not access it on I2C levels. The good news is that it
>> natively understands SMBus commands and you do not have to worry about
>> timing problems. The bad news is that non-SMBus devices connected to it can
>> @@ -68,6 +73,10 @@ this driver on those mainboards.
>> The ServerWorks Southbridges, the Intel 440MX, and the Victory66 are
>> identical to the PIIX4 in I2C/SMBus support.
>>
>> +The AMD SB700 and SP5100 chipsets implement two PIIX4-compatible SMBus
>> +controllers. If your BIOS initializes the secondary controller, it will
>> +be detected by this driver as an "Auxiliary SMBus Host Controller".
>> +
>> If you own Force CPCI735 motherboard or other OSB4 based systems you may need
>> to change the SMBus Interrupt Select register so the SMBus controller uses
>> the SMI mode.
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 7244c8b..2e7530a 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -133,7 +133,7 @@ config I2C_PIIX4
>> ATI IXP300
>> ATI IXP400
>> ATI SB600
>> - ATI SB700
>> + ATI SB700/SP5100
>> ATI SB800
>> AMD Hudson-2
>> Serverworks OSB4
>> @@ -143,6 +143,10 @@ config I2C_PIIX4
>> Serverworks HT-1100
>> SMSC Victory66
>>
>> + Some AMD chipsets contain two PIIX4-compatible SMBus
>> + controllers. This driver will attempt to use both controllers
>> + on the SB700/SP5100, if they have been initialized by the BIOS.
>> +
>> This driver can also be built as a module. If so, the module
>> will be called i2c-piix4.
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 8181963..85d3db9 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -21,11 +21,12 @@
>> Supports:
>> Intel PIIX4, 440MX
>> Serverworks OSB4, CSB5, CSB6, HT-1000, HT-1100
>> - ATI IXP200, IXP300, IXP400, SB600, SB700, SB800
>> + ATI IXP200, IXP300, IXP400, SB600, SB700/SP5100, SB800
>> AMD Hudson-2
>> SMSC Victory66
>>
>> - Note: we assume there can only be one device, with one SMBus interface.
>> + Note: we assume there can only be one device, with one or more
>> + SMBus interfaces.
>> */
>>
>> #include <linux/module.h>
>> @@ -60,6 +61,7 @@
>> #define SMBIOSIZE 8
>>
>> /* PCI Address Constants */
>> +#define SMBAUXBA 0x058
>
> AFAICS this is SB700-specific, so this should either be clarified in
> the name, or maybe we don't even want a name. After all, register
> offset is hard-coded in piix4_setup_sb800.
>
>> #define SMBBA 0x090
>> #define SMBHSTCFG 0x0D2
>> #define SMBSLVC 0x0D3
>> @@ -293,6 +295,43 @@ static int __devinit piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>> return piix4_smba;
>> }
>>
>> +static int __devinit piix4_setup_aux(struct pci_dev *PIIX4_dev,
>> + const struct pci_device_id *id,
>> + unsigned short base_reg_addr)
>> +{
>> + /* Set up auxiliary SMBus controllers found on some
>> + * AMD chipsets e.g. SP5100 (SB700 derivative) */
>> +
>> + unsigned short piix4_smba;
>> +
>> + /* Read address of auxiliary SMBus controller */
>> + pci_read_config_word(PIIX4_dev, base_reg_addr, &piix4_smba);
>> + piix4_smba &= 0xffe0;
>
> You must check bit 0 first. You want the I/O base to be set AND
> decoding thereof enabled, otherwise the controller can't be used.
>
> Also, mask 0xffe0 is OK for the SB700 but not for the SB600 which only
> has bits 3:1 marked as reserved. Given that reserved bits are supposed
> to be 0 anyway, I think it is OK to mask with 0xfff0 always.
>
>> +
>> + if (piix4_smba == 0) {
>> + dev_dbg(&PIIX4_dev->dev,
>> + "Auxiliary SMBus base address uninitialized");
>
> Missing trailing new line.
>
>> +
>
> Needless blank line.
>
>> + return -ENODEV;
>> + }
>> +
>
>> + if (acpi_check_region(piix4_smba, SMBIOSIZE, piix4_driver.name))
>> + return -ENODEV;
>> +
>> + if (!request_region(piix4_smba, SMBIOSIZE, piix4_driver.name)) {
>> + dev_err(&PIIX4_dev->dev, "Auxiliary SMBus region 0x%x "
>> + "already in use!\n", piix4_smba);
>> + return -EBUSY;
>> + }
>> +
>> + dev_info(&PIIX4_dev->dev,
>> + "Auxiliary SMBus Host Controller at 0x%x\n",
>> + piix4_smba
>> + );
>
> Undesirable line break.
>
>> +
>> + return piix4_smba;
>> +}
>> +
>> static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>> {
>> int temp;
>> @@ -504,6 +543,7 @@ static DEFINE_PCI_DEVICE_TABLE(piix4_ids) = {
>> MODULE_DEVICE_TABLE (pci, piix4_ids);
>>
>> static struct i2c_adapter *piix4_main_adapter;
>> +static struct i2c_adapter *piix4_aux_adapter;
>>
>> static int __devinit piix4_add_adapter(struct pci_dev *dev,
>> unsigned short smba,
>> @@ -565,10 +605,28 @@ static int __devinit piix4_probe(struct pci_dev *dev,
>> else
>> retval = piix4_setup(dev, id);
>>
>> + /* if no main SMBus found, give up */
>> if (retval < 0)
>> return retval;
>>
>> - return piix4_add_adapter(dev, retval, &piix4_main_adapter);
>> + /* try to register main SMBus adapter, give up if we can't */
>> + retval = piix4_add_adapter(dev, retval, &piix4_main_adapter);
>> + if (retval < 0)
>> + return retval;
>> +
>> + /* check for auxiliary SMBus on some AMD chipsets */
>> + if (dev->vendor == PCI_VENDOR_ID_ATI &&
>> + dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
>> + dev->revision < 0x40) {
>> + retval = piix4_setup_aux(dev, id, SMBAUXBA);
>> + if (retval > 0) {
>> + /* try to add the aux adapter if it exists,
>> + * piix4_add_adapter will clean up if this fails */
>> + piix4_add_adapter(dev, retval, &piix4_aux_adapter);
>> + }
>> + }
>> +
>> + return 0;
>> }
>>
>> static void __devinit piix4_adap_remove(struct i2c_adapter *adap)
>> @@ -590,6 +648,11 @@ static void __devexit piix4_remove(struct pci_dev *dev)
>> piix4_adap_remove(piix4_main_adapter);
>> piix4_main_adapter = NULL;
>> }
>> +
>> + if (piix4_aux_adapter) {
>> + piix4_adap_remove(piix4_aux_adapter);
>> + piix4_aux_adapter = NULL;
>> + }
>> }
>>
>> static struct pci_driver piix4_driver = {
>
> I've fixed them all, and applied your patch. The whole series with my
> changes incorporated is now visible at:
> http://khali.linux-fr.org/devel/linux-3/jdelvare-i2c/
>
> I would appreciate if you could test them and confirm I didn't break
> anything. Your patches will then be merged in kernel 3.6.
>
> Thanks,
> --
> Jean Delvare

I've just tested your updated patches and nothing seems broken. Thanks
for your patience with a first time submitter.

-Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Marcelo Tosatti: "Re: WARNING: at arch/x86/kernel/kvmclock.c:127"
Previous message: Srinidhi Kasagar: "Re: Fwd: [PATCH 1/3] i2c: Add Device Tree support to the NomadikI2C driver"
In reply to: Jean Delvare: "Re: [PATCH 3/3] i2c-piix4: support AMD auxiliary SMBus controller"
Next in thread: Andrew Armenia: "[PATCH 2/3] i2c-piix4: separate registration and probing code"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]