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