Re: [RFC 00/10] add support for fwnode in i2c mux system and sfp

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Feb 23, 2022 at 04:11:50PM +0100, Clément Léger wrote:
> Le Wed, 23 Feb 2022 16:46:45 +0200,
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> a écrit :

...

> > > Again, the PCI card is independent of the platform, I do not really see
> > > why it should be described using platform description language.  
> > 
> > Yep, and that why it should cope with the platforms it's designed to be used
> > with.
> 
> I don't think PCIe card manufacturer expect them to be used solely on a
> x86 platform with ACPI. So why should I used ACPI to describe it (or DT
> by the way), that's my point.

Because you want it to be used on x86 platforms. On the rest it needs DT
or whatever is required by those platforms (I dunno about Zephyr RTOS, or
VxWorks, or *BSDs).

...

> > > For the moment, I only added fwnode API support as an alternative to
> > > support both OF and software nodes. ACPI is not meant to be handled by
> > > this code "as-is". There is for sure some modifications to be made and
> > > I do not know how clocks are handled when using ACPI. Based on some
> > > thread dating back to 2018 [1], it seem it was even not supported at
> > > all.
> > > 
> > > To be clear, I added the equivalent of the OF support but using
> > > fwnode API because I was interested primarly in using it with software
> > > nodes and still wanted OF support to work. I did not planned it to be
> > > "ACPI compliant" right now since I do not have any knowledge in that
> > > field.  
> > 
> > And here is the problem. We have a few different resource providers
> > (a.k.a. firmware interfaces) which we need to cope with.
> 
> Understood that but does adding fwnode support means it should work
> as-is with both DT and ACPI ? ACPI code is still in place and only the
> of part was converted. But maybe you expect the fwnode prot to be
> conformant with ACPI.

Not only me, I believe Mark also was against using pure DT approach on
ACPI enabled platforms.

...

> > What is going on in this series seems to me quite a violation of the
> > layers and technologies. But I guess you may find a supporter of your
> > ideas (I mean Enrico). However, I'm on the other side and do not like
> > this approach.
> 
> As I said in the cover-letter, this approach is the only one that I did
> found acceptable without being tied to some firmware description. If you
> have another more portable approach, I'm ok with that. But this
> solution should ideally work with pinctrl, gpio, clk, reset, phy, i2c,
> i2c-mux without rewriting half of the code. And also allows to easily
> swap the PCIe card to other slots/computer without having to modify the
> description.

My proposal is to use overlays that card provides with itself.
These are supported mechanisms by Linux kernel.

...

> > > > > static const struct property_entry ddr_clk_props[] = {
> > > > >         PROPERTY_ENTRY_U32("clock-frequency", 30000000),    
> > > >   
> > > > >         PROPERTY_ENTRY_U32("#clock-cells", 0),    
> > > > 
> > > > Why this is used?  
> > > 
> > > These props actually describes a fixed-clock properties. When adding
> > > fwnode support to clk framework, it was needed to add the
> > > equivalent of of_xlate() for fwnode (fwnode_xlate()). The number of
> > > cells used to describe a reference is still needed to do the
> > > translation using fwnode_property_get_reference_args() and give the
> > > correct arguments to fwnode_xlate().  
> > 
> > What you described is the programming (overkilled) point. But does hardware
> > needs this? I.o.w. does it make sense in the _hardware_ description?
> 
> This does not makes sense for the hardware of course. It also does not
> makes sense for the hardware to provide that in the device-tree though.

How it can be discovered and enumerated without a hint? And under hint
we may understand, in particular, the overlay blob.

> I actually think this should be only provided by the drivers but it
> might be difficult to parse the descriptions then (either DT or
> software_node), at least that's how it works right now.

-- 
With Best Regards,
Andy Shevchenko