| Message ID | 20200309173755.955-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] soundwire: bus: Add flag to mark DPN_BlockCtrl1 as readonly | expand |
On 3/9/20 12:37 PM, Srinivas Kandagatla wrote: > According to SoundWire Specification Version 1.2. > "A Data Port number X (in the range 0-14) which supports only one > value of WordLength may implement the WordLength field in the > DPX_BlockCtrl1 Register as Read-Only, returning the fixed value of > WordLength in response to reads." Interesting. I think it's a spec issue that you want to bring to the MIPI Audio WG attention. The note below says 'the DPx_BlockCtrl1 Register remains as Read-Write, but the value written to the Read-Only field is not used' Ignoring a value and returning an error are two different behaviors indeed. My recommendation would be to add a DisCo property stating the WordLength value can be used by the bus code but not written to the Slave device registers. > > As WSA881x interfaces in PDM mode making the only field "WordLength" > in DPX_BlockCtrl1" fixed and read-only. Behaviour of writing to this > register on WSA881x soundwire slave with Qualcomm Soundwire Controller > is throwing up an error. Not sure how other controllers deal with > writing to readonly registers, but this patch provides a way to avoid > writes to DPN_BlockCtrl1 register by providing a ro_blockctrl1_reg > flag in struct sdw_port_runtime. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > > I will send patch for WSA881x to include this change once this patch > is accepted. > > drivers/soundwire/bus.h | 2 ++ > drivers/soundwire/stream.c | 17 ++++++++++------- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > index 204204a26db8..791e8d14093e 100644 > --- a/drivers/soundwire/bus.h > +++ b/drivers/soundwire/bus.h > @@ -79,6 +79,7 @@ int sdw_find_col_index(int col); > * @num: Port number. For audio streams, valid port number ranges from > * [1,14] > * @ch_mask: Channel mask > + * @ro_blockctrl1_reg: Read Only flag for DPN_BlockCtrl1 register > * @transport_params: Transport parameters > * @port_params: Port parameters > * @port_node: List node for Master or Slave port_list > @@ -89,6 +90,7 @@ int sdw_find_col_index(int col); > struct sdw_port_runtime { > int num; > int ch_mask; > + bool ro_blockctrl1_reg; > struct sdw_transport_params transport_params; > struct sdw_port_params port_params; > struct list_head port_node; > diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c > index 00348d1fc606..4491643aeb4a 100644 > --- a/drivers/soundwire/stream.c > +++ b/drivers/soundwire/stream.c > @@ -167,13 +167,15 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus, > return ret; > } > > - /* Program DPN_BlockCtrl1 register */ > - ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1)); > - if (ret < 0) { > - dev_err(&s_rt->slave->dev, > - "DPN_BlockCtrl1 register write failed for port %d\n", > - t_params->port_num); > - return ret; > + if (!p_rt->ro_blockctrl1_reg) { > + /* Program DPN_BlockCtrl1 register */ > + ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1)); > + if (ret < 0) { > + dev_err(&s_rt->slave->dev, > + "DPN_BlockCtrl1 register write failed for port %d\n", > + t_params->port_num); > + return ret; > + } > } > > /* Program DPN_SampleCtrl1 register */ > @@ -1195,6 +1197,7 @@ static struct sdw_port_runtime > > p_rt->ch_mask = port_config[port_index].ch_mask; > p_rt->num = port_config[port_index].num; > + p_rt->ro_blockctrl1_reg = port_config[port_index].ro_blockctrl1_reg; > > return p_rt; > } >
On 09/03/2020 18:05, Pierre-Louis Bossart wrote: > > My recommendation would be to add a DisCo property stating the > WordLength value can be used by the bus code but not written to the > Slave device registers. Does something like "mipi-sdw-read-only-wordlength" as slave property, make sense? --srini
Hi Srinivas, >> > My recommendation would be to add a DisCo property stating the >> WordLength value can be used by the bus code but not written to the >> Slave device registers. > > Does something like "mipi-sdw-read-only-wordlength" as slave property, > make sense? The properties can be handled at two levels. First, you'd want to change include/linux/soundwire/sdw.h, and add a new field in struct sdw_dpn_prop { u32 num; u32 max_word; u32 min_word; u32 num_words; u32 *words; + bool read_only_wordlength; Once this is added, along with the code that bypasses the programming of DPn_BlockCtrl1, the implementation has two choices: a) hard-code the field value in the codec driver. b) read the property from firmware with the DisCo helpers. There is no requirement that all properties be read from firmware, and if you look at existing code base sdw_slave_read_prop() is currently unused, each codec implements its own .read_prop() callback. We really wanted to be pragmatic, and give the possibility to either override bad firmware or extend incomplete firmware to avoid coupling OS and firmware too much. If you foresee cases where this implementation might vary and firmware distribution is not a problem, then a property read would make sense. Just once procedural reminder that all 'mipi-sdw' properties are handled by the MIPI software WG, so we'd need to have this property added in a formal MIPI document update. I suggest you talk with Lior first on this. Hope this helps -Pierre
On 09-03-20, 17:37, Srinivas Kandagatla wrote: > According to SoundWire Specification Version 1.2. > "A Data Port number X (in the range 0-14) which supports only one > value of WordLength may implement the WordLength field in the > DPX_BlockCtrl1 Register as Read-Only, returning the fixed value of > WordLength in response to reads." > > As WSA881x interfaces in PDM mode making the only field "WordLength" > in DPX_BlockCtrl1" fixed and read-only. Behaviour of writing to this > register on WSA881x soundwire slave with Qualcomm Soundwire Controller > is throwing up an error. Not sure how other controllers deal with > writing to readonly registers, but this patch provides a way to avoid > writes to DPN_BlockCtrl1 register by providing a ro_blockctrl1_reg > flag in struct sdw_port_runtime. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > > I will send patch for WSA881x to include this change once this patch > is accepted. > > drivers/soundwire/bus.h | 2 ++ > drivers/soundwire/stream.c | 17 ++++++++++------- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h > index 204204a26db8..791e8d14093e 100644 > --- a/drivers/soundwire/bus.h > +++ b/drivers/soundwire/bus.h > @@ -79,6 +79,7 @@ int sdw_find_col_index(int col); > * @num: Port number. For audio streams, valid port number ranges from > * [1,14] > * @ch_mask: Channel mask > + * @ro_blockctrl1_reg: Read Only flag for DPN_BlockCtrl1 register > * @transport_params: Transport parameters > * @port_params: Port parameters > * @port_node: List node for Master or Slave port_list > @@ -89,6 +90,7 @@ int sdw_find_col_index(int col); > struct sdw_port_runtime { > int num; > int ch_mask; > + bool ro_blockctrl1_reg; Let us use properties for this so it should be added in struct sdw_dpn_prop or struct sdw_slave_prop (I dont think that we would have different properties for each DPn when we are dealing with these simple codecs) Also the property should not have mipi tag as this is not in DiSCo spec (yet), so I would suggest sdw_ro_wordlen for this. Thanks
Thanks for the inputs On 10/03/2020 15:53, Pierre-Louis Bossart wrote: > Hi Srinivas, > >>> > My recommendation would be to add a DisCo property stating the >>> WordLength value can be used by the bus code but not written to the >>> Slave device registers. >> >> Does something like "mipi-sdw-read-only-wordlength" as slave property, >> make sense? > > The properties can be handled at two levels. > > First, you'd want to change include/linux/soundwire/sdw.h, and add a new > field in > > struct sdw_dpn_prop { > u32 num; > u32 max_word; > u32 min_word; > u32 num_words; > u32 *words; > + bool read_only_wordlength; > > Once this is added, along with the code that bypasses the programming of > DPn_BlockCtrl1, the implementation has two choices: > > a) hard-code the field value in the codec driver. This totally works for me. > > b) read the property from firmware with the DisCo helpers. > I would defer adding this for now till there is a real users for this. > There is no requirement that all properties be read from firmware, and > if you look at existing code base sdw_slave_read_prop() is currently > unused, each codec implements its own .read_prop() callback. > > We really wanted to be pragmatic, and give the possibility to either > override bad firmware or extend incomplete firmware to avoid coupling OS > and firmware too much. If you foresee cases where this implementation > might vary and firmware distribution is not a problem, then a property > read would make sense. > > Just once procedural reminder that all 'mipi-sdw' properties are handled > by the MIPI software WG, so we'd need to have this property added in a > formal MIPI document update. > > I suggest you talk with Lior first on this. Sure, I will talk to him. > > Hope this helps > -Pierre
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 204204a26db8..791e8d14093e 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -79,6 +79,7 @@ int sdw_find_col_index(int col); * @num: Port number. For audio streams, valid port number ranges from * [1,14] * @ch_mask: Channel mask + * @ro_blockctrl1_reg: Read Only flag for DPN_BlockCtrl1 register * @transport_params: Transport parameters * @port_params: Port parameters * @port_node: List node for Master or Slave port_list @@ -89,6 +90,7 @@ int sdw_find_col_index(int col); struct sdw_port_runtime { int num; int ch_mask; + bool ro_blockctrl1_reg; struct sdw_transport_params transport_params; struct sdw_port_params port_params; struct list_head port_node; diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 00348d1fc606..4491643aeb4a 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -167,13 +167,15 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus, return ret; } - /* Program DPN_BlockCtrl1 register */ - ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1)); - if (ret < 0) { - dev_err(&s_rt->slave->dev, - "DPN_BlockCtrl1 register write failed for port %d\n", - t_params->port_num); - return ret; + if (!p_rt->ro_blockctrl1_reg) { + /* Program DPN_BlockCtrl1 register */ + ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1)); + if (ret < 0) { + dev_err(&s_rt->slave->dev, + "DPN_BlockCtrl1 register write failed for port %d\n", + t_params->port_num); + return ret; + } } /* Program DPN_SampleCtrl1 register */ @@ -1195,6 +1197,7 @@ static struct sdw_port_runtime p_rt->ch_mask = port_config[port_index].ch_mask; p_rt->num = port_config[port_index].num; + p_rt->ro_blockctrl1_reg = port_config[port_index].ro_blockctrl1_reg; return p_rt; }
According to SoundWire Specification Version 1.2. "A Data Port number X (in the range 0-14) which supports only one value of WordLength may implement the WordLength field in the DPX_BlockCtrl1 Register as Read-Only, returning the fixed value of WordLength in response to reads." As WSA881x interfaces in PDM mode making the only field "WordLength" in DPX_BlockCtrl1" fixed and read-only. Behaviour of writing to this register on WSA881x soundwire slave with Qualcomm Soundwire Controller is throwing up an error. Not sure how other controllers deal with writing to readonly registers, but this patch provides a way to avoid writes to DPN_BlockCtrl1 register by providing a ro_blockctrl1_reg flag in struct sdw_port_runtime. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- I will send patch for WSA881x to include this change once this patch is accepted. drivers/soundwire/bus.h | 2 ++ drivers/soundwire/stream.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-)