[PATCH] ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

[PATCH] ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

[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.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 sound/pci/hda/hda_intel.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index fcedad9..41debbd 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -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;
 
         }
-- 
1.7.2.3


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [PATCH] ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

[Takashi Iwai]

At Thu, 10 Mar 2011 16:22:12 +0100,
Adam Lackorzynski wrote:
> 
> 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

> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  sound/pci/hda/hda_intel.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index fcedad9..41debbd 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -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;
>  
>          }
> -- 
> 1.7.2.3
> 
> 
> Adam
> -- 
> Adam                 adam@os.inf.tu-dresden.de
>   Lackorzynski         http://os.inf.tu-dresden.de/~adam/
> 

Re: [PATCH] ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

[Adam Lackorzynski]

On Thu Mar 10, 2011 at 16:44:35 +0100, Takashi Iwai wrote:
> At Thu, 10 Mar 2011 16:22:12 +0100,
> Adam Lackorzynski wrote:
> > 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.)

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.

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 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 <adam@os.inf.tu-dresden.de>
---
 sound/pci/hda/hda_intel.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index fcedad9..70a9d32 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -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:
-- 
1.7.2.3


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [PATCH] ALSA: hda: Only write ICH6_PCIREG_TCSEL on Intel systems.

[Takashi Iwai]

At Thu, 10 Mar 2011 17:41:56 +0100,
Adam Lackorzynski wrote:
> 
> 
> On Thu Mar 10, 2011 at 16:44:35 +0100, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 16:22:12 +0100,
> > Adam Lackorzynski wrote:
> > > 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.)
> 
> 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.


>  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.
> 
> 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 <adam@os.inf.tu-dresden.de>

Thanks, applied now.


Takashi

> ---
>  sound/pci/hda/hda_intel.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index fcedad9..70a9d32 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -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:
> -- 
> 1.7.2.3
> 
> 
> Adam
> -- 
> Adam                 adam@os.inf.tu-dresden.de
>   Lackorzynski         http://os.inf.tu-dresden.de/~adam/
>