| 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
| ||
|
Message-ID: <47b591c0-2f68-429d-6d1b-fa8b701785ac@linaro.org>
Date: Tue, 7 Mar 2023 13:49:40 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Varadarajan Narayanan <quic_varada@...cinc.com>
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Georgi Djakov <djakov@...nel.org>
Subject: Re: [PATCH 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes
On 07/03/2023 08:36, Varadarajan Narayanan wrote:
>
> On 3/6/2023 5:21 PM, Dmitry Baryshkov wrote:
>> On 06/03/2023 13:26, Varadarajan Narayanan wrote:
>>> Dmitry,
>>>
>>> On 3/2/2023 9:52 PM, Dmitry Baryshkov wrote:
>>>> On Thu, 2 Mar 2023 at 11:57, Varadarajan Narayanan
>>>> <quic_varada@...cinc.com> wrote:
>>>>> Add USB phy and controller related nodes
>>>>>
>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@...cinc.com>
>>>>> ---
>>>>> arch/arm64/boot/dts/qcom/ipq9574.dtsi | 92
>>>>> +++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 92 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> index 2bb4053..319b5bd 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>
>> [skipped]
>>
>>
>>>>> + usb3: usb3@...0000 {
>>>> You know the drill. This node is in the wrong place.
>>>>
>>>>> + compatible = "qcom,dwc3";
>>>>> + reg = <0x8AF8800 0x400>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <1>;
>>>>> + ranges;
>>>>> +
>>>>> + clocks = <&gcc GCC_SNOC_USB_CLK>,
>>>>> + <&gcc GCC_ANOC_USB_AXI_CLK>,
>>>>> + <&gcc GCC_USB0_MASTER_CLK>,
>>>>> + <&gcc GCC_USB0_SLEEP_CLK>,
>>>>> + <&gcc GCC_USB0_MOCK_UTMI_CLK>;
>>>>> +
>>>>> + clock-names = "sys_noc_axi",
>>>>> + "anoc_axi",
>>>>> + "master",
>>>>> + "sleep",
>>>>> + "mock_utmi";
>>>> Please fix the indentation of the lists.
>>>>
>>>>> +
>>>>> + assigned-clocks = <&gcc GCC_SNOC_USB_CLK>,
>>>>> + <&gcc GCC_ANOC_USB_AXI_CLK>,
>>>> Why do you assign clock rates to the NOC clocks? Should they be set
>>>> using the interconnect instead?
>>>
>>> The SNOC and ANOC run at a fixed speed of 350MHz and 342MHz
>>> respectively and are not scaled. These clocks are for the interface
>>> between the USB block and the SNOC/ANOC. Do we still need to use
>>> interconnect?
>>
>> Maybe I misunderstand something here. If the snoc and anoc speeds are
>> at 350 MHz and 342 MHz, why do you assign clock-rates of 200 MHz?
>>
>> Is it enough to call clk_prepare_enable() for these clocks or the rate
>> really needs to be set?
>
> The rate of 200MHz is not being set for the SNOC/ANOC. It is for the
> NIU that connects the USB and SNOC/ANOC. The reason for setting the
> rate to 200MHz is to configure the RCG parent for these interface
> clocks. That said can we configure this RCG standalone in the driver
> and enable these clocks?
We discussed this separately with Georgi Djakov. Let me quote his IRC
message: "it sounds like this is for USB port that connects to the NOC.
if bandwidth scaling is not needed (or other interconnect
configuration), then maybe this can go without interconnect provider
driver."
However as we discover more and more about this platform (e.g. PCIe
using the aggre_noc region to setup some magic registers, see [1]), I'm
more and more biased towards suggesting implementing the interconnect
driver to setup all these tiny little things. With the DT tree being an
ABI, it is much preferable to overestimate the needs rather than
underestimating them (and having to cope with the backwards
compatibility issues).
Generally I think that PCIe/USB/whatever should not poke into NoC
registers or NoC/NIU clocks directly (because this is a very
platform-specific item). Rather than that it should tell the
icc/opp/whatever subsystem, "please configure the SoC for me to work".
[1]
https://lore.kernel.org/linux-arm-msm/30cf9717-dcca-e984-c506-c71b7f8e32cd@quicinc.com/
>
> Thanks
> Varada
>
>
>>
>>
>>>
>>>>> + <&gcc GCC_USB0_MASTER_CLK>,
>>>>> + <&gcc
>>>>> GCC_USB0_MOCK_UTMI_CLK>;
>>>>> + assigned-clock-rates = <200000000>,
>>>>> + <200000000>,
>>>>> + <200000000>,
>>>>> + <24000000>;
>>>>> +
>>>>> + resets = <&gcc GCC_USB_BCR>;
>>>>> + status = "disabled";
>>>>> +
>>>>> + dwc_0: dwc3@...0000 {
>>>>> + compatible = "snps,dwc3";
>>>>> + reg = <0x8A00000 0xcd00>;
>>>>> + clock-names = "ref";
>>>>> + clocks = <&gcc
>>>>> GCC_USB0_MOCK_UTMI_CLK>;
>>>> clocks before clock-names
>>>>
>>>>> + interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
>>>>> + phys = <&qusb_phy_0>, <&usb0_ssphy>;
>>>>> + phy-names = "usb2-phy", "usb3-phy";
>>>>> + tx-fifo-resize;
>>>>> + snps,dis_ep_cache_eviction;
>>>>> + snps,is-utmi-l1-suspend;
>>>>> + snps,hird-threshold = /bits/ 8 <0x0>;
>>>>> + snps,dis_u2_susphy_quirk;
>>>>> + snps,dis_u3_susphy_quirk;
>>>>> + snps,quirk-frame-length-adjustment = <0x0A87F0A0>;
>>>>> + dr_mode = "host";
>>>>> + };
>>>>> + };
>>>>> +
>>>>> pcie0_phy: phy@...00 {
>>>>> compatible =
>>>>> "qcom,ipq9574-qmp-gen3x1-pcie-phy";
>>>>> reg = <0x00084000 0x1bc>; /* Serdes PLL */
>>>>> --
>>>>> 2.7.4
>>>
>>> Will address these and post a new revision.
>>>
>>> Thanks
>>>
>>> Varada
>>>
>>
--
With best wishes
Dmitry
Powered by blists - more mailing lists