The sampling timing of the MAC varies per board due to differences in
hardware layout or peer PHY, even within the same version. For instance,
the EMAC version on the qcs615-ride board is below 3, but it requires
swapping the RX timing to enable 10M/100M speeds. Therefore, this setting
should be adjustable rather than solely determined by the version.
The RGMII_CONFIG2_RX_PROG_SWAP bit is used to switch the RX sampling
timing between the rising edge and the falling edge of the clock.
Consequently, the condition for setting this bit should be revised to
ensure correct data sampling.
The compatible string matching for 'qcom,sa8540p-ride' in this change is
intended to ensure ABI compatibility for DTS files that do not include the
new 'qcom,rx-prog-swap' property, allowing legacy boards to function as
before. This board is the only one among legacy boards using RGMII and
needs to enable RX swap.
Signed-off-by: Yijie Yang <quic_yijiyang@quicinc.com>
---
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 36 ++++++++++++----------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..cf9633489975bc89480d065ae723a92723a12b04 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -120,6 +120,7 @@ struct qcom_ethqos {
bool rgmii_config_loopback_en;
bool has_emac_ge_3;
bool needs_sgmii_loopback;
+ bool needs_rx_prog_swap;
};
static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
@@ -401,6 +402,7 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
{
struct device *dev = ðqos->pdev->dev;
+ int rx_prog_swap = 0;
int phase_shift;
int loopback;
@@ -421,6 +423,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
else
loopback = 0;
+ if (ethqos->needs_rx_prog_swap)
+ rx_prog_swap = RGMII_CONFIG2_RX_PROG_SWAP;
+
/* Select RGMII, write 0 to interface select */
rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
0, RGMII_IO_MACRO_CONFIG);
@@ -484,14 +489,8 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
BIT(6), RGMII_IO_MACRO_CONFIG);
rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
0, RGMII_IO_MACRO_CONFIG2);
-
- if (ethqos->has_emac_ge_3)
- rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
- RGMII_CONFIG2_RX_PROG_SWAP,
- RGMII_IO_MACRO_CONFIG2);
- else
- rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
- 0, RGMII_IO_MACRO_CONFIG2);
+ rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap,
+ RGMII_IO_MACRO_CONFIG2);
/* Write 0x5 to PRG_RCLK_DLY_CODE */
rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
@@ -525,13 +524,9 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
RGMII_IO_MACRO_CONFIG);
rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
0, RGMII_IO_MACRO_CONFIG2);
- if (ethqos->has_emac_ge_3)
- rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
- RGMII_CONFIG2_RX_PROG_SWAP,
- RGMII_IO_MACRO_CONFIG2);
- else
- rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
- 0, RGMII_IO_MACRO_CONFIG2);
+ rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, rx_prog_swap,
+ RGMII_IO_MACRO_CONFIG2);
+
/* Write 0x5 to PRG_RCLK_DLY_CODE */
rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
(BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
@@ -782,7 +777,7 @@ static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
static int qcom_ethqos_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
+ struct device_node *np = pdev->dev.of_node, *root;
const struct ethqos_emac_driver_data *data;
struct plat_stmmacenet_data *plat_dat;
struct stmmac_resources stmmac_res;
@@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
ret = of_get_phy_mode(np, ðqos->phy_mode);
if (ret)
return dev_err_probe(dev, ret, "Failed to get phy mode\n");
+
+ root = of_find_node_by_path("/");
+ if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
+ ethqos->needs_rx_prog_swap = true;
+ else
+ ethqos->needs_rx_prog_swap =
+ of_property_read_bool(np, "qcom,rx-prog-swap");
+ of_node_put(root);
+
switch (ethqos->phy_mode) {
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
--
2.34.1On 25/12/2024 11:04, Yijie Yang wrote:
> static int qcom_ethqos_probe(struct platform_device *pdev)
> {
> - struct device_node *np = pdev->dev.of_node;
> + struct device_node *np = pdev->dev.of_node, *root;
> const struct ethqos_emac_driver_data *data;
> struct plat_stmmacenet_data *plat_dat;
> struct stmmac_resources stmmac_res;
> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> ret = of_get_phy_mode(np, ðqos->phy_mode);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
> +
> + root = of_find_node_by_path("/");
> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
Nope, your drivers are not supposed to poke root compatibles. Drop and
fix your driver to behave correctly for all existing devices.
> + ethqos->needs_rx_prog_swap = true;
> + else
> + ethqos->needs_rx_prog_swap =
Best regards,
Krzysztof
On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
> On 25/12/2024 11:04, Yijie Yang wrote:
>
>> static int qcom_ethqos_probe(struct platform_device *pdev)
>> {
>> - struct device_node *np = pdev->dev.of_node;
>> + struct device_node *np = pdev->dev.of_node, *root;
>> const struct ethqos_emac_driver_data *data;
>> struct plat_stmmacenet_data *plat_dat;
>> struct stmmac_resources stmmac_res;
>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>> if (ret)
>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>> +
>> + root = of_find_node_by_path("/");
>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>
>
> Nope, your drivers are not supposed to poke root compatibles. Drop and
> fix your driver to behave correctly for all existing devices.
>
Since this change introduces a new flag in the DTS, we must maintain ABI
compatibility with the kernel. The new flag is specific to the board, so
I need to ensure root nodes are matched to allow older boards to
continue functioning as before. I'm happy to adopt that approach if
there are any more elegant solutions.
>> + ethqos->needs_rx_prog_swap = true;
>> + else
>> + ethqos->needs_rx_prog_swap =
> Best regards,
> Krzysztof
--
Best Regards,
Yijie
On 26/12/2024 03:29, Yijie Yang wrote:
>
>
> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>> On 25/12/2024 11:04, Yijie Yang wrote:
>>
>>> static int qcom_ethqos_probe(struct platform_device *pdev)
>>> {
>>> - struct device_node *np = pdev->dev.of_node;
>>> + struct device_node *np = pdev->dev.of_node, *root;
>>> const struct ethqos_emac_driver_data *data;
>>> struct plat_stmmacenet_data *plat_dat;
>>> struct stmmac_resources stmmac_res;
>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>>> if (ret)
>>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>> +
>>> + root = of_find_node_by_path("/");
>>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>
>>
>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>> fix your driver to behave correctly for all existing devices.
>>
>
> Since this change introduces a new flag in the DTS, we must maintain ABI
> compatibility with the kernel. The new flag is specific to the board, so
It's not, I don't see it specific to the board in the bindings.
> I need to ensure root nodes are matched to allow older boards to
> continue functioning as before. I'm happy to adopt that approach if
> there are any more elegant solutions.
I don't think you understood the problem. Why you are not handling this
for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
Best regards,
Krzysztof
On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
> On 26/12/2024 03:29, Yijie Yang wrote:
>>
>>
>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>
>>>> static int qcom_ethqos_probe(struct platform_device *pdev)
>>>> {
>>>> - struct device_node *np = pdev->dev.of_node;
>>>> + struct device_node *np = pdev->dev.of_node, *root;
>>>> const struct ethqos_emac_driver_data *data;
>>>> struct plat_stmmacenet_data *plat_dat;
>>>> struct stmmac_resources stmmac_res;
>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>>>> if (ret)
>>>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>> +
>>>> + root = of_find_node_by_path("/");
>>>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>
>>>
>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>> fix your driver to behave correctly for all existing devices.
>>>
>>
>> Since this change introduces a new flag in the DTS, we must maintain ABI
>> compatibility with the kernel. The new flag is specific to the board, so
>
> It's not, I don't see it specific to the board in the bindings.
I'm sorry for the confusion. This feature is not board-specific but
rather a tunable option. All RGMII boards can choose whether to enable
this bit in the DTS, so there are no restrictions in the binding.
>
>> I need to ensure root nodes are matched to allow older boards to
>> continue functioning as before. I'm happy to adopt that approach if
>> there are any more elegant solutions.
>
> I don't think you understood the problem. Why you are not handling this
> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
>
This feature is specifically for RGMII boards. The driver won't enable
this bit if the DTS doesn't specify it. To handle compatibility, we need
to identify legacy RGMII boards with MAC versions greater or equal to 3
which require this bit to be enabled.
According to my knowledge, the SA8775P is of the SGMII type.
>
> Best regards,
> Krzysztof
--
Best Regards,
Yijie
On 08/01/2025 11:33, Yijie Yang wrote:
>
>
> On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
>> On 26/12/2024 03:29, Yijie Yang wrote:
>>>
>>>
>>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>>
>>>>> static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>> {
>>>>> - struct device_node *np = pdev->dev.of_node;
>>>>> + struct device_node *np = pdev->dev.of_node, *root;
>>>>> const struct ethqos_emac_driver_data *data;
>>>>> struct plat_stmmacenet_data *plat_dat;
>>>>> struct stmmac_resources stmmac_res;
>>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>>>>> if (ret)
>>>>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>> +
>>>>> + root = of_find_node_by_path("/");
>>>>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>>
>>>>
>>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>>> fix your driver to behave correctly for all existing devices.
>>>>
>>>
>>> Since this change introduces a new flag in the DTS, we must maintain ABI
>>> compatibility with the kernel. The new flag is specific to the board, so
>>
>> It's not, I don't see it specific to the board in the bindings.
>
> I'm sorry for the confusion. This feature is not board-specific but
> rather a tunable option. All RGMII boards can choose whether to enable
> this bit in the DTS, so there are no restrictions in the binding.
If it is not specific to the board, I don't see why this cannot be
implied by compatible.
>
>>
>>> I need to ensure root nodes are matched to allow older boards to
>>> continue functioning as before. I'm happy to adopt that approach if
>>> there are any more elegant solutions.
>>
>> I don't think you understood the problem. Why you are not handling this
>> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
>>
>
> This feature is specifically for RGMII boards. The driver won't enable
So board specific?
> this bit if the DTS doesn't specify it. To handle compatibility, we need
Do not describe us how drivers and DTS work. We all know.
> to identify legacy RGMII boards with MAC versions greater or equal to 3
> which require this bit to be enabled.
> According to my knowledge, the SA8775P is of the SGMII type.
Best regards,
Krzysztof
On 2025-01-13 19:26, Krzysztof Kozlowski wrote:
> On 08/01/2025 11:33, Yijie Yang wrote:
>>
>>
>> On 2024-12-27 15:03, Krzysztof Kozlowski wrote:
>>> On 26/12/2024 03:29, Yijie Yang wrote:
>>>>
>>>>
>>>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>>>
>>>>>> static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> - struct device_node *np = pdev->dev.of_node;
>>>>>> + struct device_node *np = pdev->dev.of_node, *root;
>>>>>> const struct ethqos_emac_driver_data *data;
>>>>>> struct plat_stmmacenet_data *plat_dat;
>>>>>> struct stmmac_resources stmmac_res;
>>>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>>>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>>>>>> if (ret)
>>>>>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>>>> +
>>>>>> + root = of_find_node_by_path("/");
>>>>>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>>>
>>>>>
>>>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>>>> fix your driver to behave correctly for all existing devices.
>>>>>
>>>>
>>>> Since this change introduces a new flag in the DTS, we must maintain ABI
>>>> compatibility with the kernel. The new flag is specific to the board, so
>>>
>>> It's not, I don't see it specific to the board in the bindings.
>>
>> I'm sorry for the confusion. This feature is not board-specific but
>> rather a tunable option. All RGMII boards can choose whether to enable
>> this bit in the DTS, so there are no restrictions in the binding.
>
> If it is not specific to the board, I don't see why this cannot be
> implied by compatible.
>
Whether this bit should be enabled should be determined on a per-board
basis, but it should be available for all RGMII-type boards. It should
be left to the users to decide whether to enable this bit in the DTS
file, rather than controlling its existence in the binding file,
shouldn't it?
>>
>>>
>>>> I need to ensure root nodes are matched to allow older boards to
>>>> continue functioning as before. I'm happy to adopt that approach if
>>>> there are any more elegant solutions.
>>>
>>> I don't think you understood the problem. Why you are not handling this
>>> for my board, sa8775p-rideX and sa8225-pre-ride-yellow-shrimp?
>>>
>>
>> This feature is specifically for RGMII boards. The driver won't enable
>
> So board specific?
It is 'phy-mode' specific, to be more precise.
>
>> this bit if the DTS doesn't specify it. To handle compatibility, we need
>
> Do not describe us how drivers and DTS work. We all know.
Sure, I will take care of it.
>
>> to identify legacy RGMII boards with MAC versions greater or equal to 3
>> which require this bit to be enabled.
>> According to my knowledge, the SA8775P is of the SGMII type.
>
>
> Best regards,
> Krzysztof
--
Best Regards,
Yijie
On Thu, Dec 26, 2024 at 10:29:45AM +0800, Yijie Yang wrote:
>
>
> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
> > On 25/12/2024 11:04, Yijie Yang wrote:
> >
> > > static int qcom_ethqos_probe(struct platform_device *pdev)
> > > {
> > > - struct device_node *np = pdev->dev.of_node;
> > > + struct device_node *np = pdev->dev.of_node, *root;
> > > const struct ethqos_emac_driver_data *data;
> > > struct plat_stmmacenet_data *plat_dat;
> > > struct stmmac_resources stmmac_res;
> > > @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> > > ret = of_get_phy_mode(np, ðqos->phy_mode);
> > > if (ret)
> > > return dev_err_probe(dev, ret, "Failed to get phy mode\n");
> > > +
> > > + root = of_find_node_by_path("/");
> > > + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
> >
> >
> > Nope, your drivers are not supposed to poke root compatibles. Drop and
> > fix your driver to behave correctly for all existing devices.
> >
>
> Since this change introduces a new flag in the DTS, we must maintain ABI
> compatibility with the kernel. The new flag is specific to the board, so I
> need to ensure root nodes are matched to allow older boards to continue
> functioning as before. I'm happy to adopt that approach if there are any
> more elegant solutions.
Why is it specific to this board? Does the board have a PHY which is
broken and requires this property? What we are missing are the details
needed to help you get to the correct way to solve the problem you are
facing.
Andrew
On 2024-12-27 01:21, Andrew Lunn wrote:
> On Thu, Dec 26, 2024 at 10:29:45AM +0800, Yijie Yang wrote:
>>
>>
>> On 2024-12-25 19:37, Krzysztof Kozlowski wrote:
>>> On 25/12/2024 11:04, Yijie Yang wrote:
>>>
>>>> static int qcom_ethqos_probe(struct platform_device *pdev)
>>>> {
>>>> - struct device_node *np = pdev->dev.of_node;
>>>> + struct device_node *np = pdev->dev.of_node, *root;
>>>> const struct ethqos_emac_driver_data *data;
>>>> struct plat_stmmacenet_data *plat_dat;
>>>> struct stmmac_resources stmmac_res;
>>>> @@ -810,6 +805,15 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>> ret = of_get_phy_mode(np, ðqos->phy_mode);
>>>> if (ret)
>>>> return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>>> +
>>>> + root = of_find_node_by_path("/");
>>>> + if (root && of_device_is_compatible(root, "qcom,sa8540p-ride"))
>>>
>>>
>>> Nope, your drivers are not supposed to poke root compatibles. Drop and
>>> fix your driver to behave correctly for all existing devices.
>>>
>>
>> Since this change introduces a new flag in the DTS, we must maintain ABI
>> compatibility with the kernel. The new flag is specific to the board, so I
>> need to ensure root nodes are matched to allow older boards to continue
>> functioning as before. I'm happy to adopt that approach if there are any
>> more elegant solutions.
>
> Why is it specific to this board? Does the board have a PHY which is
> broken and requires this property? What we are missing are the details
> needed to help you get to the correct way to solve the problem you are
> facing.
>
Let me clarify why this bit is necessary and why it's board-specific.
The RX programming swap bit can introduce a time delay of half a clock
cycle. This bit, along with the clock delay adjustment functionality, is
implemented by a module called 'IO Macro.' This is a Qualcomm-specific
hardware design located between the MAC and PHY in the SoC, serving the
RGMII interface. The bit works in conjunction with delay adjustment to
meet the sampling requirements. The sampling of RX data is also handled
by this module.
During the board design stage, the RGMII requirements may not have been
strictly followed, leading to uncertainty in the relationship between
the clock and data waveforms when they reach the IO Macro. This means
the time delay introduced by the PC board may not be zero. Therefore,
it's necessary for software developers to tune both the RX programming
swap bit and the delay to ensure correct sampling.
> Andrew
>
--
Best Regards,
Yijie
> > Why is it specific to this board? Does the board have a PHY which is
> > broken and requires this property? What we are missing are the details
> > needed to help you get to the correct way to solve the problem you are
> > facing.
> >
>
> Let me clarify why this bit is necessary and why it's board-specific. The RX
> programming swap bit can introduce a time delay of half a clock cycle. This
> bit, along with the clock delay adjustment functionality, is implemented by
> a module called 'IO Macro.' This is a Qualcomm-specific hardware design
> located between the MAC and PHY in the SoC, serving the RGMII interface. The
> bit works in conjunction with delay adjustment to meet the sampling
> requirements. The sampling of RX data is also handled by this module.
>
> During the board design stage, the RGMII requirements may not have been
> strictly followed, leading to uncertainty in the relationship between the
> clock and data waveforms when they reach the IO Macro.
So this indicates any board might need this feature, not just this one
board. Putting the board name in the driver then does not scale.
> This means the time
> delay introduced by the PC board may not be zero. Therefore, it's necessary
> for software developers to tune both the RX programming swap bit and the
> delay to ensure correct sampling.
O.K. Now look at how other boards tune their delays. There are
standard properties for this:
rx-internal-delay-ps:
description:
RGMII Receive Clock Delay defined in pico seconds. This is used for
controllers that have configurable RX internal delays. If this
property is present then the MAC applies the RX delay.
tx-internal-delay-ps:
description:
RGMII Transmit Clock Delay defined in pico seconds. This is used for
controllers that have configurable TX internal delays. If this
property is present then the MAC applies the TX delay.
I think you can use these properties, maybe with an additional comment
in the binding. RGMII running at 1G has a clock of 125MHz. That is a
period of 8ns. So a half clock cycle delay is then 4ns.
So an rx-internal-delay-ps of 0-2000 means this clock invert should be
disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert
should be enabled.
Now, ideally, you want the PHY to add the RGMII delays, that is what i
request all MAC/PHY pairs do, so we have a uniform setup across all
boards. So unless the PHY does not support RGMII delays, you would
expect rx-internal-delay-ps to be either just a small number of
picoseconds for fine tuning, or a small number of picoseconds + 4ns
for fine tuning.
This scales, since it can be used by an board with poor design, and it
does not require anything proprietary to Qualcomm, except the extended
range, and hopefully nobody except Qualcomms broken RDK will require
it, because obviously you will document the issue with the RDK and
tell customers how to correctly design their board to be RGMII
compliant with the clocks.
Andrew
On 2025-01-08 21:29, Andrew Lunn wrote: >>> Why is it specific to this board? Does the board have a PHY which is >>> broken and requires this property? What we are missing are the details >>> needed to help you get to the correct way to solve the problem you are >>> facing. >>> >> >> Let me clarify why this bit is necessary and why it's board-specific. The RX >> programming swap bit can introduce a time delay of half a clock cycle. This >> bit, along with the clock delay adjustment functionality, is implemented by >> a module called 'IO Macro.' This is a Qualcomm-specific hardware design >> located between the MAC and PHY in the SoC, serving the RGMII interface. The >> bit works in conjunction with delay adjustment to meet the sampling >> requirements. The sampling of RX data is also handled by this module. >> >> During the board design stage, the RGMII requirements may not have been >> strictly followed, leading to uncertainty in the relationship between the >> clock and data waveforms when they reach the IO Macro. > > So this indicates any board might need this feature, not just this one > board. Putting the board name in the driver then does not scale. > Should I ignore this if I choose to use the following standard properties? >> This means the time >> delay introduced by the PC board may not be zero. Therefore, it's necessary >> for software developers to tune both the RX programming swap bit and the >> delay to ensure correct sampling. > > O.K. Now look at how other boards tune their delays. There are > standard properties for this: > > rx-internal-delay-ps: > description: > RGMII Receive Clock Delay defined in pico seconds. This is used for > controllers that have configurable RX internal delays. If this > property is present then the MAC applies the RX delay. > tx-internal-delay-ps: > description: > RGMII Transmit Clock Delay defined in pico seconds. This is used for > controllers that have configurable TX internal delays. If this > property is present then the MAC applies the TX delay. > > I think you can use these properties, maybe with an additional comment > in the binding. RGMII running at 1G has a clock of 125MHz. That is a > period of 8ns. So a half clock cycle delay is then 4ns. > > So an rx-internal-delay-ps of 0-2000 means this clock invert should be > disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert > should be enabled. This board was designed to operate at different speed rates, not a fixed speed, and the clock rate varies for each speed. Thus, the delay introduced by inverting the clock is not fixed. Additionally, I noticed that some vendors apply the same routine for this property across all speeds in their driver code. Can this property be used just as a flag, regardless of its actual value? > > Now, ideally, you want the PHY to add the RGMII delays, that is what i > request all MAC/PHY pairs do, so we have a uniform setup across all > boards. So unless the PHY does not support RGMII delays, you would > expect rx-internal-delay-ps to be either just a small number of > picoseconds for fine tuning, or a small number of picoseconds + 4ns > for fine tuning. The delay for both TX and RX sides is added by the MAC in the Qualcomm driver, which was introduced by the initial patch. I believe there may be a refactor in the future to ensure it follows the requirements. > > This scales, since it can be used by an board with poor design, and it > does not require anything proprietary to Qualcomm, except the extended > range, and hopefully nobody except Qualcomms broken RDK will require > it, because obviously you will document the issue with the RDK and > tell customers how to correctly design their board to be RGMII > compliant with the clocks. Yes, I will make a note of it. > > Andrew -- Best Regards, Yijie
> > So this indicates any board might need this feature, not just this one > > board. Putting the board name in the driver then does not scale. > > > > Should I ignore this if I choose to use the following standard properties? You should always follow standard properties unless they don't work. And if they don't work, your commit message needs to explain why they don't work forcing your to do something special. > > > This means the time > > > delay introduced by the PC board may not be zero. Therefore, it's necessary > > > for software developers to tune both the RX programming swap bit and the > > > delay to ensure correct sampling. > > > > O.K. Now look at how other boards tune their delays. There are > > standard properties for this: > > > > rx-internal-delay-ps: > > description: > > RGMII Receive Clock Delay defined in pico seconds. This is used for > > controllers that have configurable RX internal delays. If this > > property is present then the MAC applies the RX delay. > > tx-internal-delay-ps: > > description: > > RGMII Transmit Clock Delay defined in pico seconds. This is used for > > controllers that have configurable TX internal delays. If this > > property is present then the MAC applies the TX delay. > > > > I think you can use these properties, maybe with an additional comment > > in the binding. RGMII running at 1G has a clock of 125MHz. That is a > > period of 8ns. So a half clock cycle delay is then 4ns. > > > > So an rx-internal-delay-ps of 0-2000 means this clock invert should be > > disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert > > should be enabled. > > This board was designed to operate at different speed rates, not a fixed > speed, and the clock rate varies for each speed. Thus, the delay introduced > by inverting the clock is not fixed. Additionally, I noticed that some > vendors apply the same routine for this property across all speeds in their > driver code. Can this property be used just as a flag, regardless of its > actual value? Maybe you should go read the RGMII standard, and then think about how your hardware actually works. RGMII always has a variable clock, with different clock speeds for 10/100/1G. So your board design is just plain normal, not special. Does the standard talk about different delays for different speeds? As you say, other drivers apply the same delay for all speeds. Why should your hardware be special? RGMII has been around for 25 years. Do you really think your RGMII implementation needs something special which no other implementation has needed in the last 25 years? > > Now, ideally, you want the PHY to add the RGMII delays, that is what i > > request all MAC/PHY pairs do, so we have a uniform setup across all > > boards. So unless the PHY does not support RGMII delays, you would > > expect rx-internal-delay-ps to be either just a small number of > > picoseconds for fine tuning, or a small number of picoseconds + 4ns > > for fine tuning. > > The delay for both TX and RX sides is added by the MAC in the Qualcomm > driver, which was introduced by the initial patch. I believe there may be a > refactor in the future to ensure it follows the requirements. You can do it in the MAC. But you probably want to clearly document this, that your design is different to > 95% of systems in Linux which have the PHY do the delays. Andrew
On 2025-01-21 00:49, Andrew Lunn wrote: >>> So this indicates any board might need this feature, not just this one >>> board. Putting the board name in the driver then does not scale. >>> >> >> Should I ignore this if I choose to use the following standard properties? > > You should always follow standard properties unless they don't > work. And if they don't work, your commit message needs to explain why > they don't work forcing your to do something special. > >>>> This means the time >>>> delay introduced by the PC board may not be zero. Therefore, it's necessary >>>> for software developers to tune both the RX programming swap bit and the >>>> delay to ensure correct sampling. >>> >>> O.K. Now look at how other boards tune their delays. There are >>> standard properties for this: >>> >>> rx-internal-delay-ps: >>> description: >>> RGMII Receive Clock Delay defined in pico seconds. This is used for >>> controllers that have configurable RX internal delays. If this >>> property is present then the MAC applies the RX delay. >>> tx-internal-delay-ps: >>> description: >>> RGMII Transmit Clock Delay defined in pico seconds. This is used for >>> controllers that have configurable TX internal delays. If this >>> property is present then the MAC applies the TX delay. >>> >>> I think you can use these properties, maybe with an additional comment >>> in the binding. RGMII running at 1G has a clock of 125MHz. That is a >>> period of 8ns. So a half clock cycle delay is then 4ns. >>> >>> So an rx-internal-delay-ps of 0-2000 means this clock invert should be >>> disabled. A rx-internal-delay-ps of 4000-6000 means the clock invert >>> should be enabled. >> >> This board was designed to operate at different speed rates, not a fixed >> speed, and the clock rate varies for each speed. Thus, the delay introduced >> by inverting the clock is not fixed. Additionally, I noticed that some >> vendors apply the same routine for this property across all speeds in their >> driver code. Can this property be used just as a flag, regardless of its >> actual value? > > Maybe you should go read the RGMII standard, and then think about how > your hardware actually works. > > RGMII always has a variable clock, with different clock speeds for > 10/100/1G. So your board design is just plain normal, not > special. Does the standard talk about different delays for different > speeds? As you say, other drivers apply the same delay for all > speeds. Why should your hardware be special? > > RGMII has been around for 25 years. Do you really think your RGMII > implementation needs something special which no other implementation > has needed in the last 25 years? I do not intend to violate the regulations of the RGMII standard and aim to maintain the same delay across all speeds. But the RX programming swap bit can only introduce a delay of 180 degrees. Should I assume the 1G speed clock to calculate and determine if this bit should be enabled for all speeds? > >>> Now, ideally, you want the PHY to add the RGMII delays, that is what i >>> request all MAC/PHY pairs do, so we have a uniform setup across all >>> boards. So unless the PHY does not support RGMII delays, you would >>> expect rx-internal-delay-ps to be either just a small number of >>> picoseconds for fine tuning, or a small number of picoseconds + 4ns >>> for fine tuning. >> >> The delay for both TX and RX sides is added by the MAC in the Qualcomm >> driver, which was introduced by the initial patch. I believe there may be a >> refactor in the future to ensure it follows the requirements. > > You can do it in the MAC. But you probably want to clearly document > this, that your design is different to > 95% of systems in Linux which > have the PHY do the delays. > > Andrew -- Best Regards, Yijie
> > Maybe you should go read the RGMII standard, and then think about how > > your hardware actually works. > > > > RGMII always has a variable clock, with different clock speeds for > > 10/100/1G. So your board design is just plain normal, not > > special. Does the standard talk about different delays for different > > speeds? As you say, other drivers apply the same delay for all > > speeds. Why should your hardware be special? > > > > RGMII has been around for 25 years. Do you really think your RGMII > > implementation needs something special which no other implementation > > has needed in the last 25 years? > > I do not intend to violate the regulations of the RGMII standard and aim to > maintain the same delay across all speeds. But the RX programming swap bit > can only introduce a delay of 180 degrees. Should I assume the 1G speed > clock to calculate and determine if this bit should be enabled for all > speeds? Lets rewind a bit. The RGMII standard specified which edge you sample on. Since it is defined, no other driver has a configuration like this, they just setup there hardware to be standards compliant. Why do you need the ability to break the standard, and sample on the wrong edge? I can think of two reasons: 1) You have a PHY which is broken, it also samples on the wrong edge. This is a workaround for that broken PHY. 2) You have a board with a clock line driver inserted between the RGMII output pins and the PHY, and this line driver includes a NOT? This line driver is causing the clocking to break the RGMII standard. You are working around this broken board design by getting the MAC to invert the clock. Is there a third reason? Lets first understand the details of why you need to be able to invert the clock. Andrew
© 2016 - 2026 Red Hat, Inc.