Re: [coreboot] SB600 HDA can't find codec fix

[Dan Lykowski]

Maggie,
 Oops, I can't convert binary to hex.. 
7 should have been 15. and it should have been bits [3..0].
Is this what you are referring to?

I don't understand what you mean by pci_locate_device is only used during early setup. I see it called in the SATA driver to find the SMBus. Is this incorrect also? What would be the best way to get the SMBus? Is the device being stored somewhere that I don't currently see?

Thanks,
Dan Lykowski 


--- On Sun, 2/1/09, Li, Maggie <[e-mail]> wrote:

From: Li, Maggie <[e-mail]>
Subject: Re: [coreboot] SB600 HDA can't find codec fix
To: "Dan Lykowski" <[e-mail]>
Cc: "Marc Jones" <[e-mail]>, "Carl-Daniel Hailfinger" <[e-mail]>, [e-mail]
Date: Sunday, February 1, 2009, 3:28 AM

Hi, Dan

I have tested your patch on my dbm690t (ALC882) and pistachio (ALC885) board. It really works. However, I have a suggestion for you.

/* Read in Codec location (BAR + 0xe)[2..0]*/
    dword = readl(base + 0xe);
    dword &= 7;
    if (!dword)  
        goto no_codec;
The above phrase is not correct all the time, at lease to my pistachio board. It will give me the wrong msg "No codec!".

I would appreciate and ack the patch if you can modify it.

BTW, pci_locate_device is only used in early setup stage. So, you can remove it.

Best regards
Maggie li

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Marc Jones
Sent: Thursday, January 29, 2009 1:10 PM
To: Dan Lykowski
Cc: Carl-Daniel Hailfinger; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

On Tue, Jan 27, 2009 at 11:07 AM, Dan Lykowski
<[e-mail]> wrote:
> Diff was being silly and I wanted to get the patch posted before I left work
> for the day. I've cleaned up the patch and included it.
>
> I wasn't able to find where INTA was used so I used what the RPR lists as
> default. INTG. After looking at the mptable, I agree INTA is the correct
> answer. I've corrected it. I used dev_find_slot because I copied from the
> SATA driver. I've added the comment just like the SATA driver has. I don't
> know what the difference is, or why the SATA driver did this.
>
> The reordering was based on what order things happen in the BIOS Developers
> guide, RPR, and SATA driver. I fixed the order of the devices that didn't
> matter to clean up the change log.
> 1. Enable the Chip
> 2. Setup the SMBus registers
> 3. Setup the Device Registers
> 4. Look for Codec
> 5. Init Codec
>
> The codec init was changed to match the description in the RRG pg 235.
> Mem Reg: Base + 08h Bit 0. There were unneeded things happening.
> So here is the second try.
>
> Thanks,
> Dan Lykowski
>
> Signed-off-by: Dan Lykowski <[e-mail]>

This looks good to me. The hda_init looks like it was writing to the
wrong device for the interrupt line setup. It would be good if the the
AMD guys or Carl-Daniel can test and ack it.

Marc

--
coreboot mailing list: [e-mail]
http://www.coreboot.org/mailman/listinfo/coreboot

--
coreboot mailing list: [e-mail]
http://www.coreboot.org/mailman/listinfo/coreboot

SB600 HDA can't find codec fix

[Li, Maggie]

Dan,

 

I read the RRG spec, just as you said, you should use 0xF in file sb600_hda.c to get the current audio codec, that is to say, 

dword &= 0xF;
    if (!dword)  
        goto no_codec;

 

pci_locate_device is really should be used in early setup. Device_t is the type of u32 at that time. After this stage, you should use dev_find_slot. In file sb600_sata.c, “/* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you misunderstanding about how to get the SMBus and it should be removed. You can submit a patch about sata and I will ack it. Thanks.

Best regards 
Maggie li

________________________________

From: Dan Lykowski [mailto:[e-mail]] 
Sent: Sunday, February 01, 2009 6:51 PM
To: Li, Maggie
Cc: Marc Jones; Carl-Daniel Hailfinger; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

 

Maggie,
 Oops, I can't convert binary to hex.. 
7 should have been 15. and it should have been bits [3..0].
Is this what you are referring to?

I don't understand what you mean by pci_locate_device is only used during early setup. I see it called in the SATA driver to find the SMBus. Is this incorrect also? What would be the best way to get the SMBus? Is the device being stored somewhere that I don't currently see?

Thanks,
Dan Lykowski 


--- On Sun, 2/1/09, Li, Maggie <[e-mail]> wrote:


From: Li, Maggie <[e-mail]>
Subject: Re: [coreboot] SB600 HDA can't find codec fix
To: "Dan Lykowski" <[e-mail]>
Cc: "Marc Jones" <[e-mail]>, "Carl-Daniel Hailfinger" <[e-mail]>, [e-mail]
Date: Sunday, February 1, 2009, 3:28 AM

Hi, Dan

I have tested your patch on my dbm690t (ALC882) and pistachio (ALC885) board. It really works. However, I have a suggestion for you.

/* Read in Codec location (BAR + 0xe)[2..0]*/
    dword = readl(base + 0xe);
    dword &= 7;
    if (!dword)  
        goto no_codec;
The above phrase is not correct all the time, at lease to my pistachio board. It will give me the wrong msg "No codec!".

I would appreciate and ack the patch if you can modify it.

BTW, pci_locate_device is only used in early setup stage. So, you can remove it.

Best regards
Maggie li

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Marc Jones
Sent: Thursday, January 29, 2009 1:10 PM
To: Dan Lykowski
Cc: Carl-Daniel Hailfinger; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

On Tue, Jan 27, 2009 at 11:07 AM, Dan Lykowski
<[e-mail]> wrote:
> Diff was being silly and I wanted to get the patch posted before I left work
> for the day. I've cleaned up the patch and included it.
>
> I wasn't able to find where INTA was used so I used what the RPR lists as
> default. INTG. After looking at the mptable, I agree INTA is the correct
> answer. I've corrected it. I used dev_find_slot because I copied from the
> SATA driver. I've added the comment just like the SATA driver has. I don't
> know what the difference is, or why the SATA driver did this.
>
> The reordering was based on what order things happen in the BIOS Developers
> guide, RPR, and SATA driver. I fixed the order of the devices that didn't
> matter to clean up the change log.
> 1. Enable the Chip
> 2. Setup the SMBus registers
> 3. Setup the Device Registers
> 4. Look for Codec
> 5. Init Codec
>
> The codec init was changed to match the description in the RRG pg 235.
> Mem Reg: Base + 08h Bit 0. There were unneeded things happening.
> So here is the second try.
>
> Thanks,
> Dan Lykowski
>
> Signed-off-by: Dan Lykowski <[e-mail]>

This looks good to me. The hda_init looks like it was writing to the
wrong device for the interrupt line setup. It would be good if the the
AMD guys or Carl-Daniel can test and ack it.

Marc

--
coreboot mailing list: [e-mail]
http://www.coreboot.org/mailman/listinfo/coreboot

--
coreboot mailing list: [e-mail]
http://www.coreboot.org/mailman/listinfo/coreboot

SB600 HDA can't find codec fix

[Stefan Reinauer]

Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file
> sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>     if (!dword) 
>         goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is
> the type of u32 at that time. After this stage, you should use
> dev_find_slot. In file sb600_sata.c, “/* sm_dev =
> pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you
> misunderstanding about how to get the SMBus and it should be removed.
> You can submit a patch about sata and I will ack it. Thanks.

Somewhat related, I think the
        device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));

should be passed from hda_init() instead, because the device is already
known there.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

SB600 HDA can't find codec fix

[Li, Maggie]

Stefan

I agree with you.

Best regards
Maggie li

-----Original Message-----
From: Stefan Reinauer [mailto:[e-mail]] 
Sent: Monday, February 02, 2009 10:18 AM
To: Li, Maggie
Cc: Dan Lykowski; Marc Jones; Carl-Daniel Hailfinger; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file
> sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>     if (!dword) 
>         goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is
> the type of u32 at that time. After this stage, you should use
> dev_find_slot. In file sb600_sata.c, “/* sm_dev =
> pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you
> misunderstanding about how to get the SMBus and it should be removed.
> You can submit a patch about sata and I will ack it. Thanks.

Somewhat related, I think the
        device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));

should be passed from hda_init() instead, because the device is already
known there.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: [phone] • Fax: [phone]
Email: [e-mail]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866

SB600 HDA can't find codec fix

[Carl-Daniel Hailfinger]

Hi Maggie,

while your point about pci_find_device is valid, I think that
dev_find_device is the function Dan should choose. It allows us to
address the PCI devices even if their bus locations are shifted which is
possible with some AMD chipsets and may also be true for boards with
multiple chipsets.
In general, we want to use functions which can deal with non-default bus
topologies.

On 02.02.2009 02:48, Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>     if (!dword)  
>         goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is the type of u32 at that time. After this stage, you should use dev_find_slot. In file sb600_sata.c, “/* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you misunderstanding about how to get the SMBus and it should be removed. You can submit a patch about sata and I will ack it. Thanks.

I'd prefer to use dev_find_device there as well.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

SB600 HDA can't find codec fix

[Li, Maggie]

Carl, 

I see. Pci_find_device is more reliable than dev_find_slot. However, we didn't use the former in our SB600 and RS690 code. There are many places to be modified if needed.

Best regards
Maggie li

-----Original Message-----
From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
Sent: Monday, February 02, 2009 10:26 AM
To: Li, Maggie
Cc: Dan Lykowski; Marc Jones; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

Hi Maggie,

while your point about pci_find_device is valid, I think that
dev_find_device is the function Dan should choose. It allows us to
address the PCI devices even if their bus locations are shifted which is
possible with some AMD chipsets and may also be true for boards with
multiple chipsets.
In general, we want to use functions which can deal with non-default bus
topologies.

On 02.02.2009 02:48, Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>     if (!dword)  
>         goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is the type of u32 at that time. After this stage, you should use dev_find_slot. In file sb600_sata.c, “/* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you misunderstanding about how to get the SMBus and it should be removed. You can submit a patch about sata and I will ack it. Thanks.

I'd prefer to use dev_find_device there as well.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

SB600 HDA can't find codec fix

[Dan Lykowski]

Sorry for the delay.. Way too much to do and so little time..

Fixed the spotted issues and added 1ms delay to match the BKDG while waiting for BAR+0xe to set its bits.

Dan Lykowski

Signed-off-by: Dan Lykowski<[e-mail]>




________________________________
From: "Li, Maggie" <[e-mail]>
To: Carl-Daniel Hailfinger <[e-mail]>
Cc: Dan Lykowski <[e-mail]>; Marc Jones <[e-mail]>; [e-mail]
Sent: Sunday, February 1, 2009 7:47:15 PM
Subject: RE: [coreboot] SB600 HDA can't find codec fix

Carl, 

I see. Pci_find_device is more reliable than dev_find_slot. However, we didn't use the former in our SB600 and RS690 code. There are many places to be modified if needed.

Best regards
Maggie li

-----Original Message-----
From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
Sent: Monday, February 02, 2009 10:26 AM
To: Li, Maggie
Cc: Dan Lykowski; Marc Jones; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

Hi Maggie,

while your point about pci_find_device is valid, I think that
dev_find_device is the function Dan should choose. It allows us to
address the PCI devices even if their bus locations are shifted which is
possible with some AMD chipsets and may also be true for boards with
multiple chipsets.
In general, we want to use functions which can deal with non-default bus
topologies.

On 02.02.2009 02:48, Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>     if (!dword)  
>         goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is the type of u32 at that time. After this stage, you should use dev_find_slot. In file sb600_sata.c, “/* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you misunderstanding about how to get the SMBus and it should be removed. You can submit a patch about sata and I will ack it. Thanks.

I'd prefer to use dev_find_device there as well.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

SB600 HDA can't find codec fix

[Li, Maggie]

Dan

 

I think you made a mistake. It should be “sm_dev = dev_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_ATI_SB600_SM, 0);”, not “sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);” in your file.

 

In my last letter, there is a writing error. We should say that dev_find_device is more reliable than dev_find_slot

Best regards 
Maggie li 

________________________________

From: Dan Lykowski [mailto:[e-mail]] 
Sent: Wednesday, February 04, 2009 11:58 AM
To: Li, Maggie; Carl-Daniel Hailfinger
Cc: Marc Jones; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

 

Sorry for the delay.. Way too much to do and so little time..

Fixed the spotted issues and added 1ms delay to match the BKDG while waiting for BAR+0xe to set its bits.

Dan Lykowski

Signed-off-by: Dan Lykowski<[e-mail]>

 

________________________________

From: "Li, Maggie" <[e-mail]>
To: Carl-Daniel Hailfinger <[e-mail]>
Cc: Dan Lykowski <[e-mail]>; Marc Jones <[e-mail]>; [e-mail]
Sent: Sunday, February 1, 2009 7:47:15 PM
Subject: RE: [coreboot] SB600 HDA can't find codec fix

Carl, 

I see. Pci_find_device is more reliable than dev_find_slot. However, we didn't use the former in our SB600 and RS690 code. There are many places to be modified if needed.

Best regards
Maggie li

-----Original Message-----
From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
Sent: Monday, February 02, 2009 10:26 AM
To: Li, Maggie
Cc: Dan Lykowski; Marc Jones; [e-mail]
Subject: Re: [coreboot] SB600 HDA can't find codec fix

Hi Maggie,

while your point about pci_find_device is valid, I think that
dev_find_device is the function Dan should choose. It allows us to
address the PCI devices even if their bus locations are shifted which is
possible with some AMD chipsets and may also be true for boards with
multiple chipsets.
In general, we want to use functions which can deal with non-default bus
topologies.

On 02.02.2009 02:48, Li, Maggie wrote:
> Dan,
>
> I read the RRG spec, just as you said, you should use 0xF in file sb600_hda.c to get the current audio codec, that is to say,
>
> dword &= 0xF;
>    if (!dword)  
>        goto no_codec;
>
> pci_locate_device is really should be used in early setup. Device_t is the type of u32 at that time. After this stage, you should use dev_find_slot. In file sb600_sata.c, “/* sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0) */ “ gives you misunderstanding about how to get the SMBus and it should be removed. You can submit a patch about sata and I will ack it. Thanks.

I'd prefer to use dev_find_device there as well.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

SB600 HDA can't find codec fix

[Carl-Daniel Hailfinger]

Hi Dan,

sorry for chasing you around in circles on that issue.

On 04.02.2009 05:14, Li, Maggie wrote:
> Dan
>
> I think you made a mistake. It should be “sm_dev = dev_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_ATI_SB600_SM, 0);”, not “sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);” in your file.
>
> In my last letter, there is a writing error. We should say that dev_find_device is more reliable than dev_find_slot

I believe dev_find_device is indeed best. Anyway, I think I can make
that change locally and commit if you're OK with that.

Regards,
Carl-Daniel
> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
> Sent: Monday, February 02, 2009 10:26 AM
> To: Li, Maggie
> Cc: Dan Lykowski; Marc Jones; [e-mail]
> Subject: Re: [coreboot] SB600 HDA can't find codec fix
>
> Hi Maggie,
>
> while your point about pci_find_device is valid, I think that
> dev_find_device is the function Dan should choose. It allows us to
> address the PCI devices even if their bus locations are shifted which is
> possible with some AMD chipsets and may also be true for boards with
> multiple chipsets.
> In general, we want to use functions which can deal with non-default bus
> topologies.

SB600 HDA can't find codec fix

[Dan Lykowski]

Its OK, I'm flexible. Do whatever everyone decides is right, I just need to move on to other things.

Thanks,
Dan Lykowski




________________________________
From: Carl-Daniel Hailfinger <[e-mail]>
To: "Li, Maggie" <[e-mail]>
Cc: Marc Jones <[e-mail]>; Dan Lykowski <[e-mail]>; [e-mail]
Sent: Tuesday, February 3, 2009 8:20:17 PM
Subject: Re: [coreboot] SB600 HDA can't find codec fix

Hi Dan,

sorry for chasing you around in circles on that issue.

On 04.02.2009 05:14, Li, Maggie wrote:
> Dan
>
> I think you made a mistake. It should be “sm_dev = dev_find_device(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_ATI_SB600_SM, 0);”, not “sm_dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0);” in your file.
>
> In my last letter, there is a writing error. We should say that dev_find_device is more reliable than dev_find_slot

I believe dev_find_device is indeed best. Anyway, I think I can make
that change locally and commit if you're OK with that.

Regards,
Carl-Daniel
> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]] 
> Sent: Monday, February 02, 2009 10:26 AM
> To: Li, Maggie
> Cc: Dan Lykowski; Marc Jones; [e-mail]
> Subject: Re: [coreboot] SB600 HDA can't find codec fix
>
> Hi Maggie,
>
> while your point about pci_find_device is valid, I think that
> dev_find_device is the function Dan should choose. It allows us to
> address the PCI devices even if their bus locations are shifted which is
> possible with some AMD chipsets and may also be true for boards with
> multiple chipsets.
> In general, we want to use functions which can deal with non-default bus
> topologies.

SB600 HDA can't find codec fix

[Carl-Daniel Hailfinger]

On 04.02.2009 09:01, Dan Lykowski wrote:
> Its OK, I'm flexible. Do whatever everyone decides is right, I just need to move on to other things.

Thanks, committed in r3930.

Two problems remain:
- Using dev_find_device instead of dev_find_slot causes a hang during
PCI init.
- I get unexpected warnings from the coreboot HDA code:
PCI: 00:14.2 init
base = fc400000
No codec!
codec_mask = 01
codec viddid: 10ec0883
Dev=PCI: 00:14.2
Default viddid=10ec0882
Reading viddid=10ec0883
No verb!
PCI: 00:14.3 init

Linux says something similar: "Intel 0000:00:14.2: PCI INT A -> GSI 16
(level, low) -> IRQ 16".

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/