...

static int sdw_uevent(struct device *dev, struct kobj_uevent_env *env) {

• struct sdw_slave *slave = to_sdw_slave_device(dev);

• struct sdw_slave *slave; char modalias[32];

• sdw_slave_modalias(slave, modalias, sizeof(modalias));
• if (add_uevent_var(env, "MODALIAS=%s", modalias))

• return -ENOMEM;
  

• if (is_sdw_slave(dev)) {

• slave = to_sdw_slave_device(dev);
  

• sdw_slave_modalias(slave, modalias, sizeof(modalias));
  

• if (add_uevent_var(env, "MODALIAS=%s", modalias))
  

• 	return -ENOMEM;
  

• } else {

• /*
  

•  * We only need to handle uevents for the Slave device
  

•  * type. This error cannot happen unless the .uevent
  

•  * callback is set to use this function for a
  

•  * different device type (e.g. Master or Monitor)
  

•  */
  

• dev_err(dev, "uevent for unknown Soundwire type\n");
  

• return -EINVAL;
  

At this point and after next patch, the above code would be a no-op, do we want this here, if so why?

to be future proof if someone wants to add support for a monitor, as explained above. I can remove this if you don't want it.

On 02-01-20, 11:36, Pierre-Louis Bossart wrote:

Would this work for you if we used the following names:

sdw_slave (legacy shortcut for sdw_slave_device, which could be removed in a a future cleanup if desired). sdw_slave_driver sdw_master_device sdw_master_driver

and all the 'md' replaced by the full 'master_device'.

That does sound nicer, thanks

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:

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 makes it easier to verify hardware support for SoundWire, allocate required resources as early as possible, and conversely help make the startup() callback lighter-weight with only hardware register setup.

...

+struct sdw_md_driver intel_sdw_driver = {

• .probe = intel_master_probe,
• .startup = intel_master_startup,
• .remove = intel_master_remove,

};

...

+extern struct sdw_md_driver intel_sdw_driver;

who uses this intel_sdw_driver? I would assumed someone would register this with the core...

+static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res) +{

• struct sdw_intel_link_res *link;

• struct sdw_intel_ctx *ctx;

• struct acpi_device *adev;

• struct sdw_master_device *md;

• u32 link_mask;

• int count;

• int i;

• if (!res)

• return NULL;
  

• if (acpi_bus_get_device(res->handle, &adev))

• return NULL;
  

• if (!res->count)

• return NULL;
  

• count = res->count; dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);

  ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL;

• ctx->count = count;
• ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);

• ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL); if (!ctx->links) goto link_err;

• ctx->count = count;

• ctx->mmio_base = res->mmio_base;

• ctx->link_mask = res->link_mask;

• ctx->handle = res->handle;

• link = ctx->links;

• link_mask = ctx->link_mask;

  /* Create SDW Master devices */

• for (i = 0; i < count; i++) {

• if (link_mask && !(link_mask & BIT(i))) {
  

• 	dev_dbg(&adev->dev,
  

• 		"Link %d masked, will not be enabled\n", i);
  

• 	link++;
  

• for (i = 0; i < count; i++, link++) {

• if (link_mask && !(link_mask & BIT(i)))
  continue;
  

• }
  
  

• md = sdw_md_add(&intel_sdw_driver,
  

• 		res->parent,
  

• 		acpi_fwnode_handle(adev),
  

• 		i);
  

• if (IS_ERR(md)) {
  

• 	dev_err(&adev->dev, "Could not create link %d\n", i);
  

• 	goto err;
  

• }
  

• link->md = md;
  

• link->mmio_base = res->mmio_base;
  

  link->registers = res->mmio_base + SDW_LINK_BASE

• 			+ (SDW_LINK_SIZE * i);
  

• 	+ (SDW_LINK_SIZE * i);
  

  link->shim = res->mmio_base + SDW_SHIM_BASE; link->alh = res->mmio_base + SDW_ALH_BASE;

• link->irq = res->irq;
  

  link->ops = res->ops; link->dev = res->dev;

• memset(&pdevinfo, 0, sizeof(pdevinfo));
  

• pdevinfo.parent = res->parent;
  

• pdevinfo.name = "int-sdw";
  

• pdevinfo.id = i;
  

• pdevinfo.fwnode = acpi_fwnode_handle(adev);
  

• pdev = platform_device_register_full(&pdevinfo);
  

• if (IS_ERR(pdev)) {
  

• 	dev_err(&adev->dev,
  

• 		"platform device creation failed: %ld\n",
  

• 		PTR_ERR(pdev));
  

• 	goto pdev_err;
  

• }
  

• link->pdev = pdev;
  

• link++;
  

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

That reminds me I am missing match code for master driver...

So we seem to be somewhere is middle wrt driver probing here! IIUC this is not a full master driver, thats okay, but then it is not completely transparent either...

I was somehow thinking that the driver will continue to be 'platform/acpi/of' driver and master device abstraction will be handled in the core (for example see how the busses like i2c handle this). The master device is created and used to represent but driver probing etc is not done

Thoughts..?

On 27-12-19, 18:13, Pierre-Louis Bossart wrote:

...

...

+extern struct sdw_md_driver intel_sdw_driver;

who uses this intel_sdw_driver? I would assumed someone would register this with the core...

this is a structure used by intel_init(), see the following code.

• md = sdw_md_add(&intel_sdw_driver,
  

• 		res->parent,
  

• 		acpi_fwnode_handle(adev),
  

• 		i);
  
  

that will in turn call intel_master_probe() as defined below:

+struct sdw_md_driver intel_sdw_driver = {

• .probe = intel_master_probe,
• .startup = intel_master_startup,

...

...

• link->pdev = pdev;
  

• link++;
  

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

That reminds me I am missing match code for master driver...

There is no match for the master because it doesn't have an existence in ACPI. There are no _ADR or HID that can be used, the only thing that exists is the Controller which has 4 sublinks. Each master must be added by hand.

Also the SoundWire master cannot be enumerated or matched against a SoundWire bus, since it controls the bus itself (that would be a chicken and egg problem). The SoundWire master would need to be matched on a parent bus (which does not exist for Intel) since the hardware is embedded in a larger audio cluster that's visible on PCI only.

Currently for Intel platforms, the SoundWire master device is created by the SOF driver (via the abstraction in intel_init.c).

That is okay for me, the thing that is bit confusing is having a probe etc and no match.. (more below)..

...

So we seem to be somewhere is middle wrt driver probing here! IIUC this is not a full master driver, thats okay, but then it is not completely transparent either...

I was somehow thinking that the driver will continue to be 'platform/acpi/of' driver and master device abstraction will be handled in the core (for example see how the busses like i2c handle this). The master device is created and used to represent but driver probing etc is not done

I2C controllers are typically PCI devices or have some sort of ACPI description. This is not the case for SoundWire masters on Intel platforms,

Well the world is not PCI/ACPI... We have controllers which are DT described and work in same manner as a PCI device.

so even if I wanted to I would have no ability to implement any matching or parent bus registration.

Also the notion of 'probe' does not necessarily mean that the device is attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove callbacks.

The "big" difference is that probe is called by core (asoc) and not by driver onto themselves.. IMO that needs to go away.

And if you look at the definitions, we added additional callbacks since probe/remove are not enough to deal with hardware restrictions:

For Intel platforms, we have a startup() callback which is only invoked once the DSP is powered and the rails stable. Likewise we added an 'autonomous_clock_stop()' callback which will be needed when the Linux driver hands-over control of the hardware to the DSP firmware, e.g. to deal with in-band wakes in D0i3.

FWIW, the implementation here follows what was suggested for Greybus 'Host Devices' [1] [2], so it's not like I am creating any sort of dangerous precedent.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275 [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

And if you look closely all this work is done by core not by drivers! Drivers _should_ never do all this, it is the job of core to do that for you.

On 06-01-20, 08:51, Pierre-Louis Bossart wrote:

...

...

...

...

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

That reminds me I am missing match code for master driver...

There is no match for the master because it doesn't have an existence in ACPI. There are no _ADR or HID that can be used, the only thing that exists is the Controller which has 4 sublinks. Each master must be added by hand.

Also the SoundWire master cannot be enumerated or matched against a SoundWire bus, since it controls the bus itself (that would be a chicken and egg problem). The SoundWire master would need to be matched on a parent bus (which does not exist for Intel) since the hardware is embedded in a larger audio cluster that's visible on PCI only.

Currently for Intel platforms, the SoundWire master device is created by the SOF driver (via the abstraction in intel_init.c).

That is okay for me, the thing that is bit confusing is having a probe etc and no match.. (more below)..

...

...

So we seem to be somewhere is middle wrt driver probing here! IIUC this is not a full master driver, thats okay, but then it is not completely transparent either...

I was somehow thinking that the driver will continue to be 'platform/acpi/of' driver and master device abstraction will be handled in the core (for example see how the busses like i2c handle this). The master device is created and used to represent but driver probing etc is not done

I2C controllers are typically PCI devices or have some sort of ACPI description. This is not the case for SoundWire masters on Intel platforms,

Well the world is not PCI/ACPI... We have controllers which are DT described and work in same manner as a PCI device.

Both DT and PCI would use a DIFFERENT matching on the parent bus, not a matching provided by the SoundWire subsystem itself.

...

...

so even if I wanted to I would have no ability to implement any matching or parent bus registration.

Also the notion of 'probe' does not necessarily mean that the device is attached to a bus, we use DAI 'drivers' in ASoC and still have probe/remove callbacks.

The "big" difference is that probe is called by core (asoc) and not by driver onto themselves.. IMO that needs to go away.

What I did is not different from what existed already with platform devices. They were manually created, weren't they?

Manual creation of device based on a requirement is different, did I ask you why you are creating device :)

I am simple asking you not to call probe in the driver. If you need that, move it to core! We do not want these kind of things in the drivers...

...

...

And if you look at the definitions, we added additional callbacks since probe/remove are not enough to deal with hardware restrictions:

For Intel platforms, we have a startup() callback which is only invoked once the DSP is powered and the rails stable. Likewise we added an 'autonomous_clock_stop()' callback which will be needed when the Linux driver hands-over control of the hardware to the DSP firmware, e.g. to deal with in-band wakes in D0i3.

FWIW, the implementation here follows what was suggested for Greybus 'Host Devices' [1] [2], so it's not like I am creating any sort of dangerous precedent.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275 [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

And if you look closely all this work is done by core not by drivers! Drivers _should_ never do all this, it is the job of core to do that for you.

Please look at the code again, you have a USB probe that will manually call the GreyBus device creation.

static int ap_probe(struct usb_interface *interface, const struct usb_device_id *id) { hd = gb_hd_create(&es2_driver, &udev->dev,

static struct usb_driver es2_ap_driver = { .name = "es2_ap_driver", .probe = ap_probe, <<< code above .disconnect = ap_disconnect, .id_table = id_table, .soft_unbind = 1, };

Look closely the driver es2 calls into greybus core hd.c and gets the work done, subtle but a big differances in the approaches..

The master device probe suggested here is also called as part of the parent SOF PCI device probe, same as this USB example. I really don't see what your objection is, given that there is no way to deal with the SoundWire controller as a independent entity for Intel platforms.

On 10-01-20, 10:08, Pierre-Louis Bossart wrote:

...

...

...

The "big" difference is that probe is called by core (asoc) and not by driver onto themselves.. IMO that needs to go away.

What I did is not different from what existed already with platform devices. They were manually created, weren't they?

Manual creation of device based on a requirement is different, did I ask you why you are creating device :)

I am simple asking you not to call probe in the driver. If you need that, move it to core! We do not want these kind of things in the drivers...

What core are you talking about?

soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is considered as soundwire core part and rest are drivers intel, qc, so on!

The SOF intel driver needs to create a device, which will then be bound with a SoundWire master driver.

What I am doing is no different from what your team did with platform_register_device, I am really lost on what you are asking.

Again repeating myself, you call an API to do that is absolutely fine, but we don't do that in drivers or open code these things

...

...

...

...

FWIW, the implementation here follows what was suggested for Greybus 'Host Devices' [1] [2], so it's not like I am creating any sort of dangerous precedent.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275 [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

And if you look closely all this work is done by core not by drivers! Drivers _should_ never do all this, it is the job of core to do that for you.

Please look at the code again, you have a USB probe that will manually call the GreyBus device creation.

static int ap_probe(struct usb_interface *interface, const struct usb_device_id *id) { hd = gb_hd_create(&es2_driver, &udev->dev,

static struct usb_driver es2_ap_driver = { .name = "es2_ap_driver", .probe = ap_probe, <<< code above .disconnect = ap_disconnect, .id_table = id_table, .soft_unbind = 1, };

Look closely the driver es2 calls into greybus core hd.c and gets the work done, subtle but a big differances in the approaches..

I am sorry, I have absolutely no idea what you are referring to.

The code I copy/pasted here makes no call to the greybus core, it's ap_probe -> gb_hd_create. No core involved. If I am mistaken, please show me what I got wrong.

1. es2_ap_driver is host controller driver

2. gb_hd_create() is an API provided by greybus core!

es2 driver doesn't open code creation like you are doing in intel driver, it doesn't call probe on its own, greybus does that

This is very common pattern in linux kernel subsytems, drivers dont do these things, the respective subsystem core does that... see about es2 driver and implementation of gb_hd_create(). See callers of platform_register_device() and its implementation.

I don't know how else I can explain this to you, is something wrong in how I conveyed this info or you... or something else, I dont know!!!

On 13-01-20, 09:22, Pierre-Louis Bossart wrote:

On 1/12/20 11:18 PM, Vinod Koul wrote:

...

On 10-01-20, 10:08, Pierre-Louis Bossart wrote:

...

...

...

...

The "big" difference is that probe is called by core (asoc) and not by driver onto themselves.. IMO that needs to go away.

What I did is not different from what existed already with platform devices. They were manually created, weren't they?

Manual creation of device based on a requirement is different, did I ask you why you are creating device :)

I am simple asking you not to call probe in the driver. If you need that, move it to core! We do not want these kind of things in the drivers...

What core are you talking about?

soundwire core ofcourse! IMO All that which goes into soundwire-bus-objs is considered as soundwire core part and rest are drivers intel, qc, so on!

This master code was added to the bus: v v soundwire-bus-objs := bus_type.o bus.o master.o slave.o mipi_disco.o stream.o obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o

and the API is also part of the sdw.h include file. That seems to meet exactly what you describe above, no?

git grep sdw_master_device_add (reformatted output)

drivers/soundwire/intel_init.c: md = sdw_master_device_add(&intel_sdw_driver,

drivers/soundwire/master.c: *sdw_master_device_add(struct sdw_master_driver *driver,

drivers/soundwire/master.c: EXPORT_SYMBOL_GPL(sdw_master_device_add);

include/linux/soundwire/sdw.h: *sdw_master_device_add(struct sdw_master_driver *driver,

So, what exactly is the issue?

We are not 'calling the probe in the [Intel] driver' as you state it, we use a SoundWire core API which in turn will create a device. The device core takes care of calling the probe, see the master.c code which is NOT Intel-specific.

...

...

The SOF intel driver needs to create a device, which will then be bound with a SoundWire master driver.

What I am doing is no different from what your team did with platform_register_device, I am really lost on what you are asking.

Again repeating myself, you call an API to do that is absolutely fine, but we don't do that in drivers or open code these things

That is still quite unclear, what 'open-coding' are you referring to?

I am starting to wonder if you missed the addition of the master functionality in the previous patch:

[PATCH v5 08/17] soundwire: add initial definitions for sdw_master_device

What this patch 9 does is call the core-defined API and implement the intel-specific master driver.

Yes and no, it does call things introduced in patch 8, I questioned calling probe! See below!

...

...

...

...

...

> FWIW, the implementation here follows what was suggested for Greybus 'Host > Devices' [1] [2], so it's not like I am creating any sort of dangerous > precedent. > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/greybus/es2.c#L1275 > [2] https://elixir.bootlin.com/linux/latest/source/drivers/greybus/hd.c#L124

And if you look closely all this work is done by core not by drivers! Drivers _should_ never do all this, it is the job of core to do that for you.

Please look at the code again, you have a USB probe that will manually call the GreyBus device creation.

static int ap_probe(struct usb_interface *interface, const struct usb_device_id *id) { hd = gb_hd_create(&es2_driver, &udev->dev,

static struct usb_driver es2_ap_driver = { .name = "es2_ap_driver", .probe = ap_probe, <<< code above .disconnect = ap_disconnect, .id_table = id_table, .soft_unbind = 1, };

Look closely the driver es2 calls into greybus core hd.c and gets the work done, subtle but a big differances in the approaches..

I am sorry, I have absolutely no idea what you are referring to.

The code I copy/pasted here makes no call to the greybus core, it's ap_probe -> gb_hd_create. No core involved. If I am mistaken, please show me what I got wrong.

1. es2_ap_driver is host controller driver

2. gb_hd_create() is an API provided by greybus core!

same in my code...

...

es2 driver doesn't open code creation like you are doing in intel driver, it doesn't call probe on its own, greybus does that

This is very common pattern in linux kernel subsytems, drivers dont do these things, the respective subsystem core does that... see about es2 driver and implementation of gb_hd_create(). See callers of platform_register_device() and its implementation.

I don't know how else I can explain this to you, is something wrong in how I conveyed this info or you... or something else, I dont know!!!

the new 'master' functionality is part of the bus code, so please clarify what you see as problematic for the partition.

I am quoting the code in patch, which i pointed in my first reply!

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 4b769409f6f8..42f7ae034bea 100644 --- a/drivers/soundwire/intel_init.c

This is intel specific file...

+++ b/drivers/soundwire/intel_init.c

snip ...

+static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res)

this is intel driver, intel function!

• link->pdev = pdev;
  

• link++;
  

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

^^^^^^ which does this... calls a probe()!

And my first reply was:

...

•         /* let the SoundWire master driver to its probe */
  

•         md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

I rest my case!

On 14-01-20, 10:01, Pierre-Louis Bossart wrote:

...

I am quoting the code in patch, which i pointed in my first reply!

On 17-12-19, 15:03, Pierre-Louis Bossart wrote:

...

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 4b769409f6f8..42f7ae034bea 100644 --- a/drivers/soundwire/intel_init.c

This is intel specific file...

...

+++ b/drivers/soundwire/intel_init.c

snip ...

...

+static struct sdw_intel_ctx +*sdw_intel_probe_controller(struct sdw_intel_res *res)

this is intel driver, intel function!

...

• link->pdev = pdev;
  

• link++;
  

• /* let the SoundWire master driver to its probe */
  

• md->driver->probe(md, link);
  

                         ^^^^^^

which does this... calls a probe()!

And my first reply was:

...

...

•         /* let the SoundWire master driver to its probe */
  

•         md->driver->probe(md, link);
  

So you are invoking driver probe here.. That is typically role of driver core to do that.. If we need that, make driver core do that for you!

I rest my case!

I think you are too focused on the probe case and not realizing the extensions suggested by this patchset. A "driver" is not limited to 'probe' and 'remove' cases.

As mentioned since mid-September, there is a need for an initialization of software/kernel structures (which I called probe but should have been called init really), and a second step where the hardware is actually configured -

I find it amusing that a person whom i admired for strict use of terms can get this differently!

A rename away from probe will certainly be very helpful as you would also agree that terms 'probe' and 'remove' have a very special meaning in kernel, so let us avoid these

Hi Pierre,

I took some time to look into the overall code and how this is fitting into big picture and help recommend way forward.

On 21-01-20, 11:31, Pierre-Louis Bossart wrote:

...

A rename away from probe will certainly be very helpful as you would also agree that terms 'probe' and 'remove' have a very special meaning in kernel, so let us avoid these

ok, so would the following be ok with you?

/**

• struct sdw_md_driver - SoundWire 'Master Device' driver
• @init: allocations and initializations (hardware may not be enabled yet)
• @startup: initialization handled after the hardware is enabled, all
• clock/power dependencies are available
• @shutdown: cleanups before hardware is disabled (optional)
• @exit: free all remaining resources
• @autonomous_clock_stop_enable: enable/disable driver control while
• in clock-stop mode, typically in always-on/D0ix modes. When the driver
• yields control, another entity in the system (typically firmware
• running on an always-on microprocessor) is responsible to tracking
• Slave-initiated wakes

*/ struct sdw_md_driver { int (*init)(struct sdw_master_device *md, void *link_ctx); int (*startup)(struct sdw_master_device *md); int (*shutdown)(struct sdw_master_device *md); int (*exit)(struct sdw_master_device *md); int (*autonomous_clock_stop_enable)(struct sdw_master_device *md, bool state); };

So this is a soundwire core driver structure, but the modelling and explanation provided here suggests the reasoning to be based on hardware sequencing. I am not sure if we should follow this approach. Solving hardware sequencing is fine but that should IMO be restricted to intel code as that is intel issue which may or may not be present on other controllers.

If I look at the calling sequence of the code (looked up the sof code on github, topic/sof-dev-rebase), the sof code sound/soc/sof/intel/hda.c invokes the sdw_intel_startup() and sdw_intel_probe() based on hardware sequencing and further you call .init and .probe/startup of sdw_md_driver.

I really do not see why we need a sdw_md_driver object to do that. You can easily have a another function exported by sdw_intel driver and you call these and do same functionality without having a sdw_md_driver in between.

Now, I am going to step back one more step and ask you why should we have a sdw_md_driver? I am not seeing the driver object achieving anything here expect adding wrappers which we can avoid. But we still need to add the sdw_master_device() as a new device object and use that for both sysfs representation as well as representing the master device and do all the things we want, but it *can* come without having accompanying sdw_md_driver.

This way you can retain you calling sequence and add the master device.

Stretching this one more step I would ask that maybe it is even better idea that we should hide sdw_master_device_add() calling for soundwire drivers and move that internal to bus as part of bus registration as well, I don't see sdw_master_device calling back into the driver so it should not impact your sequences as well.

Do you see a reason for sdw_md_driver which is must have? I couldn't find that by looking at the code, let me know if I have missed anything here.

So to summarize, my recommendation would be to drop sdw_md_driver, keep sdw_master_device object and make sdw_master_device_add() hidden to driver and call it from sdw_add_bus_master() and keep intel specific startup/init routine which do same steps as they have now.

Thanks

Hi Pierre,

On 28-01-20, 10:02, Pierre-Louis Bossart wrote:

...

...

struct sdw_md_driver { int (*init)(struct sdw_master_device *md, void *link_ctx); int (*startup)(struct sdw_master_device *md); int (*shutdown)(struct sdw_master_device *md); int (*exit)(struct sdw_master_device *md); int (*autonomous_clock_stop_enable)(struct sdw_master_device *md, bool state); };

So this is a soundwire core driver structure, but the modelling and explanation provided here suggests the reasoning to be based on hardware sequencing. I am not sure if we should follow this approach. Solving hardware sequencing is fine but that should IMO be restricted to intel code as that is intel issue which may or may not be present on other controllers.

I agree the directions are Intel-specific, there was no intention of requiring anyone to implement all these callbacks.

...

If I look at the calling sequence of the code (looked up the sof code on github, topic/sof-dev-rebase), the sof code sound/soc/sof/intel/hda.c invokes the sdw_intel_startup() and sdw_intel_probe() based on hardware sequencing and further you call .init and .probe/startup of sdw_md_driver.

I really do not see why we need a sdw_md_driver object to do that. You can easily have a another function exported by sdw_intel driver and you call these and do same functionality without having a sdw_md_driver in between.

I must admit I am beyond my comfort zone: I introduced this sdw_master_driver only based on Greg's recommendation to look at GreyBus.

Well the recommendation was to add a device, so adding sdw_master_device was the right approach. But we don't need to add driver as well.

All I care about are the three steps of a. creating a device (needed because the ACPI description does not model the master hardware) b. doing the required initializations and allocations c. exposing a startup callback, and provide room for expansion for future evolutions.

If there's a better way to do this I don't mind at all. The value-added of these patches is not in how the device/driver model is used but rather the hardware sequencing and power management.

That said, I have two key points that I should explain further.

When we started this work, we initially did not have a notion of 'master driver' coupled with the 'master_device'. But we faced a number of issues with ASoC registrations, which seemed to required a driver to be registered and associated with the device. We kept this part as a separate commit to make sure this requirement wasn't lost:

Yes ASoC does require the DAI driver as it holds a reference to it. That was the reason we have a platform device and a platform driver for "int-sdw".

Would it make sense to keep this as is and then export the init and startup APIs which can be invoked by SOF part for hardware initialization. So driver loading is a separate step from hardware initialization and sequencing.

Also you continue with the benefit of parent-child relations for this device :) and pm should be simpler.

Btw this helps if ever in future you have a separate instance of sdw controller that can be just hooked to acpi/pci/whatever device part and driver works without big plumbing

https://github.com/thesofproject/linux/commit/28b934ce9c165e095dac5cf71da568...

Looking back at my notes, this came from this piece of code:

static char *fmt_single_name(struct device *dev, int *id) { char *found, name[NAME_SIZE]; int id1, id2;

if (dev_name(dev) == NULL) return NULL;

strlcpy(name, dev_name(dev), NAME_SIZE);

/* are we a "%s.%d" name (platform and SPI components) */ found = strstr(name, dev->driver->name); <<< oops

so for ASoC, there is an expectation that each device does have a driver associated with it. Just for fun I reverted the commit above and that immediately trigger an kernel oops.

The other point that we came across is that PM support is only enabled thanks to a hook in the driver structure:

static const struct dev_pm_ops intel_pm = { SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume) SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL) };

struct sdw_master_driver intel_sdw_driver = { .driver = { .name = "intel-sdw", .owner = THIS_MODULE, .bus = &sdw_bus_type, .pm = &intel_pm, <<<< }, .probe = intel_master_probe, .startup = intel_master_startup, .process_wake_event = intel_master_process_wakeen_event, .remove = intel_master_remove, };

I am not aware of any other way to do this.

In short, I have two technical reasons we came across that made us rely on this 'master driver' object. I agree that the solution I suggested may not be very elegant or can be seen as overkill, but it does provide support for two integration requirements.

...

Now, I am going to step back one more step and ask you why should we have a sdw_md_driver? I am not seeing the driver object achieving anything here expect adding wrappers which we can avoid. But we still need to add the sdw_master_device() as a new device object and use that for both sysfs representation as well as representing the master device and do all the things we want, but it *can* come without having accompanying sdw_md_driver.

This way you can retain you calling sequence and add the master device.

Stretching this one more step I would ask that maybe it is even better idea that we should hide sdw_master_device_add() calling for soundwire drivers and move that internal to bus as part of bus registration as well, I don't see sdw_master_device calling back into the driver so it should not impact your sequences as well.

Do you see a reason for sdw_md_driver which is must have? I couldn't find that by looking at the code, let me know if I have missed anything here.

Yes, the two reasons explained above.

That said, one possible compromize would be to remove those callbacks and indirections, such md->driver->remove(md);

we could perfectly export something like

intel_device_remove(md)

which would directly call the relevant functions. that would essentially remove the notion of 'sdw_master_driver' but keep the bare minimal driver structure that's needed.

Note that the removal of platform devices is not something I wanted to enforce across the board. If Qualcomm are happy with the platform devices, that's fine with me. I only want to work around the two limitations I mentioned (ACPI does not support masters, only controllers, and power rail dependencies).

...

So to summarize, my recommendation would be to drop sdw_md_driver, keep sdw_master_device object and make sdw_master_device_add() hidden to driver and call it from sdw_add_bus_master() and keep intel specific startup/init routine which do same steps as they have now.

I am afraid the sequence you suggest is not technically possible: sdw_add_bus_master() requires a device to be created already, see e.g. how it's used by Qualcomm:

https://github.com/thesofproject/linux/blob/4026efd12ac983d363fb43c37a8af741...

sdw_add_bus_master() is called from the platform device probe. I cannot insert a device creation in this code.

So there are two things, lets discuss them independently. One is the device behind the controller object. I think I have given a recommendation above to keep the existing platform device for "int-sdw". This device can be PCI/ACPI/OF/Platform, we typically should not worry about it.

Second is we create the bus instance and that also create the sdw_master_device to represent the soundwire master device in sysfs, since this would be done by soundwire core (as I suggested), it would be independent of which type of driver is invoking this. In fact this needs to be completely independent of the driver calling this.

Intel code will call sdw_add_bus_master() and we would create a sdw_master_device for it and add the representation. So would the qcom code (btw with this, we wont need to do anything on qc code and it 'should just work (tm)'.

It'd actually be counter productive to add device management at the bus level, since we'd soon have incompatibilities between Intel, Qualcomm and others. What is platform-specific should be handled outside of the bus layer.

The vision I am thinking is that sdw_master_device is a representation of master for everyone independent of the platform it is being used.

Let me know if this helps

Thanks

On 29-01-20, 08:59, Pierre-Louis Bossart wrote:

Hi Vinod,

...

...

...

If I look at the calling sequence of the code (looked up the sof code on github, topic/sof-dev-rebase), the sof code sound/soc/sof/intel/hda.c invokes the sdw_intel_startup() and sdw_intel_probe() based on hardware sequencing and further you call .init and .probe/startup of sdw_md_driver.

I really do not see why we need a sdw_md_driver object to do that. You can easily have a another function exported by sdw_intel driver and you call these and do same functionality without having a sdw_md_driver in between.

I must admit I am beyond my comfort zone: I introduced this sdw_master_driver only based on Greg's recommendation to look at GreyBus.

Well the recommendation was to add a device, so adding sdw_master_device was the right approach. But we don't need to add driver as well.

I provided two reasons below why we need one [1] ASoC use of dev->driver [2] PM hooks.

I would kindly ask that you re-check these points and reply on each, thank you. I did not remove them from my reply so that you don't have to go back to the previous emails.

...

...

That said, I have two key points that I should explain further.

When we started this work, we initially did not have a notion of 'master driver' coupled with the 'master_device'. But we faced a number of issues with ASoC registrations, which seemed to required a driver to be registered and associated with the device. We kept this part as a separate commit to make sure this requirement wasn't lost:

Yes ASoC does require the DAI driver as it holds a reference to it. That was the reason we have a platform device and a platform driver for "int-sdw".

no, it's not related to references, it's using the name, see [1]. And it's not related to the 'dai driver' which is a separate entity unrelated to devices.

...

Would it make sense to keep this as is and then export the init and startup APIs which can be invoked by SOF part for hardware initialization. So driver loading is a separate step from hardware initialization and sequencing.

Also you continue with the benefit of parent-child relations for this device :) and pm should be simpler.

the parent-child relationship was used in all cases, I don't understand why things would be different, you still need to add hooks in the 'driver' for the pm of the 'master' child device, be it a platform device or not.

...

Btw this helps if ever in future you have a separate instance of sdw controller that can be just hooked to acpi/pci/whatever device part and driver works without big plumbing

...

https://github.com/thesofproject/linux/commit/28b934ce9c165e095dac5cf71da568...

[1]

...

...

Looking back at my notes, this came from this piece of code:

static char *fmt_single_name(struct device *dev, int *id) { char *found, name[NAME_SIZE]; int id1, id2;

if (dev_name(dev) == NULL) return NULL;

strlcpy(name, dev_name(dev), NAME_SIZE);

/* are we a "%s.%d" name (platform and SPI components) */ found = strstr(name, dev->driver->name); <<< oops

so for ASoC, there is an expectation that each device does have a driver associated with it. Just for fun I reverted the commit above and that immediately trigger an kernel oops.

in the existing driver (the one on upstream atm), you have:

static struct platform_driver sdw_intel_drv = { .probe = intel_probe, .remove = intel_remove, .driver = { .name = "int-sdw",

}, };

This is the DAI driver which registers the DAIs and and you pass on this device, so for platform driver and above it should just work. That is why my recommendation is to continue with this.

[2]

...

...

The other point that we came across is that PM support is only enabled thanks to a hook in the driver structure:

static const struct dev_pm_ops intel_pm = { SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume) SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL) };

struct sdw_master_driver intel_sdw_driver = { .driver = { .name = "intel-sdw", .owner = THIS_MODULE, .bus = &sdw_bus_type, .pm = &intel_pm, <<<< }, .probe = intel_master_probe, .startup = intel_master_startup, .process_wake_event = intel_master_process_wakeen_event, .remove = intel_master_remove, };

I am not aware of any other way to do this.

Again, you can add this to platform_driver sdw_intel_drv.

I suggested a minimal way of using the 'device_driver' directly in https://github.com/thesofproject/linux/pull/1755

...

...

...

So to summarize, my recommendation would be to drop sdw_md_driver, keep sdw_master_device object and make sdw_master_device_add() hidden to driver and call it from sdw_add_bus_master() and keep intel specific startup/init routine which do same steps as they have now.

I am afraid the sequence you suggest is not technically possible: sdw_add_bus_master() requires a device to be created already, see e.g. how it's used by Qualcomm:

https://github.com/thesofproject/linux/blob/4026efd12ac983d363fb43c37a8af741...

sdw_add_bus_master() is called from the platform device probe. I cannot insert a device creation in this code.

So there are two things, lets discuss them independently. One is the device behind the controller object. I think I have given a recommendation above to keep the existing platform device for "int-sdw". This device can be PCI/ACPI/OF/Platform, we typically should not worry about it.

I don't understand how a platform device can be PCI/ACPI/OF? what did you mean here?

What I meant is that keep the existing platform device for "int-sdw".

And

a master device for everyone across x86/arm/etc spectrum would be PCI/ACPI/OF/Platform and we should be agnostic and let these drivers add a bus and register that

...

Second is we create the bus instance and that also create the sdw_master_device to represent the soundwire master device in sysfs, since this would be done by soundwire core (as I suggested), it would be independent of which type of driver is invoking this. In fact this needs to be completely independent of the driver calling this.

Intel code will call sdw_add_bus_master() and we would create a sdw_master_device for it and add the representation. So would the qcom code (btw with this, we wont need to do anything on qc code and it 'should just work (tm)'.

...

It'd actually be counter productive to add device management at the bus level, since we'd soon have incompatibilities between Intel, Qualcomm and others. What is platform-specific should be handled outside of the bus layer.

The vision I am thinking is that sdw_master_device is a representation of master for everyone independent of the platform it is being used.

Let me know if this helps

Sorry no, your reply just confused me completely.

The recommendation from Greg was "remove platform devices". Here's the quote from his May 9, 2019 reply:

"So soundwire is creating platform devices? Ick, no! Don't do that, create a "real" device that is the root of your bus and attach that to the platform/pci/whatever device that is the parent of that device's resources."

Is soundwire subsystem creating platform devices, answer is no.

The problem is we were using the master device (platform/of/pci/etc) for our own sysfs attributes, which was a mistake and I want to rectify that.

The device behind the soundwire controller can be anything. If hardware creates a device for soundwire we should continue using that

If your suggestion is to keep platform devices as currently done, then I will kindly ask that you speak with Greg and let me know what the agreement is.

The second part of your reply gets more confusing, you are suggesting that "Intel code will call sdw_add_bus_master() and we would create a sdw_master_device for it and add the representation". That is in direct contradiction with the notion of using platform devices. And it would by construction break the Qualcomm controller code just merged weeks ago. Are you suggesting I start changing Qualcomm code? Or that we add a new function that does device creation+bus addition in one shot?

And last you didn't acknowledge my two technical points as to why a driver is needed for this sdw_master_device.

So I am back to scratching my head on what the direction is? I just do not understand your line of thought.

Sorry to confuse you, I guess somewhere I missed to communicate that 1. keep existing platform device for master (that solves your pm and asoc issue) 2. add a sdw_master device, but we dont need driver, use that device for sdw sysfs attributes 3. If possible, have one api, rather than 2 for bus and master device creation.

Maybe we need to stop the long sentences and use this decision flow:

a) is the master device a platform device (Yes/No)?

For your case, yes. For QC case it will be of device

b) if No for a) then b.1) is the master device created within sdw_add_bus_master()

If possible that would be great. All I am trying here is to avoid having two APIs for registration and use a single APIs for drivers to call.

b.2) is the master device created before calling sdw_add_bus_master()

IIUC, it should be called after, right?

Can you please clarify what your desired choices are on this last paragraph?

Let me know if this helped.