diff mbox series

[3/6] arm64: dts: qcom: ipq5332: Add MDIO device tree

Message ID 20240110112059.2498-4-quic_luoj@quicinc.com (mailing list archive)
State Changes Requested
Headers show
Series Add PPE device tree node for Qualcomm IPQ SoC | expand

Commit Message

Jie Luo Jan. 10, 2024, 11:20 a.m. UTC
Add the MDIO device tree of ipq5332.

Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Krzysztof Kozlowski Jan. 10, 2024, 11:56 a.m. UTC | #1
On 10/01/2024 12:20, Luo Jie wrote:
> Add the MDIO device tree of ipq5332.

Subject: drop "device tree", it is obvious. Commit msg: say something
more instead of copying the subject. Or better squash the entire patch.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index bc89480820cb..e6c780e69d6e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
>  				drive-strength = <8>;
>  				bias-pull-up;
>  			};
> +
> +			mdio0_pins: mdio0-state {
> +				mux_0 {

This wasn't tested...

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).


Best regards,
Krzysztof
Andrew Lunn Jan. 10, 2024, 1:35 p.m. UTC | #2
On Wed, Jan 10, 2024 at 07:20:56PM +0800, Luo Jie wrote:
> Add the MDIO device tree of ipq5332.
> 
> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index bc89480820cb..e6c780e69d6e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
>  				drive-strength = <8>;
>  				bias-pull-up;
>  			};
> +
> +			mdio0_pins: mdio0-state {
> +				mux_0 {
> +					pins = "gpio25";
> +					function = "mdc0";
> +					drive-strength = <8>;
> +					bias-disable;
> +				};
> +
> +				mux_1 {
> +					pins = "gpio26";
> +					function = "mdio0";
> +					drive-strength = <8>;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			mdio1_pins: mdio1-state {
> +				mux_0 {
> +					pins = "gpio27";
> +					function = "mdc1";
> +					drive-strength = <8>;
> +					bias-disable;
> +				};
> +
> +				mux_1 {
> +					pins = "gpio28";
> +					function = "mdio1";
> +					drive-strength = <8>;
> +					bias-pull-up;
> +				};

I don't know why i'm asking this, because i don't really expect a
usable answer. What sort of MUX is this? Should you be using one of
the muxes in drivers/net/mdio/mdio-mux-* or something similar?

    Andrew
Jie Luo Jan. 11, 2024, 3:59 p.m. UTC | #3
On 1/10/2024 9:35 PM, Andrew Lunn wrote:
> On Wed, Jan 10, 2024 at 07:20:56PM +0800, Luo Jie wrote:
>> Add the MDIO device tree of ipq5332.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index bc89480820cb..e6c780e69d6e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
>>   				drive-strength = <8>;
>>   				bias-pull-up;
>>   			};
>> +
>> +			mdio0_pins: mdio0-state {
>> +				mux_0 {
>> +					pins = "gpio25";
>> +					function = "mdc0";
>> +					drive-strength = <8>;
>> +					bias-disable;
>> +				};
>> +
>> +				mux_1 {
>> +					pins = "gpio26";
>> +					function = "mdio0";
>> +					drive-strength = <8>;
>> +					bias-pull-up;
>> +				};
>> +			};
>> +
>> +			mdio1_pins: mdio1-state {
>> +				mux_0 {
>> +					pins = "gpio27";
>> +					function = "mdc1";
>> +					drive-strength = <8>;
>> +					bias-disable;
>> +				};
>> +
>> +				mux_1 {
>> +					pins = "gpio28";
>> +					function = "mdio1";
>> +					drive-strength = <8>;
>> +					bias-pull-up;
>> +				};
> 
> I don't know why i'm asking this, because i don't really expect a
> usable answer. What sort of MUX is this? Should you be using one of
> the muxes in drivers/net/mdio/mdio-mux-* or something similar?
> 
>      Andrew

Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
PINs are used by the dedicated hardware MDIO block in the SoC. I will 
update the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear. 
The driver for this node is drivers/net/mdio/mdio-ipq4019.c, it is not 
related to the mdio-mux-* code.
Dmitry Baryshkov Jan. 11, 2024, 4:13 p.m. UTC | #4
On Thu, 11 Jan 2024 at 18:00, Jie Luo <quic_luoj@quicinc.com> wrote:
>
>
>
> On 1/10/2024 9:35 PM, Andrew Lunn wrote:
> > On Wed, Jan 10, 2024 at 07:20:56PM +0800, Luo Jie wrote:
> >> Add the MDIO device tree of ipq5332.
> >>
> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
> >> ---
> >>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
> >>   1 file changed, 44 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >> index bc89480820cb..e6c780e69d6e 100644
> >> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
> >>                              drive-strength = <8>;
> >>                              bias-pull-up;
> >>                      };
> >> +
> >> +                    mdio0_pins: mdio0-state {
> >> +                            mux_0 {
> >> +                                    pins = "gpio25";
> >> +                                    function = "mdc0";
> >> +                                    drive-strength = <8>;
> >> +                                    bias-disable;
> >> +                            };
> >> +
> >> +                            mux_1 {
> >> +                                    pins = "gpio26";
> >> +                                    function = "mdio0";
> >> +                                    drive-strength = <8>;
> >> +                                    bias-pull-up;
> >> +                            };
> >> +                    };
> >> +
> >> +                    mdio1_pins: mdio1-state {
> >> +                            mux_0 {
> >> +                                    pins = "gpio27";
> >> +                                    function = "mdc1";
> >> +                                    drive-strength = <8>;
> >> +                                    bias-disable;
> >> +                            };
> >> +
> >> +                            mux_1 {
> >> +                                    pins = "gpio28";
> >> +                                    function = "mdio1";
> >> +                                    drive-strength = <8>;
> >> +                                    bias-pull-up;
> >> +                            };
> >
> > I don't know why i'm asking this, because i don't really expect a
> > usable answer. What sort of MUX is this? Should you be using one of
> > the muxes in drivers/net/mdio/mdio-mux-* or something similar?
> >
> >      Andrew
>
> Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
> PINs are used by the dedicated hardware MDIO block in the SoC. I will
> update the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear.
> The driver for this node is drivers/net/mdio/mdio-ipq4019.c, it is not
> related to the mdio-mux-* code.

Have you read Documentation/devicetree/bindings/pinctrl/qcom,ipq5332-tlmm.yaml
? Have you validated your DTSI files against DT schema? How many
warnings will you observe if you rename the mux_0 node to MDC?
Andrew Lunn Jan. 11, 2024, 4:30 p.m. UTC | #5
> Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
> PINs are used by the dedicated hardware MDIO block in the SoC. I will update
> the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear. The driver
> for this node is drivers/net/mdio/mdio-ipq4019.c, it is not related to the
> mdio-mux-* code.

So these is all about pinmux.

When you say:
> PINs are used by the dedicated hardware MDIO block in the SoC

do you actually mean:

PINs are used by the two dedicated hardware MDIO blocks in the SoC.

You have two sets of mdio/mdc configurations here, so i assume there
are two MDIO hardware blocks, each being an MDIO bus master.

    Andrew
Krzysztof Kozlowski Jan. 11, 2024, 4:43 p.m. UTC | #6
On 11/01/2024 16:59, Jie Luo wrote:
>>> +				mux_1 {
>>> +					pins = "gpio28";
>>> +					function = "mdio1";
>>> +					drive-strength = <8>;
>>> +					bias-pull-up;
>>> +				};
>>
>> I don't know why i'm asking this, because i don't really expect a
>> usable answer. What sort of MUX is this? Should you be using one of
>> the muxes in drivers/net/mdio/mdio-mux-* or something similar?
>>
>>      Andrew
> 
> Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
> PINs are used by the dedicated hardware MDIO block in the SoC. I will 
> update the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear. 
> The driver for this node is drivers/net/mdio/mdio-ipq4019.c, it is not 
> related to the mdio-mux-* code.

NAK. Run dtbs_check tests on your DTS first.

Best regards,
Krzysztof
Jie Luo Jan. 12, 2024, 3:58 p.m. UTC | #7
On 1/12/2024 12:13 AM, Dmitry Baryshkov wrote:
> On Thu, 11 Jan 2024 at 18:00, Jie Luo <quic_luoj@quicinc.com> wrote:
>>
>>
>>
>> On 1/10/2024 9:35 PM, Andrew Lunn wrote:
>>> On Wed, Jan 10, 2024 at 07:20:56PM +0800, Luo Jie wrote:
>>>> Add the MDIO device tree of ipq5332.
>>>>
>>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
>>>>    1 file changed, 44 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> index bc89480820cb..e6c780e69d6e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
>>>>                               drive-strength = <8>;
>>>>                               bias-pull-up;
>>>>                       };
>>>> +
>>>> +                    mdio0_pins: mdio0-state {
>>>> +                            mux_0 {
>>>> +                                    pins = "gpio25";
>>>> +                                    function = "mdc0";
>>>> +                                    drive-strength = <8>;
>>>> +                                    bias-disable;
>>>> +                            };
>>>> +
>>>> +                            mux_1 {
>>>> +                                    pins = "gpio26";
>>>> +                                    function = "mdio0";
>>>> +                                    drive-strength = <8>;
>>>> +                                    bias-pull-up;
>>>> +                            };
>>>> +                    };
>>>> +
>>>> +                    mdio1_pins: mdio1-state {
>>>> +                            mux_0 {
>>>> +                                    pins = "gpio27";
>>>> +                                    function = "mdc1";
>>>> +                                    drive-strength = <8>;
>>>> +                                    bias-disable;
>>>> +                            };
>>>> +
>>>> +                            mux_1 {
>>>> +                                    pins = "gpio28";
>>>> +                                    function = "mdio1";
>>>> +                                    drive-strength = <8>;
>>>> +                                    bias-pull-up;
>>>> +                            };
>>>
>>> I don't know why i'm asking this, because i don't really expect a
>>> usable answer. What sort of MUX is this? Should you be using one of
>>> the muxes in drivers/net/mdio/mdio-mux-* or something similar?
>>>
>>>       Andrew
>>
>> Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
>> PINs are used by the dedicated hardware MDIO block in the SoC. I will
>> update the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear.
>> The driver for this node is drivers/net/mdio/mdio-ipq4019.c, it is not
>> related to the mdio-mux-* code.
> 
> Have you read Documentation/devicetree/bindings/pinctrl/qcom,ipq5332-tlmm.yaml
> ? Have you validated your DTSI files against DT schema? How many
> warnings will you observe if you rename the mux_0 node to MDC?
> 
Sorry for this error, we will follow the DTSI validation process and 
update the patch with the right updates after validation, when the patch 
series resumes.

Thanks for correction and the pointer to the tlmm YAML file.
Jie Luo Jan. 12, 2024, 4:05 p.m. UTC | #8
On 1/12/2024 12:30 AM, Andrew Lunn wrote:
>> Sorry for the confusion, the pin nodes are for the MDIO and MDC, these
>> PINs are used by the dedicated hardware MDIO block in the SoC. I will update
>> the node name from mux_0 to MDC, mux_1 to MDIO, to make it clear. The driver
>> for this node is drivers/net/mdio/mdio-ipq4019.c, it is not related to the
>> mdio-mux-* code.
> 
> So these is all about pinmux.

Yes, it is about pinmux.

> 
> When you say:
>> PINs are used by the dedicated hardware MDIO block in the SoC
> 
> do you actually mean:
> 
> PINs are used by the two dedicated hardware MDIO blocks in the SoC.
> 
> You have two sets of mdio/mdc configurations here, so i assume there
> are two MDIO hardware blocks, each being an MDIO bus master.
> 
>      Andrew

There are two MDIO hardware blocks on IPQ5332 SoC. One is
for the MDIO bus master(gpio27, 28), which is for accessing the MDIO
slave devices(like PHY device). The mdio-ipq4019.c driver enables
this MDIO bus master.

Another one is the MDIO slave(gpio25, 26), which is dedicated
for receiving the back pressure signal from the connected Ethernet 
switch device QCA8386.

There is a MDIO master block integrated in QCA8386 switch device, this
integrated MDIO master is dedicated for generating the back
pressure signal to IPQ5332 SoC.

This MDIO slave block of IPQ5322 just needs to configure these PIN
mux for MDC and MDIO PINs. No additional driver is needed for this MDIO
slave block of IPQ5332.
Jie Luo Jan. 12, 2024, 4:11 p.m. UTC | #9
On 1/10/2024 7:56 PM, Krzysztof Kozlowski wrote:
> On 10/01/2024 12:20, Luo Jie wrote:
>> Add the MDIO device tree of ipq5332.
> 
> Subject: drop "device tree", it is obvious. Commit msg: say something
> more instead of copying the subject. Or better squash the entire patch.

Will update the commit message to improve the description when the 
series is updated.

>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 44 +++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index bc89480820cb..e6c780e69d6e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -214,6 +214,38 @@ serial_0_pins: serial0-state {
>>   				drive-strength = <8>;
>>   				bias-pull-up;
>>   			};
>> +
>> +			mdio0_pins: mdio0-state {
>> +				mux_0 {
> 
> This wasn't tested...
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> 
> 
> Best regards,
> Krzysztof
> 

We will follow up the guidance mentioned in the link to validate the DTS 
patches.
Andrew Lunn Jan. 16, 2024, 10:56 p.m. UTC | #10
> Another one is the MDIO slave(gpio25, 26), which is dedicated
> for receiving the back pressure signal from the connected Ethernet switch
> device QCA8386.
> 
> There is a MDIO master block integrated in QCA8386 switch device, this
> integrated MDIO master is dedicated for generating the back
> pressure signal to IPQ5332 SoC.
> 
> This MDIO slave block of IPQ5322 just needs to configure these PIN
> mux for MDC and MDIO PINs. No additional driver is needed for this MDIO
> slave block of IPQ5332.

So there is a proprietary protocol running over the MDIO bus? And its
completely implemented in hardware in the slave block? Is this even
MDIO? Does it use c22 or c45 bus transactions? How is the slave
address configured, or is that also hard coded?

	Andrew
Jie Luo Jan. 17, 2024, 3:10 p.m. UTC | #11
On 1/17/2024 6:56 AM, Andrew Lunn wrote:
>> Another one is the MDIO slave(gpio25, 26), which is dedicated
>> for receiving the back pressure signal from the connected Ethernet switch
>> device QCA8386.
>>
>> There is a MDIO master block integrated in QCA8386 switch device, this
>> integrated MDIO master is dedicated for generating the back
>> pressure signal to IPQ5332 SoC.
>>
>> This MDIO slave block of IPQ5322 just needs to configure these PIN
>> mux for MDC and MDIO PINs. No additional driver is needed for this MDIO
>> slave block of IPQ5332.
> 
> So there is a proprietary protocol running over the MDIO bus? And its
> completely implemented in hardware in the slave block? Is this even
> MDIO? Does it use c22 or c45 bus transactions? How is the slave
> address configured, or is that also hard coded?
> 
> 	Andrew
> 

Hi Andrew,
Yes, this is a custom HW mechanism using the MDIO C22 frame, to enable 
back pressure from the QCA8386 switch to the IPQ5332 SoC. The slave 
block in the IPQ5332 SoC implements the back pressure function. There is 
no configuration for the MDIO slave address of IPQ5332 required, since 
the connection is one to one between slave and master.

However upon further review, we believe this node definition belongs to
the board DTS file, since the switch configuration is a board property.
We will move out this MDIO slave config from the patch series to
avoid the confusion. We will also rename the node from 'mdio0-state' to 
'backpressure-state' to make this clear.

Thanks.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index bc89480820cb..e6c780e69d6e 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -214,6 +214,38 @@  serial_0_pins: serial0-state {
 				drive-strength = <8>;
 				bias-pull-up;
 			};
+
+			mdio0_pins: mdio0-state {
+				mux_0 {
+					pins = "gpio25";
+					function = "mdc0";
+					drive-strength = <8>;
+					bias-disable;
+				};
+
+				mux_1 {
+					pins = "gpio26";
+					function = "mdio0";
+					drive-strength = <8>;
+					bias-pull-up;
+				};
+			};
+
+			mdio1_pins: mdio1-state {
+				mux_0 {
+					pins = "gpio27";
+					function = "mdc1";
+					drive-strength = <8>;
+					bias-disable;
+				};
+
+				mux_1 {
+					pins = "gpio28";
+					function = "mdio1";
+					drive-strength = <8>;
+					bias-pull-up;
+				};
+			};
 		};
 
 		gcc: clock-controller@1800000 {
@@ -863,6 +895,18 @@  group0 {
 				};
 			};
 		};
+
+		mdio: mdio@90000 {
+			compatible = "qcom,ipq4019-mdio";
+			reg = <0x90000 0x64>;
+			pinctrl-0 = <&mdio1_pins &mdio0_pins>;
+			pinctrl-names = "default";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&gcc GCC_MDIO_AHB_CLK>;
+			clock-names = "gcc_mdio_ahb_clk";
+			status = "disabled";
+		};
 	};
 
 	timer {