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

[Greg KH]

On Thu, Jan 19, 2017 at 03:53:15PM +0100, Andrew Lunn wrote:
> > > > 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.

> > 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?

> > 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