In D58812#1379542, @freebsd_sysctl.cz wrote:

• Added simplebus_driver as a base class of all mt_clk based drivers.

Why to all? What about all the other objections?

• Added simplebus_driver as a base class of all mt_clk based drivers.
• Used simplebus_attach_impl(dev, SB_FLAG_NO_RANGES, node) instead of open-coding simplebus_init() and the child enumeration loop. None of the clock controller nodes have a "ranges" property.
• Droped the embedded struct simplebus_softc from struct mt_clk_softc

Reworked along the lines @bz asked for, and split into a stack so each piece can
be read on its own. This revision is now only the consumer -- the SIOCGI2C
handler -- and the tunables are gone.

@dsl @adrian — I took D58258#1368055 literally and built MACLINK, then ran it on
an LX2160A (SolidRun CEX7, UEFI/ACPI; dpni0 = dpmac.17 RGMII, dpni1 = dpmac.8 and
dpni2 = dpmac.9, both 10G SFP+ with modules in).

In D58258#1367937, @dsl wrote:

In D58258#1367599, @adrian wrote:

In D58258#1367549, @dsl wrote:

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

What do you mean by "maclink" ? They said they used AI assistance in developing it, but it seems mostly simple enough:

• sfp is an i2c device on a bus;
• the i2c bus doesn't HAVE to be hooked up to the MAC in any way; it in theory could be hanging off of some other i2c controller in the system;
• there's information about where said bus is linked to in FDT;
• some simple shenanigans are required to be able to talk to it and fetch configuration parameters.

What doesn't quite jive with your assumptions of stuff?

MACLINK is supposed to be an abstraction which hides possibly complex topology of the devices and their interconnections which constitute a multi-gigabit link between the MAC (which is a part of a SoC usually) and a PHY/SFP+ on a PCB. This is the best explanation I've to date: https://github.com/mcusim/freebsd-src/blob/dpaa2/sys/dev/maclink/maclink.c#L31. All of the existing MACLINK bits live in https://github.com/mcusim/freebsd-src/tree/dpaa2/sys/dev/maclink, but haven't been tested yet.

Personally, I'd like a proper abstraction to be introduced first with a clear understanding how a maclink_bus can be attached by the network interface drivers and introduce new (specific?) maclink adapters which will be incapsulating all of the complex logic to discover PHYs, SFF/SFPs, PCSs, etc. and let the maclink bus (and the attaching network interface) know about high-level events, e.g. state changes, link's up/down, etc.

In D58258#1367599, @adrian wrote:

In D58258#1367549, @dsl wrote:

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

What do you mean by "maclink" ? They said they used AI assistance in developing it, but it seems mostly simple enough:

• sfp is an i2c device on a bus;
• the i2c bus doesn't HAVE to be hooked up to the MAC in any way; it in theory could be hanging off of some other i2c controller in the system;
• there's information about where said bus is linked to in FDT;
• some simple shenanigans are required to be able to talk to it and fetch configuration parameters.

What doesn't quite jive with your assumptions of stuff?

Thanks for the accept.

In D58258#1367668, @yarshure_gmail.com wrote:

.. My reading is that you have in mind a MAC-link layer
for dpaa2 along the lines of Linux's phylink: one place that owns link state for a
DPMAC and drives it from whatever is attached - a PHY via MDIO, a fixed-link, or an
SFP cage - so that module presence/LOS, TX_DISABLE and rate selection are handled
there rather than in each consumer. If that is roughly it, please correct the
details and I will work to it.

I believe the ACPI path was never tested on main - or we haven't flipped the defaults yet -- but for sure it'll fail to link modules due to unresolved symbols (in the future).

@dsl - on maclink: I want to make sure I am answering the right objection, because
I cannot find maclink in the tree (only enum dpaa2_mac_link_type in dpaa2_mac.h),
so I am guessing at its shape. My reading is that you have in mind a MAC-link layer
for dpaa2 along the lines of Linux's phylink: one place that owns link state for a
DPMAC and drives it from whatever is attached - a PHY via MDIO, a fixed-link, or an
SFP cage - so that module presence/LOS, TX_DISABLE and rate selection are handled
there rather than in each consumer. If that is roughly it, please correct the
details and I will work to it.

v3 addresses @adrian's two points:

In D58258#1367549, @dsl wrote:

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

@adrian well, I'm not sure that the contributor actually understands the code. It seems AI/ML generated to me and isn't aligned with the idea of mine about maclink. I'm against the changes.

this looks fine; please just remove teh BSD copyright text itself as the SPDX + your copyright name/email is enough. Then we should be fine for landing it!

looking good, please keep it up!

Done, thanks — new diff uploaded.

I believe you need to add your name / email address in the copyright for the new files you've written. Would you mind doing that please?

Not exactly. The D57176 has nothing to do with simplebus; all of its clock nodes in the DT are leaf nodes.

In D58812#1350916, @mmel wrote:

Sorry, I was too brief. In the past, Martin gave me many versions of the clock code for pre-review, so I have a tendency to respond briefly.
The issue is the audsys/audiosys driver (at least, I didn't check the other clocks). It is not a leaf node, but it has subnodes, so it must implement the simplebus class together with the MT_CLK class. This is impossible in the current situation because both classes have their own softc.
The audsys driver version in review only derives simplebus, which means it cannot work at all. It doesn't have a method for physical access to clock related registers or locking functions (CLKDEV interface).

In D58812#1350916, @mmel wrote:

Sorry, I was too brief. In the past, Martin gave me many versions of the clock code for pre-review, so I have a tendency to respond briefly.
The issue is the audsys/audiosys driver (at least, I didn't check the other clocks). It is not a leaf node, but it has subnodes, so it must implement the simplebus class together with the MT_CLK class. This is impossible in the current situation because both classes have their own softc.
The audsys driver version in review only derives simplebus, which means it cannot work at all. It doesn't have a method for physical access to clock related registers or locking functions (CLKDEV interface).

Sorry, I was too brief. In the past, Martin gave me many versions of the clock code for pre-review, so I have a tendency to respond briefly.
The issue is the audsys/audiosys driver (at least, I didn't check the other clocks). It is not a leaf node, but it has subnodes, so it must implement the simplebus class together with the MT_CLK class. This is impossible in the current situation because both classes have their own softc.
The audsys driver version in review only derives simplebus, which means it cannot work at all. It doesn't have a method for physical access to clock related registers or locking functions (CLKDEV interface).

In D58812#1350834, @mmel wrote:

This code is fundamentally incomplete and unusable in its current state.

• All PLL clocks are crudely faked with fixed clock events instead of being dynamic, without any check.
• The UART silently accepts and programs invalid baud rates without any error reporting.
  • pinctrl implements barely ~10% of the properties defined in the bindings, rendering it effectively useless. -Several clock driver nodes (simplebus being a clear example) lack required functionality and must be rewritten from scratch.

This code is nowhere near ready and demands substantial rework. I'm sorry, but the quality of this AI slope is significantly below my acceptance limit.

This code is fundamentally incomplete and unusable in its current state.

• All PLL clocks are crudely faked with fixed clock events instead of being dynamic, without any check.
• The UART silently accepts and programs invalid baud rates without any error reporting.
  • pinctrl implements barely ~10% of the properties defined in the bindings, rendering it effectively useless. -Several clock driver nodes (simplebus being a clear example) lack required functionality and must be rewritten from scratch.

This code is nowhere near ready and demands substantial rework. I'm sorry, but the quality of this AI slope is significantly below my acceptance limit.

This in general looks fine. I'll go try building it in a day or two. thanks!

Fix the review and remove foreign code

I removed panic() functions

Hi! Can we have a little skeleton manpage in this commit? Here is an example of a minimum implementation that is enormously useful, just 40 lines: https://freshbsd.org/freebsd/src/commit/fd1ee28bd01429aa8c38199d5fc069e8b0b75442

All of these clock drivers share too much common code. Firstly, why don't you subclass these from the base class in mdtk_clk.c? If that's not sufficient, why don't you use a new base class that implements all this glue and subclass it?

I fixed style(9) issues.

I fixed the copyright in the source code.

I fixed 8 space tabs.

This looks fairly good, modulo the style issues.