| 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 |
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
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
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.
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 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
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
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.
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.
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.
> 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
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 --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 {
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(+)