Re: Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Andrew Lunn]

On Fri, Jan 26, 2018 at 07:20:42AM +, Wang, Dongsheng wrote:
> Hi, Timur && Andrew,
> 
> Please correct me if there is any problem with my understanding.
> 
> GPIO is a general property of devices, the property point to
> an entity such as device tree or ACPI table, we also can directly
> implement it in device node.
> 
> For ACPI, there is _DSD that should include GPIO property if we need it.
> No matter which devices implement it, MAC or MDIO also can implement a 
> _DSD.
> We can explicitly define a GPIO property in MDIO, but I think this
> may conflict with the existing definition of ACPI. ACPI guys may not
> agree to do this because there already has _DSD. We just need to use
> _DSD to notify GPIO layer there may have a Device Specific Data for
> this device.
> 
> So far MDIO owns external PHY "reset" as an optional feature and MDIO
> is integrated in MAC, so we need to point the MAC adev to 
> MDIO->dev.fwnode.
> And most importantly this feature does not depend on SoC, this feature
> depends on MotherBoard design.

Hi Wang

We have two different GPIO reset lines within the MDIO/PHY layer.  If
the GPIO is at the MDIO bus node of DT, the reset applies to all PHYs
connected to the bus. This is the code in__mdiobus_register().  If the
GPIO is in the PHY node, the reset applies to just that PHY. This is
the code in mdiobus_register_gpiod() & mdio_device_reset().

These resets are used at different times. The MDIO reset is used once,
before probing the MDIO bus. The PHY reset is used before probing the
individual PHY.

Does your GPIO reset one PHY, or all the PHYs? This determines where
in belongs. Is it the ACPI device which represents the MDIO bus, or
the ACPI device which represents the PHY?

You need to extend the functions i listed above to look in your ACPI
tables to find the _DSD properties which include the GPIO information.

Please work with Marcin Wojtas to achieve this. We will not accept
anything other than a generic solution which works for everybody.

 Andrew

Re: Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Wang, Dongsheng]

Hi, Timur && Andrew,

Please correct me if there is any problem with my understanding.

GPIO is a general property of devices, the property point to
an entity such as device tree or ACPI table, we also can directly
implement it in device node.

For ACPI, there is _DSD that should include GPIO property if we need it.
No matter which devices implement it, MAC or MDIO also can implement a 
_DSD.
We can explicitly define a GPIO property in MDIO, but I think this
may conflict with the existing definition of ACPI. ACPI guys may not
agree to do this because there already has _DSD. We just need to use
_DSD to notify GPIO layer there may have a Device Specific Data for
this device.

So far MDIO owns external PHY "reset" as an optional feature and MDIO
is integrated in MAC, so we need to point the MAC adev to 
MDIO->dev.fwnode.
And most importantly this feature does not depend on SoC, this feature
depends on MotherBoard design.

ACPI "reset-gpios" example:
Documentation/acpi/gpio-properties.txt.

Cheers,
-Dongsheng

On 2018/1/26 0:05:02, "Timur Tabi"  wrote:

>On 01/25/2018 09:59 AM, Andrew Lunn wrote:
>>I expect we will implement something like acpi_mdiobus_register(), and
>>it will take a pointer to an ACPI node. And maybe on top of
>>of_mdiobus_register() and of_mdiobus_register() we will add a
>>device_mdiobus_register().
>
>Makes sense.  If you remember, please CC me on any patches.
>
>>What i'm trying to avoid is drivers ending up with different ACPI
>>bindings. If you don't want to add an ACPI node/property then no
>>problems, just don't expect to be able to use any of the optional
>>features of the MDIO core, like the GPIOs for reset.
>
>Well, if a new binding is created, we will update our ACPI tables and
>drivers use it.  But we may need to keep the legacy code in emac-phy.c
>for backwards compatibility with older firmware.
>
>--
>Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
>Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 09:59 AM, Andrew Lunn wrote:

I expect we will implement something like acpi_mdiobus_register(), and
it will take a pointer to an ACPI node. And maybe on top of
of_mdiobus_register() and of_mdiobus_register() we will add a
device_mdiobus_register().


Makes sense.  If you remember, please CC me on any patches.


What i'm trying to avoid is drivers ending up with different ACPI
bindings. If you don't want to add an ACPI node/property then no
problems, just don't expect to be able to use any of the optional
features of the MDIO core, like the GPIOs for reset.


Well, if a new binding is created, we will update our ACPI tables and 
drivers use it.  But we may need to keep the legacy code in emac-phy.c 
for backwards compatibility with older firmware.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Andrew Lunn]

On Thu, Jan 25, 2018 at 09:40:45AM -0600, Timur Tabi wrote:
> On 01/25/2018 08:15 AM, Andrew Lunn wrote:
> >If i'm reading your patch correctly, you are looking for the MDIO
> >reset in the MAC node. This is wrong. It is an MDIO property, so
> >should be in the MDIO device. Once we have figured out how to
> >represent MDIO busses in ACPI, the reset will be in the MDIO node.
> 
> Just FYI, the MDIO controller in the EMAC is integrated, so I can't see us
> creating a separate Device Tree or ACPI node/property for it. Granted, the
> code in emac-phy.c:emac_phy_config() that registers the MDIO bus is
> convoluted, so maybe there's an opportunity to replace some/all of that code
> with some generic API.  Maybe we need something like acpi_mdiobus_register()
> like we have of_mdiobus_register().

Hi Timur

I expect we will implement something like acpi_mdiobus_register(), and
it will take a pointer to an ACPI node. And maybe on top of
of_mdiobus_register() and of_mdiobus_register() we will add a
device_mdiobus_register().

What i'm trying to avoid is drivers ending up with different ACPI
bindings. If you don't want to add an ACPI node/property then no
problems, just don't expect to be able to use any of the optional
features of the MDIO core, like the GPIOs for reset.

  Andrew

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 08:15 AM, Andrew Lunn wrote:

If i'm reading your patch correctly, you are looking for the MDIO
reset in the MAC node. This is wrong. It is an MDIO property, so
should be in the MDIO device. Once we have figured out how to
represent MDIO busses in ACPI, the reset will be in the MDIO node.


Just FYI, the MDIO controller in the EMAC is integrated, so I can't see 
us creating a separate Device Tree or ACPI node/property for it. 
Granted, the code in emac-phy.c:emac_phy_config() that registers the 
MDIO bus is convoluted, so maybe there's an opportunity to replace 
some/all of that code with some generic API.  Maybe we need something 
like acpi_mdiobus_register() like we have of_mdiobus_register().


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 12:14 AM, Wang Dongsheng wrote:

mdiobus always try to get a GPIO "reset" consumer, based on ACPI
the GPIO should be described in emac-adev _DSD or _CRS.


Are you talking about this:

/* de-assert bus level PHY GPIO reset */
gpiod = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpiod)) {
dev_err(>dev, "mii_bus %s couldn't get reset GPIO\n",
bus->id);
return PTR_ERR(gpiod);

If so, I don't think we support named gpios in ACPI.  Do you have an 
actual test case for your patch?  Our ACPI tables don't specify any 
GPIOs for EMAC.  I wouldn't even know what that should look like.



diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e..69171d5 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -117,6 +117,10 @@ int emac_phy_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
  
  	if (has_acpi_companion(>dev)) {

u32 phy_addr;
+   struct fwnode_handle *fwnode;
+
+   fwnode = acpi_fwnode_handle(ACPI_COMPANION(>dev));
+   mii_bus->dev.fwnode = fwnode;


You don't need fwnode:

mii_bus->dev.fwnode =
acpi_fwnode_handle(ACPI_COMPANION(>dev));



--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Andrew Lunn]

On Wed, Jan 24, 2018 at 10:14:39PM -0800, Wang Dongsheng wrote:
> mdiobus always try to get a GPIO "reset" consumer, based on ACPI
> the GPIO should be described in emac-adev _DSD or _CRS.
> 
> ACPI uses mido common API to register, however mdio->dev->fwnode is not
> pointing to any adev. So the "reset" consumer can never be found.
> 
> OF has done this by using an of_mdiobus_register. The mdiobus get emac
> of_node and go through the of_node to find a GPIO "reset" consumer.
> 
> Not sure, ACPI needs to add the same API for mdio just like OF because
> mdio isn't a real entity in ACPI. So I think there isn't any work in
> ACPI, the mac driver needs to take adev to mdiobus when mido-bus is
> registering.

Hi Wang

People are working on an ACPI description for MDIO. There have been
some discussion about this in the thread "Armada 7k/8k PP2 ACPI
support".

If i'm reading your patch correctly, you are looking for the MDIO
reset in the MAC node. This is wrong. It is an MDIO property, so
should be in the MDIO device. Once we have figured out how to
represent MDIO busses in ACPI, the reset will be in the MDIO node.

I don't recommend you do this. It will mean your device is different
to every other device using ACPI for MDIO. A better approach is to
take part in the discussion about how to represent MDIO busses and
PHYs in ACPI.

 Andrew

[RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Wang Dongsheng]

mdiobus always try to get a GPIO "reset" consumer, based on ACPI
the GPIO should be described in emac-adev _DSD or _CRS.

ACPI uses mido common API to register, however mdio->dev->fwnode is not
pointing to any adev. So the "reset" consumer can never be found.

OF has done this by using an of_mdiobus_register. The mdiobus get emac
of_node and go through the of_node to find a GPIO "reset" consumer.

Not sure, ACPI needs to add the same API for mdio just like OF because
mdio isn't a real entity in ACPI. So I think there isn't any work in
ACPI, the mac driver needs to take adev to mdiobus when mido-bus is
registering.

Signed-off-by: Wang Dongsheng 
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e..69171d5 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -117,6 +117,10 @@ int emac_phy_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
 
if (has_acpi_companion(>dev)) {
u32 phy_addr;
+   struct fwnode_handle *fwnode;
+
+   fwnode = acpi_fwnode_handle(ACPI_COMPANION(>dev));
+   mii_bus->dev.fwnode = fwnode;
 
ret = mdiobus_register(mii_bus);
if (ret) {
-- 
2.7.4