| 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: <84ad5269-dd48-32ef-1313-6241980834bc@quicinc.com>
Date: Fri, 20 Jan 2023 20:43:30 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Andrew Halaney <ahalaney@...hat.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
"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>,
Felipe Balbi <balbi@...nel.org>, <linux-usb@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <quic_pkondeti@...cinc.com>,
<quic_ppratap@...cinc.com>, <quic_wcheng@...cinc.com>,
<quic_jackp@...cinc.com>, <quic_harshq@...cinc.com>,
<quic_shazhuss@...cinc.com>
Subject: Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support
Multiport Controller
On 1/20/2023 8:07 PM, Andrew Halaney wrote:
> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller
>>>> which requires at most one HS and one SS PHY.
>>>>
>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>> each port can have their own PHYs. Each port of the multiport
>>>> controller can either be HS+SS capable or HS only capable
>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>> and GUSB3PIPECTL registers appropriately.
>>>>
>>>> Add support for detecting, obtaining and configuring phy's supported
>>>> by a multiport controller and limit the max number of ports
>>>> supported to 4.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@...cinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>>>> ---
>>>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>> drivers/usb/dwc3/core.h | 15 +-
>>>> drivers/usb/dwc3/drd.c | 14 +-
>>>> 3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 476b63618511..7e0a9a598dfd 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>
>>> <snip>
>>>
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>> dwc->dis_split_quirk = device_property_read_bool(dev,
>>>> "snps,dis-split-quirk");
>>>> +
>>>> + /*
>>>> + * If no mulitport properties are defined, default
>>>> + * the port count to '1'.
>>>> + */
>>>> + ret = device_property_read_u32(dev, "num-ports",
>>>> + &dwc->num_ports);
>>>> + if (ret)
>>>> + dwc->num_ports = 1;
>>>> +
>>>> + ret = device_property_read_u32(dev, "num-ss-ports",
>>>> + &dwc->num_ss_ports);
>>>> + if (ret)
>>>> + dwc->num_ss_ports = 1;
>>>
>>> By using this DT property instead of using the number of each phy type you
>>> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
>>> when there's no phy to go along with it.
>>>
>> Hi Andrew,
>>
>> Thanks for the review. Yes, this decoupling is still there and its fine I
>> believe.
>>
>>> I ran into this when testing on sa8540p-ride, which only uses one of the
>>> ports on the multiport controller. I didn't enable the other phys (not
>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>> happened on a quick test.. but I thought I'd highlight that as another
>>> downside of decoupling this value from the number of phys you grab.
>>>
>> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
>> are just defaulted to '1' and as per the current logic only port-1 registers
>> must be configured. Isn't that the case happening ?
>>
>
> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
> So unless I override them I get this from your sc8280xp.dtsi:
>
> + usb_2_dwc3: usb@...0000 {
> + compatible = "snps,dwc3";
> + reg = <0 0x0a400000 0 0xcd00>;
> + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> + iommus = <&apps_smmu 0x800 0x0>;
> + num-ports = <4>;
> + num-ss-ports = <2>;
> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> + <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> + <&usb_2_hsphy2>,
> + <&usb_2_hsphy3>;
> + phy-names = "usb2-phy_port0", "usb3-phy_port0",
> + "usb2-phy_port1", "usb3-phy_port1",
> + "usb2-phy_port2",
> + "usb2-phy_port3";
> + };
>
> Since this board only uses one port of the multiport controller, I
> redefined phys/phy-names to indicate that. I figured that was more
> desireable than enabling unnecessary phys. Without overriding
> num-ports/num-ss-ports all the for loops in this patch would act like
> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
> multiple times etc as well as look for the multiport phy-names and fail
> to actually get any phys. Hope that makes sense!
>
Hi Andrew,
My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that
case it makes complete sense to override the num-ports and num-ss-ports
to "1" and the usb phy-names.
>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>> series (probably needs clean up after review, and will definitely need
>>> alteration after you update the dt-binding again). If not I'll continue
>>> to test/review so please CC me!:
>>>
>>>
>> Sure, I can add this patch (probably will add the other phy's too) during
>> the final submission.
>
> I don't have a great understanding of the mapping of the phys to
> physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
> so if it makes more sense to enable all the relevant SoC phys, write
> those registers in the DWC3 IP, etc, and only use one of the actual
> board outputs then feel free. I think this is a good example of "what if
> a board designer only uses a single port of the multiport IP" imo.
> Agreed. This could be a good example of multi port with only single port
working.
>>
>>> From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
>>> From: Andrew Halaney <ahalaney@...hat.com>
>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>> Content-type: text/plain
>>>
>>> There is now support for the multiport USB controller this uses
>>> so enable it.
>>>
>>> The board only has a single port hooked up (despite it being wired up to
>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
>>> which by default on boot is selected to mux properly. Grab the gpio
>>> controlling that and ensure it stays in the right position so USB 2.0
>>> continues to be routed from the external port to the SoC.
>>>
>>> Signed-off-by: Andrew Halaney <ahalaney@...hat.com>
>>> ---
>>> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>>> 1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> index 97957f3baa64..56d4f43faa1e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>> status = "okay";
>>> };
>>> +&usb_2 {
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&usb2_en_state>;
>>> +
>>> + status = "okay";
>>> +};
>>> +
>>> +&usb_2_dwc3 {
>>> + dr_mode = "host";
>>> + num-ports = <1>;
>>> + num-ss-ports = <1>;
>>
>> More over, if this is a multiport controller and you are using only port-1,
>> it is as good as a single port controller I believe and the normal DT
>> convention must work. Adding these properties as "1" is not required as the
>> driver logic defaults them to "1" if they are not found.
>
> See above comment about inheriting from sc8280xp.dtsi and needing to
> override their values.
>
>>
>> Just to add a point here (as I was not clear in DT Binding description, My
>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
>> HW whether they are used in DT or not. Just to cover all cases which user
>> can use [1].
>>
>> []1:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
>
> Ok, if you're going with that approach of "must indicate the HS/SS Phys
> present on HW whether they are used in the DT or not" (/me assumes DT
> here means on the board and not an incorrect coding of the DT) then I
> suppose I should not have overridden anything but phys/phy-names to
> indicate that I'm only using the first port (and used the multiport
> phy-names convention). It looks like in that link you also mention that
> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
> defined, which was my concern and reasoning above for overriding
> num-ports/num-ss-ports.
>
> Thanks,
> Andrew
>
Actually, I was trying to mandate that rule to take care of cases where
the phy's for say port2 or port3 are missing for a quad port controller
in dtsi and we don't want to end up configuring wrong dwc3-phy regs.
For just the first port, the changes you have mentioned must be
sufficient. (Furthermore, thanks for the review and testing it on
sa8295-ride and confirming nothing breaks while the first port is enabled)
Regards,
Krishna,
>>
>> Regards,
>> Krishna,
>>
>>> + phy-names = "usb2-phy", "usb3-phy";
>>> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>> +};
>>> +
>>> &usb_2_hsphy0 {
>>> vdda-pll-supply = <&vreg_l5a>;
>>> vdda18-supply = <&vreg_l7g>;
>>> @@ -313,4 +328,13 @@ wake-pins {
>>> bias-pull-up;
>>> };
>>> };
>>> +
>>> + usb2_en_state: usb2-en-state {
>>> + /* TS3USB221A USB2.0 mux select */
>>> + pins = "gpio24";
>>> + function = "gpio";
>>> + drive-strength = <2>;
>>> + bias-disable;
>>> + output-low;
>>> + };
>>> };
>>
>
Powered by blists - more mailing lists