Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On 2018-07-12 10:08, Arnd Bergmann wrote: > On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin wrote: >> On 2018-07-11 19:12, Boris Brezillon wrote: >>> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann wrote: >>> >>> That's exactly the sort of discussion I wanted to trigger. Maybe we >>> shouldn't care and expose this use case as if it was X different I3C >>> buses (with all devices present on the bus being exposed X times to the >>> system). >> >> For I2C, this multiple masters for one bus case was retrofitted in >> the i2c-demux-pinctrl driver. It's a huge kludge with a number of >> undesirable quirks. I don't know if the circumstances for adding >> this I2C driver also applies for I3C, but it might be an argument >> in favor of the proposed extra bus object... > > From reading the documentation and git history on that driver, > it seems to be used only for static configuration, i.e. you use > one driver or the other, but don't flip between them at runtime, > right? There is a sysfs file that can be used to change master at runtime (current_master). This causes all client drivers to be reprobed, which may not be the best thing to do for every client out there... > I'm guessing that even with i3c we may have to support something > like that, as a likely scenario might be that the i3c controller is > multiplexed with a traditional i2c controller and/or gpios, but you > would not be able to perform the i3c standard secondary master > transition with the latter two because they are (by definition) not > i3c compatible. i2c-demux-pinctrl should probably not be used as template for something else, but it is a good argument for some other design IMHO... Cheers, Peter
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On 2018-07-12 10:08, Arnd Bergmann wrote: > On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin wrote: >> On 2018-07-11 19:12, Boris Brezillon wrote: >>> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann wrote: >>> >>> That's exactly the sort of discussion I wanted to trigger. Maybe we >>> shouldn't care and expose this use case as if it was X different I3C >>> buses (with all devices present on the bus being exposed X times to the >>> system). >> >> For I2C, this multiple masters for one bus case was retrofitted in >> the i2c-demux-pinctrl driver. It's a huge kludge with a number of >> undesirable quirks. I don't know if the circumstances for adding >> this I2C driver also applies for I3C, but it might be an argument >> in favor of the proposed extra bus object... > > From reading the documentation and git history on that driver, > it seems to be used only for static configuration, i.e. you use > one driver or the other, but don't flip between them at runtime, > right? There is a sysfs file that can be used to change master at runtime (current_master). This causes all client drivers to be reprobed, which may not be the best thing to do for every client out there... > I'm guessing that even with i3c we may have to support something > like that, as a likely scenario might be that the i3c controller is > multiplexed with a traditional i2c controller and/or gpios, but you > would not be able to perform the i3c standard secondary master > transition with the latter two because they are (by definition) not > i3c compatible. i2c-demux-pinctrl should probably not be used as template for something else, but it is a good argument for some other design IMHO... Cheers, Peter
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin wrote: > On 2018-07-11 19:12, Boris Brezillon wrote: >> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann wrote: >> >> That's exactly the sort of discussion I wanted to trigger. Maybe we >> shouldn't care and expose this use case as if it was X different I3C >> buses (with all devices present on the bus being exposed X times to the >> system). > > For I2C, this multiple masters for one bus case was retrofitted in > the i2c-demux-pinctrl driver. It's a huge kludge with a number of > undesirable quirks. I don't know if the circumstances for adding > this I2C driver also applies for I3C, but it might be an argument > in favor of the proposed extra bus object... >From reading the documentation and git history on that driver, it seems to be used only for static configuration, i.e. you use one driver or the other, but don't flip between them at runtime, right? I'm guessing that even with i3c we may have to support something like that, as a likely scenario might be that the i3c controller is multiplexed with a traditional i2c controller and/or gpios, but you would not be able to perform the i3c standard secondary master transition with the latter two because they are (by definition) not i3c compatible. Arnd
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Thu, Jul 12, 2018 at 6:41 AM, Peter Rosin wrote: > On 2018-07-11 19:12, Boris Brezillon wrote: >> On Wed, 11 Jul 2018 17:39:56 +0200 Arnd Bergmann wrote: >> >> That's exactly the sort of discussion I wanted to trigger. Maybe we >> shouldn't care and expose this use case as if it was X different I3C >> buses (with all devices present on the bus being exposed X times to the >> system). > > For I2C, this multiple masters for one bus case was retrofitted in > the i2c-demux-pinctrl driver. It's a huge kludge with a number of > undesirable quirks. I don't know if the circumstances for adding > this I2C driver also applies for I3C, but it might be an argument > in favor of the proposed extra bus object... >From reading the documentation and git history on that driver, it seems to be used only for static configuration, i.e. you use one driver or the other, but don't flip between them at runtime, right? I'm guessing that even with i3c we may have to support something like that, as a likely scenario might be that the i3c controller is multiplexed with a traditional i2c controller and/or gpios, but you would not be able to perform the i3c standard secondary master transition with the latter two because they are (by definition) not i3c compatible. Arnd
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon
wrote:
> On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann wrote:
>> > - 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.
>>
>> I'm not following here at all, sorry for missing prior discussion if this
>> was already explained. What is the "multiple master" case? Do you
>> mean multiple devices that are controlled by Linux and that each talk
>> to other devices on the same bus, multiple operating systems that
>> have talk to are able to own the bus with the kernel being one of
>> them, a controller that controls multiple independent buses,
>> or something else?
>
> I mean several masters connected to the same bus and all exposed to the
> same Linux instance. In this case, the question is, should we have X
> I3C buses exposed (X being the number of masters) or should we only
> have one?
>
> Having a bus represented as a separate object allows us to switch to
> the "one bus : X masters" representation if we need too.
...
>>
>> This feels a bit odd: so you have bus_type that can contain devices
>> of three (?) different device types: i3c_device_type, i3c_master_type
>> and i3c_busdev_type.
>>
>> Generally speaking, we don't have a lot of subsystems that even
>> use device_types. I assume that the i3c_device_type for a
>> device that corresponds to an endpoint on the bus, but I'm
>> still confused about the other two, and why they are part of
>> the same bus_type.
>
> i3c_busdev is just a virtual device representing the bus itself.
> i3c_master is representing the I3C master driving the bus. The reason
> for having a different type here is to avoid attaching this device to a
> driver but still being able to see the master controller as a device on
> the bus. And finally, i3c_device are all remote devices that can be
> accessed through a given i3c_master.
>
> This all comes from the design choice I made to represent the bus as a
> separate object in order to be able to share it between different
> master controllers exposed through the same Linux instance. Since
> master controllers are also remote devices for other controllers, we
> need to represent them.
Ok, so I think this is the most important question to resolve: do we
actually need to control multiple masters on a single bus from one OS
or not?
The problem that I see is that it breaks the tree abstraction that
we use in the dtb interface, in the driver model and in sysfs.
If we need to deal with a hardware bus structure like
cpu
/ \
/ \
platdev platdev
||
i3c-master i3c-master
\ /
\/
i3c-bus
/\
device device
then that abstraction no longer holds. Clearly you could build
a system like that, and if we have to support it, the i3c infrastructure
should be prepared for it, since we wouldn't be able to retrofit
it later.
What would be the point of building such a system though?
Is this for performance, failover, or something else?
IOW, what feature would we lose if we were to declare that
setup above invalid (and ensure you cannot represent it in DT)?
>> > +/**
>> > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC
>> > + *
>> > + * @len: maximum write length in bytes
>> > + *
>> > + * The maximum write length is only applicable to SDR private messages or
>> > + * extended Write CCCs (like SETXTIME).
>> > + */
>> > +struct i3c_ccc_mwl {
>> > + __be16 len;
>> > +} __packed;
>>
>> I would suggest only marking structures as __packed that are not already
>> naturally packed. Note that a side-effect of __packed is that here
>> alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient
>> unaligned access will have to access the field one byte at a time because
>> they assume that it may be misaligned.
>
> These are structure used to create packets to be sent on the wire.
> Making sure that everything is packed correctly is important, so I'm
> not sure I can remove the __packed everywhere.
I mean just the ones for which the __packed attribute only changes
the alignment of the outer structure but not the layout inside of the
structure. Alternatively, set both __packed and
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Wed, Jul 11, 2018 at 4:41 PM, Boris Brezillon
wrote:
> On Wed, 11 Jul 2018 16:01:56 +0200 Arnd Bergmann wrote:
>> > - 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.
>>
>> I'm not following here at all, sorry for missing prior discussion if this
>> was already explained. What is the "multiple master" case? Do you
>> mean multiple devices that are controlled by Linux and that each talk
>> to other devices on the same bus, multiple operating systems that
>> have talk to are able to own the bus with the kernel being one of
>> them, a controller that controls multiple independent buses,
>> or something else?
>
> I mean several masters connected to the same bus and all exposed to the
> same Linux instance. In this case, the question is, should we have X
> I3C buses exposed (X being the number of masters) or should we only
> have one?
>
> Having a bus represented as a separate object allows us to switch to
> the "one bus : X masters" representation if we need too.
...
>>
>> This feels a bit odd: so you have bus_type that can contain devices
>> of three (?) different device types: i3c_device_type, i3c_master_type
>> and i3c_busdev_type.
>>
>> Generally speaking, we don't have a lot of subsystems that even
>> use device_types. I assume that the i3c_device_type for a
>> device that corresponds to an endpoint on the bus, but I'm
>> still confused about the other two, and why they are part of
>> the same bus_type.
>
> i3c_busdev is just a virtual device representing the bus itself.
> i3c_master is representing the I3C master driving the bus. The reason
> for having a different type here is to avoid attaching this device to a
> driver but still being able to see the master controller as a device on
> the bus. And finally, i3c_device are all remote devices that can be
> accessed through a given i3c_master.
>
> This all comes from the design choice I made to represent the bus as a
> separate object in order to be able to share it between different
> master controllers exposed through the same Linux instance. Since
> master controllers are also remote devices for other controllers, we
> need to represent them.
Ok, so I think this is the most important question to resolve: do we
actually need to control multiple masters on a single bus from one OS
or not?
The problem that I see is that it breaks the tree abstraction that
we use in the dtb interface, in the driver model and in sysfs.
If we need to deal with a hardware bus structure like
cpu
/ \
/ \
platdev platdev
||
i3c-master i3c-master
\ /
\/
i3c-bus
/\
device device
then that abstraction no longer holds. Clearly you could build
a system like that, and if we have to support it, the i3c infrastructure
should be prepared for it, since we wouldn't be able to retrofit
it later.
What would be the point of building such a system though?
Is this for performance, failover, or something else?
IOW, what feature would we lose if we were to declare that
setup above invalid (and ensure you cannot represent it in DT)?
>> > +/**
>> > + * struct i3c_ccc_mwl - payload passed to SETMWL/GETMWL CCC
>> > + *
>> > + * @len: maximum write length in bytes
>> > + *
>> > + * The maximum write length is only applicable to SDR private messages or
>> > + * extended Write CCCs (like SETXTIME).
>> > + */
>> > +struct i3c_ccc_mwl {
>> > + __be16 len;
>> > +} __packed;
>>
>> I would suggest only marking structures as __packed that are not already
>> naturally packed. Note that a side-effect of __packed is that here
>> alignof(struct i3c_ccc_mwl) will be '1', and architectures without efficient
>> unaligned access will have to access the field one byte at a time because
>> they assume that it may be misaligned.
>
> These are structure used to create packets to be sent on the wire.
> Making sure that everything is packed correctly is important, so I'm
> not sure I can remove the __packed everywhere.
I mean just the ones for which the __packed attribute only changes
the alignment of the outer structure but not the layout inside of the
structure. Alternatively, set both __packed and
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Fri, Mar 30, 2018 at 9:47 AM, 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
This sounds like it can be changed later if it turns out necessary, so
that's fine to me.
> - 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.
I'm not following here at all, sorry for missing prior discussion if this
was already explained. What is the "multiple master" case? Do you
mean multiple devices that are controlled by Linux and that each talk
to other devices on the same bus, multiple operating systems that
have talk to are able to own the bus with the kernel being one of
them, a controller that controls multiple independent buses,
or something else?
> - 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.
Right, this seems like a reasonable compromise.
> Missing features in this preliminary version:
> - I3C HDR modes are not supported
> - 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
All of these seem ok to me for an initial version. It's already too
big to review properly, so leaving out stuff helps.
> +/**
> + * i3cdev_to_dev() - Returns the device embedded in @i3cdev
> + * @i3cdev: I3C device
> + *
> + * Return: a pointer to a device object.
> + */
> +struct device *i3cdev_to_dev(struct i3c_device *i3cdev)
> +{
> + return >dev;
> +}
> +EXPORT_SYMBOL_GPL(i3cdev_to_dev);
> +
> +/**
> + * dev_to_i3cdev() - Returns the I3C device containing @dev
> + * @dev: device object
> + *
> + * Return: a pointer to an I3C device object.
> + */
> +struct i3c_device *dev_to_i3cdev(struct device *dev)
> +{
> + return container_of(dev, struct i3c_device, dev);
> +}
> +EXPORT_SYMBOL_GPL(dev_to_i3cdev);
Many other subsystems just make the device structure available
to all client drivers so this can be an inline operation. Is there
a strong reason to hide it here?
> +static struct i3c_device *
> +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master,
> +const struct i3c_device_info *info,
> +const struct device_type *devtype)
> +{
> + struct i3c_device *dev;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev->common.bus = master->bus;
> + dev->dev.parent = >bus->dev;
> + dev->dev.type = devtype;
> + dev->dev.bus = _bus_type;
This feels a bit odd: so you have bus_type that can contain devices
of three (?) different device types: i3c_device_type, i3c_master_type
and i3c_busdev_type.
Generally speaking, we don't have a lot of subsystems that even
use device_types. I assume that the i3c_device_type for a
device that
Re: [PATCH v4 01/10] i3c: Add core I3C infrastructure
On Fri, Mar 30, 2018 at 9:47 AM, 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
This sounds like it can be changed later if it turns out necessary, so
that's fine to me.
> - 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.
I'm not following here at all, sorry for missing prior discussion if this
was already explained. What is the "multiple master" case? Do you
mean multiple devices that are controlled by Linux and that each talk
to other devices on the same bus, multiple operating systems that
have talk to are able to own the bus with the kernel being one of
them, a controller that controls multiple independent buses,
or something else?
> - 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.
Right, this seems like a reasonable compromise.
> Missing features in this preliminary version:
> - I3C HDR modes are not supported
> - 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
All of these seem ok to me for an initial version. It's already too
big to review properly, so leaving out stuff helps.
> +/**
> + * i3cdev_to_dev() - Returns the device embedded in @i3cdev
> + * @i3cdev: I3C device
> + *
> + * Return: a pointer to a device object.
> + */
> +struct device *i3cdev_to_dev(struct i3c_device *i3cdev)
> +{
> + return >dev;
> +}
> +EXPORT_SYMBOL_GPL(i3cdev_to_dev);
> +
> +/**
> + * dev_to_i3cdev() - Returns the I3C device containing @dev
> + * @dev: device object
> + *
> + * Return: a pointer to an I3C device object.
> + */
> +struct i3c_device *dev_to_i3cdev(struct device *dev)
> +{
> + return container_of(dev, struct i3c_device, dev);
> +}
> +EXPORT_SYMBOL_GPL(dev_to_i3cdev);
Many other subsystems just make the device structure available
to all client drivers so this can be an inline operation. Is there
a strong reason to hide it here?
> +static struct i3c_device *
> +i3c_master_alloc_i3c_dev(struct i3c_master_controller *master,
> +const struct i3c_device_info *info,
> +const struct device_type *devtype)
> +{
> + struct i3c_device *dev;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev->common.bus = master->bus;
> + dev->dev.parent = >bus->dev;
> + dev->dev.type = devtype;
> + dev->dev.bus = _bus_type;
This feels a bit odd: so you have bus_type that can contain devices
of three (?) different device types: i3c_device_type, i3c_master_type
and i3c_busdev_type.
Generally speaking, we don't have a lot of subsystems that even
use device_types. I assume that the i3c_device_type for a
device that
