Message-ID: <20230405194353.pwuk7e6rxnha3uqi@pengutronix.de>
Date:   Wed, 5 Apr 2023 21:43:53 +0200
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Richard Cochran <richardcochran@...il.com>,
        Radu Pirea <radu-nicolae.pirea@....nxp.com>,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Yisen Zhuang <yisen.zhuang@...wei.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Iyappan Subramanian <iyappan@...amperecomputing.com>,
        Keyur Chudgar <keyur@...amperecomputing.com>,
        Quan Nguyen <quan@...amperecomputing.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        devicetree@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH 06/12] net: phy: add phy_device_atomic_register helper

On 23-04-05, Andrew Lunn wrote:
> > The current fwnode_mdio.c don't provide the proper helper functions yet.
> > Instead the parsing is spread between fwnode_mdiobus_register_phy() and
> > fwnode_mdiobus_phy_device_register(). Of course these can be extracted
> > and exported but I don't see the benefit. IMHO it just cause jumping
> > around files and since fwnode is a proper firmware abstraction we could
> > use is directly wihin core/lib files.
> 
> No, assuming fwnode is the proper firmware abstraction is wrong. You
> need to be very careful any time you convert of_ to fwnode_ and look
> at the history of every property. Look at the number of deprecated OF
> properties in Documentation/devicetree/bindings. They should never be
> moved to fwnode_ because then you are moving deprecated properties to
> ACPI, which never had them in the first place! 

The handling of deprecated properties is always a pain. Drivers handling
deprecated properties correctly for of_ should handle it correctly for
fwnode_ too. IMHO it would be driver bug if not existing deprecated
properties cause an error.  Of course there will be properties which
need special attention for ACPI case but I don't see a problem for
deprecated properties since those must be handled correctly for of_ case
too.

> You cannot assume DT and ACPI are the same thing, have the same
> binding. And the same is true, in theory, in the opposite direction.
> We don't want the DT properties polluted with ACPI only properties.
> Not that anybody takes ACPI seriously in networking.

My assumption was that ACPI is becoming more closer to OF and the
fwnode/device abstraction is the abstraction to have one driver
interacting correctly with OF and ACPI. As I said above there will be
some corner-cases which need special attention of course :/

Also while answering this mail, I noticed that there are already some
'small' fwnode/device_ helpers within phy_device.c. So why not bundling
everything within phy_device.c?

> > I know and I thought about adding the firmware parsing helpers first but
> > than I went this way. I can split this of course to make the patch
> > smaller.
> 
> Please do. Also, i read your commit message thinking it was a straight
> copy of the code, and hence i did not need to review the code. But in
> fact it is new code. So i need to take a close look at it.
> 
> But what i think is most important for this patchset is the
> justification for not fixing the current API. Why is it broken beyond
> repair?

Currently we have one API which creates/allocates the 'struct
phy_device' and intialize the state which is:
   - phy_device_create()

This function requests a driver based on the phy_id/c45_ids. The ID have
to come from somewhere if autodection is used. For autodetection case
   - get_phy_device()

is called. This function try to access the phy without taken possible
hardware dependencies into account. These dependecies can be reset-lines
(in my case), clocks, supplies, ...

For taking fwnode (and possible dependencies) into account fwnode_mdio.c
was written which provides two helpers:
   - fwnode_mdiobus_register_phy()
   - fwnode_mdiobus_phy_device_register().

The of_mdio.c and of_mdiobus_register_phy() is just a wrapper around
fwnode_mdiobus_register_phy().

fwnode_mdiobus_register_phy():
   1st) calls get_phy_device() in case of autodection or c45. If phy_id
        is provided and !c45 case phy_device_create() is called to get a
	'struct phy_device'
        - The autodection/c45 case try to access the PHYID registers
	  which is not possible, please see above.
   2nd) call fwnode_mdiobus_phy_device_register() or
        phy_device_register() directly.
	- phy_device_register() is the first time we taking the possible
	  hardware reset line into account, which is far to late.

fwnode_mdiobus_phy_device_register():
   - takes a 'struct phy_device' as parameter, again this have to come
     from somewhere.
   - calls phy_device_register() which is taken the possibel hardware
     reset into account, again to late.

Why do I need the autodection? Because PHYs can be changed due to EOL,
cheaper device, ... I don't wanna have a new devicetree/firmware for the
updated product, just let the magic happen :)

Why do I introduce a new API?
  1st) There are working users of get_phy_device() and I don't wanna
       break their systems, so I left this logic untouched. 
  2nd) The fwnode API is replaced by this new one, since it is
       broken (please see above). 
  3rd) IMHO the 'phy request/phy create' handling is far to complex
       therefore I introduced a single API which:
       - intialize all structures and states
       - prepare the device for interaction by using fwnode
       - initialize/detect the device and requests the coorect phy
	 module
       - applies the fixups
       - add the device to the kernel
       - finally return the 'struct phy_device' to the user, so the
	 driver can do $stuff.
  4th) The new 'struct phy_device_config' makes it easier to
       adapt/extend the API.

Thanks a lot for your fast response and feedback :)

Regards,
  Marco

Powered by blists - more mailing lists