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

[<Conor.Dooley@microchip.com>]

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@microchip.com>
>> ---
>>   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)
>> +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...