| Message ID | 20230105073024.8390-1-Frank.Sae@motor-comm.com |
|---|---|
| Headers | show |
| Series | add dts for yt8521 and yt8531s, add driver for yt8531 | expand |
On 05/01/2023 08:30, Frank wrote: > Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver. > Subject: drop second, redundant "Driver bindings". > Signed-off-by: Frank <Frank.Sae@motor-comm.com> Use full first and last name. Your email suggests something more than only "Frank". > --- > .../bindings/net/motorcomm,yt8xxx.yaml | 180 ++++++++++++++++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > MAINTAINERS | 1 + > 3 files changed, 183 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml > > diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml > new file mode 100644 > index 000000000000..337a562d864c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml > @@ -0,0 +1,180 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/motorcomm,yt8xxx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MotorComm yt8xxx Ethernet PHY > + > +maintainers: > + - frank <frank.sae@motor-comm.com> > + > +description: | > + Bindings for MotorComm yt8xxx PHYs. Instead describe the hardware. No need to state the obvious that these are bindings. > + yt8511 will be supported later. Bindings should be complete. Your driver support is not relevant here. > + > +allOf: > + - $ref: ethernet-phy.yaml# > + > +properties: > + motorcomm,clk-out-frequency: Use property suffixes matching the type. > + description: clock output in Hertz on clock output pin. Drop "Hertz". It should be obvious from the suffix. > + $ref: /schemas/types.yaml#/definitions/uint32 Drop. Anyway, does it fit standard clock-frequency property? > + enum: [0, 25000000, 125000000] > + default: 0 > + > + motorcomm,rx-delay-basic: > + description: | > + Tristate, setup the basic RGMII RX Clock delay of PHY. > + This basic delay is fixed at 2ns (1000Mbps) or 8ns (100Mbps、10Mbps). > + This basic delay usually auto set by hardware according to the voltage > + of RXD0 pin (low = 0, turn off; high = 1, turn on). > + If not exist, this delay is controlled by hardware. I don't understand that at all. What "not exist"? There is no verb and no subject. The type and description are really unclear. > + 0: turn off; 1: turn on. > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] So this is bool? > + > + motorcomm,rx-delay-additional-ps: > + description: | > + Setup the additional RGMII RX Clock delay of PHY defined in pico seconds. > + RGMII RX Clock Delay = rx-delay-basic + rx-delay-additional-ps. > + enum: Best regards, Krzysztof
Hi Krzysztof Kozlowski, On 2023/1/6 16:26, Krzysztof Kozlowski wrote: > On 05/01/2023 08:30, Frank wrote: >> Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver. >> > > Subject: drop second, redundant "Driver bindings". Change Subject from dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings to dt-bindings: net: Add Motorcomm yt8xxx ethernet phy ? > >> Signed-off-by: Frank <Frank.Sae@motor-comm.com> > > Use full first and last name. Your email suggests something more than > only "Frank". > OK , I will use Frank.Sae <Frank.Sae@motor-comm.com> >> --- >> .../bindings/net/motorcomm,yt8xxx.yaml | 180 ++++++++++++++++++ >> .../devicetree/bindings/vendor-prefixes.yaml | 2 + >> MAINTAINERS | 1 + >> 3 files changed, 183 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml >> new file mode 100644 >> index 000000000000..337a562d864c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml >> @@ -0,0 +1,180 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/motorcomm,yt8xxx.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: MotorComm yt8xxx Ethernet PHY >> + >> +maintainers: >> + - frank <frank.sae@motor-comm.com> >> + >> +description: | >> + Bindings for MotorComm yt8xxx PHYs. > > Instead describe the hardware. No need to state the obvious that these > are bindings. > I will fix. >> + yt8511 will be supported later. > > Bindings should be complete. Your driver support is not relevant here. I will fix. > >> + >> +allOf: >> + - $ref: ethernet-phy.yaml# >> + >> +properties: >> + motorcomm,clk-out-frequency: > > Use property suffixes matching the type. > >> + description: clock output in Hertz on clock output pin. > > Drop "Hertz". It should be obvious from the suffix. > >> + $ref: /schemas/types.yaml#/definitions/uint32 > > Drop. > > Anyway, does it fit standard clock-frequency property? > >> + enum: [0, 25000000, 125000000] >> + default: 0 >> + Yes, I will fix. >> + motorcomm,rx-delay-basic: >> + description: | >> + Tristate, setup the basic RGMII RX Clock delay of PHY. >> + This basic delay is fixed at 2ns (1000Mbps) or 8ns (100Mbps、10Mbps). >> + This basic delay usually auto set by hardware according to the voltage >> + of RXD0 pin (low = 0, turn off; high = 1, turn on). >> + If not exist, this delay is controlled by hardware. > > I don't understand that at all. What "not exist"? There is no verb and > no subject. > > The type and description are really unclear. > >> + 0: turn off; 1: turn on. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1] > > So this is bool? > This basic delay can be controlled by hardware or dts. Default value depends on power on strapping, according to the voltage of RXD0 pin (low = 0, turn off; high = 1, turn on). "not exist" means that This basic delay is controlled by hardware, and software don't change this. I will fix. >> + >> + motorcomm,rx-delay-additional-ps: >> + description: | >> + Setup the additional RGMII RX Clock delay of PHY defined in pico seconds. >> + RGMII RX Clock Delay = rx-delay-basic + rx-delay-additional-ps. >> + enum: > > Best regards, > Krzysztof
On 06/01/2023 10:17, Frank.Sae wrote: > Hi Krzysztof Kozlowski, > > On 2023/1/6 16:26, Krzysztof Kozlowski wrote: >> On 05/01/2023 08:30, Frank wrote: >>> Add a YAML binding document for the Motorcom yt8xxx Ethernet phy driver. >>> >> >> Subject: drop second, redundant "Driver bindings". > > Change Subject from > dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings > to > dt-bindings: net: Add Motorcomm yt8xxx ethernet phy > ? Yes. > >> >>> Signed-off-by: Frank <Frank.Sae@motor-comm.com> >> >> Use full first and last name. Your email suggests something more than >> only "Frank". >> > > OK , I will use Frank.Sae <Frank.Sae@motor-comm.com> Without "dot" between parts of name. > >>> --- >>> .../bindings/net/motorcomm,yt8xxx.yaml | 180 ++++++++++++++++++ >>> .../devicetree/bindings/vendor-prefixes.yaml | 2 + >>> MAINTAINERS | 1 + Best regards, Krzysztof
> > 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. > >> + motorcomm,tx-delay-fe-ps: > > > > So you can only set the TX delay? What is RX delay set to? Same as 1G? > > I would suggest you call this motorcomm,tx-internal-delay-fe-ps, so > > that it is similar to the standard tx-internal-delay-ps. > > > > TX delay has two type: tx-delay-ge-ps for 1G and tx-delay-fe-ps for 100M. > > RX delay set for 1G and 100M, but it has two type, rx-delay-basic and > rx-delay-additional-ps, RX delay = rx-delay-basic + rx-delay-additional-ps. > > I will rename to tx-internal-delay-fe-ps and tx-internal-delay-ge-ps. So you can set the TX delay for 1G and Fast, but RX delay has a single setting for both 1G and Fast? Have you seen boards what actually need different TX delays like this? Just because the hardware supports something does not mean Linux needs to support it. Unless there is a real need for it. So i would suggest your drop this DT property, and set the Fast delay to the same as the 1G delay. If any board actually requires this in the future, the property can be added then. > > > These two i can see being useful. But everything afterwards seems like > > just copy/paste from vendor SDK for things which the hardware can do, > > but probably nobody ever uses. Do you have a board using any of the > > following properties? > > > > tx-clk-adj-enabled, tx-clk-10-inverted, tx-clk-100-inverted and > tx-clk-1000-inverted is used and tested by Yanhong > Wang<yanhong.wang@starfivetech.com>. They used yt8531 on > jh7110-starfive-visionfive-v2. This will provide an additional way to > adjust the tx clk delay on yt8531. O.K. So they are used with a real board. Can we reduce this down to tx-clk-inverted? Have you ever seen a board which only needs the invert for one speed and not the others? To me, that would be a very odd design. > sds-tx-amplitude can be tested on my yt8531s board. Does the board break with the default value? Just because you can test it on your RDK does not mean anybody will ever use it. Andrew
> >> + motorcomm,rx-delay-basic: > >> + description: | > >> + Tristate, setup the basic RGMII RX Clock delay of PHY. > >> + This basic delay is fixed at 2ns (1000Mbps) or 8ns (100Mbps、10Mbps). > >> + This basic delay usually auto set by hardware according to the voltage > >> + of RXD0 pin (low = 0, turn off; high = 1, turn on). > >> + If not exist, this delay is controlled by hardware. > > > > I don't understand that at all. What "not exist"? There is no verb and > > no subject. > > > > The type and description are really unclear. > > > >> + 0: turn off; 1: turn on. > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + enum: [0, 1] > > > > So this is bool? > > > > This basic delay can be controlled by hardware or dts. > > Default value depends on power on strapping, according to the voltage > of RXD0 pin (low = 0, turn off; high = 1, turn on). > > "not exist" means that This basic delay is controlled by hardware, > and software don't change this. As i said in my other reply, this is not how Linux controls this. This property should be removed. Andrew
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); }
Add dts support for yt8521 and yt8531s, add driver for yt8531. These patches have been tested on our AM335x platform (motherboard) which has one RGMII interface. It can connect to daughter boards like yt8531s or yt8521 board. Passed all test cases. Frank (3): dt-bindings: net: Add Motorcomm yt8xxx ethernet phy Driver bindings net: phy: Add dts support for Motorcomm yt8521/yt8531s gigabit ethernet phy net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy .../bindings/net/motorcomm,yt8xxx.yaml | 180 +++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 1 + drivers/net/phy/Kconfig | 2 +- drivers/net/phy/motorcomm.c | 638 +++++++++++++++--- 5 files changed, 740 insertions(+), 83 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml