Message ID | 20241118-dts_qcs615-v2-2-e62b924a3cbd@quicinc.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | Enable ethernet on qcs615 | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch |
On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote: > Enable the ethernet node, add the phy node and pinctrl for ethernet. > > Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> > --- > arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts > index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644 > --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts > +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts > @@ -5,6 +5,7 @@ > /dts-v1/; > > #include <dt-bindings/regulator/qcom,rpmh-regulator.h> > +#include <dt-bindings/gpio/gpio.h> > #include "qcs615.dtsi" > / { > model = "Qualcomm Technologies, Inc. QCS615 Ride"; > @@ -196,6 +197,60 @@ vreg_l17a: ldo17 { > }; > }; > > +ðernet { > + status = "okay"; > + > + pinctrl-0 = <ðernet_defaults>; > + pinctrl-names = "default"; > + > + phy-handle = <&rgmii_phy>; > + phy-mode = "rgmii"; That is unusual. Does the board have extra long clock lines? > + max-speed = <1000>; Why do you have this property? It is normally used to slow the MAC down because of issues at higher speeds. Andrew
On 2024-11-19 09:27, Andrew Lunn wrote: > On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote: >> Enable the ethernet node, add the phy node and pinctrl for ethernet. >> >> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++ >> 1 file changed, 106 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts >> index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644 >> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts >> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts >> @@ -5,6 +5,7 @@ >> /dts-v1/; >> >> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> >> +#include <dt-bindings/gpio/gpio.h> >> #include "qcs615.dtsi" >> / { >> model = "Qualcomm Technologies, Inc. QCS615 Ride"; >> @@ -196,6 +197,60 @@ vreg_l17a: ldo17 { >> }; >> }; >> >> +ðernet { >> + status = "okay"; >> + >> + pinctrl-0 = <ðernet_defaults>; >> + pinctrl-names = "default"; >> + >> + phy-handle = <&rgmii_phy>; >> + phy-mode = "rgmii"; > > That is unusual. Does the board have extra long clock lines? Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented. > >> + max-speed = <1000>; > > Why do you have this property? It is normally used to slow the MAC > down because of issues at higher speeds. According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps. > > Andrew
On 19.11.2024 11:09 AM, Yijie Yang wrote: > > > On 2024-11-19 09:27, Andrew Lunn wrote: >> On Mon, Nov 18, 2024 at 02:44:02PM +0800, Yijie Yang wrote: >>> Enable the ethernet node, add the phy node and pinctrl for ethernet. >>> >>> Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> >>> --- >>> arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++ >>> 1 file changed, 106 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts >>> index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644 >>> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts >>> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts >>> @@ -5,6 +5,7 @@ >>> /dts-v1/; >>> #include <dt-bindings/regulator/qcom,rpmh-regulator.h> >>> +#include <dt-bindings/gpio/gpio.h> >>> #include "qcs615.dtsi" >>> / { >>> model = "Qualcomm Technologies, Inc. QCS615 Ride"; >>> @@ -196,6 +197,60 @@ vreg_l17a: ldo17 { >>> }; >>> }; >>> +ðernet { >>> + status = "okay"; >>> + >>> + pinctrl-0 = <ðernet_defaults>; >>> + pinctrl-names = "default"; >>> + >>> + phy-handle = <&rgmii_phy>; >>> + phy-mode = "rgmii"; >> >> That is unusual. Does the board have extra long clock lines? > > Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented. Is that a board-specific issue, or something that concerns the SoC itself? > >> >>> + max-speed = <1000>; >> >> Why do you have this property? It is normally used to slow the MAC >> down because of issues at higher speeds. > > According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps. I believe the question Andrew is asking is "how is that effectively different from the default setting (omitting the property)?" Konrad
> >>> +ðernet { > >>> + status = "okay"; > >>> + > >>> + pinctrl-0 = <ðernet_defaults>; > >>> + pinctrl-names = "default"; > >>> + > >>> + phy-handle = <&rgmii_phy>; > >>> + phy-mode = "rgmii"; > >> > >> That is unusual. Does the board have extra long clock lines? > > > > Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented. > > Is that a board-specific issue, or something that concerns the SoC itself? Lots of developers gets this wrong.... Searching the mainline list for patchs getting it wrong and the explanation i have given in the past might help. The usual setting here is 'rgmmii-id', which means something needs to insert a 2ns delay on the clock lines. This is not always true, a very small number of boards use extra long clock likes on the PCB to add the needed 2ns delay. Now, if 'rgmii' does work, it means something else is broken somewhere. I will let you find out what. > >>> + max-speed = <1000>; > >> > >> Why do you have this property? It is normally used to slow the MAC > >> down because of issues at higher speeds. > > > > According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps. > > I believe the question Andrew is asking is "how is that effectively > different from the default setting (omitting the property)?" Correct. If there are no issues at higher speeds, you don't need this. phylib will ask the PHY what it is capable of, and limit the negotiated speeds to its capabilities. Occasionally you do see an RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper... Andrew
On 2024-11-22 21:19, Andrew Lunn wrote: >>>>> +ðernet { >>>>> + status = "okay"; >>>>> + >>>>> + pinctrl-0 = <ðernet_defaults>; >>>>> + pinctrl-names = "default"; >>>>> + >>>>> + phy-handle = <&rgmii_phy>; >>>>> + phy-mode = "rgmii"; >>>> >>>> That is unusual. Does the board have extra long clock lines? >>> >>> Do you mean to imply that using RGMII mode is unusual? While the EMAC controller supports various modes, due to hardware design limitations, only RGMII mode can be effectively implemented. >> >> Is that a board-specific issue, or something that concerns the SoC itself? > > Lots of developers gets this wrong.... Searching the mainline list for > patchs getting it wrong and the explanation i have given in the past > might help. > > The usual setting here is 'rgmmii-id', which means something needs to > insert a 2ns delay on the clock lines. This is not always true, a very > small number of boards use extra long clock likes on the PCB to add > the needed 2ns delay. > > Now, if 'rgmii' does work, it means something else is broken > somewhere. I will let you find out what. The 'rgmii' does function correctly, but it does not necessarily mean that a time delay is required at the board level. The EPHY can also compensate for the time skew. > >>>>> + max-speed = <1000>; >>>> >>>> Why do you have this property? It is normally used to slow the MAC >>>> down because of issues at higher speeds. >>> >>> According to the databoot, the EMAC in RGMII mode can support speeds of up to 1Gbps. >> >> I believe the question Andrew is asking is "how is that effectively >> different from the default setting (omitting the property)?" > > Correct. If there are no issues at higher speeds, you don't need > this. phylib will ask the PHY what it is capable of, and limit the > negotiated speeds to its capabilities. Occasionally you do see an > RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper... > > Andrew It does unnecessary, I will remove it.
On 2024-11-27 14:17, Yijie Yang wrote: > > > On 2024-11-22 21:19, Andrew Lunn wrote: >>>>>> +ðernet { >>>>>> + status = "okay"; >>>>>> + >>>>>> + pinctrl-0 = <ðernet_defaults>; >>>>>> + pinctrl-names = "default"; >>>>>> + >>>>>> + phy-handle = <&rgmii_phy>; >>>>>> + phy-mode = "rgmii"; >>>>> >>>>> That is unusual. Does the board have extra long clock lines? >>>> >>>> Do you mean to imply that using RGMII mode is unusual? While the >>>> EMAC controller supports various modes, due to hardware design >>>> limitations, only RGMII mode can be effectively implemented. >>> >>> Is that a board-specific issue, or something that concerns the SoC >>> itself? >> >> Lots of developers gets this wrong.... Searching the mainline list for >> patchs getting it wrong and the explanation i have given in the past >> might help. >> >> The usual setting here is 'rgmmii-id', which means something needs to >> insert a 2ns delay on the clock lines. This is not always true, a very >> small number of boards use extra long clock likes on the PCB to add >> the needed 2ns delay. >> >> Now, if 'rgmii' does work, it means something else is broken >> somewhere. I will let you find out what. > > The 'rgmii' does function correctly, but it does not necessarily mean > that a time delay is required at the board level. The EPHY can also > compensate for the time skew. I was mistaken earlier; it is actually the EMAC that will introduce a time skew by shifting the phase of the clock in 'rgmii' mode. > >> >>>>>> + max-speed = <1000>; >>>>> >>>>> Why do you have this property? It is normally used to slow the MAC >>>>> down because of issues at higher speeds. >>>> >>>> According to the databoot, the EMAC in RGMII mode can support speeds >>>> of up to 1Gbps. >>> >>> I believe the question Andrew is asking is "how is that effectively >>> different from the default setting (omitting the property)?" >> >> Correct. If there are no issues at higher speeds, you don't need >> this. phylib will ask the PHY what it is capable of, and limit the >> negotiated speeds to its capabilities. Occasionally you do see an >> RGMII PHY connected to a MII MAC, because a RGMII PHY is cheaper... >> >> Andrew > > It does unnecessary, I will remove it. >
> > The usual setting here is 'rgmmii-id', which means something needs to > > insert a 2ns delay on the clock lines. This is not always true, a very > > small number of boards use extra long clock likes on the PCB to add > > the needed 2ns delay. > > > > Now, if 'rgmii' does work, it means something else is broken > > somewhere. I will let you find out what. > > The 'rgmii' does function correctly, but it does not necessarily mean that a > time delay is required at the board level. The EPHY can also compensate for > the time skew. Basic definitions for phy-mode: rgmii: Indicates the board provides the delays, normally via extra long clock lines. rgmii-id: The board does not provide the delay, the software need to arrange that either the MAC or the PHY adds the delays. We then have the values passed between the MAC and the PHY driver: PHY_INTERFACE_MODE_RGMII: The PHY should not add delays PHY_INTERFACE_MODE_RGMII_ID: The PHY should add delays. A typical MAC/PHY combination, phy-mode is passed to the PHY, and the PHY adds the delays, if needed. This is why i said there are probably two bugs: 1) phy-mode rgmii should probably be rgmii-id 2) The PHY is adding delays when it should not be, because it is being passed PHY_INTERFACE_MODE_RGMII not PHY_INTERFACE_MODE_RGMII_ID. Andrew
> I was mistaken earlier; it is actually the EMAC that will introduce a time > skew by shifting the phase of the clock in 'rgmii' mode. This is fine, but not the normal way we do this. The Linux preference is that the PHY adds the delays. There are a few exceptions, boards which have PHYs which cannot add delays. In that case the MAC adds the delays. But this is pretty unusual. If you decided you want to be unusual and have the MAC add the delays, it should not be hard coded. You need to look at phy-mode. Only add delays for rgmii-id. And you then need to mask the value passed to the PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID, so the PHY does not add delays as well. Andrew
On 2024-11-29 23:29, Andrew Lunn wrote: >> I was mistaken earlier; it is actually the EMAC that will introduce a time >> skew by shifting the phase of the clock in 'rgmii' mode. > > This is fine, but not the normal way we do this. The Linux preference > is that the PHY adds the delays. There are a few exceptions, boards > which have PHYs which cannot add delays. In that case the MAC adds the > delays. But this is pretty unusual. After testing, it has been observed that modes other than 'rgmii' do not function properly due to the current configuration sequence in the driver code. > > If you decided you want to be unusual and have the MAC add the delays, > it should not be hard coded. You need to look at phy-mode. Only add Are you suggesting that 'rgmii' indicates the delay is introduced by the board rather than the EMAC? But according to the Documentation/devicetree/bindings/net/ethernet-controller.yaml, this mode explicitly states that 'RX and TX delays are added by the MAC when required'. That is indeed my preference. > delays for rgmii-id. And you then need to mask the value passed to the > PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID, > so the PHY does not add delays as well. > > Andrew
On Mon, Dec 09, 2024 at 10:11:23AM +0800, Yijie Yang wrote: > > > On 2024-11-29 23:29, Andrew Lunn wrote: > > > I was mistaken earlier; it is actually the EMAC that will introduce a time > > > skew by shifting the phase of the clock in 'rgmii' mode. > > > > This is fine, but not the normal way we do this. The Linux preference > > is that the PHY adds the delays. There are a few exceptions, boards > > which have PHYs which cannot add delays. In that case the MAC adds the > > delays. But this is pretty unusual. > > After testing, it has been observed that modes other than 'rgmii' do not > function properly due to the current configuration sequence in the driver > code. O.K, so now you need to find out why. It not working probably suggests you are adding double delays, both in the MAC and the PHY. Where the PHY driver add delays is generally easy to see in the code. Just search for PHY_INTERFACE_MODE_RGMII_ID. For the MAC driver you probably need to read the datasheet and find registers which control the delay. > > If you decided you want to be unusual and have the MAC add the delays, > > it should not be hard coded. You need to look at phy-mode. Only add > > Are you suggesting that 'rgmii' indicates the delay is introduced by the > board rather than the EMAC? Yes. > But according to the > Documentation/devicetree/bindings/net/ethernet-controller.yaml, this mode > explicitly states that 'RX and TX delays are added by the MAC when > required'. That is indeed my preference. You need to be careful with context. If the board is not adding delays, and you pass PHY_INTERFACE_MODE_RGMII to the PHY, the MAC must be adding the delays, otherwise there will not be any delays, and it will not work. > > delays for rgmii-id. And you then need to mask the value passed to the > > PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID, > > so the PHY does not add delays as well. Andrew
On 2024-12-09 11:13, Andrew Lunn wrote: > On Mon, Dec 09, 2024 at 10:11:23AM +0800, Yijie Yang wrote: >> >> >> On 2024-11-29 23:29, Andrew Lunn wrote: >>>> I was mistaken earlier; it is actually the EMAC that will introduce a time >>>> skew by shifting the phase of the clock in 'rgmii' mode. >>> >>> This is fine, but not the normal way we do this. The Linux preference >>> is that the PHY adds the delays. There are a few exceptions, boards >>> which have PHYs which cannot add delays. In that case the MAC adds the >>> delays. But this is pretty unusual. >> >> After testing, it has been observed that modes other than 'rgmii' do not >> function properly due to the current configuration sequence in the driver >> code. > > O.K, so now you need to find out why. > > It not working probably suggests you are adding double delays, both in > the MAC and the PHY. Where the PHY driver add delays is generally easy > to see in the code. Just search for PHY_INTERFACE_MODE_RGMII_ID. For > the MAC driver you probably need to read the datasheet and find > registers which control the delay. As previously mentioned, using 'rgmii' will enable EMAC to provide the delay while disabling the delay for EPHY. So there's won't be double delay. Additionally, the current implementation of the QCOM driver code exclusively supports this mode, with the entire initialization sequence of EMAC designed and fixed for this specific mode. Therefore, no other options are available until changes are made to the driver. > >>> If you decided you want to be unusual and have the MAC add the delays, >>> it should not be hard coded. You need to look at phy-mode. Only add >> >> Are you suggesting that 'rgmii' indicates the delay is introduced by the >> board rather than the EMAC? > > Yes. > >> But according to the >> Documentation/devicetree/bindings/net/ethernet-controller.yaml, this mode >> explicitly states that 'RX and TX delays are added by the MAC when >> required'. That is indeed my preference. > > You need to be careful with context. If the board is not adding > delays, and you pass PHY_INTERFACE_MODE_RGMII to the PHY, the MAC must > be adding the delays, otherwise there will not be any delays, and it > will not work. > I'm not sure if there's a disagreement about the definition or a misunderstanding with other vendors. From my understanding, 'rgmii' should not imply that the delay must be provided by the board, based on both the definition in the dt-binding file and the implementations by other EMAC vendors. Most EMAC drivers provide the delay in this mode. I confirmed that there is no delay on the qcs615-ride board., and the QCOM EMAC driver will adds the delay by shifting the clock after receiving PHY_INTERFACE_MODE_RGMII. >>> delays for rgmii-id. And you then need to mask the value passed to the >>> PHY, pass PHY_INTERFACE_MODE_RGMII, not PHY_INTERFACE_MODE_RGMII_ID, >>> so the PHY does not add delays as well. > > Andrew
> As previously mentioned, using 'rgmii' will enable EMAC to provide the delay > while disabling the delay for EPHY. So there's won't be double delay. > > Additionally, the current implementation of the QCOM driver code exclusively > supports this mode, with the entire initialization sequence of EMAC designed > and fixed for this specific mode. OK. If it is impossible to disable these delays, you need to validate phy-mode. Only rgmii-id is allowed. Anybody trying to build a board using extra long clock lines is out of luck. It does not happen very often, but there are a small number of boards which do this, and the definitions of phy-mode are designed to support them. > I'm not sure if there's a disagreement about the definition or a > misunderstanding with other vendors. From my understanding, 'rgmii' should > not imply that the delay must be provided by the board, based on both the > definition in the dt-binding file and the implementations by other EMAC > vendors. Most EMAC drivers provide the delay in this mode. Nope. You are wrong. I've been enforcing this meaning for maybe the last 10 years. You can go search the email archive for netdev. Before that, we had a bit of a mess, developers were getting it wrong, and reviewing was not as good. And i don't review everything, so some bad code does get passed me every so often, e.g. if found out today that TI AM62 got this wrong, they hard code TX delays in the MAC, and DT developers have been using rgmii-rxid, not rgmii-id, and the MAC driver is missing the mask operation before calling phy_connect. > I confirmed that there is no delay on the qcs615-ride board., and the QCOM > EMAC driver will adds the delay by shifting the clock after receiving > PHY_INTERFACE_MODE_RGMII. Which is wrong. Because you cannot disable the delay, PHY_INTERFACE_MODE_RGMII should return in EINVAL, or maybe EOPNOTSUPP. Your hardware only supports PHY_INTERFACE_MODE_RGMII_ID, and you need to mask what you pass to phylib/phylink to make it clear the MAC has added the delays. Andrew
On 2024-12-10 12:09, Andrew Lunn wrote: >> As previously mentioned, using 'rgmii' will enable EMAC to provide the delay >> while disabling the delay for EPHY. So there's won't be double delay. >> >> Additionally, the current implementation of the QCOM driver code exclusively >> supports this mode, with the entire initialization sequence of EMAC designed >> and fixed for this specific mode. > > OK. If it is impossible to disable these delays, you need to validate > phy-mode. Only rgmii-id is allowed. Anybody trying to build a board > using extra long clock lines is out of luck. It does not happen very > often, but there are a small number of boards which do this, and the > definitions of phy-mode are designed to support them. > >> I'm not sure if there's a disagreement about the definition or a >> misunderstanding with other vendors. From my understanding, 'rgmii' should >> not imply that the delay must be provided by the board, based on both the >> definition in the dt-binding file and the implementations by other EMAC >> vendors. Most EMAC drivers provide the delay in this mode. > > Nope. You are wrong. I've been enforcing this meaning for maybe the > last 10 years. You can go search the email archive for netdev. Before > that, we had a bit of a mess, developers were getting it wrong, and > reviewing was not as good. And i don't review everything, so some bad > code does get passed me every so often, e.g. if found out today that > TI AM62 got this wrong, they hard code TX delays in the MAC, and DT > developers have been using rgmii-rxid, not rgmii-id, and the MAC > driver is missing the mask operation before calling phy_connect. > >> I confirmed that there is no delay on the qcs615-ride board., and the QCOM >> EMAC driver will adds the delay by shifting the clock after receiving >> PHY_INTERFACE_MODE_RGMII. > > Which is wrong. Because you cannot disable the delay, > PHY_INTERFACE_MODE_RGMII should return in EINVAL, or maybe > EOPNOTSUPP. Your hardware only supports PHY_INTERFACE_MODE_RGMII_ID, > and you need to mask what you pass to phylib/phylink to make it clear > the MAC has added the delays. > > Andrew I intend to follow these steps. Could you please check if they are correct? 1. Add a new flag in DTS to inform the MAC driver to include the delay when configured with 'rgmii-id'. Without this flag, the MAC driver will not be aware of the need for the delay. 2. In the driver, if this flag is set to true, change the phy_mode to PHY_INTERFACE_MODE_RGMII to instruct the PHY not to add the delay.
> I intend to follow these steps. Could you please check if they are correct? > 1. Add a new flag in DTS to inform the MAC driver to include the delay when > configured with 'rgmii-id'. Without this flag, the MAC driver will not be > aware of the need for the delay. Why do you need this flag? If the phy-mode is rgmii-id, either the MAC or the PHY needs to add the delay. The MAC driver gets to see phy-mode first. If it wants to add the delay, it can, but it needs to mask out the delays before passing phy-mode to the PHY. If the MAC driver does not want to add the delays, pass phy-mode as is the PHY, and it will add the delays. There is nothing special here, every MAC/PHY pair does this, without needing additional properties. Andrew
On 2024-12-16 17:18, Andrew Lunn wrote: >> I intend to follow these steps. Could you please check if they are correct? >> 1. Add a new flag in DTS to inform the MAC driver to include the delay when >> configured with 'rgmii-id'. Without this flag, the MAC driver will not be >> aware of the need for the delay. > > Why do you need this flag? > > If the phy-mode is rgmii-id, either the MAC or the PHY needs to add > the delay. > > The MAC driver gets to see phy-mode first. If it wants to add the > delay, it can, but it needs to mask out the delays before passing > phy-mode to the PHY. If the MAC driver does not want to add the > delays, pass phy-mode as is the PHY, and it will add the delays. In this scenario, the delay in 'rgmii-id' mode is currently introduced by the MAC as it is fixed in the driver code. How can we enable the PHY to add the delay in this mode in the future (If we intend to revert to the most common approach of the Linux kernel)? After all, the MAC driver is unsure when to add the delay. > > There is nothing special here, every MAC/PHY pair does this, without > needing additional properties. > > Andrew
On Tue, Dec 17, 2024 at 10:26:15AM +0800, Yijie Yang wrote: > > > On 2024-12-16 17:18, Andrew Lunn wrote: > > > I intend to follow these steps. Could you please check if they are correct? > > > 1. Add a new flag in DTS to inform the MAC driver to include the delay when > > > configured with 'rgmii-id'. Without this flag, the MAC driver will not be > > > aware of the need for the delay. > > > > Why do you need this flag? > > > > If the phy-mode is rgmii-id, either the MAC or the PHY needs to add > > the delay. > > > > The MAC driver gets to see phy-mode first. If it wants to add the > > delay, it can, but it needs to mask out the delays before passing > > phy-mode to the PHY. If the MAC driver does not want to add the > > delays, pass phy-mode as is the PHY, and it will add the delays. > > In this scenario, the delay in 'rgmii-id' mode is currently introduced by > the MAC as it is fixed in the driver code. How can we enable the PHY to add > the delay in this mode in the future (If we intend to revert to the most > common approach of the Linux kernel)? After all, the MAC driver is unsure > when to add the delay. You just take out the code in the MAC driver which adds the delay and masks the phy-mode. 2ns should be 2ns delay, independent of who inserts it. The only danger is, there might be some board uses a PHY which is incapable of adding the 2ns delay, and such a change breaks that board. But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is special pricing if you use the combination, so there is probably little incentive to use somebody elses PHY. And i assume you can quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't support RGMII delays are very rare, it just happened that one vendors RDK happened to use one, so they ended up with delays in the MAC being standard for their boards. Andrew
On 2024-12-17 18:18, Andrew Lunn wrote: > On Tue, Dec 17, 2024 at 10:26:15AM +0800, Yijie Yang wrote: >> >> >> On 2024-12-16 17:18, Andrew Lunn wrote: >>>> I intend to follow these steps. Could you please check if they are correct? >>>> 1. Add a new flag in DTS to inform the MAC driver to include the delay when >>>> configured with 'rgmii-id'. Without this flag, the MAC driver will not be >>>> aware of the need for the delay. >>> >>> Why do you need this flag? >>> >>> If the phy-mode is rgmii-id, either the MAC or the PHY needs to add >>> the delay. >>> >>> The MAC driver gets to see phy-mode first. If it wants to add the >>> delay, it can, but it needs to mask out the delays before passing >>> phy-mode to the PHY. If the MAC driver does not want to add the >>> delays, pass phy-mode as is the PHY, and it will add the delays. >> >> In this scenario, the delay in 'rgmii-id' mode is currently introduced by >> the MAC as it is fixed in the driver code. How can we enable the PHY to add >> the delay in this mode in the future (If we intend to revert to the most >> common approach of the Linux kernel)? After all, the MAC driver is unsure >> when to add the delay. > > You just take out the code in the MAC driver which adds the delay and > masks the phy-mode. 2ns should be 2ns delay, independent of who > inserts it. The only danger is, there might be some board uses a PHY > which is incapable of adding the 2ns delay, and such a change breaks > that board. Okay, I will follow your instructions: 1. Change the phy-mode to 'rgmii-id'. 2. Add the delay in the MAC driver. 3. Mask the phy-mode before passing it to the PHY. > > But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is > special pricing if you use the combination, so there is probably > little incentive to use somebody elses PHY. And i assume you can > quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't > support RGMII delays are very rare, it just happened that one vendors > RDK happened to use one, so they ended up with delays in the MAC being > standard for their boards. > Most Qualcomm MAC applications are actually paired with a third-party PHY. I agree that the original author's PHY might not support adding the delay. However, this assumption cannot be verified since the initial code was uploaded from the internal code base. > Andrew >
> Okay, I will follow your instructions: > 1. Change the phy-mode to 'rgmii-id'. > 2. Add the delay in the MAC driver. > 3. Mask the phy-mode before passing it to the PHY. Good. > > > > > But i assume Qualcomm RDKs always make use of a Qualcomm PHY, there is > > special pricing if you use the combination, so there is probably > > little incentive to use somebody elses PHY. And i assume you can > > quickly check all Qualcomm PHYs support RGMII delays. PHYs which don't > > support RGMII delays are very rare, it just happened that one vendors > > RDK happened to use one, so they ended up with delays in the MAC being > > standard for their boards. > > > > Most Qualcomm MAC applications are actually paired with a third-party PHY. I'm not sure the QCA8K, IPC and old Atheros people would agree with you. But since you don't have any behaviour change, you are not asking the PHY to insert the delays, you should be safe in your part of the Qualcomm world. Andrew
diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts index ee6cab3924a6d71f29934a8debba3a832882abdd..299be3aa17a0633d808f4b5d32aed946f07d5dfd 100644 --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts @@ -5,6 +5,7 @@ /dts-v1/; #include <dt-bindings/regulator/qcom,rpmh-regulator.h> +#include <dt-bindings/gpio/gpio.h> #include "qcs615.dtsi" / { model = "Qualcomm Technologies, Inc. QCS615 Ride"; @@ -196,6 +197,60 @@ vreg_l17a: ldo17 { }; }; +ðernet { + status = "okay"; + + pinctrl-0 = <ðernet_defaults>; + pinctrl-names = "default"; + + phy-handle = <&rgmii_phy>; + phy-mode = "rgmii"; + max-speed = <1000>; + + snps,mtl-rx-config = <&mtl_rx_setup>; + snps,mtl-tx-config = <&mtl_tx_setup>; + + mdio: mdio { + compatible = "snps,dwmac-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + rgmii_phy: phy@7 { + compatible = "ethernet-phy-ieee802.3-c22"; + reg = <0x7>; + + interrupts-extended = <&tlmm 121 IRQ_TYPE_EDGE_FALLING>; + device_type = "ethernet-phy"; + reset-gpios = <&tlmm 104 GPIO_ACTIVE_LOW>; + reset-assert-us = <11000>; + reset-deassert-us = <70000>; + }; + }; + + mtl_rx_setup: rx-queues-config { + snps,rx-queues-to-use = <1>; + snps,rx-sched-sp; + + queue0 { + snps,dcb-algorithm; + snps,map-to-dma-channel = <0x0>; + snps,route-up; + snps,priority = <0x1>; + }; + }; + + mtl_tx_setup: tx-queues-config { + snps,tx-queues-to-use = <1>; + snps,tx-sched-wrr; + + queue0 { + snps,weight = <0x10>; + snps,dcb-algorithm; + snps,priority = <0x0>; + }; + }; +}; + &gcc { clocks = <&rpmhcc RPMH_CXO_CLK>, <&rpmhcc RPMH_CXO_CLK_A>, @@ -210,6 +265,57 @@ &rpmhcc { clocks = <&xo_board_clk>; }; +&tlmm { + ethernet_defaults: ethernet-defaults-state { + mdc-pins { + pins = "gpio113"; + function = "rgmii"; + bias-pull-up; + }; + + mdio-pins { + pins = "gpio114"; + function = "rgmii"; + bias-pull-up; + }; + + rgmii-rx-pins { + pins = "gpio81", "gpio82", "gpio83", "gpio102", "gpio103", "gpio112"; + function = "rgmii"; + bias-disable; + drive-strength = <2>; + }; + + rgmii-tx-pins { + pins = "gpio92", "gpio93", "gpio94", "gpio95", "gpio96", "gpio97"; + function = "rgmii"; + bias-pull-up; + drive-strength = <16>; + }; + + phy-intr-pins { + pins = "gpio121"; + function = "gpio"; + bias-disable; + drive-strength = <8>; + }; + + pps-pins { + pins = "gpio91"; + function = "rgmii"; + bias-disable; + drive-strength = <8>; + }; + + phy-reset-pins { + pins = "gpio104"; + function = "gpio"; + bias-pull-up; + drive-strength = <16>; + }; + }; +}; + &uart0 { status = "okay"; };
Enable the ethernet node, add the phy node and pinctrl for ethernet. Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com> --- arch/arm64/boot/dts/qcom/qcs615-ride.dts | 106 +++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+)