mbox series

[net-next:,v2,0/7] ACPI MDIO support for Marvell controllers

Message ID 20210616190759.2832033-1-mw@semihalf.com
Headers show
Series ACPI MDIO support for Marvell controllers | expand

Message

Marcin Wojtas June 16, 2021, 7:07 p.m. UTC
Hi,

The second version of the patchset addresses all comments received
during v1 review and introduces a couple of new patches that
were requested.

fwnode_mdiobus_register() helper routine was added and it is used
now by 2 drivers (xgmac_mdio and mvmdio). In the latter a clock
handling was significantly simplified by a switch to
a devm_clk_bulk_get_optional().

Last but not least two additional MAC configuration modes ACPI
desctiption were documented ("managed" and "fixed-link") - they
can be processed by the existing fwnode_ phylink helpers and
comply with the standard _DSD properties and hierarchical
data extension. ACPI Maintainers are therefore added to reviewers' list.

More details can be found in the patches and their commit messages.

As before, the feature was verified with ACPI on MacchiatoBin, CN913x-DB
and Armada 8040 DB (fixed-link handling).
Moreover regression tests were performed (old firmware with updated kernel,
new firmware with old kernel and the operation with DT).

The firmware ACPI description is exposed in the public github branch:
https://github.com/semihalf-wojtas-marcin/edk2-platforms/commits/acpi-mdio-r20210613
There is also MacchiatoBin firmware binary available for testing:
https://drive.google.com/file/d/1eigP_aeM4wYQpEaLAlQzs3IN_w1-kQr0

I'm looking forward to the comments or remarks.

Best regards,
Marcin

Changelog:
v1->v2
* 1/7 - new patch
* 2/7 - new patch
* 3/7 - new patch
* 4/7 - new patch
* 5/7 - remove unnecessary `if (has_acpi_companion())` and rebase onto
        the new clock handling
* 6/7 - remove deprecated comment
* 7/7 - no changes

Marcin Wojtas (7):
  Documentation: ACPI: DSD: describe additional MAC configuration
  net: mdiobus: Introduce fwnode_mdbiobus_register()
  net/fsl: switch to fwnode_mdiobus_register
  net: mvmdio: simplify clock handling
  net: mvmdio: add ACPI support
  net: mvpp2: enable using phylink with ACPI
  net: mvpp2: remove unused 'has_phy' field

 drivers/net/ethernet/marvell/mvpp2/mvpp2.h      |  3 -
 include/linux/fwnode_mdio.h                     | 12 ++++
 drivers/net/ethernet/freescale/xgmac_mdio.c     | 11 +--
 drivers/net/ethernet/marvell/mvmdio.c           | 75 ++++++++------------
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 23 ++++--
 drivers/net/mdio/fwnode_mdio.c                  | 22 ++++++
 Documentation/firmware-guide/acpi/dsd/phy.rst   | 55 ++++++++++++++
 7 files changed, 138 insertions(+), 63 deletions(-)

Comments

Andrew Lunn June 16, 2021, 7:35 p.m. UTC | #1
On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:
> Utilize the newly added helper routine
> for registering the MDIO bus via fwnode_
> interface.

You need to add depends on FWNODE_MDIO

    Andrew
Andrew Lunn June 16, 2021, 7:51 p.m. UTC | #2
On Wed, Jun 16, 2021 at 09:07:57PM +0200, Marcin Wojtas wrote:
> This patch introducing ACPI support for the mvmdio driver by adding
> acpi_match_table with two entries:
> 
> * "MRVL0100" for the SMI operation
> * "MRVL0101" for the XSMI mode
> 
> Also clk enabling is skipped, because the tables do not contain
> such data and clock maintenance relies on the firmware.

This last part seems to be no longer true.

     Andrew
Andrew Lunn June 16, 2021, 7:54 p.m. UTC | #3
On Wed, Jun 16, 2021 at 09:07:59PM +0200, Marcin Wojtas wrote:
> The 'has_phy' field from struct mvpp2_port is no longer used.
> Remove it.
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Marcin Wojtas June 16, 2021, 10:37 p.m. UTC | #4
śr., 16 cze 2021 o 21:51 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> On Wed, Jun 16, 2021 at 09:07:57PM +0200, Marcin Wojtas wrote:
> > This patch introducing ACPI support for the mvmdio driver by adding
> > acpi_match_table with two entries:
> >
> > * "MRVL0100" for the SMI operation
> > * "MRVL0101" for the XSMI mode
> >
> > Also clk enabling is skipped, because the tables do not contain
> > such data and clock maintenance relies on the firmware.
>
> This last part seems to be no longer true.
>

Well, it is still relies on firmware (no clocks are passed via ACPI),
but skipping this enablement is hidden in the internals of
devm_clk_bulk_get_optional() and clk_bulk_prepare_enable().

Best regards,
Marcin
Marcin Wojtas June 16, 2021, 11:39 p.m. UTC | #5
śr., 16 cze 2021 o 21:35 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:
> > Utilize the newly added helper routine
> > for registering the MDIO bus via fwnode_
> > interface.
>
> You need to add depends on FWNODE_MDIO
>

Do you mean something like this?

--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig
@@ -68,8 +68,8 @@ config FSL_PQ_MDIO
 config FSL_XGMAC_MDIO
        tristate "Freescale XGMAC MDIO"
        select PHYLIB
-       depends on OF
-       select OF_MDIO
+       depends on ACPI || OF
+       select FWNODE_MDIO
        help

Best regards,
Marcin
Andrew Lunn June 17, 2021, 12:27 p.m. UTC | #6
On Thu, Jun 17, 2021 at 01:39:40AM +0200, Marcin Wojtas wrote:
> śr., 16 cze 2021 o 21:35 Andrew Lunn <andrew@lunn.ch> napisał(a):

> >

> > On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:

> > > Utilize the newly added helper routine

> > > for registering the MDIO bus via fwnode_

> > > interface.

> >

> > You need to add depends on FWNODE_MDIO

> >

> 

> Do you mean something like this?

> 

> --- a/drivers/net/ethernet/freescale/Kconfig

> +++ b/drivers/net/ethernet/freescale/Kconfig

> @@ -68,8 +68,8 @@ config FSL_PQ_MDIO

>  config FSL_XGMAC_MDIO

>         tristate "Freescale XGMAC MDIO"

>         select PHYLIB

> -       depends on OF

> -       select OF_MDIO

> +       depends on ACPI || OF

> +       select FWNODE_MDIO

>         help


You should not need depends on ACPI || OF. FWNODE_MDIO implies
that. And there are no direct calls to of_ functions, so you can drop
the depends on OF.

    Andrew
Marcin Wojtas June 17, 2021, 1:22 p.m. UTC | #7
czw., 17 cze 2021 o 14:28 Andrew Lunn <andrew@lunn.ch> napisał(a):
>

> On Thu, Jun 17, 2021 at 01:39:40AM +0200, Marcin Wojtas wrote:

> > śr., 16 cze 2021 o 21:35 Andrew Lunn <andrew@lunn.ch> napisał(a):

> > >

> > > On Wed, Jun 16, 2021 at 09:07:55PM +0200, Marcin Wojtas wrote:

> > > > Utilize the newly added helper routine

> > > > for registering the MDIO bus via fwnode_

> > > > interface.

> > >

> > > You need to add depends on FWNODE_MDIO

> > >

> >

> > Do you mean something like this?

> >

> > --- a/drivers/net/ethernet/freescale/Kconfig

> > +++ b/drivers/net/ethernet/freescale/Kconfig

> > @@ -68,8 +68,8 @@ config FSL_PQ_MDIO

> >  config FSL_XGMAC_MDIO

> >         tristate "Freescale XGMAC MDIO"

> >         select PHYLIB

> > -       depends on OF

> > -       select OF_MDIO

> > +       depends on ACPI || OF

> > +       select FWNODE_MDIO

> >         help

>

> You should not need depends on ACPI || OF. FWNODE_MDIO implies

> that. And there are no direct calls to of_ functions, so you can drop

> the depends on OF.

>


Ok, I'll leave:
depends on FWNODE_MDIO
only.

Thanks,
Marcin