Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
On Wed, 24 Jul 2019 10:37:48 +0200, Jean Delvare wrote: > Hi Andrew, > > Sorry for the delay... What can I say :( Unfortunately by now Andrew is gone. So I will be the one rebasing and resubmitting this series. -- Jean Delvare SUSE L3 Support
Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
Hi Andrew,
Sorry for the delay... What can I say :(
On Mon, 26 Feb 2018 10:28:43 +1000, Andrew Cooks wrote:
> Family 16h Model 30h SMBus controller needs the same port selection fix
> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
> selection for AMD Family 17h chips")
>
> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
> register") also fixed the port selection for Hudson2, but unfortunately
> this is not the exact same device and the AMD naming and PCI Device IDs
> aren't particularly helpful here.
>
> The SMBus port selection register is common to the following Families
> and models, as documented in AMD's publicly available BIOS and Kernel
> Developer Guides:
>
> 50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
>
> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
> between Bolton FCH and Family 16h Model 30h, but the location of the
> SmBus0Sel port selection bits are different:
>
> 51192 - Bolton Register Reference Guide
>
> We distinguish between Bolton and Family 16h Model 30h using the PCI
> Revision ID:
>
> Bolton is device 0x780b, revision 0x15
> Family 16h Model 30h is device 0x780b, revision 0x1F
> Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
>
> The following additional public AMD BKDG documents were checked and do
> not share the same port selection register:
>
> 42301 - Family 15h Model 00h-0Fh doesn't mention any
> 42300 - Family 15h Model 10h-1Fh doesn't mention any
> 49125 - Family 15h Model 30h-3Fh doesn't mention any
>
> 48751 - Family 16h Model 00h-0Fh uses the previously supported
> index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
>
> Signed-off-by: Andrew Cooks
> ---
> drivers/i2c/busses/i2c-piix4.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
> (...)
Looks good to me. Unfortunately the patch no longer applies (my fault
obviously), it needs to be rebased on top of commit
24beb83ad289c68bce7c01351cb90465bbb1940a ("i2c-piix4: Add Hygon Dhyana
SMBus support").
I also agree with Tobin's suggestion to remove unneeded parentheses.
Reviewed-by: Jean Delvare
This patch should also address Will Wagner's (Cc'd) complaint in another
thread ("[BUG] i2c_piix4: Hudson2 uses wrong port to access SMBus
controller").
I believe this is stable branch material.
--
Jean Delvare
SUSE L3 Support
Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
Hi Tobin
On 26/02/18 18:58, Tobin C. Harding wrote:
> On Mon, Feb 26, 2018 at 10:28:43AM +1000, Andrew Cooks wrote:
>> Family 16h Model 30h SMBus controller needs the same port selection fix
>> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
>> selection for AMD Family 17h chips")
>>
>> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
>> register") also fixed the port selection for Hudson2, but unfortunately
>> this is not the exact same device and the AMD naming and PCI Device IDs
>> aren't particularly helpful here.
>>
>> The SMBus port selection register is common to the following Families
>> and models, as documented in AMD's publicly available BIOS and Kernel
>> Developer Guides:
>>
>> 50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>> 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>> 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
>>
>> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
>> between Bolton FCH and Family 16h Model 30h, but the location of the
>> SmBus0Sel port selection bits are different:
>>
>> 51192 - Bolton Register Reference Guide
>>
>> We distinguish between Bolton and Family 16h Model 30h using the PCI
>> Revision ID:
>>
>> Bolton is device 0x780b, revision 0x15
>> Family 16h Model 30h is device 0x780b, revision 0x1F
>> Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
>>
>> The following additional public AMD BKDG documents were checked and do
>> not share the same port selection register:
>>
>> 42301 - Family 15h Model 00h-0Fh doesn't mention any
>> 42300 - Family 15h Model 10h-1Fh doesn't mention any
>> 49125 - Family 15h Model 30h-3Fh doesn't mention any
>>
>> 48751 - Family 16h Model 00h-0Fh uses the previously supported
>> index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
>>
>> Signed-off-by: Andrew Cooks
>> ---
>> drivers/i2c/busses/i2c-piix4.c | 12 +---
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 174579d..5c90a44 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -99,7 +99,7 @@
>> #define SB800_PIIX4_PORT_IDX_MASK 0x06
>> #define SB800_PIIX4_PORT_IDX_SHIFT 1
>>
>> -/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
>> +/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
>> #define SB800_PIIX4_PORT_IDX_KERNCZ 0x02
>> #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ0x18
>> #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ 3
>> @@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>>
>> /* Find which register is used for port selection */
>> if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
>> -switch (PIIX4_dev->device) {
>> -case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
>> +if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
>
> nit: if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
>
>> +(PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
>> + PIIX4_dev->revision >= 0x1F)) {
>
>
> Hope this helps,
It does, thanks. I'll roll this fix in with whatever other feedback I get
before sending the next patch version.
a.
Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h
On Mon, Feb 26, 2018 at 10:28:43AM +1000, Andrew Cooks wrote:
> Family 16h Model 30h SMBus controller needs the same port selection fix
> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
> selection for AMD Family 17h chips")
>
> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
> register") also fixed the port selection for Hudson2, but unfortunately
> this is not the exact same device and the AMD naming and PCI Device IDs
> aren't particularly helpful here.
>
> The SMBus port selection register is common to the following Families
> and models, as documented in AMD's publicly available BIOS and Kernel
> Developer Guides:
>
> 50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
> 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
>
> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
> between Bolton FCH and Family 16h Model 30h, but the location of the
> SmBus0Sel port selection bits are different:
>
> 51192 - Bolton Register Reference Guide
>
> We distinguish between Bolton and Family 16h Model 30h using the PCI
> Revision ID:
>
> Bolton is device 0x780b, revision 0x15
> Family 16h Model 30h is device 0x780b, revision 0x1F
> Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
>
> The following additional public AMD BKDG documents were checked and do
> not share the same port selection register:
>
> 42301 - Family 15h Model 00h-0Fh doesn't mention any
> 42300 - Family 15h Model 10h-1Fh doesn't mention any
> 49125 - Family 15h Model 30h-3Fh doesn't mention any
>
> 48751 - Family 16h Model 00h-0Fh uses the previously supported
> index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
>
> Signed-off-by: Andrew Cooks
> ---
> drivers/i2c/busses/i2c-piix4.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 174579d..5c90a44 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -99,7 +99,7 @@
> #define SB800_PIIX4_PORT_IDX_MASK0x06
> #define SB800_PIIX4_PORT_IDX_SHIFT 1
>
> -/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
> +/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
> #define SB800_PIIX4_PORT_IDX_KERNCZ 0x02
> #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ 0x18
> #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ3
> @@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>
> /* Find which register is used for port selection */
> if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
> - switch (PIIX4_dev->device) {
> - case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
> + if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
nit:if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
> + (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
> +PIIX4_dev->revision >= 0x1F)) {
Hope this helps,
Tobin.
