| Message ID | 20230807193507.6488-2-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: enable EMAC1 on sa8775p | expand |
On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> FWIW this seems to match downstream sources. Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > --- > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi > index 7b55cb701472..38d10af37ab0 100644 > --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi > +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi > @@ -1846,6 +1846,15 @@ serdes0: phy@8901000 { > status = "disabled"; > }; > > + serdes1: phy@8902000 { > + compatible = "qcom,sa8775p-dwmac-sgmii-phy"; > + reg = <0x0 0x08902000 0x0 0xe10>; > + clocks = <&gcc GCC_SGMI_CLKREF_EN>; > + clock-names = "sgmi_ref"; > + #phy-cells = <0>; > + status = "disabled"; > + }; > + > pdc: interrupt-controller@b220000 { > compatible = "qcom,sa8775p-pdc", "qcom,pdc"; > reg = <0x0 0x0b220000 0x0 0x30000>, > -- > 2.39.2 >
On Mon, Aug 07, 2023 at 04:10:34PM -0500, Andrew Halaney wrote: > On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > FWIW this seems to match downstream sources. > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> Why does matching downstream make it correct and deserve a Reviewed-by? Did you actually review the change? Andrew
On Mon, Aug 07, 2023 at 11:28:15PM +0200, Andrew Lunn wrote: > On Mon, Aug 07, 2023 at 04:10:34PM -0500, Andrew Halaney wrote: > > On Mon, Aug 07, 2023 at 09:34:59PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Add a node for the SerDes PHY used by EMAC1 on sa8775p-ride. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > FWIW this seems to match downstream sources. > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > Why does matching downstream make it correct and deserve a > Reviewed-by? > > Did you actually review the change? > > Andrew > That wording of "match downstream sources" is amiguous, sorry. What I meant was that: 1. This looks like a properly formatted dtsi node, follows conventions in the file for ordering, etc 2. The downstream devicetree from Qualcomm uses the same MMIO region same dependencies for clocks, etc. I do not have documentation to further review past that I didn't suspect anyone else would cross check this information, so I did and thought it deserves a RB after that. Please let me know if you'd rather I just leave a comment saying such without any formal tags (but I thought since the whole patch looks proper and that information looks correct compared to the "documentation" I have access to it deserved such). In the rest of this series review I may have used similar phrasing with respect to downstream, but that expanded description is what I meant in those cases as well. Thanks, Andrew
> That wording of "match downstream sources" is amiguous, sorry. > > What I meant was that: > > 1. This looks like a properly formatted dtsi node, follows > conventions in the file for ordering, etc > 2. The downstream devicetree from Qualcomm uses the same MMIO region > same dependencies for clocks, etc. I do not have documentation > to further review past that O.K. This does make your reviews worthwhile. Vendor crap gets that name for a reason. So just saying it is the same as the vendor code is not really helpful. So i would avoid this ambiguous statement. And your later comment on a patch which points out real problems adds to my confidence you did a real review. Thanks Andrew
diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi index 7b55cb701472..38d10af37ab0 100644 --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi @@ -1846,6 +1846,15 @@ serdes0: phy@8901000 { status = "disabled"; }; + serdes1: phy@8902000 { + compatible = "qcom,sa8775p-dwmac-sgmii-phy"; + reg = <0x0 0x08902000 0x0 0xe10>; + clocks = <&gcc GCC_SGMI_CLKREF_EN>; + clock-names = "sgmi_ref"; + #phy-cells = <0>; + status = "disabled"; + }; + pdc: interrupt-controller@b220000 { compatible = "qcom,sa8775p-pdc", "qcom,pdc"; reg = <0x0 0x0b220000 0x0 0x30000>,