| Message ID | 20181026173544.136037-1-evgreen@chromium.org |
|---|---|
| Headers | show |
| Series | arm64: dts: qcom: sdm845: Add UFS DT nodes | expand |
Quoting Evan Green (2018-10-26 10:35:43) > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > index eedfaf8922e2..d5fddea71a85 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > @@ -356,6 +356,20 @@ > status = "okay"; > }; > > +&ufshc1 { > + status = "okay"; > + > + vcc-supply = <&vreg_l20a_2p95>; > + vcc-max-microamp = <600000>; Is this board dependent? I would guess this is SoC specific and not board specific. > +}; > + > +&ufsphy1 { > + status = "okay"; > + > + vdda-phy-supply = <&vdda_ufs1_core>; > + vdda-pll-supply = <&vdda_ufs1_1p2>; These two properties can be specified in the SoC dtsi file instead of each board variant file. This way we don't have to specify the things that are SoC independent in each board file. The board integrator just has to attach the labels to the right regulator nodes, in this case vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be matched up with the right regulator automatically. It's also nice so that board integrators don't have to know anything besides what regulator goes to what pin on the SoC. The status property has to stay of course.
Hi, On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Evan Green (2018-10-26 10:35:43) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > > index eedfaf8922e2..d5fddea71a85 100644 > > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > > @@ -356,6 +356,20 @@ > > status = "okay"; > > }; > > > > +&ufshc1 { > > + status = "okay"; > > + > > + vcc-supply = <&vreg_l20a_2p95>; > > + vcc-max-microamp = <600000>; > > Is this board dependent? I would guess this is SoC specific and not > board specific. > > > +}; > > + > > +&ufsphy1 { > > + status = "okay"; > > + > > + vdda-phy-supply = <&vdda_ufs1_core>; > > + vdda-pll-supply = <&vdda_ufs1_1p2>; > > These two properties can be specified in the SoC dtsi file instead of > each board variant file. This way we don't have to specify the things > that are SoC independent in each board file. The board integrator just > has to attach the labels to the right regulator nodes, in this case > vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be > matched up with the right regulator automatically. It's also nice so > that board integrators don't have to know anything besides what > regulator goes to what pin on the SoC. This is an interesting proposal and it feels like we have to consider the tradeoffs. I agree that it would be nice not to have to specify this in every single board .dts file, but at the same time what if you've got a board that doesn't use UFS? Such a board would bother adding the labels "vdda_ufs1_core" and "vdda_ufs1_1p2". This would lead to a compile error in the device tree bindings. That seems pretty undesirable. +Bjorn since I think Bjorn wasn't a huge fan of the labels like "vdda_ufs1_core" to start with. -Doug
Quoting Doug Anderson (2018-11-19 11:25:08) > On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Evan Green (2018-10-26 10:35:43) > > > > > +}; > > > + > > > +&ufsphy1 { > > > + status = "okay"; > > > + > > > + vdda-phy-supply = <&vdda_ufs1_core>; > > > + vdda-pll-supply = <&vdda_ufs1_1p2>; > > > > These two properties can be specified in the SoC dtsi file instead of > > each board variant file. This way we don't have to specify the things > > that are SoC independent in each board file. The board integrator just > > has to attach the labels to the right regulator nodes, in this case > > vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be > > matched up with the right regulator automatically. It's also nice so > > that board integrators don't have to know anything besides what > > regulator goes to what pin on the SoC. > > This is an interesting proposal and it feels like we have to consider > the tradeoffs. > > I agree that it would be nice not to have to specify this in every > single board .dts file, but at the same time what if you've got a > board that doesn't use UFS? Such a board would bother adding the > labels "vdda_ufs1_core" and "vdda_ufs1_1p2". This would lead to a > compile error in the device tree bindings. That seems pretty > undesirable. > A workaround for this somewhat rare case would be to specify /delete-property/ on those nodes that aren't used. Unless that can't even work because the phandle is parsed before properties are deleted? I haven't tried. Or we could try to have dtc ignore broken phandles in status = "disabled" nodes or omit them entirely from the dtb so this isn't a problem. Either way, I would push for making it easier for the users of the SoC to not need to know the SoC internal details too much and rely on the SoC dtsi file to get it right. From the user perspective it's just a bunch of pins connected to something. We could also have a 0 volt "ground" regulator for any grounded/unconnected pins if that helps. It could be marked as status = "disabled" so that no runtime code is used while dtc is still happy to have the disabled node with a label.
On Mon 19 Nov 11:42 PST 2018, Stephen Boyd wrote: > Quoting Doug Anderson (2018-11-19 11:25:08) > > On Mon, Nov 19, 2018 at 11:19 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Evan Green (2018-10-26 10:35:43) > > > > > > > +}; > > > > + > > > > +&ufsphy1 { > > > > + status = "okay"; > > > > + > > > > + vdda-phy-supply = <&vdda_ufs1_core>; > > > > + vdda-pll-supply = <&vdda_ufs1_1p2>; > > > > > > These two properties can be specified in the SoC dtsi file instead of > > > each board variant file. This way we don't have to specify the things > > > that are SoC independent in each board file. The board integrator just > > > has to attach the labels to the right regulator nodes, in this case > > > vdda_ufs1_core and vdda_ufs1_1p2, and then the sdm845.dtsi file will be > > > matched up with the right regulator automatically. It's also nice so > > > that board integrators don't have to know anything besides what > > > regulator goes to what pin on the SoC. > > > > This is an interesting proposal and it feels like we have to consider > > the tradeoffs. > > > > I agree that it would be nice not to have to specify this in every > > single board .dts file, but at the same time what if you've got a > > board that doesn't use UFS? Such a board would bother adding the > > labels "vdda_ufs1_core" and "vdda_ufs1_1p2". This would lead to a > > compile error in the device tree bindings. That seems pretty > > undesirable. > > > > A workaround for this somewhat rare case would be to specify > /delete-property/ on those nodes that aren't used. Unless that can't > even work because the phandle is parsed before properties are deleted? I > haven't tried. Or we could try to have dtc ignore broken phandles in > status = "disabled" nodes or omit them entirely from the dtb so this > isn't a problem. > > Either way, I would push for making it easier for the users of the SoC > to not need to know the SoC internal details too much and rely on the > SoC dtsi file to get it right. From the user perspective it's just a > bunch of pins connected to something. We could also have a 0 volt > "ground" regulator for any grounded/unconnected pins if that helps. It > could be marked as status = "disabled" so that no runtime code is used > while dtc is still happy to have the disabled node with a label. > I dislike the use of the labels "vdda_ufs1_core" as that doesn't tell me which regulator this is actually wired to, without having to jump around in the board dts file. It's annoying, but it's pretty much write-once so it's not a big issue. But I really don't like the idea of having sdm845.dtsi depend on labels specified in the board dtsi. This just means that in order to add a new board I need to figure out the information and the way to specify it is to change a label in the board file. The static configuration here is that vdda-phy-supply is connected to the vdda_ufs1_core pin on the SoC, which is connected to some board specific regulator. If anything I think we should model that intermediate entity, in which case we could move the link between the phy and the pin to the platform dtsi. But I'm fine with us just merging Evan's patch as is. Regards, Bjorn
On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote: > Add the second lane registers for the USB PHY, now that the > QMP phy bindings have been updated. This way the driver can stop > reaching beyond its register region to get at the second lane. > > Signed-off-by: Evan Green <evgreen@chromium.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > > Changes in v5: None > Changes in v4: None > Changes in v3: > - Removed erroneous fixup for USB UniPro PHY, which is not dual lane (Doug) > > Changes in v2: None > > arch/arm64/boot/dts/qcom/sdm845.dtsi | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index 9c72edb678ec..ff2db36ec4fa 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -1188,10 +1188,12 @@ > <&gcc GCC_USB3_PHY_PRIM_BCR>; > reset-names = "phy", "common"; > > - usb_1_ssphy: lane@88e9200 { > + usb_1_ssphy: lanes@88e9200 { > reg = <0x88e9200 0x128>, > <0x88e9400 0x200>, > <0x88e9c00 0x218>, > + <0x88e9600 0x128>, > + <0x88e9800 0x200>, > <0x88e9a00 0x100>; > #phy-cells = <0>; > clocks = <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>; > -- > 2.16.4 >
On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote: > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index b72bdb0a31a5..9c72edb678ec 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -808,6 +808,73 @@ > }; > }; > > + ufshc1: ufshc@1d84000 { There's only one ufshc and one ufsphy, so no need to include the index. [..] > + resets = <&gcc GCC_UFS_PHY_BCR>; > + reset-names = "rst"; I have this as well, but this is not used by the upstream driver nor is it mentioned in the dt-binding. > + > + status = "disabled"; > + }; > + > + ufsphy1: phy@1d87000 { With reservation for the "reset" issue: Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn
On Wed, Nov 21, 2018 at 11:18 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 26 Oct 10:35 PDT 2018, Evan Green wrote: > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > index b72bdb0a31a5..9c72edb678ec 100644 > > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > > @@ -808,6 +808,73 @@ > > }; > > }; > > > > + ufshc1: ufshc@1d84000 { > > There's only one ufshc and one ufsphy, so no need to include the index. Aren't there two UFS controllers on SDM845, a "card" one and a "mem" one? I'm only adding the "mem" one here since that's all I can test, but I thought it made sense to leave the number there so someone could add the "card" one later if needed. > > [..] > > + resets = <&gcc GCC_UFS_PHY_BCR>; > > + reset-names = "rst"; > > I have this as well, but this is not used by the upstream driver nor is > it mentioned in the dt-binding. I see it in Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt, but then the only place I see it being used is ufs-hisi.c. So you're right, I think I should spin and remove this. Since I'm spinning, let me know about the numbering thing above. > > > + > > + status = "disabled"; > > + }; > > + > > + ufsphy1: phy@1d87000 { > > With reservation for the "reset" issue: > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Regards, > Bjorn