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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux