[PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Ruslan Babayev]

Lookup I2C adapter using the "i2c-bus" device property on ACPI based
systems similar to how it's done with DT.

An example DSD describing an SFP on an ACPI based system:

Device (SFP0)
{
    Name (_HID, "PRP0001")
    Name (_DSD, Package ()
    {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () { "compatible", "sff,sfp" },
            Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
        },
    })
}

Signed-off-by: Ruslan Babayev <ruslan@babayev.com>
Cc: xe-linux-external@cisco.com
---
 drivers/net/phy/sfp.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index d4635c2178d1..7a6c8df8899b 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/acpi.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
 #include <linux/rtnetlink.h>
@@ -1783,6 +1784,7 @@ static int sfp_probe(struct platform_device *pdev)
 {
 	const struct sff_data *sff;
 	struct sfp *sfp;
+	struct i2c_adapter *i2c = NULL;
 	bool poll = false;
 	int irq, err, i;
 
@@ -1801,7 +1803,6 @@ static int sfp_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node) {
 		struct device_node *node = pdev->dev.of_node;
 		const struct of_device_id *id;
-		struct i2c_adapter *i2c;
 		struct device_node *np;
 
 		id = of_match_node(sfp_of_match, node);
@@ -1818,14 +1819,30 @@ static int sfp_probe(struct platform_device *pdev)
 
 		i2c = of_find_i2c_adapter_by_node(np);
 		of_node_put(np);
-		if (!i2c)
-			return -EPROBE_DEFER;
-
-		err = sfp_i2c_configure(sfp, i2c);
-		if (err < 0) {
-			i2c_put_adapter(i2c);
-			return err;
+	} else if (ACPI_COMPANION(&pdev->dev)) {
+		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+		struct fwnode_handle *fw = acpi_fwnode_handle(adev);
+		struct fwnode_reference_args args;
+		struct acpi_handle *acpi_handle;
+		int ret;
+
+		ret = acpi_node_get_property_reference(fw, "i2c-bus", 0, &args);
+		if (ACPI_FAILURE(ret) || !is_acpi_device_node(args.fwnode)) {
+			dev_err(&pdev->dev, "missing 'i2c-bus' property\n");
+			return -ENODEV;
 		}
+
+		acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode);
+		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
+	}
+
+	if (!i2c)
+		return -EPROBE_DEFER;
+
+	err = sfp_i2c_configure(sfp, i2c);
+	if (err < 0) {
+		i2c_put_adapter(i2c);
+		return err;
 	}
 
 	for (i = 0; i < GPIO_MAX; i++)
-- 
2.17.1

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Mika Westerberg]

On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> systems similar to how it's done with DT.
> 
> An example DSD describing an SFP on an ACPI based system:
> 
> Device (SFP0)
> {
>     Name (_HID, "PRP0001")
>     Name (_DSD, Package ()
>     {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () { "compatible", "sff,sfp" },
>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },

Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
using that?

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Andrew Lunn]

On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> systems similar to how it's done with DT.
> 
> An example DSD describing an SFP on an ACPI based system:
> 
> Device (SFP0)
> {
>     Name (_HID, "PRP0001")
>     Name (_DSD, Package ()
>     {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () { "compatible", "sff,sfp" },
>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
>         },
>     })
> }

Hi Ruslan

So this gives you the I2C bus. But what about the 6 GPIOs? And the
maximum power property? You are defining the ACPI interface which from
now on everybody has to follow. So it would be good to make it
complete. ACPI also seems to be poorly documented. There does not
appear to be anything like Documentation/devicetree. So having one
patch, with a good commit message, which implements everything makes
it easier for those that follow.

This appears to be enough to get a very minimal SFP instantiated. But
then what?  How are you using it? How do you instantiate a Phylink
instance for the MAC? How do you link the SFP to the Phylink?

Before accepting this patch, i would like to know more about the
complete solution.

Thanks
	Andrew

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Ruslan Babayev]

Mika Westerberg writes:

> On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
>> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
>> systems similar to how it's done with DT.
>> 
>> An example DSD describing an SFP on an ACPI based system:
>> 
>> Device (SFP0)
>> {
>>     Name (_HID, "PRP0001")
>>     Name (_DSD, Package ()
>>     {
>>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>         Package () {
>>             Package () { "compatible", "sff,sfp" },
>>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
>
> Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
> using that?

I am not an ACPI expert, but my understanding is I2cSerialBusV2() is
used for slave connections. I am trying to reference an I2C controller
here.

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Ruslan Babayev]

Andrew Lunn writes:

> On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
>> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
>> systems similar to how it's done with DT.
>>
>> An example DSD describing an SFP on an ACPI based system:
>>
>> Device (SFP0)
>> {
>>     Name (_HID, "PRP0001")
>>     Name (_DSD, Package ()
>>     {
>>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>         Package () {
>>             Package () { "compatible", "sff,sfp" },
>>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
>>         },
>>     })
>> }
>
> Hi Ruslan
>
> So this gives you the I2C bus. But what about the 6 GPIOs? And the
> maximum power property? You are defining the ACPI interface which from
> now on everybody has to follow. So it would be good to make it
> complete. ACPI also seems to be poorly documented. There does not
> appear to be anything like Documentation/devicetree. So having one
> patch, with a good commit message, which implements everything makes
> it easier for those that follow.
>
Hi Andrew,

I had the GPIOs and the "maximum-power" property in my ACPI snippet initially,
but then decided to take it out thinking it was not relevant for the
current patch. I can add the missing pieces back in V2.
This is what it would like:

Device (SFP0)
{
    Name (_HID, "PRP0001")
    Name (_CRS, ResourceTemplate()
    {
        GpioIo(Exclusive, PullDefault, 0, 0, IoRestrictionNone,
               "\\_SB.PCI0.RP01.GPIO", 0, ResourceConsumer)
            { 0, 1, 2, 3, 4 }
    })
    Name (_DSD, Package ()
    {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () { "compatible", "sff,sfp" },
            Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
            Package () { "maximum-power-milliwatt", 1000 },
            Package () { "tx-disable-gpios", Package () { ^SFP0, 0, 0, 1} },
            Package () { "reset-gpio",       Package () { ^SFP0, 0, 1, 1} },
            Package () { "mod-def0-gpios",   Package () { ^SFP0, 0, 2, 1} },
            Package () { "tx-fault-gpios",   Package () { ^SFP0, 0, 3, 0} },
            Package () { "los-gpios",        Package () { ^SFP0, 0, 4, 1} },
        },
    })
}


> This appears to be enough to get a very minimal SFP instantiated. But
> then what?  How are you using it? How do you instantiate a Phylink
> instance for the MAC? How do you link the SFP to the Phylink?
>
> Before accepting this patch, i would like to know more about the
> complete solution.
>
> Thanks
> 	Andrew

I haven't gotten that far yet, but for the Phylink I was thinking something along the
lines of:

Device (PHY0)
{
    Name (_HID, "PRP0001")
    Name (_DSD, Package ()
    {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () { "compatible", "ethernet-phy-ieee802.3-c45" },
            Package () { "sfp", \_SB.PCI0.RP01.SFP0 },
        },
    })
}

Phylink is already using the fwnode_property_get_reference_args(fwnode,
"sfp", ...), so it should work with ACPI.

I don't have a complete solution working yet. With these patches
I was hoping to get some early feedback.

Thanks,
Ruslan

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Andrew Lunn]

> Hi Andrew,
> 
> I had the GPIOs and the "maximum-power" property in my ACPI snippet initially,
> but then decided to take it out thinking it was not relevant for the
> current patch. I can add the missing pieces back in V2.
> This is what it would like:
> 
> Device (SFP0)
> {
>     Name (_HID, "PRP0001")
>     Name (_CRS, ResourceTemplate()
>     {
>         GpioIo(Exclusive, PullDefault, 0, 0, IoRestrictionNone,
>                "\\_SB.PCI0.RP01.GPIO", 0, ResourceConsumer)
>             { 0, 1, 2, 3, 4 }
>     })
>     Name (_DSD, Package ()
>     {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () { "compatible", "sff,sfp" },
>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
>             Package () { "maximum-power-milliwatt", 1000 },
>             Package () { "tx-disable-gpios", Package () { ^SFP0, 0, 0, 1} },
>             Package () { "reset-gpio",       Package () { ^SFP0, 0, 1, 1} },
>             Package () { "mod-def0-gpios",   Package () { ^SFP0, 0, 2, 1} },
>             Package () { "tx-fault-gpios",   Package () { ^SFP0, 0, 3, 0} },
>             Package () { "los-gpios",        Package () { ^SFP0, 0, 4, 1} },
>         },
>     })
> }

Hi Ruslan

I know approximately 0 about ACPI. But that at least lists all the
properties we expect. Thanks.

> > Before accepting this patch, i would like to know more about the
> > complete solution.
> 
> I haven't gotten that far yet, but for the Phylink I was thinking something along the
> lines of:
> 
> Device (PHY0)
> {
>     Name (_HID, "PRP0001")
>     Name (_DSD, Package ()
>     {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () { "compatible", "ethernet-phy-ieee802.3-c45" },
>             Package () { "sfp", \_SB.PCI0.RP01.SFP0 },
>         },
>     })
> }

You probably also need managed = "in-band-status" and
phy-mode = "sgmii";

armada-388-clearfog.dtsi is probably the best reference, much of the
development work for Phylink and SFPs was done on that board.

> I don't have a complete solution working yet. With these patches
> I was hoping to get some early feedback.

Please post your patches as "RFC" in the subject line, if you are
wanting early feedback.

Thanks
	Andrew

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Andrew Lunn]

On Mon, May 06, 2019 at 07:59:51AM +0300, Mika Westerberg wrote:
> On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> > Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> > systems similar to how it's done with DT.
> > 
> > An example DSD describing an SFP on an ACPI based system:
> > 
> > Device (SFP0)
> > {
> >     Name (_HID, "PRP0001")
> >     Name (_DSD, Package ()
> >     {
> >         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >         Package () {
> >             Package () { "compatible", "sff,sfp" },
> >             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
> 
> Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
> using that?

Hi Mika

Does that reference the bus as a whole, or a device on the bus?

The SFP subsystem needs a reference to the bus as a whole. There can
be an at24 like EEPROM at addresses 0x50 and 0x51. There can be an
MDIO bus encapsulated in i2c at addresses 0x40-0x5f, excluding 0x50
and 0x51. What actually is there depends on the SFP module which is
plugged into the SFP cage, and it is all hot-plugable.

	Andrew

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Mika Westerberg]

On Mon, May 06, 2019 at 11:14:15AM -0700, Ruslan Babayev wrote:
> 
> Mika Westerberg writes:
> 
> > On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> >> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> >> systems similar to how it's done with DT.
> >> 
> >> An example DSD describing an SFP on an ACPI based system:
> >> 
> >> Device (SFP0)
> >> {
> >>     Name (_HID, "PRP0001")
> >>     Name (_DSD, Package ()
> >>     {
> >>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> >>         Package () {
> >>             Package () { "compatible", "sff,sfp" },
> >>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
> >
> > Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
> > using that?
> 
> I am not an ACPI expert, but my understanding is I2cSerialBusV2() is
> used for slave connections. I am trying to reference an I2C controller
> here.

Ah, the device itself is not sitting on an I2C bus? In that case I
agree, I2CSerialBusV2() is not correct here.

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Mika Westerberg]

On Mon, May 06, 2019 at 09:40:19PM +0200, Andrew Lunn wrote:
> On Mon, May 06, 2019 at 07:59:51AM +0300, Mika Westerberg wrote:
> > On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> > > Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> > > systems similar to how it's done with DT.
> > > 
> > > An example DSD describing an SFP on an ACPI based system:
> > > 
> > > Device (SFP0)
> > > {
> > >     Name (_HID, "PRP0001")
> > >     Name (_DSD, Package ()
> > >     {
> > >         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >         Package () {
> > >             Package () { "compatible", "sff,sfp" },
> > >             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
> > 
> > Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
> > using that?
> 
> Hi Mika
> 
> Does that reference the bus as a whole, or a device on the bus?

It references a single device on the bus.

> The SFP subsystem needs a reference to the bus as a whole. There can
> be an at24 like EEPROM at addresses 0x50 and 0x51. There can be an
> MDIO bus encapsulated in i2c at addresses 0x40-0x5f, excluding 0x50
> and 0x51. What actually is there depends on the SFP module which is
> plugged into the SFP cage, and it is all hot-plugable.

Yeah, as I replied on the other email I2CSerialBusV2() cannot really be
used here. I did not realize that the device is referencing the whole bus.

Re: [PATCH net-next 2/2] net: phy: sfp: enable i2c-bus detection on ACPI based systems

[Russell King - ARM Linux admin]

On Tue, May 07, 2019 at 12:29:46PM +0300, Mika Westerberg wrote:
> On Mon, May 06, 2019 at 11:14:15AM -0700, Ruslan Babayev wrote:
> > 
> > Mika Westerberg writes:
> > 
> > > On Sun, May 05, 2019 at 03:05:23PM -0700, Ruslan Babayev wrote:
> > >> Lookup I2C adapter using the "i2c-bus" device property on ACPI based
> > >> systems similar to how it's done with DT.
> > >> 
> > >> An example DSD describing an SFP on an ACPI based system:
> > >> 
> > >> Device (SFP0)
> > >> {
> > >>     Name (_HID, "PRP0001")
> > >>     Name (_DSD, Package ()
> > >>     {
> > >>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > >>         Package () {
> > >>             Package () { "compatible", "sff,sfp" },
> > >>             Package () { "i2c-bus", \_SB.PCI0.RP01.I2C.MUX.CH0 },
> > >
> > > Hmm, ACPI has I2cSerialBusV2() resource for this purpose. Why you are not
> > > using that?
> > 
> > I am not an ACPI expert, but my understanding is I2cSerialBusV2() is
> > used for slave connections. I am trying to reference an I2C controller
> > here.
> 
> Ah, the device itself is not sitting on an I2C bus? In that case I
> agree, I2CSerialBusV2() is not correct here.

There are several possibilities:

- Identifying information in EEPROM-like device at 0x50.
- Optional diagnostics information and measurements at 0x51.
- Optional network PHY at some other address.

Hence, we need access to the bus to be able to parse the EEPROM without
interfering with the AT24 driver that would otherwise bind to it, to
be able to read the diagnostics, and to probe for the network PHY
without needing to have a big table of module vendors/descriptions to
PHY information (and therefore limiting our SFP support to only
"approved" known modules (which, common with big-name switches, pisses
users off and is widely seen as a vendor lock-in measure.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up