Skip to content

Soundwire: register intel sdw master driver with driver_register()#1607

Merged
plbossart merged 1 commit intothesofproject:integration/soundwire-debug-fixesfrom
bardliao:sdw-bind-device
Dec 11, 2019
Merged

Soundwire: register intel sdw master driver with driver_register()#1607
plbossart merged 1 commit intothesofproject:integration/soundwire-debug-fixesfrom
bardliao:sdw-bind-device

Conversation

@bardliao
Copy link
Collaborator

@bardliao bardliao commented Dec 9, 2019

dev->power.no_pm_callbacks flag is used to check if there is pm ops
for the device. Pm callback function will not be invoked if there is
no pm ops. Currently the flag is true for sdw-master devices which
is incorrect since we do have pm ops in intel_sdw_driver. As a result,
intel_suspend() which is the suspend callback function of intel-sdw
driver may not be invoked in system suspend.
With driver_register() and setting md->dev.driver before
device_register(), dev->power.no_pm_callbacks flag will be set to the
correct value (false) by device_pm_check_callbacks() which will be
triggered by device_register().

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

Fixes #1595 #1614

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

@bardliao can you clarify what issue this fixes and why?

Also the exit/release flow doesn't seem quite right?

void sdw_intel_exit(struct sdw_intel_ctx *ctx)
{
sdw_intel_cleanup(ctx);
driver_register(&intel_sdw_driver.driver);
Copy link
Member

Choose a reason for hiding this comment

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

driver_unregister() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Fix it now.

}

md->dev.driver = &driver->driver;
ret = device_bind_driver(&md->dev);
Copy link
Member

Choose a reason for hiding this comment

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

don't we need a matching device_release_driver() ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bardliao typically device_add should take care of binding the driver isnt it? I may be wrong. But is the problem simply that we should set md->dev.driver before we call device_resgiter() in sdw_md_add?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bardliao typically device_add should take care of binding the driver isnt it? I may be wrong. But is the problem simply that we should set md->dev.driver before we call device_resgiter() in sdw_md_add?

@ranj063 You are right. Actually, I tried it before. But we didn't call driver_register() at that time so it result in system crash. Now moving md->dev.driver before device_resgiter() work fine.

@plbossart plbossart force-pushed the integration/soundwire-debug-fixes branch from f22b344 to fa47d1f Compare December 9, 2019 18:25
@plbossart
Copy link
Member

@bardliao can you also clarify what issue you are chasing? Last week it was a problem with issue #1595 but I can't reproduce the error with the existing integration/soundwire-latest.

@bardliao
Copy link
Collaborator Author

@bardliao can you also clarify what issue you are chasing? Last week it was a problem with issue #1595 but I can't reproduce the error with the existing integration/soundwire-latest.

@plbossart What I want to fix is to make sure intel_suspend() will be triggered for all links. I will update my commit message then.

@plbossart
Copy link
Member

@bardliao see trace attached

dmesg_arecord1_rtcwake_arecord4.log

[   23.582884] intel-sdw sdw-master-2: intel_suspend_runtime start
[   23.583131] intel-sdw sdw-master-2: intel_suspend_runtime done
[   29.393476] intel-sdw sdw-master-1: intel_suspend_runtime start
[   29.393894] intel-sdw sdw-master-1: intel_suspend_runtime done
[   29.847667] intel-sdw sdw-master-3: intel_suspend_runtime start
[   29.847979] intel-sdw sdw-master-3: intel_suspend_runtime done
[   38.462559] intel-sdw sdw-master-3: intel_suspend start
[   38.462562] intel-sdw sdw-master-3: intel_suspend: pm_runtime status: suspended
[   38.462567] intel-sdw sdw-master-2: intel_suspend start
[   38.462569] intel-sdw sdw-master-2: intel_suspend: pm_runtime status: suspended
[   38.462574] intel-sdw sdw-master-1: intel_suspend start
[   38.462577] intel-sdw sdw-master-1: intel_suspend: pm_runtime status: suspended
[   38.462587] intel-sdw sdw-master-0: intel_suspend start
[   38.462599] intel-sdw sdw-master-0: intel_suspend done
[   46.313061] intel-sdw sdw-master-2: intel_suspend_runtime start
[   46.313090] intel-sdw sdw-master-3: intel_suspend_runtime start
[   46.313109] intel-sdw sdw-master-1: intel_suspend_runtime start
[   46.313581] intel-sdw sdw-master-1: intel_suspend_runtime done
[   46.313605] intel-sdw sdw-master-3: intel_suspend_runtime done
[   48.321434] intel-sdw sdw-master-2: intel_suspend_runtime done
[   54.025043] intel-sdw sdw-master-0: intel_suspend_runtime start
[   54.025669] intel-sdw sdw-master-0: intel_suspend_runtime done

and yes I have the standard rt700 on all links

/sys/kernel/debug/soundwire# ls master-*
master-0:
intel-sdw  sdw:0:25d:700:0  sdw:0:25d:711:0

master-1:
intel-sdw  sdw:1:25d:1308:0  sdw:1:25d:700:0

master-2:
intel-sdw  sdw:2:25d:700:0  sdw:2:25d:701:0

master-3:
intel-sdw  sdw:3:25d:700:0  sdw:3:25d:715:0

but I haven't updated the BIOS since the original release with the SOF key

@plbossart
Copy link
Member

@bardliao I am using only the clock_quirks 0x8, no other pm_runtime filter or link mask btw

dev->power.no_pm_callbacks flag is used to check if there is pm ops
for the device. Pm callback function will not be invoked if there is
no pm ops. Currently the flag is true for sdw-master devices which
is incorrect since we do have pm ops in intel_sdw_driver. As a result,
intel_suspend() which is the suspend callback function of intel-sdw
driver may not be invoked in system suspend.
With driver_register() and setting md->dev.driver before
device_register(), dev->power.no_pm_callbacks flag will be set to the
correct value (false) by device_pm_check_callbacks() which will be
triggered by device_register().

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
@bardliao bardliao changed the title Soundwire: bind master driver to device. Soundwire: register intel sdw master driver with driver_register() Dec 10, 2019
@bardliao
Copy link
Collaborator Author

@plbossart So the reasons you don't see #1595 is

  1. The issue can only be seen when runtime pm is disabled.
  2. The rt700 codecs are the "ghost". Can you try removing rt700 driver from the kernel?

@plbossart
Copy link
Member

@plbossart So the reasons you don't see #1595 is

  1. The issue can only be seen when runtime pm is disabled.
  2. The rt700 codecs are the "ghost". Can you try removing rt700 driver from the kernel?

ok, but can you file a bug that has all the information? it's impossible to guess or reproduce otherwise. thanks!

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants