Fix SC59x Ethernet PHY descriptions and ADIN PHY Driver - #130
Conversation
|
Related with: analogdevicesinc/linux#2883 |
|
@gastmaier the llm-review is not triggered here 😭 |
it did run https://github.com/analogdevicesinc/u-boot/actions/runs/29704103738/job/88238050338 but died |
LLM reviewThis series adds ADIN1200/1300 Ethernet PHY support for SC594/SC598 boards, fixing incorrect DP83867/DP83848 PHY descriptions in device trees, removing stale board-level GPIO-hog reset hooks, enabling run: 29742054170
|
Configure the shared ADIN vendor registers for both ADIN1200 and ADIN1300 PHYs. This enables RMII mode and programs the RMII FIFO depth for ADIN1200 instead of relying only on generic PHY configuration. Keep the device-tree values in physical units. When the delay and FIFO properties are absent, use the ADIN1300 defaults of 2 ns for RGMII internal delay and 8 bits for RMII FIFO depth. Signed-off-by: Ozan Durgut <ozan.durgut@analog.com>
Enable the ADIN PHY driver in the SC594 and SC598 SPL defconfigs, which describe ADIN1200 and ADIN1300 PHYs. Signed-off-by: Ozan Durgut <ozan.durgut@analog.com>
The ADIN1300 reset GPIO is now described by the PHY node. Remove the obsolete carrier reset hooks so the PHY driver is the sole reset owner. Signed-off-by: Ozan Durgut <ozan.durgut@analog.com>
The SC598 boards use an ADIN1300 on eth0, and the EZKIT also uses an ADIN1200 on eth1. Replace the stale DP83867 description, describe the second RMII PHY, and remove the obsolete board PHY fixup. Move the ADIN1300 reset GPIO into the PHY nodes. The ADIN1300 data sheet, Table 3, requires RESET_N to be asserted for at least 10 us and specifies a 5 ms delay before MDIO registers are accessible after reset deassertion. Signed-off-by: Ozan Durgut <ozan.durgut@analog.com>
The SC594 EZKIT uses an ADIN1300 on eth0 and an ADIN1200 on eth1. Replace the stale PHY descriptions, describe the second RMII PHY, and remove the obsolete board PHY fixup. Move the ADIN1300 reset GPIO into the PHY nodes. The ADIN1300 data sheet, Table 3, requires RESET_N to be asserted for at least 10 us and specifies a 5 ms delay before MDIO registers are accessible after reset deassertion. Signed-off-by: Ozan Durgut <ozan.durgut@analog.com>
34946ba to
036effb
Compare
|
|
||
| static int adin1300_config(struct phy_device *phydev) | ||
| { | ||
| printf("ADIN1300 PHY detected at addr %d\n", phydev->addr); |
There was a problem hiding this comment.
I do wonder how useful this print really is (imagine if all devices announced their presence like this)
|
|
||
| dflt = adin_lookup_reg_value(tbl, dflt_cfg); | ||
| if (dflt < 0) | ||
| return dflt; |
There was a problem hiding this comment.
if dflt is negative, you are then implicitly casting that negative value to a u32 when returning. looks like the callers of this function expect a valid value to always be returned. but now you might be returning ((u32)(-EINVAL)) which is pretty dank
I am all for error handling, but maybe it's easier to just drop this check since it is anyway unrelated to your PR. otherwise you need to find a way for this function to unambiguously return error/success, and also update the callers to actually check for it (otherwise why bother?)
| printf("%s: failed to find %s, using default %d\n", | ||
| __func__, prop_name, dflt); | ||
| printf("%s: failed to find %s, using default %u\n", | ||
| __func__, prop_name, dflt_cfg); |
There was a problem hiding this comment.
if the property is not required by the bindings then I think such a print is a bit noisy, but maybe it's just me
| return adin_config(phydev); | ||
| } | ||
|
|
||
| U_BOOT_PHY_DRIVER(ADIN1200) = { |
There was a problem hiding this comment.
ah, weird, I was looking at mainline u-boot and noticed this is already there.
Since we agreed in the Linux PR not to bother explicitly configuring RMII FIFO size, maybe you wanna just backport the above patch instead? Should achieve the same thing (except erroneously printing "ADIN1300" detected for an ADIN1200)
| #size-cells = <0>; | ||
|
|
||
| adin1200: ethernet-phy@1 { | ||
| compatible = "ethernet-phy-ieee802.3-c22"; |
There was a problem hiding this comment.
not sure what u-boot policy is on whether to require this compatible or not, but worth a look
| #size-cells = <0>; | ||
|
|
||
| dp83867: ethernet-phy@0 { | ||
| adin1300: ethernet-phy@0 { |
There was a problem hiding this comment.
this doesn't look right. the PHY is on the carrier board, not the SoM
| { | ||
| struct gpio_desc *eth1; | ||
| struct gpio_desc *eth1_reset; | ||
| struct gpio_desc *gige_reset; |
There was a problem hiding this comment.
this commit should be ordered after the update to the DT, or reworded as such, since in the commit message it suggests that the reset has already been put there
This series updates SC594 and SC598 + EZKIT or EZLITE ethernet support for ADIN1200 and ADIN1300 PHYs.
Changes include:
Hardware test is performed on SC598 with EZKIT on ADIN1300:
References:
https://www.analog.com/media/en/technical-documentation/data-sheets/ADIN1300.pdf
https://wiki.analog.com/resources/tools-software/linux-drivers/net-phy/adin