* [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller
2018-10-26 17:35 [PATCH v5 0/5] arm64: dts: qcom: sdm845: Add UFS DT nodes Evan Green
@ 2018-10-26 17:35 ` Evan Green
2018-11-22 7:18 ` Bjorn Andersson
2018-10-26 17:35 ` [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp Evan Green
2018-10-26 17:35 ` [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two Evan Green
2 siblings, 1 reply; 12+ messages in thread
From: Evan Green @ 2018-10-26 17:35 UTC (permalink / raw)
To: Rob Herring, Andy Gross, Kishon Vijay Abraham I
Cc: Douglas Anderson, Stephen Boyd, Evan Green, devicetree,
linux-arm-msm, linux-kernel, Rob Herring, David Brown,
Mark Rutland, linux-soc
Add the UFS controller and PHY to SDM845.
Signed-off-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
As Doug mentioned in v2, this should land after (or with) the driver fix
in this series.
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Renamed ufsphy to phy (Vivek)
- Removed #clock-cells (Vivek)
arch/arm64/boot/dts/qcom/sdm845.dtsi | 67 ++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
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 {
+ compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
+ "jedec,ufs-2.0";
+ reg = <0x1d84000 0x2500>;
+ interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
+ phys = <&ufsphy1_lanes>;
+ phy-names = "ufsphy";
+ lanes-per-direction = <2>;
+ power-domains = <&gcc UFS_PHY_GDSC>;
+
+ clock-names =
+ "core_clk",
+ "bus_aggr_clk",
+ "iface_clk",
+ "core_clk_unipro",
+ "ref_clk",
+ "tx_lane0_sync_clk",
+ "rx_lane0_sync_clk",
+ "rx_lane1_sync_clk";
+ clocks =
+ <&gcc GCC_UFS_PHY_AXI_CLK>,
+ <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
+ <&gcc GCC_UFS_PHY_AHB_CLK>,
+ <&gcc GCC_UFS_PHY_UNIPRO_CORE_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>,
+ <&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
+ <&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
+ <&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
+ freq-table-hz =
+ <50000000 200000000>,
+ <0 0>,
+ <0 0>,
+ <37500000 150000000>,
+ <0 0>,
+ <0 0>,
+ <0 0>,
+ <0 0>;
+
+ resets = <&gcc GCC_UFS_PHY_BCR>;
+ reset-names = "rst";
+
+ status = "disabled";
+ };
+
+ ufsphy1: phy@1d87000 {
+ compatible = "qcom,sdm845-qmp-ufs-phy";
+ reg = <0x1d87000 0x18c>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ clock-names = "ref",
+ "ref_aux";
+ clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
+ <&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
+
+ status = "disabled";
+
+ ufsphy1_lanes: lanes@1d87400 {
+ reg = <0x1d87400 0x108>,
+ <0x1d87600 0x1e0>,
+ <0x1d87c00 0x1dc>,
+ <0x1d87800 0x108>,
+ <0x1d87a00 0x1e0>;
+ #phy-cells = <0>;
+ };
+ };
+
tcsr_mutex_regs: syscon@1f40000 {
compatible = "syscon";
reg = <0x1f40000 0x40000>;
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller
2018-10-26 17:35 ` [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller Evan Green
@ 2018-11-22 7:18 ` Bjorn Andersson
2018-11-28 23:43 ` Evan Green
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2018-11-22 7:18 UTC (permalink / raw)
To: Evan Green
Cc: Rob Herring, Andy Gross, Kishon Vijay Abraham I, Douglas Anderson,
Stephen Boyd, devicetree, linux-arm-msm, linux-kernel,
Rob Herring, David Brown, Mark Rutland, linux-soc
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller
2018-11-22 7:18 ` Bjorn Andersson
@ 2018-11-28 23:43 ` Evan Green
0 siblings, 0 replies; 12+ messages in thread
From: Evan Green @ 2018-11-28 23:43 UTC (permalink / raw)
To: Bjorn Andersson
Cc: robh, Andy Gross, kishon, Doug Anderson, swboyd, devicetree,
linux-arm-msm, linux-kernel, robh+dt, David Brown, mark.rutland,
linux-soc
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-10-26 17:35 [PATCH v5 0/5] arm64: dts: qcom: sdm845: Add UFS DT nodes Evan Green
2018-10-26 17:35 ` [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller Evan Green
@ 2018-10-26 17:35 ` Evan Green
2018-11-19 19:19 ` Stephen Boyd
2018-10-26 17:35 ` [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two Evan Green
2 siblings, 1 reply; 12+ messages in thread
From: Evan Green @ 2018-10-26 17:35 UTC (permalink / raw)
To: Rob Herring, Andy Gross, Kishon Vijay Abraham I
Cc: Douglas Anderson, Stephen Boyd, Can Guo, Evan Green, devicetree,
linux-arm-msm, linux-kernel, Rob Herring, David Brown,
Mark Rutland, linux-soc
From: Can Guo <cang@codeaurora.org>
Enable the UFS host controller and PHY on sdm845-mtp.
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 14 ++++++++++++++
1 file changed, 14 insertions(+)
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>;
+};
+
+&ufsphy1 {
+ status = "okay";
+
+ vdda-phy-supply = <&vdda_ufs1_core>;
+ vdda-pll-supply = <&vdda_ufs1_1p2>;
+};
+
&usb_1 {
status = "okay";
};
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-10-26 17:35 ` [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp Evan Green
@ 2018-11-19 19:19 ` Stephen Boyd
2018-11-19 19:25 ` Doug Anderson
0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2018-11-19 19:19 UTC (permalink / raw)
To: Andy Gross, Kishon Vijay Abraham I, Rob Herring
Cc: Douglas Anderson, Can Guo, Evan Green, devicetree, linux-arm-msm,
linux-kernel, Rob Herring, David Brown, Mark Rutland, linux-soc
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-11-19 19:19 ` Stephen Boyd
@ 2018-11-19 19:25 ` Doug Anderson
2018-11-19 19:42 ` Stephen Boyd
0 siblings, 1 reply; 12+ messages in thread
From: Doug Anderson @ 2018-11-19 19:25 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andy Gross, Evan Green, Kishon Vijay Abraham I, Rob Herring, cang,
devicetree, linux-arm-msm, LKML, Rob Herring, David Brown,
Mark Rutland, open list:ARM/QUALCOMM SUPPORT, Bjorn Andersson
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-11-19 19:25 ` Doug Anderson
@ 2018-11-19 19:42 ` Stephen Boyd
2018-11-22 7:04 ` Bjorn Andersson
0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2018-11-19 19:42 UTC (permalink / raw)
To: Doug Anderson
Cc: Andy Gross, Evan Green, Kishon Vijay Abraham I, Rob Herring, cang,
devicetree, linux-arm-msm, LKML, Rob Herring, David Brown,
Mark Rutland, open list:ARM/QUALCOMM SUPPORT, Bjorn Andersson
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-11-19 19:42 ` Stephen Boyd
@ 2018-11-22 7:04 ` Bjorn Andersson
2018-11-28 2:31 ` Stephen Boyd
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2018-11-22 7:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Doug Anderson, Andy Gross, Evan Green, Kishon Vijay Abraham I,
Rob Herring, cang, devicetree, linux-arm-msm, LKML, Rob Herring,
David Brown, Mark Rutland, open list:ARM/QUALCOMM SUPPORT
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp
2018-11-22 7:04 ` Bjorn Andersson
@ 2018-11-28 2:31 ` Stephen Boyd
0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2018-11-28 2:31 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Doug Anderson, Andy Gross, Evan Green, Kishon Vijay Abraham I,
Rob Herring, cang, devicetree, linux-arm-msm, LKML, Rob Herring,
David Brown, Mark Rutland, open list:ARM/QUALCOMM SUPPORT
Quoting Bjorn Andersson (2018-11-21 23:04:12)
> On Mon 19 Nov 11:42 PST 2018, Stephen Boyd wrote:
> >
> > 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.
>
Hmm alright. This is a similar problem that the DT connectors have with
phandle remapping through the connector to the actual phandle that is
desired. I can think of two solutions. One would be to make a
phandle-map binding to remap phandles within the soc node to actually be
another phandle. The downside is that we need to make nodes for
everything just to remap them. For example:
soc {
phandle-map {
vdda_ufs1_core: vdda-ufs1-core {
}
};
};
In the SoC dtsi file and then
&vdda_ufs1_core {
phandle-alias = <&pm8998_ldo19>
};
Would be in the board specific dtsi file.
The regulator code to fix that up is rather simple, but not generic. It
just walks the chain of alias nodes until it doesn't find anything else.
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 210fc20f7de7..9c1346374351 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -432,6 +432,15 @@ static int of_node_match(struct device *dev, const void *data)
struct regulator_dev *of_find_regulator_by_node(struct device_node *np)
{
struct device *dev;
+ struct device_node *aliased_np;
+
+ do {
+ aliased_np = of_parse_phandle(np, "phandle-alias", 0);
+
+ if (aliased_np)
+ np = aliased_np;
+
+ } while (aliased_np);
dev = class_find_device(®ulator_class, NULL, np, of_node_match);
Another idea would be to have a phandle nexus node that converts
phandles into some #cells property. This way, we can make the regulator
supply binding parse the cells of the nexus node and figure out that the
next specifier after it corresponds to something that needs to be set in
the nexus node by the board.
soc {
soc_regulator: regulator-nexus {
#regulator-cells = <1>;
#define VDDA_UFS1_CORE 0
};
ufs {
vdda-supply = <&soc_regulator VDDA_UFS1_CORE>;
};
};
In the SoC dtsi file and then
&soc_regulator {
regulator-map = <VDDA_UFS1_CORE &pm8998_ldo19>
}
And then some parsing code in regulator core to remap the cells on the
left side to the phandle on the right of the cells. I suppose that makes
things sort of awkward and makes the binding look different depending on
if the node has the #regulator-cells property or not for the supply
binding.
Another idea would be to have dts phandle fixups applied somehow when
the DT is loaded by the kernel or possibly bootloader or extra
efficiently with a compiler change. So then we can alias labels to
different labels in the board file.
So in the board dtsi file we would have something like
soc {
phandle-map {
vdda-ufs1-core = <&pm8998_ldo19>;
};
};
and the SoC dtsi file would have:
soc {
phandle-map {
vdda_ufs1_core: vdda-ufs1-core = <0>;
};
};
except I can't get this to compile right now because syntax errors.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two
2018-10-26 17:35 [PATCH v5 0/5] arm64: dts: qcom: sdm845: Add UFS DT nodes Evan Green
2018-10-26 17:35 ` [PATCH v5 3/5] arm64: dts: qcom: sdm845: add UFS controller Evan Green
2018-10-26 17:35 ` [PATCH v5 4/5] arm64: dts: qcom: sdm845: Add UFS nodes for sdm845-mtp Evan Green
@ 2018-10-26 17:35 ` Evan Green
2018-11-22 7:07 ` Bjorn Andersson
2 siblings, 1 reply; 12+ messages in thread
From: Evan Green @ 2018-10-26 17:35 UTC (permalink / raw)
To: Rob Herring, Andy Gross, Kishon Vijay Abraham I
Cc: Douglas Anderson, Stephen Boyd, Evan Green, devicetree,
linux-arm-msm, linux-kernel, Rob Herring, David Brown,
Mark Rutland, linux-soc
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>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two
2018-10-26 17:35 ` [PATCH v5 5/5] arm64: dts: qcom: sdm845: Add USB PHY lane two Evan Green
@ 2018-11-22 7:07 ` Bjorn Andersson
0 siblings, 0 replies; 12+ messages in thread
From: Bjorn Andersson @ 2018-11-22 7:07 UTC (permalink / raw)
To: Evan Green
Cc: Rob Herring, Andy Gross, Kishon Vijay Abraham I, Douglas Anderson,
Stephen Boyd, devicetree, linux-arm-msm, linux-kernel,
Rob Herring, David Brown, Mark Rutland, linux-soc
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread