Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 4 Jan 2021 14:08:31 -0400
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Mark Brown <broonie@kernel.org>
CC:     Greg KH <gregkh@linuxfoundation.org>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        <alsa-devel@alsa-project.org>, Kiran Patil <kiran.patil@intel.com>,
        linux-rdma <linux-rdma@vger.kernel.org>,
        Shiraz Saleem <shiraz.saleem@intel.com>,
        Martin Habets <mhabets@solarflare.com>,
        "Liam Girdwood" <lgirdwood@gmail.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Fred Oh <fred.oh@linux.intel.com>,
        "Dave Ertman" <david.m.ertman@intel.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Netdev <netdev@vger.kernel.org>,
        Leon Romanovsky <leonro@nvidia.com>,
        David Miller <davem@davemloft.net>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Parav Pandit <parav@mellanox.com>, <lee.jones@linaro.org>
Subject: Re: [resend/standalone PATCH v4] Add auxiliary bus support
Message-ID: <20210104180831.GD552508@nvidia.com>
References: <X9xV+8Mujo4dhfU4@kroah.com> <20201218131709.GA5333@sirena.org.uk>
 <20201218140854.GW552508@nvidia.com> <20201218155204.GC5333@sirena.org.uk>
 <20201218162817.GX552508@nvidia.com> <20201218180310.GD5333@sirena.org.uk>
 <20201218184150.GY552508@nvidia.com> <20201218203211.GE5333@sirena.org.uk>
 <20201218205856.GZ552508@nvidia.com> <20201221185140.GD4521@sirena.org.uk>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20201221185140.GD4521@sirena.org.uk>
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from mlx.ziepe.ca (206.223.160.26) by MN2PR04CA0021.namprd04.prod.outlook.com (2603:10b6:208:d4::34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3721.20 via Frontend Transport; Mon, 4 Jan 2021 18:08:33 +0000
Received: from jgg by mlx with local (Exim 4.94)        (envelope-from <jgg@nvidia.com>)        id 1kwUHP-001zC1-Fx; Mon, 04 Jan 2021 14:08:31 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
        t=1609783717; bh=jYWF5Za5RQIKsxGqPAjvR0Bf5RAvnJ0hHtXWaOODqsw=;
        h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:Date:
         From:To:CC:Subject:Message-ID:References:Content-Type:
         Content-Disposition:In-Reply-To:X-ClientProxiedBy:MIME-Version:
         X-MS-Exchange-MessageSentRepresentingType;
        b=LPiLlt1g/HZDtpXcsJ2Bwq06TRD6MJWZ42Cszedhx65KExX4C06vZyyV/gzV/xKgB
         nkcNWrqx4S+4UaK/DGfjRg1Dy/BR854zj97kCs9anlkuBetxNoJwyRangA6gRCxren
         0nOgW6p9MAIW6dVBE9VtDA1/7Z6MFACZ33Ig1QqGgiBH9+OzF6WOWqecFcr9iVtiYT
         0gGWflIvnIwlzZ1+LSB0di1vMpcrNyPYFsC/LuFV/elXWwXwZJ8FtKKZiuut3Vl/1V
         6vO3aX9hPCThx3N3phGl5Xo0QKHW1gK3WaJaoKopWXXBEjWC3XDZ+VnBLS0mWtcG4M
         /Kaan5mZGrKaQ==
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Dec 21, 2020 at 06:51:40PM +0000, Mark Brown wrote:

> > with some kind of inheritance scheme where platform device remained as
> > only instantiated directly in board files, while drivers could bind to
> > OF/DT/ACPI/FPGA/etc device instantiations with minimal duplication &
> > boilerplate.
> 
> Like I said in my previous message that is essentially what we have now.
> It's not worded in quite that way but it's how all the non-enumerable
> buses work.  

I think it is about half way there. We jammed everything into platform
device and platform bus and then had a few api aspects to tell if
which of the subtypes it might be.

That functions sort of like an object model with inheritance, but a
single type and 'is it a XXX' queries is not quite the same thing.

> BTW I did have a bit of a scan through some of the ACPI devices and
> for a good proportion of them it seems fairly clear that they are
> not platform devices at all - they were mostly interacting with ACPI
> firmware functionality rather than hardware, something you can't
> really do with FDT at all.

Right, that is kind of the point. We also have cases where ACPI
devices are just an ioresource list and don't have any special
ACPIness. IIRC DT has a similar issue where there are DT drivers that
just don't work without the OF stuff. Why are they platform drivers?

IMHO the point of the bus type is to tell the driver what API set you
have. If you have a of_device then you have an OF node and can do all
the of operations. Same for PCI/ACPI/etc.

We fake this idea out by being able to convert platform to DT and OF,
but if platform is to be the universal device then why do we have PCI
device and not a 'platform to pci' operator instead? None of this is
consistent.

Regardless of the shortcut to make everything a struct
platform_device, I think it was a mistake to put OF devices on
platform_bus. Those should have remained on some of_bus even if they
are represented by struct platform_device and fiddling in the core
done to make that work OK.

It is much easier to identify what a bus_type is (the unique
collection of APIs) and thus when to create those.

If the bus_type should contain struct platform_device or a unqiue
struct then becomes a different question.

Yes that is very hacky, but it feels less hacky than the platform
bus/device is everything and can be used everwhere idea.

> > The only problem with mfd as far as SOF is concerned was Greg was not
> > happy when he saw PCI stuff in the MFD subsystem.
> 
> This is a huge part of the problem here - there's no clearly articulated
> logic, it's all coming back to these sorts of opinion statements about
> specific cases which aren't really something you can base anything
> on.

I agree with this, IMHO there is no really cohesive explanation for
when to create a bus vs use the "universal bus" (platform) that can
also explain the things platform is already doing.

This feels like a good conference topic someday..

> Personally I'm even struggling to identify a practical problem that
> we're trying to solve here.  Like Alexandre says what would an
> mfd_driver actually buy us?

Well, there is the minor issue of name collision eg
/sys/bus/XX/devices/* must list all devices in the system with no
collisions.

The owner of the bus is supposed to define the stable naming scheme
and all the devices are supposed to follow it. platform doesn't have
this:

$ ls /sys/bus/platform/devices/
 ACPI000C:00	     dell-smbios.0	'Fixed MDIO bus.0'   INT33A1:00         microcode     PNP0C04:00   PNP0C0B:03   PNP0C14:00
 alarmtimer.0.auto   dell-smbios.1	 GHES.0		     intel_rapl_msr.0   MSFT0101:00   PNP0C0B:00   PNP0C0B:04   PNP0C14:01
 coretemp.0	     efi-framebuffer.0	 GHES.1		     iTCO_wdt	        pcspkr	      PNP0C0B:01   PNP0C0C:00   reg-dummy
 dcdbas		     eisa.0		 INT0800:00	     kgdboc	        PNP0103:00    PNP0C0B:02   PNP0C0E:00   serial8250

Why are ACPI names in here? It looks like "because platform drivers
were used to bind to ACPI devices" 

eg INT33A1 is pmc_core_driver so the device was moved from acpi_bus to
platform_bus? How does that make sense??

Why is pmc_core_driver a platform device instead of ACPI? Because some
platforms don't have ACPI and the board file properly creates a
platform device in C code.

> I have some bad news for you about the hardware description problem
> space.  Among other things we have a bunch of platform devices that
> don't have any resources exposed through the resource API but are still
> things like chips on a board, doing some combination of exposing
> resources for other devices (eg, a fixed voltage regulator) and
> consuming things like clocks or GPIOs that don't appear in the resource
> API.

So in these cases how do I use the generic platform bus API to find
the GPIOs, regulators, and so on to connect with?

If drivers take a platform device and immediately covert it to an OF
object and use OF APIs to find those connections then it really
*never* was a platform device in the first place and coding an OF
driver as platform is an abuse.

A decent step would be to accept that 'platform_device' is something
weird and special and split its bus_type. Only devices created
direclty in C code should be on the platform_bus, OF/ACPI/etc should
all be on their own bus_types, even though they all use the same
'struct platform_bus'

Yes, it is super hacky, but at least the bus side would follow the
basic architecture..
 
> that but it is not clear what the upside of doing that would be,
> especially given the amount of upheval it would generate and the
> classification issues that will inevitably result.

Well, I think the upside for existing is very small, but I would like
to see a shared idea about how to answer questions like 'when should I
use a existing device type' and 'when should I make a new device type'.

Jason