diff mbox series

[RFC,10/10] net: sfp: add support for fwnode

Message ID 20220221162652.103834-11-clement.leger@bootlin.com (mailing list archive)
State RFC, archived
Headers show
Series add support for fwnode in i2c mux system and sfp | expand

Commit Message

Clément Léger Feb. 21, 2022, 4:26 p.m. UTC
Add support to retrieve a i2c bus in sfp with a fwnode. This support
is using the fwnode API which also works with device-tree and ACPI.
For this purpose, the device-tree and ACPI code handling the i2c
adapter retrieval was factorized with the new code. This also allows
i2c devices using a software_node description to be used by sfp code.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/phy/sfp.c | 44 +++++++++++++------------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

Comments

Russell King (Oracle) Feb. 21, 2022, 4:45 p.m. UTC | #1
On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> is using the fwnode API which also works with device-tree and ACPI.
> For this purpose, the device-tree and ACPI code handling the i2c
> adapter retrieval was factorized with the new code. This also allows
> i2c devices using a software_node description to be used by sfp code.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I think this looks fine.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks.
Andy Shevchenko Feb. 21, 2022, 5:57 p.m. UTC | #2
On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> is using the fwnode API which also works with device-tree and ACPI.
> For this purpose, the device-tree and ACPI code handling the i2c
> adapter retrieval was factorized with the new code. This also allows
> i2c devices using a software_node description to be used by sfp code.

If I'm not mistaken this patch can even go separately right now, since all used
APIs are already available.
Clément Léger Feb. 22, 2022, 1:25 p.m. UTC | #3
Le Mon, 21 Feb 2022 19:57:39 +0200,
Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > is using the fwnode API which also works with device-tree and ACPI.
> > For this purpose, the device-tree and ACPI code handling the i2c
> > adapter retrieval was factorized with the new code. This also allows
> > i2c devices using a software_node description to be used by sfp code.  
> 
> If I'm not mistaken this patch can even go separately right now, since all used
> APIs are already available.
> 

This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
probably be contributed both in a separate series.
Andy Shevchenko Feb. 23, 2022, 11:22 a.m. UTC | #4
On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> Le Mon, 21 Feb 2022 19:57:39 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> 
> > On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > > Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > > is using the fwnode API which also works with device-tree and ACPI.
> > > For this purpose, the device-tree and ACPI code handling the i2c
> > > adapter retrieval was factorized with the new code. This also allows
> > > i2c devices using a software_node description to be used by sfp code.  
> > 
> > If I'm not mistaken this patch can even go separately right now, since all used
> > APIs are already available.
> 
> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> probably be contributed both in a separate series.

I summon Hans into the discussion since I remember he recently refactored
a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
picture approach with this series based on his ACPI experience.
Hans de Goede Feb. 23, 2022, 12:02 p.m. UTC | #5
Hi,

On 2/23/22 12:22, Andy Shevchenko wrote:
> On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
>> Le Mon, 21 Feb 2022 19:57:39 +0200,
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>>
>>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
>>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
>>>> is using the fwnode API which also works with device-tree and ACPI.
>>>> For this purpose, the device-tree and ACPI code handling the i2c
>>>> adapter retrieval was factorized with the new code. This also allows
>>>> i2c devices using a software_node description to be used by sfp code.  
>>>
>>> If I'm not mistaken this patch can even go separately right now, since all used
>>> APIs are already available.
>>
>> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
>> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
>> probably be contributed both in a separate series.
> 
> I summon Hans into the discussion since I remember he recently refactored
> a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> picture approach with this series based on his ACPI experience.

If I understand this series correctly then this is about a PCI-E card
which has an I2C controller on the card and behind that I2C-controller
there are a couple if I2C muxes + I2C clients.

And the goal of the series is to describe those I2C muxes + I2C clients
with software nodes so that the existing I2C enumeration code can be
used (after porting the existing I2C enumeration code from OF functions
to generic fwnode functions).

Did I understand this bit correctly? I believe that a lot of the
discussion here is caused by the initial problem / hw-setup this
series tries to address / support is not described very well ?

Assuming I did understand the above correctly. One alternative would be
to simply manually instantiate the I2C muxes + clients using
i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
will work for the muxes without adding some software_nodes which
brings us back to something like this patch-set.

In general I believe that porting things away from OF specific parsing
to the generic fwnode APIs is a good thing.

Making device_get_match_data() for devices with only a software fwnode
use of_device_id matching feels a bit weird. But it also makes sense
since that requires just adding a compatible string to the software
fwnode properties which is easy and it allows re-uses existing
matching code in the drivers.

I understand various people falling over this weirdness but AFAICT
the alternative would be adding some special swnode_id type + matching
code for devices where the primary fwnode is a software fwnode, which
would just be a whole bunch of extra code ending up with something
similar.

So re-using of_device_id-s for matching of devices where the primary
fwnode is a software fwnode seems like a good idea. *But* this all
needs to be explained in the commit message a lot better. It really
needs to be spelled out that this is:

a) Only for matching devices where the primary fwnode is a software fwnode 

b) Really has nothing to do with of/dt but of_device_id matching is
   used here to avoid having to introduce a new matching mechanism just
   for devices where the primary fwnode is a software fwnode

c) That introducing a new software fwnode matching mechanism would be
   a bad idea since this will require adding new swnode_match tables
   to many drivers, where as re-using of_device_id will make drivers
   which already have an of_match_table just work.

And this should be spelled out in both the commit message as well
as in some documentation / kdoc comments. Because although a useful
trick, reusing the of_match_id-s is also confusing which I believe
is what has led to a lot of the discussion on this patch-set so far.

Note the above all relies on my interpretation of this patch set,
which may be wrong, since as said the patch-set does seem to be
lacking when it comes to documentation / motivation of the patches.

Regards,

Hans
Andrew Lunn Feb. 23, 2022, 12:31 p.m. UTC | #6
> If I understand this series correctly then this is about a PCI-E card
> which has an I2C controller on the card and behind that I2C-controller
> there are a couple if I2C muxes + I2C clients.

They are not i2c clients in the normal sense. The i2c bus connects to
an SFP cage. You can hot plug different sort of network modules into
the cage. So for example fibre optic modules or copper modules. The
modules have an 'at24' like block of memory, which is a mix of EEPROM
and status values. For copper modules, there is an MDIO over I2C
protocol which allows access to the Copper Ethernet PHY inside the
module.

The current device tree binding is that you have a node for the SFP
cage, which includes a phandle to the i2c bus, plus a list of GPIOs
connected to pins on the SFP cage. The SFP driver will then directly
access the memory, maybe instantiate an mdio-over-i2c device if
needed, and control the GPIOs. The Ethernet driver then has an OF node
with a phandle pointing to the SFP device.

The whole design of this is based around the hardware being a
collection of standard parts, i2c bus, i2c mux, gpio controller,
ethernet controller, each with their own driver as part of a linux
subsystem, and then having some glue to put all the parts
together. And at the moment, that glue is DT.

> Note the above all relies on my interpretation of this patch set,
> which may be wrong, since as said the patch-set does seem to be
> lacking when it comes to documentation / motivation of the patches.

I think some examples from DT will help with this.

  Andrew
Russell King (Oracle) Feb. 23, 2022, 12:41 p.m. UTC | #7
On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/23/22 12:22, Andy Shevchenko wrote:
> > On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> >> Le Mon, 21 Feb 2022 19:57:39 +0200,
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> >>
> >>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> >>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> >>>> is using the fwnode API which also works with device-tree and ACPI.
> >>>> For this purpose, the device-tree and ACPI code handling the i2c
> >>>> adapter retrieval was factorized with the new code. This also allows
> >>>> i2c devices using a software_node description to be used by sfp code.  
> >>>
> >>> If I'm not mistaken this patch can even go separately right now, since all used
> >>> APIs are already available.
> >>
> >> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> >> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> >> probably be contributed both in a separate series.
> > 
> > I summon Hans into the discussion since I remember he recently refactored
> > a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> > picture approach with this series based on his ACPI experience.
> 
> If I understand this series correctly then this is about a PCI-E card
> which has an I2C controller on the card and behind that I2C-controller
> there are a couple if I2C muxes + I2C clients.

That is what I gathered as well.

> Assuming I did understand the above correctly. One alternative would be
> to simply manually instantiate the I2C muxes + clients using
> i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
> will work for the muxes without adding some software_nodes which
> brings us back to something like this patch-set.

That assumes that an I2C device is always present, which is not always
the case - there are hot-pluggable devices on I2C buses.

Specifically, this series includes pluggable SFP modules, which fall
into this category of "hot-pluggable I2C devices" - spanning several
bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
as the top 128 bytes is paged and sometimes buggy in terms of access
behaviour. 0x51 contains a bunch of monitoring and other controls
for the module which again can be paged. At 0x56, there may possibly
be some kind of device that translates I2C accesses to MDIO accesses
to access a PHY onboard.

Consequently, the SFP driver and MDIO translation layer wants access to
the I2C bus, rather than a device.

Now, before ARM was converted to DT, we had ways to cope with
non-firmware described setups like this by using platform devices and
platform data. Much of that ended up deprecated, because - hey - DT
is great and more modern and the old way is disgusting and we want to
get rid of it.

However, that approach locks us into describing stuff in firmware,
which is unsuitable when something like this comes along.

I think what we need is both approaches. We need a way for the SFP
driver (which is a platform_driver) to be used _without_ needing
descriptions in firmware. I think we have that for GPIOs, but for an
I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
bus number - we could either pass the i2c_adapter or the adapter
number through platform data to the SFP driver.

Or is there another solution to being able to reuse multi-driver
based infrastructure that we have developed based on DT descriptions
in situations such as an add-in PCI card?
Hans de Goede Feb. 23, 2022, 1:39 p.m. UTC | #8
Hi,

On 2/23/22 13:41, Russell King (Oracle) wrote:
> On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/23/22 12:22, Andy Shevchenko wrote:
>>> On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
>>>> Le Mon, 21 Feb 2022 19:57:39 +0200,
>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
>>>>
>>>>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
>>>>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
>>>>>> is using the fwnode API which also works with device-tree and ACPI.
>>>>>> For this purpose, the device-tree and ACPI code handling the i2c
>>>>>> adapter retrieval was factorized with the new code. This also allows
>>>>>> i2c devices using a software_node description to be used by sfp code.  
>>>>>
>>>>> If I'm not mistaken this patch can even go separately right now, since all used
>>>>> APIs are already available.
>>>>
>>>> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
>>>> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
>>>> probably be contributed both in a separate series.
>>>
>>> I summon Hans into the discussion since I remember he recently refactored
>>> a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
>>> picture approach with this series based on his ACPI experience.
>>
>> If I understand this series correctly then this is about a PCI-E card
>> which has an I2C controller on the card and behind that I2C-controller
>> there are a couple if I2C muxes + I2C clients.
> 
> That is what I gathered as well.
> 
>> Assuming I did understand the above correctly. One alternative would be
>> to simply manually instantiate the I2C muxes + clients using
>> i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
>> will work for the muxes without adding some software_nodes which
>> brings us back to something like this patch-set.
> 
> That assumes that an I2C device is always present, which is not always
> the case - there are hot-pluggable devices on I2C buses.
> 
> Specifically, this series includes pluggable SFP modules, which fall
> into this category of "hot-pluggable I2C devices" - spanning several
> bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
> as the top 128 bytes is paged and sometimes buggy in terms of access
> behaviour. 0x51 contains a bunch of monitoring and other controls
> for the module which again can be paged. At 0x56, there may possibly
> be some kind of device that translates I2C accesses to MDIO accesses
> to access a PHY onboard.
> 
> Consequently, the SFP driver and MDIO translation layer wants access to
> the I2C bus, rather than a device.
> 
> Now, before ARM was converted to DT, we had ways to cope with
> non-firmware described setups like this by using platform devices and
> platform data. Much of that ended up deprecated, because - hey - DT
> is great and more modern and the old way is disgusting and we want to
> get rid of it.
> 
> However, that approach locks us into describing stuff in firmware,
> which is unsuitable when something like this comes along.
> 
> I think what we need is both approaches. We need a way for the SFP
> driver (which is a platform_driver) to be used _without_ needing
> descriptions in firmware. I think we have that for GPIOs, but for an
> I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
> bus number - we could either pass the i2c_adapter or the adapter
> number through platform data to the SFP driver.
> 
> Or is there another solution to being able to reuse multi-driver
> based infrastructure that we have developed based on DT descriptions
> in situations such as an add-in PCI card?

The use of software fwnode-s as proposed in this patch-set is another
way to deal with this. There has been work to abstract ACPI vs
of/dt firmware-nodes into a generic fwnode concept and software-nodes
are a third way to define fwnode-s for "struct device" devices.

Software nodes currently are mainly used as so called secondary
fwnodes which means they can e.g. add extra properties to cover
for the firmware description missing some info (which at least
on ACPI happens more often then we would like).

But a software-node can also be used as the primary fwnode for
a device. So what this patch-set does is move the i2c of/dt
enumeration code over to the fwnode abstraction (1). This allows
the driver for the SPF card to attach a software fwnode to the
device for the i2c-controller which describes the hotplug pins +
any other always present hw in the same way as it would be done
in a devicetree fwnode and then the existing of/dt based SPF
code can be re-used as is.

At least that is my understanding of this patch-set.

Regards,

Hans



1) This should result in no functional changes for existing
devicetree use cases.
Clément Léger Feb. 23, 2022, 2:14 p.m. UTC | #9
Le Wed, 23 Feb 2022 14:39:27 +0100,
Hans de Goede <hdegoede@redhat.com> a écrit :

> > I think what we need is both approaches. We need a way for the SFP
> > driver (which is a platform_driver) to be used _without_ needing
> > descriptions in firmware. I think we have that for GPIOs, but for an
> > I2C bus, We have i2c_get_adapter() for I2C buses, but that needs the
> > bus number - we could either pass the i2c_adapter or the adapter
> > number through platform data to the SFP driver.
> > 
> > Or is there another solution to being able to reuse multi-driver
> > based infrastructure that we have developed based on DT descriptions
> > in situations such as an add-in PCI card?  
> 
> The use of software fwnode-s as proposed in this patch-set is another
> way to deal with this. There has been work to abstract ACPI vs
> of/dt firmware-nodes into a generic fwnode concept and software-nodes
> are a third way to define fwnode-s for "struct device" devices.
> 
> Software nodes currently are mainly used as so called secondary
> fwnodes which means they can e.g. add extra properties to cover
> for the firmware description missing some info (which at least
> on ACPI happens more often then we would like).
> 
> But a software-node can also be used as the primary fwnode for
> a device. So what this patch-set does is move the i2c of/dt
> enumeration code over to the fwnode abstraction (1). This allows
> the driver for the SPF card to attach a software fwnode to the
> device for the i2c-controller which describes the hotplug pins +
> any other always present hw in the same way as it would be done
> in a devicetree fwnode and then the existing of/dt based SPF
> code can be re-used as is.
> 
> At least that is my understanding of this patch-set.
> 
> Regards,
> 
> Hans

Hello Hans, your understanding is totally correct.

This PCIe device actually embeds much more than just a I2C controller.
I should have made that clearer in the cover letter, sorry for the
confusion. The PCIe card is actually using a lan9662x SoC which is
meant to be used as an ethernet switch with 4 ports (2 RJ45 and two
SFPS). In order to use this switch, the following drivers can be reused:
 - lan966x-switch
 - reset-microchip-sparx5
 - lan966x_serdes
 - reset-microchip-lan966x-phy
 - mdio-mscc-miim
 - pinctrl-lan966x
 - atmel-flexcom
 - i2c-at91
 - i2c-mux
 - i2c-mux-pinctrl
 - sfp
 - clk-lan966x
 - lan966x-pci-mfd

All theses drivers are using of_* API and as such only works with a DT
description. One solution that did seems acceptable to me (although
not great)was to use mfd_cells and software_node description
as primary node.

Since I wanted to convert these to be software_node compatible, I had
to modify many subsystems (pinctrl, gpio, i2c, clocks, reset, etc).
This is why I stated in the cover letter that "This series is part of a
larger changeset that touches multiple subsystems". But clearly, it
lacks more context and namely the list of subsystems that needed to be
modify as well as the PCIe card type. I will modify this cover-letter to
add more informations.

So indeed, this series is targetting at using devices which uses a
software_node as a primary node and modifying subsystems to use the
fwnode API in order to make that compatible with these software nodes.
As you said, in order to avoid redefining the match tables and allow
device_get_match_data to work with software_node, the trick was to
reuse the of_table_id

However, I'm not totally happy with that as it seems we are doing what
was done with the "old" platform_data (a bit cleaner maybe since it
allows to reuse the driver with the fwnode API).

As Russell asked, I'm also really interested if someone has a solution
to reuse device-tree description (overlays ?) to describe such
hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
seems a bit complicated on this side. This also requires to load a
device-tree overlay from the filesystem to describe the card, but that
might be something that could be made generic to allow other uses-cases.
Andy Shevchenko Feb. 23, 2022, 2:37 p.m. UTC | #10
On Wed, Feb 23, 2022 at 12:41:47PM +0000, Russell King (Oracle) wrote:
> On Wed, Feb 23, 2022 at 01:02:23PM +0100, Hans de Goede wrote:
> > On 2/23/22 12:22, Andy Shevchenko wrote:
> > > On Tue, Feb 22, 2022 at 02:25:13PM +0100, Clément Léger wrote:
> > >> Le Mon, 21 Feb 2022 19:57:39 +0200,
> > >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :
> > >>> On Mon, Feb 21, 2022 at 05:26:52PM +0100, Clément Léger wrote:
> > >>>> Add support to retrieve a i2c bus in sfp with a fwnode. This support
> > >>>> is using the fwnode API which also works with device-tree and ACPI.
> > >>>> For this purpose, the device-tree and ACPI code handling the i2c
> > >>>> adapter retrieval was factorized with the new code. This also allows
> > >>>> i2c devices using a software_node description to be used by sfp code.  
> > >>>
> > >>> If I'm not mistaken this patch can even go separately right now, since all used
> > >>> APIs are already available.
> > >>
> > >> This patches uses fwnode_find_i2c_adapter_by_node() which is introduced
> > >> by "i2c: fwnode: add fwnode_find_i2c_adapter_by_node()" but they can
> > >> probably be contributed both in a separate series.
> > > 
> > > I summon Hans into the discussion since I remember he recently refactored
> > > a bit I2C (ACPI/fwnode) APIs. Also he might have an idea about entire big
> > > picture approach with this series based on his ACPI experience.
> > 
> > If I understand this series correctly then this is about a PCI-E card
> > which has an I2C controller on the card and behind that I2C-controller
> > there are a couple if I2C muxes + I2C clients.
> 
> That is what I gathered as well.
> 
> > Assuming I did understand the above correctly. One alternative would be
> > to simply manually instantiate the I2C muxes + clients using
> > i2c_new_client_device(). But I'm not sure if i2c_new_client_device()
> > will work for the muxes without adding some software_nodes which
> > brings us back to something like this patch-set.
> 
> That assumes that an I2C device is always present, which is not always
> the case - there are hot-pluggable devices on I2C buses.
> 
> Specifically, this series includes pluggable SFP modules, which fall
> into this category of "hot-pluggable I2C devices" - spanning several
> bus addresses (0x50, 0x51, 0x56). 0x50 is EEPROM like, but not quite
> as the top 128 bytes is paged and sometimes buggy in terms of access
> behaviour. 0x51 contains a bunch of monitoring and other controls
> for the module which again can be paged. At 0x56, there may possibly
> be some kind of device that translates I2C accesses to MDIO accesses
> to access a PHY onboard.
> 
> Consequently, the SFP driver and MDIO translation layer wants access to
> the I2C bus, rather than a device.
> 
> Now, before ARM was converted to DT, we had ways to cope with
> non-firmware described setups like this by using platform devices and
> platform data. Much of that ended up deprecated, because - hey - DT
> is great and more modern and the old way is disgusting and we want to
> get rid of it.
> 
> However, that approach locks us into describing stuff in firmware,
> which is unsuitable when something like this comes along.

Looks like this is a way to reinvent what FPGA should cope with already.
And if I remember correctly the discussions about PCIe FPGAs (from 2016,
though) the idea is that FPGA should have provided a firmware description
with itself. I.o.w. If we are talking about "run-time configurable"
devices they should provide a way to bring their description to the
system.

The currently available way to do it is to get this from EEPROM / ROM
specified on the hardware side in form of DT and ACPI blobs (representing
overlays). Then the only part that is missed (at least for ACPI case) is
to dynamically insert that based on the PCI BDF of the corresponding
PCI bridge.

TL;DR: In my opinion such hardware must bring the description with itself
in case it uses non-enumerable busses, such as SPI, I²C.

I dunno what was the last development in this area for FPGAs cases.
Andrew Lunn Feb. 23, 2022, 3:23 p.m. UTC | #11
> As Russell asked, I'm also really interested if someone has a solution
> to reuse device-tree description (overlays ?) to describe such
> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> seems a bit complicated on this side.

It does work, intel even used it for one of there tiny x86 SoCs. Maybe
it was Newton? If you search around you can find maybe a Linux
Plumbers presentation about DT and x86.

You can probably use a udev rule, triggered by the PCIe device ID to
load the DT overlay.

Do you actually need anything from the host other than PCIe? It sounds
like this card is pretty self contained, so you won't need phandles
pointing to the host i2c bus, or the hosts GPIOs? You only need
phandles to your own i2c bus, your own GPIOs? That will make the
overlay much simpler.

	Andrew
Hans de Goede Feb. 23, 2022, 3:27 p.m. UTC | #12
Hi,

On 2/23/22 16:23, Andrew Lunn wrote:
>> As Russell asked, I'm also really interested if someone has a solution
>> to reuse device-tree description (overlays ?) to describe such
>> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
>> seems a bit complicated on this side.
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton?

IIRC those SoCs did not use standard EFI/ACPI though, but rather some
other special firmware, I think it was SFI ?  This is not so much about
the CPU architecture as it is about the firmware/bootloader <->
OS interface.

Note I'm not saying this can not be done with EFI/ACPI systems, but
I think it has never been tried.

Regards,

Hans
Hans de Goede Feb. 23, 2022, 3:27 p.m. UTC | #13
Hi,

On 2/23/22 16:23, Andrew Lunn wrote:
>> As Russell asked, I'm also really interested if someone has a solution
>> to reuse device-tree description (overlays ?) to describe such
>> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
>> seems a bit complicated on this side.
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton?

IIRC those SoCs did not use standard EFI/ACPI though, but rather some
other special firmware, I think it was SFI ?  This is not so much about
the CPU architecture as it is about the firmware/bootloader <->
OS interface.

Note I'm not saying this can not be done with EFI/ACPI systems, but
I think it has never been tried.

Regards,

Hans
Andy Shevchenko Feb. 23, 2022, 3:36 p.m. UTC | #14
On Wed, Feb 23, 2022 at 04:27:33PM +0100, Hans de Goede wrote:
> On 2/23/22 16:23, Andrew Lunn wrote:
> >> As Russell asked, I'm also really interested if someone has a solution
> >> to reuse device-tree description (overlays ?) to describe such
> >> hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> >> seems a bit complicated on this side.
> > 
> > It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> > it was Newton?
> 
> IIRC those SoCs did not use standard EFI/ACPI though, but rather some
> other special firmware, I think it was SFI ?  This is not so much about
> the CPU architecture as it is about the firmware/bootloader <->
> OS interface.

I think Andrew refers to Intel SoCs that are using OF. Those so far are
CE4xxx and SoFIA SoCs.
Clément Léger Feb. 23, 2022, 3:38 p.m. UTC | #15
Le Wed, 23 Feb 2022 16:23:46 +0100,
Andrew Lunn <andrew@lunn.ch> a écrit :

> > As Russell asked, I'm also really interested if someone has a solution
> > to reuse device-tree description (overlays ?) to describe such
> > hardware. However, the fact that CONFIG_OF isn't enabled on x86 config
> > seems a bit complicated on this side.  
> 
> It does work, intel even used it for one of there tiny x86 SoCs. Maybe
> it was Newton? If you search around you can find maybe a Linux
> Plumbers presentation about DT and x86.

Oh yes, I know it works and I tried loading an overlay using CONFIG_OF
on a x86. Currently it does not works and generate a oops due to the
fact that the lack of "/" node is not handled and that the error path
has probably not been thoroughly tested. Adress remapping for PCI and
lack of PCI bus description might also be a bit cumbersome but maybe
this is the way to go.

I was saying a "bit complicated" as it is not often used and it would
require enabling CONFIG_OF to support this feature as well as allowing
loading overlays without any root node. But this is probably something
that is also achievable.

> 
> You can probably use a udev rule, triggered by the PCIe device ID to
> load the DT overlay.

Or maybe load the overlay from the PCIe driver ? This would also allow
to introduce some remapping of addresses (see below) inside the driver
maybe.

> 
> Do you actually need anything from the host other than PCIe? It sounds
> like this card is pretty self contained, so you won't need phandles
> pointing to the host i2c bus, or the hosts GPIOs? You only need
> phandles to your own i2c bus, your own GPIOs? That will make the
> overlay much simpler.

Yes, the device is almost self contained, only the IRQ needs to be
chained with the MSI.

> 
> 	Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 4720b24ca51b..9d9e3d209408 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2499,43 +2499,25 @@  static int sfp_probe(struct platform_device *pdev)
 		return err;
 
 	sff = sfp->type = &sfp_data;
+	if (dev_fwnode(&pdev->dev)) {
+		struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
+		struct fwnode_handle *np;
 
-	if (pdev->dev.of_node) {
-		struct device_node *node = pdev->dev.of_node;
-		const struct of_device_id *id;
-		struct device_node *np;
+		if (!is_acpi_device_node(fwnode)) {
+			sff = device_get_match_data(&pdev->dev);
+			if (WARN_ON(!sff))
+				return -EINVAL;
 
-		id = of_match_node(sfp_of_match, node);
-		if (WARN_ON(!id))
-			return -EINVAL;
-
-		sff = sfp->type = id->data;
-
-		np = of_parse_phandle(node, "i2c-bus", 0);
-		if (!np) {
-			dev_err(sfp->dev, "missing 'i2c-bus' property\n");
-			return -ENODEV;
+			sfp->type = sff;
 		}
 
-		i2c = of_find_i2c_adapter_by_node(np);
-		of_node_put(np);
-	} else if (has_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 (ret || !is_acpi_device_node(args.fwnode)) {
-			dev_err(&pdev->dev, "missing 'i2c-bus' property\n");
+		np = fwnode_find_reference(fwnode, "i2c-bus", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Cannot parse i2c-bus\n");
 			return -ENODEV;
 		}
-
-		acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode);
-		i2c = i2c_acpi_find_adapter_by_handle(acpi_handle);
-	} else {
-		return -EINVAL;
+		i2c = fwnode_find_i2c_adapter_by_node(np);
+		fwnode_handle_put(np);
 	}
 
 	if (!i2c)