Hi Russell, Andy,
On Mon, Jul 18, 2022 at 09:38:42PM +0100, Russell King (Oracle) wrote:
> On Mon, Jul 18, 2022 at 11:07:30PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 18, 2022 at 08:11:40PM +0100, Russell King (Oracle) wrote:
> > > Good point - I guess we at least need to attach the swnode parent to the
> > > device so its path is unique, because right now that isn't the case. I'm
> > > guessing that:
> > >
> > > new_port_fwnode = fwnode_create_software_node(port_props, NULL);
> > >
> > > will create something at the root of the swnode tree, and then:
> > >
> > > fixed_link_fwnode = fwnode_create_named_software_node(fixed_link_props,
> > > new_port_fwnode,
> > > "fixed-link");
> > >
> > > will create a node with a fixed name. I guess it in part depends what
> > > pathname the first node gets (which we don't specify.) I'm not familiar
> > > with the swnode code to know what happens with the naming for the first
> > > node.
> >
> > First node's name will be unique which is guaranteed by IDA framework. If we
> > have already 2B nodes, then yes, it would be problematic (but 2^31 ought to be
> > enough :-).
> >
> > > However, it seems sensible to me to attach the first node to the device
> > > node, thus giving it a unique fwnode path. Does that solve the problem
> > > in swnode land?
> >
> > Yes, but in the driver you will have that as child of the device, analogue in DT
> >
> > my_root_node { // equal the level of device node you attach it to
> > fixed-link {
> > }
> > }
> >
> > (Sorry, I don't know the DT syntax by heart, but I hope you got the idea.)
>
> Yes, that looks about right.
>
> What we're attempting to do here is create the swnode equivalent of this
> DT description:
>
> some_node {
> phy-mode = "foo";
>
> fixed-link {
> speed = X;
> full-duplex;
> };
> };
>
> and the some_node fwnode handle gets passed into phylink for it to
> parse - we never attach it to the firmware tree itself. Once phylink
> has parsed it, we destroy the swnode tree since it's no longer useful.
>
> This would get used in this situation as an example:
>
> switch@4 {
> compatible = "marvell,mv88e6085";
>
> ports {
> port@0 {
> reg = <0>;
> phy-mode = "internal";
> phy-handle = <&sw_phy_0>;
> };
> ...
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <ð1>;
> };
> };
> };
>
> The DSA driver knows the capabilities of the chip, so it knows what the
> fastest "phy-mode" and speed would be, and whether full or half duplex
> are supported.
>
> We need to get this information into phylink some how, and my initial
> approach was to add a new function to phylink to achieve that.
>
> We would normally have passed the "port@5" node to phylink, just as we
> pass the "port@0" node. However, because the "port@5" operates as a
> fixed-link as determined by the hardware/driver, we need some way to
> get that into phylink.
>
> So, Vladimir's approach is to create a swnode tree that reflects the
> DT layout, and rather than passing the "port@5" as a fwnode to phylink,
> we instead pass that "some_node" swnode instead. Phylink then uses
> normal fwnode APIs to parse the swnode tree it's been given, resulting
> in it picking up the fixed-link specification as if it had been in the
> original DT.
>
> We don't augment the existing firmware tree for "port@5", we are
> effectively creating a small sub-tree and using it as a subsitute
> description.
>
> I hope that clarifies what is going on here and why.
Basically what your patch is doing is adding a helper function that creates
an fwnode with a given name. This functionality was there previously through
software_node_register_nodes(), with node allocation responsibility residing
on the caller. It's used e.g. here:
drivers/media/pci/intel/ipu3/cio2-bridge.c .
The larger question is perhaps when can you safely remove software nodes.
And which of these two APIs would be preferred. I haven't checked how many
users each has. There's no refcounting nor locking for software nodes, so
once made visible to the rest of the kernel, they're always expected to be
there, unchanged, or at least it needs to be known when they can be removed.
--
Kind regards,
Sakari Ailus
