On Fri, 8 Jul 2022 16:25:03 +0100
"Russell King (Oracle)" [off-list ref] wrote:

Hi,

On Thu, Jul 07, 2022 at 10:37:53PM +0300, Vladimir Oltean wrote:

quoted

+static int dsa_port_fixup_broken_dt(struct dsa_port *dp)  

As I mentioned, I doubt that Andrew considers this "broken DT" as he's
been promoting this as a standard DSA feature.

quoted

+{
+	struct property_entry fixed_link_props[] = {
+		PROPERTY_ENTRY_BOOL("full-duplex"),
+		PROPERTY_ENTRY_U32("speed", 1000), /* TODO determine actual speed */
+		{},
+	};
+	struct property_entry port_props[3] = {};
+	struct fwnode_handle *fixed_link_fwnode;
+	struct fwnode_handle *new_port_fwnode;
+	struct device_node *dn = dp->dn;
+	phy_interface_t mode;
+	int err;
+
+	if (of_parse_phandle(dn, "phy-handle", 0) ||
+	    of_phy_is_fixed_link(dn))
+		/* Nothing broken, nothing to fix.
+		 * TODO: As discussed with Russell, maybe phylink could provide
+		 * a more comprehensive helper to determine what constitutes a
+		 * valid fwnode binding than this guerilla kludge.
+		 */
+		return 0;  

I think this is sufficient. Yes, phylink accepts "phy" and "phy-device"
because it has to for compatibility with other drivers, but the binding
document for DSA quite clearly states that "phy-handle" is what DSA
accepts, so DT in the kernel will be validated against the yaml file
and enforce correctness here.

We do need to check for "sfp" being present as well.

quoted

+
+	err = of_get_phy_mode(dn, &mode);
+	if (err)
+		/* TODO this may be missing too, ask the driver for the
+		 * max-speed interface mode for this port
+		 */
+		mode = PHY_INTERFACE_MODE_NA;  

I think it would be easier to omit the phy-mode property in the swnode
if it isn't present in DT, because then we can handle that in
dsa_port_phylink_create() as I've done in my patch series via the
ds->ops->phylink_get_caps() method.

quoted

+
+	port_props[0] = PROPERTY_ENTRY_U32("reg", dp->index);  

You said in one of your other replies that this node we're constructing
is only for phylink, do we need the "reg" property? phylink doesn't care
about it.

We don't. Vladimir wrote: "We don't even need the "reg" u32 property, I
just added that for no reason (I wasn't completely sure what the API
offers, then I didn't remove it)."

Marek