[U-Boot] [PATCH 23/33] sound: x86: link: Add sound support

Simon Glass sjg at chromium.org
Sun Feb 17 03:25:19 UTC 2019


Hi Bin,

On Wed, 13 Feb 2019 at 02:39, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Jan 22, 2019 at 9:14 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Add sound support for link, using the HDA codec implementation.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  arch/x86/cpu/ivybridge/Kconfig                |   1 +
> >  arch/x86/cpu/ivybridge/northbridge.c          |  27 ++++
> >  arch/x86/dts/chromebook_link.dts              |  91 ++++++++++++
> >  .../include/asm/arch-ivybridge/sandybridge.h  |   3 +
> >  configs/chromebook_link_defconfig             |   2 +
> >  drivers/sound/Kconfig                         |  10 ++
> >  drivers/sound/Makefile                        |   1 +
> >  drivers/sound/ivybridge_sound.c               | 137 ++++++++++++++++++
> >  8 files changed, 272 insertions(+)
> >  create mode 100644 drivers/sound/ivybridge_sound.c
> >
> > diff --git a/arch/x86/cpu/ivybridge/Kconfig b/arch/x86/cpu/ivybridge/Kconfig
> > index 5f0e60837c..2f42393786 100644
> > --- a/arch/x86/cpu/ivybridge/Kconfig
> > +++ b/arch/x86/cpu/ivybridge/Kconfig
> > @@ -21,6 +21,7 @@ config NORTHBRIDGE_INTEL_IVYBRIDGE
> >         imply USB_EHCI_HCD
> >         imply USB_XHCI_HCD
> >         imply VIDEO_VESA
> > +       imply SOUND_IVYBRIDGE
> >
> >  if NORTHBRIDGE_INTEL_IVYBRIDGE
> >
> > diff --git a/arch/x86/cpu/ivybridge/northbridge.c b/arch/x86/cpu/ivybridge/northbridge.c
> > index 39bab7bdf3..0ac0a34acc 100644
> > --- a/arch/x86/cpu/ivybridge/northbridge.c
> > +++ b/arch/x86/cpu/ivybridge/northbridge.c
> > @@ -177,6 +177,30 @@ static void sandybridge_setup_northbridge_bars(struct udevice *dev)
> >         dm_pci_write_config8(dev, PAM6, 0x33);
> >  }
> >
> > +static void sandybridge_init_iommu(struct udevice *dev)
> > +{
> > +       u32 capid0_a;
> > +
> > +       dm_pci_read_config32(dev, 0xe4, &capid0_a);
> > +       if (capid0_a & (1 << 23)) {
> > +               log_debug("capid0_a not needed\n");
> > +               return;
> > +       }
> > +
> > +       /* setup BARs */
> > +       writel(IOMMU_BASE1 >> 32, MCHBAR_REG(0x5404));
> > +       writel(IOMMU_BASE1 | 1, MCHBAR_REG(0x5400));
> > +       writel(IOMMU_BASE2 >> 32, MCHBAR_REG(0x5414));
> > +       writel(IOMMU_BASE2 | 1, MCHBAR_REG(0x5410));
> > +
> > +       /* lock policies */
> > +       writel(0x80000000, IOMMU_BASE1 + 0xff0);
> > +
> > +       /* Enable azalia sound */
> > +       writel(0x20000000, IOMMU_BASE2 + 0xff0);
> > +       writel(0xa0000000, IOMMU_BASE2 + 0xff0);
> > +}
>
> The function name looks it has something to do with IOMMU, but why
> does enabling sound support need IOMMU which is mainly for hypervisor
> stuff? Better providing some comments here, and if possible use macros
> instead of hard-coded numbers.

I've looked very hard but cannot find any documentation for these
registers. They seem to be in a reserved area. The coreboot patch that
this code came from is at
https://review.coreboot.org/c/coreboot/+/12194/2 but there are no
comments.

>
> > +
> >  static int bd82x6x_northbridge_early_init(struct udevice *dev)
> >  {
> >         const int chipset_type = SANDYBRIDGE_MOBILE;
> > @@ -197,6 +221,9 @@ static int bd82x6x_northbridge_early_init(struct udevice *dev)
> >
> >         sandybridge_setup_northbridge_bars(dev);
> >
> > +       /* Setup IOMMU BARs */
> > +       sandybridge_init_iommu(dev);
> > +
> >         /* Device Enable */
> >         dm_pci_write_config32(dev, DEVEN, DEVEN_HOST | DEVEN_IGD);
> >
> > diff --git a/arch/x86/dts/chromebook_link.dts b/arch/x86/dts/chromebook_link.dts
> > index f9f0979730..7e0a615a60 100644
> > --- a/arch/x86/dts/chromebook_link.dts
> > +++ b/arch/x86/dts/chromebook_link.dts
> > @@ -1,6 +1,8 @@
> >  /dts-v1/;
> >
> >  #include <dt-bindings/gpio/x86-gpio.h>
> > +#include <dt-bindings/sound/azalia.h>
> > +#include <pci_ids.h>
> >
> >  /include/ "skeleton.dtsi"
> >  /include/ "keyboard.dtsi"
> > @@ -372,6 +374,30 @@
> >                         compatible = "ehci-pci";
> >                 };
> >
> > +               hda at 1b,0 {
> > +                       reg = <0x0000d800 0 0 0 0>;
> > +                       compatible = "intel,bd82x6x-hda";
> > +                       beep-verbs = <
> > +                               0x00170500      /* power up codec */
> > +                               0x00270500      /* power up DAC */
> > +                               0x00b70500      /* power up speaker */
> > +                               0x00b70740      /* enable speaker out */
> > +                               0x00b78d00      /* enable EAPD pin */
> > +                               0x00b70c02      /* set EAPD pin */
> > +                               0x0143b013>;    /* beep volume */
>
> Are these verbs from the chipset datasheet?

They are actually using the HD audio spec. In the first one 705 is to
set the power state and 1 is the node ID.

It would be possible to change these to defines but it is quite
laborious and I'm not sure how useful it is. I could use the macros to
separate out the fields if you think that would help a lot. But people
still need to look up the spec to understand these values.

I'll add a comment mentioning the specification again.

>
> > +
> > +                       codecs {
> > +                               creative_codec: creative-ca0132 {
> > +                                       vendor-id = <PCI_VENDOR_ID_CREATIVE>;
> > +                                       device-id = <PCI_DEVICE_ID_CREATIVE_CA01322>;
> > +                               };
> > +                               intel_hdmi: hdmi {
> > +                                       vendor-id = <PCI_VENDOR_ID_INTEL>;
> > +                                       device-id = <PCI_DEVICE_ID_INTEL_COUGARPOINT_HDMI>;
> > +                               };
> > +                       };
> > +               };
> > +
> >                 usb_0: usb at 1d,0 {
> >                         reg = <0x0000e800 0 0 0 0>;
> >                         compatible = "ehci-pci";
> > @@ -492,3 +518,68 @@
> >         };
> >
> >  };
> > +
> > +&creative_codec {
> > +       verbs =  <
> > +               /* Malcolm Setup */
> > +               0x01570d09 0x01570c23 0x01570a01 0x01570df0
> > +               0x01570efe 0x01570775 0x015707d3 0x01570709
> > +               0x01570753 0x015707d4 0x015707ef 0x01570775
> > +               0x015707d3 0x01570709 0x01570702 0x01570737
> > +               0x01570778 0x01553cce 0x015575c9 0x01553dce
> > +               0x0155b7c9 0x01570de8 0x01570efe 0x01570702
> > +               0x01570768 0x01570762 0x01553ace 0x015546c9
> > +               0x01553bce 0x0155e8c9 0x01570d49 0x01570c88
> > +               0x01570d20 0x01570e19 0x01570700 0x01571a05
> > +               0x01571b29 0x01571a04 0x01571b29 0x01570a01
> > +
>
> And here? Is this something one can easily get from the chipset
> datasheet? If not, better adding some comments to point out the
> sources of these magic numbers.

Again I'll add a comment. Let me know what you think.

[..]

Regards,
Simon


More information about the U-Boot mailing list