From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

The use of drvdata mixes two structures. There was no harm the first structure is embedded as the first element of the second, but that's not good. Make sure all drvdata is based on the 'sdw_cdns' structure.

While we are at it, remove indirections for 'dev' and 'cdns' to make the code more readable.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 10e55c708694..ef1196ea93a4 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1077,6 +1077,7 @@ static int intel_master_probe(struct platform_device *pdev) struct sdw_cdns_stream_config config; struct device *dev = &pdev->dev; struct sdw_intel *sdw; + struct sdw_cdns *cdns; struct sdw_bus *bus; int ret;

@@ -1084,24 +1085,26 @@ static int intel_master_probe(struct platform_device *pdev) if (!sdw) return -ENOMEM;

- bus = &sdw->cdns.bus; + cdns = &sdw->cdns; + bus = &cdns->bus;

sdw->instance = pdev->id; sdw->link_res = dev_get_platdata(dev); - sdw->cdns.dev = dev; - sdw->cdns.registers = sdw->link_res->registers; - sdw->cdns.instance = sdw->instance; - sdw->cdns.msg_count = 0; + cdns->dev = dev; + cdns->registers = sdw->link_res->registers; + cdns->instance = sdw->instance; + cdns->msg_count = 0; + bus->link_id = pdev->id;

- sdw_cdns_probe(&sdw->cdns); + sdw_cdns_probe(cdns);

/* Set property read ops */ sdw_intel_ops.read_prop = intel_prop_read; bus->ops = &sdw_intel_ops;

/* set driver data, accessed by snd_soc_dai_get_drvdata() */ - platform_set_drvdata(pdev, sdw); + dev_set_drvdata(dev, cdns);

ret = sdw_bus_master_add(bus, dev, dev->fwnode); if (ret) { @@ -1123,7 +1126,7 @@ static int intel_master_probe(struct platform_device *pdev)

/* Read the PDI config and initialize cadence PDI */ intel_pdi_init(sdw, &config); - ret = sdw_cdns_pdi_init(&sdw->cdns, config); + ret = sdw_cdns_pdi_init(cdns, config); if (ret) goto err_init;

@@ -1132,20 +1135,20 @@ static int intel_master_probe(struct platform_device *pdev) /* Acquire IRQ */ ret = request_threaded_irq(sdw->link_res->irq, sdw_cdns_irq, sdw_cdns_thread, - IRQF_SHARED, KBUILD_MODNAME, &sdw->cdns); + IRQF_SHARED, KBUILD_MODNAME, cdns); if (ret < 0) { dev_err(dev, "unable to grab IRQ %d, disabling device\n", sdw->link_res->irq); goto err_init; }

- ret = sdw_cdns_enable_interrupt(&sdw->cdns, true); + ret = sdw_cdns_enable_interrupt(cdns, true); if (ret < 0) { dev_err(dev, "cannot enable interrupts\n"); goto err_init; }

- ret = sdw_cdns_exit_reset(&sdw->cdns); + ret = sdw_cdns_exit_reset(cdns); if (ret < 0) { dev_err(dev, "unable to exit bus reset sequence\n"); goto err_interrupt; @@ -1164,7 +1167,7 @@ static int intel_master_probe(struct platform_device *pdev) return 0;

err_interrupt: - sdw_cdns_enable_interrupt(&sdw->cdns, false); + sdw_cdns_enable_interrupt(cdns, false); free_irq(sdw->link_res->irq, sdw); err_init: sdw_bus_master_delete(bus); @@ -1174,16 +1177,13 @@ static int intel_master_probe(struct platform_device *pdev) static int intel_master_remove(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct sdw_intel *sdw; - struct sdw_bus *bus; - - sdw = platform_get_drvdata(pdev); - - bus = &sdw->cdns.bus; + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus;

if (!bus->prop.hw_disabled) { intel_debugfs_exit(sdw); - sdw_cdns_enable_interrupt(&sdw->cdns, false); + sdw_cdns_enable_interrupt(cdns, false); free_irq(sdw->link_res->irq, sdw); snd_soc_unregister_component(dev); }

From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Make error handling simpler with devm_ allocation.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel_init.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index e87f17240547..f9d190157c47 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -42,9 +42,6 @@ static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) link++; }

- kfree(ctx->links); - ctx->links = NULL; - return 0; }

@@ -96,14 +93,15 @@ static struct sdw_intel_ctx

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

- ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); + ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL;

ctx->count = count; - ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL); + ctx->links = devm_kcalloc(&adev->dev, ctx->count, + sizeof(*ctx->links), GFP_KERNEL); if (!ctx->links) - goto link_err; + return NULL;

link = ctx->links;

@@ -146,9 +144,8 @@ static struct sdw_intel_ctx return ctx;

pdev_err: + ctx->count = i; sdw_intel_cleanup_pdev(ctx); -link_err: - kfree(ctx); return NULL; }

@@ -216,7 +213,6 @@ void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res) void sdw_intel_exit(struct sdw_intel_ctx *ctx) { sdw_intel_cleanup_pdev(ctx); - kfree(ctx); } EXPORT_SYMBOL(sdw_intel_exit);

From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Rather than a plain-vanilla init/exit, this patch provides 3 steps in the initialization needed for driver selection, machine driver selection and deal with power rail dependencies.

- ACPI scan: this step is done at a very early stage to detect the presence of a SoundWire Controller and enabled links at the BIOS level. This step may be called from the legacy HDaudio driver, which will abort its probe to let the Sound Open Firmware (SOF) handle the hardware.

- probe: this step allocates all the required memory and will add a sdw_bus, which in turn will result in identifying all possible Slaves listed below the Controller ACPI companion device. All the information is reported to the parent PCI driver which will select the relevant machine driver.

- startup: this last step starts the bus reset, which results in Slave devices reporting as ATTACHED and being enumerated. This step is only done during the card creation stage, after the DSP is powered to account for internal power rail dependencies.

These 3 steps are already supported in the Sound Open firmware drivers and upstream.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 60 +++++--- drivers/soundwire/intel.h | 2 + drivers/soundwire/intel_init.c | 242 ++++++++++++++++++++++++++------- 3 files changed, 237 insertions(+), 67 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ef1196ea93a4..f966ac33cd3b 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1074,7 +1074,6 @@ static int intel_init(struct sdw_intel *sdw) */ static int intel_master_probe(struct platform_device *pdev) { - struct sdw_cdns_stream_config config; struct device *dev = &pdev->dev; struct sdw_intel *sdw; struct sdw_cdns *cdns; @@ -1112,25 +1111,10 @@ static int intel_master_probe(struct platform_device *pdev) return ret; }

- if (bus->prop.hw_disabled) { + if (bus->prop.hw_disabled) dev_info(dev, "SoundWire master %d is disabled, will be ignored\n", bus->link_id); - return 0; - } - - /* Initialize shim, controller and Cadence IP */ - ret = intel_init(sdw); - if (ret) - goto err_init; - - /* Read the PDI config and initialize cadence PDI */ - intel_pdi_init(sdw, &config); - ret = sdw_cdns_pdi_init(cdns, config); - if (ret) - goto err_init; - - intel_pdi_ch_update(sdw);

/* Acquire IRQ */ ret = request_threaded_irq(sdw->link_res->irq, @@ -1142,6 +1126,42 @@ static int intel_master_probe(struct platform_device *pdev) goto err_init; }

+ return 0; + +err_init: + sdw_bus_master_delete(bus); + return ret; +} + +int intel_master_startup(struct platform_device *pdev) +{ + struct sdw_cdns_stream_config config; + struct device *dev = &pdev->dev; + struct sdw_cdns *cdns = dev_get_drvdata(dev); + struct sdw_intel *sdw = cdns_to_intel(cdns); + struct sdw_bus *bus = &cdns->bus; + int ret; + + if (bus->prop.hw_disabled) { + dev_info(dev, + "SoundWire master %d is disabled, ignoring\n", + sdw->instance); + return 0; + } + + /* Initialize shim, controller and Cadence IP */ + ret = intel_init(sdw); + if (ret) + goto err_init; + + /* Read the PDI config and initialize cadence PDI */ + intel_pdi_init(sdw, &config); + ret = sdw_cdns_pdi_init(cdns, config); + if (ret) + goto err_init; + + intel_pdi_ch_update(sdw); + ret = sdw_cdns_enable_interrupt(cdns, true); if (ret < 0) { dev_err(dev, "cannot enable interrupts\n"); @@ -1168,9 +1188,7 @@ static int intel_master_probe(struct platform_device *pdev)

err_interrupt: sdw_cdns_enable_interrupt(cdns, false); - free_irq(sdw->link_res->irq, sdw); err_init: - sdw_bus_master_delete(bus); return ret; }

@@ -1196,12 +1214,12 @@ static struct platform_driver sdw_intel_drv = { .probe = intel_master_probe, .remove = intel_master_remove, .driver = { - .name = "int-sdw", + .name = "intel-sdw", }, };

module_platform_driver(sdw_intel_drv);

MODULE_LICENSE("Dual BSD/GPL"); -MODULE_ALIAS("platform:int-sdw"); +MODULE_ALIAS("platform:intel-sdw"); MODULE_DESCRIPTION("Intel Soundwire Master Driver"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 001968c362cd..1ff71eb4c219 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -38,4 +38,6 @@ struct sdw_intel {

#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1)

+int intel_master_startup(struct platform_device *pdev); + #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index ff1017e64abd..41c06e9bcecb 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -24,73 +24,146 @@ #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; + u32 link_mask; int i;

if (!link) return 0;

- for (i = 0; i < ctx->count; i++) { + link_mask = ctx->link_mask; + + for (i = 0; i < ctx->count; i++, link++) { + if (!(link_mask & BIT(i))) + continue; + if (link->pdev) platform_device_unregister(link->pdev); - link++; }

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; }

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; + info->link_mask = 0; + + 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 platform_device_info pdevinfo; + struct platform_device *pdev; + struct sdw_intel_link_res *link; + struct sdw_intel_ctx *ctx; + struct acpi_device *adev; + 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 = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL); @@ -103,19 +176,25 @@ static struct sdw_intel_ctx if (!ctx->links) return NULL;

+ 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++) { + for (i = 0; i < count; i++, link++) { if (!(link_mask & BIT(i))) { dev_dbg(&adev->dev, "Link %d masked, will not be enabled\n", i); - link++; continue; }

+ 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;

@@ -125,7 +204,7 @@ static struct sdw_intel_ctx memset(&pdevinfo, 0, sizeof(pdevinfo));

pdevinfo.parent = res->parent; - pdevinfo.name = "int-sdw"; + pdevinfo.name = "intel-sdw"; pdevinfo.id = i; pdevinfo.fwnode = acpi_fwnode_handle(adev); pdevinfo.data = link; @@ -136,25 +215,63 @@ static struct sdw_intel_ctx dev_err(&adev->dev, "platform device creation failed: %ld\n", PTR_ERR(pdev)); - goto pdev_err; + goto err; } - link->pdev = pdev; - link++; }

return ctx;

-pdev_err: +err: ctx->count = i; - sdw_intel_cleanup_pdev(ctx); + sdw_intel_cleanup(ctx); return NULL; }

+static int +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) +{ + struct acpi_device *adev; + struct sdw_intel_link_res *link; + 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; + + /* Startup SDW Master devices */ + for (i = 0; i < ctx->count; i++, link++) { + if (!(link_mask & BIT(i))) + continue; + + intel_master_startup(link->pdev); + } + + 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; @@ -168,7 +285,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 @@ -185,36 +302,69 @@ 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 + * @res: resource data + * + * This registers a platform device for each Master handled by the controller, + * and SoundWire Master and Slave devices will be created by the platform + * device probe. All the information necessary is stored in the context, and + * the res argument pointer can be freed after this step. + * This function will be called after sdw_intel_acpi_scan() by SOF probe. + */ +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 + * + * Startup Intel SoundWire controller. This function will be called after + * Intel Audio DSP is powered up. + */ +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); } EXPORT_SYMBOL(sdw_intel_exit);