[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-12-13 14:06, Thierry Reding wrote:
> On Fri, Dec 13, 2013 at 01:29:17PM +0200, Tomi Valkeinen wrote:
>> On 2013-12-13 13:22, Andrzej Hajda wrote:
>>> On 12/12/2013 01:19 PM, Thierry Reding wrote:
 On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote:
> On 2013-12-09 18:10, Thierry Reding wrote:
>
> Btw, about single linux device handling multiple VC IDs: I noticed that
> the DSI spec has an example, in which a DSI peripheral receives
> interlaced video, and the video packets containing even field have VC ID
> 0 and packets for odd field have VC ID 1. I'm not sure how relevant
> interlaced video is, but I think there's an example where having
> separate linux devices for each VC ID would be somewhat clumsy.
 Ugh... that's pretty bad.
>>> I wonder if this scenario could not be solved just
>>> by allowing range of VCs per device, for example:
>>> dsi {
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> panel_with_interleaved_vc at 0 {
>>>  reg = <0, 2>;
>>> };
>>> };
>>
>> I don't think range is good, as then you can't have, say, VC IDs 0 and
>> 2. But giving the VC IDs individually as I did in my recent reply,
>> should do the same thing as above, without ranges.
>>
>> It's still open to me if that method is good or not.
> 
> I think we can actually support both of those variants with the same
> binding. #address-cells = <1> and #size-cells = <0> means that every
> device has a single address, which matches Tomi's example:
> 
>   dsi {
>   peripheral at 0 {
>   reg = <0,  /* first VC ID */
>  2>; /* second VC ID */
>   };
>   };
> 
> The difference to what Andrzej proposed is that we additionally have
> #size-cells = <1>, which implies each entry in reg can have a "size" as
> well:
> 
>   dsi {
>   peripheral at 0 {
>   reg = <0 2>; /* VC IDs 0 and 1 */
>   };
>   };
> 
> While I could be wrong of course, I do think that the former is unlikely
> since it's much easier to support consecutive addresses than completely
> separate ones. Or perhaps it isn't. Either way I think the binding would
> support both of those cases.

Right, and even if #size-cells = <1>, you can have multiple distinct VC IDs:

/* two ranges, 0-0 and 2-2 */
reg = <0 1 2 1>;

If there's a simple to use of_xxx helper function to get the VC IDs in
any case, I think we can easily support all those cases.

But if we need to parse the reg ourselves, and have different code paths
for #size-cells 0 and 1, I'd rather stick to the simplest model. No
point in having lots of code lines to handle different #size-cells, as
we most likely just one one VC ID.

I do agree that consecutive VC IDs sounds much more probable. However,
array of VC IDs work for all cases, but a (single) range doesn't. And we
can only have 4 VC IDs, so the biggest array would be <0 1 2 3>.

 Tomi


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: 

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-12-13 13:22, Andrzej Hajda wrote:
> On 12/12/2013 01:19 PM, Thierry Reding wrote:
>> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote:
>>> On 2013-12-09 18:10, Thierry Reding wrote:
>>>
>>> Btw, about single linux device handling multiple VC IDs: I noticed that
>>> the DSI spec has an example, in which a DSI peripheral receives
>>> interlaced video, and the video packets containing even field have VC ID
>>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant
>>> interlaced video is, but I think there's an example where having
>>> separate linux devices for each VC ID would be somewhat clumsy.
>> Ugh... that's pretty bad.
> I wonder if this scenario could not be solved just
> by allowing range of VCs per device, for example:
> dsi {
> #address-cells = <1>;
> #size-cells = <1>;
> panel_with_interleaved_vc at 0 {
>  reg = <0, 2>;
> };
> };

I don't think range is good, as then you can't have, say, VC IDs 0 and
2. But giving the VC IDs individually as I did in my recent reply,
should do the same thing as above, without ranges.

It's still open to me if that method is good or not.

 Tomi


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: 

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

Hi,

On 2013-12-12 14:19, Thierry Reding wrote:

> In that case I think maybe a better thing to do would be to dynamically
> assign the VC ID's perhaps using some enumeration algorithm.

Whether or not we can configure the VC IDs depends on the HW in
question. Some allow setting VC ID, or the routing, some don't. So I
don't think we can make any assumptions about this.

> Can it initiate transactions itself? Based on I2C messages that it
> receives? If so I think that would qualify it as a DSI host. If on the
> other hand it only routes DSI packets to downstream ports based on
> registers configured via I2C, then it is not a DSI host, because it
> cannot create DSI packets itself.

Yes, it can initiate DSI transactions. I agree with the above.

> Okay, so we've talked about many of these issues now and I appreciate
> you providing all that feedback. In an attempt to move things forward
> can I propose that we start off with something simple so we can get
> started and gather some experience with the matter.

Agreed.

> The most difficult part will be the DT bindings. I've sent a proposal a
> while ago[0]. It's explicitly very terse and I don't think it would
> prevent any of the more advanced setups involving hubs in the future.
> Basically it says you can have a DSI host and up to four children

Well. I don't know... On what level do we describe things in the DT? If
a host and a DSI peripheral is described based on the physical link,
then a DSI host can have only one child.

If we have a DSI hub, it's a different thing as discussed, and we can
ignore it for now.

So the case where there could be multiple children, is if a single DSI
peripheral accepts multiple VC IDs. In that case we'd have:

dsi-host {
#address-cells = <1>;
#size-cells = <0>;

peripheral at 0 {
compatible = "...";
reg = <0>;
};

peripheral at 1 {
compatible = "...";
reg = <1>;
};
};

I think you already expressed earlier that you think the above is ok,
and we could as well have separate drivers if a DSI peripheral accepts
multiple VC IDs.

I'm still not sure about that. I think that's more of describing logical
setup, not the hardware. But, then again, maybe that doesn't matter, as
long as the real hardware setup can be deduced from the description. In
the above case it can: we know DSI is one-to-one link on the wire level,
which means the two peripherals described above must use that same link,
and thus must be in the same DSI hardware peripheral.



I wanted to avoid the DSI hub discussions, but, here goes again. You can
ignore this is you're too tired already =).

If we supported DSI hubs, and the hub had two output DSI ports, and we
had a peripheral answering to two VC IDs attached to the hub. In that
case we would need separate dsi-host sub-nodes for the two DSI ports,
instead of having the peripherals directly under the hub node, as you
had in your reply.

I.e. we could have something like:

dsi-host {
#address-cells = <1>;
#size-cells = <0>;

/* hub is controlled via VC ID 0 */
hub at 0 {
compatible = "...";
reg = <0>;
};

/* video data is sent to VC ID 1 */
hub at 1 {
compatible = "...";
reg = <1>;

/* port 0 of the hub */
dsi-host at 0 {
reg = <0>;

/* panel answers to VC ID 0 */
panel at 0 {
reg = <0>;
};

/* panel answers also to VC ID 1 */
panel at 1 {
reg = <1>;
};
};

/* port 1 of the hub */
dsi-host at 1 {
reg = <0>;
/* nothing attached to hub's second port in this case */
};
};
};

Well, that gets messy, and is probably rather unlikely scenario. But I
think that shows (at least to me) that having separate nodes for each VC
ID is problematic. I added the hub's peripherals to hub at 1 node, but
hub at 0 could also make sense.

Adding multiple values to reg field makes it a bit cleaner:

dsi-host {
#address-cells = <1>;
#size-cells = <0>;

/* hub answers to VC ID 0 and 1 */
hub at 0 {
compatible = "...";
reg = <0 1>;

/* port 0 of the hub */
dsi-host at 0 {
reg = <0>;

/* panel answers to VC ID 0 and 1 */
panel at 0 {
reg = <0 1>;
};
};

/* port 1 of the hub */
dsi-host at 1 {
reg = <0>;
/* nothing attached

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Fri, Dec 13, 2013 at 01:29:17PM +0200, Tomi Valkeinen wrote:
> On 2013-12-13 13:22, Andrzej Hajda wrote:
> > On 12/12/2013 01:19 PM, Thierry Reding wrote:
> >> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote:
> >>> On 2013-12-09 18:10, Thierry Reding wrote:
> >>>
> >>> Btw, about single linux device handling multiple VC IDs: I noticed that
> >>> the DSI spec has an example, in which a DSI peripheral receives
> >>> interlaced video, and the video packets containing even field have VC ID
> >>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant
> >>> interlaced video is, but I think there's an example where having
> >>> separate linux devices for each VC ID would be somewhat clumsy.
> >> Ugh... that's pretty bad.
> > I wonder if this scenario could not be solved just
> > by allowing range of VCs per device, for example:
> > dsi {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > panel_with_interleaved_vc at 0 {
> >  reg = <0, 2>;
> > };
> > };
> 
> I don't think range is good, as then you can't have, say, VC IDs 0 and
> 2. But giving the VC IDs individually as I did in my recent reply,
> should do the same thing as above, without ranges.
> 
> It's still open to me if that method is good or not.

I think we can actually support both of those variants with the same
binding. #address-cells = <1> and #size-cells = <0> means that every
device has a single address, which matches Tomi's example:

dsi {
peripheral at 0 {
reg = <0,  /* first VC ID */
   2>; /* second VC ID */
};
};

The difference to what Andrzej proposed is that we additionally have
#size-cells = <1>, which implies each entry in reg can have a "size" as
well:

dsi {
peripheral at 0 {
reg = <0 2>; /* VC IDs 0 and 1 */
};
};

While I could be wrong of course, I do think that the former is unlikely
since it's much easier to support consecutive addresses than completely
separate ones. Or perhaps it isn't. Either way I think the binding would
support both of those cases.

I'll see if I can come up with some wording to make that a part of the
binding and perhaps mention that only one-to-one relationships are taken
into account in the binding.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Andrzej Hajda]

On 12/12/2013 01:19 PM, Thierry Reding wrote:
> On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote:
>> On 2013-12-09 18:10, Thierry Reding wrote:
>>
>> Btw, about single linux device handling multiple VC IDs: I noticed that
>> the DSI spec has an example, in which a DSI peripheral receives
>> interlaced video, and the video packets containing even field have VC ID
>> 0 and packets for odd field have VC ID 1. I'm not sure how relevant
>> interlaced video is, but I think there's an example where having
>> separate linux devices for each VC ID would be somewhat clumsy.
> Ugh... that's pretty bad.
I wonder if this scenario could not be solved just
by allowing range of VCs per device, for example:
dsi {
#address-cells = <1>;
#size-cells = <1>;
panel_with_interleaved_vc at 0 {
 reg = <0, 2>;
};
};

Regards
Andrzej

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Tue, Dec 10, 2013 at 11:19:54AM +0200, Tomi Valkeinen wrote:
> On 2013-12-09 18:10, Thierry Reding wrote:
> 
> Btw, about single linux device handling multiple VC IDs: I noticed that
> the DSI spec has an example, in which a DSI peripheral receives
> interlaced video, and the video packets containing even field have VC ID
> 0 and packets for odd field have VC ID 1. I'm not sure how relevant
> interlaced video is, but I think there's an example where having
> separate linux devices for each VC ID would be somewhat clumsy.

Ugh... that's pretty bad.

> >> That call would go to the DSI hub driver. It knows how it routes the
> >> packets (the routing configuration is either hardcoded or passed via DT
> >> data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and
> >> the packet data.
> > 
> > So it is the DSI hub driver that translates VC 0 to VC 2? How does it
> > know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass
> > in a different VC as panel 1?
> 
> I'm not sure what you mean. The routing information is passed via DT
> data and then configured to the hub HW, or is hardcoded in the hub hardware.

Okay, that's what I meant.

> > It's really the same thing. If you define VC IDs in a link local manner
> > you don't need them at all. What's the point in having them if you can
> > only reach a single device anyway.
> 
> You have to define the VC IDs somewhere in link-local manner for the HW
> to work. The panels in my example respond to VC ID 0, even if from SW
> (DSI host's) point of view they are addressed with VC ID 2 or 3. Both VC
> ID 0 and VC IDs 2 and 3 need to be programmed to the hub and possibly to
> the panel.

In that case I think maybe a better thing to do would be to dynamically
assign the VC ID's perhaps using some enumeration algorithm.

Or perhaps DSI devices are assigned fixed numbers as defined by software
(DT) and these are then used to program hubs.

> > From a software perspective we can always only describe devices from the
> > viewpoint where things leave software control. In this case the last
> > point of control is the DSI host. Therefore if we want to send a packet
> > to any device connected to the DSI host (whether directly or through a
> > hub), we need to pass some VC ID to the DSI host as a destination. That
> > number needs to be known up front.
> > 
> > So either you hide that information in some software framework, such as
> > you seem to be doing given your example above, and make the panel driver
> > call into the hub driver and have the hub driver translate the VC ID to
> > a different one.
> > 
> > The alternative would be to pass the correct address to the DSI host
> > directly. Either way, though, whatever is written into the registers of
> > the DSI host will end up the same, won't it?
> 
> Yes.
> 
> > You always address peripherals from the DSI host's viewpoint.
> 
> Yes. Except, of course, if the hub can be accessed with i2c also, in
> which case you need some other addressing mechanism.

Well, you address DSI peripherals from the DSI host's viewpoint. If they
have an additional I2C configuration interface then that isn't directly
related to DSI. Even if both devices are physically the same, the Linux
representation is a logical one, so you can very well have two types of
logical devices referring to the same physical device.

> >> The panels here should should be inside the hub node for DSI case. They
> >> are connected to the hub, not the SoC's DSI, and need the hub to be
> >> powered up and configured to work. And the hub should contain two output
> >> DSI busses, each having one panel with VC ID 0. Probably something like:
> >>
> >> dsi {
> >>hub at 0 {
> >>dsi at 0 {
> >>panel at 0 {
> >>};
> >>};
> >>
> >>dsi at 1 {
> >>panel at 0 {
> >>};
> >>};
> >>};
> >> };
> >>
> >> That kind of ruins the idea of representing the panels as DSI devices
> >> with VC IDs of 2 and 3. I don't know how that should be managed then...
> > 
> > Why does that ruin it? You still address panel at 0 using VC 2 and panel at 
> > 1
> > using VC 3, don't you? Therefore I see no reason why they shouldn't be
> > represented as DSI devices with their respective VC IDs.
> 
> The DT data should reflect the hardware. In this case both panels are
> addressed by the DSI hub with VC ID 0. That information has to be
> somewhere, and logical place for it is to define the panels as having VC
> ID 0.

It seems to me like for these cases we'd need some sort of automatic
enumeration (as I mentioned above) or some mechanism to translate from
lower busses to upper busses.

Perhaps something like this would work:

dsi {
hub at 0 {
/*
 * means the hub has VC ID 0 on the primary bus
 * which translates to global VC ID 0

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-12-09 18:10, Thierry Reding wrote:

Btw, about single linux device handling multiple VC IDs: I noticed that
the DSI spec has an example, in which a DSI peripheral receives
interlaced video, and the video packets containing even field have VC ID
0 and packets for odd field have VC ID 1. I'm not sure how relevant
interlaced video is, but I think there's an example where having
separate linux devices for each VC ID would be somewhat clumsy.

>> That call would go to the DSI hub driver. It knows how it routes the
>> packets (the routing configuration is either hardcoded or passed via DT
>> data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and
>> the packet data.
> 
> So it is the DSI hub driver that translates VC 0 to VC 2? How does it
> know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass
> in a different VC as panel 1?

I'm not sure what you mean. The routing information is passed via DT
data and then configured to the hub HW, or is hardcoded in the hub hardware.

> It's really the same thing. If you define VC IDs in a link local manner
> you don't need them at all. What's the point in having them if you can
> only reach a single device anyway.

You have to define the VC IDs somewhere in link-local manner for the HW
to work. The panels in my example respond to VC ID 0, even if from SW
(DSI host's) point of view they are addressed with VC ID 2 or 3. Both VC
ID 0 and VC IDs 2 and 3 need to be programmed to the hub and possibly to
the panel.

> From a software perspective we can always only describe devices from the
> viewpoint where things leave software control. In this case the last
> point of control is the DSI host. Therefore if we want to send a packet
> to any device connected to the DSI host (whether directly or through a
> hub), we need to pass some VC ID to the DSI host as a destination. That
> number needs to be known up front.
> 
> So either you hide that information in some software framework, such as
> you seem to be doing given your example above, and make the panel driver
> call into the hub driver and have the hub driver translate the VC ID to
> a different one.
> 
> The alternative would be to pass the correct address to the DSI host
> directly. Either way, though, whatever is written into the registers of
> the DSI host will end up the same, won't it?

Yes.

> You always address peripherals from the DSI host's viewpoint.

Yes. Except, of course, if the hub can be accessed with i2c also, in
which case you need some other addressing mechanism.

>> The panels here should should be inside the hub node for DSI case. They
>> are connected to the hub, not the SoC's DSI, and need the hub to be
>> powered up and configured to work. And the hub should contain two output
>> DSI busses, each having one panel with VC ID 0. Probably something like:
>>
>> dsi {
>>  hub at 0 {
>>  dsi at 0 {
>>  panel at 0 {
>>  };
>>  };
>>
>>  dsi at 1 {
>>  panel at 0 {
>>  };
>>  };
>>  };
>> };
>>
>> That kind of ruins the idea of representing the panels as DSI devices
>> with VC IDs of 2 and 3. I don't know how that should be managed then...
> 
> Why does that ruin it? You still address panel at 0 using VC 2 and panel at 1
> using VC 3, don't you? Therefore I see no reason why they shouldn't be
> represented as DSI devices with their respective VC IDs.

The DT data should reflect the hardware. In this case both panels are
addressed by the DSI hub with VC ID 0. That information has to be
somewhere, and logical place for it is to define the panels as having VC
ID 0.

The fact that the DSI host can send a packet to VC ID 3, and the hub
routes it to the second panel, is then part of the hub's operation.

>> But this DT example shows how DSI is really a one-to-one bus in the
>> hardware.
> 
> How is that one-to-one? The DSI hub connects to two peripherals, that's
> one-to-two.

Well, we keep going around this subject.

I'm speaking from the HW perspective. DSI link is a point to point link.
Each DSI link is totally independent, having its own speed and
configuration. The two output ports on the hub are as independent as two
DSI hosts on a SoC. Any routing that happens between DSI links is
totally up to the hub.

I agree that from SW perspective we can say the DSI hub connects to two
peripherals, addressed with VC2 and VC3. But we can't ignore the HW
side, as that information is required to program the hardware settings
properly (e.g. the routing configuration in the hub).

>> With DSI you always have a hub, which can translate the addresses. With
>> my example above, both panels are identical and they have the same VC ID
>> of 0.
> 
> No. If they had the same VC ID how do you address them differently? When
> the transactions originate at the DSI host, you have to tell it where
> they should go. If you pass them the same VC ID they will go to the 

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Mon, Dec 09, 2013 at 05:05:20PM +0200, Tomi Valkeinen wrote:
> On 2013-12-09 15:10, Thierry Reding wrote:
[...]
> > But even if you have a tree of one-to-one links, you still need some way
> > to address the individual nodes in the tree. The VC ID is the only way
> > by which you can address a node. I don't see how you can possible send
> > packets to more than one node if you keep sending packets to the same
> > address. Where does the missing information magically come from?
> 
> From the DSI hub.
> 
> In the example case below, let's say the DSI panel 1 driver is told to
> send a configuration packet to the panel. The panel driver would call a
> dsi_transmit op, giving as arguments the VC number 0, and the packet data.
> 
> That call would go to the DSI hub driver. It knows how it routes the
> packets (the routing configuration is either hardcoded or passed via DT
> data), and then calls dsi_transmit op on SoC DSI, with VC number 2 and
> the packet data.

So it is the DSI hub driver that translates VC 0 to VC 2? How does it
know that VC 0 should be VC 2 but not VC 3? Does the panel 2 driver pass
in a different VC as panel 1?

> >> For the sake of discussion, let's consider a simple DSI hub setup:
> >>
> >> SoC DSI -> DSI Hub -> DSI panel 1
> >>-> DSI panel 2
> >>
> >> The hub would use, say VC0 for hub configuration, and it'd route VC2 to
> >> panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would
> >> translate the VC ID accordingly.
> >>
> >> How would you represent this in Linux?
> > 
> > You keep saying that devices use various VC IDs (VC0 for hub config, VC2
> > and VC3 for panels 1 and 2 in this case). But those are exactly the
> > addresses. You've got to have some way within the kernel to store those.
> > 
> > Given the limited address space of DSI there's no way to accurately
> > represent the hierarchy of the above in the bus/device numbering. But
> > that doesn't mean you can't assign addresses (VC IDs) to the devices. In
> > fact you've given examples yourself.
> 
> Yes, but I guess the difference in our views is that I see the VC IDs as
> "link local" and routing is done by the hubs as they see fit. In other
> words, if I'm not mistaken, you'd have a Linux DSI bus with three
> devices in the bus, each having its own VC ID, whereas I'd have a DSI
> "link" between the SoC and the hub, without any general information
> about the VC IDs used, and two additional DSI links, from the DSI hub to
> each panel.

It's really the same thing. If you define VC IDs in a link local manner
you don't need them at all. What's the point in having them if you can
only reach a single device anyway.

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-12-09 15:10, Thierry Reding wrote:

>> No idea. But I have worked with a device, that used VC0 for the device's
>> configuration registers, VC1 for buffer commands (the device had a
>> framebuffer), VC2 and VC3 for panels connected to that device.
> 
> Well, VC2 and VC3 certainly sound like logically separate devices to me.
> If they are only connected to the device it sounds like they aren't part
> of the device at all.

Yes, the two panels are obviously separate devices.

> One could even argue that a device's configuration and the framebuffer
> are logically separate and therefore it wouldn't be a problem to address
> them as separate devices.

True. But one could also argue that handling them with separate linux
devices just causes unnecessary complexity.

 So I think we should consider DSI as a one-to-one link, and let the DSI
 peripheral manage the VC IDs as it wants.
>>>
>>> But doing so would prevent us from supporting setups where we have two
>>> separate peripherals with different VC numbers.
>>
>> No it wouldn't. We could still communicate with the extra peripherals
>> via the hub device. What I was trying to say is that we shouldn't think
>> or model DSI with multiple devices as multiple devices connected to the
>> same DSI bus. Instead, it should be seen as a tree of one-to-one links
>> (as it is in the HW).
> 
> But even if you have a tree of one-to-one links, you still need some way
> to address the individual nodes in the tree. The VC ID is the only way
> by which you can address a node. I don't see how you can possible send
> packets to more than one node if you keep sending packets to the same
> address. Where does the missing information magically come from?

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Mon, Dec 09, 2013 at 01:34:06PM +0200, Tomi Valkeinen wrote:
> On 2013-12-06 14:54, Thierry Reding wrote:
> > On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote:
> >> On 2013-11-27 12:54, Thierry Reding wrote:
> >>
>  I am not sure about hardwiring devices to virtual channels.
>  There could be devices which uses more than one virtual channel,
>  in fact exynos-drm docs suggests that such hardware exists.
> >>>
> >>> In that case, why not make them two logically separate devices within
> >>> the kernel. We need the channel number so that the device can be
> >>> addressed in the first place, so I don't see what's wrong with using
> >>> that number in the device's name.
> >>>
> >>> The whole point of this patch is to add MIPI DSI bus infrastructure, and
> >>> the virtual channel is one of the fundamental aspects of that bus, so I
> >>> think we need to make it an integral part of the implementation.
> >>
> >> (I speak here more in the context of OMAP display subsystem and CDF, and
> >> this might not be totally applicable to DRM).
> >>
> >> In my opinion, DSI shouldn't be though of in the same way as other buses.
> >>
> >> In most of the cases, there's just one DSI peripheral connected. This
> >> peripheral may answer to multiple DSI VC IDs, though. I don't like the
> >> idea of having to split one device driver into multiple drivers, just to
> >> manage multiple DSI VC IDs.
> > 
> > If they respond to multiple VCs, then I suppose they would be logically
> > separate devices, even if only a single physical device. What would be
> > the point of addressing them individually if they are just the same
> > device?
> 
> No idea. But I have worked with a device, that used VC0 for the device's
> configuration registers, VC1 for buffer commands (the device had a
> framebuffer), VC2 and VC3 for panels connected to that device.

Well, VC2 and VC3 certainly sound like logically separate devices to me.
If they are only connected to the device it sounds like they aren't part
of the device at all.

One could even argue that a device's configuration and the framebuffer
are logically separate and therefore it wouldn't be a problem to address
them as separate devices.

> >> So I think we should consider DSI as a one-to-one link, and let the DSI
> >> peripheral manage the VC IDs as it wants.
> > 
> > But doing so would prevent us from supporting setups where we have two
> > separate peripherals with different VC numbers.
> 
> No it wouldn't. We could still communicate with the extra peripherals
> via the hub device. What I was trying to say is that we shouldn't think
> or model DSI with multiple devices as multiple devices connected to the
> same DSI bus. Instead, it should be seen as a tree of one-to-one links
> (as it is in the HW).

But even if you have a tree of one-to-one links, you still need some way
to address the individual nodes in the tree. The VC ID is the only way
by which you can address a node. I don't see how you can possible send
packets to more than one node if you keep sending packets to the same
address. Where does the missing information magically come from?

> For the sake of discussion, let's consider a simple DSI hub setup:
> 
> SoC DSI -> DSI Hub -> DSI panel 1
>-> DSI panel 2
> 
> The hub would use, say VC0 for hub configuration, and it'd route VC2 to
> panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would
> translate the VC ID accordingly.
> 
> How would you represent this in Linux?

You keep saying that devices use various VC IDs (VC0 for hub config, VC2
and VC3 for panels 1 and 2 in this case). But those are exactly the
addresses. You've got to have some way within the kernel to store those.

Given the limited address space of DSI there's no way to accurately
represent the hierarchy of the above in the bus/device numbering. But
that doesn't mean you can't assign addresses (VC IDs) to the devices. In
fact you've given examples yourself.

> How about the case where the DSI hub uses i2c for control (DSI used
> only for video data), and the messages to DSI panels are sent via i2c?

Well you still have to send the video data somewhere, right? Each video
packet still has a destination VC ID, independent of whether that same
bus is used for configuration or not.

Of course it's more difficult to represent these devices than if they
were accessed using only a single bus. Perhaps it could help to look at
Linux devices not so much as the equivalent of a physical device, but
rather as a representation of the logical functionality of that device.

In your example above, the DSI hub would be a single physical device.
But it also has two logically separate functions. It can be configured
and it passes through video to one or more panels. Similarily for the
panels, where each has an I2C control input and a DSI video input.

The I2C portion can be a device separate from the DSI portion. That way
your problem becomes one of interconnecting two "device

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-12-06 14:54, Thierry Reding wrote:
> On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote:
>> On 2013-11-27 12:54, Thierry Reding wrote:
>>
 I am not sure about hardwiring devices to virtual channels.
 There could be devices which uses more than one virtual channel,
 in fact exynos-drm docs suggests that such hardware exists.
>>>
>>> In that case, why not make them two logically separate devices within
>>> the kernel. We need the channel number so that the device can be
>>> addressed in the first place, so I don't see what's wrong with using
>>> that number in the device's name.
>>>
>>> The whole point of this patch is to add MIPI DSI bus infrastructure, and
>>> the virtual channel is one of the fundamental aspects of that bus, so I
>>> think we need to make it an integral part of the implementation.
>>
>> (I speak here more in the context of OMAP display subsystem and CDF, and
>> this might not be totally applicable to DRM).
>>
>> In my opinion, DSI shouldn't be though of in the same way as other buses.
>>
>> In most of the cases, there's just one DSI peripheral connected. This
>> peripheral may answer to multiple DSI VC IDs, though. I don't like the
>> idea of having to split one device driver into multiple drivers, just to
>> manage multiple DSI VC IDs.
> 
> If they respond to multiple VCs, then I suppose they would be logically
> separate devices, even if only a single physical device. What would be
> the point of addressing them individually if they are just the same
> device?

No idea. But I have worked with a device, that used VC0 for the device's
configuration registers, VC1 for buffer commands (the device had a
framebuffer), VC2 and VC3 for panels connected to that device.

Of course, that kind of devices are probably (?) rare, so it may not be
a big issue.

>> In some rare cases (I've never seen one in production) there may be a
>> DSI hub, and one or two DSI peripherals behind it. But the hub is not
>> really a hub, but a router, and the router requires configuration. The
>> case here is not really one DSI bus with two or more peripherals, but
>> two or more independent 1-to-1 DSI buses.
> 
> From the CPUs perspective there's still only one bus. Or perhaps to put
> it another way, there is a single address space, because that's what's
> implied by how the virtual channel number is defined.
> 
> So you would still represent the DSI hub as one device with a specific
> VC and the two peripherals behind it with different VC numbers. The VC
> number space simply doesn't allow much else to be done.
> 
>> I have never seen a pure hub, i.e. something that would just
>> forward/broadcast the DSI packet to two or more DSI peripherals.
>>
>> So I think we should consider DSI as a one-to-one link, and let the DSI
>> peripheral manage the VC IDs as it wants.
> 
> But doing so would prevent us from supporting setups where we have two
> separate peripherals with different VC numbers.

No it wouldn't. We could still communicate with the extra peripherals
via the hub device. What I was trying to say is that we shouldn't think
or model DSI with multiple devices as multiple devices connected to the
same DSI bus. Instead, it should be seen as a tree of one-to-one links
(as it is in the HW).

For the sake of discussion, let's consider a simple DSI hub setup:

SoC DSI -> DSI Hub -> DSI panel 1
   -> DSI panel 2

The hub would use, say VC0 for hub configuration, and it'd route VC2 to
panel 1 and VC3 to panel2. Both panels would use VC0, so the hub would
translate the VC ID accordingly.

How would you represent this in Linux? How about the case where the DSI
hub uses i2c for control (DSI used only for video data), and the
messages to DSI panels are sent via i2c?

>> I don't see any benefit in trying to create a similar linux bus for
>> DSI as we have for, say, i2c or spi.
> 
>>> Without that fixed virtual channel number we can't use the device in the
>>> first place. How are you going to address the device if you don't know
>>> its virtual channel?
>>
>> The DSI peripheral driver must know the VC IDs. Often they are hardcoded
>> values, and they can be defined in the driver code.
> 
> When you say "often", can you make a guarantee that it will always be
> the case? I don't think so. I can easily imagine somebody making the VC
> configurable via straps, which would come in handy if you wanted to use
> the same IC multiple times within the same design.

Of course I can't guarantee things like that. But I have never seen a
DSI device that can be configured in such a way. Have you?

> In that case you still need some way to specify the VC on a per-board
> basis, either in DT or "platform data".

I'm not saying we should prevent that kind of device from working. But
if all the known devices use a fixed VC ID scheme, maybe there's not
much point in requiring to enter the VC ID manually in all the dts
files. If there's a device with configurable VC ID, it can still be
supported by

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Thu, Dec 05, 2013 at 04:37:39PM +0200, Tomi Valkeinen wrote:
> On 2013-11-27 12:54, Thierry Reding wrote:
> 
> >> I am not sure about hardwiring devices to virtual channels.
> >> There could be devices which uses more than one virtual channel,
> >> in fact exynos-drm docs suggests that such hardware exists.
> > 
> > In that case, why not make them two logically separate devices within
> > the kernel. We need the channel number so that the device can be
> > addressed in the first place, so I don't see what's wrong with using
> > that number in the device's name.
> > 
> > The whole point of this patch is to add MIPI DSI bus infrastructure, and
> > the virtual channel is one of the fundamental aspects of that bus, so I
> > think we need to make it an integral part of the implementation.
> 
> (I speak here more in the context of OMAP display subsystem and CDF, and
> this might not be totally applicable to DRM).
> 
> In my opinion, DSI shouldn't be though of in the same way as other buses.
> 
> In most of the cases, there's just one DSI peripheral connected. This
> peripheral may answer to multiple DSI VC IDs, though. I don't like the
> idea of having to split one device driver into multiple drivers, just to
> manage multiple DSI VC IDs.

If they respond to multiple VCs, then I suppose they would be logically
separate devices, even if only a single physical device. What would be
the point of addressing them individually if they are just the same
device?

> In some rare cases (I've never seen one in production) there may be a
> DSI hub, and one or two DSI peripherals behind it. But the hub is not
> really a hub, but a router, and the router requires configuration. The
> case here is not really one DSI bus with two or more peripherals, but
> two or more independent 1-to-1 DSI buses.

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Tomi Valkeinen]

On 2013-11-27 12:54, Thierry Reding wrote:

>> I am not sure about hardwiring devices to virtual channels.
>> There could be devices which uses more than one virtual channel,
>> in fact exynos-drm docs suggests that such hardware exists.
> 
> In that case, why not make them two logically separate devices within
> the kernel. We need the channel number so that the device can be
> addressed in the first place, so I don't see what's wrong with using
> that number in the device's name.
> 
> The whole point of this patch is to add MIPI DSI bus infrastructure, and
> the virtual channel is one of the fundamental aspects of that bus, so I
> think we need to make it an integral part of the implementation.

(I speak here more in the context of OMAP display subsystem and CDF, and
this might not be totally applicable to DRM).

In my opinion, DSI shouldn't be though of in the same way as other buses.

In most of the cases, there's just one DSI peripheral connected. This
peripheral may answer to multiple DSI VC IDs, though. I don't like the
idea of having to split one device driver into multiple drivers, just to
manage multiple DSI VC IDs.

In some rare cases (I've never seen one in production) there may be a
DSI hub, and one or two DSI peripherals behind it. But the hub is not
really a hub, but a router, and the router requires configuration. The
case here is not really one DSI bus with two or more peripherals, but
two or more independent 1-to-1 DSI buses.

I have never seen a pure hub, i.e. something that would just
forward/broadcast the DSI packet to two or more DSI peripherals.

So I think we should consider DSI as a one-to-one link, and let the DSI
peripheral manage the VC IDs as it wants. I don't see any benefit in
trying to create a similar linux bus for DSI as we have for, say, i2c or
spi.

>> I can also imagine scenarios when dynamic virtual channel (re-)association
>> can be useful and DSI specs are not against it AFAIK.
> 
> How is that going to work? There's no hotplug support or similar in DSI,
> so why would anyone want to reassign virtual channels.
> 
> Supposing even that we wanted to support this eventually, I think a more
> appropriate solution would be to completely remove the device and add a
> new one, because that also takes care of keeping the channel number
> embedded within the struct mipi_dsi_device up to date.
> 
>> reg property means device is at fixed virtual channel.
>> DSI specs says nothing about it IMHO.
> 
> Without that fixed virtual channel number we can't use the device in the
> first place. How are you going to address the device if you don't know
> its virtual channel?

The DSI peripheral driver must know the VC IDs. Often they are hardcoded
values, and they can be defined in the driver code. I don't see why the
DSI host driver would need to know the VC IDs, as it can't really do
anything independently with the peripheral anyway. All the transactions
should be started by the DSI peripheral driver.

 Tomi


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: 

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Fri, Nov 29, 2013 at 04:28:45PM +0100, Andrzej Hajda wrote:
> On 11/27/2013 11:54 AM, Thierry Reding wrote:
> > On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote:
> > [...]
>  +/**
>  + * mipi_dsi_device_register - register a DSI device
>  + * @dev: DSI device we're registering
>  + */
>  +int mipi_dsi_device_register(struct mipi_dsi_device *dev,
>  +  struct mipi_dsi_bus *bus)
>  +{
>  +device_initialize(&dev->dev);
>  +
>  +dev->bus = bus;
>  +dev->dev.bus = &mipi_dsi_bus_type;
>  +dev->dev.parent = bus->dev;
>  +dev->dev.type = &mipi_dsi_dev_type;
>  +
>  +if (dev->id != -1)
> >>> Does that case actually make sense in the context of DSI? There's a
> >>> defined hierarchy in DSI, so shouldn't we construct the names in a more
> >>> hierarchical way? I'm not sure if the ID is useful at all, perhaps it
> >>> should really be the virtual channel?
> >>>
> >>> The patch that I proposed used the following to make up the device name:
> >>>
> >>>   dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel);
> >>>
> >>> That has the advantage of reflecting the bus topology.
> >>>
> >>> It seems like your code was copied mostly from platform_device, for
> >>> which there's no well-defined topology and therefore having an ID makes
> >>> sense to differentiate between multiple instances of the same device.
> >>> But for DSI any host/bus can only have a single device with a given
> >>> channel, so that makes for a pretty good for unique name.
> >> Code was based on mipi_dbi_bus.c from CDF.
> >> I am not sure about hardwiring devices to virtual channels.
> >> There could be devices which uses more than one virtual channel,
> >> in fact exynos-drm docs suggests that such hardware exists.
> > In that case, why not make them two logically separate devices within
> > the kernel. We need the channel number so that the device can be
> > addressed in the first place, so I don't see what's wrong with using
> > that number in the device's name.
> >
> > The whole point of this patch is to add MIPI DSI bus infrastructure, and
> > the virtual channel is one of the fundamental aspects of that bus, so I
> > think we need to make it an integral part of the implementation.
> There are already devices which IMO do not fit to this model, for example
> TC358710 [1] can work as DSI hub, which I suppose uses two or three virtual
> channels of the main DSI-host and performs "automatic virtual channel
> translation".
> 
> [1]
> http://www.toshiba.com/taec/components/ProdBrief/10F02_TC358710_ProdBrief.pdf

It's not apparent from that product brief whether or not the hub itself
can be addressed. But that doesn't really matter either. If it can be
addressed, it will respond to one of the four possible virtual channels
while forwarding everything destined at another virtual channel to one
of the downstream ports.

If the hub cannot be addressed, then all downstream ports still need to
be addressed using a valid virtual channel number. In order to do so we
still need that virtual channel to be associated with each peripheral,
otherwise we don't know who to send packets to.

>  +struct mipi_dsi_device {
>  +char name[MIPI_DSI_NAME_SIZE];
>  +int id;
>  +struct device dev;
>  +
>  +const struct mipi_dsi_device_id *id_entry;
>  +struct mipi_dsi_bus *bus;
>  +struct videomode vm;
> >>> Why is the videomode part of this structure? What if a device supports
> >>> more than a single mode?
> >> This is the current video mode. If we want to change mode,
> >> we call:
> >> ops->set_stream(false);
> >> dev->vm =...new mode
> >> ops->set_stream(true);
> > I don't think that needs to be part of the DSI device. Rather it seems
> > to me that whatever object type is implemented by the DSI device driver
> > should have subsystem-specific code to do this.
> >
> > For example, if a DSI device is a bridge, then it will implement a
> > drm_bridge object, and in that case the drm_bridge_funcs.mode_set()
> > would be the equivalent of this.
> I think mode setting is common for most DSI-hosts, at least
> for all having video mode.

I agree, but I don't think we need to make it an integral part of the
DSI peripheral or DSI host structures. Most of the drivers for a DSI
host or DSI peripheral will have some subsystem specific way to deal
with that anyway, so I don't think we're doing us a favour by adding an
extra layer here.

>  +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \
>  +({\
>  +const u8 d[] = { seq };\
>  +BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for 
>  stack");\
>  +mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\
>  +})
>  +
>  +#define mipi_dsi_dcs_write_static_seq(dev, channel, seq...) \
>  +({\
>  +static const u8

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Andrzej Hajda]

On 11/27/2013 11:54 AM, Thierry Reding wrote:
> On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote:
> [...]
 +/**
 + * mipi_dsi_device_register - register a DSI device
 + * @dev: DSI device we're registering
 + */
 +int mipi_dsi_device_register(struct mipi_dsi_device *dev,
 +struct mipi_dsi_bus *bus)
 +{
 +  device_initialize(&dev->dev);
 +
 +  dev->bus = bus;
 +  dev->dev.bus = &mipi_dsi_bus_type;
 +  dev->dev.parent = bus->dev;
 +  dev->dev.type = &mipi_dsi_dev_type;
 +
 +  if (dev->id != -1)
>>> Does that case actually make sense in the context of DSI? There's a
>>> defined hierarchy in DSI, so shouldn't we construct the names in a more
>>> hierarchical way? I'm not sure if the ID is useful at all, perhaps it
>>> should really be the virtual channel?
>>>
>>> The patch that I proposed used the following to make up the device name:
>>>
>>> dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel);
>>>
>>> That has the advantage of reflecting the bus topology.
>>>
>>> It seems like your code was copied mostly from platform_device, for
>>> which there's no well-defined topology and therefore having an ID makes
>>> sense to differentiate between multiple instances of the same device.
>>> But for DSI any host/bus can only have a single device with a given
>>> channel, so that makes for a pretty good for unique name.
>> Code was based on mipi_dbi_bus.c from CDF.
>> I am not sure about hardwiring devices to virtual channels.
>> There could be devices which uses more than one virtual channel,
>> in fact exynos-drm docs suggests that such hardware exists.
> In that case, why not make them two logically separate devices within
> the kernel. We need the channel number so that the device can be
> addressed in the first place, so I don't see what's wrong with using
> that number in the device's name.
>
> The whole point of this patch is to add MIPI DSI bus infrastructure, and
> the virtual channel is one of the fundamental aspects of that bus, so I
> think we need to make it an integral part of the implementation.
There are already devices which IMO do not fit to this model, for example
TC358710 [1] can work as DSI hub, which I suppose uses two or three virtual
channels of the main DSI-host and performs "automatic virtual channel
translation".

[1]
http://www.toshiba.com/taec/components/ProdBrief/10F02_TC358710_ProdBrief.pdf
>
>> I can also imagine scenarios when dynamic virtual channel (re-)association
>> can be useful and DSI specs are not against it AFAIK.
> How is that going to work? There's no hotplug support or similar in DSI,
> so why would anyone want to reassign virtual channels.
>
> Supposing even that we wanted to support this eventually, I think a more
> appropriate solution would be to completely remove the device and add a
> new one, because that also takes care of keeping the channel number
> embedded within the struct mipi_dsi_device up to date.
>
>> reg property means device is at fixed virtual channel.
>> DSI specs says nothing about it IMHO.
> Without that fixed virtual channel number we can't use the device in the
> first place. How are you going to address the device if you don't know
> its virtual channel?
>
 +  ssize_t (*transfer)(struct mipi_dsi_bus *bus,
 +  struct mipi_dsi_device *dev,
 +  struct mipi_dsi_msg *msg);
>>> Why do we need the dev parameter? There's already a channel field in the
>>> mipi_dsi_msg structure. Isn't that all that the transfer function needs
>>> to know about the device?
>> For simple HSI transfers, yes. In case transfer would depend on dev state
>> it would be useful, but I have no use case yet, so it could be removed.
> I don't know what an HSI transfer is. The transfer function is something
> that will be called by the peripheral's device driver, and that driver
> will know the state of the device, so I don't think the DSI host should
> care.
It should be HS, ie high-speed. But as I wrote before we can remove it
for now.
It can be added again in the future if I will have a real use case.
>
 +struct mipi_dsi_device {
 +  char name[MIPI_DSI_NAME_SIZE];
 +  int id;
 +  struct device dev;
 +
 +  const struct mipi_dsi_device_id *id_entry;
 +  struct mipi_dsi_bus *bus;
 +  struct videomode vm;
>>> Why is the videomode part of this structure? What if a device supports
>>> more than a single mode?
>> This is the current video mode. If we want to change mode,
>> we call:
>> ops->set_stream(false);
>> dev->vm =...new mode
>> ops->set_stream(true);
> I don't think that needs to be part of the DSI device. Rather it seems
> to me that whatever object type is implemented by the DSI device driver
> should have subsystem-specific code to do this.
>
> For example, if a DSI device is a bridge, then it will implement a
> drm_bridge object, and in that case the drm_bridge_funcs.mode_set()
> wou

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Mon, Nov 25, 2013 at 04:05:41PM +0100, Andrzej Hajda wrote:
[...]
> >> +/**
> >> + * mipi_dsi_device_register - register a DSI device
> >> + * @dev: DSI device we're registering
> >> + */
> >> +int mipi_dsi_device_register(struct mipi_dsi_device *dev,
> >> +struct mipi_dsi_bus *bus)
> >> +{
> >> +  device_initialize(&dev->dev);
> >> +
> >> +  dev->bus = bus;
> >> +  dev->dev.bus = &mipi_dsi_bus_type;
> >> +  dev->dev.parent = bus->dev;
> >> +  dev->dev.type = &mipi_dsi_dev_type;
> >> +
> >> +  if (dev->id != -1)
> > Does that case actually make sense in the context of DSI? There's a
> > defined hierarchy in DSI, so shouldn't we construct the names in a more
> > hierarchical way? I'm not sure if the ID is useful at all, perhaps it
> > should really be the virtual channel?
> >
> > The patch that I proposed used the following to make up the device name:
> >
> > dev_set_name(&dsi->dev, "%s.%u", dev_name(host->dev), dsi->channel);
> >
> > That has the advantage of reflecting the bus topology.
> >
> > It seems like your code was copied mostly from platform_device, for
> > which there's no well-defined topology and therefore having an ID makes
> > sense to differentiate between multiple instances of the same device.
> > But for DSI any host/bus can only have a single device with a given
> > channel, so that makes for a pretty good for unique name.
> Code was based on mipi_dbi_bus.c from CDF.
> I am not sure about hardwiring devices to virtual channels.
> There could be devices which uses more than one virtual channel,
> in fact exynos-drm docs suggests that such hardware exists.

In that case, why not make them two logically separate devices within
the kernel. We need the channel number so that the device can be
addressed in the first place, so I don't see what's wrong with using
that number in the device's name.

The whole point of this patch is to add MIPI DSI bus infrastructure, and
the virtual channel is one of the fundamental aspects of that bus, so I
think we need to make it an integral part of the implementation.

> I can also imagine scenarios when dynamic virtual channel (re-)association
> can be useful and DSI specs are not against it AFAIK.

How is that going to work? There's no hotplug support or similar in DSI,
so why would anyone want to reassign virtual channels.

Supposing even that we wanted to support this eventually, I think a more
appropriate solution would be to completely remove the device and add a
new one, because that also takes care of keeping the channel number
embedded within the struct mipi_dsi_device up to date.

> reg property means device is at fixed virtual channel.
> DSI specs says nothing about it IMHO.

Without that fixed virtual channel number we can't use the device in the
first place. How are you going to address the device if you don't know
its virtual channel?

> >> +  ssize_t (*transfer)(struct mipi_dsi_bus *bus,
> >> +  struct mipi_dsi_device *dev,
> >> +  struct mipi_dsi_msg *msg);
> > Why do we need the dev parameter? There's already a channel field in the
> > mipi_dsi_msg structure. Isn't that all that the transfer function needs
> > to know about the device?
> For simple HSI transfers, yes. In case transfer would depend on dev state
> it would be useful, but I have no use case yet, so it could be removed.

I don't know what an HSI transfer is. The transfer function is something
that will be called by the peripheral's device driver, and that driver
will know the state of the device, so I don't think the DSI host should
care.

> >> +struct mipi_dsi_device {
> >> +  char name[MIPI_DSI_NAME_SIZE];
> >> +  int id;
> >> +  struct device dev;
> >> +
> >> +  const struct mipi_dsi_device_id *id_entry;
> >> +  struct mipi_dsi_bus *bus;
> >> +  struct videomode vm;
> > Why is the videomode part of this structure? What if a device supports
> > more than a single mode?
> This is the current video mode. If we want to change mode,
> we call:
> ops->set_stream(false);
> dev->vm =...new mode
> ops->set_stream(true);

I don't think that needs to be part of the DSI device. Rather it seems
to me that whatever object type is implemented by the DSI device driver
should have subsystem-specific code to do this.

For example, if a DSI device is a bridge, then it will implement a
drm_bridge object, and in that case the drm_bridge_funcs.mode_set()
would be the equivalent of this.

On a side-note, I think we should rename .set_stream() to something more
DSI specific, such as:

enum mipi_dsi_mode {
MIPI_DSI_COMMAND_MODE,
MIPI_DSI_VIDEO_MODE,
};

int (*set_mode)(struct mipi_dsi_host *host, enum mipi_dsi_mode mode);

> >> +#define mipi_dsi_dcs_write_seq(dev, channel, seq...) \
> >> +({\
> >> +  const u8 d[] = { seq };\
> >> +  BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "DCS sequence too long for 
> >> stack");\
> >> +  mipi_dsi_dcs_write(dev, channel, d, ARRAY_SIZE(d));\
> >> +}

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Andrzej Hajda]

Hi Thierry,

Thanks for the review.


On 11/22/2013 06:41 PM, Thierry Reding wrote:
> On Mon, Nov 18, 2013 at 05:25:23PM +0100, Andrzej Hajda wrote:
>> MIPI DSI bus allows to model DSI hosts
>> and DSI devices using Linux bus.
> This looks somewhat terse. Any chance you could be more verbose about
> what exactly this does? You could mention for instance that it allows
> DSI devices to be instantiated from device tree and manually. That a
> Linux bus type is provided and device drivers can use that to bind to
> DSI devices.
OK.
>
>> Signed-off-by: Andrzej Hajda 
>> Signed-off-by: Kyungmin Park 
>> ---
>> Hi,
>>
>> This is my implementation of mipi dsi bus.
>> The first time it was posted as a part of CDF infrastructure [1],
>> but if it can be merged independently I will be glad.
>>
>> I have not addressed yet Bert's comments, but I think it should
>> be easy, once we have agreement how to implement it.
>>
>> There are also working drivers which uses this bus:
>> - Exynos DSI master,
>> - s6e8aa0 panel.
>> I will post them later.
>>
>> [1] http://www.mail-archive.com/dri-devel at 
>> lists.freedesktop.org/msg45252.html
>>
>> Changes:
>> v2:
>> - set_power callback removed (its role is passed to RUNTIME_PM),
>> - changed transfer ops parameters to struct,
>> - changed source location,
>> - minor fixes
>>
>> Regards
>> Andrzej
>>
>> ---
>>  drivers/gpu/drm/Kconfig|   4 +
>>  drivers/gpu/drm/Makefile   |   2 +
>>  drivers/gpu/drm/drm_mipi_dsi.c | 344 
>> +
>>  include/drm/drm_mipi_dsi.h | 154 ++
>>  4 files changed, 504 insertions(+)
>>  create mode 100644 drivers/gpu/drm/drm_mipi_dsi.c
>>  create mode 100644 include/drm/drm_mipi_dsi.h
> This seems to be missing a device tree binding document. That could
> probably be rather small since there's not a lot there right now. I
> volunteer to draft that document if you don't have the time to do
> that.
>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index f864275..58a603d 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -20,6 +20,10 @@ menuconfig DRM
>>details.  You should also select and configure AGP
>>(/dev/agpgart) support if it is available for your platform.
>>  
>> +config DRM_MIPI_DSI
>> +tristate
>> +default y
> Shouldn't this remain off by default? And only be selected by drivers
> that actually need it. I think that DSI host drivers could select this
> symbol and DSI peripheral drivers can depend on it. That way you'll
> automatically be able to only select peripheral drivers if there's at
> least one DSI host driver enabled.
Yes, of course. I just forgot to set it back properly after last tests.
>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> [...]
>> +/*
>> + * MIPI DSI Bus
>> + *
>> + * Copyright (C) 2012, Samsung Electronics, Co., Ltd.
> Perhaps this should now be "2012-2013"?
Yes, thanks.
>
>> + * Andrzej Hajda 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> Can these please be ordered alphabetically?
OK
>
>> +/* 
>> -
>> + * Bus operations
>> + */
> Nitpick: I'm not a huge fan of these separators. They have a tendency to
> get in the way when people add new functions and then all of a sudden
> they no longer fit into that category...
>
>> +int mipi_dsi_init(struct mipi_dsi_device *dev)
>> +{
> [...]
>> +}
>> +EXPORT_SYMBOL_GPL(mipi_dsi_init);
>> +
>> +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on)
>> +{
> [...]
>> +}
>> +EXPORT_SYMBOL_GPL(mipi_dsi_set_stream);
> These are missing documentation. It's not clear at all what they are
> supposed to do.
>
>> +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 
>> *data,
> unsigned int for channel, please. And const void * for data so that the
> same type is used consistently.
>
>> +static struct device_attribute mipi_dsi_dev_attrs[] = {
> static const?
>
> I also believe these now need to be done using attribute groups. Look at
> commit d06262e58546 (driver-core: platform: convert bus code to use
> dev_groups) for an example of how to do that.

>> +static const struct dev_pm_ops mipi_dsi_dev_pm_ops = {
>> +.runtime_suspend = pm_generic_runtime_suspend,
>> +.runtime_resume = pm_generic_runtime_resume,
>> +.suspend = pm_generic_suspend,
>> +.resume = pm_generic_resume,
>> +.freeze = pm_generic_freeze,
>> +.thaw = pm_generic_thaw,
>> +.poweroff = pm_generic_poweroff,
>> +.restore = pm_generic_restore,
>> +};
>> +
>> +static struct bus_type mipi_dsi_bus_type =

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Thierry Reding]

On Mon, Nov 18, 2013 at 05:25:23PM +0100, Andrzej Hajda wrote:
> MIPI DSI bus allows to model DSI hosts
> and DSI devices using Linux bus.

This looks somewhat terse. Any chance you could be more verbose about
what exactly this does? You could mention for instance that it allows
DSI devices to be instantiated from device tree and manually. That a
Linux bus type is provided and device drivers can use that to bind to
DSI devices.

> Signed-off-by: Andrzej Hajda 
> Signed-off-by: Kyungmin Park 
> ---
> Hi,
> 
> This is my implementation of mipi dsi bus.
> The first time it was posted as a part of CDF infrastructure [1],
> but if it can be merged independently I will be glad.
> 
> I have not addressed yet Bert's comments, but I think it should
> be easy, once we have agreement how to implement it.
> 
> There are also working drivers which uses this bus:
> - Exynos DSI master,
> - s6e8aa0 panel.
> I will post them later.
> 
> [1] http://www.mail-archive.com/dri-devel at 
> lists.freedesktop.org/msg45252.html
> 
> Changes:
> v2:
> - set_power callback removed (its role is passed to RUNTIME_PM),
> - changed transfer ops parameters to struct,
> - changed source location,
> - minor fixes
> 
> Regards
> Andrzej
> 
> ---
>  drivers/gpu/drm/Kconfig|   4 +
>  drivers/gpu/drm/Makefile   |   2 +
>  drivers/gpu/drm/drm_mipi_dsi.c | 344 
> +
>  include/drm/drm_mipi_dsi.h | 154 ++
>  4 files changed, 504 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_mipi_dsi.c
>  create mode 100644 include/drm/drm_mipi_dsi.h

This seems to be missing a device tree binding document. That could
probably be rather small since there's not a lot there right now. I
volunteer to draft that document if you don't have the time to do
that.

> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f864275..58a603d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -20,6 +20,10 @@ menuconfig DRM
> details.  You should also select and configure AGP
> (/dev/agpgart) support if it is available for your platform.
>  
> +config DRM_MIPI_DSI
> + tristate
> + default y

Shouldn't this remain off by default? And only be selected by drivers
that actually need it. I think that DSI host drivers could select this
symbol and DSI peripheral drivers can depend on it. That way you'll
automatically be able to only select peripheral drivers if there's at
least one DSI host driver enabled.

> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
[...]
> +/*
> + * MIPI DSI Bus
> + *
> + * Copyright (C) 2012, Samsung Electronics, Co., Ltd.

Perhaps this should now be "2012-2013"?

> + * Andrzej Hajda 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Can these please be ordered alphabetically?

> +/* 
> -
> + * Bus operations
> + */

Nitpick: I'm not a huge fan of these separators. They have a tendency to
get in the way when people add new functions and then all of a sudden
they no longer fit into that category...

> +int mipi_dsi_init(struct mipi_dsi_device *dev)
> +{
[...]
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_init);
> +
> +int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on)
> +{
[...]
> +}
> +EXPORT_SYMBOL_GPL(mipi_dsi_set_stream);

These are missing documentation. It's not clear at all what they are
supposed to do.

> +int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 
> *data,

unsigned int for channel, please. And const void * for data so that the
same type is used consistently.

> +static struct device_attribute mipi_dsi_dev_attrs[] = {

static const?

I also believe these now need to be done using attribute groups. Look at
commit d06262e58546 (driver-core: platform: convert bus code to use
dev_groups) for an example of how to do that.

> +static const struct dev_pm_ops mipi_dsi_dev_pm_ops = {
> + .runtime_suspend = pm_generic_runtime_suspend,
> + .runtime_resume = pm_generic_runtime_resume,
> + .suspend = pm_generic_suspend,
> + .resume = pm_generic_resume,
> + .freeze = pm_generic_freeze,
> + .thaw = pm_generic_thaw,
> + .poweroff = pm_generic_poweroff,
> + .restore = pm_generic_restore,
> +};
> +
> +static struct bus_type mipi_dsi_bus_type = {
> + .name   = "mipi-dsi",
> + .dev_attrs  = mipi_dsi_dev_attrs,
> + .match  = mipi_dsi_match,
> + .uevent = mipi_dsi_uevent,
> + .pm = &mipi_dsi_dev_pm_ops,
> +};

Can we please stick to one style for these structures? I personally
prefer the one you used fo

[RFC v2 PATCH] mipi-dsi-bus: add MIPI DSI bus support

[Andrzej Hajda]

MIPI DSI bus allows to model DSI hosts
and DSI devices using Linux bus.

Signed-off-by: Andrzej Hajda 
Signed-off-by: Kyungmin Park 
---
Hi,

This is my implementation of mipi dsi bus.
The first time it was posted as a part of CDF infrastructure [1],
but if it can be merged independently I will be glad.

I have not addressed yet Bert's comments, but I think it should
be easy, once we have agreement how to implement it.

There are also working drivers which uses this bus:
- Exynos DSI master,
- s6e8aa0 panel.
I will post them later.

[1] http://www.mail-archive.com/dri-devel at lists.freedesktop.org/msg45252.html

Changes:
v2:
- set_power callback removed (its role is passed to RUNTIME_PM),
- changed transfer ops parameters to struct,
- changed source location,
- minor fixes

Regards
Andrzej

---
 drivers/gpu/drm/Kconfig|   4 +
 drivers/gpu/drm/Makefile   |   2 +
 drivers/gpu/drm/drm_mipi_dsi.c | 344 +
 include/drm/drm_mipi_dsi.h | 154 ++
 4 files changed, 504 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_mipi_dsi.c
 create mode 100644 include/drm/drm_mipi_dsi.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f864275..58a603d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -20,6 +20,10 @@ menuconfig DRM
  details.  You should also select and configure AGP
  (/dev/agpgart) support if it is available for your platform.

+config DRM_MIPI_DSI
+   tristate
+   default y
+
 config DRM_USB
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index cc08b84..6bab99c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -19,6 +19,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
 drm-$(CONFIG_PCI) += ati_pcigart.o

+drm-mipi-dsi-y := drm_mipi_dsi.o
 drm-usb-y   := drm_usb.o

 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
@@ -31,6 +32,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 CFLAGS_drm_trace_points.o := -I$(src)

 obj-$(CONFIG_DRM)  += drm.o
+obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
 obj-$(CONFIG_DRM_USB)   += drm_usb.o
 obj-$(CONFIG_DRM_TTM)  += ttm/
 obj-$(CONFIG_DRM_TDFX) += tdfx/
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
new file mode 100644
index 000..ead35da
--- /dev/null
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -0,0 +1,344 @@
+/*
+ * MIPI DSI Bus
+ *
+ * Copyright (C) 2012, Samsung Electronics, Co., Ltd.
+ * Andrzej Hajda 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* 
-
+ * Bus operations
+ */
+
+int mipi_dsi_init(struct mipi_dsi_device *dev)
+{
+   const struct mipi_dsi_bus_ops *ops = dev->bus->ops;
+
+   if (!ops->init)
+   return -ENOSYS;
+
+   return ops->init(dev->bus, dev);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_init);
+
+int mipi_dsi_set_stream(struct mipi_dsi_device *dev, bool on)
+{
+   const struct mipi_dsi_bus_ops *ops = dev->bus->ops;
+
+   if (!ops->set_stream)
+   return -ENOSYS;
+
+   return ops->set_stream(dev->bus, dev, on);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_set_stream);
+
+int mipi_dsi_dcs_write(struct mipi_dsi_device *dev, int channel, const u8 
*data,
+  size_t len)
+{
+   const struct mipi_dsi_bus_ops *ops = dev->bus->ops;
+   struct mipi_dsi_msg msg = {
+   .channel = channel,
+   .tx_buf = data,
+   .tx_len = len
+   };
+
+   if (!ops->transfer)
+   return -ENOSYS;
+
+   switch (len) {
+   case 0:
+   return -EINVAL;
+   case 1:
+   msg.type = MIPI_DSI_DCS_SHORT_WRITE;
+   break;
+   case 2:
+   msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
+   break;
+   default:
+   msg.type = MIPI_DSI_DCS_LONG_WRITE;
+   }
+
+   return ops->transfer(dev->bus, dev, &msg);
+}
+EXPORT_SYMBOL_GPL(mipi_dsi_dcs_write);
+
+ssize_t mipi_dsi_dcs_read(struct mipi_dsi_device *dev, int channel, u8 cmd,
+ u8 *data, size_t len)
+{
+   const struct mipi_dsi_bus_ops *ops = dev->bus->ops;
+   struct mipi_dsi_msg msg = {
+   .channel = channel,
+   .type = MIPI_DSI_DCS_READ,
+   .tx_buf = &cmd,
+   .tx_len = 1,
+   .rx_buf = data,
+   .rx_len = len
+   };
+
+   if (!ops->transfer)
+   return -ENOSYS;
+
+   return ops->transfer(dev->bus, dev, &msg);
+}
+EXPORT_SYMBOL_GPL(mip