Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700175 > unrolled thread

[RFC 0/5] Add I3C subsystem

Back to article view | Back to linux.kernel

Contents

  [RFC 0/5] Add I3C subsystem Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-07-31 18:30 +0200
    [RFC 5/5] dt-bindings: i3c: Document Cadence I3C master bindings Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-07-31 18:30 +0200
    Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-07-31 21:20 +0200
      Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-07-31 22:50 +0200
    Re: [RFC 0/5] Add I3C subsystem Wolfram Sang <wsa@the-dreams.de> - 2017-07-31 21:20 +0200
      Re: [RFC 0/5] Add I3C subsystem Wolfram Sang <wsa@the-dreams.de> - 2017-07-31 22:50 +0200
      Re: [RFC 0/5] Add I3C subsystem Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-07-31 22:50 +0200
    Re: [RFC 2/5] i3c: Add core I3C infrastructure Arnd Bergmann <arnd@arndb.de> - 2017-07-31 22:20 +0200
      Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-07-31 23:20 +0200
        Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-07-31 23:50 +0200
          Re: [RFC 2/5] i3c: Add core I3C infrastructure "Andrew F. Davis" <afd@ti.com> - 2017-08-01 18:50 +0200
            Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-08-01 19:30 +0200
              Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 23:50 +0200
                Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-08-02 12:30 +0200
        Re: [RFC 2/5] i3c: Add core I3C infrastructure Arnd Bergmann <arnd@arndb.de> - 2017-08-01 14:10 +0200
          Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 14:30 +0200
            Re: [RFC 2/5] i3c: Add core I3C infrastructure Arnd Bergmann <arnd@arndb.de> - 2017-08-01 15:20 +0200
              Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 15:40 +0200
                Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 16:00 +0200
                  Re: [RFC 2/5] i3c: Add core I3C infrastructure Arnd Bergmann <arnd@arndb.de> - 2017-08-01 16:30 +0200
                    Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 17:20 +0200
                      Re: [RFC 2/5] i3c: Add core I3C infrastructure Arnd Bergmann <arnd@arndb.de> - 2017-08-01 22:20 +0200
                Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-08-01 16:20 +0200
                  Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 16:50 +0200
                    Re: [RFC 2/5] i3c: Add core I3C infrastructure Wolfram Sang <wsa@the-dreams.de> - 2017-08-01 17:10 +0200
                      Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 17:30 +0200
                        Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-03 10:10 +0200
    Re: [RFC 2/5] i3c: Add core I3C infrastructure Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-01 03:50 +0200
      Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 12:50 +0200
        Re: [RFC 2/5] i3c: Add core I3C infrastructure Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-01 20:00 +0200
          Re: [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-08-01 23:40 +0200
            Re: [RFC 2/5] i3c: Add core I3C infrastructure Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-02 03:00 +0200
            Re: [RFC 2/5] i3c: Add core I3C infrastructure Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-02 04:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1701144 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, 1 Aug 2017 16:22:21 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> On Tue, Aug 1, 2017 at 3:58 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Tue, 1 Aug 2017 15:34:14 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:  
> >> On Tue, 1 Aug 2017 15:11:44 +0200
> >> Arnd Bergmann <arnd@arndb.de> wrote:  
> >> > On Tue, Aug 1, 2017 at 2:29 PM, Boris Brezillon
> >> > <boris.brezillon@free-electrons.com> wrote:  
> > I just realized I forgot to add a "depends on I2C" in the I3C Kconfig
> > entry. Indeed, I'm unconditionally calling functions provided by the
> > I2C framework which have no dummy wrapper when I2C support is disabled.
> > I could of course conditionally compile some portion of the I3C
> > framework so that it still builds when I2C is disabled but I'm not sure
> > it's worth the trouble.
> >
> > This "depends on I2C" should also solve the I2C+I3C driver issue, since
> > I2C is necessarily enabled when I3C is.
> >
> > Am I missing something?  
> 
> That should solve another part of the problem, as a combined driver then
> just needs 'depends on I3C'.
> 
> On top of that, the i3c_driver structure could also contain callback
> pointers for the i2c subsystem, e.g. i2c_probe(), i2c_remove() etc.
> When the i2c_probe() callback exists, the i3c layer could construct
> a 'struct i2c_driver' with those callbacks and register that under the
> cover. This would mean that combined drivers no longer need to
> register two driver objects.

That should work. Actually, i2c_driver contains a few more hooks, like
->alert(), ->command() and ->detect(). Of course we could assume that
I3C/I2C drivers do not need them, but I'm wondering if it's not easier
to just add an i2c_driver pointer inside the i3c_driver struct and let
the driver populate it if it needs to supports both protocols.

Something like:

	struct i3c_driver {
		...
		struct i2c_driver *i2c_compat;
		...
	};


and then in I3C/I2C drivers:

	static struct i2c_driver my_i2c_driver = {
		...
	};

	static struct i3c_driver my_i3c_driver = {
		...
		.i2c_compat = &my_i2c_driver,
		...
	};
	module_i3c_driver(my_i3c_driver);



Of course, you'll have a few fields of ->i2c_compat that would be
filled by the core (like the driver name which can be extracted from
my_i3c_driver->driver.name).

[toc] | [prev] | [next] | [standalone]

#1701434 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, Aug 1, 2017 at 5:14 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 1 Aug 2017 16:22:21 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Aug 1, 2017 at 3:58 PM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > On Tue, 1 Aug 2017 15:34:14 +0200
>> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> >> On Tue, 1 Aug 2017 15:11:44 +0200
>> >> Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Tue, Aug 1, 2017 at 2:29 PM, Boris Brezillon
>> >> > <boris.brezillon@free-electrons.com> wrote:
>> > I just realized I forgot to add a "depends on I2C" in the I3C Kconfig
>> > entry. Indeed, I'm unconditionally calling functions provided by the
>> > I2C framework which have no dummy wrapper when I2C support is disabled.
>> > I could of course conditionally compile some portion of the I3C
>> > framework so that it still builds when I2C is disabled but I'm not sure
>> > it's worth the trouble.
>> >
>> > This "depends on I2C" should also solve the I2C+I3C driver issue, since
>> > I2C is necessarily enabled when I3C is.
>> >
>> > Am I missing something?
>>
>> That should solve another part of the problem, as a combined driver then
>> just needs 'depends on I3C'.
>>
>> On top of that, the i3c_driver structure could also contain callback
>> pointers for the i2c subsystem, e.g. i2c_probe(), i2c_remove() etc.
>> When the i2c_probe() callback exists, the i3c layer could construct
>> a 'struct i2c_driver' with those callbacks and register that under the
>> cover. This would mean that combined drivers no longer need to
>> register two driver objects.
>
> That should work. Actually, i2c_driver contains a few more hooks, like
> ->alert(), ->command() and ->detect(). Of course we could assume that
> I3C/I2C drivers do not need them,

I was thinking we can add them as they are needed.

> but I'm wondering if it's not easier
> to just add an i2c_driver pointer inside the i3c_driver struct and let
> the driver populate it if it needs to supports both protocols.
>
> Something like:
>
>         struct i3c_driver {
>                 ...
>                 struct i2c_driver *i2c_compat;
>                 ...
>         };
>
>
> and then in I3C/I2C drivers:
>
>         static struct i2c_driver my_i2c_driver = {
>                 ...
>         };
>
>         static struct i3c_driver my_i3c_driver = {
>                 ...
>                 .i2c_compat = &my_i2c_driver,
>                 ...
>         };
>         module_i3c_driver(my_i3c_driver);
>
>
>
> Of course, you'll have a few fields of ->i2c_compat that would be
> filled by the core (like the driver name which can be extracted from
> my_i3c_driver->driver.name).

Right, that would work too, but it's almost the same as the version
you proposed earlier that would use

module_i2c_i3c_driver(my_i2c_driver, my_i3c_driver);

It's probably a little cleaner this way in the subsystem implementation
compared to my suggestion of adding the i2c callback pointers in
struct i3c_driver, while that would make the drivers look a little nicer
(and save a few lines per driver).

         Arnd

[toc] | [prev] | [next] | [standalone]

#1701108 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

[Multipart message — attachments visible in raw view] — view raw

> > The second way is to have a number of #ifdef and complex
> > Kconfig dependencies for the driver to only register the
> > device_driver objects for the buses that are enabled. This
> > is also doable, but everyone gets the logic wrong the first time.
> 
> Hm, I understand now why you'd prefer to have a single bus. Can't we
> solve this problem with a module_i3c_i2c_driver() macro that would hide
> all this complexity from I2C/I3C drivers?

Do you know of devices speaking both i3c and i2c as of today?

I think I3C/I2C is a bit different than I2C/SPI. For the latter, it
might happen that you have only this or that bus on the board, so it
makes sense to support both. But if you have I3C, you can simply attach
the I2C device onto it. I guess you would only implement I3C in the
device if you explicitly need its feature set. And then, a I2C fallback
doesn't make much sense? Or am I missing something?

OK, now I know that those I3C+I2C devices will exist, even if only for
Murphy's law. However, my assumptions would be that those devices are
not common and so we could live with the core plus bus_drivers
seperation we have for SPI/I2C already (although I would love a common
regmap-based I2C/SPI abstraction).

[toc] | [prev] | [next] | [standalone]

#1701120 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, 1 Aug 2017 16:12:18 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> > > The second way is to have a number of #ifdef and complex
> > > Kconfig dependencies for the driver to only register the
> > > device_driver objects for the buses that are enabled. This
> > > is also doable, but everyone gets the logic wrong the first time.  
> > 
> > Hm, I understand now why you'd prefer to have a single bus. Can't we
> > solve this problem with a module_i3c_i2c_driver() macro that would hide
> > all this complexity from I2C/I3C drivers?  
> 
> Do you know of devices speaking both i3c and i2c as of today?

I do not know of any real devices as of today (all my tests have been
done with a dummy/fake I3C slaves emulated with a slave IP), but the
spec clearly describe what legacy/static addresses are for and one of
their use case is to connect an I3C device on an I2C bus and let it act
as an I2C device.

> 
> I think I3C/I2C is a bit different than I2C/SPI. For the latter, it
> might happen that you have only this or that bus on the board, so it
> makes sense to support both. But if you have I3C, you can simply attach
> the I2C device onto it. I guess you would only implement I3C in the
> device if you explicitly need its feature set. And then, a I2C fallback
> doesn't make much sense? Or am I missing something?

Unless you want your device (likely a sensor) to be compatible with both
I3C and I2C so that you can target even more people.

> 
> OK, now I know that those I3C+I2C devices will exist, even if only for
> Murphy's law. However, my assumptions would be that those devices are
> not common and so we could live with the core plus bus_drivers
> seperation we have for SPI/I2C already (although I would love a common
> regmap-based I2C/SPI abstraction).
> 

I'm perfectly fine with the I3C / I2C framework separation. The only
minor problem I had with that was the inaccuracy of the
sysfs/device-model representation: we don't have one i2c and one i3c
bus, we just have one i3c bus with a mix of i2c and i3c devices.

Apart from that, I'm happy with the current approach.

[toc] | [prev] | [next] | [standalone]

#1701139 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

[Multipart message — attachments visible in raw view] — view raw

> I do not know of any real devices as of today (all my tests have been
> done with a dummy/fake I3C slaves emulated with a slave IP),

I see.

> spec clearly describe what legacy/static addresses are for and one of
> their use case is to connect an I3C device on an I2C bus and let it act
> as an I2C device.

OK. That makes it more likely.

> Unless you want your device (likely a sensor) to be compatible with both
> I3C and I2C so that you can target even more people.

Right. My question was if this is a realistic or more academic scenario.

> I'm perfectly fine with the I3C / I2C framework separation. The only
> minor problem I had with that was the inaccuracy of the
> sysfs/device-model representation: we don't have one i2c and one i3c
> bus, we just have one i3c bus with a mix of i2c and i3c devices.

I understand that. What if I2C had the same seperation between the "bus"
and the "master"?

[toc] | [prev] | [next] | [standalone]

#1701155 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, 1 Aug 2017 17:01:08 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> > I do not know of any real devices as of today (all my tests have been
> > done with a dummy/fake I3C slaves emulated with a slave IP),  
> 
> I see.
> 
> > spec clearly describe what legacy/static addresses are for and one of
> > their use case is to connect an I3C device on an I2C bus and let it act
> > as an I2C device.  
> 
> OK. That makes it more likely.
> 
> > Unless you want your device (likely a sensor) to be compatible with both
> > I3C and I2C so that you can target even more people.  
> 
> Right. My question was if this is a realistic or more academic scenario.
> 
> > I'm perfectly fine with the I3C / I2C framework separation. The only
> > minor problem I had with that was the inaccuracy of the
> > sysfs/device-model representation: we don't have one i2c and one i3c
> > bus, we just have one i3c bus with a mix of i2c and i3c devices.  
> 
> I understand that. What if I2C had the same seperation between the "bus"
> and the "master"?
> 

Yep, it might work if we can register an i2c_adapter and pass it an
existing bus object. We'd still need a common base for i2c and i3c
busses, unless we consider the bus as an opaque "struct device *"
object.

[toc] | [prev] | [next] | [standalone]

#1702748 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, 1 Aug 2017 17:20:41 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Tue, 1 Aug 2017 17:01:08 +0200
> Wolfram Sang <wsa@the-dreams.de> wrote:
> 
> > > I do not know of any real devices as of today (all my tests have been
> > > done with a dummy/fake I3C slaves emulated with a slave IP),    
> > 
> > I see.
> >   
> > > spec clearly describe what legacy/static addresses are for and one of
> > > their use case is to connect an I3C device on an I2C bus and let it act
> > > as an I2C device.    
> > 
> > OK. That makes it more likely.
> >   
> > > Unless you want your device (likely a sensor) to be compatible with both
> > > I3C and I2C so that you can target even more people.    
> > 
> > Right. My question was if this is a realistic or more academic scenario.
> >   
> > > I'm perfectly fine with the I3C / I2C framework separation. The only
> > > minor problem I had with that was the inaccuracy of the
> > > sysfs/device-model representation: we don't have one i2c and one i3c
> > > bus, we just have one i3c bus with a mix of i2c and i3c devices.    
> > 
> > I understand that. What if I2C had the same seperation between the "bus"
> > and the "master"?
> >   
> 
> Yep, it might work if we can register an i2c_adapter and pass it an
> existing bus object. We'd still need a common base for i2c and i3c
> busses, unless we consider the bus as an opaque "struct device *"
> object.

I tried to envision how this could be implemented but realized
separating the bus and master concepts in I2C wouldn't solve all
problems.

Each device is attached a bus_type which defines how to match devices
and drivers, uevent format, ... But it also defines where the device
appears in sysfs (/sys/bus/<bus-name>/devices).

First question: where should an I2C device connected on an I3C bus
appear? /sys/bus/i3c/devices/ or /sys/bus/i2c/devices/? I'd say both
(with one of them being a symlink to the other) but I'm not sure.

Also, if we go for a 'single bus per master' representation but still
want i3c and i2c to be differentiated, that means when one adds an
i2c_driver we'll have to duplicate this driver object and register one
instance to the i2c framework and the other one to the i3c framework,
because device <-> driver matching is done per bus_type.

One solution would be to go for Arnd suggestion to extend i2c_bus_type
with I3C support, but then i3c related kojects would be exposed
under /sys/bus/i2c/ which can be disturbing for people who are used to
look at /sys/bus/<bus-name> to find devices connected on a specific bus
type.

Honestly, I don't know what's the best solution here. Every solution has
its pros and cons:

1/ The "one i2c bus and one i3c bus per i3c master" I proposed in this
   RFC is non-invasive but the resulting sysfs/device-model
   representation is not accurate.
2/ Separating the I3C and I2C framework with a thin layer between them
   to propagate i2c drivers registration to the i3c framework and make
   sure i2c devices are exposed in both worlds is much more complicated
   to implement but should provide an accurate bus <-> device
   representation.
3/ Extending i2c_bus_type (and more generally the I2C framework) to
   support I3C devices/busses is invasive and we still have a
   non-accurate representation (i2c busses are mixed with i3c busses
   and all exposed under /sys/bus/i2c/). One advantage with this
   solution compared to #2 is that we don't need to duplicate
   i2c_driver objects in order to register them to both i2c and i3c bus
   types.

Any advice is welcome.

Thanks,

Boris

[toc] | [prev] | [next] | [standalone]

#1700574 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Mon, Jul 31, 2017 at 06:24:47PM +0200, Boris Brezillon wrote:
> Add core infrastructure to support I3C in Linux and document it.
> 
> This infrastructure is not complete yet and will be extended over
> time.
> 
> There are a few design choices that are worth mentioning because they
> impact the way I3C device drivers can interact with their devices:
> 
> - all functions used to send I3C/I2C frames must be called in
>   non-atomic context. Mainly done this way to ease implementation, but
>   this is still open to discussion. Please let me know if you think it's
>   worth considering an asynchronous model here
> - the bus element is a separate object and is not implicitly described
>   by the master (as done in I2C). The reason is that I want to be able
>   to handle multiple master connected to the same bus and visible to
>   Linux.
>   In this situation, we should only have one instance of the device and
>   not one per master, and sharing the bus object would be part of the
>   solution to gracefully handle this case.
>   I'm not sure we will ever need to deal with multiple masters
>   controlling the same bus and exposed under Linux, but separating the
>   bus and master concept is pretty easy, hence the decision to do it
>   like that.
>   The other benefit of separating the bus and master concepts is that
>   master devices appear under the bus directory in sysfs.
> - I2C backward compatibility has been designed to be transparent to I2C
>   drivers and the I2C subsystem. The I3C master just registers an I2C
>   adapter which creates a new I2C bus. I'd say that, from a
>   representation PoV it's not ideal because what should appear as a
>   single I3C bus exposing I3C and I2C devices here appears as 2
>   different busses connected to each other through the parenting (the
>   I3C master is the parent of the I2C and I3C busses).
>   On the other hand, I don't see a better solution if we want something
>   that is not invasive.
> - the whole API is exposed through a single header file (i3c.h), but I'm
>   seriously considering the option of splitting the I3C driver/user API
>   and the I3C master one, mainly to hide I3C core internals and restrict
>   what I3C users can do to a limited set of functionalities (send
>   I3C/I2C frames to a specific device and that's all).
> 
> Missing features in this preliminary version:
> - no support for IBI (In Band Interrupts). This is something I'm working
>   on, and I'm still unsure how to represent it: an irqchip or a
>   completely independent representation that would be I3C specific.
>   Right now, I'm more inclined to go for the irqchip approach, since
>   this is something people are used to deal with already.
> - no Hot Join support, which is similar to hotplug
> - no support for multi-master and the associated concepts (mastership
>   handover, support for secondary masters, ...)
> - I2C devices can only be described using DT because this is the only
>   use case I have. However, the framework can easily be extended with
>   ACPI and board info support
> - I3C slave framework. This has been completely omitted, but shouldn't
>   have a huge impact on the I3C framework because I3C slaves don't see
>   the whole bus, it's only about handling master requests and generating
>   IBIs. Some of the struct, constant and enum definitions could be
>   shared, but most of the I3C slave framework logic will be different
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  Documentation/i3c/conf.py               |   10 +
>  Documentation/i3c/device-driver-api.rst |    7 +
>  Documentation/i3c/index.rst             |    9 +
>  Documentation/i3c/master-driver-api.rst |    8 +
>  Documentation/i3c/protocol.rst          |  199 +++++
>  Documentation/index.rst                 |    1 +
>  drivers/Kconfig                         |    2 +
>  drivers/Makefile                        |    2 +-
>  drivers/i3c/Kconfig                     |   24 +
>  drivers/i3c/Makefile                    |    3 +
>  drivers/i3c/core.c                      |  532 ++++++++++++++
>  drivers/i3c/device.c                    |  138 ++++
>  drivers/i3c/internals.h                 |   45 ++
>  drivers/i3c/master.c                    | 1225 +++++++++++++++++++++++++++++++
>  drivers/i3c/master/Kconfig              |    0
>  drivers/i3c/master/Makefile             |    0
>  include/linux/i3c/ccc.h                 |  389 ++++++++++
>  include/linux/i3c/device.h              |  212 ++++++
>  include/linux/i3c/master.h              |  453 ++++++++++++
>  include/linux/mod_devicetable.h         |   15 +
>  20 files changed, 3273 insertions(+), 1 deletion(-)

Any chance you can break the documentation out from this patch to make
it smaller and a bit simpler to review?

Here's a few random review comments, I have only glanced at this, not
done any real reading of this at all...

> +menu "I3C support"
> +
> +config I3C
> +	tristate "I3C support"
> +	---help---
> +	  I3C (pronounce: I-cube-C) is a serial protocol standardized by the
> +	  MIPI alliance.
> +
> +	  It's supposed to be backward compatible with I2C while providing
> +	  support for high speed transfers and native interrupt support
> +	  without the need for extra pins.
> +
> +	  The I3C protocol also standardizes the slave device types and is
> +	  mainly design to communicate with sensors.
> +
> +	  If you want I3C support, you should say Y here and also to the
> +	  specific driver for your bus adapter(s) below.
> +
> +	  This I3C support can also be built as a module.  If so, the module
> +	  will be called i3c.
> +
> +source "drivers/i3c/master/Kconfig"

Don't source unless i3c is enabled, right?

> +
> +endmenu
> diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> new file mode 100644
> index 000000000000..0605a275f47b
> --- /dev/null
> +++ b/drivers/i3c/Makefile
> @@ -0,0 +1,3 @@
> +i3c-y				:= core.o device.o master.o
> +obj-$(CONFIG_I3C)		+= i3c.o
> +obj-$(CONFIG_I3C)		+= master/
> diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> new file mode 100644
> index 000000000000..c000fb458547
> --- /dev/null
> +++ b/drivers/i3c/core.c
> @@ -0,0 +1,532 @@
> +/*
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.

I have to ask, do you really mean "or any later version"?

> +static DEFINE_IDR(i3c_bus_idr);

You never clean this up when the module goes away :(

> +static DEFINE_MUTEX(i3c_core_lock);
> +
> +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> +{
> +	if (exclusive)
> +		down_write(&bus->lock);
> +	else
> +		down_read(&bus->lock);
> +}

The "exclusive" flag is odd, and messy, and hard to understand, don't
you agree?  And have you measured the difference in using a rw lock over
a normal mutex and found it to be faster?  If not, just use a normal
mutex, it's simpler and almost always better in the end.

> +
> +void i3c_bus_unlock(struct i3c_bus *bus, bool exclusive)
> +{
> +	if (exclusive)
> +		up_write(&bus->lock);
> +	else
> +		up_read(&bus->lock);
> +}
> +
> +static ssize_t bcr_show(struct device *dev,
> +			struct device_attribute *da,
> +			char *buf)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(bus, false);
> +	ret = sprintf(buf, "%x\n", i3cdev->info.bcr);
> +	i3c_bus_unlock(bus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(bcr);
> +
> +static ssize_t dcr_show(struct device *dev,
> +			struct device_attribute *da,
> +			char *buf)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(bus, false);
> +	ret = sprintf(buf, "%x\n", i3cdev->info.dcr);
> +	i3c_bus_unlock(bus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(dcr);
> +
> +static ssize_t pid_show(struct device *dev,
> +			struct device_attribute *da,
> +			char *buf)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(bus, false);
> +	ret = sprintf(buf, "%llx\n", i3cdev->info.pid);
> +	i3c_bus_unlock(bus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(pid);

No Documentation/ABI entries for all of these sysfs files?

> +
> +static ssize_t address_show(struct device *dev,
> +			    struct device_attribute *da,
> +			    char *buf)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(bus, false);
> +	ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr);
> +	i3c_bus_unlock(bus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(address);
> +
> +static const char * const hdrcap_strings[] = {
> +	"hdr-ddr", "hdr-tsp", "hdr-tsl",
> +};
> +
> +static ssize_t hdrcap_show(struct device *dev,
> +			   struct device_attribute *da,
> +			   char *buf)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> +	unsigned long caps = i3cdev->info.hdr_cap;
> +	ssize_t offset = 0, ret;
> +	int mode;
> +
> +	i3c_bus_lock(bus, false);
> +	for_each_set_bit(mode, &caps, 8) {
> +		if (mode >= ARRAY_SIZE(hdrcap_strings))
> +			break;
> +
> +		if (!hdrcap_strings[mode])
> +			continue;
> +
> +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);

Multiple lines in a single sysfs file?  No.

> +		if (ret < 0)
> +			goto out;
> +
> +		offset += ret;
> +	}
> +	ret = offset;
> +
> +out:
> +	i3c_bus_unlock(bus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(hdrcap);
> +
> +static struct attribute *i3c_device_attrs[] = {
> +	&dev_attr_bcr.attr,
> +	&dev_attr_dcr.attr,
> +	&dev_attr_pid.attr,
> +	&dev_attr_address.attr,
> +	&dev_attr_hdrcap.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group i3c_device_group = {
> +	.attrs = i3c_device_attrs,
> +};
> +
> +static const struct attribute_group *i3c_device_groups[] = {
> +	&i3c_device_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS()?


> +
> +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> +	u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> +	u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> +	u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> +
> +	if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid))
> +		return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
> +				      i3cdev->info.dcr, manuf);
> +
> +	return add_uevent_var(env,
> +			      "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x",
> +			      i3cdev->info.dcr, manuf, part, ext);
> +}
> +
> +const struct device_type i3c_device_type = {
> +	.groups	= i3c_device_groups,
> +	.uevent = i3c_device_uevent,
> +};

No release type?  Oh that's bad bad bad and implies you have never
removed a device from your system as the kernel would have complained
loudly at you.

> +
> +static const struct attribute_group *i3c_master_groups[] = {
> +	&i3c_device_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS()?

> +
> +const struct device_type i3c_master_type = {
> +	.groups	= i3c_master_groups,
> +};
> +
> +static const char * const i3c_bus_mode_strings[] = {
> +	[I3C_BUS_MODE_PURE] = "pure",
> +	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
> +	[I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
> +};
> +
> +static ssize_t mode_show(struct device *dev,
> +			 struct device_attribute *da,
> +			 char *buf)
> +{
> +	struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(i3cbus, false);
> +	if (i3cbus->mode < 0 ||
> +	    i3cbus->mode > ARRAY_SIZE(i3c_bus_mode_strings) ||
> +	    !i3c_bus_mode_strings[i3cbus->mode])
> +		ret = sprintf(buf, "unknown\n");
> +	else
> +		ret = sprintf(buf, "%s\n", i3c_bus_mode_strings[i3cbus->mode]);
> +	i3c_bus_unlock(i3cbus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(mode);
> +
> +static ssize_t current_master_show(struct device *dev,
> +				   struct device_attribute *da,
> +				   char *buf)
> +{
> +	struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(i3cbus, false);
> +	ret = sprintf(buf, "%s\n", dev_name(&i3cbus->cur_master->dev));
> +	i3c_bus_unlock(i3cbus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(current_master);
> +
> +static ssize_t i3c_scl_frequency_show(struct device *dev,
> +				      struct device_attribute *da,
> +				      char *buf)
> +{
> +	struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(i3cbus, false);
> +	ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i3c);
> +	i3c_bus_unlock(i3cbus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(i3c_scl_frequency);
> +
> +static ssize_t i2c_scl_frequency_show(struct device *dev,
> +				      struct device_attribute *da,
> +				      char *buf)
> +{
> +	struct i3c_bus *i3cbus = container_of(dev, struct i3c_bus, dev);
> +	ssize_t ret;
> +
> +	i3c_bus_lock(i3cbus, false);
> +	ret = sprintf(buf, "%ld\n", i3cbus->scl_rate.i2c);
> +	i3c_bus_unlock(i3cbus, false);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(i2c_scl_frequency);
> +
> +static struct attribute *i3c_busdev_attrs[] = {
> +	&dev_attr_mode.attr,
> +	&dev_attr_current_master.attr,
> +	&dev_attr_i3c_scl_frequency.attr,
> +	&dev_attr_i2c_scl_frequency.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(i3c_busdev);

Yeah, you used it here!

that's all the time I have right now...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1700848 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

Hello Greg,

On Mon, 31 Jul 2017 18:40:21 -0700
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Mon, Jul 31, 2017 at 06:24:47PM +0200, Boris Brezillon wrote:
> > Add core infrastructure to support I3C in Linux and document it.
> > 
> > This infrastructure is not complete yet and will be extended over
> > time.
> > 
> > There are a few design choices that are worth mentioning because they
> > impact the way I3C device drivers can interact with their devices:
> > 
> > - all functions used to send I3C/I2C frames must be called in
> >   non-atomic context. Mainly done this way to ease implementation, but
> >   this is still open to discussion. Please let me know if you think it's
> >   worth considering an asynchronous model here
> > - the bus element is a separate object and is not implicitly described
> >   by the master (as done in I2C). The reason is that I want to be able
> >   to handle multiple master connected to the same bus and visible to
> >   Linux.
> >   In this situation, we should only have one instance of the device and
> >   not one per master, and sharing the bus object would be part of the
> >   solution to gracefully handle this case.
> >   I'm not sure we will ever need to deal with multiple masters
> >   controlling the same bus and exposed under Linux, but separating the
> >   bus and master concept is pretty easy, hence the decision to do it
> >   like that.
> >   The other benefit of separating the bus and master concepts is that
> >   master devices appear under the bus directory in sysfs.
> > - I2C backward compatibility has been designed to be transparent to I2C
> >   drivers and the I2C subsystem. The I3C master just registers an I2C
> >   adapter which creates a new I2C bus. I'd say that, from a
> >   representation PoV it's not ideal because what should appear as a
> >   single I3C bus exposing I3C and I2C devices here appears as 2
> >   different busses connected to each other through the parenting (the
> >   I3C master is the parent of the I2C and I3C busses).
> >   On the other hand, I don't see a better solution if we want something
> >   that is not invasive.
> > - the whole API is exposed through a single header file (i3c.h), but I'm
> >   seriously considering the option of splitting the I3C driver/user API
> >   and the I3C master one, mainly to hide I3C core internals and restrict
> >   what I3C users can do to a limited set of functionalities (send
> >   I3C/I2C frames to a specific device and that's all).
> > 
> > Missing features in this preliminary version:
> > - no support for IBI (In Band Interrupts). This is something I'm working
> >   on, and I'm still unsure how to represent it: an irqchip or a
> >   completely independent representation that would be I3C specific.
> >   Right now, I'm more inclined to go for the irqchip approach, since
> >   this is something people are used to deal with already.
> > - no Hot Join support, which is similar to hotplug
> > - no support for multi-master and the associated concepts (mastership
> >   handover, support for secondary masters, ...)
> > - I2C devices can only be described using DT because this is the only
> >   use case I have. However, the framework can easily be extended with
> >   ACPI and board info support
> > - I3C slave framework. This has been completely omitted, but shouldn't
> >   have a huge impact on the I3C framework because I3C slaves don't see
> >   the whole bus, it's only about handling master requests and generating
> >   IBIs. Some of the struct, constant and enum definitions could be
> >   shared, but most of the I3C slave framework logic will be different
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  Documentation/i3c/conf.py               |   10 +
> >  Documentation/i3c/device-driver-api.rst |    7 +
> >  Documentation/i3c/index.rst             |    9 +
> >  Documentation/i3c/master-driver-api.rst |    8 +
> >  Documentation/i3c/protocol.rst          |  199 +++++
> >  Documentation/index.rst                 |    1 +
> >  drivers/Kconfig                         |    2 +
> >  drivers/Makefile                        |    2 +-
> >  drivers/i3c/Kconfig                     |   24 +
> >  drivers/i3c/Makefile                    |    3 +
> >  drivers/i3c/core.c                      |  532 ++++++++++++++
> >  drivers/i3c/device.c                    |  138 ++++
> >  drivers/i3c/internals.h                 |   45 ++
> >  drivers/i3c/master.c                    | 1225 +++++++++++++++++++++++++++++++
> >  drivers/i3c/master/Kconfig              |    0
> >  drivers/i3c/master/Makefile             |    0
> >  include/linux/i3c/ccc.h                 |  389 ++++++++++
> >  include/linux/i3c/device.h              |  212 ++++++
> >  include/linux/i3c/master.h              |  453 ++++++++++++
> >  include/linux/mod_devicetable.h         |   15 +
> >  20 files changed, 3273 insertions(+), 1 deletion(-)  
> 
> Any chance you can break the documentation out from this patch to make
> it smaller and a bit simpler to review?

Sure. I'll put the doc and code in 2 different commits.

> 
> Here's a few random review comments, I have only glanced at this, not
> done any real reading of this at all...
> 
> > +menu "I3C support"
> > +
> > +config I3C
> > +	tristate "I3C support"
> > +	---help---
> > +	  I3C (pronounce: I-cube-C) is a serial protocol standardized by the
> > +	  MIPI alliance.
> > +
> > +	  It's supposed to be backward compatible with I2C while providing
> > +	  support for high speed transfers and native interrupt support
> > +	  without the need for extra pins.
> > +
> > +	  The I3C protocol also standardizes the slave device types and is
> > +	  mainly design to communicate with sensors.
> > +
> > +	  If you want I3C support, you should say Y here and also to the
> > +	  specific driver for your bus adapter(s) below.
> > +
> > +	  This I3C support can also be built as a module.  If so, the module
> > +	  will be called i3c.
> > +
> > +source "drivers/i3c/master/Kconfig"  
> 
> Don't source unless i3c is enabled, right?

Right, I'll also switch to menuconfig instead of menu+config.

> 
> > +
> > +endmenu
> > diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> > new file mode 100644
> > index 000000000000..0605a275f47b
> > --- /dev/null
> > +++ b/drivers/i3c/Makefile
> > @@ -0,0 +1,3 @@
> > +i3c-y				:= core.o device.o master.o
> > +obj-$(CONFIG_I3C)		+= i3c.o
> > +obj-$(CONFIG_I3C)		+= master/
> > diff --git a/drivers/i3c/core.c b/drivers/i3c/core.c
> > new file mode 100644
> > index 000000000000..c000fb458547
> > --- /dev/null
> > +++ b/drivers/i3c/core.c
> > @@ -0,0 +1,532 @@
> > +/*
> > + * Copyright (C) 2017 Cadence Design Systems Inc.
> > + *
> > + * Author: Boris Brezillon <boris.brezillon@free-electrons.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the Free
> > + * Software Foundation; either version 2 of the License, or (at your option)
> > + * any later version.  
> 
> I have to ask, do you really mean "or any later version"?

I'll ask Cadence.

> 
> > +static DEFINE_IDR(i3c_bus_idr);  
> 
> You never clean this up when the module goes away :(

I'll call idr_destroy() in i3c_exit().

> 
> > +static DEFINE_MUTEX(i3c_core_lock);
> > +
> > +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> > +{
> > +	if (exclusive)
> > +		down_write(&bus->lock);
> > +	else
> > +		down_read(&bus->lock);
> > +}  
> 
> The "exclusive" flag is odd, and messy, and hard to understand, don't
> you agree?

I could create 2 functions, one for the exclusive lock and the other
one for the shared lock.

> And have you measured the difference in using a rw lock over
> a normal mutex and found it to be faster?  If not, just use a normal
> mutex, it's simpler and almost always better in the end.

I did not measure the difference, but using a standard lock means
serializing all I3C accesses going through a given master in the core.

Note that this lock is not here to guarantee that calls to
master->ops->xxx() are serialized (this part is left to the master
driver), but to ensure that when a bus management operation is in
progress (like changing the address of a device on the bus), no one can
send I3C frames. If we don't do that, one could queue an I3C transfer,
and by the time this transfer reach the bus, the I3C device this
message was targeting may have a different address.

Unless you see a good reason to not use a R/W lock, I'd like to keep it
this way because master IPs are likely to implement advanced queuing
mechanism (allows one to queue new transfers even if the master is
already busy processing other requests), and serializing things at the
framework level will just prevent us from using this kind of
optimization.

> 
> > +
> > +void i3c_bus_unlock(struct i3c_bus *bus, bool exclusive)
> > +{
> > +	if (exclusive)
> > +		up_write(&bus->lock);
> > +	else
> > +		up_read(&bus->lock);
> > +}
> > +
> > +static ssize_t bcr_show(struct device *dev,
> > +			struct device_attribute *da,
> > +			char *buf)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > +	ssize_t ret;
> > +
> > +	i3c_bus_lock(bus, false);
> > +	ret = sprintf(buf, "%x\n", i3cdev->info.bcr);
> > +	i3c_bus_unlock(bus, false);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(bcr);
> > +
> > +static ssize_t dcr_show(struct device *dev,
> > +			struct device_attribute *da,
> > +			char *buf)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > +	ssize_t ret;
> > +
> > +	i3c_bus_lock(bus, false);
> > +	ret = sprintf(buf, "%x\n", i3cdev->info.dcr);
> > +	i3c_bus_unlock(bus, false);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(dcr);
> > +
> > +static ssize_t pid_show(struct device *dev,
> > +			struct device_attribute *da,
> > +			char *buf)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > +	ssize_t ret;
> > +
> > +	i3c_bus_lock(bus, false);
> > +	ret = sprintf(buf, "%llx\n", i3cdev->info.pid);
> > +	i3c_bus_unlock(bus, false);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(pid);  
> 
> No Documentation/ABI entries for all of these sysfs files?

Yep, I mentioned the lack of sysfs doc in my cover letter. Will address
that in v2.

> 
> > +
> > +static ssize_t address_show(struct device *dev,
> > +			    struct device_attribute *da,
> > +			    char *buf)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > +	ssize_t ret;
> > +
> > +	i3c_bus_lock(bus, false);
> > +	ret = sprintf(buf, "%02x\n", i3cdev->info.dyn_addr);
> > +	i3c_bus_unlock(bus, false);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(address);
> > +
> > +static const char * const hdrcap_strings[] = {
> > +	"hdr-ddr", "hdr-tsp", "hdr-tsl",
> > +};
> > +
> > +static ssize_t hdrcap_show(struct device *dev,
> > +			   struct device_attribute *da,
> > +			   char *buf)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > +	unsigned long caps = i3cdev->info.hdr_cap;
> > +	ssize_t offset = 0, ret;
> > +	int mode;
> > +
> > +	i3c_bus_lock(bus, false);
> > +	for_each_set_bit(mode, &caps, 8) {
> > +		if (mode >= ARRAY_SIZE(hdrcap_strings))
> > +			break;
> > +
> > +		if (!hdrcap_strings[mode])
> > +			continue;
> > +
> > +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);  
> 
> Multiple lines in a single sysfs file?  No.

Okay. Would that be okay with a different separator (like a comma)?

> 
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		offset += ret;
> > +	}
> > +	ret = offset;
> > +
> > +out:
> > +	i3c_bus_unlock(bus, false);
> > +
> > +	return ret;
> > +}
> > +static DEVICE_ATTR_RO(hdrcap);
> > +
> > +static struct attribute *i3c_device_attrs[] = {
> > +	&dev_attr_bcr.attr,
> > +	&dev_attr_dcr.attr,
> > +	&dev_attr_pid.attr,
> > +	&dev_attr_address.attr,
> > +	&dev_attr_hdrcap.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group i3c_device_group = {
> > +	.attrs = i3c_device_attrs,
> > +};
> > +
> > +static const struct attribute_group *i3c_device_groups[] = {
> > +	&i3c_device_group,
> > +	NULL,
> > +};  
> 
> ATTRIBUTE_GROUPS()?

My initial plan was to have a common set of attributes that apply to
both devices and masters, and then add specific attributes in each of
them when they only apply to a specific device type.

Something like:

static const struct attribute_group i3c_common_group = {
	.attrs = i3c_common_attrs,
};

static const struct attribute_group i3c_device_group = {
	.attrs = i3c_device_attrs,
};

static const struct attribute_group *i3c_device_groups[] = {
	&i3c_common_group,
	&i3c_device_group,
	NULL,
};

static const struct attribute_group i3c_master_group = {
	.attrs = i3c_master_attrs,
};

static const struct attribute_group *i3c_master_groups[] = {
	&i3c_common_group,
	&i3c_master_group,
	NULL,
};

It turned out I didn't have device or master specific attributes in this
version, so this differentiation is useless right now. I'll drop that
in my v2.

Just out of curiosity, what's the preferred solution when you need to
do something like that? Should we just use ATTRIBUTE_GROUPS() and
duplicate the entries that apply to both device types?

> 
> 
> > +
> > +static int i3c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > +	u16 manuf = I3C_PID_MANUF_ID(i3cdev->info.pid);
> > +	u16 part = I3C_PID_PART_ID(i3cdev->info.pid);
> > +	u16 ext = I3C_PID_EXTRA_INFO(i3cdev->info.pid);
> > +
> > +	if (I3C_PID_RND_LOWER_32BITS(i3cdev->info.pid))
> > +		return add_uevent_var(env, "MODALIAS=i3c:dcr%02Xmanuf%04X",
> > +				      i3cdev->info.dcr, manuf);
> > +
> > +	return add_uevent_var(env,
> > +			      "MODALIAS=i3c:dcr%02Xmanuf%04Xpart%04xext%04x",
> > +			      i3cdev->info.dcr, manuf, part, ext);
> > +}
> > +
> > +const struct device_type i3c_device_type = {
> > +	.groups	= i3c_device_groups,
> > +	.uevent = i3c_device_uevent,
> > +};  
> 
> No release type?  Oh that's bad bad bad and implies you have never
> removed a device from your system as the kernel would have complained
> loudly at you.

You got me, never tried to remove a device :-). Note that these
->release() hooks will just be dummy ones, because right now, device
resources are freed at bus destruction time.

Also, I see that dev->release() is called instead of
dev->type->release() if it's not NULL [1]. what's the preferred solution
here? Set dev->release or type->release()?

Thanks for your review,

Boris

[1]http://elixir.free-electrons.com/linux/v4.13-rc3/source/drivers/base/core.c#L809

[toc] | [prev] | [next] | [standalone]

#1701285 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, Aug 01, 2017 at 12:48:01PM +0200, Boris Brezillon wrote:
> > > +static DEFINE_MUTEX(i3c_core_lock);
> > > +
> > > +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> > > +{
> > > +	if (exclusive)
> > > +		down_write(&bus->lock);
> > > +	else
> > > +		down_read(&bus->lock);
> > > +}  
> > 
> > The "exclusive" flag is odd, and messy, and hard to understand, don't
> > you agree?
> 
> I could create 2 functions, one for the exclusive lock and the other
> one for the shared lock.

Or you could just use a simple mutex until you determine you really
would do better with a rw lock :)

> > And have you measured the difference in using a rw lock over
> > a normal mutex and found it to be faster?  If not, just use a normal
> > mutex, it's simpler and almost always better in the end.
> 
> I did not measure the difference, but using a standard lock means
> serializing all I3C accesses going through a given master in the core.

Which you are doing with a rw lock anyway, right?

> Note that this lock is not here to guarantee that calls to
> master->ops->xxx() are serialized (this part is left to the master
> driver), but to ensure that when a bus management operation is in
> progress (like changing the address of a device on the bus), no one can
> send I3C frames. If we don't do that, one could queue an I3C transfer,
> and by the time this transfer reach the bus, the I3C device this
> message was targeting may have a different address.

That sounds really odd.  locks should protect data, not bus access,
right?

> Unless you see a good reason to not use a R/W lock, I'd like to keep it
> this way because master IPs are likely to implement advanced queuing
> mechanism (allows one to queue new transfers even if the master is
> already busy processing other requests), and serializing things at the
> framework level will just prevent us from using this kind of
> optimization.

Unless you can prove otherwise, using a rw lock is almost always worse
than just a mutex.  And you shouldn't have a lock for bus transactions,
that's just going to be a total mess.  You could have a lock for a
single device access, but that still seems really strange, is the i3c
spec that bad?

> > > +static ssize_t hdrcap_show(struct device *dev,
> > > +			   struct device_attribute *da,
> > > +			   char *buf)
> > > +{
> > > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > > +	unsigned long caps = i3cdev->info.hdr_cap;
> > > +	ssize_t offset = 0, ret;
> > > +	int mode;
> > > +
> > > +	i3c_bus_lock(bus, false);
> > > +	for_each_set_bit(mode, &caps, 8) {
> > > +		if (mode >= ARRAY_SIZE(hdrcap_strings))
> > > +			break;
> > > +
> > > +		if (!hdrcap_strings[mode])
> > > +			continue;
> > > +
> > > +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);  
> > 
> > Multiple lines in a single sysfs file?  No.
> 
> Okay. Would that be okay with a different separator (like a comma)?

No, sysfs files are "one value per file", given you don't have any
documentation saying what this file is supposed to be showing, I can't
really judge the proper way for you to present it to userspace :)

> > > +static const struct attribute_group *i3c_device_groups[] = {
> > > +	&i3c_device_group,
> > > +	NULL,
> > > +};  
> > 
> > ATTRIBUTE_GROUPS()?
> 
> My initial plan was to have a common set of attributes that apply to
> both devices and masters, and then add specific attributes in each of
> them when they only apply to a specific device type.

That's fine, but you do know that attributes can be enabled/disabled at
device creation time with the return value of the callback is_visable(),
right?  Why not just use that here, simplifying a lot of logic?

> Just out of curiosity, what's the preferred solution when you need to
> do something like that? Should we just use ATTRIBUTE_GROUPS() and
> duplicate the entries that apply to both device types?

is_visable()?

> > No release type?  Oh that's bad bad bad and implies you have never
> > removed a device from your system as the kernel would have complained
> > loudly at you.
> 
> You got me, never tried to remove a device :-). Note that these
> ->release() hooks will just be dummy ones, because right now, device
> resources are freed at bus destruction time.

You better not have a "dummy" release hook, do that and as per the
kernel documentation, I get to make fun of you in public for doing that
:(

> Also, I see that dev->release() is called instead of
> dev->type->release() if it's not NULL [1]. what's the preferred solution
> here? Set dev->release or type->release()?

It depends on how your bus is managed, who controls the creation of the
resources, free it in the same place you create it.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1701522 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

Hi Greg,

Le Tue, 1 Aug 2017 10:51:33 -0700,
Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :

> On Tue, Aug 01, 2017 at 12:48:01PM +0200, Boris Brezillon wrote:
> > > > +static DEFINE_MUTEX(i3c_core_lock);
> > > > +
> > > > +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> > > > +{
> > > > +	if (exclusive)
> > > > +		down_write(&bus->lock);
> > > > +	else
> > > > +		down_read(&bus->lock);
> > > > +}    
> > > 
> > > The "exclusive" flag is odd, and messy, and hard to understand, don't
> > > you agree?  
> > 
> > I could create 2 functions, one for the exclusive lock and the other
> > one for the shared lock.  
> 
> Or you could just use a simple mutex until you determine you really
> would do better with a rw lock :)
> 
> > > And have you measured the difference in using a rw lock over
> > > a normal mutex and found it to be faster?  If not, just use a normal
> > > mutex, it's simpler and almost always better in the end.  
> > 
> > I did not measure the difference, but using a standard lock means
> > serializing all I3C accesses going through a given master in the core.  
> 
> Which you are doing with a rw lock anyway, right?

Absolutely not. If you look more closely at the code you'll see that
most of the time the lock is taken in read/non-exclusive mode. The only
situations where it's taken in exclusive mode is when the operation (and
associated command) has an impact on the bus and/or its devices.

For instance, resetting addresses of all I3C devices on the bus (using
RSTDAA) means you won't be able to send I3C messages to these devices
after that. If you don't take an exclusive lock to protect this
operation, that means you might end up with drivers queuing messages
with a device address that is about to be invalid. Also, we might soon
provide the possibility to change I3C dynamic address at runtime, which
is important since the dynamic address also encodes the priority of the
device when it's generating in-band interrupts (the lower the address
the higher the priority). We need this 'change dynamic address'
operation to be atomic for the same reason: to prevent drivers from
sending messages to an invalid address.

> 
> > Note that this lock is not here to guarantee that calls to
> > master->ops->xxx() are serialized (this part is left to the master
> > driver), but to ensure that when a bus management operation is in
> > progress (like changing the address of a device on the bus), no one can
> > send I3C frames. If we don't do that, one could queue an I3C transfer,
> > and by the time this transfer reach the bus, the I3C device this
> > message was targeting may have a different address.  
> 
> That sounds really odd.  locks should protect data, not bus access,
> right?

Well, it's protecting data: the dynamic address is a piece of
information attached to the device. 

> 
> > Unless you see a good reason to not use a R/W lock, I'd like to keep it
> > this way because master IPs are likely to implement advanced queuing
> > mechanism (allows one to queue new transfers even if the master is
> > already busy processing other requests), and serializing things at the
> > framework level will just prevent us from using this kind of
> > optimization.  
> 
> Unless you can prove otherwise, using a rw lock is almost always worse
> than just a mutex.

Is it still true when it's taken in non-exclusive mode most of the
time, and the time you spend in the critical section is non-negligible?

I won't pretend I know better than you do what is preferable, it's just
that the RW lock seemed appropriate to me for the situation I tried to
described here.

> And you shouldn't have a lock for bus transactions,
> that's just going to be a total mess.

It's not a lock for bus transactions, it's lock to protect from any
operation that can have an impact on the bus or its devices (see the
'change device address' example above).

> You could have a lock for a
> single device access, but that still seems really strange, is the i3c
> spec that bad?

Having a lock per device would complicate even more the situation
because some operations like the broadcasted RSTDAA have an impact on
all devices on the bus.

> 
> > > > +static ssize_t hdrcap_show(struct device *dev,
> > > > +			   struct device_attribute *da,
> > > > +			   char *buf)
> > > > +{
> > > > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > > > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > > > +	unsigned long caps = i3cdev->info.hdr_cap;
> > > > +	ssize_t offset = 0, ret;
> > > > +	int mode;
> > > > +
> > > > +	i3c_bus_lock(bus, false);
> > > > +	for_each_set_bit(mode, &caps, 8) {
> > > > +		if (mode >= ARRAY_SIZE(hdrcap_strings))
> > > > +			break;
> > > > +
> > > > +		if (!hdrcap_strings[mode])
> > > > +			continue;
> > > > +
> > > > +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);    
> > > 
> > > Multiple lines in a single sysfs file?  No.  
> > 
> > Okay. Would that be okay with a different separator (like a comma)?  
> 
> No, sysfs files are "one value per file", given you don't have any
> documentation saying what this file is supposed to be showing, I can't
> really judge the proper way for you to present it to userspace :)

Okay. Let's put that aside until I send a v2 with a sysfs doc.

Still, note that the "one value per file" rule does not apply to all
sysfs files. I have 2 examples in mind (maybe they are bad examples,
but they exist):

- /sys/class/leds/<led>/trigger returns a list of supported triggers
  each of them is separated by a space
- /sys/class/graphics/<fbx>/modes lists the supported video modes, one
  per line


> 
> > > > +static const struct attribute_group *i3c_device_groups[] = {
> > > > +	&i3c_device_group,
> > > > +	NULL,
> > > > +};    
> > > 
> > > ATTRIBUTE_GROUPS()?  
> > 
> > My initial plan was to have a common set of attributes that apply to
> > both devices and masters, and then add specific attributes in each of
> > them when they only apply to a specific device type.  
> 
> That's fine, but you do know that attributes can be enabled/disabled at
> device creation time with the return value of the callback is_visable(),
> right?  Why not just use that here, simplifying a lot of logic?

I didn't know that, thanks for the hint.

> 
> > Just out of curiosity, what's the preferred solution when you need to
> > do something like that? Should we just use ATTRIBUTE_GROUPS() and
> > duplicate the entries that apply to both device types?  
> 
> is_visable()?
> 
> > > No release type?  Oh that's bad bad bad and implies you have never
> > > removed a device from your system as the kernel would have complained
> > > loudly at you.  
> > 
> > You got me, never tried to remove a device :-). Note that these  
> > ->release() hooks will just be dummy ones, because right now, device  
> > resources are freed at bus destruction time.  
> 
> You better not have a "dummy" release hook, do that and as per the
> kernel documentation, I get to make fun of you in public for doing that
> :(

I'm not afraid of admitting I don't know everything, even the
simplest things that you consider as basics for a kernel developer. You
can make fun of me publicly if you want but that's not helping :-P.

BTW, the very reason I Cc-ed you in the first place is to have feedback
on this implementation, and please note that this is an RFC, so of
course, not everything is perfect. I'm here to learn from your reviews,
but that doesn't prevent me from asking more details about the
reasoning behind your suggestions. That's part of the learning process,
right?

The reason I proposed this dummy ->release() hook is because the
lifetime of the i2c/i3c dev allocated by the I3C framework goes beyond
the underlying struct device object embedded in it. Indeed, when the
i3c_device object is allocated, the I3C master can attach private data
to it (this is done inside master->ops->bus_init()), and these data
are expected to be freed in master->ops->bus_cleanup() which is done
after all devices attached to the I3C bus have been unregistered (so
the ->release() callback of each I3C dev has already been called before
we enter ->bus_cleanup()).

Now, maybe I took a wrong approach here, but I just wanted to explain
why releasing the I3C dev inside dev->release() or
dev->type->release() is not possible with the current implementation. 

> 
> > Also, I see that dev->release() is called instead of
> > dev->type->release() if it's not NULL [1]. what's the preferred solution
> > here? Set dev->release or type->release()?  
> 
> It depends on how your bus is managed, who controls the creation of the
> resources, free it in the same place you create it.

I3C devices are allocated by the master inside its ->bus_init() hook,
and I currently free all devices in i3c_bus_cleanup(), which is kind of
symmetric.

I understand that you're not happy with this solution, so I'll try to
come up with a different approach where dev->release() calls a master
hook to release private master data and only then frees the i3c_device
object.

I hope you're not taking my answers as a sign of arrogance, because
this is definitely not what it is. I just want to make sure you
understand why I made some (bad?) choices when designing this framework.

I really appreciate your reviews and will of course take all of your
comments into account before sending a v2.

Thanks for your time.

Boris

[toc] | [prev] | [next] | [standalone]

#1701611 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, Aug 01, 2017 at 11:30:01PM +0200, Boris Brezillon wrote:
> > > > No release type?  Oh that's bad bad bad and implies you have never
> > > > removed a device from your system as the kernel would have complained
> > > > loudly at you.  
> > > 
> > > You got me, never tried to remove a device :-). Note that these  
> > > ->release() hooks will just be dummy ones, because right now, device  
> > > resources are freed at bus destruction time.  
> > 
> > You better not have a "dummy" release hook, do that and as per the
> > kernel documentation, I get to make fun of you in public for doing that
> > :(
> 
> I'm not afraid of admitting I don't know everything, even the
> simplest things that you consider as basics for a kernel developer. You
> can make fun of me publicly if you want but that's not helping :-P.

No, I am referring to the Documentation/kobject.txt file, where it says:
	One important point cannot be overstated: every kobject must
	have a release() method, and the kobject must persist (in a
	consistent state) until that method is called. If these
	constraints are not met, the code is flawed.  Note that the
	kernel will warn you if you forget to provide a release()
	method.  Do not try to get rid of this warning by providing an
	"empty" release function; you will be mocked mercilessly by the
	kobject maintainer if you attempt this.

Sometimes I wonder why I even write documentation...

The point is, you have to release the memory the device structure "owns"
in the release callback, if not, then the model is not correct.  That's
all, please fix up your code to do so and I will be glad to review it
again.  I'm not trying to be rude here at all, but please, at the least,
read the documentation we have already first...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1701628 — Re: [RFC 2/5] i3c: Add core I3C infrastructure

On Tue, Aug 01, 2017 at 11:30:01PM +0200, Boris Brezillon wrote:
> Hi Greg,
> 
> Le Tue, 1 Aug 2017 10:51:33 -0700,
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> a écrit :
> 
> > On Tue, Aug 01, 2017 at 12:48:01PM +0200, Boris Brezillon wrote:
> > > > > +static DEFINE_MUTEX(i3c_core_lock);
> > > > > +
> > > > > +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> > > > > +{
> > > > > +	if (exclusive)
> > > > > +		down_write(&bus->lock);
> > > > > +	else
> > > > > +		down_read(&bus->lock);
> > > > > +}    
> > > > 
> > > > The "exclusive" flag is odd, and messy, and hard to understand, don't
> > > > you agree?  
> > > 
> > > I could create 2 functions, one for the exclusive lock and the other
> > > one for the shared lock.  
> > 
> > Or you could just use a simple mutex until you determine you really
> > would do better with a rw lock :)
> > 
> > > > And have you measured the difference in using a rw lock over
> > > > a normal mutex and found it to be faster?  If not, just use a normal
> > > > mutex, it's simpler and almost always better in the end.  
> > > 
> > > I did not measure the difference, but using a standard lock means
> > > serializing all I3C accesses going through a given master in the core.  
> > 
> > Which you are doing with a rw lock anyway, right?
> 
> Absolutely not. If you look more closely at the code you'll see that
> most of the time the lock is taken in read/non-exclusive mode. The only
> situations where it's taken in exclusive mode is when the operation (and
> associated command) has an impact on the bus and/or its devices.

Then you really need to document the heck out of this, as it was not
obvious at all :)

> > > Unless you see a good reason to not use a R/W lock, I'd like to keep it
> > > this way because master IPs are likely to implement advanced queuing
> > > mechanism (allows one to queue new transfers even if the master is
> > > already busy processing other requests), and serializing things at the
> > > framework level will just prevent us from using this kind of
> > > optimization.  
> > 
> > Unless you can prove otherwise, using a rw lock is almost always worse
> > than just a mutex.
> 
> Is it still true when it's taken in non-exclusive mode most of the
> time, and the time you spend in the critical section is non-negligible?
> 
> I won't pretend I know better than you do what is preferable, it's just
> that the RW lock seemed appropriate to me for the situation I tried to
> described here.

Again, measure it.  If you can't measure it, then don't use it.  Use a
simple lock instead.  Seriously, don't make it more complex until you
really have to.  It sounds like you didn't measure it at all, which
isn't good, please do so.

> > And you shouldn't have a lock for bus transactions,
> > that's just going to be a total mess.
> 
> It's not a lock for bus transactions, it's lock to protect from any
> operation that can have an impact on the bus or its devices (see the
> 'change device address' example above).

Again, document it really well please.

> > > > > +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);    
> > > > 
> > > > Multiple lines in a single sysfs file?  No.  
> > > 
> > > Okay. Would that be okay with a different separator (like a comma)?  
> > 
> > No, sysfs files are "one value per file", given you don't have any
> > documentation saying what this file is supposed to be showing, I can't
> > really judge the proper way for you to present it to userspace :)
> 
> Okay. Let's put that aside until I send a v2 with a sysfs doc.
> 
> Still, note that the "one value per file" rule does not apply to all
> sysfs files. I have 2 examples in mind (maybe they are bad examples,
> but they exist):
> 
> - /sys/class/leds/<led>/trigger returns a list of supported triggers
>   each of them is separated by a space
> - /sys/class/graphics/<fbx>/modes lists the supported video modes, one
>   per line

I'm not saying there are not bad files, but I didn't review them :)

A list of supported triggers all on one line and one you pick from it
(the power file is also the same way), is different from multiple lines.
The graphics stuff should be fixed up.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web