Re: [net-next PATCH RESEND 2/2] net: macb: add polarfire soc reset support

[Claudiu.Beznea]

On 02.07.2022 18:34, Conor Dooley - M52691 wrote:
> On 01/07/2022 08:55, Conor Dooley wrote:
>> On 01/07/2022 08:47, Claudiu Beznea - M18063 wrote:
>>> On 01.07.2022 09:58, Conor Dooley wrote:
>>>> To date, the Microchip PolarFire SoC (MPFS) has been using the
>>>> cdns,macb compatible, however the generic device does not have reset
>>>> support. Add a new compatible & .data for MPFS to hook into the reset
>>>> functionality added for zynqmp support (and make the zynqmp init
>>>> function generic in the process).
>>>>
>>>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>> ---
>>>>   drivers/net/ethernet/cadence/macb_main.c | 25 +++++++++++++++++-------
>>>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>>> index d89098f4ede8..325f0463fd42 100644
>>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>>> @@ -4689,33 +4689,32 @@ static const struct macb_config np4_config = {
>>>>       .usrio = &macb_default_usrio,
>>>>   };
>>>>   -static int zynqmp_init(struct platform_device *pdev)
> 
> I noticed that this function is oddly placed within the macb_config
> structs definitions. Since I am already modifying it, would you like
> me to move it above them to where the fu540 init functions are?

That would be good, thanks!

> 
>>>> +static int init_reset_optional(struct platform_device *pdev)
>>>
>>> It doesn't sound like a good name for this function but I don't have
>>> something better to propose.
>>
>> It's better than zynqmp_init, but yeah...
>>
>>>
>>>>   {
>>>>       struct net_device *dev = platform_get_drvdata(pdev);
>>>>       struct macb *bp = netdev_priv(dev);
>>>>       int ret;
>>>>         if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>>>> -        /* Ensure PS-GTR PHY device used in SGMII mode is ready */
>>>> +        /* Ensure PHY device used in SGMII mode is ready */
>>>>           bp->sgmii_phy = devm_phy_optional_get(&pdev->dev, NULL);
>>>>             if (IS_ERR(bp->sgmii_phy)) {
>>>>               ret = PTR_ERR(bp->sgmii_phy);
>>>>               dev_err_probe(&pdev->dev, ret,
>>>> -                      "failed to get PS-GTR PHY\n");
>>>> +                      "failed to get SGMII PHY\n");
>>>>               return ret;
>>>>           }
>>>>             ret = phy_init(bp->sgmii_phy);
>>>>           if (ret) {
>>>> -            dev_err(&pdev->dev, "failed to init PS-GTR PHY: %d\n",
>>>> +            dev_err(&pdev->dev, "failed to init SGMII PHY: %d\n",
>>>>                   ret);
>>>>               return ret;
>>>>           }
>>>>       }
>>>>   -    /* Fully reset GEM controller at hardware level using zynqmp-reset driver,
>>>> -     * if mapped in device tree.
>>>> +    /* Fully reset controller at hardware level if mapped in device tree
>>>>        */
>>>
>>> The new comment can fit on a single line.
>>>
>>>>       ret = device_reset_optional(&pdev->dev);
>>>>       if (ret) {
>>>> @@ -4737,7 +4736,7 @@ static const struct macb_config zynqmp_config = {
>>>>               MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
>>>>       .dma_burst_length = 16,
>>>>       .clk_init = macb_clk_init,
>>>> -    .init = zynqmp_init,
>>>> +    .init = init_reset_optional,
>>>>       .jumbo_max_len = 10240,
>>>>       .usrio = &macb_default_usrio,
>>>>   };
>>>> @@ -4751,6 +4750,17 @@ static const struct macb_config zynq_config = {
>>>>       .usrio = &macb_default_usrio,
>>>>   };
>>>>   +static const struct macb_config mpfs_config = {
>>>> +    .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>>>> +            MACB_CAPS_JUMBO |
>>>> +            MACB_CAPS_GEM_HAS_PTP,
>>>
>>> Except for zynqmp and default_gem_config the rest of the capabilities for
>>> other SoCs are aligned something like this:
>>>
>>> +    .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>>> +        MACB_CAPS_JUMBO |
>>> +        MACB_CAPS_GEM_HAS_PTP,
>>>
>>> To me it looks better to have you caps aligned this way.
>>
>> Yeah, I picked that b/c I copied straight from the default config.
>> I have no preference, but if you're not a fan of the default...
>