linux-kernel - Re: [PATCH v2 1/5] soundwire: qcom: add support to missing transport params

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Mar 2021 09:36:24 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        vkoul@...nel.org
Cc:     sanyog.r.kale@...el.com, yung-chuan.liao@...ux.intel.com,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH v2 1/5] soundwire: qcom: add support to missing transport
 params



On 02/03/2021 14:29, Pierre-Louis Bossart wrote:
> 
>>>>       for (i = 0; i < nports; i++) {
>>>>           ctrl->pconfig[i].si = si[i];
>>>>           ctrl->pconfig[i].off1 = off1[i];
>>>>           ctrl->pconfig[i].off2 = off2[i];
>>>>           ctrl->pconfig[i].bp_mode = bp_mode[i];
>>>> +        ctrl->pconfig[i].hstart = hstart[i];
>>>> +        ctrl->pconfig[i].hstop = hstop[i];
>>>> +        ctrl->pconfig[i].word_length = word_length[i];
>>>> +        ctrl->pconfig[i].blk_group_count = blk_group_count[i];
>>>> +        ctrl->pconfig[i].lane_control = lane_control[i];
>>>>       }
>>>
>>> I don't get why you test the values parsed from DT before writing the 
>>> registers. Why do test them here? if some values are incorrect it's 
>>> much better to provide an error log instead of writing a partially 
>>> valid setup to hardware, no?
>>
>> from DT we pass parameters for all the master ports, however some of 
>> these parameters are not really applicable for some of the ports! so 
>> the way we handle this is by marking them as 0xFF which means these 
>> values are not applicable for those ports! Having said that I think I 
>> should probably redefine SWR_INVALID_PARAM to QCOM_SWR_PARAM_NA or 
>> something on those lines!
> 
> Humm, do you have an example here? It's a bit odd to define DT 



In DT we describe parameters for each port in an array so, parameters 
for ports that are not applicable/available for that platform can be 
marked with 0xFF.

Most importantly, Some of these registers are only implemented based on 
the data ports. Ex: GROUP_CONTROL register is only implemented for data 
ports that support Group Count other than 0. Like wise for HSTART/STOP 
for Full Data ports only!
So avoiding reading/writing to registers by passing 
invalid/not-applicable value 0xFF made more sense!

Here are some examples of controller instances on SM8250 SoC.

soundwire-controller@...0000 {
	reg = <0 0x3230000 0 0x2000>;
	compatible = "qcom,soundwire-v1.5.1";
	interrupts-extended = <&intc GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>,
			<&pdc 109 IRQ_TYPE_LEVEL_HIGH>;
	interrupt-names = "core", "wakeup";

	qcom,clock-stop-mode0;
	clocks = <&txmacro>;
	clock-names = "iface";

	qcom,din-ports = <5>;
	qcom,dout-ports = <0>;
	qcom,ports-sinterval-low = /bits/ 8 <0xFF 0x01 0x01 0x03 0x03>;
	qcom,ports-offset1 = /bits/ 8 <0xFF 0x01 0x00 0x02 0x00>;
	qcom,ports-offset2 = /bits/ 8 <0xFF 0x00 0x00 0x00 0x00>;
	qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>;
	qcom,ports-hstart = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>;
	qcom,ports-hstop = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>;
	qcom,ports-word-length = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>;
	qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0xFF>;
	qcom,ports-lane-control = /bits/ 8 <0xFF 0x00 0x01 0x00 0x01>;
	qcom,port-offset = <1>;
	#sound-dai-cells = <1>;
	#address-cells = <2>;
	#size-cells = <0>;
};


soundwire-controller@...0000 {
	reg = <0 0x3210000 0 0x2000>;
	compatible = "qcom,soundwire-v1.5.1";
	interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
	clocks = <&rxmacro>;
	clock-names = "iface";
	qcom,clock-stop-mode0;
	qcom,din-ports = <0>;
	qcom,dout-ports = <5>;

	qcom,ports-sinterval-low = /bits/ 8 <0x03 0x1F 0x1F 0x07 0x00>;
	qcom,ports-offset1 = /bits/ 8 <0x00 0x00 0x0B 0x01 0x00>;
	qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x0B 0x00 0x00>;
	qcom,ports-hstart = /bits/ 8 <0xFF 0x03 0xFF 0xFF 0xFF>;
	qcom,ports-hstop = /bits/ 8 <0xFF 0x06 0xFF 0xFF 0xFF>;
	qcom,ports-word-length = /bits/ 8 <0x01 0x07 0x04 0xFF 0xFF>;
	qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0x00 0x01 0xFF 0xFF>;
	qcom,ports-lane-control = /bits/ 8 <0x01 0x00 0x00 0x00 0x00>;
	qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0x00>;

	#sound-dai-cells = <1>;
	#address-cells = <2>;
	#size-cells = <0>;
};
> properties that may or may not be valid. If this is intentional and 
> desired, this should still be captured somehow, e.g. in the bindings 
> documentation or in the code with a comment, no?

Yes, I agree with you on this, I should document this in bindings!

--srini