Re: [net-next,PATCH 2/2] net: stmmac: dwmac-stm32: stm32: add management of stm32mp25 for stm32

[Christophe ROULLIER <christophe.roullier@foss.st.com>]

Hi Marek,

On 6/14/24 15:58, Marek Vasut wrote:
> On 6/14/24 3:08 PM, Christophe Roullier wrote:
>
> [...]
>
>> +static int stm32mp2_configure_syscfg(struct plat_stmmacenet_data 
>> *plat_dat)
>> +{
>> +    struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
>> +    u32 reg = dwmac->mode_reg;
>> +    int val = 0;
>> +
>> +    switch (plat_dat->mac_interface) {
>> +    case PHY_INTERFACE_MODE_MII:
>> +        break;
>
> dwmac->enable_eth_ck does not apply to MII mode ? Why ?

It is like MP1 and MP13, nothing to set in syscfg register for case MII 
mode wo crystal.

>
>> +    case PHY_INTERFACE_MODE_GMII:
>> +        if (dwmac->enable_eth_ck)
>> +            val |= SYSCFG_ETHCR_ETH_CLK_SEL;
>> +        break;
>> +    case PHY_INTERFACE_MODE_RMII:
>> +        val = SYSCFG_ETHCR_ETH_SEL_RMII;
>> +        if (dwmac->enable_eth_ck)
>> +            val |= SYSCFG_ETHCR_ETH_REF_CLK_SEL;
>> +        break;
>> +    case PHY_INTERFACE_MODE_RGMII:
>> +    case PHY_INTERFACE_MODE_RGMII_ID:
>> +    case PHY_INTERFACE_MODE_RGMII_RXID:
>> +    case PHY_INTERFACE_MODE_RGMII_TXID:
>> +        val = SYSCFG_ETHCR_ETH_SEL_RGMII;
>> +        if (dwmac->enable_eth_ck)
>> +            val |= SYSCFG_ETHCR_ETH_CLK_SEL;
>> +        break;
>> +    default:
>> +        dev_err(dwmac->dev, "Mode %s not supported",
>> +            phy_modes(plat_dat->mac_interface));
>> +        /* Do not manage others interfaces */
>> +        return -EINVAL;
>> +    }
>> +
>> +    dev_dbg(dwmac->dev, "Mode %s", phy_modes(plat_dat->mac_interface));
>> +
>> +    /*  select PTP (IEEE1588) clock selection from RCC 
>> (ck_ker_ethxptp) */
>
> Drop extra leading space.
> Sentence starts with capital letter.
ok
>
>> +    val |= SYSCFG_ETHCR_ETH_PTP_CLK_SEL;
>> +
>> +    /* Update ETHCR (set register) */
>> +    return regmap_update_bits(dwmac->regmap, reg,
>> +                 SYSCFG_MP2_ETH_MASK, val);
>> +}
>> +
>>   static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
>>   {
>>       int ret;
>> @@ -292,6 +346,21 @@ static int stm32mp1_set_mode(struct 
>> plat_stmmacenet_data *plat_dat)
>>       return stm32mp1_configure_pmcr(plat_dat);
>>   }
>>   +static int stm32mp2_set_mode(struct plat_stmmacenet_data *plat_dat)
>> +{
>> +    int ret;
>> +
>> +    ret = stm32mp1_select_ethck_external(plat_dat);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = stm32mp1_validate_ethck_rate(plat_dat);
>> +    if (ret)
>> +        return ret;
>
>
> Is it necessary to duplicate this entire function instead of some:
>
> if (is_mp2)
>   return stm32mp2_configure_syscfg(plat_dat);
> else
>   return stm32mp1_configure_syscfg(plat_dat);
>
> ?
ok I would like to avoid to use is_mp2 boolean but you are right it is 
simplify visibility of the code.
>
>> +    return stm32mp2_configure_syscfg(plat_dat);
>> +}
>> +
>>   static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat)
>>   {
>>       struct stm32_dwmac *dwmac = plat_dat->bsp_priv;
>> @@ -348,12 +417,6 @@ static int stm32_dwmac_parse_data(struct 
>> stm32_dwmac *dwmac,
>>           return PTR_ERR(dwmac->clk_rx);
>>       }
>>   -    if (dwmac->ops->parse_data) {
>> -        err = dwmac->ops->parse_data(dwmac, dev);
>> -        if (err)
>> -            return err;
>> -    }
>> -
>>       /* Get mode register */
>>       dwmac->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
>>       if (IS_ERR(dwmac->regmap))
>> @@ -365,20 +428,14 @@ static int stm32_dwmac_parse_data(struct 
>> stm32_dwmac *dwmac,
>>           return err;
>>       }
>>   -    dwmac->mode_mask = SYSCFG_MP1_ETH_MASK;
>> -    err = of_property_read_u32_index(np, "st,syscon", 2, 
>> &dwmac->mode_mask);
>> -    if (err) {
>> -        if (dwmac->ops->is_mp13)
>> -            dev_err(dev, "Sysconfig register mask must be set 
>> (%d)\n", err);
>> -        else
>> -            dev_dbg(dev, "Warning sysconfig register mask not set\n");
>> -    }
>> +    if (dwmac->ops->parse_data)
>> +        err = dwmac->ops->parse_data(dwmac, dev);
>
> Why is this change here ? What is the purpose ?
> This should be documented in commit message too.
>
> The indirect call is not necessary either, simply do
>
> if (is_mp2)
>   return err;
>
> ... do mp15/13 stuff here ...
>
> return err;
Right, with use of is_mp2 variable it is more simple
>
> [...]