On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:
> fixed-link PHYs API is used by DSA and a number of drivers
> and was depending on of_. Switch to fwnode_ so to make it
> hardware description agnostic and allow to be used in ACPI
> world as well.
Would it be better to let the fixed-link PHY die, and have everyone use
the more flexible fixed link implementation in phylink?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote:
> On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:
> > fixed-link PHYs API is used by DSA and a number of drivers
> > and was depending on of_. Switch to fwnode_ so to make it
> > hardware description agnostic and allow to be used in ACPI
> > world as well.
>
> Would it be better to let the fixed-link PHY die, and have everyone use
> the more flexible fixed link implementation in phylink?
Would it be even better if DSA had some driver-level prerequisites to
impose for ACPI support - like phylink support rather than adjust_link -
and we would simply branch off to a dsa_shared_port_link_register_acpi()
function, leaving the current dsa_shared_port_link_register_of() alone,
with all its workarounds and hacks? I don't believe that carrying all
that logic over to a common fwnode based API is the proper way forward.
On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote:
> On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote:
> > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:
> > > fixed-link PHYs API is used by DSA and a number of drivers
> > > and was depending on of_. Switch to fwnode_ so to make it
> > > hardware description agnostic and allow to be used in ACPI
> > > world as well.
> >
> > Would it be better to let the fixed-link PHY die, and have everyone use
> > the more flexible fixed link implementation in phylink?
>
> Would it be even better if DSA had some driver-level prerequisites to
> impose for ACPI support - like phylink support rather than adjust_link -
> and we would simply branch off to a dsa_shared_port_link_register_acpi()
> function, leaving the current dsa_shared_port_link_register_of() alone,
> with all its workarounds and hacks? I don't believe that carrying all
> that logic over to a common fwnode based API is the proper way forward.
I agree with you there, here is little attempt to make a clean ACPI
binding. Most of the attempts to add ACPI support seem to try to take
the short cut for just search/replace of_ with fwnode_. And we then
have to push back and say no, and generally it then goes quiet.
Marcin, please approach this from the other end. Please document in
Documentation/firmware-guide/acpi/dsd what a clean binding should look
like, and then try to implement it.
Andrew
Hi Andrew and Vladimir,
pon., 16 sty 2023 o 23:04 Andrew Lunn <[email protected]> napisał(a):
>
> On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote:
> > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote:
> > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:
> > > > fixed-link PHYs API is used by DSA and a number of drivers
> > > > and was depending on of_. Switch to fwnode_ so to make it
> > > > hardware description agnostic and allow to be used in ACPI
> > > > world as well.
> > >
> > > Would it be better to let the fixed-link PHY die, and have everyone use
> > > the more flexible fixed link implementation in phylink?
> >
> > Would it be even better if DSA had some driver-level prerequisites to
> > impose for ACPI support - like phylink support rather than adjust_link -
> > and we would simply branch off to a dsa_shared_port_link_register_acpi()
> > function, leaving the current dsa_shared_port_link_register_of() alone,
> > with all its workarounds and hacks? I don't believe that carrying all
> > that logic over to a common fwnode based API is the proper way forward.
In the past couple of years, a number of subsystems have migrated to a
more generic HW description abstraction (e.g. a big chunk of network,
pinctrl, gpio). ACPI aside, with this patchset one can even try to
describe the switch topology with the swnode (I haven't tried that
though). I fully agree that there should be no 0-day baggage in the
DSA ACPI binding (FYI the more fwnode- version of the
dsa_shared_port_validate_of() cought one issue in the WIP ACPI
description in my setup). On the other hand, I find fwnode_/device_
APIs really helpful for most of the cases - ACPI/OF/swnode differences
can be hidden to a generic layer and the need of maintaining separate
code paths related to the hardware description on the driver/subsystem
level is minimized. An example could be found in v1 of this series,
the last 4 patches in [1] show that it can be done in a simple /
seamless way, especially given the ACPI (fwnode) PHY description in
phylink is already settled and widely used. I am aware at the end of
the day, after final review all this can be more complex.
I expect that the actual DSA ACPI support acceptance will require a
lot of discussions and decisions, on whether certain solutions are
worth migrating from OF world or require spec modification. For now my
goal was to migrate to a more generic HW description API, and so to
allow possible follow-up ACPI-related modifications, and additions to
be extracted and better tracked.
>
> I agree with you there, here is little attempt to make a clean ACPI
> binding. Most of the attempts to add ACPI support seem to try to take
> the short cut for just search/replace of_ with fwnode_. And we then
> have to push back and say no, and generally it then goes quiet.
In most cases, the devices' description is pretty straightforward:
* a node (single or with some children), resources (mem, irqs), mmio
address space, optionally address on a bus and a couple of properties
The DSDT/SSDT tables are very well suited for this. In case of
separate, contained drivers that is also really easy to maintain.
However, I fully understand your concerns and caution before blessing
any change related to subsystem/generic code. Therefore ACPI support
addition was split after v1 (refer to discussion in [1]) and will
require ACPI maintainers' input and guidelines.
>
> Marcin, please approach this from the other end. Please document in
> Documentation/firmware-guide/acpi/dsd what a clean binding should look
> like, and then try to implement it.
>
This is how I initially approached this (original submission: [2]; a
bit updated version, working on top of the current patchset: [3]). We
then agreed that in order to remove a bit hacky mitigation of the
double ACPI scan problem, an MDIOSerialBus _CRS method should be
defined in the ACPI spec, similar to the
I2CSerialBus/SPISerialBus/UARTSerialBus. I am going to submit the
first version for review in the coming days. The DSA purely
ACPI-related changes would be updated and submitted, once the method
is accepted.
Best regards,
Marcin
[1] https://www.spinics.net/lists/netdev/msg827337.html
[2] https://www.spinics.net/lists/netdev/msg827345.html
[3] https://github.com/semihalf-wojtas-marcin/Linux-Kernel/commit/e017e69c0eda18747029bfe0c335df204670ba59
On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote:
> In the past couple of years, a number of subsystems have migrated to a
> more generic HW description abstraction (e.g. a big chunk of network,
> pinctrl, gpio). ACPI aside, with this patchset one can even try to
> describe the switch topology with the swnode (I haven't tried that
> though). I fully agree that there should be no 0-day baggage in the
> DSA ACPI binding (FYI the more fwnode- version of the
> dsa_shared_port_validate_of() cought one issue in the WIP ACPI
> description in my setup). On the other hand, I find fwnode_/device_
> APIs really helpful for most of the cases - ACPI/OF/swnode differences
> can be hidden to a generic layer and the need of maintaining separate
> code paths related to the hardware description on the driver/subsystem
> level is minimized. An example could be found in v1 of this series,
> the last 4 patches in [1] show that it can be done in a simple /
> seamless way, especially given the ACPI (fwnode) PHY description in
> phylink is already settled and widely used. I am aware at the end of
> the day, after final review all this can be more complex.
>
> I expect that the actual DSA ACPI support acceptance will require a
> lot of discussions and decisions, on whether certain solutions are
> worth migrating from OF world or require spec modification. For now my
> goal was to migrate to a more generic HW description API, and so to
> allow possible follow-up ACPI-related modifications, and additions to
> be extracted and better tracked.
I have a simple question.
If you expect that the DSA ACPI bindings will require a lot of
discussions, then how do you know that what you convert to fwnode now
will be needed later, and why do you insist to mechanically convert
everything to fwnode without having that discussion first?
You see the lack of a need to maintain separate code paths between OF
and ACPI as useful. Yet the DSA maintainers don't, and in some cases
this is specifically what they want to avoid. So a mechanical conversion
will end up making absolutely no progress.
Hi,
wt., 17 sty 2023 o 17:34 Vladimir Oltean <[email protected]> napisał(a):
>
> On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote:
> > In the past couple of years, a number of subsystems have migrated to a
> > more generic HW description abstraction (e.g. a big chunk of network,
> > pinctrl, gpio). ACPI aside, with this patchset one can even try to
> > describe the switch topology with the swnode (I haven't tried that
> > though). I fully agree that there should be no 0-day baggage in the
> > DSA ACPI binding (FYI the more fwnode- version of the
> > dsa_shared_port_validate_of() cought one issue in the WIP ACPI
> > description in my setup). On the other hand, I find fwnode_/device_
> > APIs really helpful for most of the cases - ACPI/OF/swnode differences
> > can be hidden to a generic layer and the need of maintaining separate
> > code paths related to the hardware description on the driver/subsystem
> > level is minimized. An example could be found in v1 of this series,
> > the last 4 patches in [1] show that it can be done in a simple /
> > seamless way, especially given the ACPI (fwnode) PHY description in
> > phylink is already settled and widely used. I am aware at the end of
> > the day, after final review all this can be more complex.
> >
> > I expect that the actual DSA ACPI support acceptance will require a
> > lot of discussions and decisions, on whether certain solutions are
> > worth migrating from OF world or require spec modification. For now my
> > goal was to migrate to a more generic HW description API, and so to
> > allow possible follow-up ACPI-related modifications, and additions to
> > be extracted and better tracked.
>
> I have a simple question.
>
> If you expect that the DSA ACPI bindings will require a lot of
> discussions, then how do you know that what you convert to fwnode now
> will be needed later, and why do you insist to mechanically convert
> everything to fwnode without having that discussion first?
>
Ok, let me clarify. From the technical standpoint, I think it is
fairly easy and to a very big extent, we should be able to reuse, what
is already existing - I made it work with a really minimal set of
changes, using a standard nodes' hierarchy and generic methods in the
ACPI tables. As more difficult, I consider getting this solution
accepted by the ACPI and the network subsystem maintainers, also given
the OF quirks/legacy stuff, that apparently needs to be ruled out in
such circumstances. However, I perceive a preparation step, with
migrating to the more generic HW description API in the generic
net/dsa, as a sort of improvement, but I get your point and I will
wait with resubmitting these changes again.
> You see the lack of a need to maintain separate code paths between OF
> and ACPI as useful. Yet the DSA maintainers don't, and in some cases
> this is specifically what they want to avoid. So a mechanical conversion
> will end up making absolutely no progress.
Fair enough. I'll keep it on hold until MDIOSerialBus gets accepted
and repost a bigger, combined patchset with all changes like in the
v1.
Best regards,
Marcin
On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote:
> Hi Andrew and Vladimir,
>
> pon., 16 sty 2023 o 23:04 Andrew Lunn <[email protected]> napisał(a):
> >
> > On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote:
> > > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote:
> > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:
> > > > > fixed-link PHYs API is used by DSA and a number of drivers
> > > > > and was depending on of_. Switch to fwnode_ so to make it
> > > > > hardware description agnostic and allow to be used in ACPI
> > > > > world as well.
> > > >
> > > > Would it be better to let the fixed-link PHY die, and have everyone use
> > > > the more flexible fixed link implementation in phylink?
> > >
> > > Would it be even better if DSA had some driver-level prerequisites to
> > > impose for ACPI support - like phylink support rather than adjust_link -
> > > and we would simply branch off to a dsa_shared_port_link_register_acpi()
> > > function, leaving the current dsa_shared_port_link_register_of() alone,
> > > with all its workarounds and hacks? I don't believe that carrying all
> > > that logic over to a common fwnode based API is the proper way forward.
>
> In the past couple of years, a number of subsystems have migrated to a
> more generic HW description abstraction (e.g. a big chunk of network,
> pinctrl, gpio). ACPI aside, with this patchset one can even try to
> describe the switch topology with the swnode (I haven't tried that
> though). I fully agree that there should be no 0-day baggage in the
> DSA ACPI binding (FYI the more fwnode- version of the
> dsa_shared_port_validate_of() cought one issue in the WIP ACPI
> description in my setup). On the other hand, I find fwnode_/device_
> APIs really helpful for most of the cases - ACPI/OF/swnode differences
> can be hidden to a generic layer and the need of maintaining separate
> code paths related to the hardware description on the driver/subsystem
> level is minimized.
It looks like we are heading towards three different descriptions. OF,
ACPI and swnode. Each is likely to be different. OF has a lot of
history in it, deprecated things etc, which should not appear in the
others. So i see a big ugly block of code for the OF binding, and
hopefully clean and tidy code for ACPI binding and a clean and tidy
bit of code for swmode.,
It would be nice if the results of that parsing could be presented to
the drivers in a uniform way, so the driver itself does not need to
care where the information came from. But to me it is clear that this
uniform layer has no direct access to the databases, since the
database as are different.
Andrew