| Message ID | 20190505220524.37266-3-ruslan@babayev.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/2] i2c: acpi: export i2c_acpi_find_adapter_by_handle | expand |
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?
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
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.
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
> 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
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
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.
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.
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.)
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++)
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(-)