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