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

[Conor.Dooley]

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?

>>> +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...