RE: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus

[Calvin Johnson (OSS)]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus
> 
> On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@xxxxxxx>
> wrote:
> >
> > From: Calvin Johnson <calvin.johnson@xxxxxxxxxxx>
> >
> > Add ACPI support for MDIO bus registration while maintaining the
> > existing DT support.
> 
> ...
> 
> > -       ret = of_address_to_resource(np, 0, &res);
> > -       if (ret) {
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res) {
> >                 dev_err(&pdev->dev, "could not obtain address\n");
> > -               return ret;
> > +               return -ENODEV;
> >         }
> 
> ...
> 
> > -       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long
> long)res.start);
> > +       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx",
> > +                (unsigned long long)res->start);
> 
> Why this has been touched?

Without this change, I get:
---------------------------------------------------------
drivers/net/ethernet/freescale/xgmac_mdio.c: In function 'xgmac_mdio_probe':
drivers/net/ethernet/freescale/xgmac_mdio.c:269:27: error: request for member 'start' in something not a structure or union
    (unsigned long long)res.start);
                           ^
scripts/Makefile.build:265: recipe for target 'drivers/net/ethernet/freescale/xgmac_mdio.o' failed
make[4]: *** [drivers/net/ethernet/freescale/xgmac_mdio.o] Error 1
---------------------------------------------------------

On checking other files that calls platform_get_resource, I can see that this is the way they refer 'start'.

> 
> ...
> 
> > -       priv->mdio_base = of_iomap(np, 0);
> > +       priv->mdio_base = devm_ioremap_resource(&pdev->dev, res);
> >         if (!priv->mdio_base) {
> 
> Are you sure the check is correct now?
devm_ioremap_resource returns non-NULL error values. So, this doesn't look right. 
I'll work on it for v2.

> >                 ret = -ENOMEM;
> >                 goto err_ioremap;
> >         }
> 
> ...
> 
> >
> > -       priv->is_little_endian = of_property_read_bool(pdev->dev.of_node,
> > -                                                      "little-endian");
> > -
> > -       priv->has_a011043 = of_property_read_bool(pdev->dev.of_node,
> > -                                                 "fsl,erratum-a011043");
> > -
> > -       ret = of_mdiobus_register(bus, np);
> > -       if (ret) {
> > -               dev_err(&pdev->dev, "cannot register MDIO bus\n");
> 
> > +       if (is_of_node(pdev->dev.fwnode)) {
> 
> > +       } else if (is_acpi_node(pdev->dev.fwnode)) {
> 
> Oh, no, this is wrong. Pure approach AFAICS is to use fwnode API or device
> property API.
> 
> And actually what you need to include is rather <linux/property.h>, and not
> acpi.h.

Understood. I had got some issues while using fwnode API to handle DT case due to which
DT/ACPI checks were done and both are handled separately.  Let me see if I can root cause it.

> 
> > +       } else {
> > +               dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n");
> > +               ret = -ENXIO;
> >                 goto err_registration;
> >         }
> 
> > +static const struct acpi_device_id xgmac_mdio_acpi_match[] = {
> > +       {"NXP0006", 0}
> 
> How did you test this on platforms with the same IP and without device  of
> this ACPI ID present?

I didn't test it on any other platforms other than LX2160ARDB.  AFAIU, without
device of this ACPI ID present, the driver won't get probed. 
 
> (Hint: missed terminator)
static const struct acpi_device_id xgmac_mdio_acpi_match[] = {
        { "NXP0006", 0 },
        { }
};
Is this what you meant?

> 
> > +};
> > +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match);
> 
> > +               .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match),
> 
> ACPI_PTR is not needed otherwise you will get a compiler warning.

No compiler warning was observed in both cases.
I can see other drivers using this macro.
drivers/net/ethernet/apm/xgene-v2/main.c:734:              .acpi_match_table = ACPI_PTR(xge_acpi_match),
drivers/net/ethernet/apm/xgene/xgene_enet_main.c:2172:             .acpi_match_table = ACPI_PTR(xgene_enet_acpi_match),
drivers/net/ethernet/hisilicon/hns/hns_enet.c:2445:             .acpi_match_table = ACPI_PTR(hns_enet_acpi_match),
drivers/net/ethernet/hisilicon/hns_mdio.c:566:             .acpi_match_table = ACPI_PTR(hns_mdio_acpi_match),
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5997:           .acpi_match_table = ACPI_PTR(mvpp2_acpi_match),
drivers/net/ethernet/qualcomm/emac/emac.c:766:          .acpi_match_table = ACPI_PTR(emac_acpi_match),
drivers/net/ethernet/smsc/smsc911x.c:2667:              .acpi_match_table = ACPI_PTR(smsc911x_acpi_match),
drivers/net/ethernet/socionext/netsec.c:2187:           .acpi_match_table = ACPI_PTR(netsec_acpi_ids),
drivers/net/phy/mdio-xgene.c:456:               .acpi_match_table = ACPI_PTR(xgene_mdio_acpi_match), 


Thanks
Calvin