Re: [PATCH v2 03/11] net: stmmac: dwmac-stm32: rework glue to simplify management

[Marek Vasut <marex@denx.de>]

On 5/13/24 2:48 PM, Christophe ROULLIER wrote:
> Hi
> 
> On 4/26/24 16:53, Marek Vasut wrote:
>> On 4/26/24 2:56 PM, Christophe Roullier wrote:
>>> Change glue to be more generic and manage easily next stm32 products.
>>> The goal of this commit is to have one stm32mp1_set_mode function which
>>> can manage different STM32 SOC. SOC can have different SYSCFG register
>>> bitfields. so in pmcsetr we defined the bitfields corresponding to 
>>> the SOC.
>>>
>>> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com>
>>> ---
>>>   .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------
>>>   1 file changed, 51 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c 
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> index c92dfc4ecf57..68a02de25ac7 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
>>> @@ -23,10 +23,6 @@
>>>     #define SYSCFG_MCU_ETH_MASK        BIT(23)
>>>   #define SYSCFG_MP1_ETH_MASK        GENMASK(23, 16)
>>> -#define SYSCFG_PMCCLRR_OFFSET        0x40
>>> -
>>> -#define SYSCFG_PMCR_ETH_CLK_SEL        BIT(16)
>>> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL    BIT(17)
>>>     /* CLOCK feed to PHY*/
>>>   #define ETH_CK_F_25M    25000000
>>> @@ -46,9 +42,6 @@
>>>    * RMII  |   1     |   0      |   0       |  n/a  |
>>>    *------------------------------------------
>>>    */
>>> -#define SYSCFG_PMCR_ETH_SEL_MII        BIT(20)
>>> -#define SYSCFG_PMCR_ETH_SEL_RGMII    BIT(21)
>>> -#define SYSCFG_PMCR_ETH_SEL_RMII    BIT(23)
>>>   #define SYSCFG_PMCR_ETH_SEL_GMII    0
>>>   #define SYSCFG_MCU_ETH_SEL_MII        0
>>>   #define SYSCFG_MCU_ETH_SEL_RMII        1
>>> @@ -90,19 +83,33 @@ struct stm32_dwmac {
>>>       int eth_ref_clk_sel_reg;
>>>       int irq_pwr_wakeup;
>>>       u32 mode_reg;         /* MAC glue-logic mode register */
>>> +    u32 mode_mask;
>>>       struct regmap *regmap;
>>>       u32 speed;
>>>       const struct stm32_ops *ops;
>>>       struct device *dev;
>>>   };
>>>   +struct stm32_syscfg_pmcsetr {
>>> +    u32 eth1_clk_sel;
>>> +    u32 eth1_ref_clk_sel;
>>> +    u32 eth1_selmii;
>>> +    u32 eth1_sel_rgmii;
>>> +    u32 eth1_sel_rmii;
>>> +    u32 eth2_clk_sel;
>>> +    u32 eth2_ref_clk_sel;
>>> +    u32 eth2_sel_rgmii;
>>> +    u32 eth2_sel_rmii;
>>> +};
>>
>> [...]
>>
>>> @@ -487,8 +502,19 @@ static struct stm32_ops stm32mp1_dwmac_data = {
>>>       .suspend = stm32mp1_suspend,
>>>       .resume = stm32mp1_resume,
>>>       .parse_data = stm32mp1_parse_data,
>>> -    .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
>>> -    .clk_rx_enable_in_suspend = true
>>> +    .clk_rx_enable_in_suspend = true,
>>> +    .syscfg_clr_off = 0x44,
>>> +    .pmcsetr = {
>>> +        .eth1_clk_sel        = BIT(16),
>>> +        .eth1_ref_clk_sel    = BIT(17),
>>> +        .eth1_selmii        = BIT(20),
>>> +        .eth1_sel_rgmii        = BIT(21),
>>> +        .eth1_sel_rmii        = BIT(23),
>>> +        .eth2_clk_sel        = 0,
>>> +        .eth2_ref_clk_sel    = 0,
>>> +        .eth2_sel_rgmii        = 0,
>>> +        .eth2_sel_rmii        = 0
>>> +    }
>>>   };
>>
>> Is this structure really necessary ?
>>
> I prefer to keep this implementation for the moment, as it is working 
> fine. Maybe at a later stage, I will send some optimizations.

BIT() and left shift by 8 works all the same, without all this 
complexity and new structures, since all you really have on MP13 are two 
identical bitfields (one at offset 16, the other at offset 24) with the 
same bits in them, so why not make this simple ?