Message-ID: <55F12007.6020307@codeaurora.org>
Date:	Thu, 10 Sep 2015 11:45:35 +0530
From:	Archit Taneja <architt@...eaurora.org>
To:	Andrzej Hajda <a.hajda@...sung.com>,
	Thierry Reding <treding@...dia.com>
Cc:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: Re: [RFC 0/2] drm/dsi: DSI for devices with different control bus



On 09/08/2015 03:57 PM, Andrzej Hajda wrote:
> On 09/07/2015 01:46 PM, Archit Taneja wrote:
>> Thierry,
>>
>> On 08/21/2015 11:39 AM, Archit Taneja wrote:
>>>
>>>
>>> On 08/20/2015 05:18 PM, Thierry Reding wrote:
>>>> On Thu, Aug 20, 2015 at 09:46:14AM +0530, Archit Taneja wrote:
>>>>> Hi Thierry, Lucas,
>>>>>
>>>>>
>>>>> On 08/19/2015 08:32 PM, Thierry Reding wrote:
>>>>>> On Wed, Aug 19, 2015 at 04:52:24PM +0200, Lucas Stach wrote:
>>>>>>> Am Mittwoch, den 19.08.2015, 16:34 +0200 schrieb Thierry Reding:
>>>>>>>> On Wed, Aug 19, 2015 at 04:17:08PM +0200, Lucas Stach wrote:
>>>>>>>>> Hi Thierry, Archit,
>>>>>>>>>
>>>>>>> [...]
>>>>>>>>>> Perhaps a better way would be to invert this relationship.
>>>>>>>>>> According to
>>>>>>>>>> your proposal we'd have to have DT like this:
>>>>>>>>>>
>>>>>>>>>>      i2c@... {
>>>>>>>>>>          ...
>>>>>>>>>>
>>>>>>>>>>          dsi-device@... {
>>>>>>>>>>              ...
>>>>>>>>>>              dsi-bus = <&dsi>;
>>>>>>>>>>              ...
>>>>>>>>>>          };
>>>>>>>>>>
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>>      dsi@... {
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>> Inversing the relationship would become something like this:
>>>>>>>>>>
>>>>>>>>>>      i2c@... {
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>>      dsi@... {
>>>>>>>>>>          ...
>>>>>>>>>>
>>>>>>>>>>          peripheral@... {
>>>>>>>>>>              ...
>>>>>>>>>>              i2c-bus = <&i2c>;
>>>>>>>>>>              ...
>>>>>>>>>>          };
>>>>>>>>>>
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>> Both of those aren't fundamentally different, and they both have
>>>>>>>>>> the
>>>>>>>>>> disavantage of lacking ways to transport configuration data that
>>>>>>>>>> the
>>>>>>>>>> other bus needs to instantiate the dummy device (such as the reg
>>>>>>>>>> property for example, denoting the I2C slave address or the DSI
>>>>>>>>>> VC).
>>>>>>>>>>
>>>>>>>>>> So how about we create two devices in the device tree and fuse
>>>>>>>>>> them at
>>>>>>>>>> the driver level:
>>>>>>>>>>
>>>>>>>>>>      i2c@... {
>>>>>>>>>>          ...
>>>>>>>>>>
>>>>>>>>>>          i2cdsi: dsi-device@... {
>>>>>>>>>>              ...
>>>>>>>>>>          };
>>>>>>>>>>
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>>      dsi@... {
>>>>>>>>>>          ...
>>>>>>>>>>
>>>>>>>>>>          peripheral@... {
>>>>>>>>>>              ...
>>>>>>>>>>              control = <&i2cdsi>;
>>>>>>>>>>              ...
>>>>>>>>>>          };
>>>>>>>>>>
>>>>>>>>>>          ...
>>>>>>>>>>      };
>>>>>>>>>>
>>>>>>>>>> This way we'll get both an I2C device and a DSI device that we
>>>>>>>>>> can fully
>>>>>>>>>> describe using the standard device tree bindings. At driver time
>>>>>>>>>> we can
>>>>>>>>>> get the I2C device from the phandle in the control property of
>>>>>>>>>> the DSI
>>>>>>>>>> device and use it to execute I2C transactions.
>>>>>>>>>>
>>>>>>>>> I don't really like to see that you are inventing yet-another-way to
>>>>>>>>> handle devices connected to multiple buses.
>>>>>>>>>
>>>>>>>>> Devicetree is structured along the control buses, even if the
>>>>>>>>> devices
>>>>>>>>> are connected to multiple buses, in the DT they are always
>>>>>>>>> children of
>>>>>>>>> the bus that is used to control their registers from the CPUs
>>>>>>>>> perspective. So a DSI encoder that is controlled through i2c is
>>>>>>>>> clearly
>>>>>>>>> a child of the i2c master controller and only of that one.
>>>>>>>>
>>>>>>>> I think that's a flawed interpretation of what's going on here. The
>>>>>>>> device in fact has two interfaces: one is I2C, the other is DSI.
>>>>>>>> In my
>>>>>>>> opinion that's reason enough to represent it as two logical devices.
>>>>>>>>
>>>>>>> Does it really have 2 control interfaces that are used at the same
>>>>>>> time?
>>>>>>> Or is the DSI connection a passive data bus if the register control
>>>>>>> happens through i2c?
>>>>>>
>>>>>> The interfaces may not be used at the same time, and the DSI interface
>>>>>> may even be crippled, but the device is still addressable from the DSI
>>>>>> host controller, if for nothing else than for routing the video stream.
>>>>>>
>>>>>>>>> If you need to model connections between devices that are not
>>>>>>>>> reflected
>>>>>>>>> through the control bus hierarchy you should really consider
>>>>>>>>> using the
>>>>>>>>> standardized of-graph bindings.
>>>>>>>>> (Documentation/devicetree/bindings/graph.txt)
>>>>>>>>
>>>>>>>> The problem is that the original proposal would instantiate a dummy
>>>>>>>> device, so it wouldn't be represented in DT at all. So unless you
>>>>>>>> do add
>>>>>>>> two logical devices to DT (one for each bus interface), you don't
>>>>>>>> have
>>>>>>>> anything to glue together with an OF graph.
>>>>>>>>
>>>>>>> I see that the having dummy device is the least desirable solution.
>>>>>>> But
>>>>>>> if there is only one control bus to the device I think it should be
>>>>>>> one
>>>>>>> device sitting beneath the control bus.
>>>>>>>
>>>>>>> You can then use of-graph to model the data path between the DSI
>>>>>>> encoder
>>>>>>> and device.
>>>>>>
>>>>>> But you will be needing a device below the DSI host controller to
>>>>>> represent that endpoint of the connection. The DSI host controller
>>>>>> itself is in no way connected to the I2C adapter. You would have to
>>>>>> add some sort of quirk to the DSI controller binding to allow it to
>>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> I implemented this to support ADV7533 DSI to HDMI encoder chip, which
>>>>> has a DSI video bus and an i2c control bus.
>>>>>
>>>>> There weren't any quirks as such in the device tree when I tried to
>>>>> implement this. The DT seems to manage fine without a node
>>>>> corresponding to a mipi_dsi_device:
>>>>>
>>>>> i2c_adap@.. {
>>>>>      adv7533@.. {
>>>>>
>>>>>          port {
>>>>>              adv_in: endpoint {
>>>>>                  remote-endpoint = <&dsi_out>;
>>>>>              };
>>>>>          };
>>>>>      };
>>>>> };
>>>>>
>>>>> dsi_host@.. {
>>>>>      ...
>>>>>      ...
>>>>>
>>>>>      port {
>>>>>          dsi_out: endpoint {
>>>>>              remote-endpoint = <&adv_in>;
>>>>>          }
>>>>>      };
>>>>> };
>>>>>
>>>>> It's the i2c driver's job to parse the graph and retrieve the
>>>>> phandle to the dsi host. Using this, it can proceed with
>>>>> registering itself to this host using the new dsi funcs. This
>>>>> patch does the same for the adv7533 i2c driver:
>>>>>
>>>>> http://www.spinics.net/lists/dri-devel/msg86840.html
>>>>>
>>>>>> hook up with a control endpoint. And then you'll need more quirks
>>>>>> to describe what kind of DSI device this is.
>>>>>
>>>>> Could you explain what you meant by this? I.e. describing the kind
>>>>> of DSI device?
>>>>
>>>> Describing the number of lanes, specifying the virtual channel, mode
>>>> flags, etc. You could probably set the number of lanes and mode flags
>>>> via the I2C driver, but especially the virtual channel cannot be set
>>>> because it isn't known to the I2C DT branch of the device.
>>>
>>> I agree with the VC part. It could be a DT entry within the i2c client
>>> node, but that does make it seem like a quirk. The 'reg' way under the
>>> DSI host is definitely better to populate the virtual channel.
>>>
>>>>
>>>>> The dsi device created isn't really a dummy device as such. It's
>>>>> dummy in the sense that there isn't a real dsi driver associated
>>>>> with it. The dsi device is still used to attach to a mipi dsi host,
>>>>> the way normal dsi devices do.
>>>>
>>>> I understand, but I don't see why it has to be instantiated by the I2C
>>>> driver, that's what I find backwards. There is already a standard way
>>>> for instantiating DSI devices, why not use it?
>>>
>>> I assumed we could either represent the device using an i2c driver, or
>>> dsi, but not both. Hence, I came up with this approach.
>>>
>>>>
>>>>>> On the other hand if you properly instantiate the DSI device you can
>>>>>> easily write a driver for it, and the driver will set up the correct
>>>>>> properties as implied by the compatible string. Once you have that you
>>>>>> can easily hook it up to the I2C control interface in whatever way you
>>>>>> like, be that an OF graph or just a simple unidirectional link by
>>>>>> phandle.
>>>>>>
>>>>>
>>>>> With the fused approach you suggested, we would have 2 drivers: one i2c
>>>>> and the other dsi. The i2c client driver would be more or less minimal,
>>>>> preparing an i2c_client device for the dsi driver to use. Is my
>>>>> understanding correct?
>>>>
>>>> Correct. That's kind of similar to the way an HDMI encoder driver would
>>>> use an I2C adapter to query EDID. The i2c_client device would be a means
>>>> for the DSI driver to access the control interface.
>>>
>>> Okay.
>>>
>>> Although, I'm not sure about the HDMI encoder example. An HDMI
>>> encoder would read off edid directly from the adapter(with an address
>>> specified), it wouldn't need to create an i2c client device. Therefore,
>>> an HDMI encoder wouldn't need to have a separate node for i2c in DT.
>>>
>>>>
>>>>> We can do without dummy dsi devices with this method. But, representing
>>>>> a device with 2 DT nodes seems a bit off. We'd also need to compatible
>>>>> strings for the same device, one for the i2c part, and the other for
>>>>> the dsi part.
>>>>
>>>> I agree that this somewhat stretches the capabilities of device tree.
>>>> Another alternative I guess would be to not have a compatible string for
>>>> the I2C device at all (that's technically not valid, I guess) because we
>>>> really don't need an I2C driver for the device. What we really need is a
>>>> DSI driver with a means to talk over some I2C bus with some other part
>>>> of its device.
>>>
>>> I think what the driver should 'really' be is a bit subjective, and can
>>> vary based on what the buses are used for in the device. For the Toshiba
>>> chip that Jani mentioned, it tends more towards a DSI driver. Whereas,
>>> for an ADV75xx chip, it's closer to an I2C driver since only I2C can be
>>> used to configure the chip registers.
>>>
>>> Although, I agree with the point you made about the DSI bus here:
>>>
>>> "and the DSI interface may even be crippled, but the device is still
>>> addressable from the DSI host controller, if for nothing else than for
>>> routing the video stream."
>>>
>>> The fact that the data on the DSI bus contains routing information (i.e,
>>> virtual channel number) always gives it some 'control' aspect.
>>>
>>>>
>>>>>   From an adv75xx driver perspective, it should also support the ADV7511
>>>>> chip, which is a RGB/DPI to HDMI encoder. For adv7511, we don't need a
>>>>> DSI DT node. It would be a bit inconsistent to have the bindings
>>>>> require both DSI and I2C nodes for one chip, and only I2C node for the
>>>>> other, with both chips being supported by the same driver.
>>>>
>>>> Why would that be inconsistent? That sounds like the most accurate
>>>> representation of the hardware to me.
>>>
>>> Inconsistent wasn't the right term. I should have used 'uncommon' :)
>>> It's common for two chips of the same family to have a different set
>>> optional properties in DT, but it's not common for two chips of the
>>> same family to be represented by a different number of devices in
>>> DT.
>>>
>>> I don't have an issue with the fused approach you suggested, as long
>>> as people are okay with the DT parts. Especially the part of needing 2
>>> compatible strings in the DT.
>>
>> I implemented the ADV7533 driver with the approach you suggested above
>> (2 drivers for 2 different components of the chip). I posted it out
>> just a while back (with you in loop).
>>
>> The DT node with this apporach would look like this:
>>
>> https://github.com/boddob/linux/blob/c24cbf63a6998d00095c10122ce5e37b764c7dba/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L162
>>
>> The main irritant with the '2 driver' approach is that we need to
>> share the per-device driver data with them. For ADV7533, I've made
>> the i2c driver allocate driver data (struct adv7511).
>>
>> The dsi driver gets the driver data in the following way:
>>
>> - The i2c driver sets the driver data as its client data using
>>     i2c_set_clientdata()
>> - Parse the i2c-control phandle to get the corresponding i2c client.
>> - Extract the adv7511 struct by getting i2c_get_clientdata()
>>
>> This way of getting the same driver data is a bit strange, but it
>> works. For this, we do need to ensure that the dsi driver defers
>> as long as the i2c driver isn't probed.
>>
>> I've now implemented both approaches for the driver. The first using
>> a dummy dsi device, and this one using 2 drivers (with both being
>> represented in DT). The advantage of the latter is that we don't need
>> to create any dummy device stuff, the disadvantage is that DT is a bit
>> uncommon.
>>
>> Can we now come to a conclusion on what approach is better?
>
> DSI by design is data bus which can be used additionally as a control bus, but
> in this particular case it is purely data bus. So of-graph bindings seem to be
> better choice. As already Lucas Stach said DT hierarchy should describe control
> buses and of-graph bindings should describe data bus. Argument that hw has two
> interfaces does not seem to be valid here - it has only one control interface.
> The other one is only for data, representing every data interface using DT
> hierarchy would lead to inflation of pseudo devices.
>
> On the other side I do not see dummy device approach ideal solution, I guess
> lightweight framework providing DSI hosts detached from Linux device model could
> work better here.
> The only problem here is that it should coexist somehow with dsi bus used to
> control devices. Anyway implementing it shouldn't be hard, question is if it
> would be eventually accepted :) I guess we can live for now with dummy devs.
>
> Summarizing I would prefer version with dummy devices, as it seems more
> compatible with DT design.

Thanks for the feedback. I'll spin a newer version of the dummy dsi dev 
patches after waiting for some more comments.

Archit

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists