All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Jason Gunthorpe <jgg@nvidia.com>
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
Date: Fri, 18 Dec 2020 20:32:11 +0000	[thread overview]
Message-ID: <20201218203211.GE5333@sirena.org.uk> (raw)
In-Reply-To: <20201218184150.GY552508@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 4641 bytes --]

On Fri, Dec 18, 2020 at 02:41:50PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 18, 2020 at 06:03:10PM +0000, Mark Brown wrote:

> > If it's not supposed to use platform devices so I'm assuming that the
> > intention is that it should use aux devices, otherwise presumably it'd
> > be making some new clone of the platform bus but I've not seen anyone
> > suggesting this.

> I wouldn't assume that, I certainly don't want to see all the HW
> related items in platform_device cloned roughly into aux device.

> I've understood the bus type should be basically related to the thing
> that is creating the device. In a clean view platform code creates
> platform devices. DT should create DT devices, ACPI creates ACPI
> devices, PNP does pnp devices, etc

Ah, so we *used* to do that and in fact at least acpi_device still
exists but it was realized that this was causing a lot of effort with
boilerplate - like Lee said board files, ACPI and DT are all just
enumeration methods which have zero effect on the underlying hardware so
you end up having duplication on both the bus and driver side.  Since
this applies to all non-enumerable buses this process gets repeated for
all of them, we wouldn't just have an of_device we'd have of_i2c_device,
of_spi_device, of_1wire_device and so on or have to jump through hoops
to map things into the actual bus type.  See eca3930163ba8884060ce9d9
(of: Merge of_platform_bus_type with platform_bus_type) for part of this
getting unwound.

Fundamentally this is conflating physical bus type and enumeration
method, for enumerable buses they are of course the same (mostly) but
for non-enumerable buses not so much.

> So, I strongly suspect, MFD should create mfd devices on a MFD bus
> type.

Historically people did try to create custom bus types, as I have
pointed out before there was then pushback that these were duplicating
the platform bus so everything uses platform bus.

> Alexandre's point is completely valid, and I think is the main
> challenge here, somehow avoiding duplication.

> If we were to look at it with some OOP viewpoint I'd say the generic
> HW resource related parts should be some shared superclass between
> 'struct device' and 'struct platform/pnp/pci/acpi/mfd/etc_device'.

Right, duplication is the big issue with separate firmware based bus
types particularly as we consider all non-enumerable buses.  I think
what you're looking for here is multiple inheritance, that's potentially
interesting but it's pretty much what we have already TBH.  We have the
physical bus type as a primary type for devices but we also can enquire
if they also have the properties of a DT or ACPI object and then use
those APIs on them.

Consider also FPGAs which can have the same problem Alexandre raised,
there's the parent device for the FPGA and then we can instantiate
bitstreams within that which may expose standard IPs which can also
appear directly within a SoC.

> > > The places I see aux device being used are a terrible fit for the cell
> > > idea. If there are MFD drivers that are awkardly crammed into that
> > > cell description then maybe they should be aux devices?

> > When you say the MFD cell model it's not clear what you mean - I *think*
> > you're referring to the idea of the subdevices getting all the

> I mean using static "struct mfd_cell" arrays to describe things.

OK, but then SOF has been actively pushed into using auxiliary devices
since there is a desire to avoid using mfd_cells on PCI devices rather
than the fact that it wasn't able to use a static array, and of course
you might have devices with a mix of static and dynamic functions, or
functions that can be both static and dynamic.

> > Look at something like wm8994 for example - the subdevices just know
> > which addresses in the device I2C/SPI regmap to work with but some of
> > them have interrupts passed through to them (and could potentially also
> > have separate subdevices for clocks and pinctrl).  These subdevices are
> > not memory mapped, not enumerated by firmware and the hardware has
> > indistinct separation of functions in the register map compared to how
> > Linux models the chips.

> wm8994 seems to fit in the mfd_cell static arrays pretty well..

I can't tell the difference between what it's doing and what SOF is
doing, the code I've seen is just looking at the system it's running
on and registering a fixed set of client devices.  It looks slightly
different because it's registering a device at a time with some wrapper
functions involved but that's what the code actually does.

Clearly there's something other than just the registration method going
on here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	lee.jones@linaro.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Kiran Patil <kiran.patil@intel.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	linux-rdma <linux-rdma@vger.kernel.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Martin Habets <mhabets@solarflare.com>,
	alsa-devel@alsa-project.org,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Fred Oh <fred.oh@linux.intel.com>,
	Netdev <netdev@vger.kernel.org>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Dave Ertman <david.m.ertman@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Shiraz Saleem <shiraz.saleem@intel.com>,
	David Miller <davem@davemloft.net>,
	Leon Romanovsky <leonro@nvidia.com>,
	Parav Pandit <parav@mellanox.com>
Subject: Re: [resend/standalone PATCH v4] Add auxiliary bus support
Date: Fri, 18 Dec 2020 20:32:11 +0000	[thread overview]
Message-ID: <20201218203211.GE5333@sirena.org.uk> (raw)
In-Reply-To: <20201218184150.GY552508@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 4641 bytes --]

On Fri, Dec 18, 2020 at 02:41:50PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 18, 2020 at 06:03:10PM +0000, Mark Brown wrote:

> > If it's not supposed to use platform devices so I'm assuming that the
> > intention is that it should use aux devices, otherwise presumably it'd
> > be making some new clone of the platform bus but I've not seen anyone
> > suggesting this.

> I wouldn't assume that, I certainly don't want to see all the HW
> related items in platform_device cloned roughly into aux device.

> I've understood the bus type should be basically related to the thing
> that is creating the device. In a clean view platform code creates
> platform devices. DT should create DT devices, ACPI creates ACPI
> devices, PNP does pnp devices, etc

Ah, so we *used* to do that and in fact at least acpi_device still
exists but it was realized that this was causing a lot of effort with
boilerplate - like Lee said board files, ACPI and DT are all just
enumeration methods which have zero effect on the underlying hardware so
you end up having duplication on both the bus and driver side.  Since
this applies to all non-enumerable buses this process gets repeated for
all of them, we wouldn't just have an of_device we'd have of_i2c_device,
of_spi_device, of_1wire_device and so on or have to jump through hoops
to map things into the actual bus type.  See eca3930163ba8884060ce9d9
(of: Merge of_platform_bus_type with platform_bus_type) for part of this
getting unwound.

Fundamentally this is conflating physical bus type and enumeration
method, for enumerable buses they are of course the same (mostly) but
for non-enumerable buses not so much.

> So, I strongly suspect, MFD should create mfd devices on a MFD bus
> type.

Historically people did try to create custom bus types, as I have
pointed out before there was then pushback that these were duplicating
the platform bus so everything uses platform bus.

> Alexandre's point is completely valid, and I think is the main
> challenge here, somehow avoiding duplication.

> If we were to look at it with some OOP viewpoint I'd say the generic
> HW resource related parts should be some shared superclass between
> 'struct device' and 'struct platform/pnp/pci/acpi/mfd/etc_device'.

Right, duplication is the big issue with separate firmware based bus
types particularly as we consider all non-enumerable buses.  I think
what you're looking for here is multiple inheritance, that's potentially
interesting but it's pretty much what we have already TBH.  We have the
physical bus type as a primary type for devices but we also can enquire
if they also have the properties of a DT or ACPI object and then use
those APIs on them.

Consider also FPGAs which can have the same problem Alexandre raised,
there's the parent device for the FPGA and then we can instantiate
bitstreams within that which may expose standard IPs which can also
appear directly within a SoC.

> > > The places I see aux device being used are a terrible fit for the cell
> > > idea. If there are MFD drivers that are awkardly crammed into that
> > > cell description then maybe they should be aux devices?

> > When you say the MFD cell model it's not clear what you mean - I *think*
> > you're referring to the idea of the subdevices getting all the

> I mean using static "struct mfd_cell" arrays to describe things.

OK, but then SOF has been actively pushed into using auxiliary devices
since there is a desire to avoid using mfd_cells on PCI devices rather
than the fact that it wasn't able to use a static array, and of course
you might have devices with a mix of static and dynamic functions, or
functions that can be both static and dynamic.

> > Look at something like wm8994 for example - the subdevices just know
> > which addresses in the device I2C/SPI regmap to work with but some of
> > them have interrupts passed through to them (and could potentially also
> > have separate subdevices for clocks and pinctrl).  These subdevices are
> > not memory mapped, not enumerated by firmware and the hardware has
> > indistinct separation of functions in the register map compared to how
> > Linux models the chips.

> wm8994 seems to fit in the mfd_cell static arrays pretty well..

I can't tell the difference between what it's doing and what SOF is
doing, the code I've seen is just looking at the system it's running
on and registering a fixed set of client devices.  It looks slightly
different because it's registering a device at a time with some wrapper
functions involved but that's what the code actually does.

Clearly there's something other than just the registration method going
on here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  parent reply	other threads:[~2020-12-18 20:33 UTC|newest]

Thread overview: 114+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-03  0:54 [resend/standalone PATCH v4] Add auxiliary bus support Dan Williams
2020-12-03  0:54 ` Dan Williams
2020-12-03 15:06 ` Greg KH
2020-12-03 15:06   ` Greg KH
2020-12-04  2:33   ` Jason Gunthorpe
2020-12-04  2:33     ` Jason Gunthorpe
2020-12-04  3:37     ` Dan Williams
2020-12-04  3:37       ` Dan Williams
2020-12-03 15:07 ` Greg KH
2020-12-03 15:07   ` Greg KH
2020-12-03 15:55   ` Leon Romanovsky
2020-12-03 15:55     ` Leon Romanovsky
2020-12-04 11:42 ` Greg KH
2020-12-04 11:42   ` Greg KH
2020-12-04 11:43   ` [PATCH 1/3] driver core: auxiliary bus: move slab.h from include file Greg KH
2020-12-04 11:43     ` Greg KH
2020-12-04 11:44     ` [PATCH 2/3] driver core: auxiliary bus: make remove function return void Greg KH
2020-12-04 11:44       ` Greg KH
2020-12-04 11:44       ` [PATCH 3/3] driver core: auxiliary bus: minor coding style tweaks Greg KH
2020-12-04 11:44         ` Greg KH
2020-12-04 11:48         ` Greg KH
2020-12-04 11:48           ` Greg KH
2020-12-04 11:49       ` [PATCH v2 " Greg KH
2020-12-04 11:49         ` Greg KH
2020-12-04 12:32   ` [resend/standalone PATCH v4] Add auxiliary bus support Leon Romanovsky
2020-12-04 12:32     ` Leon Romanovsky
2020-12-04 12:43     ` Parav Pandit
2020-12-04 12:43       ` Parav Pandit
2020-12-04 12:59     ` Greg KH
2020-12-04 12:59       ` Greg KH
2020-12-04 17:10       ` Ranjani Sridharan
2020-12-04 17:10         ` Ranjani Sridharan
2020-12-05  9:02         ` Greg KH
2020-12-05  9:02           ` Greg KH
2020-12-04 16:41   ` Dan Williams
2020-12-04 16:41     ` Dan Williams
2020-12-05 15:51     ` Greg KH
2020-12-05 15:51       ` Greg KH
2020-12-17 21:19       ` Alexandre Belloni
2020-12-17 21:19         ` Alexandre Belloni
2020-12-18  2:39         ` Dan Williams
2020-12-18  2:39           ` Dan Williams
2020-12-18 14:20           ` Mark Brown
2020-12-18 14:20             ` Mark Brown
2020-12-18  7:10         ` Greg KH
2020-12-18  7:10           ` Greg KH
2020-12-18 13:17           ` Mark Brown
2020-12-18 13:17             ` Mark Brown
2020-12-18 13:46             ` Lee Jones
2020-12-18 13:46               ` Lee Jones
2020-12-18 14:08             ` Jason Gunthorpe
2020-12-18 14:08               ` Jason Gunthorpe
2020-12-18 15:52               ` Mark Brown
2020-12-18 15:52                 ` Mark Brown
2020-12-18 16:28                 ` Jason Gunthorpe
2020-12-18 16:28                   ` Jason Gunthorpe
2020-12-18 17:15                   ` Alexandre Belloni
2020-12-18 17:15                     ` Alexandre Belloni
2020-12-18 18:03                   ` Mark Brown
2020-12-18 18:03                     ` Mark Brown
2020-12-18 18:41                     ` Jason Gunthorpe
2020-12-18 18:41                       ` Jason Gunthorpe
2020-12-18 19:09                       ` Lee Jones
2020-12-18 19:09                         ` Lee Jones
2020-12-18 20:14                         ` Jason Gunthorpe
2020-12-18 20:14                           ` Jason Gunthorpe
2020-12-18 20:32                       ` Mark Brown [this message]
2020-12-18 20:32                         ` Mark Brown
2020-12-18 20:58                         ` Jason Gunthorpe
2020-12-18 20:58                           ` Jason Gunthorpe
2020-12-18 21:16                           ` Alexandre Belloni
2020-12-18 21:16                             ` Alexandre Belloni
2020-12-18 22:36                             ` Dan Williams
2020-12-18 22:36                               ` Dan Williams
2020-12-18 23:36                             ` Jason Gunthorpe
2020-12-18 23:36                               ` Jason Gunthorpe
2020-12-19  0:22                               ` Alexandre Belloni
2020-12-19  0:22                                 ` Alexandre Belloni
2020-12-21 18:51                           ` Mark Brown
2020-12-21 18:51                             ` Mark Brown
2021-01-04 18:08                             ` Jason Gunthorpe
2021-01-04 18:08                               ` Jason Gunthorpe
2021-01-04 21:19                               ` Mark Brown
2021-01-04 21:19                                 ` Mark Brown
2021-01-05  0:13                                 ` Jason Gunthorpe
2021-01-05  0:13                                   ` Jason Gunthorpe
2021-01-05  0:51                                   ` Dan Williams
2021-01-05  0:51                                     ` Dan Williams
2021-01-05  1:53                                     ` Jason Gunthorpe
2021-01-05  1:53                                       ` Jason Gunthorpe
2021-01-05  3:12                                       ` Dan Williams
2021-01-05  3:12                                         ` Dan Williams
2021-01-05 12:49                                         ` Jason Gunthorpe
2021-01-05 12:49                                           ` Jason Gunthorpe
2021-01-05 13:42                                   ` Mark Brown
2021-01-05 13:42                                     ` Mark Brown
2021-01-05 14:36                                     ` Jason Gunthorpe
2021-01-05 14:36                                       ` Jason Gunthorpe
2021-01-05 15:47                                       ` Mark Brown
2021-01-05 15:47                                         ` Mark Brown
2020-12-04 12:35 ` Greg KH
2020-12-04 12:35   ` Greg KH
2020-12-04 12:54   ` Leon Romanovsky
2020-12-04 12:54     ` Leon Romanovsky
2020-12-04 16:25     ` Jakub Kicinski
2020-12-04 16:25       ` Jakub Kicinski
2020-12-04 17:57       ` Saeed Mahameed
2020-12-04 17:57         ` Saeed Mahameed
2020-12-04 18:05   ` Ranjani Sridharan
2020-12-04 18:05     ` Ranjani Sridharan
2020-12-06  0:24 ` David Ahern
2020-12-06  0:24   ` David Ahern
2020-12-06  0:32   ` Dan Williams
2020-12-06  0:32     ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201218203211.GE5333@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=alexandre.belloni@bootlin.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=dan.j.williams@intel.com \
    --cc=davem@davemloft.net \
    --cc=david.m.ertman@intel.com \
    --cc=fred.oh@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jgg@nvidia.com \
    --cc=kiran.patil@intel.com \
    --cc=kuba@kernel.org \
    --cc=lee.jones@linaro.org \
    --cc=leonro@nvidia.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=mhabets@solarflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=parav@mellanox.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=shiraz.saleem@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.