[RFC] Clock stop and jack detection support by RanderWang · Pull Request #1456 · thesofproject/linux

RanderWang · 2019-11-08T11:58:56Z

These patches add support for clock stop mode and jack detection on soundwire platform.

Changes to the sdw_slave structure needed to solve race conditions on driver probe. The functionality is added in the next patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

We need an async mechanism to prevent access to Slaves that are not fully-enumerated. init_completion() will be invoked when the Slave becomes UNATTACHED, and complete() will be invoked when the state become ATTACHED. Any read/write before this status change will be delayed with a wait_for_completion(). Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

This helper is useful to make sure that the resume stages wait for the device to be fully initialized, not just enumerated. Depending on how initialization is handled, a Slave device driver may wait for enumeration_complete, or for initialization_complete, both are valid synchronization points. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Expose updated interfaces in sdw_intel.h, mainly to allow SoundWire and ASoC parts to be merged separately once the header files are shared between trees. To avoid compilation issues, the conflicts in intel_init.c are blindly removed. This would in theory prevent the code from working, but since there are no users of the Intel Soundwire driver this has no impact. Functionality will be restored when the removal of platform devices is complete. Support for SoundWire + SOF builds will only be provided once all the required pieces are upstream. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…erations Rename 'config_stream' as 'params_stream' to be closer to ALSA/ASoC concepts. Add a 'free_stream' callback in case any resources allocated in the 'params_stream' stage need to be released. Define structures for callbacks, mainly to allow for extensions as needed. Add the link_id and alh_stream_id parameters which are needed to align with firmware IPC needs. Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The previous formula is incorrect for PDI0/1, the mapping is not linear but has a discontinuity between PDI1 and PDI2. This change has no effect on PCM PDIs (same mapping). Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

There is no good reason why the unique_id needs to be stored as 4 bits. The code will work without changes with a u8 since all values are already filtered while parsing the ACPI tables and Slave devID registers. Use u8 representation. This will allow us to encode a "IGNORE_UNIQUE_ID" value to account for firmware/BIOS creativity. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Simplify the loop with a helper. The only functionality change is that we continue the loop even with an ACPI error. Follow-up patches will build on this change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The uniqueID is useful when there are two or more devices of the same type (identical manufacturer ID, part ID) on the same link. When there is a single device of a given type on a link, its uniqueID is irrelevant. It's not uncommon on actual platforms to see variations of the uniqueID, or differences between devID registers and ACPI _ADR fields. This patch suggests a filter on startup to identify 'single' devices and tag them accordingly. The uniqueID is then not used for the probe, and the device name omits the uniqueID as well. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add clearer references to sdw_slave_driver for internal macros No change for sdw_driver and module_sdw_driver to avoid compatibility issues with existing codec devices No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Since we want to introduce master devices, rename macro so that we have consistency between slave and master device access, following the Grey Bus example. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Align with previous renames and shorten macro No functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Before we add master driver support, make sure there is no ambiguity and no occurrences of sdw_drv_ functions. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

There are too many fields called 'res' so add prefix to make it easier to track what the structures are. Pure rename, no functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Currently the bus does not have any explicit support for master devices. First add explicit support for sdw_slave_type and error checks if this type is not set. In follow-up patches we can add support for the sdw_md_type (md==Master Device), following the Grey Bus example. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Since we want an explicit support for the SoundWire Master device, add the definitions, following the Grey Bus example. Unlike for the Slave device, we do not set a variable when dealing with the master uevent. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use sdw_master_device and driver instead of platform devices To quote GregKH: "Don't mess with a platform device unless you really have no other possible choice. And even then, don't do it and try to do something else. Platform devices are really abused, don't perpetuate it " In addition, rather than a plain-vanilla init/exit, this patch provides 3 steps in the initialization (ACPI scan, probe, startup) which make it easier to verify support and allocate required resources as early as possible, and conversely help make the startup lighter-weight with only hardware register setup. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Setting an device driver is necessary for ASoC to register DAI components. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Now that the SoundWire core supports the multi-step initialization, call the relevant APIs. The actual hardware enablement can be done in two places, ideally we'd want to startup the SoundWire IP as soon as possible (while still taking power rail dependencies into account) However when suspend/resume is implemented, the DSP device will be resumed first, and only when the DSP firmware is downloaded/booted would the SoundWire child devices be resumed, so there are only marginal benefits in starting the IP earlier for the first probe. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ALH was inserted in the wrong place during integration, add after DMIC to mirror the file used by SOF firmware. No functional change, just text move in the same file to better track changes, if any. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

These callbacks are invoked when a matching hw_params/hw_free() DAI operation takes place, and will result in IPC operations with the SOF firmware. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

For now we have a limited number of machine driver configurations, and we can detect them based on the link configuration returned after checking hardware and firmware (BIOS) configurations. It's likely that in the future we will need to check for _ADR match as well, which can easily be done by extending the acpi_info structure. There is a chance that in extreme cases where the BIOS contains too much information we would need to detect which Slave devices actually report as 'attached'. This would be more accurate than static table-based solutions, but it also introduces timing dependencies since we don't know when those devices might become attached, so will only be only be looked at if we see limitations with static methods and the usual quirks based e.g. on DMI information. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Doing this avoid conflicts and errors reported on the bus. The interrupts are only re-enabled on resume after the firmware is downloaded, so the behavior is not fully symmetric Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Select SoundWire capabilities on newer Intel platforms, starting with CannonLake/CoffeeLake/CometLake. As done for HDaudio, the SoundWire link is an opt-in capability. We explicitly test for ACPI to avoid warnings on unmet dependencies on the SoundWire side. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Without this cycle, HDaudio capability parsing fails on some devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Initial support for ALC/RT700 in SoundWire mode This codec is present on Intel CNL/CML and ICL RVP boards and the code was tested by both Realtek and Intel. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>

The driver for this amplifier was tested by Realtek and Intel using the Realtek 3-in-1 add-on boards connected to CometLake and IceLake platforms Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>

The driver for this codec was tested by Realtek and Intel using the Realtek 3-in-1 add-on board connected to CometLake and IceLake platforms. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>

The driver for this capture device was tested by Realtek and Intel using the Realtek 3-in-1 add-on boards connected to CometLake and IceLake platforms Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Jack Yu <jack.yu@realtek.com>

Don't push upstream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Signed-off-by: Shuming Fan <shumingf@realtek.com>

…nto integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…ntegration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…egration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…o integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…rs' into integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

…into integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

SoundWire supports two clock stop modes. Add support to handle the clock stop modes and add pm_runtime calls in the bus. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add Cadence runtime power management routines, to be used for clock-stop mode. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The default behavior is to use clock_stop only for pm_runtime. During regular resume, the link is completely re-initialized. An kernel module option can override this policy and disable clock-stop modes Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add resume function for Cadence runtime power management routines, to be used for clock-stop mode Signed-off-by: Rander Wang <rander.wang@intel.com>

Invoke master pm function to enable clock stop mode Signed-off-by: Rander Wang <rander.wang@intel.com>

When system is suspended in clock stop mode on intel platforms, both master and slave are in clock stop mode and soundwire bus is taken over by a special hardware. The bus message for jack event is processed by this special hardware, which will tigger a interrupt to ACPI system and resume audio pci device. Then audio pci driver will resume soundwire master and slave, transfer bus ownership to master, finally slave will report jack event to master and codec driver is triggered to check jack status. Signed-off-by: Rander Wang <rander.wang@intel.com>

After FW transfers soundwire ownership to host, host can check shim registers to check jack status. Signed-off-by: Rander Wang <rander.wang@intel.com>

plbossart

Thanks @RanderWang
It looks quite good for a first pass. Can you look at the comments below and submit an update?
Please submit against integration/soundwire-intel so that the code is easier to merge.

plbossart · 2019-11-11T20:51:33Z

drivers/soundwire/cadence_master.c

+	if (ret < 0)
+		return ret;
+
+	ret = cdns_set_wait(cdns, CDNS_MCP_STAT, CDNS_MCP_STAT_CLK_STOP);


that part is suspicious.
When the bus changes to clock stop, it's an operation synchronous on all sides.
I don't see why we need to wait here, this should already be in CLK_STOP mode after calling sdw_bus_clk_stop().

Can you recheck @RanderWang?

@plbossart I checked internal code and spec again. Next step is to transfer ownership to glue hw, so we need to make sure master is in clk_stop mode now and do a smooth transition to glue hw.

@RanderWang I don't mind if we check that the clock is stopped here, but the wait part should not happen. If we engage the clock stop procedure, and then keep the clock active for a bit longer until that bit is set, then I have no idea what slaves will do. It's actually an illegal transition, the Master shall drive the data line for 2 cycles before resuming the clock.
Let's recheck this, shall we?

plbossart · 2019-11-11T20:54:54Z

drivers/soundwire/cadence_master.c

+		return 1;
+	}
+
+	/* Disable block wakeup */


add a command that the hardware specification for the IP requires this bit to be set prior to entering Clock Stop state.

I will refine it

plbossart · 2019-11-11T20:59:35Z

drivers/soundwire/intel.c

-	sdw_clear_slave_status(&sdw->cdns.bus);
+	if (clock_stop)
+		/* make sure all Slaves are tagged as UNATTACHED */
+		sdw_clear_slave_status(&sdw->cdns.bus);


No, this is not correct,
We should only tag the slaves as UNATTACHED when we do a hw_reset.

yes, I got it.

plbossart · 2019-11-11T21:00:54Z

drivers/soundwire/intel.c


-	/* make sure all Slaves are tagged as UNATTACHED */
-	sdw_clear_slave_status(&sdw->cdns.bus);
+	return _suspend(dev, clock_stop);


I suggest we first work through the code changes for suspend and pm_runtime, and at a later time we merge the code in helpers
This just too hard to review.

yes, I will refine it

plbossart · 2019-11-11T21:03:06Z

drivers/soundwire/bus.c

+		mode = slave->curr_clk_stop_mode;
+
+		if (mode == SDW_CLK_STOP_MODE1)
+			sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);


let's drop CLK_STOP_MODE1 for now, it's not really useful.

ok, I will refine it

plbossart · 2019-11-11T21:03:50Z

drivers/soundwire/bus.c

+
+	return ret;
+}
+


let's drop clock_stop_mode1, it's not really useful

ok, I will refine it

plbossart · 2019-11-11T21:05:27Z

drivers/soundwire/cadence_master.c

+	int ret;
+
+	/* Disable block wakeup */
+	cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_BLOCK_WAKEUP, 0);


why is this here? It's already set on suspend?

yes, it is set in suspend. Comments should be refined.

plbossart · 2019-11-11T21:06:55Z

drivers/soundwire/cadence_master.c

+		return ret;
+	}
+
+	ret = sdw_bus_exit_clk_stop(&cdns->bus);


again that is suspicious. Why would we restart the clock before we tell the bus the clock restarted.

without master clock restart, we can't tell the bus the clock restarted. I got IO transfer timeout.

plbossart · 2019-11-11T21:15:39Z

drivers/soundwire/intel.c

+		list_for_each_entry(slave, &bus->slaves, node) {
+			if (slave->prop.wake_capable) {
+				pm_runtime_get(&slave->dev);
+				pm_runtime_put(&slave->dev);


sorry, but this looks like a horrible hack.
what are you trying to achieve with this?

I suspect the idea is that we don't know which Slave device generated the wake, so we request all Slaves to become active and each driver will then double-check the interrupt source that caused the wake.

but isn't this what pm_runtime_request_resume() is all about

int pm_request_resume(struct device *dev); - submit a request to execute the subsystem-level resume callback for the device (the request is represented by a work item in pm_wq); returns 0 on success, 1 if the device's runtime PM status was already 'active', or error code if the request hasn't been queued up

sorry, but this looks like a horrible hack.
what are you trying to achieve with this?

I suspect the idea is that we don't know which Slave device generated the wake, so we request all Slaves to become active and each driver will then double-check the interrupt source that caused the wake.

yes, this is my idea. we only know which master trigger a interrupt, but don't know which slave.
I will refine it in another way.

plbossart · 2019-11-19T20:10:38Z

replaced by PR #1498

plbossart and others added 30 commits November 6, 2019 11:01

soundwire: sdw_slave: add new fields to track probe status

41227b2

Changes to the sdw_slave structure needed to solve race conditions on driver probe. The functionality is added in the next patch. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: slave: add helper to extract slave ID

fb8e530

Simplify the loop with a helper. The only functionality change is that we continue the loop even with an ACPI error. Follow-up patches will build on this change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: rename dev_to_sdw_dev macro

ea80c01

Since we want to introduce master devices, rename macro so that we have consistency between slave and master device access, following the Grey Bus example. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: rename drv_to_sdw_slave_driver macro

c46560f

Align with previous renames and shorten macro No functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: bus_type: rename sdw_drv_ to sdw_slave_drv

9e6843f

Before we add master driver support, make sure there is no ambiguity and no occurrences of sdw_drv_ functions. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: intel: rename res field as link_res

a53c311

There are too many fields called 'res' so add prefix to make it easier to track what the structures are. Pure rename, no functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: add device driver to sdw_md_driver

a255d99

Setting an device driver is necessary for ASoC to register DAI components. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: Intel: hda-ctrl: add reset cycle before parsing capabilities

acdb781

Without this cycle, HDaudio capability parsing fails on some devices. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: codecs: Add DEBUG to Makefile

74b8b4e

Don't push upstream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

shumingfan and others added 7 commits November 8, 2019 11:11

ASoC: rt1308-sdw: re-do io_init in cache_bypass mode when system resume

b28f04c

Signed-off-by: Shuming Fan <shumingf@realtek.com>

Merge remote-tracking branch 'sof/integration/soundwire-interfaces' i…

26858f5

…nto integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Merge remote-tracking branch 'sof/integration/soundwire-intel' into i…

92b0b25

…ntegration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Merge remote-tracking branch 'sof/integration/soundwire-sof' into int…

6749d7b

…egration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Merge remote-tracking branch 'sof/integration/realtek-sdw-codecs' int…

a7f4bbd

…o integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Merge remote-tracking branch 'sof/integration/soundwire-machine-drive…

46f4e6a

…rs' into integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Merge remote-tracking branch 'sof/integration/soundwire-debug-fixes' …

391b5f9

…into integration/soundwire-latest Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the integration/soundwire-latest branch from b12acfe to 391b5f9 Compare November 8, 2019 22:49

RanderWang added 3 commits November 11, 2019 14:54

soundwire: Add clock stop support

b1a5ef4

SoundWire supports two clock stop modes. Add support to handle the clock stop modes and add pm_runtime calls in the bus. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

soundwire: cadence_master: add power management support

616c1d3

Add Cadence runtime power management routines, to be used for clock-stop mode. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

RanderWang force-pushed the clock_stop_jack branch from 8c4f953 to 07efd93 Compare November 11, 2019 06:54

RanderWang added 4 commits November 11, 2019 15:14

soundwire: cadence_master: add resume support

be7ea25

Add resume function for Cadence runtime power management routines, to be used for clock-stop mode Signed-off-by: Rander Wang <rander.wang@intel.com>

soundwire: refine pm function

82d6575

Invoke master pm function to enable clock stop mode Signed-off-by: Rander Wang <rander.wang@intel.com>

ASoC: sof: check jack status for soundwire

5af1c09

After FW transfers soundwire ownership to host, host can check shim registers to check jack status. Signed-off-by: Rander Wang <rander.wang@intel.com>

RanderWang force-pushed the clock_stop_jack branch from 07efd93 to 5af1c09 Compare November 11, 2019 07:16

plbossart force-pushed the integration/soundwire-latest branch from 391b5f9 to 0c30933 Compare November 11, 2019 20:03

plbossart requested changes Nov 11, 2019

View reviewed changes

plbossart force-pushed the integration/soundwire-latest branch 9 times, most recently from 246ab27 to 4cd6497 Compare November 18, 2019 20:40

plbossart closed this Nov 19, 2019

Conversation

RanderWang commented Nov 8, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants