ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

PATCH Applied Adam Lackorzynski 2 participants 4 messages Mar 10, 2011 Sound

lore

Share & tools

lore

Adam Lackorzynski

author

2011-03-10 15:55 UTC #0 lore raw

PATCH 1/1 ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

Hi,

azx_init_pci() always writes PCI config register ICH6_PCIREG_TCSEL although this looks to be only defined on Intel systems and has a different meaning on AMD systems. On AMD systems the PCI interrupt pin control register is modified instead. Since the register has 'ICH' in its name I pulled the call to update_pci_byte() into the switch block to only happen on Intel variants. I'm not sure on the other variants.

Signed-off-by Adam Lackorzynski <[email protected]>

---
 sound/pci/hda/hda_intel.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

sound/pci/hda/hda_intel.c +8 -7

index fcedad9..41debbd 100644
@@ -1049,13 +1049,6 @@ static void azx_init_pci(struct azx *chip)
 {
 	unsigned short snoop;
 
-	/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
-	 * TCSEL == Traffic Class Select Register, which sets PCI express QOS
-	 * Ensuring these bits are 0 clears playback static on some HD Audio
-	 * codecs
-	 */
-	update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
-
 	switch (chip->driver_type) {
 	case AZX_DRIVER_ATI:
 		/* For ATI SB450 azalia HD audio, we need to enable snoop */

@@ -1087,6 +1080,14 @@ static void azx_init_pci(struct azx *chip)
 				(snoop & INTEL_SCH_HDA_DEVC_NOSNOOP)
 				? "Failed" : "OK");
 		}
+		/* fall through */
+	case AZX_DRIVER_ICH:
+		/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
+		 * TCSEL == Traffic Class Select Register, which sets PCI
+		 * express QOS Ensuring these bits are 0 clears playback
+		 * static on some HD Audio codecs
+		 */
+		update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
 		break;
 
         }

Signature · 7 lines

1.7.2.3


Adam
-- 
Adam                 [email protected]
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Takashi Iwai

2011-03-10 15:44 UTC #1 lore raw

At Thu, 10 Mar 2011 16:22:12 +0100,

Quoted 9 lines · Adam Lackorzynski

Hi,
azx_init_pci() always writes PCI config register ICH6_PCIREG_TCSEL although this looks to be only defined on Intel systems and has a different meaning on AMD systems. On AMD systems the PCI interrupt pin control register is modified instead. Since the register has 'ICH' in its name I pulled the call to update_pci_byte() into the switch block to only happen on Intel variants. I'm not sure on the other variants.

So, you did check only AMD systems, right?

Then it's safer to exclude this for AMD but apply to others for the time being, since we haven't got any relevant bug reports by this. Could you re-submit with that change?

The fact that it has ICH prefix doesn't mean much -- it's just like the driver is called snd-hda-intel. The first hardware, the first set. (At that time, we weren't sure at that time that HD-audio spec would be followed widely by most vendors.)

thanks,

Takashi

Quoted 48 lines

Signed-off-by Adam Lackorzynski <[email protected]>

---
 sound/pci/hda/hda_intel.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

sound/pci/hda/hda_intel.c +8 -7

index fcedad9..41debbd 100644
@@ -1049,13 +1049,6 @@ static void azx_init_pci(struct azx *chip)
 {
 	unsigned short snoop;
 
-	/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
-	 * TCSEL == Traffic Class Select Register, which sets PCI express QOS
-	 * Ensuring these bits are 0 clears playback static on some HD Audio
-	 * codecs
-	 */
-	update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
-
 	switch (chip->driver_type) {
 	case AZX_DRIVER_ATI:
 		/* For ATI SB450 azalia HD audio, we need to enable snoop */

@@ -1087,6 +1080,14 @@ static void azx_init_pci(struct azx *chip)
 				(snoop & INTEL_SCH_HDA_DEVC_NOSNOOP)
 				? "Failed" : "OK");
 		}
+		/* fall through */
+	case AZX_DRIVER_ICH:
+		/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
+		 * TCSEL == Traffic Class Select Register, which sets PCI
+		 * express QOS Ensuring these bits are 0 clears playback
+		 * static on some HD Audio codecs
+		 */
+		update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
 		break;
 
         }

Signature · 7 lines

1.7.2.3


Adam
-- 
Adam                 [email protected]
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Adam Lackorzynski

author

2011-03-10 16:42 UTC #2 lore raw

Quoted 11 lines · Takashi Iwai

At Thu, 10 Mar 2011 16:22:12 +0100,
Quoted 6 lines · Adam Lackorzynski
azx_init_pci() always writes PCI config register ICH6_PCIREG_TCSEL although this looks to be only defined on Intel systems and has a different meaning on AMD systems. On AMD systems the PCI interrupt pin control register is modified instead. Since the register has 'ICH' in its name I pulled the call to update_pci_byte() into the switch block to only happen on Intel variants. I'm not sure on the other variants.
So, you did check only AMD systems, right?

Quoted 8 lines

Then it's safer to exclude this for AMD but apply to others for the time being, since we haven't got any relevant bug reports by this. Could you re-submit with that change?
The fact that it has ICH prefix doesn't mean much -- it's just like the driver is called snd-hda-intel. The first hardware, the first set. (At that time, we weren't sure at that time that HD-audio spec would be followed widely by most vendors.)

I tried to find other chipset specifications and had no luck, so I don't know what other vendors have there. I think it's luck that those PCI config writes cause no harm.

Subject: [PATCH] ALSA: hda: Prevent writing ICH6_PCIREG_TCSEL on AMD systems.

Since the meaning of offset 0x44 in device specific configuration space is unknown for devices by other vendors, we only exclude AMD systems to retain the current behaviour.

Signed-off-by Adam Lackorzynski <[email protected]>

---
 sound/pci/hda/hda_intel.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

sound/pci/hda/hda_intel.c +5 -2

index fcedad9..70a9d32 100644
@@ -1052,9 +1052,12 @@ static void azx_init_pci(struct azx *chip)
 	/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
 	 * TCSEL == Traffic Class Select Register, which sets PCI express QOS
 	 * Ensuring these bits are 0 clears playback static on some HD Audio
-	 * codecs
+	 * codecs.
+	 * The PCI register TCSEL is defined in the Intel manuals.
 	 */
-	update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
+	if (chip->driver_type != AZX_DRIVER_ATI &&
+	    chip->driver_type != AZX_DRIVER_ATIHDMI)
+		update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
 
 	switch (chip->driver_type) {
 	case AZX_DRIVER_ATI:

Signature · 7 lines

1.7.2.3


Adam
-- 
Adam                 [email protected]
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Takashi Iwai

Applied

2011-03-10 16:46 UTC #3 lore raw

At Thu, 10 Mar 2011 17:41:56 +0100,

Quoted 18 lines · Adam Lackorzynski

Quoted 15 lines · Takashi Iwai
At Thu, 10 Mar 2011 16:22:12 +0100,
Quoted 6 lines · Adam Lackorzynski
azx_init_pci() always writes PCI config register ICH6_PCIREG_TCSEL although this looks to be only defined on Intel systems and has a different meaning on AMD systems. On AMD systems the PCI interrupt pin control register is modified instead. Since the register has 'ICH' in its name I pulled the call to update_pci_byte() into the switch block to only happen on Intel variants. I'm not sure on the other variants.
So, you did check only AMD systems, right?
Then it's safer to exclude this for AMD but apply to others for the time being, since we haven't got any relevant bug reports by this. Could you re-submit with that change?

Quoted 8 lines

Quoted 4 lines
The fact that it has ICH prefix doesn't mean much -- it's just like the driver is called snd-hda-intel. The first hardware, the first set. (At that time, we weren't sure at that time that HD-audio spec would be followed widely by most vendors.)
I tried to find other chipset specifications and had no luck, so I don't know what other vendors have there. I think it's luck that those PCI config writes cause no harm.

Or they follow also this behavior to be compatible :) Who knows.

Quoted 7 lines

Subject: [PATCH] ALSA: hda: Prevent writing ICH6_PCIREG_TCSEL on AMD systems.
azx_init_pci() always writes PCI config register ICH6_PCIREG_TCSEL although this looks to be only defined on Intel systems and has a different meaning on AMD systems. On AMD systems the PCI interrupt pin control register is modified instead.

Quoted 5 lines

Since the meaning of offset 0x44 in device specific configuration space is unknown for devices by other vendors, we only exclude AMD systems to retain the current behaviour.
Signed-off-by Adam Lackorzynski <[email protected]>

Thanks, applied now.

Takashi

Quoted 32 lines

---
 sound/pci/hda/hda_intel.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

sound/pci/hda/hda_intel.c +5 -2

index fcedad9..70a9d32 100644
@@ -1052,9 +1052,12 @@ static void azx_init_pci(struct azx *chip)
 	/* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)
 	 * TCSEL == Traffic Class Select Register, which sets PCI express QOS
 	 * Ensuring these bits are 0 clears playback static on some HD Audio
-	 * codecs
+	 * codecs.
+	 * The PCI register TCSEL is defined in the Intel manuals.
 	 */
-	update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
+	if (chip->driver_type != AZX_DRIVER_ATI &&
+	    chip->driver_type != AZX_DRIVER_ATIHDMI)
+		update_pci_byte(chip->pci, ICH6_PCIREG_TCSEL, 0x07, 0);
 
 	switch (chip->driver_type) {
 	case AZX_DRIVER_ATI:

Signature · 7 lines

1.7.2.3


Adam
-- 
Adam                 [email protected]
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/