diff mbox series

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

Message ID 20220701065831.632785-3-conor.dooley@microchip.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series PolarFire SoC macb reset support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 25 this patch: 25
netdev/cc_maintainers warning 3 maintainers not CCed: paul.walmsley@sifive.com palmer@dabbelt.com aou@eecs.berkeley.edu
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 25 this patch: 25
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 70 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Conor Dooley July 1, 2022, 6:58 a.m. UTC 
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(-)

Comments

Claudiu Beznea July 1, 2022, 7:47 a.m. UTC | #1 
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.

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

Thank you,
Claudiu Beznea

> +	.dma_burst_length = 16,
> +	.clk_init = macb_clk_init,
> +	.init = init_reset_optional,
> +	.usrio = &macb_default_usrio,
> +	.jumbo_max_len = 10240,
> +};
> +
>  static const struct macb_config sama7g5_gem_config = {
>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_CLK_HW_CHG |
>  		MACB_CAPS_MIIONRGMII,
> @@ -4787,6 +4797,7 @@ static const struct of_device_id macb_dt_ids[] = {
>  	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
>  	{ .compatible = "cdns,zynq-gem", .data = &zynq_config },
>  	{ .compatible = "sifive,fu540-c000-gem", .data = &fu540_c000_config },
> +	{ .compatible = "microchip,mpfs-macb", .data = &mpfs_config },
>  	{ .compatible = "microchip,sama7g5-gem", .data = &sama7g5_gem_config },
>  	{ .compatible = "microchip,sama7g5-emac", .data = &sama7g5_emac_config },
>  	{ /* sentinel */ }
Conor Dooley July 1, 2022, 7:55 a.m. UTC | #2 
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...
Conor Dooley July 1, 2022, 8:06 a.m. UTC | #3 
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@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...
s/default.../default I can make them all match...
Conor Dooley July 2, 2022, 3:34 p.m. UTC | #4 
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@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)

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...
Claudiu Beznea July 4, 2022, 7:07 a.m. UTC | #5 
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@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)
> 
> 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...
>
Conor Dooley July 4, 2022, 7:31 a.m. UTC | #6 
On 04/07/2022 08:07, Claudiu Beznea - M18063 wrote:
> 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@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)
>>
>> 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!

Cool, I'll respin with extra patches for the cleanup.
Thanks for the review Claudiu :)
diff mbox series

Patch

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)
 {
 	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
 	 */
 	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,
+	.dma_burst_length = 16,
+	.clk_init = macb_clk_init,
+	.init = init_reset_optional,
+	.usrio = &macb_default_usrio,
+	.jumbo_max_len = 10240,
+};
+
 static const struct macb_config sama7g5_gem_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_CLK_HW_CHG |
 		MACB_CAPS_MIIONRGMII,
@@ -4787,6 +4797,7 @@  static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config},
 	{ .compatible = "cdns,zynq-gem", .data = &zynq_config },
 	{ .compatible = "sifive,fu540-c000-gem", .data = &fu540_c000_config },
+	{ .compatible = "microchip,mpfs-macb", .data = &mpfs_config },
 	{ .compatible = "microchip,sama7g5-gem", .data = &sama7g5_gem_config },
 	{ .compatible = "microchip,sama7g5-emac", .data = &sama7g5_emac_config },
 	{ /* sentinel */ }