Re: [PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support

[Andrew Lunn]

On Mon, Jan 22, 2024 at 11:52:30PM +0800, Ziyang Huang wrote:
> 在 2024/1/22 0:45, Andrew Lunn 写道:
> > On Sun, Jan 21, 2024 at 08:42:36PM +0800, Ziyang Huang wrote:
> > > Signed-off-by: Ziyang Huang <hzyitc@xxxxxxxxxxx>
> > > ---
> > >   arch/arm64/boot/dts/qcom/ipq5018.dtsi | 120 +++++++++++++++++++++++++-
> > >   1 file changed, 116 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > index e502a3ecf4b7..b36e5c2136b7 100644
> > > --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> > > @@ -94,6 +94,63 @@ soc: soc@0 {
> > >   		#size-cells = <1>;
> > >   		ranges = <0 0 0 0xffffffff>;
> > > +		mdio0: mdio@88000 {
> > > +			#address-cells = <1>;
> > > +			#size-cells = <0>;
> > > +			compatible = "qcom,ipq5018-mdio", "qcom,qca-mdio";
> > > +			reg = <0x88000 0x64>;
> > > +			resets = <&gcc GCC_GEPHY_MDC_SW_ARES>,
> > > +				 <&gcc GCC_GEPHY_DSP_HW_ARES>;
> > 
> > What do these two resets do? An MDIO bus controller is unlikely to
> > have a DSP in it. That is something a PHY is more likely to have. An
> > MDIO bus controller does have an MDC line, but why is it
> > GCC_GEPHY_MDC_SW_ARES not GCC_MDIO_MDC_SW_ARES? So this again makes me
> > think this is a PHY reset, so should be in the PHY node.
> > 
> 
> IPQ5018 has two mdio bus. mdio0 is an internal bus which only connects to
> internal PHY while mdio1 is outgoing and can be used to connect PHY or
> switch.
> 
> So I thought GCC_GEPHY_MDC_SW_ARES is for mdio0 and GCC_MDIO_MDC_SW_ARES is
> for mdio1.
> 
> GCC_GEPHY_DSP_HW_ARES is seem like PHY reset. But if we don't deassert it,
> the phy node can't be scaned. So I add it here like what reset-gpios do - to
> reset PHY or switch.
> 
> > A device tree binding will help sort this out.
> 
> Base on the functions I explained, I can't write the accurate names and
> number of resets here.

Without a device tree binding, your patch will not be accepted. At
minimum, you need to describe your understanding of the hardware, what
you think the resets do.

Since this is an internal PHY, you know exactly what it is. So you can
use ID values in the compatible so the driver loads. The driver can
then deal with clocks and resets. With complex setups like this,
scanning is not always possible.

     Andrew