Re: [PATCH net-next v3 06/10] net: dsa: Migrate to device_find_class()

On Thu, Jan 19, 2017 at 03:53:15PM +0100, Andrew Lunn wrote:

quoted

quoted

quoted

struct dsa_platform_data {
        /*
         * Reference to a Linux network interface that connects
         * to the root switch chip of the tree.
         */
        struct device   *netdev;

This I think is the oddest thing, why do you need to have the "root
switch" here?  You seem to have dropped the next value in this
structure:
	struct net_device *of_netdev;

We are implementing platform_data for devices which don't support
device tree. When using OF, we don't have any of these issues. We can
go straight to the device.

It is a bit convoluted, but look at
arch/arm/mach-orion5x/rd88f5181l-ge-setup.c. It defines the start of
the dsa_platform_data in that file. It then gets passed through
common.c: orion5x_eth_switch_init() to 
arch/arm/plat-orion/common.c:orion_ge00_switch_init() :

void __init orion_ge00_switch_init(struct dsa_platform_data *d)
{
        int i;

        d->netdev = &orion_ge00.dev;
        for (i = 0; i < d->nr_chips; i++)
                d->chip[i].host_dev = &orion_ge_mvmdio.dev;

        platform_device_register_data(NULL, "dsa", 0, d, sizeof(d));
}

Where we have

static struct platform_device orion_ge00 = {
        .name           = MV643XX_ETH_NAME,
        .id             = 0,
        .num_resources  = 1,
        .resource       = orion_ge00_resources,
        .dev            = {
                .coherent_dma_mask      = DMA_BIT_MASK(32),
        },
};

So this is the platform device for the Ethernet device. We cannot go
to the net_device, because it does not exist until this Ethernet
platform device is instantiated.

Ok, fine, but why isn't the ethernet device a child of this platform
device?  Why is it floating around somewhere else?  You don't see that
happening for other devices.

quoted

Shouldn't you have a bus for RGMII devices?  Is that the real problem
here, you don't have a representation for your RGMII "bus" with a
controller to bundle everything under (like a USB host controller, it
bridges from one bus to another).

RGMII is not a bus. It is a point to point link.

That's fine, but you have multiple devices talking across it, so in the
kernel driver model "naming", it's a bus.  Anything can be a bus, it's
just a way to group together devices of the same type.

Normally, it is
between the Ethernet MAC and the Ethernet PHY. But you can also have
it between an Ethernet MAC and another Ethernet MAC. I'm not sure
describing this is a bus would be practical. It would mean every
ethernet driver also becomes a bus driver!

Instead of a custom platform device driver, yes.  Is that a big deal?
How many do you have?

Every Ethernet PHY would become a bus device. That is a huge change,
for a few legacy boards which are not getting converted to device
tree.

How many different drivers are we talking about here?

quoted

If so, why is eth1 not below f1072004.mdio-mi in the heirachy already?

See the initial diagram above. The switch has two parents. It hangs of
an MDIO bus, and you would like to make RGMII also a bus. Can the
device model handle that? I thought it was a tree, not a graph?

It is a tree, you are correct.  But right now you are picking and
choosing where you want to put that network device.  Why not put it over
on the mdio bus?  Or, like I mentioned, make it a custom bus where you
can properly show this relationship, not just in a generic "let's jump
to the parent and poke around randomly."

Again, it's that last sentance that I object the most to here.  You all
keep ignoring it for some reason...

thanks,

greg k-h