Skip to content

ASoC: SOF: refine the selection of machine drivers #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RanderWang wants to merge 1 commit into from
Closed

ASoC: SOF: refine the selection of machine drivers #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,6 @@ static int sof_probe(struct platform_device *pdev)
spin_lock_init(&sdev->ipc_lock);
spin_lock_init(&sdev->hw_lock);

/* set up platform component driver */
snd_sof_new_platform_drv(sdev);

/* set default timeouts if none provided */
if (plat_data->desc->ipc_timeout == 0)
sdev->ipc_timeout = TIMEOUT_DEFAULT_IPC;
Expand All @@ -280,6 +277,9 @@ static int sof_probe(struct platform_device *pdev)
return ret;
}

/* set up platform component driver */
snd_sof_new_platform_drv(sdev);

/* register any debug/trace capabilities */
ret = snd_sof_dbg_init(sdev);
if (ret < 0) {
Expand Down
41 changes: 36 additions & 5 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <sound/hdaudio.h>
#include <sound/hda_i915.h>
#include <sound/sof/xtensa.h>
#include <sound/soc-acpi-intel-match.h>

#include "../sof-priv.h"
#include "../ops.h"
Expand Down Expand Up @@ -438,8 +439,11 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
struct pci_dev *pci = sdev->pci;
struct hdac_ext_link *hlink = NULL;
struct snd_soc_acpi_mach_params *mach_params;
struct snd_soc_acpi_mach *mach;
struct snd_sof_pdata *pdata;
int codec_num = 0;
int ret = 0;
int err;
int err, i;

device_disable_async_suspend(bus->dev);

Expand Down Expand Up @@ -472,10 +476,37 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
else
dev_info(bus->dev, "hda codecs found, mask %lx!\n", bus->codec_mask);

/* used by hda machine driver to create dai links */
mach_params = (struct snd_soc_acpi_mach_params *)
&sdev->pdata->machine->mach_params;
mach_params->codec_mask = bus->codec_mask;
if (bus->codec_mask) {
for (i = 0; i < HDA_MAX_CODECS; i++) {
if (bus->codec_mask & (1 << i))
codec_num++;
}

/* if there are HDMI codec and HDA codecs, hda machine
* is applied. For I2S codec + HDMI codec, there is only one
* hda codec. For other cases, the value should be zero.
*/
if (codec_num > 1) {
Copy link
Member

@plbossart plbossart Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is wrong. Is there is a single codec, the skl_hda_generic_machine driver can work
this should be codec_num > 0

Copy link
Author

@RanderWang RanderWang Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, one case

/* used by hda machine driver to create dai links */
pdata = sdev->pdata;

mach = snd_soc_acpi_intel_hda_machines;
mach->pdata = pdata->machine->pdata;
mach->sof_fw_filename =
pdata->desc->nocodec_fw_filename;
mach->new_mach_data =
pdata->machine->new_mach_data;

mach_params = &mach->mach_params;
mach_params->codec_mask = bus->codec_mask;
mach_params->platform = pdata->platform;

devm_kfree(sdev->dev, (void *)pdata->machine);
pdata->machine = mach;

dev_info(sdev->dev, "Find hda codecs, use hda machine driver\n");
}
Copy link

@keyonjie keyonjie Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do nocodec setup here:

else
        sof_nocodec_setup()

Copy link
Author

@RanderWang RanderWang Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please refer to sof_xxx_probe. machine driver carries some information for sof_probe.
And if no codec is found at probe function, a default codec should be used at there, no here

Copy link
Collaborator

@ranj063 ranj063 Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang as I see it the logic is:
if force-no-codec-mode
use no-codec machine drive
else
If acpi match found
use the appropriate machine driver
else
use no-codec machine driver

if num-hda-codecs > 1
use hda machine driver

Is this correct? then does this mean we cannot force no-codec machine driver if hda is enabled?

Copy link
Author

@RanderWang RanderWang Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 please try CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE.
if hda is enable and no hda codec, it would be nocodec

}

/* create codec instances */
hda_codec_probe_bus(sdev);
Expand Down
6 changes: 0 additions & 6 deletions sound/soc/sof/sof-acpi-dev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,12 @@ static int sof_acpi_probe(struct platform_device *pdev)
/* find machine */
mach = snd_soc_acpi_find_machine(desc->machines);
if (!mach) {
#if IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)
/* fallback to nocodec mode */
Copy link
Member

@plbossart plbossart Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you changing this? This has nothing to do with HDaudio?

Copy link
Author

@RanderWang RanderWang Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but you need to add
#ifndef CONFIG_SND_SOC_SOF_NOCODEC
return error
#endif

If I don't delete it, it should be selected for HDaudio, or pci_probe or acpic_probe will quit with error. And CONFIG_SND_SOC_SOF_NOCODEC has nothing to do with HDaudio, so it is by no means to keep it here.

dev_warn(dev, "No matching ASoC machine driver found - using nocodec\n");
mach = devm_kzalloc(dev, sizeof(*mach), GFP_KERNEL);
ret = sof_nocodec_setup(dev, sof_pdata, mach, desc, ops);
if (ret < 0)
return ret;
#else
dev_warn(dev, "No matching ASoC machine driver found - falling back to HDA codec\n");
mach = snd_soc_acpi_intel_hda_machines;
mach->sof_fw_filename = desc->nocodec_fw_filename;
#endif
Copy link
Member

@plbossart plbossart Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was a bug though, not sure why we had HDaudio codec mentioned in that file.

}
#endif

Expand Down
16 changes: 0 additions & 16 deletions sound/soc/sof/sof-pci-dev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,20 +222,6 @@ static int sof_pci_probe(struct pci_dev *pci,
#else
/* find machine */
mach = snd_soc_acpi_find_machine(desc->machines);
Copy link
Member

@plbossart plbossart Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the flow here. For HDaudio this will fail, so how do you not end-up in the error case.
Even if you set the mach variable earlier, it'll be overridden here.
And you are doing this before doing the DSP probe (the platform driver is registered later)

Does this even work?

Copy link
Author

@RanderWang RanderWang Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For HDaudio, nocodec is setup here. in sof_probe, if hd codec is detected , HDaudio machine driver would be applied.

platform driver is registered after HDaudio machine driver is applied.

I test it on WHL, it works.

Copy link
Author

@RanderWang RanderWang Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart , I will wait for your PR to be merged and do it based on it.

Copy link
Member

@plbossart plbossart Nov 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang
I don't get what you are saying and 'it works' isn't a good explanation, sorry I don't know what 'it' means.
The ask is that we can have a single kernel with HDaudio compiled in, and the behavior is as follows
a. try to find an ACPI ID and create the relevant machine driver
b. if this fails, check if there are HDAudio codecs. if HDaudio + iDISP are found, use the generic HDaudio machine driver.
c1. if nothing is detected fall back to nocodec.
c2. If only iDISP is found, either fallback to a nocodec+iDISP configuration or to a simple iDISP configuration if nocodec is not selected.

Is this clearer?

Copy link
Author

@RanderWang RanderWang Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart Please forgive me about my English, I should improve it.
I have a discuss with Mengdong, now I totally understand your idea. We need to detect hda code in pci_probe. I check the driver code and find it is impossible to finish it and validate it today.
(1) sof-pci-dev.c is in our core layer, but hda is in intel layer. Again the same issue for us like link dma setting, a interface is needed for communication.
(2) if hda codec is detected in pci_probe, the initialization and detection code should be deleted in intel layer ?

This is my idea, I am sure is not wrong. I will close my PR

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
if (!mach) {
dev_warn(dev, "No matching ASoC machine driver found - falling back to HDA codec\n");
mach = snd_soc_acpi_intel_hda_machines;
mach->sof_fw_filename = desc->nocodec_fw_filename;

/*
* TODO: we need to find a way to check if codecs are actually
* present
*/
}
#endif /* CONFIG_SND_SOC_SOF_HDA */

#if IS_ENABLED(CONFIG_SND_SOC_SOF_NOCODEC)
if (!mach) {
/* fallback to nocodec mode */
dev_warn(dev, "No matching ASoC machine driver found - using nocodec\n");
Expand All @@ -244,8 +230,6 @@ static int sof_pci_probe(struct pci_dev *pci,
if (ret < 0)
goto release_regions;
}
#endif /* CONFIG_SND_SOC_SOF_NOCODEC */

#endif /* CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE */

mach->pdata = ops;
Expand Down