Re: [PATCH] soundwire: stream: fix bad unlock balance
On Thu, May 23, 2019 at 09:43:14AM +0100, Srinivas Kandagatla wrote: > > > On 22/05/2019 17:41, Pierre-Louis Bossart wrote: > > > > > > On 5/22/19 11:25 AM, Srinivas Kandagatla wrote: > > > This patch fixes below warning due to unlocking without locking. > > > > > > ?? = > > > ?? WARNING: bad unlock balance detected! > > > ?? 5.1.0-16506-gc1c383a6f0a2-dirty #1523 Tainted: G?? W > > > ?? - > > > ?? aplay/2954 is trying to release lock (&bus->msg_lock) at: > > > ?? do_bank_switch+0x21c/0x480 > > > ?? but there are no more locks to release! > > > > > > Signed-off-by: Srinivas Kandagatla > > > --- > > > ?? drivers/soundwire/stream.c | 3 ++- > > > ?? 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > > > index 544925ff0b40..d16268f30e4f 100644 > > > --- a/drivers/soundwire/stream.c > > > +++ b/drivers/soundwire/stream.c > > > @@ -814,7 +814,8 @@ static int do_bank_switch(struct > > > sdw_stream_runtime *stream) > > > ?? goto error; > > > ?? } > > > -?? mutex_unlock(&bus->msg_lock); > > > +?? if (mutex_is_locked(&bus->msg_lock)) > > > +?? utex_unlock(&bus->msg_lock); > > > > Does this even compile? should be mutex_unlock, no? > > > > We also may want to identify the issue in more details without pushing > > it under the rug. The locking mechanism is far from simple and it's > > likely there are a number of problems with it. > > > msg_lock is taken conditionally during multi link bank switch cases, however > the unlock is done unconditionally leading to this warning. > > Having a closer look show that there could be a dead lock in this path while > executing sdw_transfer(). And infact there is no need to take msg_lock in > multi link switch cases as sdw_transfer should take care of this. > > Vinod/Sanyog any reason why msg_lock is really required in this path? > In case of multi link we use sdw_transfer_defer instead of sdw_transfer where lock is not acquired, hence lock is acquired in do_bank_switch for multi link. we should add same check of multi link to release lock in do_bank_switch. > --srini > > > > ?? } > > > ?? return ret; > > > --
Re: [PATCH] soundwire: stream: fix bad unlock balance
On Thu, May 23, 2019 at 10:30:20AM +0100, Srinivas Kandagatla wrote:
>
>
> On 23/05/2019 10:20, Sanyog Kale wrote:
> > On Thu, May 23, 2019 at 09:43:14AM +0100, Srinivas Kandagatla wrote:
> > >
> > >
> > > On 22/05/2019 17:41, Pierre-Louis Bossart wrote:
> > > >
> > > >
> > > > On 5/22/19 11:25 AM, Srinivas Kandagatla wrote:
> > > > > This patch fixes below warning due to unlocking without locking.
> > > > >
> > > > > ?? =
> > > > > ?? WARNING: bad unlock balance detected!
> > > > > ?? 5.1.0-16506-gc1c383a6f0a2-dirty #1523 Tainted: G?? W
> > > > > ?? -
> > > > > ?? aplay/2954 is trying to release lock (&bus->msg_lock) at:
> > > > > ?? do_bank_switch+0x21c/0x480
> > > > > ?? but there are no more locks to release!
> > > > >
> > > > > Signed-off-by: Srinivas Kandagatla
> > > > > ---
> > > > > ?? drivers/soundwire/stream.c | 3 ++-
> > > > > ?? 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> > > > > index 544925ff0b40..d16268f30e4f 100644
> > > > > --- a/drivers/soundwire/stream.c
> > > > > +++ b/drivers/soundwire/stream.c
> > > > > @@ -814,7 +814,8 @@ static int do_bank_switch(struct
> > > > > sdw_stream_runtime *stream)
> > > > > ?? goto error;
> > > > > ?? }
> > > > > -?? mutex_unlock(&bus->msg_lock);
> > > > > +?? if (mutex_is_locked(&bus->msg_lock))
> > > > > +?? utex_unlock(&bus->msg_lock);
> > > >
> > > > Does this even compile? should be mutex_unlock, no?
> > > >
> > > > We also may want to identify the issue in more details without pushing
> > > > it under the rug. The locking mechanism is far from simple and it's
> > > > likely there are a number of problems with it.
> > > >
> > > msg_lock is taken conditionally during multi link bank switch cases,
> > > however
> > > the unlock is done unconditionally leading to this warning.
> > >
> > > Having a closer look show that there could be a dead lock in this path
> > > while
> > > executing sdw_transfer(). And infact there is no need to take msg_lock in
> > > multi link switch cases as sdw_transfer should take care of this.
> > >
> > > Vinod/Sanyog any reason why msg_lock is really required in this path?
> > >
> >
> > In case of multi link we use sdw_transfer_defer instead of sdw_transfer
> > where lock is not acquired, hence lock is acquired in do_bank_switch for
> > multi link. we should add same check of multi link to release lock in
> > do_bank_switch.
>
> probably we should just add the lock around the sdw_transfer_defer call in
> sdw_bank_switch()?
> This should cleanup the code a bit too.
>
> something like:
>
> >cut<-
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index d01060dbee96..f455af5b8151 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -676,10 +676,13 @@ static int sdw_bank_switch(struct sdw_bus *bus, int
> m_rt_count)
> */
> multi_link = bus->multi_link && (m_rt_count > 1);
>
> - if (multi_link)
> + if (multi_link) {
> + mutex_lock(&bus->msg_lock);
> ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg);
> - else
> + mutex_unlock(&bus->msg_lock);
you cant release bus_lock here since message is not yet transferred.
we can only release bus_lock after sdw_ml_sync_bank_switch function where
we confirm that message transfer is completed.
> + } else {
> ret = sdw_transfer(bus, wr_msg);
> + }
>
> if (ret < 0) {
> dev_err(bus->dev, "Slave frame_ctrl reg write failed\n");
> @@ -742,25 +745,19 @@ static int do_bank_switch(struct sdw_stream_runtime
> *stream)
> struct sdw_master_runtime *m_rt = NULL;
> const struct sdw_master_ops *ops;
> struct sdw_bus *bus = NULL;
> - bool multi_link = false;
>
Re: [alsa-devel] [PATCH v2 0/3] soundwire: debugfs support for 5.4
On Mon, Aug 12, 2019 at 06:59:39PM -0500, Pierre-Louis Bossart wrote:
> This patchset enables debugfs support and corrects all the feedback
> provided on an earlier RFC ('soundwire: updates for 5.4')
>
> There is one remaining hard-coded value in intel.c that will need to
> be fixed in a follow-up patchset not specific to debugfs: we need to
> remove hard-coded Intel-specific configurations from cadence_master.c
> (PDI offsets, etc).
>
> Changes since v1 (Feedback from GKH)
> Handle debugfs in a more self-contained way (no dentry as return or parameter)
> Used CONFIG_DEBUG_FS in structures and code to make it easier to
> remove if need be.
> No functional change for register dumps.
>
> Changes since RFC (Feedback from GKH, Vinod, Guennadi, Cezary, Sanyog):
> removed error checks
> used DEFINE_SHOW_ATTRIBUTE and seq_file
> fixed copyright dates
> fixed SPDX license info to use GPL2.0 only
> fixed Makefile to include debugfs only if CONFIG_DEBUG_FS is selected
> used static inlines for fallback compilation
> removed intermediate variables
> removed hard-coded constants in loops (used registers offsets and
> hardware capabilities)
> squashed patch 3
>
Changes looks good to me.
Acked-by: Sanyog Kale
> ___
> Alsa-devel mailing list
> alsa-de...@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
Re: [RFC PATCH 02/40] soundwire: cadence_master: add debugfs register dump
On Thu, Jul 25, 2019 at 06:39:54PM -0500, Pierre-Louis Bossart wrote:
> Add debugfs file to dump the Cadence master registers
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
> is the use of scnprintf to avoid known issues with snprintf.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/cadence_master.c | 98 ++
> drivers/soundwire/cadence_master.h | 2 +
> 2 files changed, 100 insertions(+)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index ff4badc9b3de..91e8bacb83e3 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -8,6 +8,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -223,6 +224,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int
> offset, u32 value)
> return -EAGAIN;
> }
>
> +/*
> + * debugfs
> + */
> +
> +#define RD_BUF (2 * PAGE_SIZE)
> +
> +static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
> + char *buf, size_t pos, unsigned int reg)
> +{
> + return scnprintf(buf + pos, RD_BUF - pos,
> + "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
> +}
> +
> +static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct sdw_cdns *cdns = file->private_data;
> + char *buf;
> + ssize_t ret;
> + int i, j;
> +
> + buf = kzalloc(RD_BUF, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = scnprintf(buf, RD_BUF, "Register Value\n");
> + ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
> + for (i = 0; i < 8; i++) /* 8 MCP registers */
> + ret += cdns_sprintf(cdns, buf, ret, i * 4);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nStatus & Intr Registers\n");
> + for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nSSP & Clk ctrl Registers\n");
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nDPn B0 Registers\n");
> + for (i = 0; i < 7; i++) {
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nDP-%d\n", i);
> + for (j = 0; j < 6; j++)
> + ret += cdns_sprintf(cdns, buf, ret,
> + CDNS_DPN_B0_CONFIG(i) + j * 4);
> + }
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nDPn B1 Registers\n");
> + for (i = 0; i < 7; i++) {
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nDP-%d\n", i);
> +
> + for (j = 0; j < 6; j++)
> + ret += cdns_sprintf(cdns, buf, ret,
> + CDNS_DPN_B1_CONFIG(i) + j * 4);
> + }
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nDPn Control Registers\n");
> + for (i = 0; i < 7; i++)
> + ret += cdns_sprintf(cdns, buf, ret,
> + CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
> +
> + ret += scnprintf(buf + ret, RD_BUF - ret,
> + "\nPDIn Config Registers\n");
> + for (i = 0; i < 7; i++)
please use macros for all the hardcodings.
> + ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
> +
> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> + kfree(buf);
> +
> + return ret;
> +}
> +
> +static const struct file_operations cdns_reg_fops = {
> + .open = simple_open,
> + .read = cdns_reg_read,
> + .llseek = default_llseek,
> +};
> +
> +/**
> + * sdw_cdns_debugfs_init() - Cadence debugfs init
> + * @cdns: Cadence instance
> + * @root: debugfs root
> + */
> +void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
> +{
> + debugfs_create_file("cdns-registers",
Re: [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
On Thu, Jul 25, 2019 at 06:40:01PM -0500, Pierre-Louis Bossart wrote:
> Per the hardware documentation, all changes to MCP_CONFIG,
> MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a
> self-clearing write to MCP_CONFIG_UPDATE.
>
> For some reason, the existing code only does this write to
> CONFIG_UPDATE when enabling interrupts. Add a helper and do the update
> when the CONFIG is changed.
>
the sequence of intel_probe is as follows:
1. intel_link_power_up
2. intel_shim_init
3. sdw_cdns_init
4. sdw_cdns_enable_interrupt
Since we do self-clearing write to MCP_CONFIG_UPDATE in
sdw_cdns_enable_interrupt once for all the config changes,
we dont perform it as part of sdw_cdns_init.
It does make sense to seperate it out from sdw_cdns_enable_interrupt so
that we can use when clockstop is enabled where we dont need to enable
interrupts.
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/cadence_master.c | 29 +
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index 9f611a1fff0a..eb46cf651d62 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int
> offset, u32 value)
> return -EAGAIN;
> }
>
> +/*
> + * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL
> + * need to be confirmed with a write to MCP_CONFIG_UPDATE
> + */
> +static int cdns_update_config(struct sdw_cdns *cdns)
> +{
> + int ret;
> +
> + ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
> + CDNS_MCP_CONFIG_UPDATE_BIT);
> + if (ret < 0)
> + dev_err(cdns->dev, "Config update timedout\n");
> +
> + return ret;
> +}
> +
> /*
> * debugfs
> */
> @@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
> */
> int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> {
> - int ret;
> -
> _cdns_enable_interrupt(cdns);
> - ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
> - CDNS_MCP_CONFIG_UPDATE_BIT);
> - if (ret < 0)
> - dev_err(cdns->dev, "Config update timedout\n");
>
> - return ret;
> + return 0;
> }
> EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
>
> @@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>
> cdns_writel(cdns, CDNS_MCP_CONFIG, val);
>
> - return 0;
> + /* commit changes */
> + ret = cdns_update_config(cdns);
> +
> + return ret;
> }
> EXPORT_SYMBOL(sdw_cdns_init);
>
> --
> 2.20.1
>
--
Re: [RFC PATCH 21/40] soundwire: export helpers to find row and column values
On Thu, Jul 25, 2019 at 06:40:13PM -0500, Pierre-Louis Bossart wrote:
> Add a prefix for common tables and export 2 helpers to set the frame
> shapes based on row/col values.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/bus.h| 7 +--
> drivers/soundwire/stream.c | 14 --
> 2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 06ac4adb0074..c57c9c23f6ca 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -73,8 +73,11 @@ struct sdw_msg {
>
> #define SDW_DOUBLE_RATE_FACTOR 2
>
> -extern int rows[SDW_FRAME_ROWS];
> -extern int cols[SDW_FRAME_COLS];
> +extern int sdw_rows[SDW_FRAME_ROWS];
> +extern int sdw_cols[SDW_FRAME_COLS];
> +
> +int sdw_find_row_index(int row);
> +int sdw_find_col_index(int col);
We use index values only in bank switch operations to program registers. Do we
really need to export sdw_find_row_index & sdw_find_col_index?? If i understand
correctly the allocation algorithm only needs to know about cols and rows values
and not index.
>
> /**
> * sdw_port_runtime: Runtime port parameters for Master or Slave
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index a0476755a459..53f5e790fcd7 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -21,37 +21,39 @@
> * The rows are arranged as per the array index value programmed
> * in register. The index 15 has dummy value 0 in order to fill hole.
> */
> -int rows[SDW_FRAME_ROWS] = {48, 50, 60, 64, 75, 80, 125, 147,
> +int sdw_rows[SDW_FRAME_ROWS] = {48, 50, 60, 64, 75, 80, 125, 147,
> 96, 100, 120, 128, 150, 160, 250, 0,
> 192, 200, 240, 256, 72, 144, 90, 180};
>
> -int cols[SDW_FRAME_COLS] = {2, 4, 6, 8, 10, 12, 14, 16};
> +int sdw_cols[SDW_FRAME_COLS] = {2, 4, 6, 8, 10, 12, 14, 16};
>
> -static int sdw_find_col_index(int col)
> +int sdw_find_col_index(int col)
> {
> int i;
>
> for (i = 0; i < SDW_FRAME_COLS; i++) {
> - if (cols[i] == col)
> + if (sdw_cols[i] == col)
> return i;
> }
>
> pr_warn("Requested column not found, selecting lowest column no: 2\n");
> return 0;
> }
> +EXPORT_SYMBOL(sdw_find_col_index);
>
> -static int sdw_find_row_index(int row)
> +int sdw_find_row_index(int row)
> {
> int i;
>
> for (i = 0; i < SDW_FRAME_ROWS; i++) {
> - if (rows[i] == row)
> + if (sdw_rows[i] == row)
> return i;
> }
>
> pr_warn("Requested row not found, selecting lowest row no: 48\n");
> return 0;
> }
> +EXPORT_SYMBOL(sdw_find_row_index);
>
> static int _sdw_program_slave_port_params(struct sdw_bus *bus,
> struct sdw_slave *slave,
> --
> 2.20.1
>
--
Re: [RFC PATCH 23/40] soundwire: stream: fix disable sequence
On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote:
> When we disable the stream and then call hw_free, two bank switches
> will be handled and as a result we re-enable the stream on hw_free.
>
I didnt quite get why there will be two bank switches as part of disable flow
which leads to enabling of stream?
> Make sure the stream is disabled on both banks.
>
> TODO: we need to completely revisit all this and make sure we have a
> mirroring mechanism between current and alternate banks.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/stream.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 53f5e790fcd7..75b9ad1fb1a6 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct
> sdw_stream_runtime *stream)
> }
> }
>
> - return do_bank_switch(stream);
> + ret = do_bank_switch(stream);
> + if (ret < 0) {
> + dev_err(bus->dev, "Bank switch failed: %d\n", ret);
> + return ret;
> + }
> +
> + /* make sure alternate bank (previous current) is also disabled */
> + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> + bus = m_rt->bus;
> + /* Disable port(s) */
> + ret = sdw_enable_disable_ports(m_rt, false);
> + if (ret < 0) {
> + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> }
>
> /**
> --
> 2.20.1
>
--
Re: [RFC PATCH 24/40] soundwire: cadence_master: use BIOS defaults for frame shape
On Thu, Jul 25, 2019 at 06:40:16PM -0500, Pierre-Louis Bossart wrote:
> Remove hard-coding and use BIOS values. If they are wrong use default
> 48x2 frame shape.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/cadence_master.c | 19 +--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index 442f78c00f09..d84344e29f71 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -175,7 +175,6 @@
> /* Driver defaults */
>
> #define CDNS_DEFAULT_CLK_DIVIDER 0
> -#define CDNS_DEFAULT_FRAME_SHAPE 0x30
> #define CDNS_DEFAULT_SSP_INTERVAL0x18
> #define CDNS_TX_TIMEOUT 2000
>
> @@ -954,6 +953,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
> }
> EXPORT_SYMBOL(sdw_cdns_pdi_init);
>
> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
> +{
> + u32 val;
> + int c;
> + int r;
> +
> + r = sdw_find_row_index(n_rows);
> + c = sdw_find_col_index(n_cols);
> +
Now i get why you need above functions to be exported, please ignore my
previous comment.
> + val = (r << 3) | c;
> +
> + return val;
> +}
> +
> /**
> * sdw_cdns_init() - Cadence initialization
> * @cdns: Cadence instance
> @@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
> cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>
> /* Set the default frame shape */
> - cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
> + val = cdns_set_default_frame_shape(prop->default_row,
> +prop->default_col);
> + cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
>
> /* Set SSP interval to default value */
> cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
> --
> 2.20.1
>
--
Re: [RFC PATCH 25/40] soundwire: intel: use BIOS information to set clock dividers
On Thu, Jul 25, 2019 at 06:40:17PM -0500, Pierre-Louis Bossart wrote:
> The BIOS provides an Intel-specific property, let's use it to avoid
> hard-coded clock dividers.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/cadence_master.c | 26 ++
> drivers/soundwire/intel.c | 26 ++
> include/linux/soundwire/sdw.h | 2 ++
> 3 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index d84344e29f71..10ebcef2e84e 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -173,8 +173,6 @@
> #define CDNS_PDI_CONFIG_PORT GENMASK(4, 0)
>
> /* Driver defaults */
> -
> -#define CDNS_DEFAULT_CLK_DIVIDER 0
> #define CDNS_DEFAULT_SSP_INTERVAL0x18
> #define CDNS_TX_TIMEOUT 2000
>
> @@ -973,7 +971,10 @@ static u32 cdns_set_default_frame_shape(int n_rows, int
> n_cols)
> */
> int sdw_cdns_init(struct sdw_cdns *cdns)
> {
> + struct sdw_bus *bus = &cdns->bus;
> + struct sdw_master_prop *prop = &bus->prop;
> u32 val;
> + int divider;
> int ret;
>
> /* Exit clock stop */
> @@ -985,9 +986,17 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
> }
>
> /* Set clock divider */
> + divider = (prop->mclk_freq / prop->max_clk_freq) - 1;
Do you expect mclk_freq and max_clk_freq to be same?
> val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
> - val |= CDNS_DEFAULT_CLK_DIVIDER;
> + val |= divider;
> cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
> + cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
> +
> + pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
> +prop->mclk_freq,
> +prop->max_clk_freq,
> +divider,
> +val);
This can be removed.
>
> /* Set the default frame shape */
> val = cdns_set_default_frame_shape(prop->default_row,
> @@ -1035,6 +1044,7 @@ EXPORT_SYMBOL(sdw_cdns_init);
>
> int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
> {
> + struct sdw_master_prop *prop = &bus->prop;
> struct sdw_cdns *cdns = bus_to_cdns(bus);
> int mcp_clkctrl_off, mcp_clkctrl;
> int divider;
> @@ -1044,7 +1054,9 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
> sdw_bus_params *params)
> return -EINVAL;
> }
>
> - divider = (params->max_dr_freq / params->curr_dr_freq) - 1;
> + divider = prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR /
What is the reason for not using max_dr_freq? Its precomputed as
prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
> + params->curr_dr_freq;
> + divider--; /* divider is 1/(N+1) */
>
> if (params->next_bank)
> mcp_clkctrl_off = CDNS_MCP_CLK_CTRL1;
> @@ -1055,6 +1067,12 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
> sdw_bus_params *params)
> mcp_clkctrl |= divider;
> cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
>
> + pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
> +prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
> +params->curr_dr_freq,
> +divider,
> +mcp_clkctrl);
> +
> return 0;
> }
> EXPORT_SYMBOL(cdns_bus_conf);
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index c718c9c67a37..796ac2bc8cea 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -917,11 +917,37 @@ static int intel_register_dai(struct sdw_intel *sdw)
> dais, num_dai);
> }
>
> +static int sdw_master_read_intel_prop(struct sdw_bus *bus)
> +{
> + struct sdw_master_prop *prop = &bus->prop;
> + struct fwnode_handle *link;
> + char name[32];
> + int nval, i;
> +
> + /* Find master handle */
> + snprintf(name, sizeof(name),
> + "mipi-sdw-link-%d-subproperties", bus->link_id);
> +
> + link = device_get_named_child_node(bus->dev, name);
> + if (!link) {
> + dev_err(bus->dev, "Master node %s not found\n", name);
> + return -EIO;
> + }
> +
> + fwnode_property_read_u32(link,
> + "intel-sdw-ip-clock",
> + &prop->mclk_freq);
> + return 0;
> +}
> +
> static int intel_prop_read(struct sdw_bus *bus)
> {
> /* Initialize with default handler to read all DisCo properties */
> sdw_master_read_prop(bus);
>
> + /* read Intel-specific properties */
> + sdw_master_read_intel_prop(bus);
> +
> return 0;
> }
>
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 31d1e8acf25b..b6acc436ac80 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -379,6 +379,7 @@ struct sdw_slave_prop {
> * @dynamic_frame: Dynamic frame shape supported
>
Re: [RFC PATCH 26/40] soundwire: cadence_master: fix divider setting in clock register
On Thu, Jul 25, 2019 at 06:40:18PM -0500, Pierre-Louis Bossart wrote:
> From: Rander Wang
>
> The existing code uses an OR operation which would mix the original
> divider setting with the new one, resulting in an invalid
> configuration that can make codecs hang.
>
> Add the mask definition and use cdns_updatel to update divider
>
> Signed-off-by: Rander Wang
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/cadence_master.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index 10ebcef2e84e..18c6ac026e85 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -57,6 +57,7 @@
> #define CDNS_MCP_SSP_CTRL1 0x28
> #define CDNS_MCP_CLK_CTRL0 0x30
> #define CDNS_MCP_CLK_CTRL1 0x38
> +#define CDNS_MCP_CLK_MCLKD_MASK GENMASK(7, 0)
>
> #define CDNS_MCP_STAT0x40
>
> @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
> /* Set clock divider */
> divider = (prop->mclk_freq / prop->max_clk_freq) - 1;
> val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
reg read of CLK_CTRL0 can be removed.
> - val |= divider;
> - cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
> - cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
> +
> + cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
> + CDNS_MCP_CLK_MCLKD_MASK, divider);
> + cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
> + CDNS_MCP_CLK_MCLKD_MASK, divider);
>
> pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
> prop->mclk_freq,
> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
> sdw_bus_params *params)
> mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>
> mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
same as above.
> - mcp_clkctrl |= divider;
> - cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
> + cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
>
> pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
> prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
> --
> 2.20.1
>
--
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
On Mon, Aug 05, 2019 at 10:33:25AM -0500, Pierre-Louis Bossart wrote:
>
>
> On 8/5/19 4:56 AM, Sanyog Kale wrote:
> > On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote:
> > > When we disable the stream and then call hw_free, two bank switches
> > > will be handled and as a result we re-enable the stream on hw_free.
> > >
> >
> > I didnt quite get why there will be two bank switches as part of disable
> > flow
> > which leads to enabling of stream?
>
> You have two bank switches, one to stop streaming and on in de-prepare. It's
> symmetrical with the start sequence, where we do a bank switch to prepare
> and another to enable.
Got it. I misunderstood it that two bank switches are performed as part of
disable_stream.
>
> Let's assume we are using bank0 when streaming.
>
> Before the first bank switch, the channel_enable is set to false in the
> alternate bank1. When the bank switch happens, bank1 become active and the
> streaming stops. But bank0 registers have not been modified so when we do
> the second bank switch in de-prepare we make bank0 active, and the ch_enable
> bits are still set so streaming will restart... When we stop streaming, we
> need to make sure the ch_enable bits are cleared in the two banks.
This is clear. Even though the channels remains enabled, i believe there
won't be any data pushed on lines as stream will be closed.
Regarding mirroring approach, I assume after bank switch we will take
snapshot of active bank and program same in inactive bank.
>
>
> >
> > > Make sure the stream is disabled on both banks.
> > >
> > > TODO: we need to completely revisit all this and make sure we have a
> > > mirroring mechanism between current and alternate banks.
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > ---
> > > drivers/soundwire/stream.c | 19 ++-
> > > 1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> > > index 53f5e790fcd7..75b9ad1fb1a6 100644
> > > --- a/drivers/soundwire/stream.c
> > > +++ b/drivers/soundwire/stream.c
> > > @@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct
> > > sdw_stream_runtime *stream)
> > > }
> > > }
> > > - return do_bank_switch(stream);
> > > + ret = do_bank_switch(stream);
> > > + if (ret < 0) {
> > > + dev_err(bus->dev, "Bank switch failed: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + /* make sure alternate bank (previous current) is also disabled */
> > > + list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > + bus = m_rt->bus;
> > > + /* Disable port(s) */
> > > + ret = sdw_enable_disable_ports(m_rt, false);
> > > + if (ret < 0) {
> > > + dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > }
> > > /**
> > > --
> > > 2.20.1
> > >
> >
--
Re: [RFC PATCH 28/40] soundwire: intel: handle disabled links
On Thu, Jul 25, 2019 at 06:40:20PM -0500, Pierre-Louis Bossart wrote:
> On most hardware platforms, SoundWire interfaces are pin-muxed with
> other interfaces (typically DMIC or I2S) and the status of each link
> needs to be checked at boot time.
>
> For Intel platforms, the BIOS provides a menu to enable/disable the
> links separately, and the information is provided to the OS with an
> Intel-specific _DSD property. The same capability will be added to
> revisions of the MIPI DisCo specification.
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/intel.c | 26 ++
> include/linux/soundwire/sdw.h | 2 ++
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 796ac2bc8cea..5947fa8e840b 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -90,6 +90,8 @@
> #define SDW_ALH_STRMZCFG_DMATGENMASK(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,
> @@ -922,7 +924,7 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
> struct sdw_master_prop *prop = &bus->prop;
> struct fwnode_handle *link;
> char name[32];
> - int nval, i;
> + u32 quirk_mask;
>
> /* Find master handle */
> snprintf(name, sizeof(name),
> @@ -937,6 +939,14 @@ static int sdw_master_read_intel_prop(struct sdw_bus
> *bus)
> fwnode_property_read_u32(link,
>"intel-sdw-ip-clock",
>&prop->mclk_freq);
> +
> + fwnode_property_read_u32(link,
> + "intel-quirk-mask",
> + &quirk_mask);
> +
> + if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
> + prop->hw_disabled = true;
> +
> return 0;
> }
>
> @@ -997,6 +1007,12 @@ static int intel_probe(struct platform_device *pdev)
> goto err_master_reg;
> }
>
> + if (sdw->cdns.bus.prop.hw_disabled) {
> + dev_info(&pdev->dev, "SoundWire master %d is disabled,
> ignoring\n",
> + sdw->cdns.bus.link_id);
> + return 0;
> + }
> +
> /* Initialize shim and controller */
> intel_link_power_up(sdw);
> intel_shim_init(sdw);
> @@ -1050,9 +1066,11 @@ static int intel_remove(struct platform_device *pdev)
>
> sdw = platform_get_drvdata(pdev);
>
> - intel_debugfs_exit(sdw);
> - free_irq(sdw->res->irq, sdw);
> - snd_soc_unregister_component(sdw->cdns.dev);
> + if (!sdw->cdns.bus.prop.hw_disabled) {
> + intel_debugfs_exit(sdw);
> + free_irq(sdw->res->irq, sdw);
> + snd_soc_unregister_component(sdw->cdns.dev);
> + }
> sdw_delete_bus_master(&sdw->cdns.bus);
>
> return 0;
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index c7dfc824be80..f78b076a8782 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -380,6 +380,7 @@ struct sdw_slave_prop {
> * @err_threshold: Number of times that software may retry sending a single
> * command
> * @mclk_freq: clock reference passed to SoundWire Master, in Hz.
> + * @hw_disabled: if true, the Master is not functional, typically due to
> pin-mux
> */
> struct sdw_master_prop {
> u32 revision;
> @@ -395,6 +396,7 @@ struct sdw_master_prop {
> bool dynamic_frame;
> u32 err_threshold;
> u32 mclk_freq;
> + bool hw_disabled;
Do we have such cases where some of SoundWire links are disabled and
some enabled?
> };
>
> int sdw_master_read_prop(struct sdw_bus *bus);
> --
> 2.20.1
>
--
Re: [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm
On Thu, Jul 25, 2019 at 06:40:19PM -0500, Pierre-Louis Bossart wrote:
> This algorithm computes bus parameters like clock frequency, frame
> shape and port transport parameters based on active stream(s) running
> on the bus.
>
> This implementation is optimal for Intel platforms. Developers can
> also implement their own .compute_params() callback for specific
> resource management algorithm.
>
> Credits: this patch is based on an earlier internal contribution by
> Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded
> values were removed from the initial contribution to use BIOS
> information instead.
>
> FIXME: remove checkpatch report
> WARNING: Reusing the krealloc arg is almost always a bug
> + group->rates = krealloc(group->rates,
>
> Signed-off-by: Pierre-Louis Bossart
> ---
> drivers/soundwire/Makefile | 2 +-
> drivers/soundwire/algo_dynamic_allocation.c | 403
> drivers/soundwire/bus.c | 3 +
> drivers/soundwire/bus.h | 46 ++-
> drivers/soundwire/stream.c | 20 +
> include/linux/soundwire/sdw.h | 5 +
> 6 files changed, 476 insertions(+), 3 deletions(-)
> create mode 100644 drivers/soundwire/algo_dynamic_allocation.c
>
> diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
> index 88990cac48a7..f59a9d4a28fd 100644
> --- a/drivers/soundwire/Makefile
> +++ b/drivers/soundwire/Makefile
> @@ -5,7 +5,7 @@
>
> #Bus Objs
> soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
> - debugfs.o
> + debugfs.o algo_dynamic_allocation.o
>
> obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
>
> diff --git a/drivers/soundwire/algo_dynamic_allocation.c
> b/drivers/soundwire/algo_dynamic_allocation.c
> new file mode 100644
> index ..89edb39162b8
> --- /dev/null
> +++ b/drivers/soundwire/algo_dynamic_allocation.c
> @@ -0,0 +1,403 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +// Copyright(c) 2015-18 Intel Corporation.
> +
> +/*
> + * Bandwidth management algorithm based on 2^n gears
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include "bus.h"
> +
> +#define SDW_STRM_RATE_GROUPING 1
> +
> +struct sdw_group_params {
> + unsigned int rate;
> + int full_bw;
> + int payload_bw;
> + int hwidth;
> +};
> +
> +struct sdw_group {
> + unsigned int count;
> + unsigned int max_size;
> + unsigned int *rates;
> +};
> +
> +struct sdw_transport_data {
> + int hstart;
> + int hstop;
> + int block_offset;
> + int sub_block_offset;
> +};
> +
> +
> +/**
> + * sdw_compute_port_params: Compute transport and port parameters
> + *
> + * @bus: SDW Bus instance
> + */
> +static int sdw_compute_port_params(struct sdw_bus *bus)
> +{
> + struct sdw_group_params *params = NULL;
> + struct sdw_group group;
> + int ret;
> +
> + ret = sdw_get_group_count(bus, &group);
> + if (ret < 0)
> + goto out;
> +
> + if (group.count == 0)
> + goto out;
> +
> + params = kcalloc(group.count, sizeof(*params), GFP_KERNEL);
> + if (!params) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Compute transport parameters for grouped streams */
> + ret = sdw_compute_group_params(bus, params,
> +&group.rates[0], group.count);
> + if (ret < 0)
> + goto out;
> +
> + _sdw_compute_port_params(bus, params, group.count);
> +
> +out:
> + kfree(params);
> + kfree(group.rates);
> +
> + return ret;
> +}
> +
> +static int sdw_select_row_col(struct sdw_bus *bus, int clk_freq)
> +{
> + struct sdw_master_prop *prop = &bus->prop;
> + int frame_int, frame_freq;
> + int r, c;
> +
> + for (c = 0; c < SDW_FRAME_COLS; c++) {
> + for (r = 0; r < SDW_FRAME_ROWS; r++) {
> + if (sdw_rows[r] != prop->default_row ||
> + sdw_cols[c] != prop->default_col)
> + continue;
Are we only supporting default rows and cols?
> +
> + frame_int = sdw_rows[r] * sdw_cols[c];
> + frame_freq = clk_freq / frame_int;
> +
> + if ((clk_freq - (frame_freq * SDW_FRAME_CTRL_BITS)) <
> + bus->params.bandwidth)
> + continue;
> +
> + bus->params.row = sdw_rows[r];
> + bus->params.col = sdw_cols[c];
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> --
> 2.20.1
>
--
Re: [alsa-devel] [PATCH v2] soundwire: stream: fix bad unlock balance
On Thu, Jun 06, 2019 at 10:36:02AM -0500, Pierre-Louis Bossart wrote: > On 6/6/19 9:58 AM, Srinivas Kandagatla wrote: > > > > > > On 06/06/2019 15:28, Pierre-Louis Bossart wrote: > > > On 6/6/19 6:22 AM, Srinivas Kandagatla wrote: > > > > multi bank switching code takes lock on condition but releases without > > > > any check resulting in below warning. > > > > This patch fixes this. > > > > > > > > > Question to make sure we are talking about the same thing: > > > multi-link bank switching is a capability beyond the scope of the > > > SoundWire spec which requires hardware support to synchronize links > > > and as Sanyog hinted at in a previous email follow a different flow > > > for bank switches. > > > > > > You would not use the multi-link mode if you have different links > > > that can operate independently and have no synchronization > > > requirement. You would conversely use the multi-link mode if you > > > have two devices on the same type on different links and want audio > > > to be rendered at the same time. > > > > > > Can you clarify if indeed you were using the full-blown multi-link > > > mode with hardware synchronization or a regular single-link > > > operation? I am not asking for details of your test hardware, just > > > trying to reconstruct the program flow leading to this problem. > > > > > > > Am testing on a regular single link, which hits this path. > > > > > It could also be that your commit message was meant to say: > > > "the msg lock is taken for multi-link cases only but released > > > unconditionally, leading to an unlock balance warning for > > > single-link usages"? > > Yes. > > Thanks for the precision. the change is legit so assuming the commit message > is reworded to mention single link usage please feel free to take the > following tag. > > Acked-by: Pierre-Louis Bossart > Changes looks okay to me. Please update commit message as pierre suggested. Acked-by: Sanyog Kale > > Thanks! --
Re: [alsa-devel] [Patch v6 1/7] slimbus: Device management on SLIMbus
On Fri, Oct 06, 2017 at 05:51:30PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia
>
> SLIMbus (Serial Low Power Interchip Media Bus) is a specification
> developed by MIPI (Mobile Industry Processor Interface) alliance.
> SLIMbus is a 2-wire implementation, which is used to communicate with
> peripheral components like audio-codec.
> SLIMbus uses Time-Division-Multiplexing to accommodate multiple data
> channels, and control channel. Control channel has messages to do
> device-enumeration, messages to send/receive control-data to/from
> slimbus devices, messages for port/channel management, and messages to
> do bandwidth allocation.
> The framework supports multiple instances of the bus (1 controller per
> bus), and multiple slave devices per controller.
>
> This patch does device enumeration, logical address assignment,
> informing device when the device reports present/absent etc.
> Reporting present may need the driver to do the needful (e.g. turning
> on voltage regulators powering the device). Additionally device is
> probed when it reports present if that device doesn't need any such
> steps mentioned above.
>
> Signed-off-by: Sagar Dharia
> Signed-off-by: Srinivas Kandagatla
> ---
> Documentation/devicetree/bindings/slimbus/bus.txt | 57 ++
> Documentation/slimbus/summary | 109
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/slimbus/Kconfig | 11 +
> drivers/slimbus/Makefile | 5 +
> drivers/slimbus/slim-core.c | 695
> ++
> include/linux/mod_devicetable.h | 13 +
> include/linux/slimbus.h | 299 ++
> 9 files changed, 1192 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/slimbus/bus.txt
> create mode 100644 Documentation/slimbus/summary
> create mode 100644 drivers/slimbus/Kconfig
> create mode 100644 drivers/slimbus/Makefile
> create mode 100644 drivers/slimbus/slim-core.c
> create mode 100644 include/linux/slimbus.h
>
> diff --git a/Documentation/devicetree/bindings/slimbus/bus.txt
> b/Documentation/devicetree/bindings/slimbus/bus.txt
> new file mode 100644
> index 000..cb658bb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/bus.txt
> @@ -0,0 +1,57 @@
> +SLIM(Serial Low Power Interchip Media Bus) bus
> +
> +SLIMbus is a 2-wire bus, and is used to communicate with peripheral
> +components like audio-codec.
> +
> +Controller is a normal device using binding for whatever bus it is
> +on (e.g. platform bus).
> +Required property for SLIMbus controller node:
> +- compatible - name of SLIMbus controller following generic names
> + recommended practice.
> +- #address-cells - should be 2
> +- #size-cells- should be 0
> +
> +No other properties are required in the SLIMbus controller bus node.
> +
> +Child nodes:
> +Every SLIMbus controller node can contain zero or more child nodes
> +representing slave devices on the bus. Every SLIMbus slave device is
> +uniquely determined by the enumeration address containing 4 fields:
> +Manufacturer ID, Product code, Device index, and Instance value for
> +the device.
> +If child node is not present and it is instantiated after device
> +discovery (slave device reporting itself present).
> +
> +In some cases it may be necessary to describe non-probeable device
> +details such as non-standard ways of powering up a device. In
> +such cases, child nodes for those devices will be present as
> +slaves of the slimbus-controller, as detailed below.
> +
> +Required property for SLIMbus child node if it is present:
> +- reg- Is Duplex (Device index, Instance ID) from Enumeration
> + Address.
> + Device Index Uniquely identifies multiple Devices within
> + a single Component.
> + Instance ID Is for the cases where multiple Devices of the
> + same type or Class are attached to the bus.
> +
> +- compatible -"slimMID,PID". The textual representation of Manufacturer ID,
> + Product Code, shall be in lower case hexadecimal with leading
> + zeroes suppressed
> +
> +SLIMbus example for Qualcomm's slimbus manager component:
> +
> + slim@2808 {
> + compatible = "qcom,slim-msm";
> + reg = <0x2808 0x2000>,
> + interrupts = <0 33 0>;
> + clocks = <&lcc SLIMBUS_SRC>, <&lcc AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60"";
> + reg = <1 0>;
> + };
> + };
> diff --git a/Documentation/slimbus/summary b/Documentation/slimbus/summary
> new file mode 100644
Re: [PATCH v2 04/14] soundwire: Add MIPI DisCo property helpers
On Thu, Nov 23, 2017 at 02:38:12PM +, Charles Keepax wrote:
> On Fri, Nov 10, 2017 at 05:19:06PM +0530, Vinod Koul wrote:
> > MIPI Discovery And Configuration (DisCo) Specification for SoundWire
> > specifies properties to be implemented for SoundWire Masters and
> > Slaves. The DisCo spec doesn't mandate these properties. However,
> > SDW bus cannot work without knowing these values.
> >
> > The helper functions read the Master and Slave properties.
> > Implementers of Master or Slave drivers can use any of the below
> > three mechanisms:
> >a) Use these APIs here as .read_prop() callback for Master
> > and Slave
> >b) Implement own methods and set those as .read_prop(), but invoke
> > APIs in this file for generic read and override the values with
> > platform specific data
> >c) Implement ones own methods which do not use anything provided
> > here
> >
> > Signed-off-by: Sanyog Kale
> > Signed-off-by: Vinod Koul
> > ---
> > 5 files changed, 733 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/soundwire/mipi_disco.c
> > --- a/drivers/soundwire/bus_type.c
> > +++ b/drivers/soundwire/bus_type.c
> > @@ -135,6 +135,8 @@ static int sdw_drv_probe(struct device *dev)
> > return ret;
> > }
> >
> > + slave->ops = drv->ops;
> > +
> > ret = drv->probe(slave, id);
> > if (ret) {
> > dev_err(dev, "Probe of %s failed: %d\n", drv->name, ret);
> > @@ -142,6 +144,22 @@ static int sdw_drv_probe(struct device *dev)
> > return ret;
> > }
> >
> > + /* device is probed so let's read the properties now */
> > + if (slave->ops && slave->ops->read_prop)
> > + slave->ops->read_prop(slave);
> > +
> > + /*
> > +* Check for valid clk_stop_timeout, use DisCo worst case value of
> > +* 300ms
> > +*
> > +* TODO: check the timeouts and driver removal case
> > +*/
> > + if (slave->prop.clk_stop_timeout == 0)
> > + slave->prop.clk_stop_timeout = 300;
> > +
> > + slave->bus->clk_stop_timeout = max_t(u32, slave->bus->clk_stop_timeout,
> > + slave->prop.clk_stop_timeout);
> > +
> > return 0;
> > }
> >
>
> This brings up something I have been struggling with a little on
> both the soundwire and slimbus chains at the moment.
>
> This happens after probe but if I am understanding right these
> properties might be required to communicate with the device.
> Which directly implies that probe shouldn't attempt to talk to
> the device? This is also implied in both series by the fact that
> probe will usually setup the things necessary for enumeration
> (power, clocks, etc). But it is fairly common in probe to at
> least do something like read some ID registers to check we are
> probing the right device.
>
> On the slimbus side I guess you can get round this using the fact
> they have device_status callbacks to the driver so the driver can
> tell when the device is actually available, and could perform
> some actions when the device first becomes available on the
> bus. But this SoundWire chain doesn't have an equivalent.
We have similar device status callback (update_status) to report driver
about Slave status change. Bus calls update_status when Slave reports
status change (please check patch 09/14).
> Apologies for the long and slightly vague comment, but I guess my
> question is do you have a thought on how drivers should know when
> it is safe to communicate with a SoundWire device?
IMO it is safe to communicate with SoundWire device when the Slave
status is ATTACHED. In any case bus will report error if it is not able
to communicate with SoundWire device.
> Thanks,
> Charles
--
