mbox series

[v3,0/2] soundwire: Add support to Qualcomm SoundWire master

Message ID 20191011154423.2506-1-srinivas.kandagatla@linaro.org
Headers show
Series soundwire: Add support to Qualcomm SoundWire master | expand

Message

Srinivas Kandagatla Oct. 11, 2019, 3:44 p.m. UTC
Thanks for reviewing the v2 patchset.
Here is new patchset addressing all the comments from v2

This patchset adds support for Qualcomm SoundWire Master Controller
found in most of Qualcomm SoCs and WCD audio codecs.

This driver along with WCD934x codec and WSA881x Class-D Smart Speaker
Amplifier drivers is tested on on DragonBoard DB845c based of SDM845
SoC and Lenovo YOGA C630 Laptop based on SDM850.

SoundWire controller on SDM845 is integrated in WCD934x audio codec via
SlimBus interface.

Currently this driver is very minimal and only supports PDM.

Most of the code in this driver is rework of Qualcomm downstream drivers
used in Andriod. Credits to Banajit Goswami and Patrick Lai's Team.

TODO:
	Test and add PCM support.

Thanks,
srini

Changes since v2:
- Added support to set_sdw_stream

Srinivas Kandagatla (2):
  dt-bindings: soundwire: add bindings for Qcom controller
  soundwire: qcom: add support for SoundWire controller

 .../bindings/soundwire/qcom,sdw.txt           | 167 ++++
 drivers/soundwire/Kconfig                     |   9 +
 drivers/soundwire/Makefile                    |   4 +
 drivers/soundwire/qcom.c                      | 935 ++++++++++++++++++
 4 files changed, 1115 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,sdw.txt
 create mode 100644 drivers/soundwire/qcom.c

-- 
2.21.0

Comments

Srinivas Kandagatla Oct. 14, 2019, 5:34 p.m. UTC | #1
Thanks Rob for taking time to review,

On 14/10/2019 18:12, Rob Herring wrote:
> On Fri, Oct 11, 2019 at 04:44:22PM +0100, Srinivas Kandagatla wrote:

>> This patch adds bindings for Qualcomm soundwire controller.

>>

>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs

>> either integrated as part of WCD audio codecs via slimbus or

>> as part of SOC I/O.

>>

>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

>> ---

>>   .../bindings/soundwire/qcom,sdw.txt           | 167 ++++++++++++++++++

>>   1 file changed, 167 insertions(+)

>>   create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> 

> Next time, do a DT schema.

> 

Sure! I can do that in next version!

>> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt b/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

>> new file mode 100644

>> index 000000000000..436547f3b155

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

>> @@ -0,0 +1,167 @@

>> +Qualcomm SoundWire Controller Bindings

>> +

>> +

>> +This binding describes the Qualcomm SoundWire Controller along with its

>> +board specific bus parameters.

>> +

>> +- compatible:

>> +	Usage: required

>> +	Value type: <stringlist>

>> +	Definition: must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",

>> +		    Example:

>> +			"qcom,soundwire-v1.3.0"

>> +			"qcom,soundwire-v1.5.0"

>> +			"qcom,soundwire-v1.6.0"

> 

> This needs to be the actual versions supported, not examples. Elsewhere

> in QCom bindings, we've used standard SoC specific compatibles as there

> never tends to be many SoCs with the same version. Anything different

> here?

> 


These values of MAJOR MINOR and STEP are defined as part of IP spec. And 
most of the QCom IPs follow such scheme. We can read back these values 
from the Hardware registers.

Having SoC Names here might not be very efficient here.
We have used such compatibles on may QCom IPs, like BAM DMA, SLIMBus and 
other drivers.

>> +- reg:

>> +	Usage: required

>> +	Value type: <prop-encoded-array>

>> +	Definition: the base address and size of SoundWire controller

>> +		    address space.

>> +

>> +- interrupts:

>> +	Usage: required

>> +	Value type: <prop-encoded-array>

>> +	Definition: should specify the SoundWire Controller IRQ

>> +

>> +- clock-names:

>> +	Usage: required

>> +	Value type: <stringlist>

>> +	Definition: should be "iface" for SoundWire Controller interface clock

>> +

>> +- clocks:

>> +	Usage: required

>> +	Value type: <prop-encoded-array>

>> +	Definition: should specify the SoundWire Controller interface clock

>> +

>> +- #sound-dai-cells:

>> +	Usage: required

>> +	Value type: <u32>

>> +	Definition: must be 1 for digital audio interfaces on the controller.

>> +

>> +- qcom,dout-ports:

>> +	Usage: required

>> +	Value type: <u32>

>> +	Definition: must be count of data out ports

> 

> Up to how many?

> 

>> +

>> +- qcom,din-ports:

>> +	Usage: required

>> +	Value type: <u32>

>> +	Definition: must be count of data in ports

> 

> Up to how many?


Up to 15 data ports in total

> 

>> +

...

>> +Note:

>> +	More Information on detail of encoding of these fields can be

>> +found in MIPI Alliance SoundWire 1.0 Specifications.

>> +

>> += SoundWire devices

>> +Each subnode of the bus represents SoundWire device attached to it.

>> +The properties of these nodes are defined by the individual bindings.

> 

> Is there some sort of addressing that needs to be defined?

> 

Thanks, Looks like I missed that here.

it should be something like this,

#address-cells = <2>;
#size-cells = <0>;

Will add the in next version.


>> +

>> += EXAMPLE

>> +The following example represents a SoundWire controller on DB845c board

>> +which has controller integrated inside WCD934x codec on SDM845 SoC.

>> +

>> +soundwire: soundwire@c85 {

>> +	compatible = "qcom,soundwire-v1.3.0";

>> +	reg = <0xc85 0x20>;

>> +	interrupts = <20 IRQ_TYPE_EDGE_RISING>;

>> +	clocks = <&wcc>;

>> +	clock-names = "iface";

>> +	#sound-dai-cells = <1>;

>> +	qcom,dports-type = <0>;

>> +	qcom,dout-ports	= <6>;

>> +	qcom,din-ports	= <2>;

>> +	qcom,ports-sinterval-low = /bits/ 8  <0x07 0x1F 0x3F 0x7 0x1F 0x3F 0x0F 0x0F>;

>> +	qcom,ports-offset1 = /bits/ 8 <0x01 0x02 0x0C 0x6 0x12 0x0D 0x07 0x0A >;

>> +	qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x1F 0x00 0x00 0x1F 0x00 0x00>;

>> +

>> +	/* Left Speaker */

>> +	left{

> 

> space       ^

>> +		....

>> +	};

>> +

>> +	/* Right Speaker */

>> +	right{

> 

> ditto

> 

>> +		....

>> +	};

>> +};

>> -- 

>> 2.21.0

>>
Rob Herring Oct. 15, 2019, 11:35 a.m. UTC | #2
On Mon, Oct 14, 2019 at 12:34 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>

> Thanks Rob for taking time to review,

>

> On 14/10/2019 18:12, Rob Herring wrote:

> > On Fri, Oct 11, 2019 at 04:44:22PM +0100, Srinivas Kandagatla wrote:

> >> This patch adds bindings for Qualcomm soundwire controller.

> >>

> >> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs

> >> either integrated as part of WCD audio codecs via slimbus or

> >> as part of SOC I/O.

> >>

> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> >> ---

> >>   .../bindings/soundwire/qcom,sdw.txt           | 167 ++++++++++++++++++

> >>   1 file changed, 167 insertions(+)

> >>   create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> >

> > Next time, do a DT schema.

> >

> Sure! I can do that in next version!


I meant the next binding you write, not v4. However, ...

[...]

> >> += SoundWire devices

> >> +Each subnode of the bus represents SoundWire device attached to it.

> >> +The properties of these nodes are defined by the individual bindings.

> >

> > Is there some sort of addressing that needs to be defined?

> >

> Thanks, Looks like I missed that here.

>

> it should be something like this,

>

> #address-cells = <2>;

> #size-cells = <0>;

>

> Will add the in next version.


You need a common soundwire binding for this. You also need to define
the format of 'reg' and unit addresses. And it needs to be a schema.
So perhaps this binding too should be.

Rob
Rob Herring Oct. 15, 2019, 1:36 p.m. UTC | #3
On Tue, Oct 15, 2019 at 7:22 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>

>

>

> On 15/10/2019 12:35, Rob Herring wrote:

> > On Mon, Oct 14, 2019 at 12:34 PM Srinivas Kandagatla

> > <srinivas.kandagatla@linaro.org> wrote:

> >>

> >> Thanks Rob for taking time to review,

> >>

> >> On 14/10/2019 18:12, Rob Herring wrote:

> >>> On Fri, Oct 11, 2019 at 04:44:22PM +0100, Srinivas Kandagatla wrote:

> >>>> This patch adds bindings for Qualcomm soundwire controller.

> >>>>

> >>>> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs

> >>>> either integrated as part of WCD audio codecs via slimbus or

> >>>> as part of SOC I/O.

> >>>>

> >>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> >>>> ---

> >>>>    .../bindings/soundwire/qcom,sdw.txt           | 167 ++++++++++++++++++

> >>>>    1 file changed, 167 insertions(+)

> >>>>    create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> >>>

> >>> Next time, do a DT schema.

> >>>

> >> Sure! I can do that in next version!

> >

> > I meant the next binding you write, not v4. However, ...

> >

> > [...]

> >

> >>>> += SoundWire devices

> >>>> +Each subnode of the bus represents SoundWire device attached to it.

> >>>> +The properties of these nodes are defined by the individual bindings.

> >>>

> >>> Is there some sort of addressing that needs to be defined?

> >>>

> >> Thanks, Looks like I missed that here.

> >>

> >> it should be something like this,

> >>

> >> #address-cells = <2>;

> >> #size-cells = <0>;

> >>

> >> Will add the in next version.

> >

> > You need a common soundwire binding for this. You also need to define

> > the format of 'reg' and unit addresses. And it needs to be a schema.

> > So perhaps this binding too should be.

>

> We already have a common SoundWire bindings in mainline for this

>

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/soundwire/soundwire-controller.yaml?h=v5.4-rc3


Indeed... :)

> Should this binding just make a reference to it instead of duplicating

> this same info here?


Yes, that should be sufficient.

Rob
Vinod Koul Oct. 21, 2019, 4:27 a.m. UTC | #4
On 11-10-19, 16:44, Srinivas Kandagatla wrote:
> This patch adds bindings for Qualcomm soundwire controller.

> 

> Qualcomm SoundWire Master controller is present in most Qualcomm SoCs

> either integrated as part of WCD audio codecs via slimbus or

> as part of SOC I/O.

> 

> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---

>  .../bindings/soundwire/qcom,sdw.txt           | 167 ++++++++++++++++++

>  1 file changed, 167 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> 

> diff --git a/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt b/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> new file mode 100644

> index 000000000000..436547f3b155

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/soundwire/qcom,sdw.txt

> @@ -0,0 +1,167 @@

> +Qualcomm SoundWire Controller Bindings

> +

> +

> +This binding describes the Qualcomm SoundWire Controller along with its

> +board specific bus parameters.

> +

> +- compatible:

> +	Usage: required

> +	Value type: <stringlist>

> +	Definition: must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",

> +		    Example:

> +			"qcom,soundwire-v1.3.0"

> +			"qcom,soundwire-v1.5.0"

> +			"qcom,soundwire-v1.6.0"

> +- reg:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: the base address and size of SoundWire controller

> +		    address space.

> +

> +- interrupts:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: should specify the SoundWire Controller IRQ

> +

> +- clock-names:

> +	Usage: required

> +	Value type: <stringlist>

> +	Definition: should be "iface" for SoundWire Controller interface clock

> +

> +- clocks:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: should specify the SoundWire Controller interface clock

> +

> +- #sound-dai-cells:

> +	Usage: required

> +	Value type: <u32>

> +	Definition: must be 1 for digital audio interfaces on the controller.

> +

> +- qcom,dout-ports:

> +	Usage: required

> +	Value type: <u32>

> +	Definition: must be count of data out ports

> +

> +- qcom,din-ports:

> +	Usage: required

> +	Value type: <u32>

> +	Definition: must be count of data in ports

> +

> +- qcom,ports-offset1:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: should specify payload transport window offset1 of each

> +		    data port. Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-offset2:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: should specify payload transport window offset2 of each

> +		    data port. Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.


Do we need to define these two in DT? Would this not be allocated in
Software and programmed?

> +

> +- qcom,ports-sinterval-low:

> +	Usage: required

> +	Value type: <prop-encoded-array>

> +	Definition: should be sample interval low of each data port.

> +		    Out ports followed by In ports. Used for Sample Interval

> +		    calculation.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-word-length:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be size of payload channel sample.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-block-pack-mode:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be 0 or 1 to indicate the block packing mode.

> +		    0 to indicate Blocks are per Channel

> +		    1 to indicate Blocks are per Port.

> +		    Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-block-group-count:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be in range 1 to 4 to indicate how many sample

> +		    intervals are combined into a payload.

> +		    Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-lane-control:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be in range 0 to 7 to identify which	data lane

> +		    the data port uses.

> +		    Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-hstart:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be number identifying lowerst numbered coloum in

> +		    SoundWire Frame, i.e. left edge of the Transport sub-frame

> +		    for each port. Values between 0 and 15 are valid.

> +		    Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.

> +

> +- qcom,ports-hstop:

> +	Usage: optional

> +	Value type: <prop-encoded-array>

> +	Definition: should be number identifying highest numbered coloum in

> +		    SoundWire Frame, i.e. the right edge of the Transport

> +		    sub-frame for each port. Values between 0 and 15 are valid.

> +		    Out ports followed by In ports.

> +		    More info in MIPI Alliance SoundWire 1.0 Specifications.


Ditto with these two as well
-- 
~Vinod