Re: [PATCH net-next v1 1/3] dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings

From: Frank.Sae
Date: Thu Jan 19 2023 - 00:56:11 EST

Hi Andrew,

On 2023/1/18 21:40, Andrew Lunn wrote:
> On Wed, Jan 11, 2023 at 05:20:18PM +0800, Frank.Sae wrote:
>> Hi Andrew,
>>
>> On 2023/1/6 21:55, Andrew Lunn wrote:
>>>>> Why is this needed? When the MAC driver connects to the PHY, it passes
>>>>> phy-mode. For RGMII, this is one of:
>>>>
>>>>> linux/phy.h: PHY_INTERFACE_MODE_RGMII,
>>>>> linux/phy.h: PHY_INTERFACE_MODE_RGMII_ID,
>>>>> linux/phy.h: PHY_INTERFACE_MODE_RGMII_RXID,
>>>>> linux/phy.h: PHY_INTERFACE_MODE_RGMII_TXID,
>>>>>
>>>>> This tells you if you need to add a delay for the RX clock line, the
>>>>> TX clock line, or both. That is all you need to know for basic RGMII
>>>>> delays.
>>>>>
>>>>
>>>> This basic delay can be controlled by hardware or the phy-mode which
>>>> passes from MAC driver.
>>>> Default value depends on power on strapping, according to the voltage
>>>> of RXD0 pin (low = 0, turn off; high = 1, turn on).
>>>>
>>>> Add this for the case that This basic delay is controlled by hardware,
>>>> and software don't change this.
>>>
>>> You should always do what phy-mode contains. Always. We have had
>>> problems in the past where a PHY driver ignored the phy-mode, and left
>>> the PHY however it was strapped. Which worked. But developers put the
>>> wrong phy-mode value in DT. Then somebody had a board which actually
>>> required that the DT value really did work, because the strapping was
>>> wrong. So the driver was fixed to respect the PHY mode, made that
>>> board work, and broke all the other boards which had the wrong
>>> phy-mode in DT.
>>>
>>> If the user want the driver to leave the mode alone, use the
>>> strapping, they should use PHY_INTERFACE_MODE_NA. It is not well
>>> documented, but it is used in a few places. However, i don't recommend
>>> it.
>>>
>>
>> RX delay = rx-delay-basic (0ns or 1.9ns) + x-delay-additional-ps
>> (N*150ps, N = 0 ~ 15)
>> If rx-delay-basic is removed and controlled by phy-mode.
>> when phy-mode is rgmii-id or rgmii-rxid, RX delay is 1.9ns + N*150ps.
>> But sometimes 1.9ns is still too big, we just need 0ns + N*150ps.
>>
>> For this case, can we do like following ?
>> rx-internal-delay-ps:
>> enum: [ 0, 150, 300, 450, 600, 750, 900, 1050, 1200, 1350, 1500,
>> 1650, 1800, 1900, 1950, 2050, 2100, 2200, 2250, 2350, 2500, 2650, 2800,
>> 2950, 3100, 3250, 3400, 3550, 3700, 3850, 4000, 4150 ]
>> default: 0
>> rx-internal-delay-ps is 0ns + N*150ps and 1.9ns + N*150ps.
>> And check whether need rx-delay-basic (1.9ns) by the val of
>> rx-internal-delay-ps?
>
> Please take a look at phy_get_internal_delay() and the drivers which
> use it.
>
> Andrew

Thanks. But it may be not suitable.

rx-internal-delay-ps has two part:
0ns + N*150ps =
0,150,300,450,600,750,900,1050,1200,1350,1500,1650,1800,1950,2100,2250

1.9ns + N*150ps =
1900,2050,2200,2350,2500,2650,2800,2950,3100,3250,3400,3550,3700,3850,4000,4150

The problem is "1900,2050,2200" is less than "2250".

If I take this two parts in one sorted table, there will be three
tables, one for tx-internal-delay-ps, one for rx-internal-delay-ps and
one for the rx index to reg(delay value and 1.9ns on or off) value.

So we tend to use the following methods.

#define YT8521_CCR_RXC_DLY_1_900_NS 1900

#define YT8521_RC1R_RGMII_0_000_NS 0
#define YT8521_RC1R_RGMII_0_150_NS 1
...
#define YT8521_RC1R_RGMII_2_250_NS 15

struct ytphy_cfg_reg_map {
u32 cfg;
u32 reg;
};

static const struct ytphy_cfg_reg_map ytphy_rgmii_delays[] = {
/* for tx delay / rx delay with YT8521_CCR_RXC_DLY_EN is not set. */
{ 0, YT8521_RC1R_RGMII_0_000_NS },
{ 150, YT8521_RC1R_RGMII_0_150_NS },
...
{ 2250, YT8521_RC1R_RGMII_2_250_NS },

/* only for rx delay with YT8521_CCR_RXC_DLY_EN is set. */
{ 0 + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_0_000_NS },
{ 150 + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_0_150_NS },
...
{ 2250 + YT8521_CCR_RXC_DLY_1_900_NS,YT8521_RC1R_RGMII_2_250_NS }
};


static u32 ytphy_get_delay_reg_value(struct phy_device *phydev,
const char *prop_name,
const struct ytphy_cfg_reg_map *tbl,
int tb_size,
u16 *rxc_dly_en,
u32 dflt)
{
struct device_node *node = phydev->mdio.dev.of_node;
int tb_size_half = tb_size / 2;
u32 val;
int i;

if (of_property_read_u32(node, prop_name, &val))
return dflt;

/* when rxc_dly_en is NULL, it is get the delay for tx, only half of
* tb_size is valid.
*/
if (!rxc_dly_en)
tb_size = tb_size_half;

for (i = 0; i < tb_size; i++) {
if (tbl[i].cfg == val) {
if (rxc_dly_en && i < tb_size_half)
*rxc_dly_en = 0;
return tbl[i].reg;
}
}

phydev_warn(phydev, "Unsupported value %d for %s using default (%u)\n",
val, prop_name, dflt);
return dflt;
}

static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
{
int tb_size = ARRAY_SIZE(ytphy_rgmii_delays);
u16 rxc_dly_en = YT8521_CCR_RXC_DLY_EN;
u32 rx_reg, tx_reg;
u16 mask, val = 0;
int ret;

rx_reg = ytphy_get_delay_reg_value(phydev, "rx-internal-delay-ps",
ytphy_rgmii_delays, tb_size,
&rxc_dly_en,
YT8521_RC1R_RGMII_0_000_NS);
tx_reg = ytphy_get_delay_reg_value(phydev, "tx-internal-delay-ps",
ytphy_rgmii_delays, tb_size, NULL,
YT8521_RC1R_RGMII_0_150_NS);

switch (phydev->interface) {
case PHY_INTERFACE_MODE_RGMII:
rxc_dly_en = 0;
break;
case PHY_INTERFACE_MODE_RGMII_RXID:
val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg);
break;
case PHY_INTERFACE_MODE_RGMII_TXID:
rxc_dly_en = 0;
val |= FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
break;
case PHY_INTERFACE_MODE_RGMII_ID:
val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg) |
FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
break;
default: /* do not support other modes */
return -EOPNOTSUPP;
}

ret = ytphy_modify_ext(phydev, YT8521_CHIP_CONFIG_REG,
YT8521_CCR_RXC_DLY_EN, rxc_dly_en);
if (ret < 0)
return ret;

/* Generally, it is not necessary to adjust YT8521_RC1R_FE_TX_DELAY */
mask = YT8521_RC1R_RX_DELAY_MASK | YT8521_RC1R_GE_TX_DELAY_MASK;
return ytphy_modify_ext(phydev, YT8521_RGMII_CONFIG1_REG, mask, val);
}