| Message ID | 20191217210314.20410-10-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: implement new ASoC interfaces | expand |
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..?
>> +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). > 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, 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. 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
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.
>>>> + /* 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? > >> 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, }; 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 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.
>>> 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? 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. >>>> 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.
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 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. > >>>>>> 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.
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 64f97bb1a135..36dbcbab0d65 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -92,8 +92,6 @@ #define SDW_ALH_STRMZCFG_DMAT GENMASK(7, 0) #define SDW_ALH_STRMZCFG_CHN GENMASK(19, 16) -#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) - enum intel_pdi_type { INTEL_PDI_IN = 0, INTEL_PDI_OUT = 1, @@ -923,24 +921,23 @@ static int intel_init(struct sdw_intel *sdw) /* * probe and init */ -static int intel_probe(struct platform_device *pdev) +static int intel_master_probe(struct sdw_master_device *md, void *link_ctx) { - struct sdw_cdns_stream_config config; struct sdw_intel *sdw; int ret; - sdw = devm_kzalloc(&pdev->dev, sizeof(*sdw), GFP_KERNEL); + sdw = devm_kzalloc(&md->dev, sizeof(*sdw), GFP_KERNEL); if (!sdw) return -ENOMEM; - sdw->instance = pdev->id; - sdw->link_res = dev_get_platdata(&pdev->dev); - sdw->cdns.dev = &pdev->dev; + sdw->instance = md->link_id; + sdw->link_res = link_ctx; + sdw->cdns.dev = &md->dev; sdw->cdns.registers = sdw->link_res->registers; - sdw->cdns.instance = sdw->instance; + sdw->cdns.instance = md->link_id; sdw->cdns.msg_count = 0; - sdw->cdns.bus.dev = &pdev->dev; - sdw->cdns.bus.link_id = pdev->id; + sdw->cdns.bus.dev = &md->dev; + sdw->cdns.bus.link_id = md->link_id; sdw_cdns_probe(&sdw->cdns); @@ -948,16 +945,50 @@ static int intel_probe(struct platform_device *pdev) sdw_intel_ops.read_prop = intel_prop_read; sdw->cdns.bus.ops = &sdw_intel_ops; - platform_set_drvdata(pdev, sdw); + md->pdata = sdw; + + /* set driver data, accessed by snd_soc_dai_set_drvdata() */ + dev_set_drvdata(&md->dev, &sdw->cdns); ret = sdw_add_bus_master(&sdw->cdns.bus); if (ret) { - dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret); + dev_err(&md->dev, "sdw_add_bus_master fail: %d\n", ret); return ret; } if (sdw->cdns.bus.prop.hw_disabled) { - dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n", + dev_info(&md->dev, "SoundWire master %d is disabled, ignoring\n", + sdw->cdns.bus.link_id); + return 0; + } + + /* Acquire IRQ */ + ret = request_threaded_irq(sdw->link_res->irq, + sdw_cdns_irq, sdw_cdns_thread, + IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); + if (ret < 0) { + dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", + sdw->link_res->irq); + goto err_init; + } + + return 0; + +err_init: + sdw_delete_bus_master(&sdw->cdns.bus); + return ret; +} + +static int intel_master_startup(struct sdw_master_device *md) +{ + struct sdw_cdns_stream_config config; + struct sdw_intel *sdw; + int ret; + + sdw = md->pdata; + + if (sdw->cdns.bus.prop.hw_disabled) { + dev_info(&md->dev, "SoundWire master %d is disabled, ignoring\n", sdw->cdns.bus.link_id); return 0; } @@ -975,16 +1006,6 @@ static int intel_probe(struct platform_device *pdev) intel_pdi_ch_update(sdw); - /* Acquire IRQ */ - ret = request_threaded_irq(sdw->link_res->irq, - sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); - if (ret < 0) { - dev_err(sdw->cdns.dev, "unable to grab IRQ %d, disabling device\n", - sdw->link_res->irq); - goto err_init; - } - ret = sdw_cdns_enable_interrupt(&sdw->cdns, true); if (ret < 0) { dev_err(sdw->cdns.dev, "cannot enable interrupts\n"); @@ -1011,17 +1032,17 @@ static int intel_probe(struct platform_device *pdev) err_interrupt: sdw_cdns_enable_interrupt(&sdw->cdns, false); - free_irq(sdw->link_res->irq, sdw); err_init: + free_irq(sdw->link_res->irq, sdw); sdw_delete_bus_master(&sdw->cdns.bus); return ret; } -static int intel_remove(struct platform_device *pdev) +static int intel_master_remove(struct sdw_master_device *md) { struct sdw_intel *sdw; - sdw = platform_get_drvdata(pdev); + sdw = md->pdata; if (!sdw->cdns.bus.prop.hw_disabled) { intel_debugfs_exit(sdw); @@ -1031,19 +1052,17 @@ static int intel_remove(struct platform_device *pdev) } sdw_delete_bus_master(&sdw->cdns.bus); + device_unregister(&md->dev); + return 0; } -static struct platform_driver sdw_intel_drv = { - .probe = intel_probe, - .remove = intel_remove, - .driver = { - .name = "int-sdw", - - }, +struct sdw_md_driver intel_sdw_driver = { + .probe = intel_master_probe, + .startup = intel_master_startup, + .remove = intel_master_remove, }; - -module_platform_driver(sdw_intel_drv); +EXPORT_SYMBOL(intel_sdw_driver); MODULE_LICENSE("Dual BSD/GPL"); MODULE_ALIAS("platform:int-sdw"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 38b7c125fb10..cfab2f00214d 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -7,7 +7,7 @@ /** * struct sdw_intel_link_res - Soundwire Intel link resource structure, * typically populated by the controller driver. - * @pdev: platform_device + * @md: master device * @mmio_base: mmio base of SoundWire registers * @registers: Link IO registers base * @shim: Audio shim pointer @@ -17,7 +17,7 @@ * @dev: device implementing hw_params and free callbacks */ struct sdw_intel_link_res { - struct platform_device *pdev; + struct sdw_master_device *md; void __iomem *mmio_base; /* not strictly needed, useful for debug */ void __iomem *registers; void __iomem *shim; @@ -27,4 +27,8 @@ struct sdw_intel_link_res { struct device *dev; }; +#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1) + +extern struct sdw_md_driver intel_sdw_driver; + #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 4b769409f6f8..42f7ae034bea 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -11,7 +11,7 @@ #include <linux/export.h> #include <linux/io.h> #include <linux/module.h> -#include <linux/platform_device.h> +#include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_intel.h> #include "intel.h" @@ -23,22 +23,47 @@ #define SDW_LINK_BASE 0x30000 #define SDW_LINK_SIZE 0x10000 -static int link_mask; -module_param_named(sdw_link_mask, link_mask, int, 0444); +static int ctrl_link_mask; +module_param_named(sdw_link_mask, ctrl_link_mask, int, 0444); MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)"); -static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) +static bool is_link_enabled(struct fwnode_handle *fw_node, int i) +{ + struct fwnode_handle *link; + char name[32]; + u32 quirk_mask = 0; + + /* Find master handle */ + snprintf(name, sizeof(name), + "mipi-sdw-link-%d-subproperties", i); + + link = fwnode_get_named_child_node(fw_node, name); + if (!link) + return false; + + fwnode_property_read_u32(link, + "intel-quirk-mask", + &quirk_mask); + + if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE) + return false; + + return true; +} + +static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) { struct sdw_intel_link_res *link = ctx->links; + struct sdw_master_device *md; int i; if (!link) return 0; - for (i = 0; i < ctx->count; i++) { - if (link->pdev) - platform_device_unregister(link->pdev); - link++; + for (i = 0; i < ctx->count; i++, link++) { + md = link->md; + if (md) + md->driver->remove(md); } kfree(ctx->links); @@ -47,112 +72,194 @@ static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) return 0; } -static struct sdw_intel_ctx -*sdw_intel_add_controller(struct sdw_intel_res *res) +static int +sdw_intel_scan_controller(struct sdw_intel_acpi_info *info) { - struct platform_device_info pdevinfo; - struct platform_device *pdev; - struct sdw_intel_link_res *link; - struct sdw_intel_ctx *ctx; struct acpi_device *adev; int ret, i; u8 count; - u32 caps; - if (acpi_bus_get_device(res->handle, &adev)) - return NULL; + if (acpi_bus_get_device(info->handle, &adev)) + return -EINVAL; /* Found controller, find links supported */ count = 0; ret = fwnode_property_read_u8_array(acpi_fwnode_handle(adev), "mipi-sdw-master-count", &count, 1); - /* Don't fail on error, continue and use hw value */ + /* + * In theory we could check the number of links supported in + * hardware, but in that step we cannot assume SoundWire IP is + * powered. + * + * In addition, if the BIOS doesn't even provide this + * 'master-count' property then all the inits based on link + * masks will fail as well. + * + * We will check the hardware capabilities in the startup() step + */ + if (ret) { dev_err(&adev->dev, "Failed to read mipi-sdw-master-count: %d\n", ret); - count = SDW_MAX_LINKS; + return -EINVAL; } - /* Check SNDWLCAP.LCOUNT */ - caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); - caps &= GENMASK(2, 0); - - /* Check HW supported vs property value and use min of two */ - count = min_t(u8, caps, count); - /* Check count is within bounds */ if (count > SDW_MAX_LINKS) { dev_err(&adev->dev, "Link count %d exceeds max %d\n", count, SDW_MAX_LINKS); - return NULL; + return -EINVAL; } else if (!count) { dev_warn(&adev->dev, "No SoundWire links detected\n"); - return NULL; + return -EINVAL; } + dev_dbg(&adev->dev, "ACPI reports %d SDW Link devices\n", count); + + info->count = count; + for (i = 0; i < count; i++) { + if (ctrl_link_mask && !(ctrl_link_mask & BIT(i))) { + dev_dbg(&adev->dev, + "Link %d masked, will not be enabled\n", i); + continue; + } + + if (!is_link_enabled(acpi_fwnode_handle(adev), i)) { + dev_dbg(&adev->dev, + "Link %d not selected in firmware\n", i); + continue; + } + + info->link_mask |= BIT(i); + } + + return 0; +} + +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); } return ctx; -pdev_err: - sdw_intel_cleanup_pdev(ctx); +err: + sdw_intel_cleanup(ctx); link_err: kfree(ctx); return NULL; } +static int +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) +{ + struct acpi_device *adev; + struct sdw_intel_link_res *link; + struct sdw_master_device *md; + u32 caps; + u32 link_mask; + int i; + + if (acpi_bus_get_device(ctx->handle, &adev)) + return -EINVAL; + + /* Check SNDWLCAP.LCOUNT */ + caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); + caps &= GENMASK(2, 0); + + /* Check HW supported vs property value */ + if (caps < ctx->count) { + dev_err(&adev->dev, + "BIOS master count is larger than hardware capabilities\n"); + return -EINVAL; + } + + if (!ctx->links) + return -EINVAL; + + link = ctx->links; + link_mask = ctx->link_mask; + + /* Create SDW Master devices */ + for (i = 0; i < ctx->count; i++, link++) { + if (link_mask && !(link_mask & BIT(i))) + continue; + + md = link->md; + + md->driver->startup(md); + } + + return 0; +} + static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, void *cdata, void **return_value) { - struct sdw_intel_res *res = cdata; + struct sdw_intel_acpi_info *info = cdata; struct acpi_device *adev; acpi_status status; u64 adr; @@ -166,7 +273,7 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, return AE_NOT_FOUND; } - res->handle = handle; + info->handle = handle; /* * On some Intel platforms, multiple children of the HDAS @@ -183,36 +290,66 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, } /** - * sdw_intel_init() - SoundWire Intel init routine + * sdw_intel_acpi_scan() - SoundWire Intel init routine * @parent_handle: ACPI parent handle - * @res: resource data + * @info: description of what firmware/DSDT tables expose * - * This scans the namespace and creates SoundWire link controller devices - * based on the info queried. + * This scans the namespace and queries firmware to figure out which + * links to enable. A follow-up use of sdw_intel_probe() and + * sdw_intel_startup() is required for creation of devices and bus + * startup */ -void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res) +int sdw_intel_acpi_scan(acpi_handle *parent_handle, + struct sdw_intel_acpi_info *info) { acpi_status status; status = acpi_walk_namespace(ACPI_TYPE_DEVICE, parent_handle, 1, sdw_intel_acpi_cb, - NULL, res, NULL); + NULL, info, NULL); if (ACPI_FAILURE(status)) - return NULL; + return -ENODEV; - return sdw_intel_add_controller(res); + return sdw_intel_scan_controller(info); } +EXPORT_SYMBOL(sdw_intel_acpi_scan); +/** + * sdw_intel_probe() - SoundWire Intel probe routine + * @parent_handle: ACPI parent handle + * @res: resource data + * + * This creates SoundWire Master and Slave devices below the controller. + * All the information necessary is stored in the context, and the res + * argument pointer can be freed after this step. + */ +struct sdw_intel_ctx +*sdw_intel_probe(struct sdw_intel_res *res) +{ + return sdw_intel_probe_controller(res); +} +EXPORT_SYMBOL(sdw_intel_probe); + +/** + * sdw_intel_startup() - SoundWire Intel startup + * @ctx: SoundWire context allocated in the probe + * + */ +int sdw_intel_startup(struct sdw_intel_ctx *ctx) +{ + return sdw_intel_startup_controller(ctx); +} +EXPORT_SYMBOL(sdw_intel_startup); /** * sdw_intel_exit() - SoundWire Intel exit - * @arg: callback context + * @ctx: SoundWire context allocated in the probe * * Delete the controller instances created and cleanup */ void sdw_intel_exit(struct sdw_intel_ctx *ctx) { - sdw_intel_cleanup_pdev(ctx); + sdw_intel_cleanup(ctx); kfree(ctx); } EXPORT_SYMBOL(sdw_intel_exit);