Re: [PATCH net-next 3/3] net: stmmac: Add UltraRISC DP1000 GMAC support

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 9/7/26 07:48, Jia Wang wrote:
> On 2026-09-04 14:01 +0800, Jia Wang wrote:
>> On 2026-09-03 14:30 +0200, Maxime Chevallier wrote:
>>>
>>>
>>> On 9/3/26 14:10, Andrew Lunn wrote:
>>>>>> This looks to be 90% identical to dwmac-sophgo.c.
>>>>>>
>>>>>> Do you expect any further changes to this driver? Is everything
>>>>>> supported? Are there more clocks? A GPIO for WoL?
>>>>>>
>>>>>> I'm just wondering if the common code should be pulled out, or even
>>>>>> dwmac-sophgo.c made generic so it can handle all plain boring devices
>>>>>> which have RGMII delays and not a lot else.
>>>>>>
>>>>>
>>>>> The probe boilerplate is indeed largely similar because both drivers
>>>>> use the common stmmac platform helpers. However, their platform-specific
>>>>> requirements differ: Sophgo requires additional TX clock handling and
>>>>> other configuration, while DP1000 requires fixed TX and RX delay
>>>>> handling. ACPI support under development is also intended to use the
>>>>> UltraRISC glue driver.
>>>>
>>>> ACPI is going to be fun, since nobody does ACPI networking.
>>>>
>>>> But how big is the change to the glue driver? I expect the change will
>>>> be minor, a call to devm_stmmac_probe_config_acpi(), in addition to
>>>> devm_stmmac_probe_config_dt(). Anything else?
>>>
>>> I agree that if ACPI support is added, let's try to make it part of the
>>> core.
>>>
>>
>> The ACPI support is still under development, but the current prototype
>> adds ACPI device matching and ACPI-specific platform-data setup. Some
>> common stmmac changes are also needed for ACPI-described platform
>> resources and MDIO.
>>
>>>>
>>>> One problem the stmmac driver has is cut/paste glue driver
>>>> development. The same code is repeated again and again. Russell King
>>>> spent a lot of time and effort trying to reduce the duplication. So we
>>>> should not be adding more duplicated code unless it is actually
>>>> required.
>>>
>>> It seems to me that all the RGMII handling could be made much more generic,
>>> as this is commong pattern in glue drivers :
>>>
>>> dwmac-sophgo has the "has_internal_rx_delay" field, and dwmac-eic7700 has
>>> both has_internal_[r|t]x_delay.
>>>
>>> With this third one, it makes sense to start sharing that logic across
>>> glues.
>>>
>>
>> I agree. DP1000 is another user of this pattern, with fixed internal
>> delays on both the TX and RX paths. I will look at moving this handling
>> into the common stmmac platform code, leaving each glue driver to
>> describe its internal delay capabilities.
>>
> 
> After looking further into the implementation, I see two possible
> approaches:
> 
> 1. Keep a minimal UltraRISC glue driver that only describes the fixed TX
> and RX delay capabilities, while the common stmmac code handles the PHY
> mode adjustment.
> 
> 2. Store these capabilities as compatible-specific match data in
> dwmac-generic and remove the UltraRISC glue driver entirely.
Let's go with 1 for now I'd say. UtraRISC will be the only one that's basically
an empty glue calling core code (maybe you have some plat->flags to passs ?).

If we manage to simplify even further (e.g. tx clock handling for exmaple), we could
also deal with dwmac-sophgo the same way. We could then consider something like
dwmac-simple.c , keeping dwmac-generic.c for the raw dwmac IPs.

Maxime