* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 12:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <3768216.eiA2v5KI6a@avalon>
[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]
On 17/10/13 15:17, Laurent Pinchart wrote:
> Hi Tomi,
>
> On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote:
>> On 17/10/13 14:51, Laurent Pinchart wrote:
>>>> I'm not sure if there's a specific need for the port or endpoint nodes
>>>> in cases like the above. Even if we have common properties describing
>>>> the endpoint, I guess they could just be in the parent node.
>>>>
>>>> panel {
>>>> remote = <&dc>;
>>>> common-video-property = <asd>;
>>>> };
>>>>
>>>> The above would imply one port and one endpoint. Would that work? If we
>>>> had a function like parse_endpoint(node), we could just point it to
>>>> either a real endpoint node, or to the device's node.
>>>
>>> You reference the display controller here, not a specific display
>>> controller output. Don't most display controllers have several outputs ?
>>
>> Sure. Then the display controller could have more verbose description.
>> But the panel could still have what I wrote above, except the 'remote'
>> property would point to a real endpoint node inside the dispc node, not
>> to the dispc node.
>>
>> This would, of course, need some extra code to handle the different
>> cases, but just from DT point of view, I think all the relevant
>> information is there.
>
> There's many ways to describe the same information in DT. While we could have
> DT bindings that use different descriptions for different devices and still
> support all of them in our code, why should we opt for that option that will
> make the implementation much more complex when we can describe connections in
> a simple and generic way ?
My suggestion was simple and generic. I'm not proposing per-device
custom bindings.
My point was, if we can describe the connections as I described above,
which to me sounds possible, we can simplify the panel DT data for 99.9%
of the cases.
To me, the first of these looks much nicer:
panel {
remote = <&remote-endpoint>;
common-video-property = <asd>;
};
panel {
port {
endpoint {
remote = <&remote-endpoint>;
common-video-property = <asd>;
};
};
};
If that can be supported in the SW by adding complexity to a few
functions, and it covers practically all the panels, isn't it worth it?
Note that I have not tried this, so I don't know if there are issues.
It's just a thought. Even if there's need for a endpoint node, perhaps
the port node can be made optional.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH/RFC v3 00/19] Common Display Framework
From: Andrzej Hajda @ 2013-10-17 12:26 UTC (permalink / raw)
To: Tomi Valkeinen, Laurent Pinchart
Cc: linux-fbdev, dri-devel, Jesse Barnes, Benjamin Gaignard, Tom Gall,
Kyungmin Park, linux-media, Stephen Warren, Mark Zhang,
Alexandre Courbot, Ragesh Radhakrishnan, Thomas Petazzoni,
Sunil Joshi, Maxime Ripard, Vikas Sajjan, Marcus Lorentzon
In-Reply-To: <525F9D4D.4000702@ti.com>
On 10/17/2013 10:18 AM, Tomi Valkeinen wrote:
> On 17/10/13 10:48, Andrzej Hajda wrote:
>
>> The main function of DSI is to transport pixels from one IP to another IP
>> and this function IMO should not be modeled by display entity.
>> "Power, clocks, etc" will be performed via control bus according to
>> panel demands.
>> If 'DSI chip' has additional functions for video processing they can
>> be modeled by CDF entity if it makes sense.
> Now I don't follow. What do you mean with "display entity" and with "CDF
> entity"? Are they the same?
Yes, they are the same, sorry for confusion.
>
> Let me try to clarify my point:
>
> On OMAP SoC we have a DSI encoder, which takes input from the display
> controller in parallel RGB format, and outputs DSI.
>
> Then there are external encoders that take MIPI DPI as input, and output
> DSI.
>
> The only difference with the above two components is that the first one
> is embedded into the SoC. I see no reason to represent them in different
> ways (i.e. as you suggested, not representing the SoC's DSI at all).
>
> Also, if you use DSI burst mode, you will have to have different video
> timings in the DSI encoder's input and output. And depending on the
> buffering of the DSI encoder, you could have different timings in any case.
I am not sure what exactly the encoder performs, if this is only image
transport from dispc to panel CDF pipeline in both cases should look like:
dispc ----> panel
The only difference is that panels will be connected via different Linux bus
adapters, but it will be irrelevant to CDF itself. In this case I would say
this is DSI-master rather than encoder, or at least that the only
function of the
encoder is DSI.
If display_timings on input and output differs, I suppose it should be
modeled
as display_entity, as this is an additional functionality(not covered by
DSI standard AFAIK).
CDF in such case:
dispc ---> encoder ---> panel
In this case I would call it encoder with DSI master.
>
> Furthermore, both components could have extra processing. I know the
> external encoders sometimes do have features like scaling.
The same as above, ISP with embedded DSI.
>
>>> We still have two different endpoint configurations for the same
>>> DSI-master port. If that configuration is in the DSI-master's port node,
>>> not inside an endpoint data, then that can't be supported.
>> I am not sure if I understand it correctly. But it seems quite simple:
>> when panel starts/resumes it request DSI (via control bus) to fulfill
>> its configuration settings.
>> Of course there are some settings which are not panel dependent and those
>> should reside in DSI node.
> Exactly. And when the two panels require different non-panel-dependent
> settings, how do you represent them in the DT data?
non-panel-dependent setting cannot depend on panel, by definition :)
>
>>>> We say then: callee handles locking :)
>>> Sure, but my point was that the caller handling the locking is much
>>> simpler than the callee handling locking. And the latter causes
>>> atomicity issues, as the other API could be invoked in between two calls
>>> for the first API.
>>>
>>>
>> Could you describe such scenario?
> If we have two independent APIs, ctrl and video, that affect the same
> underlying hardware, the DSI bus, we could have a scenario like this:
>
> thread 1:
>
> ctrl->op_foo();
> ctrl->op_bar();
>
> thread 2:
>
> video->op_baz();
>
> Even if all those ops do locking properly internally, the fact that
> op_baz() can be called in between op_foo() and op_bar() may cause problems.
>
> To avoid that issue with two APIs we'd need something like:
>
> thread 1:
>
> ctrl->lock();
> ctrl->op_foo();
> ctrl->op_bar();
> ctrl->unlock();
>
> thread 2:
>
> video->lock();
> video->op_baz();
> video->unlock();
I should mention I was asking for real hw/drivers configuration.
I do not know what do you mean with video->op_baz() ?
DSI-master is not modeled in CDF, and only CDF provides video
operations.
I guess one scenario, when two panels are connected to single DSI-master.
In such case both can call DSI ops, but I do not know how do you want to
prevent it in case of your CDF-T implementation.
>
>>>> Platform devices
>>>> ~~~~~~~~~~~~~~~~
>>>> Platform devices are devices that typically appear as autonomous
>>>> entities in the system. This includes legacy port-based devices and
>>>> host bridges to peripheral buses, and most controllers integrated
>>>> into system-on-chip platforms. What they usually have in common
>>>> is direct addressing from a CPU bus. Rarely, a platform_device will
>>>> be connected through a segment of some other kind of bus; but its
>>>> registers will still be directly addressable.
>>> Yep, "typically" and "rarely" =). I agree, it's not clear. I think there
>>> are things with DBI/DSI that clearly point to a platform device, but
>>> also the other way.
>> Just to be sure, we are talking here about DSI-slaves, ie. for example
>> about panels,
>> where direct accessing from CPU bus usually is not possible.
> Yes. My point is that with DBI/DSI there's not much bus there (if a
> normal bus would be PCI/USB/i2c etc), it's just a point to point link
> without probing or a clearly specified setup sequence.
This is why I considered replacing DSI bus with DSI-master as parent
device and panel as slave platorm_device, like in MFD devices.
>
> If DSI/DBI was used only for control, a linux bus would probably make
> sense. But DSI/DBI is mainly a video transport channel, with the
> control-part being "secondary".
>
> And when considering that the video and control data are sent over the
> same channel (i.e. there's no separate, independent ctrl channel), and
> the strict timing restrictions with video, my gut feeling is just that
> all the extra complexity brought with separating the control to a
> separate bus is not worth it.
There is additional complexity due to bus implementation requirements
(I would rather call it boiler-plate code), but in core it is still a
matter of ops.
With Linux bus those ops are available only to DSI-slave, which is
also a good thing I guess.
Andrzej
>
> Tomi
>
>
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:23 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
Rob Clark, Daniel Vetter
In-Reply-To: <20131017120646.GE10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 7685 bytes --]
Hi Thierry,
On Thursday 17 October 2013 14:06:47 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > > I keep wondering why we absolutely must have compatibility between
> > > > > CDF and this simple panel binding. DT content is supposed to concern
> > > > > itself with the description of hardware only. What about the
> > > > > following isn't an accurate description of the panel hardware?
> > > > >
> > > > > panel: panel {
> > > > > compatible = "cptt,claa101wb01";
> > > > >
> > > > > power-supply = <&vdd_pnl_reg>;
> > > > > enable-gpios = <&gpio 90 0>;
> > > > >
> > > > > backlight = <&backlight>;
> > > > > };
> > > > >
> > > > > dsi-controller {
> > > > > panel = <&panel>;
> > > > > };
> > > >
> > > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > > bindings work. The difference is that I have a reference from the
> > > > panel node to the video source (dsi-controller), instead of the other
> > > > way around. I just find it more natural. It works the same way as,
> > > > say, regulators, gpios, etc.
> > >
> > > I suppose that depends on the way you look at it. In the above proposal
> > > I consider the output (DSI controller) to use the panel as a resource
> > > that provides a certain set of services (query mode timings, provide a
> > > way to enable or disable the panel). Similarly the panel uses the
> > > backlight as a resource that provides a certain set of services (such as
> > > changing the brightness).
> > >
> > > The above also follows the natural order of control. The panel has no
> > > way to control the DSI output. However, it is the output that controls
> > > when a panel is required and turn it on as appropriate.
> >
> > I'm no DSI expert, but I know enough about it to be sure that Tomi will
> > disagree. DSI panels can have complex power sequences that require the
> > input stream to be finely controlled (for instance it might require a
> > clock without video frames for some time, switch a GPIO or regulator,
> > send a command to the panel, and then only get video frames). For that
> > reason all developers I've talked to who have an in-depth knowledge of
> > DSI and DSI panels told me that the panel needs to control the video bus,
> > and request the video source to start/stop the video stream.
>
> Oh, I'm very well aware of the various flavours of funkiness that DSI
> panels come in. But it's wrong to say that the panel needs to control
> the video bus. There's simply no way that a panel can actually do that.
> It is true, however, that in order to make this work in a maintainable
> fashion, the DSI panel *driver* may need to control the DSI bus. That's
> an entirely different story.
Sure, but I don't think that's really related to the DT bindings. We don't
have to model every electrical signal in a detailed way that matches the
direction of the electrons flow :-) What we need to model is a connection
between a display controller and a panel (possibly with a direction). What I'd
like to do is to express that link in a way that can also express more complex
pipeline topologies. I don't want to make it overly complex, I had hoped that
my DT bindings proposal would be a good approach as it's both generic and
still pretty simple.
> Again, that's exactly how I2C works as well. I2C slaves, by definition,
> do not control the I2C bus. However, I2C client drivers can do so by
> using the (master) services provided by the I2C controller.
>
> So for DSI what we could easily do is provide a DSI "adapter" to the
> panel when it is attached to the DSI controller. If the DSI adapter
> implements all of the required services, then the DSI panel driver is
> easily capable of handling all the required special quirks.
>
> I'm Cc'ing Rob and Daniel, both of whom have been working on this
> lately. Actually I'm not sure if Daniel was directly involved, but at
> least somebody from Intel's team was working on it.
>
> > > > However, one thing unclear is where the panel node should be. You seem
> > > > to have a DSI panel here. If the panel is truly not controlled in any
> > > > way, maybe having the panel as a normal platform device is ok. But if
> > > > the DSI panel is controlled with DSI messages, should it be a child of
> > > > the dsi-controller node, the same way i2c peripherals are children of
> > > > i2c master?
> > >
> > > That's one possibility. In this particular case it's not even necessary
> > > to use any of DSIs control methods to talk to the panel. The panel only
> > > receives the data stream and "just works".
> > >
> > > Even if the panel were to be controlled via DSI, I think keeping it as
> > > a separate device node is equally valid. It's really boils down to what
> > > I already said above. The output uses the panel as a resource, similar
> > > to how other devices might use a GPIO controller to use a GPIO pin, or
> > > an interrupt controller to request an IRQ.
> >
> > Except that the display controller can also provides resources to the
> > panel. For DSI panels that support DSI commands, the control bus is
> > provided by the display controller. Much like an I2C device must be a
> > child of its I2C controller node, the DSI panel device should then be a
> > child of the display controller node.
>
> I don't think resources is the right term here. The DSI controller
> provides services. That's traditionally been handled in DT by handing
> out phandles to the service providers.
>
> That said I see why it would make sense to make a DSI panel the child of
> the DSI controller node, so we agree at least partially. I don't like it
> all that much because it makes handling of panels inconsistent across
> various types of output and therefore makes it more complicated to
> support it drivers, but I'm sure I can live with it if somebody wants to
> enforce that.
>
> > > > > I do agree though that it might be useful to tweak the mode in case
> > > > > the default one doesn't work. How about we provide a means to
> > > > > override the mode encoded in the driver using one specified in the
> > > > > DT? That's obviously a backwards-compatible change, so it could be
> > > > > added if or when it becomes necessary.
> > > >
> > > > This sounds good to me.
> > > >
> > > > Although maybe it'd be good to have the driver compatible with
> > > > something like "generic-panel", so that you could have:
> > > >
> > > > compatible = "foo,specific-panel", "generic-panel";
> > > >
> > > > and if there's no need for any power/gpio setup for your board, you
> > > > may skip adding "foo,specific-panel" support to the panel driver.
> > > > Later, somebody else may need to implement fine grained power/gpio
> > > > support for "foo,specific-panel", and it would just work.
> > > >
> > > > Maybe that would help us with the problem of adding support in the
> > > > driver for a hundred different simple panels each used only once on a
> > > > specific board.
> > >
> > > Sure, that all sounds like reasonable additions. All of the suggestions
> > > are even backwards-compatible and hence can be added when needed without
> > > breaking compatibility with existing users of the binding.
> >
> > What's wrong with moving the three hardcoded modes we already have in the
> > driver to DT before pushing this to mainline ?
>
> I think I've answered that in a different subthread.
Then I might not have understood your answer :-)
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:17 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FD12D.3000200-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1564 bytes --]
Hi Tomi,
On Thursday 17 October 2013 14:59:41 Tomi Valkeinen wrote:
> On 17/10/13 14:51, Laurent Pinchart wrote:
> >> I'm not sure if there's a specific need for the port or endpoint nodes
> >> in cases like the above. Even if we have common properties describing
> >> the endpoint, I guess they could just be in the parent node.
> >>
> >> panel {
> >> remote = <&dc>;
> >> common-video-property = <asd>;
> >> };
> >>
> >> The above would imply one port and one endpoint. Would that work? If we
> >> had a function like parse_endpoint(node), we could just point it to
> >> either a real endpoint node, or to the device's node.
> >
> > You reference the display controller here, not a specific display
> > controller output. Don't most display controllers have several outputs ?
>
> Sure. Then the display controller could have more verbose description.
> But the panel could still have what I wrote above, except the 'remote'
> property would point to a real endpoint node inside the dispc node, not
> to the dispc node.
>
> This would, of course, need some extra code to handle the different
> cases, but just from DT point of view, I think all the relevant
> information is there.
There's many ways to describe the same information in DT. While we could have
DT bindings that use different descriptions for different devices and still
support all of them in our code, why should we opt for that option that will
make the implementation much more complex when we can describe connections in
a simple and generic way ?
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 12:14 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017114139.GD10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 13265 bytes --]
On Thursday 17 October 2013 13:41:40 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> > On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
> [...]
>
> > CDF needs to model video pipelines that can be more complex than just a
> > display controller with a single output connected to a panel with a single
> > input. For that reason the DT bindings need to describe connections in a
> > generic enough way. The above DT fragment might be slightly more complex
> > than one would expect when thinking about the really simple case only,
> > but it's nowhere near overly complex in my opinion.
> >
> > Please note that, when a device has as single port, the ports node can be
> > omitted, and the port doesn't need to be numbered. You would then end up
> > with>
> > dc: display-controller {
> > port {
> > remote-endpoint = <&panel>;
> > };
> > };
> >
> > panel: panel {
> > port {
> > remote-endpoint = <&dc>;
> > };
> > };
> >
> > I don't think there's a way to simplify it further.
>
> That binding looks completely inconsistent to me. If we can write it either
> using a ports node with port@X subnodes, in my opinion we can equally well
> just use a unidirectional link for the case where we don't need the link
> back from the panel to the output. Where's the difference?
>
> Consider for example the case where an LVDS panel is connected to a simple
> LVDS output. For what possible reason would the panel need to access the
> output?
The point is that the video pipeline must be described in DT. Having a per-
device way to represent connections would be a nightmare to support from an
implementation point of view, hence the need for a generic way to describe
them.
> > > > I also plan to specify video bus parameters in DT for CDF, but this
> > > > hasn't been finalized yet.
> > >
> > > That effectively blocks any of the code that I've been working on until
> > > CDF has been merged. It's been discussed for over a year now, and there
> > > has even been significant pushback on using CDF in DRM, so even if CDF
> > > is eventually merged, that will still leave DRM with no way to turn on
> > > a simple panel.
> >
> > I'm probably even more concerned about the long time all this took and
> > will still take than you are, sincerely. I'm obviously to blame for not
> > being able to explain the problems and solutions correctly, as I see no
> > way why people would push back if they realized that the other solutions
> > that have been proposed, far from being future-proof, won't even be
> > present-proof. We would be shooting ourselves in the foot, but what can I
> > do when too many people want to do so ?
>
> I'm curious why you think this current proposal isn't present-proof. All
> three panels that I've tested with work with this proposal currently and
> I'm convinced that many more setups can be covered using something as
> simple as this.
My apologies for not being clear enough. I wasn't implying that your proposal
isn't present-proof, I was talking about the counter-proposals that have been
made in reaction to CDF.
> > I'll still keep trying of course, but not for years.I have yet to
> > see someone proposing a viable solution to share drivers between DRM and
> > V4L2, which is a real pressing requirement, partly due to DT.
>
> Yeah... I still don't get that side of the argument. Why exactly do we need
> to share the drivers again?
Think about a scaler IP in a SoC or FPGA, with one instance used in a camera
capture pipeline, supported by a V4L2 driver, and one instance used in a video
output pipeline, supported by a DRM driver. We want a single driver for that
IP core. Same story for video encoders.
> If we really need to support display output within V4L2, shouldn't we
> instead improve interoperability between the two and use DRM/KMS exclusively
> for the output part? Then the problem of having to share drivers between
> subsystems goes away and we'll actually be using the right subsystem for the
> right job at hand.
In the long term we definitely should improve interoperability between the
two. I'd go further than that, having one API for video capture and one API
for video output doesn't make much sense, except for historical reasons.
> Of course none of that is relevant to the topic of DT bindings, which is
> what we should probably be concentrating on.
It actually is. Given that DT bindings must describe hardware, the scaler (or
encoder, or other entity) would use the same bindings regardless of the Linux
subsystem that will be involved. We thus need to make sure the bindings will
be usable on both sides.
> Nobody's been able to come up with a valid reason why the proposed binding
> is broken. It works for the tested devices (likely many more as well) and I
> don't forsee why it can't be future-proof. Why would it suddenly need to
> change? And even if it needed to change, why would the changes be backwards-
> incompatible rather than simply additional features that don't impact
> existing users?
>
> > Regarding the use of CDF in DRM, please note that it will be optional for
> > drivers to switch to the new framework. Nothing should hopefully require
> > existing drivers to be converted, drivers that want to make use of CDF to
> > support panels will be welcome to do so, but they big DRM drivers for
> > desktop GPUs won't be affected.
>
> I'm not at all opposed to the idea of CDF or the problems it tries to
> address. I don't think it necessarily should be a separate framework for
> reasons explained earlier. But even if it were to end up in a separate
> framework there's absolutely nothing preventing us from adding a DRM panel
> driver that hooks into CDF as a "backend" of sorts for panels that require
> something more complex than the simple-panel binding.
Once again it's not about panel having complex needs, but more about using
simple panels in complex pipelines. I'm fine with the drm_panel
infrastructure, what I would like is DT bindings that describe connections in
a more future-proof way. The rest is fine.
> > > I keep wondering why we absolutely must have compatibility between CDF
> > > and this simple panel binding. DT content is supposed to concern itself
> > > with the description of hardware only. What about the following isn't an
> > > accurate description of the panel hardware?
> > >
> > > panel: panel {
> > > compatible = "cptt,claa101wb01";
> > >
> > > power-supply = <&vdd_pnl_reg>;
> > > enable-gpios = <&gpio 90 0>;
> > >
> > > backlight = <&backlight>;
> > > };
> > >
> > > dsi-controller {
> > > panel = <&panel>;
> > > };
> > >
> > > Note how the above is also not incompatible with anything that the CDF
> > > (to be) mandates, so it could easily be extended with any nodes or
> > > properties that CDF needs to work properly without breaking backwards-
> > > compatibility.
> >
> > As mentioned above, CDF needs to describe more complex pipelines. To make
> > the driver life as simple as possible the core code will handle the
> > pipeline description in the DT bindings, and it does require it to be
> > generic enough. You can of course describe connections in various ways
> > depending on the hardware, which would make the DT-related code
> > needlessly overcomplex. One of the issue with the above description is
> > that there's no way to go from the panel to the display controller, which
> > might be a show stopper in the future when code requires that. A
> > workaround would be to look through the whole DT for the reverse link,
> > which would be pretty inefficient.
>
> But that's precisely the point. Why would we need to go back from the panel
> to the display controller? Especially for dumb panels that can't or don't
> have to be configured in any way. Even if they needed some sort of setup,
> why can't that be done from the display controller/output.
>
> Even given a setup where a DSI controller needs to write some registers in a
> panel upon initialization, I don't see why the reverse connection needs to
> be described. The fact alone that an output dereferences a panel's phandle
> should be enough to connect both of them and have any panel driver use the
> DSI controller that it's been attached to for the programming.
>
> That's very much analogous to how I2C works. There are no connections back
> to the I2C master from the slaves. Yet each I2C client driver manages to use
> the services provided by the I2C master to perform transactions on the I2C
> bus. In a similar way the DSI controller is the bus master that talks to DSI
> panels. DSI panels don't actively talk to the DSI controller.
It's all about modeling video pipeline graphs in DT. To be able to walk the
graph we need to describe connections. Not having bidirectional information
means that we restrict the points at which we can start walking the graph,
potentially making our life much more difficult in the future.
What's so wrong about adding a port node and link information to the panel DT
node ? It describe what's there: the panel has one input, why not make that
explicit ?
> > > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > > +{
> > > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > > + int err;
> > > > > +
> > > > > + if (p->enabled)
> > > > > + return;
> > > > > +
> > > > > + err = regulator_enable(p->supply);
> > > > > + if (err < 0)
> > > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err);
> > > >
> > > > Is that really a non-fatal error ? Shouldn't the enable operation
> > > > return an int ?
> > >
> > > There's no way to propagate this in DRM, so why go through the trouble
> > > of returning the error? Furthermore, there's nothing that the caller
> > > could do to remedy the situation anyway.
> >
> > That's a DRM issue, which could be fixed. While the caller can't remedy
> > the situation, it should at least report the error to the application
> > instead of silently ignoring it.
>
> Perhaps. It's not really relevant to the discussion and can always be
> fixed in a subsequent patch.
Most things can be fixed by a subsequent patch, that's not a good enough
reason not to address the known problems before pushing the code to mainline
(to clarify my point of view, there's no need to fix DRM to use the return
value before pushing this patch set to mainline, but I'd like a v2 with an int
return value).
> > > > > +static const struct drm_display_mode auo_b101aw03_mode = {
> > > > > + .clock = 51450,
> > > > > + .hdisplay = 1024,
> > > > > + .hsync_start = 1024 + 156,
> > > > > + .hsync_end = 1024 + 156 + 8,
> > > > > + .htotal = 1024 + 156 + 8 + 156,
> > > > > + .vdisplay = 600,
> > > > > + .vsync_start = 600 + 16,
> > > > > + .vsync_end = 600 + 16 + 6,
> > > > > + .vtotal = 600 + 16 + 6 + 16,
> > > > > + .vrefresh = 60,
> > > > > +};
> > > >
> > > > Instead of hardcoding the modes in the driver, which would then
> > > > require to be updated for every new simple panel model (and we know
> > > > there are lots of them), why don't you specify the modes in the panel
> > > > DT node ? The simple panel driver would then become much more generic.
> > > > It would also allow board designers to tweak h/v sync timings
> > > > depending on the system requirements.
> > >
> > > Sigh... we keep second-guessing ourselves. Back at the time when power
> > > sequences were designed (and NAKed at the last minute), we all decided
> > > that the right thing to do would be to use specific compatible values
> > > for individual panels, because that would allow us to encode the power
> > > sequencing within the driver. And when we already have the panel model
> > > encoded in the compatible value, we might just as well encode the mode
> > > within the driver for that panel. Otherwise we'll have to keep adding
> > > the same mode timings for every board that uses the same panel.
> > >
> > > I do agree though that it might be useful to tweak the mode in case the
> > > default one doesn't work. How about we provide a means to override the
> > > mode encoded in the driver using one specified in the DT? That's
> > > obviously a backwards-compatible change, so it could be added if or when
> > > it becomes necessary.
> >
> > I share Tomi's point of view here. Your three panels use the same power
> > sequence, so I believe we should have a generic panel compatible string
> > that would use modes described in DT for the common case. Only if a panel
> > required something more complex which can't (or which could, but won't
> > for any reason) accurately be described in DT would you need to extend
> > the driver.
>
> I don't see the point quite frankly. You seem to be assuming that every
> panel will only be used in a single board.
No, but in practice that's often the case, at least for boards with .dts files
in the mainline kernel.
> However what you're proposing will require the mode timings to be repeated
> in every board's DT file, if the same panel is ever used on more than a
> single board.
Is that a problem ? You could #include a .dtsi for the panel that would
specify the video mode if you want to avoid multiple copies.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 12:12 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FCF07.6070006-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]
On Thu, Oct 17, 2013 at 02:50:31PM +0300, Tomi Valkeinen wrote:
> On 17/10/13 14:05, Thierry Reding wrote:
>
> >> That's quite similar to how my current out-of-mainline OMAP DSS DT
> >> bindings work. The difference is that I have a reference from the panel
> >> node to the video source (dsi-controller), instead of the other way
> >> around. I just find it more natural. It works the same way as, say,
> >> regulators, gpios, etc.
> >
> > I suppose that depends on the way you look at it. In the above proposal
> > I consider the output (DSI controller) to use the panel as a resource
> > that provides a certain set of services (query mode timings, provide a
> > way to enable or disable the panel). Similarly the panel uses the
> > backlight as a resource that provides a certain set of services (such as
> > changing the brightness).
> >
> > The above also follows the natural order of control. The panel has no
> > way to control the DSI output. However, it is the output that controls
> > when a panel is required and turn it on as appropriate.
>
> I've discussed this issue to death with other people, and for some
> reason I still see this the other way =).
>
> I see the panel using the DSI controller as a resource. The panel driver
> uses the DSI controller to send the video data to the panel device.
> Compare this to, say, i2c client driver, which uses i2c master to send
> data to the i2c device.
>
> In the OMAP DSS driver model, the panel driver is in control. The panel
> driver tells the DSI controller which kind of video timings to use, or
> which bus speed to use, or whether to use low-power or high-speed mode,
> when to enable the DSI clock or DSI video stream, etc.
>
> With a simple panel the model you describe works usually fine. The
> reason I went with the different model is that we had rather complex
> display peripherals, that needed more specific control of the bus.
>
> A simple example could be the enable sequence. I presume in your model
> the DSI controller's enable would be maybe something like:
>
> configure_dsi_timings(panel->get_timings());
> enable_dsi_output();
> panel->enable();
>
> What if the DSI peripheral requires an enable sequence of, say:
>
> - enable DSI clock
> - use LP mode
> - send some config messages
> - re-configure DSI clock to higher speed
> - send some more config messages
> - enable HS mode
> - send some more config messages
> - enable DSI video stream
>
> It gets a bit difficult to manage that kind of setup in a generic way in
> the DSI controller driver. As I see it, the DSI peripheral driver has to
> be in control of the nuances. The same goes for other video buses also,
> but DSI is perhaps one of the most complex ones to support.
I've briefly gone over this in another subthread, but here it is again
for the sake of completeness.
The difference in perception I think comes from the fact that the DSI
panel *driver* may need to control the DSI bus. However, DT describes
the hardware in terms of devices. And from that point of view it is
still the DSI _control_ler that _control_s the panel. There's no
electrical way that the panel can take possession of the bus. Well,
there's BTA, but even that happens under supervision of the DSI
controller.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 12:06 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
Rob Clark, Daniel Vetter
In-Reply-To: <3610893.n6xJ6DUJ0E@avalon>
[-- Attachment #1: Type: text/plain, Size: 6882 bytes --]
On Thu, Oct 17, 2013 at 01:15:22PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
>
> On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > > On 17/10/13 11:53, Thierry Reding wrote:
> > > > I keep wondering why we absolutely must have compatibility between CDF
> > > > and this simple panel binding. DT content is supposed to concern itself
> > > > with the description of hardware only. What about the following isn't an
> > > > accurate description of the panel hardware?
> > > >
> > > > panel: panel {
> > > > compatible = "cptt,claa101wb01";
> > > >
> > > > power-supply = <&vdd_pnl_reg>;
> > > > enable-gpios = <&gpio 90 0>;
> > > >
> > > > backlight = <&backlight>;
> > > > };
> > > >
> > > > dsi-controller {
> > > > panel = <&panel>;
> > > > };
> > >
> > > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > > bindings work. The difference is that I have a reference from the panel
> > > node to the video source (dsi-controller), instead of the other way
> > > around. I just find it more natural. It works the same way as, say,
> > > regulators, gpios, etc.
> >
> > I suppose that depends on the way you look at it. In the above proposal
> > I consider the output (DSI controller) to use the panel as a resource
> > that provides a certain set of services (query mode timings, provide a
> > way to enable or disable the panel). Similarly the panel uses the
> > backlight as a resource that provides a certain set of services (such as
> > changing the brightness).
> >
> > The above also follows the natural order of control. The panel has no
> > way to control the DSI output. However, it is the output that controls
> > when a panel is required and turn it on as appropriate.
>
> I'm no DSI expert, but I know enough about it to be sure that Tomi will
> disagree. DSI panels can have complex power sequences that require the input
> stream to be finely controlled (for instance it might require a clock without
> video frames for some time, switch a GPIO or regulator, send a command to the
> panel, and then only get video frames). For that reason all developers I've
> talked to who have an in-depth knowledge of DSI and DSI panels told me that
> the panel needs to control the video bus, and request the video source to
> start/stop the video stream.
Oh, I'm very well aware of the various flavours of funkiness that DSI
panels come in. But it's wrong to say that the panel needs to control
the video bus. There's simply no way that a panel can actually do that.
It is true, however, that in order to make this work in a maintainable
fashion, the DSI panel *driver* may need to control the DSI bus. That's
an entirely different story.
Again, that's exactly how I2C works as well. I2C slaves, by definition,
do not control the I2C bus. However, I2C client drivers can do so by
using the (master) services provided by the I2C controller.
So for DSI what we could easily do is provide a DSI "adapter" to the
panel when it is attached to the DSI controller. If the DSI adapter
implements all of the required services, then the DSI panel driver is
easily capable of handling all the required special quirks.
I'm Cc'ing Rob and Daniel, both of whom have been working on this
lately. Actually I'm not sure if Daniel was directly involved, but at
least somebody from Intel's team was working on it.
> > > However, one thing unclear is where the panel node should be. You seem
> > > to have a DSI panel here. If the panel is truly not controlled in any
> > > way, maybe having the panel as a normal platform device is ok. But if
> > > the DSI panel is controlled with DSI messages, should it be a child of
> > > the dsi-controller node, the same way i2c peripherals are children of
> > > i2c master?
> >
> > That's one possibility. In this particular case it's not even necessary
> > to use any of DSIs control methods to talk to the panel. The panel only
> > receives the data stream and "just works".
> >
> > Even if the panel were to be controlled via DSI, I think keeping it as
> > a separate device node is equally valid. It's really boils down to what
> > I already said above. The output uses the panel as a resource, similar
> > to how other devices might use a GPIO controller to use a GPIO pin, or
> > an interrupt controller to request an IRQ.
>
> Except that the display controller can also provides resources to the panel.
> For DSI panels that support DSI commands, the control bus is provided by the
> display controller. Much like an I2C device must be a child of its I2C
> controller node, the DSI panel device should then be a child of the display
> controller node.
I don't think resources is the right term here. The DSI controller
provides services. That's traditionally been handled in DT by handing
out phandles to the service providers.
That said I see why it would make sense to make a DSI panel the child of
the DSI controller node, so we agree at least partially. I don't like it
all that much because it makes handling of panels inconsistent across
various types of output and therefore makes it more complicated to
support it drivers, but I'm sure I can live with it if somebody wants to
enforce that.
> > > > I do agree though that it might be useful to tweak the mode in case the
> > > > default one doesn't work. How about we provide a means to override the
> > > > mode encoded in the driver using one specified in the DT? That's
> > > > obviously a backwards-compatible change, so it could be added if or when
> > > > it becomes necessary.
> > >
> > > This sounds good to me.
> > >
> > > Although maybe it'd be good to have the driver compatible with something
> > > like "generic-panel", so that you could have:
> > >
> > > compatible = "foo,specific-panel", "generic-panel";
> > >
> > > and if there's no need for any power/gpio setup for your board, you may
> > > skip adding "foo,specific-panel" support to the panel driver. Later,
> > > somebody else may need to implement fine grained power/gpio support for
> > > "foo,specific-panel", and it would just work.
> > >
> > > Maybe that would help us with the problem of adding support in the
> > > driver for a hundred different simple panels each used only once on a
> > > specific board.
> >
> > Sure, that all sounds like reasonable additions. All of the suggestions
> > are even backwards-compatible and hence can be added when needed without
> > breaking compatibility with existing users of the binding.
>
> What's wrong with moving the three hardcoded modes we already have in the
> driver to DT before pushing this to mainline ?
I think I've answered that in a different subthread.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 11:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <4885946.7Zgjf9zNXx@avalon>
[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]
On 17/10/13 14:51, Laurent Pinchart wrote:
>> I'm not sure if there's a specific need for the port or endpoint nodes
>> in cases like the above. Even if we have common properties describing
>> the endpoint, I guess they could just be in the parent node.
>>
>> panel {
>> remote = <&dc>;
>> common-video-property = <asd>;
>> };
>>
>> The above would imply one port and one endpoint. Would that work? If we
>> had a function like parse_endpoint(node), we could just point it to
>> either a real endpoint node, or to the device's node.
>
> You reference the display controller here, not a specific display controller
> output. Don't most display controllers have several outputs ?
Sure. Then the display controller could have more verbose description.
But the panel could still have what I wrote above, except the 'remote'
property would point to a real endpoint node inside the dispc node, not
to the dispc node.
This would, of course, need some extra code to handle the different
cases, but just from DT point of view, I think all the relevant
information is there.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 11:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FCB95.6070401-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]
Hi Tomi,
On Thursday 17 October 2013 14:35:49 Tomi Valkeinen wrote:
> On 17/10/13 14:02, Laurent Pinchart wrote:
> >> Okay, so if I understand correctly, translating those bindings to panel
> >>
> >> nodes would look somewhat like this:
> >> dc: display-controller {
> >> ports {
> >> port@0 {
> >> remote-endpoint = <&panel>;
> >> };
> >> };
> >> };
> >>
> >> panel: panel {
> >> ports {
> >> port@0 {
> >> remote-endpoint = <&dc>;
> >> };
> >> };
> >> };
> >>
> >> The above leaves out any of the other, non-relevant properties. Does
> >> that sound about right?
> >
> > Yes it does.
>
> It does?
>
> Shouldn't it be something like:
>
> panel {
> ports {
> port@0 {
> endpoint@0 {
> remote = <&dc>;
> };
> };
> };
> };
>
> And simplified:
>
> panel {
> port {
> endpoint@0 {
> remote = <&dc>;
> };
> };
> };
>
> You do need a node for the endpoint, a remote-endpoint property is not
> enough.
My bad, you'r absolutely right. More sleep is needed.
(And while we're at it, the remote-endpoint properties must point to an
endpoint, not the device DT node.
> > Please note that, when a device has as single port, the ports node can be
> > omitted, and the port doesn't need to be numbered. You would then end up
> > with>
> > dc: display-controller {
> > port {
> > remote-endpoint = <&panel>;
> > };
> > };
> >
> > panel: panel {
> > port {
> > remote-endpoint = <&dc>;
> > };
> > };
> >
> > I don't think there's a way to simplify it further.
>
> I'm not sure if there's a specific need for the port or endpoint nodes
> in cases like the above. Even if we have common properties describing
> the endpoint, I guess they could just be in the parent node.
>
> panel {
> remote = <&dc>;
> common-video-property = <asd>;
> };
>
> The above would imply one port and one endpoint. Would that work? If we
> had a function like parse_endpoint(node), we could just point it to
> either a real endpoint node, or to the device's node.
You reference the display controller here, not a specific display controller
output. Don't most display controllers have several outputs ?
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 11:50 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017110517.GC10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]
On 17/10/13 14:05, Thierry Reding wrote:
>> That's quite similar to how my current out-of-mainline OMAP DSS DT
>> bindings work. The difference is that I have a reference from the panel
>> node to the video source (dsi-controller), instead of the other way
>> around. I just find it more natural. It works the same way as, say,
>> regulators, gpios, etc.
>
> I suppose that depends on the way you look at it. In the above proposal
> I consider the output (DSI controller) to use the panel as a resource
> that provides a certain set of services (query mode timings, provide a
> way to enable or disable the panel). Similarly the panel uses the
> backlight as a resource that provides a certain set of services (such as
> changing the brightness).
>
> The above also follows the natural order of control. The panel has no
> way to control the DSI output. However, it is the output that controls
> when a panel is required and turn it on as appropriate.
I've discussed this issue to death with other people, and for some
reason I still see this the other way =).
I see the panel using the DSI controller as a resource. The panel driver
uses the DSI controller to send the video data to the panel device.
Compare this to, say, i2c client driver, which uses i2c master to send
data to the i2c device.
In the OMAP DSS driver model, the panel driver is in control. The panel
driver tells the DSI controller which kind of video timings to use, or
which bus speed to use, or whether to use low-power or high-speed mode,
when to enable the DSI clock or DSI video stream, etc.
With a simple panel the model you describe works usually fine. The
reason I went with the different model is that we had rather complex
display peripherals, that needed more specific control of the bus.
A simple example could be the enable sequence. I presume in your model
the DSI controller's enable would be maybe something like:
configure_dsi_timings(panel->get_timings());
enable_dsi_output();
panel->enable();
What if the DSI peripheral requires an enable sequence of, say:
- enable DSI clock
- use LP mode
- send some config messages
- re-configure DSI clock to higher speed
- send some more config messages
- enable HS mode
- send some more config messages
- enable DSI video stream
It gets a bit difficult to manage that kind of setup in a generic way in
the DSI controller driver. As I see it, the DSI peripheral driver has to
be in control of the nuances. The same goes for other video buses also,
but DSI is perhaps one of the most complex ones to support.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 11:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <12566222.aRYc4MDoOm@avalon>
[-- Attachment #1: Type: text/plain, Size: 10518 bytes --]
On Thu, Oct 17, 2013 at 01:02:38PM +0200, Laurent Pinchart wrote:
> On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
[...]
> CDF needs to model video pipelines that can be more complex than just a
> display controller with a single output connected to a panel with a single
> input. For that reason the DT bindings need to describe connections in a
> generic enough way. The above DT fragment might be slightly more complex than
> one would expect when thinking about the really simple case only, but it's
> nowhere near overly complex in my opinion.
>
> Please note that, when a device has as single port, the ports node can be
> omitted, and the port doesn't need to be numbered. You would then end up with
>
> dc: display-controller {
> port {
> remote-endpoint = <&panel>;
> };
> };
>
> panel: panel {
> port {
> remote-endpoint = <&dc>;
> };
> };
>
> I don't think there's a way to simplify it further.
That binding looks completely inconsistent to me. If we can write it
either using a ports node with port@X subnodes, in my opinion we can
equally well just use a unidirectional link for the case where we
don't need the link back from the panel to the output. Where's the
difference?
Consider for example the case where an LVDS panel is connected to a
simple LVDS output. For what possible reason would the panel need to
access the output?
> > > I also plan to specify video bus parameters in DT for CDF, but this hasn't
> > > been finalized yet.
> >
> > That effectively blocks any of the code that I've been working on until
> > CDF has been merged. It's been discussed for over a year now, and there
> > has even been significant pushback on using CDF in DRM, so even if CDF
> > is eventually merged, that will still leave DRM with no way to turn on
> > a simple panel.
>
> I'm probably even more concerned about the long time all this took and will
> still take than you are, sincerely. I'm obviously to blame for not being able
> to explain the problems and solutions correctly, as I see no way why people
> would push back if they realized that the other solutions that have been
> proposed, far from being future-proof, won't even be present-proof. We would
> be shooting ourselves in the foot, but what can I do when too many people want
> to do so ?
I'm curious why you think this current proposal isn't present-proof. All
three panels that I've tested with work with this proposal currently and
I'm convinced that many more setups can be covered using something as
simple as this.
> I'll still keep trying of course, but not for years.I have yet to
> see someone proposing a viable solution to share drivers between DRM and V4L2,
> which is a real pressing requirement, partly due to DT.
Yeah... I still don't get that side of the argument. Why exactly do we
need to share the drivers again? If we really need to support display
output within V4L2, shouldn't we instead improve interoperability
between the two and use DRM/KMS exclusively for the output part? Then
the problem of having to share drivers between subsystems goes away and
we'll actually be using the right subsystem for the right job at hand.
Of course none of that is relevant to the topic of DT bindings, which is
what we should probably be concentrating on. Nobody's been able to come
up with a valid reason why the proposed binding is broken. It works for
the tested devices (likely many more as well) and I don't forsee why it
can't be future-proof. Why would it suddenly need to change? And even if
it needed to change, why would the changes be backwards-incompatible
rather than simply additional features that don't impact existing users?
> Regarding the use of CDF in DRM, please note that it will be optional for
> drivers to switch to the new framework. Nothing should hopefully require
> existing drivers to be converted, drivers that want to make use of CDF to
> support panels will be welcome to do so, but they big DRM drivers for desktop
> GPUs won't be affected.
I'm not at all opposed to the idea of CDF or the problems it tries to
address. I don't think it necessarily should be a separate framework for
reasons explained earlier. But even if it were to end up in a separate
framework there's absolutely nothing preventing us from adding a DRM
panel driver that hooks into CDF as a "backend" of sorts for panels that
require something more complex than the simple-panel binding.
> > I keep wondering why we absolutely must have compatibility between CDF
> > and this simple panel binding. DT content is supposed to concern itself
> > with the description of hardware only. What about the following isn't an
> > accurate description of the panel hardware?
> >
> > panel: panel {
> > compatible = "cptt,claa101wb01";
> >
> > power-supply = <&vdd_pnl_reg>;
> > enable-gpios = <&gpio 90 0>;
> >
> > backlight = <&backlight>;
> > };
> >
> > dsi-controller {
> > panel = <&panel>;
> > };
> >
> > Note how the above is also not incompatible with anything that the CDF
> > (to be) mandates, so it could easily be extended with any nodes or
> > properties that CDF needs to work properly without breaking backwards-
> > compatibility.
>
> As mentioned above, CDF needs to describe more complex pipelines. To make the
> driver life as simple as possible the core code will handle the pipeline
> description in the DT bindings, and it does require it to be generic enough.
> You can of course describe connections in various ways depending on the
> hardware, which would make the DT-related code needlessly overcomplex. One of
> the issue with the above description is that there's no way to go from the
> panel to the display controller, which might be a show stopper in the future
> when code requires that. A workaround would be to look through the whole DT
> for the reverse link, which would be pretty inefficient.
But that's precisely the point. Why would we need to go back from the
panel to the display controller? Especially for dumb panels that can't
or don't have to be configured in any way. Even if they needed some sort
of setup, why can't that be done from the display controller/output.
Even given a setup where a DSI controller needs to write some registers
in a panel upon initialization, I don't see why the reverse connection
needs to be described. The fact alone that an output dereferences a
panel's phandle should be enough to connect both of them and have any
panel driver use the DSI controller that it's been attached to for the
programming.
That's very much analogous to how I2C works. There are no connections
back to the I2C master from the slaves. Yet each I2C client driver
manages to use the services provided by the I2C master to perform
transactions on the I2C bus. In a similar way the DSI controller is the
bus master that talks to DSI panels. DSI panels don't actively talk to
the DSI controller.
> > > > +static void panel_simple_enable(struct drm_panel *panel)
> > > > +{
> > > > + struct panel_simple *p = to_panel_simple(panel);
> > > > + int err;
> > > > +
> > > > + if (p->enabled)
> > > > + return;
> > > > +
> > > > + err = regulator_enable(p->supply);
> > > > + if (err < 0)
> > > > + dev_err(panel->dev, "failed to enable supply: %d\n", err);
> > >
> > > Is that really a non-fatal error ? Shouldn't the enable operation return
> > > an int ?
> >
> > There's no way to propagate this in DRM, so why go through the trouble
> > of returning the error? Furthermore, there's nothing that the caller
> > could do to remedy the situation anyway.
>
> That's a DRM issue, which could be fixed. While the caller can't remedy the
> situation, it should at least report the error to the application instead of
> silently ignoring it.
Perhaps. It's not really relevant to the discussion and can always be
fixed in a subsequent patch.
> > > > +static const struct drm_display_mode auo_b101aw03_mode = {
> > > > + .clock = 51450,
> > > > + .hdisplay = 1024,
> > > > + .hsync_start = 1024 + 156,
> > > > + .hsync_end = 1024 + 156 + 8,
> > > > + .htotal = 1024 + 156 + 8 + 156,
> > > > + .vdisplay = 600,
> > > > + .vsync_start = 600 + 16,
> > > > + .vsync_end = 600 + 16 + 6,
> > > > + .vtotal = 600 + 16 + 6 + 16,
> > > > + .vrefresh = 60,
> > > > +};
> > >
> > > Instead of hardcoding the modes in the driver, which would then require to
> > > be updated for every new simple panel model (and we know there are lots
> > > of them), why don't you specify the modes in the panel DT node ? The
> > > simple panel driver would then become much more generic. It would also
> > > allow board designers to tweak h/v sync timings depending on the system
> > > requirements.
> >
> > Sigh... we keep second-guessing ourselves. Back at the time when power
> > sequences were designed (and NAKed at the last minute), we all decided
> > that the right thing to do would be to use specific compatible values
> > for individual panels, because that would allow us to encode the power
> > sequencing within the driver. And when we already have the panel model
> > encoded in the compatible value, we might just as well encode the mode
> > within the driver for that panel. Otherwise we'll have to keep adding
> > the same mode timings for every board that uses the same panel.
> >
> > I do agree though that it might be useful to tweak the mode in case the
> > default one doesn't work. How about we provide a means to override the
> > mode encoded in the driver using one specified in the DT? That's
> > obviously a backwards-compatible change, so it could be added if or when
> > it becomes necessary.
>
> I share Tomi's point of view here. Your three panels use the same power
> sequence, so I believe we should have a generic panel compatible string that
> would use modes described in DT for the common case. Only if a panel required
> something more complex which can't (or which could, but won't for any reason)
> accurately be described in DT would you need to extend the driver.
I don't see the point quite frankly. You seem to be assuming that every
panel will only be used in a single board. However what you're proposing
will require the mode timings to be repeated in every board's DT file,
if the same panel is ever used on more than a single board.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 6/6] omapdss: hdmi4/hdmi5: set regulator voltage to required level
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja, Somnath Mukherjee
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
It's not sufficient to request the regulator. We need to set the IO voltage as
well if the min and max voltage values of the regulator differ.
However, if the regulator on a platform doesn't permit the range we requested
for, or is a fixed voltage regulator, we just give a warning that we didn't get
voltage range we requested for. The recommended voltage range for vdda_hdmi as
per the HDMI PHY spec is 1.8V to 1.98V for OMAP4 and 1.5V to 1.8V for OMAP5.
Originally worked on by Somnath Mukherjee <somnath@ti.com>
Cc: Somnath Mukherjee <somnath@ti.com>
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi4.c | 5 +++++
drivers/video/omap2/dss/hdmi5.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/drivers/video/omap2/dss/hdmi4.c b/drivers/video/omap2/dss/hdmi4.c
index e140096..95638b8 100644
--- a/drivers/video/omap2/dss/hdmi4.c
+++ b/drivers/video/omap2/dss/hdmi4.c
@@ -83,6 +83,7 @@ static void hdmi_runtime_put(void)
static int hdmi_init_regulator(void)
{
+ int r;
struct regulator *reg;
if (hdmi.vdda_hdmi_dac_reg != NULL)
@@ -99,6 +100,10 @@ static int hdmi_init_regulator(void)
return PTR_ERR(reg);
}
+ r = regulator_set_voltage(reg, 1800000, 1980000);
+ if (r)
+ DSSWARN("can't set regulator voltage");
+
hdmi.vdda_hdmi_dac_reg = reg;
return 0;
diff --git a/drivers/video/omap2/dss/hdmi5.c b/drivers/video/omap2/dss/hdmi5.c
index 5a1cfd6..aa91f4e 100644
--- a/drivers/video/omap2/dss/hdmi5.c
+++ b/drivers/video/omap2/dss/hdmi5.c
@@ -85,6 +85,7 @@ static void hdmi_runtime_put(void)
static int hdmi_init_regulator(void)
{
+ int r;
struct regulator *reg;
if (hdmi.vdda_hdmi_dac_reg != NULL)
@@ -96,6 +97,10 @@ static int hdmi_init_regulator(void)
return PTR_ERR(reg);
}
+ r = regulator_set_voltage(reg, 1500000, 1800000);
+ if (r)
+ DSSWARN("can't set regulator voltage");
+
hdmi.vdda_hdmi_dac_reg = reg;
return 0;
--
1.8.1.2
^ permalink raw reply related
* [PATCH 5/6] omapdss: hdmi: add OMAP5/DRA7x hdmi driver
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
The HDMI IP on OMAP5/DRA7x has a different core sub block compared to the one
one OMAP4. The rest of the IPs are the same. Add a hdmi5 platform driver which
calls hdmi5_core.c apis. The rest of the driver uses the same api's for wrapper,
pll and phy, and the common helper funcs for timings.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/Kconfig | 10 +
drivers/video/omap2/dss/Makefile | 2 +
drivers/video/omap2/dss/core.c | 6 +
drivers/video/omap2/dss/dss.h | 3 +
drivers/video/omap2/dss/hdmi.h | 2 +-
drivers/video/omap2/dss/hdmi5.c | 699 ++++++++++++++++++++++++++++++++++++++
drivers/video/omap2/dss/hdmi_wp.c | 2 +-
7 files changed, 722 insertions(+), 2 deletions(-)
create mode 100644 drivers/video/omap2/dss/hdmi5.c
diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
index dde4281..a00932f 100644
--- a/drivers/video/omap2/dss/Kconfig
+++ b/drivers/video/omap2/dss/Kconfig
@@ -69,6 +69,16 @@ config OMAP4_DSS_HDMI
config OMAP4_DSS_HDMI_AUDIO
bool
+config OMAP5_DSS_HDMI
+ bool "OMAP5 HDMI support"
+ default y
+ help
+ HDMI Interface for OMAP5 and SoCs containing the same HDMI core IP.
+
+config OMAP5_DSS_HDMI_AUDIO
+ depends on OMAP5_DSS_HDMI
+ bool
+
config OMAP2_DSS_SDI
bool "SDI support"
default n
diff --git a/drivers/video/omap2/dss/Makefile b/drivers/video/omap2/dss/Makefile
index d3aa91b..89d83be 100644
--- a/drivers/video/omap2/dss/Makefile
+++ b/drivers/video/omap2/dss/Makefile
@@ -12,4 +12,6 @@ omapdss-$(CONFIG_OMAP2_DSS_SDI) += sdi.o
omapdss-$(CONFIG_OMAP2_DSS_DSI) += dsi.o
omapdss-$(CONFIG_OMAP4_DSS_HDMI) += hdmi4.o hdmi_common.o hdmi_wp.o hdmi_pll.o \
hdmi_phy.o hdmi4_core.o
+omapdss-$(CONFIG_OMAP5_DSS_HDMI) += hdmi5.o hdmi_common.o hdmi_wp.o hdmi_pll.o \
+ hdmi_phy.o hdmi5_core.o
ccflags-$(CONFIG_OMAP2_DSS_DEBUG) += -DDEBUG
diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index ffa45c8..6b74f73 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c
@@ -268,6 +268,9 @@ static int (*dss_output_drv_reg_funcs[])(void) __initdata = {
#ifdef CONFIG_OMAP4_DSS_HDMI
hdmi4_init_platform_driver,
#endif
+#ifdef CONFIG_OMAP5_DSS_HDMI
+ hdmi5_init_platform_driver,
+#endif
};
static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
@@ -289,6 +292,9 @@ static void (*dss_output_drv_unreg_funcs[])(void) __exitdata = {
#ifdef CONFIG_OMAP4_DSS_HDMI
hdmi4_uninit_platform_driver,
#endif
+#ifdef CONFIG_OMAP5_DSS_HDMI
+ hdmi5_uninit_platform_driver,
+#endif
};
static bool dss_output_drv_loaded[ARRAY_SIZE(dss_output_drv_reg_funcs)];
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index f777962..bb8a39c 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -430,6 +430,9 @@ void venc_uninit_platform_driver(void) __exit;
int hdmi4_init_platform_driver(void) __init;
void hdmi4_uninit_platform_driver(void) __exit;
+int hdmi5_init_platform_driver(void) __init;
+void hdmi5_uninit_platform_driver(void) __exit;
+
/* RFBI */
int rfbi_init_platform_driver(void) __init;
void rfbi_uninit_platform_driver(void) __exit;
diff --git a/drivers/video/omap2/dss/hdmi.h b/drivers/video/omap2/dss/hdmi.h
index 8fa2121..2b2db2d 100644
--- a/drivers/video/omap2/dss/hdmi.h
+++ b/drivers/video/omap2/dss/hdmi.h
@@ -429,7 +429,7 @@ const struct hdmi_config *hdmi_default_timing(void);
const struct hdmi_config *hdmi_get_timings(int mode, int code);
struct hdmi_cm hdmi_get_code(struct omap_video_timings *timing);
-#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO) || defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
int hdmi_compute_acr(u32 pclk, u32 sample_freq, u32 *n, u32 *cts);
int hdmi_wp_audio_enable(struct hdmi_wp_data *wp, bool enable);
int hdmi_wp_audio_core_req_enable(struct hdmi_wp_data *wp, bool enable);
diff --git a/drivers/video/omap2/dss/hdmi5.c b/drivers/video/omap2/dss/hdmi5.c
new file mode 100644
index 0000000..5a1cfd6
--- /dev/null
+++ b/drivers/video/omap2/dss/hdmi5.c
@@ -0,0 +1,699 @@
+/*
+ * hdmi.c
+ *
+ * HDMI interface DSS driver setting for TI's OMAP4 family of processor.
+ * Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com/
+ * Authors: Yong Zhi
+ * Mythri pk <mythripk@ti.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define DSS_SUBSYS_NAME "HDMI"
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/string.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
+#include <video/omapdss.h>
+
+#include "hdmi5_core.h"
+#include "dss.h"
+#include "dss_features.h"
+
+static struct {
+ struct mutex lock;
+ struct platform_device *pdev;
+
+ struct hdmi_wp_data wp;
+ struct hdmi_pll_data pll;
+ struct hdmi_phy_data phy;
+ struct hdmi_core_data core;
+
+ struct hdmi_config cfg;
+
+ struct clk *sys_clk;
+ struct regulator *vdda_hdmi_dac_reg;
+
+ bool core_enabled;
+
+ struct omap_dss_device output;
+} hdmi;
+
+static int hdmi_runtime_get(void)
+{
+ int r;
+
+ DSSDBG("hdmi_runtime_get\n");
+
+ r = pm_runtime_get_sync(&hdmi.pdev->dev);
+ WARN_ON(r < 0);
+ if (r < 0)
+ return r;
+
+ return 0;
+}
+
+static void hdmi_runtime_put(void)
+{
+ int r;
+
+ DSSDBG("hdmi_runtime_put\n");
+
+ r = pm_runtime_put_sync(&hdmi.pdev->dev);
+ WARN_ON(r < 0 && r != -ENOSYS);
+}
+
+static int hdmi_init_regulator(void)
+{
+ struct regulator *reg;
+
+ if (hdmi.vdda_hdmi_dac_reg != NULL)
+ return 0;
+
+ reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
+ if (IS_ERR(reg)) {
+ DSSERR("can't get VDDA_HDMI_DAC regulator\n");
+ return PTR_ERR(reg);
+ }
+
+ hdmi.vdda_hdmi_dac_reg = reg;
+
+ return 0;
+}
+
+static int hdmi_power_on_core(struct omap_dss_device *dssdev)
+{
+ int r;
+
+ r = regulator_enable(hdmi.vdda_hdmi_dac_reg);
+ if (r)
+ return r;
+
+ r = hdmi_runtime_get();
+ if (r)
+ goto err_runtime_get;
+
+ /* Make selection of HDMI in DSS */
+ dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK);
+
+ hdmi.core_enabled = true;
+
+ return 0;
+
+err_runtime_get:
+ regulator_disable(hdmi.vdda_hdmi_dac_reg);
+
+ return r;
+}
+
+static void hdmi_power_off_core(struct omap_dss_device *dssdev)
+{
+ hdmi.core_enabled = false;
+
+ hdmi_runtime_put();
+ regulator_disable(hdmi.vdda_hdmi_dac_reg);
+}
+
+static int hdmi_power_on_full(struct omap_dss_device *dssdev)
+{
+ int r;
+ struct omap_video_timings *p;
+ struct omap_overlay_manager *mgr = hdmi.output.manager;
+ unsigned long phy;
+
+ r = hdmi_power_on_core(dssdev);
+ if (r)
+ return r;
+
+ dss_mgr_disable(mgr);
+
+ p = &hdmi.cfg.timings;
+
+ DSSDBG("hdmi_power_on x_res= %d y_res = %d\n", p->x_res, p->y_res);
+
+ phy = p->pixel_clock;
+
+ hdmi_pll_compute(&hdmi.pll, clk_get_rate(hdmi.sys_clk), phy);
+
+ hdmi_wp_video_stop(&hdmi.wp);
+
+ /* config the PLL and PHY hdmi_set_pll_pwrfirst */
+ r = hdmi_pll_enable(&hdmi.pll, &hdmi.wp);
+ if (r) {
+ DSSDBG("Failed to lock PLL\n");
+ goto err_pll_enable;
+ }
+
+ r = hdmi_phy_enable(&hdmi.phy, &hdmi.wp, &hdmi.cfg);
+ if (r) {
+ DSSDBG("Failed to start PHY\n");
+ goto err_phy_enable;
+ }
+
+ hdmi5_configure(&hdmi.core, &hdmi.wp, &hdmi.cfg);
+
+ /* bypass TV gamma table */
+ dispc_enable_gamma_table(0);
+
+ /* tv size */
+ dss_mgr_set_timings(mgr, p);
+
+ r = hdmi_wp_video_start(&hdmi.wp);
+ if (r)
+ goto err_vid_enable;
+
+ r = dss_mgr_enable(mgr);
+ if (r)
+ goto err_mgr_enable;
+
+ return 0;
+
+err_mgr_enable:
+ hdmi_wp_video_stop(&hdmi.wp);
+err_vid_enable:
+ hdmi_phy_disable(&hdmi.phy, &hdmi.wp);
+err_phy_enable:
+ hdmi_pll_disable(&hdmi.pll, &hdmi.wp);
+err_pll_enable:
+ hdmi_power_off_core(dssdev);
+ return -EIO;
+}
+
+static void hdmi_power_off_full(struct omap_dss_device *dssdev)
+{
+ struct omap_overlay_manager *mgr = hdmi.output.manager;
+
+ dss_mgr_disable(mgr);
+
+ hdmi_wp_video_stop(&hdmi.wp);
+ hdmi_phy_disable(&hdmi.phy, &hdmi.wp);
+ hdmi_pll_disable(&hdmi.pll, &hdmi.wp);
+
+ hdmi_power_off_core(dssdev);
+}
+
+static int hdmi_display_check_timing(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ struct hdmi_cm cm;
+
+ cm = hdmi_get_code(timings);
+ if (cm.code = -1)
+ return -EINVAL;
+
+ return 0;
+
+}
+
+static void hdmi_display_set_timing(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ struct hdmi_cm cm;
+ const struct hdmi_config *t;
+
+ mutex_lock(&hdmi.lock);
+
+ cm = hdmi_get_code(timings);
+ hdmi.cfg.cm = cm;
+
+ t = hdmi_get_timings(cm.mode, cm.code);
+ if (t != NULL) {
+ hdmi.cfg = *t;
+
+ dispc_set_tv_pclk(t->timings.pixel_clock * 1000);
+ }
+
+ mutex_unlock(&hdmi.lock);
+}
+
+static void hdmi_display_get_timings(struct omap_dss_device *dssdev,
+ struct omap_video_timings *timings)
+{
+ const struct hdmi_config *cfg;
+ struct hdmi_cm cm = hdmi.cfg.cm;
+
+ cfg = hdmi_get_timings(cm.mode, cm.code);
+ if (cfg = NULL)
+ cfg = hdmi_default_timing();
+
+ memcpy(timings, &cfg->timings, sizeof(cfg->timings));
+}
+
+static void hdmi_dump_regs(struct seq_file *s)
+{
+ mutex_lock(&hdmi.lock);
+
+ if (hdmi_runtime_get()) {
+ mutex_unlock(&hdmi.lock);
+ return;
+ }
+
+ hdmi_wp_dump(&hdmi.wp, s);
+ hdmi_pll_dump(&hdmi.pll, s);
+ hdmi_phy_dump(&hdmi.phy, s);
+ hdmi5_core_dump(&hdmi.core, s);
+
+ hdmi_runtime_put();
+ mutex_unlock(&hdmi.lock);
+}
+
+static int read_edid(u8 *buf, int len)
+{
+ int r;
+
+ mutex_lock(&hdmi.lock);
+
+ r = hdmi_runtime_get();
+ BUG_ON(r);
+
+ r = hdmi5_read_edid(&hdmi.core, buf, len);
+
+ hdmi_runtime_put();
+ mutex_unlock(&hdmi.lock);
+
+ return r;
+}
+
+static int hdmi_display_enable(struct omap_dss_device *dssdev)
+{
+ struct omap_dss_device *out = &hdmi.output;
+ int r = 0;
+
+ DSSDBG("ENTER hdmi_display_enable\n");
+
+ mutex_lock(&hdmi.lock);
+
+ if (out = NULL || out->manager = NULL) {
+ DSSERR("failed to enable display: no output/manager\n");
+ r = -ENODEV;
+ goto err0;
+ }
+
+ r = hdmi_power_on_full(dssdev);
+ if (r) {
+ DSSERR("failed to power on device\n");
+ goto err0;
+ }
+
+ mutex_unlock(&hdmi.lock);
+ return 0;
+
+err0:
+ mutex_unlock(&hdmi.lock);
+ return r;
+}
+
+static void hdmi_display_disable(struct omap_dss_device *dssdev)
+{
+ DSSDBG("Enter hdmi_display_disable\n");
+
+ mutex_lock(&hdmi.lock);
+
+ hdmi_power_off_full(dssdev);
+
+ mutex_unlock(&hdmi.lock);
+}
+
+static int hdmi_core_enable(struct omap_dss_device *dssdev)
+{
+ int r = 0;
+
+ DSSDBG("ENTER omapdss_hdmi_core_enable\n");
+
+ mutex_lock(&hdmi.lock);
+
+ r = hdmi_power_on_core(dssdev);
+ if (r) {
+ DSSERR("failed to power on device\n");
+ goto err0;
+ }
+
+ mutex_unlock(&hdmi.lock);
+ return 0;
+
+err0:
+ mutex_unlock(&hdmi.lock);
+ return r;
+}
+
+static void hdmi_core_disable(struct omap_dss_device *dssdev)
+{
+ DSSDBG("Enter omapdss_hdmi_core_disable\n");
+
+ mutex_lock(&hdmi.lock);
+
+ hdmi_power_off_core(dssdev);
+
+ mutex_unlock(&hdmi.lock);
+}
+
+static int hdmi_get_clocks(struct platform_device *pdev)
+{
+ struct clk *clk;
+
+ clk = devm_clk_get(&pdev->dev, "sys_clk");
+ if (IS_ERR(clk)) {
+ DSSERR("can't get sys_clk\n");
+ return PTR_ERR(clk);
+ }
+
+ hdmi.sys_clk = clk;
+
+ return 0;
+}
+
+static int hdmi_connect(struct omap_dss_device *dssdev,
+ struct omap_dss_device *dst)
+{
+ struct omap_overlay_manager *mgr;
+ int r;
+
+ r = hdmi_init_regulator();
+ if (r)
+ return r;
+
+ mgr = omap_dss_get_overlay_manager(dssdev->dispc_channel);
+ if (!mgr)
+ return -ENODEV;
+
+ r = dss_mgr_connect(mgr, dssdev);
+ if (r)
+ return r;
+
+ r = omapdss_output_set_device(dssdev, dst);
+ if (r) {
+ DSSERR("failed to connect output to new device: %s\n",
+ dst->name);
+ dss_mgr_disconnect(mgr, dssdev);
+ return r;
+ }
+
+ return 0;
+}
+
+static void hdmi_disconnect(struct omap_dss_device *dssdev,
+ struct omap_dss_device *dst)
+{
+ WARN_ON(dst != dssdev->dst);
+
+ if (dst != dssdev->dst)
+ return;
+
+ omapdss_output_unset_device(dssdev);
+
+ if (dssdev->manager)
+ dss_mgr_disconnect(dssdev->manager, dssdev);
+}
+
+static int hdmi_read_edid(struct omap_dss_device *dssdev,
+ u8 *edid, int len)
+{
+ bool need_enable;
+ int r;
+
+ need_enable = hdmi.core_enabled = false;
+
+ if (need_enable) {
+ r = hdmi_core_enable(dssdev);
+ if (r)
+ return r;
+ }
+
+ r = read_edid(edid, len);
+
+ if (need_enable)
+ hdmi_core_disable(dssdev);
+
+ return r;
+}
+
+#if defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
+static int hdmi_audio_enable(struct omap_dss_device *dssdev)
+{
+ int r;
+
+ mutex_lock(&hdmi.lock);
+
+ if (!hdmi_mode_has_audio(hdmi.cfg.cm.mode)) {
+ r = -EPERM;
+ goto err;
+ }
+
+ r = hdmi_wp_audio_enable(&hdmi.wp, true);
+ if (r)
+ goto err;
+
+ mutex_unlock(&hdmi.lock);
+ return 0;
+
+err:
+ mutex_unlock(&hdmi.lock);
+ return r;
+}
+
+static void hdmi_audio_disable(struct omap_dss_device *dssdev)
+{
+ hdmi_wp_audio_enable(&hdmi.wp, false);
+}
+
+static int hdmi_audio_start(struct omap_dss_device *dssdev)
+{
+ return hdmi_wp_audio_core_req_enable(&hdmi.wp, true);
+}
+
+static void hdmi_audio_stop(struct omap_dss_device *dssdev)
+{
+ hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
+}
+
+static bool hdmi_audio_supported(struct omap_dss_device *dssdev)
+{
+ bool r;
+
+ mutex_lock(&hdmi.lock);
+
+ r = hdmi_mode_has_audio(hdmi.cfg.cm.mode);
+
+ mutex_unlock(&hdmi.lock);
+ return r;
+}
+
+static int hdmi_audio_config(struct omap_dss_device *dssdev,
+ struct omap_dss_audio *audio)
+{
+ int r;
+ u32 pclk = hdmi.cfg.timings.pixel_clock;
+
+ mutex_lock(&hdmi.lock);
+
+ if (!hdmi_mode_has_audio(hdmi.cfg.cm.mode)) {
+ r = -EPERM;
+ goto err;
+ }
+
+ r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, audio, pclk);
+ if (r)
+ goto err;
+
+ mutex_unlock(&hdmi.lock);
+ return 0;
+
+err:
+ mutex_unlock(&hdmi.lock);
+ return r;
+}
+#else
+static int hdmi_audio_enable(struct omap_dss_device *dssdev)
+{
+ return -EPERM;
+}
+
+static void hdmi_audio_disable(struct omap_dss_device *dssdev)
+{
+}
+
+static int hdmi_audio_start(struct omap_dss_device *dssdev)
+{
+ return -EPERM;
+}
+
+static void hdmi_audio_stop(struct omap_dss_device *dssdev)
+{
+}
+
+static bool hdmi_audio_supported(struct omap_dss_device *dssdev)
+{
+ return false;
+}
+
+static int hdmi_audio_config(struct omap_dss_device *dssdev,
+ struct omap_dss_audio *audio)
+{
+ return -EPERM;
+}
+#endif
+
+static const struct omapdss_hdmi_ops hdmi_ops = {
+ .connect = hdmi_connect,
+ .disconnect = hdmi_disconnect,
+
+ .enable = hdmi_display_enable,
+ .disable = hdmi_display_disable,
+
+ .check_timings = hdmi_display_check_timing,
+ .set_timings = hdmi_display_set_timing,
+ .get_timings = hdmi_display_get_timings,
+
+ .read_edid = hdmi_read_edid,
+
+ .audio_enable = hdmi_audio_enable,
+ .audio_disable = hdmi_audio_disable,
+ .audio_start = hdmi_audio_start,
+ .audio_stop = hdmi_audio_stop,
+ .audio_supported = hdmi_audio_supported,
+ .audio_config = hdmi_audio_config,
+};
+
+static void hdmi_init_output(struct platform_device *pdev)
+{
+ struct omap_dss_device *out = &hdmi.output;
+
+ out->dev = &pdev->dev;
+ out->id = OMAP_DSS_OUTPUT_HDMI;
+ out->output_type = OMAP_DISPLAY_TYPE_HDMI;
+ out->name = "hdmi.0";
+ out->dispc_channel = OMAP_DSS_CHANNEL_DIGIT;
+ out->ops.hdmi = &hdmi_ops;
+ out->owner = THIS_MODULE;
+
+ omapdss_register_output(out);
+}
+
+static void __exit hdmi_uninit_output(struct platform_device *pdev)
+{
+ struct omap_dss_device *out = &hdmi.output;
+
+ omapdss_unregister_output(out);
+}
+
+/* HDMI HW IP initialisation */
+static int omapdss_hdmihw_probe(struct platform_device *pdev)
+{
+ int r;
+
+ hdmi.pdev = pdev;
+
+ mutex_init(&hdmi.lock);
+
+ r = hdmi_wp_init(pdev, &hdmi.wp);
+ if (r)
+ return r;
+
+ r = hdmi_pll_init(pdev, &hdmi.pll);
+ if (r)
+ return r;
+
+ r = hdmi_phy_init(pdev, &hdmi.phy);
+ if (r)
+ return r;
+
+ r = hdmi5_core_init(pdev, &hdmi.core);
+ if (r)
+ return r;
+
+ r = hdmi_get_clocks(pdev);
+ if (r) {
+ DSSERR("can't get clocks\n");
+ return r;
+ }
+
+ pm_runtime_enable(&pdev->dev);
+
+ hdmi_init_output(pdev);
+
+ dss_debugfs_create_file("hdmi", hdmi_dump_regs);
+
+ return 0;
+}
+
+static int __exit omapdss_hdmihw_remove(struct platform_device *pdev)
+{
+ hdmi_uninit_output(pdev);
+
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+static int hdmi_runtime_suspend(struct device *dev)
+{
+ clk_disable_unprepare(hdmi.sys_clk);
+
+ dispc_runtime_put();
+
+ return 0;
+}
+
+static int hdmi_runtime_resume(struct device *dev)
+{
+ int r;
+
+ r = dispc_runtime_get();
+ if (r < 0)
+ return r;
+
+ clk_prepare_enable(hdmi.sys_clk);
+
+ return 0;
+}
+
+static const struct dev_pm_ops hdmi_pm_ops = {
+ .runtime_suspend = hdmi_runtime_suspend,
+ .runtime_resume = hdmi_runtime_resume,
+};
+
+static const struct of_device_id hdmi_of_match[] = {
+ { .compatible = "ti,omap5-hdmi", },
+ {},
+};
+
+static struct platform_driver omapdss_hdmihw_driver = {
+ .probe = omapdss_hdmihw_probe,
+ .remove = __exit_p(omapdss_hdmihw_remove),
+ .driver = {
+ .name = "omapdss_hdmi5",
+ .owner = THIS_MODULE,
+ .pm = &hdmi_pm_ops,
+ .of_match_table = hdmi_of_match,
+ },
+};
+
+int __init hdmi5_init_platform_driver(void)
+{
+ return platform_driver_register(&omapdss_hdmihw_driver);
+}
+
+void __exit hdmi5_uninit_platform_driver(void)
+{
+ platform_driver_unregister(&omapdss_hdmihw_driver);
+}
diff --git a/drivers/video/omap2/dss/hdmi_wp.c b/drivers/video/omap2/dss/hdmi_wp.c
index 8151d89..a44f709 100644
--- a/drivers/video/omap2/dss/hdmi_wp.c
+++ b/drivers/video/omap2/dss/hdmi_wp.c
@@ -181,7 +181,7 @@ void hdmi_wp_init_vid_fmt_timings(struct hdmi_video_format *video_fmt,
timings->interlace = param->timings.interlace;
}
-#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO) || defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
void hdmi_wp_audio_config_format(struct hdmi_wp_data *wp,
struct hdmi_audio_format *aud_fmt)
{
--
1.8.1.2
^ permalink raw reply related
* [PATCH 4/6] omapdss: hdmi pll: Incorporate changes for OMAP5/DRA7x
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
Add a features struct to differentiate between the HDMI PLLs on OMAP4 and
OMAP5/DRA7x.
The OMAP5/DRA7x PLL are more sensitive compared to the PLL on OMAP4 when it
comes to locking. We need to ensure that the DCO freq calculated isn't too low
for lower pixel clocks. If not followed, the PLL doesn't lock for lower
frequencies.
Modify the PLL computation slightly to ensure the HDMI PLL locks for lower
frequencies. This will be replaced by a more complex computation which makes
sure all the PLL constraints are met.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi_pll.c | 74 ++++++++++++++++++++++++++++++++++++--
1 file changed, 71 insertions(+), 3 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi_pll.c b/drivers/video/omap2/dss/hdmi_pll.c
index d3e6e78..e1c55c4 100644
--- a/drivers/video/omap2/dss/hdmi_pll.c
+++ b/drivers/video/omap2/dss/hdmi_pll.c
@@ -21,6 +21,13 @@
#define HDMI_DEFAULT_REGN 16
#define HDMI_DEFAULT_REGM2 1
+struct hdmi_pll_features {
+ bool sys_reset;
+ bool bound_dcofreq;
+};
+
+static const struct hdmi_pll_features *pll_feat;
+
void hdmi_pll_dump(struct hdmi_pll_data *pll, struct seq_file *s)
{
#define DUMPPLL(r) seq_printf(s, "%-35s %08x\n", #r,\
@@ -55,7 +62,17 @@ void hdmi_pll_compute(struct hdmi_pll_data *pll, unsigned long clkin, int phy)
refclk = clkin / pi->regn;
- pi->regm2 = HDMI_DEFAULT_REGM2;
+ /*
+ * HACK: the HDMI PLL on omap5/dra7x doesn't lock if the required
+ * DCOFREQ is lesser than the minimum value of the DCO's lower frequency
+ * range. For now, we just make sure that low pixel clock rates also
+ * generate a high enough DCOFREQ by setting the M2 post divider to a
+ * higher value
+ */
+ if (pll_feat->bound_dcofreq && phy <= 65000)
+ pi->regm2 = 3;
+ else
+ pi->regm2 = HDMI_DEFAULT_REGM2;
/*
* multiplier is pixel_clk/ref_clk
@@ -151,8 +168,12 @@ static int hdmi_pll_config(struct hdmi_pll_data *pll)
static int hdmi_pll_reset(struct hdmi_pll_data *pll)
{
- /* SYSRESET controlled by power FSM */
- REG_FLD_MOD(pll->base, PLLCTRL_PLL_CONTROL, 0x0, 3, 3);
+ /*
+ * SYSRESET controlled by power FSM.
+ * SYSRESETN needs to be set for omap5/dra7x, but cleared for omap4
+ * HDMI PLL
+ */
+ REG_FLD_MOD(pll->base, PLLCTRL_PLL_CONTROL, pll_feat->sys_reset, 3, 3);
/* READ 0x0 reset is in progress */
if (hdmi_wait_for_bit_change(pll->base, PLLCTRL_PLL_STATUS, 0, 0, 1)
@@ -195,11 +216,58 @@ void hdmi_pll_disable(struct hdmi_pll_data *pll, struct hdmi_wp_data *wp)
#define PLL_OFFSET 0x200
#define PLL_SIZE 0x100
+static const struct hdmi_pll_features omap44xx_pll_feats = {
+ .sys_reset = false,
+ .bound_dcofreq = false,
+};
+
+static const struct hdmi_pll_features omap54xx_pll_feats = {
+ .sys_reset = true,
+ .bound_dcofreq = true,
+};
+
+static int __init hdmi_pll_init_features(struct platform_device *pdev)
+{
+ struct hdmi_pll_features *dst;
+ const struct hdmi_pll_features *src;
+
+ dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
+ if (!dst) {
+ dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
+ return -ENOMEM;
+ }
+
+ switch (omapdss_get_version()) {
+ case OMAPDSS_VER_OMAP4430_ES1:
+ case OMAPDSS_VER_OMAP4430_ES2:
+ case OMAPDSS_VER_OMAP4:
+ src = &omap44xx_pll_feats;
+ break;
+
+ case OMAPDSS_VER_OMAP5:
+ src = &omap54xx_pll_feats;
+ break;
+
+ default:
+ return -ENODEV;
+ }
+
+ memcpy(dst, src, sizeof(*dst));
+ pll_feat = dst;
+
+ return 0;
+}
+
int hdmi_pll_init(struct platform_device *pdev, struct hdmi_pll_data *pll)
{
+ int r;
struct resource *res;
struct resource temp_res;
+ r = hdmi_pll_init_features(pdev);
+ if (r)
+ return r;
+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hdmi_pllctrl");
if (!res) {
DSSDBG("can't get PLL mem resource by name\n");
--
1.8.1.2
^ permalink raw reply related
* [PATCH 3/6] omapdss: hdmi phy: Incorporate changes for OMAP5/DRA7x
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
OMAP5/DRA7x HDMI PHY has some differences compared to OMAP4 HDMI PHY.
Create a features struct which help the diagram configure the PHY based
on what SoC it is.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi.h | 1 +
drivers/video/omap2/dss/hdmi_phy.c | 94 +++++++++++++++++++++++++++++++++++++-
2 files changed, 93 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.h b/drivers/video/omap2/dss/hdmi.h
index b410b14..8fa2121 100644
--- a/drivers/video/omap2/dss/hdmi.h
+++ b/drivers/video/omap2/dss/hdmi.h
@@ -80,6 +80,7 @@
#define HDMI_TXPHY_DIGITAL_CTRL 0x4
#define HDMI_TXPHY_POWER_CTRL 0x8
#define HDMI_TXPHY_PAD_CFG_CTRL 0xC
+#define HDMI_TXPHY_BIST_CONTROL 0x1C
enum hdmi_pll_pwr {
HDMI_PLLPWRCMD_ALLOFF = 0,
diff --git a/drivers/video/omap2/dss/hdmi_phy.c b/drivers/video/omap2/dss/hdmi_phy.c
index 45acb99..4d9debb 100644
--- a/drivers/video/omap2/dss/hdmi_phy.c
+++ b/drivers/video/omap2/dss/hdmi_phy.c
@@ -12,11 +12,22 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
#include <video/omapdss.h>
#include "dss.h"
#include "hdmi.h"
+struct hdmi_phy_features {
+ bool bist_ctrl;
+ bool calc_freqout;
+ bool ldo_voltage;
+ unsigned long dcofreq_min; /* in KHz */
+ unsigned long max_phy; /* in KHz */
+};
+
+static const struct hdmi_phy_features *phy_feat;
+
void hdmi_phy_dump(struct hdmi_phy_data *phy, struct seq_file *s)
{
#define DUMPPHY(r) seq_printf(s, "%-35s %08x\n", #r,\
@@ -26,6 +37,8 @@ void hdmi_phy_dump(struct hdmi_phy_data *phy, struct seq_file *s)
DUMPPHY(HDMI_TXPHY_DIGITAL_CTRL);
DUMPPHY(HDMI_TXPHY_POWER_CTRL);
DUMPPHY(HDMI_TXPHY_PAD_CFG_CTRL);
+ if (phy_feat->bist_ctrl)
+ DUMPPHY(HDMI_TXPHY_BIST_CONTROL);
}
static irqreturn_t hdmi_irq_handler(int irq, void *data)
@@ -64,12 +77,20 @@ int hdmi_phy_enable(struct hdmi_phy_data *phy, struct hdmi_wp_data *wp,
{
u16 r = 0;
u32 irqstatus;
+ u8 freqout;
hdmi_wp_clear_irqenable(wp, 0xffffffff);
irqstatus = hdmi_wp_get_irqstatus(wp);
hdmi_wp_set_irqstatus(wp, irqstatus);
+ /*
+ * In OMAP5+, the HFBITCLK must be divided by 2 before issuing the
+ * HDMI_PHYPWRCMD_LDOON command.
+ */
+ if (phy_feat->bist_ctrl)
+ REG_FLD_MOD(phy->base, HDMI_TXPHY_BIST_CONTROL, 1, 11, 11);
+
r = hdmi_wp_set_phy_pwr(wp, HDMI_PHYPWRCMD_LDOON);
if (r)
return r;
@@ -80,17 +101,33 @@ int hdmi_phy_enable(struct hdmi_phy_data *phy, struct hdmi_wp_data *wp,
*/
hdmi_read_reg(phy->base, HDMI_TXPHY_TX_CTRL);
+ if (phy_feat->calc_freqout) {
+ /* DCOCLK/10 is pixel clock, compare pclk with DCOCLK_MIN/10 */
+ u32 dco_min = phy_feat->dcofreq_min / 10;
+ u32 pclk = cfg->timings.pixel_clock;
+
+ if (pclk < dco_min)
+ freqout = 0;
+ else if ((pclk >= dco_min) && (pclk < phy_feat->max_phy))
+ freqout = 1;
+ else
+ freqout = 2;
+ } else {
+ freqout = 1;
+ }
+
/*
* Write to phy address 0 to configure the clock
* use HFBITCLK write HDMI_TXPHY_TX_CONTROL_FREQOUT field
*/
- REG_FLD_MOD(phy->base, HDMI_TXPHY_TX_CTRL, 0x1, 31, 30);
+ REG_FLD_MOD(phy->base, HDMI_TXPHY_TX_CTRL, freqout, 31, 30);
/* Write to phy address 1 to start HDMI line (TXVALID and TMDSCLKEN) */
hdmi_write_reg(phy->base, HDMI_TXPHY_DIGITAL_CTRL, 0xF0000000);
/* Setup max LDO voltage */
- REG_FLD_MOD(phy->base, HDMI_TXPHY_POWER_CTRL, 0xB, 3, 0);
+ if (phy_feat->ldo_voltage)
+ REG_FLD_MOD(phy->base, HDMI_TXPHY_POWER_CTRL, 0xB, 3, 0);
/* Write to phy address 3 to change the polarity control */
REG_FLD_MOD(phy->base, HDMI_TXPHY_PAD_CFG_CTRL, 0x1, 27, 27);
@@ -119,11 +156,64 @@ void hdmi_phy_disable(struct hdmi_phy_data *phy, struct hdmi_wp_data *wp)
#define PHY_OFFSET 0x300
#define PHY_SIZE 0x100
+static const struct hdmi_phy_features omap44xx_phy_feats = {
+ .bist_ctrl = false,
+ .calc_freqout = false,
+ .ldo_voltage = true,
+ .dcofreq_min = 500000,
+ .max_phy = 185675,
+};
+
+static const struct hdmi_phy_features omap54xx_phy_feats = {
+ .bist_ctrl = true,
+ .calc_freqout = true,
+ .ldo_voltage = false,
+ .dcofreq_min = 750000,
+ .max_phy = 186000,
+};
+
+static int hdmi_phy_init_features(struct platform_device *pdev)
+{
+ struct hdmi_phy_features *dst;
+ const struct hdmi_phy_features *src;
+
+ dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL);
+ if (!dst) {
+ dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n");
+ return -ENOMEM;
+ }
+
+ switch (omapdss_get_version()) {
+ case OMAPDSS_VER_OMAP4430_ES1:
+ case OMAPDSS_VER_OMAP4430_ES2:
+ case OMAPDSS_VER_OMAP4:
+ src = &omap44xx_phy_feats;
+ break;
+
+ case OMAPDSS_VER_OMAP5:
+ src = &omap54xx_phy_feats;
+ break;
+
+ default:
+ return -ENODEV;
+ }
+
+ memcpy(dst, src, sizeof(*dst));
+ phy_feat = dst;
+
+ return 0;
+}
+
int hdmi_phy_init(struct platform_device *pdev, struct hdmi_phy_data *phy)
{
+ int r;
struct resource *res;
struct resource temp_res;
+ r = hdmi_phy_init_features(pdev);
+ if (r)
+ return r;
+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hdmi_txphy");
if (!res) {
DSSDBG("can't get PHY mem resource by name\n");
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/6] omapdss: hdmi: Add initial OMAP5/DRA7x HDMI core IP library
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
Add the OMAP5 HDMI core IP library. This will used by the hdmi5 driver to
configure the HDMI core sub block found on OMAP5/DRA7x.
The new HDMI core IP consists of the video composing sub blocks: video sampler,
color space converter, video packetizer and frame composer. These are connected
to each other serially. There is an i2c/ddc master sub block which is
responsible for reading EDID info.
Each of the video sub block is programmed in hdmi5_configure. Currenlty, only the
minimal features for each sub block are exposed. We can configure the core only
in 24 bit color mode. As we add more features, the register programming to
configure the serial sub blocks will increase in complexity.
The register configurations for enabling and clearing interrupts are provided,
but interrupts themselves aren't used at the moment. One place where they will
come in use to read the hdmi ddc block's register which recieves a byte of data
from the slave. We currently just read the register after every millisecond.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi5_core.c | 812 +++++++++++++++++++++++++++++++++++
drivers/video/omap2/dss/hdmi5_core.h | 298 +++++++++++++
2 files changed, 1110 insertions(+)
create mode 100644 drivers/video/omap2/dss/hdmi5_core.c
create mode 100644 drivers/video/omap2/dss/hdmi5_core.h
diff --git a/drivers/video/omap2/dss/hdmi5_core.c b/drivers/video/omap2/dss/hdmi5_core.c
new file mode 100644
index 0000000..c9c0ccc
--- /dev/null
+++ b/drivers/video/omap2/dss/hdmi5_core.c
@@ -0,0 +1,812 @@
+/*
+ * OMAP5 HDMI CORE IP driver Library
+ * Copyright (C) 2013 Texas Instruments Incorporated
+ * Author: Mythri pk <mythripk@ti.com>
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/string.h>
+#include <linux/seq_file.h>
+#include <drm/drm_edid.h>
+#if defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
+#include <sound/asound.h>
+#include <sound/asoundef.h>
+#endif
+
+#include "hdmi5_core.h"
+
+/* only 24 bit color depth used for now */
+static const struct csc_table csc_table_deepcolor[] = {
+ /* HDMI_DEEP_COLOR_24BIT */
+ [0] = { 7036, 0, 0, 32, 0, 7036, 0, 32, 0, 0, 7036, 32, },
+ /* HDMI_DEEP_COLOR_30BIT */
+ [1] = { 7015, 0, 0, 128, 0, 7015, 0, 128, 0, 0, 7015, 128, },
+ /* HDMI_DEEP_COLOR_36BIT */
+ [2] = { 7010, 0, 0, 512, 0, 7010, 0, 512, 0, 0, 7010, 512, },
+ /* FULL RANGE */
+ [3] = { 8192, 0, 0, 0, 0, 8192, 0, 0, 0, 0, 8192, 0, },
+};
+
+static void hdmi_core_ddc_req_addr(struct hdmi_core_data *core, u8 addr, u8 ext)
+{
+ void __iomem *base = core->base;
+ u8 seg_ptr = ext / 2;
+ u8 edidaddr = ((ext % 2) * 0x80) + addr;
+
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_ADDRESS, edidaddr, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SEGPTR, seg_ptr, 7, 0);
+
+ if (seg_ptr)
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_OPERATION, 1, 1, 1);
+ else
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_OPERATION, 1, 0, 0);
+}
+
+static void hdmi_core_ddc_init(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SDA_HOLD_ADDR, 0x18, 7, 0);
+
+ /* mask the i2c interrupts */
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_CTLINT, 0x0, 2, 2);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_CTLINT, 0x0, 6, 6);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_INT, 0x0, 2, 2);
+
+ /* Master clock division */
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_DIV, 0x5, 3, 0);
+
+ /* Standard speed counter */
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SS_SCL_HCNT_1_ADDR, 0x00, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SS_SCL_HCNT_0_ADDR, 0x6c, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SS_SCL_LCNT_1_ADDR, 0x00, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SS_SCL_LCNT_0_ADDR, 0x7f, 7, 0);
+
+ /* Fast speed counter */
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_FS_SCL_HCNT_1_ADDR, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_FS_SCL_HCNT_0_ADDR, 0x0f, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_FS_SCL_LCNT_1_ADDR, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_FS_SCL_LCNT_0_ADDR, 0x21, 7, 0);
+
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SLAVE, 0x50, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_SEGADDR, 0x30, 6, 0);
+}
+
+static int hdmi_core_ddc_edid(struct hdmi_core_data *core, u8 *pedid, u8 ext)
+{
+ void __iomem *base = core->base;
+ u8 cur_addr = 0;
+
+ hdmi_core_ddc_req_addr(core, cur_addr, ext);
+
+ /* unmask the i2c interrupts */
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_CTLINT, 0x1, 2, 2);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_CTLINT, 0x1, 6, 6);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_INT, 0x1, 2, 2);
+
+ /*
+ * FIXME: We read 128 bytes of data here with a msleep. Ideally, we
+ * should read based on a completion interrupt, the completion
+ * interrupt doesn't seem to work, so we just sleep for around a
+ * millisecond instead.
+ */
+ while (cur_addr < 128) {
+ usleep_range(1000, 1500);
+ pedid[cur_addr++] = REG_GET(base, HDMI_CORE_I2CM_DATAI, 7, 0);
+ hdmi_core_ddc_req_addr(core, cur_addr, ext);
+ }
+
+ return 0;
+}
+
+int hdmi5_read_edid(struct hdmi_core_data *core, u8 *edid, int len)
+{
+ int r, n, i;
+ int max_ext_blocks = (len / 128) - 1;
+
+ if (len < 128)
+ return -EINVAL;
+
+ hdmi_core_ddc_init(core);
+
+ r = hdmi_core_ddc_edid(core, edid, 0);
+ if (r) {
+ return r;
+ } else {
+ n = edid[0x7e];
+
+ if (n > max_ext_blocks)
+ n = max_ext_blocks;
+
+ for (i = 1; i <= n; i++) {
+ r = hdmi_core_ddc_edid(core, edid + i * EDID_LENGTH, i);
+ if (r)
+ return r;
+ }
+ }
+
+ return 0;
+}
+
+void hdmi5_core_dump(struct hdmi_core_data *core, struct seq_file *s)
+{
+
+#define DUMPCORE(r) seq_printf(s, "%-35s %08x\n", #r,\
+ hdmi_read_reg(core->base, r))
+
+ DUMPCORE(HDMI_CORE_FC_INVIDCONF);
+ DUMPCORE(HDMI_CORE_FC_INHACTIV0);
+ DUMPCORE(HDMI_CORE_FC_INHACTIV1);
+ DUMPCORE(HDMI_CORE_FC_INHBLANK0);
+ DUMPCORE(HDMI_CORE_FC_INHBLANK1);
+ DUMPCORE(HDMI_CORE_FC_INVACTIV0);
+ DUMPCORE(HDMI_CORE_FC_INVACTIV1);
+ DUMPCORE(HDMI_CORE_FC_INVBLANK);
+ DUMPCORE(HDMI_CORE_FC_HSYNCINDELAY0);
+ DUMPCORE(HDMI_CORE_FC_HSYNCINDELAY1);
+ DUMPCORE(HDMI_CORE_FC_HSYNCINWIDTH0);
+ DUMPCORE(HDMI_CORE_FC_HSYNCINWIDTH1);
+ DUMPCORE(HDMI_CORE_FC_VSYNCINDELAY);
+ DUMPCORE(HDMI_CORE_FC_VSYNCINWIDTH);
+ DUMPCORE(HDMI_CORE_FC_CTRLDUR);
+ DUMPCORE(HDMI_CORE_FC_EXCTRLDUR);
+ DUMPCORE(HDMI_CORE_FC_EXCTRLSPAC);
+ DUMPCORE(HDMI_CORE_FC_CH0PREAM);
+ DUMPCORE(HDMI_CORE_FC_CH1PREAM);
+ DUMPCORE(HDMI_CORE_FC_CH2PREAM);
+ DUMPCORE(HDMI_CORE_FC_AVICONF0);
+ DUMPCORE(HDMI_CORE_FC_AVICONF1);
+ DUMPCORE(HDMI_CORE_FC_AVICONF2);
+ DUMPCORE(HDMI_CORE_FC_AVIVID);
+ DUMPCORE(HDMI_CORE_FC_PRCONF);
+
+ DUMPCORE(HDMI_CORE_MC_CLKDIS);
+ DUMPCORE(HDMI_CORE_MC_SWRSTZREQ);
+ DUMPCORE(HDMI_CORE_MC_FLOWCTRL);
+ DUMPCORE(HDMI_CORE_MC_PHYRSTZ);
+ DUMPCORE(HDMI_CORE_MC_LOCKONCLOCK);
+
+ DUMPCORE(HDMI_CORE_I2CM_SLAVE);
+ DUMPCORE(HDMI_CORE_I2CM_ADDRESS);
+ DUMPCORE(HDMI_CORE_I2CM_DATAO);
+ DUMPCORE(HDMI_CORE_I2CM_DATAI);
+ DUMPCORE(HDMI_CORE_I2CM_OPERATION);
+ DUMPCORE(HDMI_CORE_I2CM_INT);
+ DUMPCORE(HDMI_CORE_I2CM_CTLINT);
+ DUMPCORE(HDMI_CORE_I2CM_DIV);
+ DUMPCORE(HDMI_CORE_I2CM_SEGADDR);
+ DUMPCORE(HDMI_CORE_I2CM_SOFTRSTZ);
+ DUMPCORE(HDMI_CORE_I2CM_SEGPTR);
+ DUMPCORE(HDMI_CORE_I2CM_SS_SCL_HCNT_1_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_SS_SCL_HCNT_0_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_SS_SCL_LCNT_1_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_SS_SCL_LCNT_0_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_FS_SCL_HCNT_1_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_FS_SCL_HCNT_0_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_FS_SCL_LCNT_1_ADDR);
+ DUMPCORE(HDMI_CORE_I2CM_FS_SCL_LCNT_0_ADDR);
+
+#undef DUMPCORE
+}
+
+static void hdmi_core_init(struct hdmi_core_vid_config *video_cfg,
+ struct hdmi_core_infoframe_avi *avi_cfg,
+ struct hdmi_config *cfg)
+{
+ DSSDBG("hdmi_core_init\n");
+
+ /* video core */
+ video_cfg->data_enable_pol = 1; /* It is always 1*/
+ video_cfg->v_fc_config.timings.hsync_level = cfg->timings.hsync_level;
+ video_cfg->v_fc_config.timings.x_res = cfg->timings.x_res;
+ video_cfg->v_fc_config.timings.hsw = cfg->timings.hsw - 1;
+ video_cfg->v_fc_config.timings.hbp = cfg->timings.hbp;
+ video_cfg->v_fc_config.timings.hfp = cfg->timings.hfp;
+ video_cfg->hblank = cfg->timings.hfp +
+ cfg->timings.hbp + cfg->timings.hsw - 1;
+ video_cfg->v_fc_config.timings.vsync_level = cfg->timings.vsync_level;
+ video_cfg->v_fc_config.timings.y_res = cfg->timings.y_res;
+ video_cfg->v_fc_config.timings.vsw = cfg->timings.vsw;
+ video_cfg->v_fc_config.timings.vfp = cfg->timings.vfp;
+ video_cfg->v_fc_config.timings.vbp = cfg->timings.vbp;
+ video_cfg->vblank_osc = 0; /* Always 0 - need to confirm */
+ video_cfg->vblank = cfg->timings.vsw +
+ cfg->timings.vfp + cfg->timings.vbp;
+ video_cfg->v_fc_config.cm.mode = cfg->cm.mode;
+ video_cfg->v_fc_config.timings.interlace = cfg->timings.interlace;
+
+ /* info frame */
+ avi_cfg->db1_format = 0;
+ avi_cfg->db1_active_info = 0;
+ avi_cfg->db1_bar_info_dv = 0;
+ avi_cfg->db1_scan_info = 0;
+ avi_cfg->db2_colorimetry = 0;
+ avi_cfg->db2_aspect_ratio = 0;
+ avi_cfg->db2_active_fmt_ar = 0;
+ avi_cfg->db3_itc = 0;
+ avi_cfg->db3_ec = 0;
+ avi_cfg->db3_q_range = 0;
+ avi_cfg->db3_nup_scaling = 0;
+ avi_cfg->db4_videocode = 0;
+ avi_cfg->db5_pixel_repeat = 0;
+ avi_cfg->db6_7_line_eoftop = 0;
+ avi_cfg->db8_9_line_sofbottom = 0;
+ avi_cfg->db10_11_pixel_eofleft = 0;
+ avi_cfg->db12_13_pixel_sofright = 0;
+}
+
+static void hdmi_core_config_fc(struct hdmi_core_data *core,
+ struct hdmi_core_vid_config *cfg)
+{
+ void __iomem *base = core->base;
+ unsigned char r = 0;
+ bool vsync_pol, hsync_pol;
+
+ vsync_pol + cfg->v_fc_config.timings.vsync_level = OMAPDSS_SIG_ACTIVE_HIGH;
+ hsync_pol + cfg->v_fc_config.timings.hsync_level = OMAPDSS_SIG_ACTIVE_HIGH;
+
+ /* Set hsync, vsync and data-enable polarity */
+ r = hdmi_read_reg(base, HDMI_CORE_FC_INVIDCONF);
+ r = FLD_MOD(r, vsync_pol, 6, 6);
+ r = FLD_MOD(r, hsync_pol, 5, 5);
+ r = FLD_MOD(r, cfg->data_enable_pol, 4, 4);
+ r = FLD_MOD(r, cfg->vblank_osc, 1, 1);
+ r = FLD_MOD(r, cfg->v_fc_config.timings.interlace, 0, 0);
+ hdmi_write_reg(base, HDMI_CORE_FC_INVIDCONF, r);
+
+ /* set x resolution */
+ REG_FLD_MOD(base, HDMI_CORE_FC_INHACTIV1,
+ cfg->v_fc_config.timings.x_res >> 8, 4, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_INHACTIV0,
+ cfg->v_fc_config.timings.x_res & 0xff, 7, 0);
+
+ /* set y resolution */
+ REG_FLD_MOD(base, HDMI_CORE_FC_INVACTIV1,
+ cfg->v_fc_config.timings.y_res >> 8, 4, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_INVACTIV0,
+ cfg->v_fc_config.timings.y_res & 0xff, 7, 0);
+
+ /* set horizontal blanking pixels */
+ REG_FLD_MOD(base, HDMI_CORE_FC_INHBLANK1, cfg->hblank >> 8, 4, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_INHBLANK0, cfg->hblank & 0xff, 7, 0);
+
+ /* set vertial blanking pixels */
+ REG_FLD_MOD(base, HDMI_CORE_FC_INVBLANK, cfg->vblank, 7, 0);
+
+ /* set horizontal sync offset */
+ REG_FLD_MOD(base, HDMI_CORE_FC_HSYNCINDELAY1,
+ cfg->v_fc_config.timings.hfp >> 8, 4, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_HSYNCINDELAY0,
+ cfg->v_fc_config.timings.hfp & 0xff, 7, 0);
+
+ /* set vertical sync offset */
+ REG_FLD_MOD(base, HDMI_CORE_FC_VSYNCINDELAY,
+ cfg->v_fc_config.timings.vfp, 7, 0);
+
+ /* set horizontal sync pulse width */
+ REG_FLD_MOD(base, HDMI_CORE_FC_HSYNCINWIDTH1,
+ (cfg->v_fc_config.timings.hsw >> 8), 1, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_HSYNCINWIDTH0,
+ cfg->v_fc_config.timings.hsw & 0xff, 7, 0);
+
+ /* set vertical sync pulse width */
+ REG_FLD_MOD(base, HDMI_CORE_FC_VSYNCINWIDTH,
+ cfg->v_fc_config.timings.vsw, 5, 0);
+
+ /* select DVI mode */
+ REG_FLD_MOD(base, HDMI_CORE_FC_INVIDCONF,
+ cfg->v_fc_config.cm.mode, 3, 3);
+}
+
+static void hdmi_core_config_video_packetizer(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+ int clr_depth = 0; /* 24 bit color depth */
+
+ /* COLOR_DEPTH */
+ REG_FLD_MOD(base, HDMI_CORE_VP_PR_CD, clr_depth, 7, 4);
+ /* BYPASS_EN */
+ REG_FLD_MOD(base, HDMI_CORE_VP_CONF, clr_depth ? 0 : 1, 6, 6);
+ /* PP_EN */
+ REG_FLD_MOD(base, HDMI_CORE_VP_CONF, clr_depth ? 1 : 0, 5, 5);
+ /* YCC422_EN */
+ REG_FLD_MOD(base, HDMI_CORE_VP_CONF, 0, 3, 3);
+ /* PP_STUFFING */
+ REG_FLD_MOD(base, HDMI_CORE_VP_STUFF, clr_depth ? 1 : 0, 1, 1);
+ /* YCC422_STUFFING */
+ REG_FLD_MOD(base, HDMI_CORE_VP_STUFF, 1, 2, 2);
+ /* OUTPUT_SELECTOR */
+ REG_FLD_MOD(base, HDMI_CORE_VP_CONF, clr_depth ? 0 : 2, 1, 0);
+}
+
+static void hdmi_core_config_csc(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+ int clr_depth;
+ struct csc_table csc_coeff = { 0 };
+
+ /* support limited range with 24 bit color depth for now */
+ csc_coeff = csc_table_deepcolor[0];
+ clr_depth = 0;
+ core->avi_cfg.db3_q_range = HDMI_INFOFRAME_AVI_DB3Q_LR;
+
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_MSB, csc_coeff.a1 >> 8 , 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A1_LSB, csc_coeff.a1, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_MSB, csc_coeff.a2 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A2_LSB, csc_coeff.a2, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_MSB, csc_coeff.a3 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A3_LSB, csc_coeff.a3, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_MSB, csc_coeff.a4 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_A4_LSB, csc_coeff.a4, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_MSB, csc_coeff.b1 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B1_LSB, csc_coeff.b1, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_MSB, csc_coeff.b2 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B2_LSB, csc_coeff.b2, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_MSB, csc_coeff.b3 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B3_LSB, csc_coeff.b3, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_MSB, csc_coeff.b4 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_B4_LSB, csc_coeff.b4, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_MSB, csc_coeff.c1 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C1_LSB, csc_coeff.c1, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_MSB, csc_coeff.c2 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C2_LSB, csc_coeff.c2, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_MSB, csc_coeff.c3 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C3_LSB, csc_coeff.c3, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_MSB, csc_coeff.c4 >> 8, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CSC_COEF_C4_LSB, csc_coeff.c4, 7, 0);
+
+ /* CSC_FEED_THROUGH_OFF */
+ REG_FLD_MOD(base, HDMI_CORE_MC_FLOWCTRL, 0x1, 0, 0);
+
+ /* CSC_COLORDEPTH */
+ REG_FLD_MOD(base, HDMI_CORE_CSC_SCALE, clr_depth, 7, 4);
+}
+
+static void hdmi_core_config_video_sampler(struct hdmi_core_data *core)
+{
+ int video_mapping = 1; /* for 24 bit color depth */
+
+ /* VIDEO_MAPPING */
+ REG_FLD_MOD(core->base, HDMI_CORE_TX_INVID0, video_mapping, 4, 0);
+}
+
+static void hdmi_core_config_fc_aux_infoframe_avi(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+ struct hdmi_core_infoframe_avi avi = core->avi_cfg;
+
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF0, avi.db1_format, 1, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF0, avi.db1_active_info, 6, 6);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF0, avi.db1_bar_info_dv, 3, 2);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF0, avi.db1_scan_info, 5, 4);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF1, avi.db2_colorimetry, 7, 6);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF1, avi.db2_aspect_ratio, 5, 4);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF1, avi.db2_active_fmt_ar, 3, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF2, avi.db3_itc, 7, 7);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF2, avi.db3_ec, 6, 4);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF2, avi.db3_q_range, 3, 2);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVICONF2, avi.db3_nup_scaling, 1, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AVIVID, avi.db4_videocode, 6, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_PRCONF, avi.db5_pixel_repeat, 3, 0);
+}
+
+static void hdmi_core_enable_video_path(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+
+ DSSDBG("hdmi_core_enable_video_path\n");
+
+ REG_FLD_MOD(base, HDMI_CORE_FC_CTRLDUR, 0x0C, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_EXCTRLDUR, 0x20, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_EXCTRLSPAC, 0x01, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_CH0PREAM, 0x0B, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_CH1PREAM, 0x16, 5, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_CH2PREAM, 0x21, 5, 0);
+ REG_FLD_MOD(base, HDMI_CORE_MC_CLKDIS, 0x00, 0, 0);
+ REG_FLD_MOD(base, HDMI_CORE_MC_CLKDIS, 0x00, 1, 1);
+}
+
+static void hdmi_core_mask_interrupts(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+
+ REG_FLD_MOD(base, HDMI_CORE_VP_MASK, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_MASK0, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_MASK1, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_MASK2, 0x0, 1, 0);
+ REG_FLD_MOD(base, HDMI_CORE_PHY_MASK0, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_PHY_I2CM_INT_ADDR, 0x8, 3, 0);
+ REG_FLD_MOD(base, HDMI_CORE_PHY_I2CM_CTLINT_ADDR, 0x88, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_INT, 0xa3, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CC08, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_D010, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_MASK, 0x3, 1, 0);
+ REG_FLD_MOD(base, HDMI_CORE_HDCP_MASK, 0x0, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_CEC_MASK, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_INT, 0x1, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_I2CM_CTLINT, 0xff, 7, 0);
+}
+
+static void hdmi_core_enable_interrupts(struct hdmi_core_data *core)
+{
+ /* Unmute interrupts */
+ REG_FLD_MOD(core->base, HDMI_CORE_IH_MUTE, 0x0, 1, 0);
+}
+
+int hdmi5_core_handle_irqs(struct hdmi_core_data *core)
+{
+ void __iomem *base = core->base;
+
+ REG_FLD_MOD(base, HDMI_CORE_IH_FC_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_FC_STAT1, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_FC_STAT2, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_AS_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_PHY_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_I2CM_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_CEC_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_VP_STAT0, 0xff, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_IH_I2CMPHY_STAT0, 0xff, 7, 0);
+
+ return 0;
+}
+
+void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
+ struct hdmi_config *cfg)
+{
+ struct omap_video_timings video_timing;
+ struct hdmi_video_format video_format;
+ struct hdmi_core_vid_config v_core_cfg;
+ struct hdmi_core_infoframe_avi *avi_cfg = &core->avi_cfg;
+
+ hdmi_core_mask_interrupts(core);
+
+ hdmi_core_init(&v_core_cfg, avi_cfg, cfg);
+
+ hdmi_wp_init_vid_fmt_timings(&video_format, &video_timing, cfg);
+
+ hdmi_wp_video_config_timing(wp, &video_timing);
+
+ /* video config */
+ video_format.packing_mode = HDMI_PACK_24b_RGB_YUV444_YUV422;
+
+ hdmi_wp_video_config_format(wp, &video_format);
+
+ hdmi_wp_video_config_interface(wp, &video_timing);
+
+ /*
+ * configure core video path, set software reset in the core
+ */
+ v_core_cfg.packet_mode = HDMI_PACKETMODE24BITPERPIXEL;
+
+ hdmi_core_config_fc(core, &v_core_cfg);
+ hdmi_core_config_video_packetizer(core);
+ hdmi_core_config_csc(core);
+ hdmi_core_config_video_sampler(core);
+
+ /*
+ * configure packet info frame video according to CEA861-D
+ */
+ avi_cfg->db1_format = HDMI_INFOFRAME_AVI_DB1Y_RGB;
+ avi_cfg->db1_active_info + HDMI_INFOFRAME_AVI_DB1A_ACTIVE_FORMAT_OFF;
+ avi_cfg->db1_bar_info_dv = HDMI_INFOFRAME_AVI_DB1B_NO;
+ avi_cfg->db1_scan_info = HDMI_INFOFRAME_AVI_DB1S_0;
+ avi_cfg->db2_colorimetry = HDMI_INFOFRAME_AVI_DB2C_NO;
+ avi_cfg->db2_aspect_ratio = HDMI_INFOFRAME_AVI_DB2M_NO;
+ avi_cfg->db2_active_fmt_ar = HDMI_INFOFRAME_AVI_DB2R_SAME;
+ avi_cfg->db3_itc = HDMI_INFOFRAME_AVI_DB3ITC_NO;
+ avi_cfg->db3_ec = HDMI_INFOFRAME_AVI_DB3EC_XVYUV601;
+ avi_cfg->db3_q_range = HDMI_INFOFRAME_AVI_DB3Q_DEFAULT;
+ avi_cfg->db3_nup_scaling = HDMI_INFOFRAME_AVI_DB3SC_NO;
+ avi_cfg->db4_videocode = cfg->cm.code;
+ avi_cfg->db5_pixel_repeat = HDMI_INFOFRAME_AVI_DB5PR_NO;
+ avi_cfg->db6_7_line_eoftop = 0;
+ avi_cfg->db8_9_line_sofbottom = 0;
+ avi_cfg->db10_11_pixel_eofleft = 0;
+ avi_cfg->db12_13_pixel_sofright = 0;
+
+ hdmi_core_config_fc_aux_infoframe_avi(core);
+
+ hdmi_core_enable_video_path(core);
+
+ hdmi_core_enable_interrupts(core);
+}
+
+
+#if defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
+
+static void hdmi5_core_audio_config(struct hdmi_core_data *core,
+ struct hdmi_core_audio_config *cfg)
+{
+ void __iomem *base = core->base;
+ u8 val;
+
+ /* Mute audio before configuring */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCONF, 0xf, 7, 4);
+
+ /* Set the N parameter */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_N1, cfg->n, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_N2, cfg->n >> 8, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_N3, cfg->n >> 16, 3, 0);
+
+ /*
+ * CTS manual mode. Automatic mode is not supported when using audio
+ * parallel interface.
+ */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CTS3, 1, 4, 4);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CTS1, cfg->cts, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CTS2, cfg->cts >> 8, 7, 0);
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CTS3, cfg->cts >> 16, 3, 0);
+
+ /* Layout of Audio Sample Packets: 2-channel or multichannels */
+ if (cfg->layout = HDMI_AUDIO_LAYOUT_2CH)
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCONF, 0, 0, 0);
+ else
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCONF, 1, 0, 0);
+
+ /* Configure IEC-609580 Validity bits */
+ /* Channel 0 is valid */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, 0, 0, 0);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, 0, 4, 4);
+
+ if (cfg->layout = HDMI_AUDIO_LAYOUT_2CH)
+ val = 1;
+ else
+ val = 0;
+
+ /* Channels 1, 2 setting */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 1, 1);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 5, 5);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 2, 2);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 6, 6);
+ /* Channel 3 setting */
+ if (cfg->layout = HDMI_AUDIO_LAYOUT_6CH)
+ val = 1;
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 3, 3);
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSV, val, 7, 7);
+
+ /* Configure IEC-60958 User bits */
+ /* TODO: should be set by user. */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSU, 0, 7, 0);
+
+ /* Configure IEC-60958 Channel Status word */
+ /* CGMSA */
+ val = cfg->iec60958_cfg->status[5] & IEC958_AES5_CON_CGMSA;
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(0), val, 5, 4);
+
+ /* Copyright */
+ val = (cfg->iec60958_cfg->status[0] &
+ IEC958_AES0_CON_NOT_COPYRIGHT) >> 2;
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(0), val, 0, 0);
+
+ /* Category */
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDSCHNLS(1),
+ cfg->iec60958_cfg->status[1]);
+
+ /* PCM audio mode */
+ val = (cfg->iec60958_cfg->status[0] & IEC958_AES0_CON_MODE) >> 6;
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(2), val, 6, 4);
+
+ /* Source number */
+ val = cfg->iec60958_cfg->status[2] & IEC958_AES2_CON_SOURCE;
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(2), val, 3, 4);
+
+ /* Channel number right 0 */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(3), 2, 3, 0);
+ /* Channel number right 1*/
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(3), 4, 7, 4);
+ /* Channel number right 2 */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(4), 6, 3, 0);
+ /* Channel number right 3*/
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(4), 8, 7, 4);
+ /* Channel number left 0 */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(5), 1, 3, 0);
+ /* Channel number left 1*/
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(5), 3, 7, 4);
+ /* Channel number left 2 */
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(6), 5, 3, 0);
+ /* Channel number left 3*/
+ REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(6), 7, 7, 4);
+
+ /* Clock accuracy and sample rate */
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDSCHNLS(7),
+ cfg->iec60958_cfg->status[3]);
+
+ /* Original sample rate and word length */
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDSCHNLS(8),
+ cfg->iec60958_cfg->status[4]);
+
+ /* Enable FIFO empty and full interrupts */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_INT, 3, 3, 2);
+
+ /* Configure GPA */
+ /* select HBR/SPDIF interfaces */
+ if (cfg->layout = HDMI_AUDIO_LAYOUT_2CH) {
+ /* select HBR/SPDIF interfaces */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CONF0, 0, 5, 5);
+ /* enable two channels in GPA */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_CONF1, 3, 7, 0);
+ } else if (cfg->layout = HDMI_AUDIO_LAYOUT_6CH) {
+ /* select HBR/SPDIF interfaces */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CONF0, 0, 5, 5);
+ /* enable six channels in GPA */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_CONF1, 0x3f, 7, 0);
+ } else {
+ /* select HBR/SPDIF interfaces */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_CONF0, 0, 5, 5);
+ /* enable eight channels in GPA */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_CONF1, 0xff, 7, 0);
+ }
+
+ /* disable HBR */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_CONF2, 0, 0, 0);
+ /* enable PCUV */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_CONF2, 1, 1, 1);
+ /* enable GPA FIFO full and empty mask */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_MASK, 3, 1, 0);
+ /* set polarity of GPA FIFO empty interrupts */
+ REG_FLD_MOD(base, HDMI_CORE_AUD_GP_POL, 1, 0, 0);
+
+ /* unmute audio */
+ REG_FLD_MOD(core_sys_base, HDMI_CORE_FC_AUDSCONF, 0, 7, 4);
+}
+
+static void hdmi5_core_audio_infoframe_cfg(struct hdmi_core_data *core,
+ struct snd_cea_861_aud_if *info_aud)
+{
+ void __iomem *base = core->base;
+
+ /* channel count and coding type fields in AUDICONF0 are swapped */
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF0,
+ (info_aud->db1_ct_cc & CEA861_AUDIO_INFOFRAME_DB1CC) << 4 |
+ (info_aud->db1_ct_cc & CEA861_AUDIO_INFOFRAME_DB1CT) >> 4);
+
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF1, info_aud->db2_sf_ss);
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF2, info_aud->db4_ca);
+ hdmi_write_reg(base, HDMI_CORE_FC_AUDICONF3, info_aud->db5_dminh_lsv);
+}
+
+int hdmi5_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
+ struct omap_dss_audio *audio, u32 pclk)
+{
+ struct hdmi_audio_format audio_format;
+ struct hdmi_audio_dma audio_dma;
+ struct hdmi_core_audio_config core_cfg;
+ int err, n, cts, channel_count;
+ unsigned int fs_nr;
+ bool word_length_16b = false;
+
+ if (!audio || !audio->iec || !audio->cea || !core)
+ return -EINVAL;
+
+ core_cfg.iec60958_cfg = audio->iec;
+
+ if (!(audio->iec->status[4] & IEC958_AES4_CON_MAX_WORDLEN_24) &&
+ (audio->iec->status[4] & IEC958_AES4_CON_WORDLEN_20_16))
+ word_length_16b = true;
+
+ /* only 16-bit word length supported atm */
+ if (!word_length_16b)
+ return -EINVAL;
+
+ switch (audio->iec->status[3] & IEC958_AES3_CON_FS) {
+ case IEC958_AES3_CON_FS_32000:
+ fs_nr = 32000;
+ break;
+ case IEC958_AES3_CON_FS_44100:
+ fs_nr = 44100;
+ break;
+ case IEC958_AES3_CON_FS_48000:
+ fs_nr = 48000;
+ break;
+ case IEC958_AES3_CON_FS_88200:
+ fs_nr = 88200;
+ break;
+ case IEC958_AES3_CON_FS_96000:
+ fs_nr = 96000;
+ break;
+ case IEC958_AES3_CON_FS_176400:
+ fs_nr = 176400;
+ break;
+ case IEC958_AES3_CON_FS_192000:
+ fs_nr = 192000;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ err = hdmi_compute_acr(pclk, fs_nr, &n, &cts);
+ core_cfg.n = n;
+ core_cfg.cts = cts;
+
+ /* Audio channels settings */
+ channel_count = (audio->cea->db1_ct_cc & CEA861_AUDIO_INFOFRAME_DB1CC)
+ + 1;
+
+ if (channel_count = 2)
+ core_cfg.layout = HDMI_AUDIO_LAYOUT_2CH;
+ else if (channel_count = 6)
+ core_cfg.layout = HDMI_AUDIO_LAYOUT_6CH;
+ else
+ core_cfg.layout = HDMI_AUDIO_LAYOUT_8CH;
+
+ /* DMA settings */
+ if (word_length_16b)
+ audio_dma.transfer_size = 0x10;
+ else
+ audio_dma.transfer_size = 0x20;
+ audio_dma.block_size = 0xc0;
+ audio_dma.mode = HDMI_AUDIO_TRANSF_DMA;
+ audio_dma.fifo_threshold = 0x20; /* in number of samples */
+
+ /* audio FIFO format settings for 16-bit samples*/
+ audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_TWOSAMPLES;
+ audio_format.sample_size = HDMI_AUDIO_SAMPLE_16BITS;
+ audio_format.justification = HDMI_AUDIO_JUSTIFY_LEFT;
+
+ /* only LPCM atm */
+ audio_format.type = HDMI_AUDIO_TYPE_LPCM;
+
+ /* disable start/stop signals of IEC 60958 blocks */
+ audio_format.en_sig_blk_strt_end = HDMI_AUDIO_BLOCK_SIG_STARTEND_ON;
+
+ /* configure DMA and audio FIFO format*/
+ hdmi_wp_audio_config_dma(wp, &audio_dma);
+ hdmi_wp_audio_config_format(wp, &audio_format);
+
+ /* configure the core */
+ hdmi5_core_audio_config(core, &core_cfg);
+
+ /* configure CEA 861 audio infoframe */
+ hdmi5_core_audio_infoframe_cfg(core, audio->cea);
+
+ return 0;
+}
+#endif
+
+#define CORE_OFFSET 0x20000
+#define CORE_SIZE 0x17900
+
+int hdmi5_core_init(struct platform_device *pdev, struct hdmi_core_data *core)
+{
+ struct resource *res;
+ struct resource temp_res;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hdmi_core");
+ if (!res) {
+ DSSDBG("can't get CORE mem resource by name\n");
+ /*
+ * if hwmod/DT doesn't have the memory resource information
+ * split into HDMI sub blocks by name, we try again by getting
+ * the platform's first resource. this code will be removed when
+ * the driver can get the mem resources by name
+ */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ DSSERR("can't get CORE mem resource\n");
+ return -EINVAL;
+ }
+
+ temp_res.start = res->start + CORE_OFFSET;
+ temp_res.end = temp_res.start + CORE_SIZE - 1;
+ res = &temp_res;
+ }
+
+ core->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+ if (!core->base) {
+ DSSERR("can't ioremap CORE\n");
+ return -ENOMEM;
+ }
+
+ return 0;
+}
diff --git a/drivers/video/omap2/dss/hdmi5_core.h b/drivers/video/omap2/dss/hdmi5_core.h
new file mode 100644
index 0000000..26def54
--- /dev/null
+++ b/drivers/video/omap2/dss/hdmi5_core.h
@@ -0,0 +1,298 @@
+/*
+ * HDMI driver definition for TI OMAP5 processors.
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated
+ *
+ * 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.
+ */
+
+#ifndef _HDMI5_CORE_H_
+#define _HDMI5_CORE_H_
+
+#include "hdmi.h"
+
+/* HDMI IP Core System */
+
+/* HDMI Identification */
+#define HDMI_CORE_DESIGN_ID 0x00000
+#define HDMI_CORE_REVISION_ID 0x00004
+#define HDMI_CORE_PRODUCT_ID0 0x00008
+#define HDMI_CORE_PRODUCT_ID1 0x0000C
+#define HDMI_CORE_CONFIG0_ID 0x00010
+#define HDMI_CORE_CONFIG1_ID 0x00014
+#define HDMI_CORE_CONFIG2_ID 0x00018
+#define HDMI_CORE_CONFIG3_ID 0x0001C
+
+/* HDMI Interrupt */
+#define HDMI_CORE_IH_FC_STAT0 0x00400
+#define HDMI_CORE_IH_FC_STAT1 0x00404
+#define HDMI_CORE_IH_FC_STAT2 0x00408
+#define HDMI_CORE_IH_AS_STAT0 0x0040C
+#define HDMI_CORE_IH_PHY_STAT0 0x00410
+#define HDMI_CORE_IH_I2CM_STAT0 0x00414
+#define HDMI_CORE_IH_CEC_STAT0 0x00418
+#define HDMI_CORE_IH_VP_STAT0 0x0041C
+#define HDMI_CORE_IH_I2CMPHY_STAT0 0x00420
+#define HDMI_CORE_IH_MUTE 0x007FC
+
+/* HDMI Video Sampler */
+#define HDMI_CORE_TX_INVID0 0x00800
+#define HDMI_CORE_TX_INSTUFFING 0x00804
+#define HDMI_CORE_TX_RGYDATA0 0x00808
+#define HDMI_CORE_TX_RGYDATA1 0x0080C
+#define HDMI_CORE_TX_RCRDATA0 0x00810
+#define HDMI_CORE_TX_RCRDATA1 0x00814
+#define HDMI_CORE_TX_BCBDATA0 0x00818
+#define HDMI_CORE_TX_BCBDATA1 0x0081C
+
+/* HDMI Video Packetizer */
+#define HDMI_CORE_VP_STATUS 0x02000
+#define HDMI_CORE_VP_PR_CD 0x02004
+#define HDMI_CORE_VP_STUFF 0x02008
+#define HDMI_CORE_VP_REMAP 0x0200C
+#define HDMI_CORE_VP_CONF 0x02010
+#define HDMI_CORE_VP_STAT 0x02014
+#define HDMI_CORE_VP_INT 0x02018
+#define HDMI_CORE_VP_MASK 0x0201C
+#define HDMI_CORE_VP_POL 0x02020
+
+/* Frame Composer */
+#define HDMI_CORE_FC_INVIDCONF 0x04000
+#define HDMI_CORE_FC_INHACTIV0 0x04004
+#define HDMI_CORE_FC_INHACTIV1 0x04008
+#define HDMI_CORE_FC_INHBLANK0 0x0400C
+#define HDMI_CORE_FC_INHBLANK1 0x04010
+#define HDMI_CORE_FC_INVACTIV0 0x04014
+#define HDMI_CORE_FC_INVACTIV1 0x04018
+#define HDMI_CORE_FC_INVBLANK 0x0401C
+#define HDMI_CORE_FC_HSYNCINDELAY0 0x04020
+#define HDMI_CORE_FC_HSYNCINDELAY1 0x04024
+#define HDMI_CORE_FC_HSYNCINWIDTH0 0x04028
+#define HDMI_CORE_FC_HSYNCINWIDTH1 0x0402C
+#define HDMI_CORE_FC_VSYNCINDELAY 0x04030
+#define HDMI_CORE_FC_VSYNCINWIDTH 0x04034
+#define HDMI_CORE_FC_INFREQ0 0x04038
+#define HDMI_CORE_FC_INFREQ1 0x0403C
+#define HDMI_CORE_FC_INFREQ2 0x04040
+#define HDMI_CORE_FC_CTRLDUR 0x04044
+#define HDMI_CORE_FC_EXCTRLDUR 0x04048
+#define HDMI_CORE_FC_EXCTRLSPAC 0x0404C
+#define HDMI_CORE_FC_CH0PREAM 0x04050
+#define HDMI_CORE_FC_CH1PREAM 0x04054
+#define HDMI_CORE_FC_CH2PREAM 0x04058
+#define HDMI_CORE_FC_AVICONF3 0x0405C
+#define HDMI_CORE_FC_GCP 0x04060
+#define HDMI_CORE_FC_AVICONF0 0x04064
+#define HDMI_CORE_FC_AVICONF1 0x04068
+#define HDMI_CORE_FC_AVICONF2 0x0406C
+#define HDMI_CORE_FC_AVIVID 0x04070
+#define HDMI_CORE_FC_AVIETB0 0x04074
+#define HDMI_CORE_FC_AVIETB1 0x04078
+#define HDMI_CORE_FC_AVISBB0 0x0407C
+#define HDMI_CORE_FC_AVISBB1 0x04080
+#define HDMI_CORE_FC_AVIELB0 0x04084
+#define HDMI_CORE_FC_AVIELB1 0x04088
+#define HDMI_CORE_FC_AVISRB0 0x0408C
+#define HDMI_CORE_FC_AVISRB1 0x04090
+#define HDMI_CORE_FC_AUDICONF0 0x04094
+#define HDMI_CORE_FC_AUDICONF1 0x04098
+#define HDMI_CORE_FC_AUDICONF2 0x0409C
+#define HDMI_CORE_FC_AUDICONF3 0x040A0
+#define HDMI_CORE_FC_VSDIEEEID0 0x040A4
+#define HDMI_CORE_FC_VSDSIZE 0x040A8
+#define HDMI_CORE_FC_VSDIEEEID1 0x040C0
+#define HDMI_CORE_FC_VSDIEEEID2 0x040C4
+#define HDMI_CORE_FC_VSDPAYLOAD(n) (n * 4 + 0x040C8)
+#define HDMI_CORE_FC_SPDVENDORNAME(n) (n * 4 + 0x04128)
+#define HDMI_CORE_FC_SPDPRODUCTNAME(n) (n * 4 + 0x04148)
+#define HDMI_CORE_FC_SPDDEVICEINF 0x04188
+#define HDMI_CORE_FC_AUDSCONF 0x0418C
+#define HDMI_CORE_FC_AUDSSTAT 0x04190
+#define HDMI_CORE_FC_AUDSV 0x04194
+#define HDMI_CORE_FC_AUDSU 0x04198
+#define HDMI_CORE_FC_AUDSCHNLS(n) (n * 4 + 0x0419C)
+#define HDMI_CORE_FC_CTRLQHIGH 0x041CC
+#define HDMI_CORE_FC_CTRLQLOW 0x041D0
+#define HDMI_CORE_FC_ACP0 0x041D4
+#define HDMI_CORE_FC_ACP(n) ((16-n) * 4 + 0x04208)
+#define HDMI_CORE_FC_ISCR1_0 0x04248
+#define HDMI_CORE_FC_ISCR1(n) ((16-n) * 4 + 0x0424C)
+#define HDMI_CORE_FC_ISCR2(n) ((15-n) * 4 + 0x0428C)
+#define HDMI_CORE_FC_DATAUTO0 0x042CC
+#define HDMI_CORE_FC_DATAUTO1 0x042D0
+#define HDMI_CORE_FC_DATAUTO2 0x042D4
+#define HDMI_CORE_FC_DATMAN 0x042D8
+#define HDMI_CORE_FC_DATAUTO3 0x042DC
+#define HDMI_CORE_FC_RDRB(n) (n * 4 + 0x042E0)
+#define HDMI_CORE_FC_STAT0 0x04340
+#define HDMI_CORE_FC_INT0 0x04344
+#define HDMI_CORE_FC_MASK0 0x04348
+#define HDMI_CORE_FC_POL0 0x0434C
+#define HDMI_CORE_FC_STAT1 0x04350
+#define HDMI_CORE_FC_INT1 0x04354
+#define HDMI_CORE_FC_MASK1 0x04358
+#define HDMI_CORE_FC_POL1 0x0435C
+#define HDMI_CORE_FC_STAT2 0x04360
+#define HDMI_CORE_FC_INT2 0x04364
+#define HDMI_CORE_FC_MASK2 0x04368
+#define HDMI_CORE_FC_POL2 0x0436C
+#define HDMI_CORE_FC_PRCONF 0x04380
+#define HDMI_CORE_FC_GMD_STAT 0x04400
+#define HDMI_CORE_FC_GMD_EN 0x04404
+#define HDMI_CORE_FC_GMD_UP 0x04408
+#define HDMI_CORE_FC_GMD_CONF 0x0440C
+#define HDMI_CORE_FC_GMD_HB 0x04410
+#define HDMI_CORE_FC_GMD_PB(n) (n * 4 + 0x04414)
+#define HDMI_CORE_FC_DBGFORCE 0x04800
+#define HDMI_CORE_FC_DBGAUD0CH0 0x04804
+#define HDMI_CORE_FC_DBGAUD1CH0 0x04808
+#define HDMI_CORE_FC_DBGAUD2CH0 0x0480C
+#define HDMI_CORE_FC_DBGAUD0CH1 0x04810
+#define HDMI_CORE_FC_DBGAUD1CH1 0x04814
+#define HDMI_CORE_FC_DBGAUD2CH1 0x04818
+#define HDMI_CORE_FC_DBGAUD0CH2 0x0481C
+#define HDMI_CORE_FC_DBGAUD1CH2 0x04820
+#define HDMI_CORE_FC_DBGAUD2CH2 0x04824
+#define HDMI_CORE_FC_DBGAUD0CH3 0x04828
+#define HDMI_CORE_FC_DBGAUD1CH3 0x0482C
+#define HDMI_CORE_FC_DBGAUD2CH3 0x04830
+#define HDMI_CORE_FC_DBGAUD0CH4 0x04834
+#define HDMI_CORE_FC_DBGAUD1CH4 0x04838
+#define HDMI_CORE_FC_DBGAUD2CH4 0x0483C
+#define HDMI_CORE_FC_DBGAUD0CH5 0x04840
+#define HDMI_CORE_FC_DBGAUD1CH5 0x04844
+#define HDMI_CORE_FC_DBGAUD2CH5 0x04848
+#define HDMI_CORE_FC_DBGAUD0CH6 0x0484C
+#define HDMI_CORE_FC_DBGAUD1CH6 0x04850
+#define HDMI_CORE_FC_DBGAUD2CH6 0x04854
+#define HDMI_CORE_FC_DBGAUD0CH7 0x04858
+#define HDMI_CORE_FC_DBGAUD1CH7 0x0485C
+#define HDMI_CORE_FC_DBGAUD2CH7 0x04860
+#define HDMI_CORE_FC_DBGTMDS0 0x04864
+#define HDMI_CORE_FC_DBGTMDS1 0x04868
+#define HDMI_CORE_FC_DBGTMDS2 0x0486C
+#define HDMI_CORE_PHY_MASK0 0x0C018
+#define HDMI_CORE_PHY_I2CM_INT_ADDR 0x0C09C
+#define HDMI_CORE_PHY_I2CM_CTLINT_ADDR 0x0C0A0
+
+/* HDMI Audio */
+#define HDMI_CORE_AUD_CONF0 0x0C400
+#define HDMI_CORE_AUD_CONF1 0x0C404
+#define HDMI_CORE_AUD_INT 0x0C408
+#define HDMI_CORE_AUD_N1 0x0C800
+#define HDMI_CORE_AUD_N2 0x0C804
+#define HDMI_CORE_AUD_N3 0x0C808
+#define HDMI_CORE_AUD_CTS1 0x0C80C
+#define HDMI_CORE_AUD_CTS2 0x0C810
+#define HDMI_CORE_AUD_CTS3 0x0C814
+#define HDMI_CORE_AUD_INCLKFS 0x0C818
+#define HDMI_CORE_AUD_CC08 0x0CC08
+#define HDMI_CORE_AUD_GP_CONF0 0x0D400
+#define HDMI_CORE_AUD_GP_CONF1 0x0D404
+#define HDMI_CORE_AUD_GP_CONF2 0x0D408
+#define HDMI_CORE_AUD_D010 0x0D010
+#define HDMI_CORE_AUD_GP_STAT 0x0D40C
+#define HDMI_CORE_AUD_GP_INT 0x0D410
+#define HDMI_CORE_AUD_GP_POL 0x0D414
+#define HDMI_CORE_AUD_GP_MASK 0x0D418
+
+/* HDMI Main Controller */
+#define HDMI_CORE_MC_CLKDIS 0x10004
+#define HDMI_CORE_MC_SWRSTZREQ 0x10008
+#define HDMI_CORE_MC_FLOWCTRL 0x10010
+#define HDMI_CORE_MC_PHYRSTZ 0x10014
+#define HDMI_CORE_MC_LOCKONCLOCK 0x10018
+
+/* HDMI COLOR SPACE CONVERTER */
+#define HDMI_CORE_CSC_CFG 0x10400
+#define HDMI_CORE_CSC_SCALE 0x10404
+#define HDMI_CORE_CSC_COEF_A1_MSB 0x10408
+#define HDMI_CORE_CSC_COEF_A1_LSB 0x1040C
+#define HDMI_CORE_CSC_COEF_A2_MSB 0x10410
+#define HDMI_CORE_CSC_COEF_A2_LSB 0x10414
+#define HDMI_CORE_CSC_COEF_A3_MSB 0x10418
+#define HDMI_CORE_CSC_COEF_A3_LSB 0x1041C
+#define HDMI_CORE_CSC_COEF_A4_MSB 0x10420
+#define HDMI_CORE_CSC_COEF_A4_LSB 0x10424
+#define HDMI_CORE_CSC_COEF_B1_MSB 0x10428
+#define HDMI_CORE_CSC_COEF_B1_LSB 0x1042C
+#define HDMI_CORE_CSC_COEF_B2_MSB 0x10430
+#define HDMI_CORE_CSC_COEF_B2_LSB 0x10434
+#define HDMI_CORE_CSC_COEF_B3_MSB 0x10438
+#define HDMI_CORE_CSC_COEF_B3_LSB 0x1043C
+#define HDMI_CORE_CSC_COEF_B4_MSB 0x10440
+#define HDMI_CORE_CSC_COEF_B4_LSB 0x10444
+#define HDMI_CORE_CSC_COEF_C1_MSB 0x10448
+#define HDMI_CORE_CSC_COEF_C1_LSB 0x1044C
+#define HDMI_CORE_CSC_COEF_C2_MSB 0x10450
+#define HDMI_CORE_CSC_COEF_C2_LSB 0x10454
+#define HDMI_CORE_CSC_COEF_C3_MSB 0x10458
+#define HDMI_CORE_CSC_COEF_C3_LSB 0x1045C
+#define HDMI_CORE_CSC_COEF_C4_MSB 0x10460
+#define HDMI_CORE_CSC_COEF_C4_LSB 0x10464
+
+/* HDMI HDCP */
+#define HDMI_CORE_HDCP_MASK 0x14020
+
+/* HDMI CEC */
+#define HDMI_CORE_CEC_MASK 0x17408
+
+/* HDMI I2C Master */
+#define HDMI_CORE_I2CM_SLAVE 0x157C8
+#define HDMI_CORE_I2CM_ADDRESS 0x157CC
+#define HDMI_CORE_I2CM_DATAO 0x157D0
+#define HDMI_CORE_I2CM_DATAI 0X157D4
+#define HDMI_CORE_I2CM_OPERATION 0x157D8
+#define HDMI_CORE_I2CM_INT 0x157DC
+#define HDMI_CORE_I2CM_CTLINT 0x157E0
+#define HDMI_CORE_I2CM_DIV 0x157E4
+#define HDMI_CORE_I2CM_SEGADDR 0x157E8
+#define HDMI_CORE_I2CM_SOFTRSTZ 0x157EC
+#define HDMI_CORE_I2CM_SEGPTR 0x157F0
+#define HDMI_CORE_I2CM_SS_SCL_HCNT_1_ADDR 0x157F4
+#define HDMI_CORE_I2CM_SS_SCL_HCNT_0_ADDR 0x157F8
+#define HDMI_CORE_I2CM_SS_SCL_LCNT_1_ADDR 0x157FC
+#define HDMI_CORE_I2CM_SS_SCL_LCNT_0_ADDR 0x15800
+#define HDMI_CORE_I2CM_FS_SCL_HCNT_1_ADDR 0x15804
+#define HDMI_CORE_I2CM_FS_SCL_HCNT_0_ADDR 0x15808
+#define HDMI_CORE_I2CM_FS_SCL_LCNT_1_ADDR 0x1580C
+#define HDMI_CORE_I2CM_FS_SCL_LCNT_0_ADDR 0x15810
+#define HDMI_CORE_I2CM_SDA_HOLD_ADDR 0x15814
+
+enum hdmi_core_packet_mode {
+ HDMI_PACKETMODERESERVEDVALUE = 0,
+ HDMI_PACKETMODE24BITPERPIXEL = 4,
+ HDMI_PACKETMODE30BITPERPIXEL = 5,
+ HDMI_PACKETMODE36BITPERPIXEL = 6,
+ HDMI_PACKETMODE48BITPERPIXEL = 7,
+};
+
+struct hdmi_core_vid_config {
+ struct hdmi_config v_fc_config;
+ enum hdmi_core_packet_mode packet_mode;
+ int data_enable_pol;
+ int vblank_osc;
+ int hblank;
+ int vblank;
+};
+
+struct csc_table {
+ u16 a1, a2, a3, a4;
+ u16 b1, b2, b3, b4;
+ u16 c1, c2, c3, c4;
+};
+
+int hdmi5_read_edid(struct hdmi_core_data *core, u8 *edid, int len);
+void hdmi5_core_dump(struct hdmi_core_data *core, struct seq_file *s);
+int hdmi5_core_handle_irqs(struct hdmi_core_data *core);
+void hdmi5_configure(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
+ struct hdmi_config *cfg);
+int hdmi5_core_init(struct platform_device *pdev, struct hdmi_core_data *core);
+
+#if defined(CONFIG_OMAP5_DSS_HDMI_AUDIO)
+int hdmi5_audio_config(struct hdmi_core_data *core, struct hdmi_wp_data *wp,
+ struct omap_dss_audio *audio, u32 pclk);
+#endif
+#endif
--
1.8.1.2
^ permalink raw reply related
* [PATCH 1/6] omapdss: hdmi: support larger register offsets for OMAP5 HDMI core
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1382009202-18984-1-git-send-email-archit@ti.com>
The HDMI core IP on OMAP5 has a wider register address range. The offsets for
these registers can't fit into the u16 type currently used for hdmi register
read and write functions. Use u32 for offsets instead.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/hdmi.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.h b/drivers/video/omap2/dss/hdmi.h
index b049376..b410b14 100644
--- a/drivers/video/omap2/dss/hdmi.h
+++ b/drivers/video/omap2/dss/hdmi.h
@@ -360,13 +360,13 @@ struct hdmi_core_data {
struct hdmi_core_infoframe_avi avi_cfg;
};
-static inline void hdmi_write_reg(void __iomem *base_addr, const u16 idx,
+static inline void hdmi_write_reg(void __iomem *base_addr, const u32 idx,
u32 val)
{
__raw_writel(val, base_addr + idx);
}
-static inline u32 hdmi_read_reg(void __iomem *base_addr, const u16 idx)
+static inline u32 hdmi_read_reg(void __iomem *base_addr, const u32 idx)
{
return __raw_readl(base_addr + idx);
}
--
1.8.1.2
^ permalink raw reply related
* [PATCH 0/6] omapdss: hdmi: Add support for hdmi on OMAP5 and DRA7x SoCs
From: Archit Taneja @ 2013-10-17 11:38 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
The HDMI IP on OMAP5/DRA7xx has very similar wrapper, PLL and PHY sub blocks
found in OMAP4 DSS. However, the HDMI core IP(which implements the HDMI
protocol) is completely new.
Create a library for configuring the new HDMI core IP, make minor modifications
in PLL and PHY. And finally, add a hdmi5 driver. This driver can eventually be
merged with the hdmi4 driver such that both of them use the same driver, we
would need to create core IP ops for that, we've left that for later.
Archit Taneja (6):
omapdss: hdmi: support larger register offsets for OMAP5 HDMI core
omapdss: hdmi: Add initial OMAP5/DRA7x HDMI core IP library
omapdss: hdmi phy: Incorporate changes for OMAP5/DRA7x
omapdss: hdmi pll: Incorporate changes for OMAP5/DRA7x
omapdss: hdmi: add OMAP5/DRA7x hdmi driver
omapdss: hdmi4/hdmi5: set regulator voltage to required level
drivers/video/omap2/dss/Kconfig | 10 +
drivers/video/omap2/dss/Makefile | 2 +
drivers/video/omap2/dss/core.c | 6 +
drivers/video/omap2/dss/dss.h | 3 +
drivers/video/omap2/dss/hdmi.h | 7 +-
drivers/video/omap2/dss/hdmi4.c | 5 +
drivers/video/omap2/dss/hdmi5.c | 704 ++++++++++++++++++++++++++++++
drivers/video/omap2/dss/hdmi5_core.c | 812 +++++++++++++++++++++++++++++++++++
drivers/video/omap2/dss/hdmi5_core.h | 298 +++++++++++++
drivers/video/omap2/dss/hdmi_phy.c | 94 +++-
drivers/video/omap2/dss/hdmi_pll.c | 74 +++-
drivers/video/omap2/dss/hdmi_wp.c | 2 +-
12 files changed, 2008 insertions(+), 9 deletions(-)
create mode 100644 drivers/video/omap2/dss/hdmi5.c
create mode 100644 drivers/video/omap2/dss/hdmi5_core.c
create mode 100644 drivers/video/omap2/dss/hdmi5_core.h
--
1.8.1.2
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 11:35 UTC (permalink / raw)
To: Laurent Pinchart, Thierry Reding
Cc: Laurent Pinchart, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <12566222.aRYc4MDoOm@avalon>
[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]
On 17/10/13 14:02, Laurent Pinchart wrote:
>> Okay, so if I understand correctly, translating those bindings to panel
>> nodes would look somewhat like this:
>>
>> dc: display-controller {
>> ports {
>> port@0 {
>> remote-endpoint = <&panel>;
>> };
>> };
>> };
>>
>> panel: panel {
>> ports {
>> port@0 {
>> remote-endpoint = <&dc>;
>> };
>> };
>> };
>>
>> The above leaves out any of the other, non-relevant properties. Does
>> that sound about right?
>
> Yes it does.
It does?
Shouldn't it be something like:
panel {
ports {
port@0 {
endpoint@0 {
remote = <&dc>;
};
};
};
};
And simplified:
panel {
port {
endpoint@0 {
remote = <&dc>;
};
};
};
You do need a node for the endpoint, a remote-endpoint property is not
enough.
> Please note that, when a device has as single port, the ports node can be
> omitted, and the port doesn't need to be numbered. You would then end up with
>
> dc: display-controller {
> port {
> remote-endpoint = <&panel>;
> };
> };
>
> panel: panel {
> port {
> remote-endpoint = <&dc>;
> };
> };
>
> I don't think there's a way to simplify it further.
I'm not sure if there's a specific need for the port or endpoint nodes
in cases like the above. Even if we have common properties describing
the endpoint, I guess they could just be in the parent node.
panel {
remote = <&dc>;
common-video-property = <asd>;
};
The above would imply one port and one endpoint. Would that work? If we
had a function like parse_endpoint(node), we could just point it to
either a real endpoint node, or to the device's node.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Tomi Valkeinen @ 2013-10-17 11:21 UTC (permalink / raw)
To: Thierry Reding
Cc: Dave Airlie, Laurent Pinchart, Laurent Pinchart, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131017104856.GA10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4519 bytes --]
On 17/10/13 13:48, Thierry Reding wrote:
> As far as the simple panel device tree binding is concerned, it covers
> all the cases I know of. I can't seriously be expected to do any better
> than that. These panels are dumb. They only come with a power supply
> that needs to be switched on and an additional GPIO to enable it once
> powered up. All three panels just work with that set of properties. So
> anything that doesn't can arguably be covered by some other, not-so-
> simple binding.
Agreed. I thought the DT bindings were missing information about the
video path, but I didn't realize you have a reference from the video
output to the panel.
However, I think there's usually a bit more information needed than what
you describe above. All the dump panels I know have requirements on the
sequences related to the video signal, powers and gpios, and delays
between those.
I guess the most common enable scenario is something like:
- enable video signal
- enable power
- wait n msecs
- set GPIO
For some panels, the enable GPIO might actually be a reset GPIO, not
enable. Some could want the video signal only be enabled after the
enable/reset is done.
Anyway, I think what you describe fits most of the panels, so no need to
worry about the possible ones I described. I just wanted to point out
that there may be very dump panels that are still different.
> I'm still missing the point here. For one, I don't see any reason that
> this driver would ever need to gain CDF support. At the same time, CDF
> could easily be supported by just adding a few required properties and
> it should be easy to support any non-CDF cases just as well to remain
> backwards-compatible.
Well, I guess the point here is that if you have a SoC display
controller driver and panel drivers, and you want to support complex
panels using CDF, you need to implement CDF support into your SoC
display controller driver in addition to adding CDF support to the panel
driver.
And, you still want to be able to use the simple panels. So either you
support both CDF panels and non-CDF panels with your SoC dispc, which
could be a bit messy, or you add CDF support to all panel drivers you use.
>> I guess one option there would be to implement a new driver for the same
>> panel devices, one that supports CDF, and leave the old one as is. Not
>> so nice option, though.
>
> As I said, anything that really needs a CDF binding to work likely isn't
> "simple" anymore, therefore a separate driver can easily be justified.
I think it's rarely the case that a panel specifically needs CDF. It's
more that CDF offers a common framework for display components, making
it easy to make them independent of the platform used. And if you make
your SoC support CDF, you are able to use all the CDF drivers already
implemented.
That said, I don't see anything technically preventing from having
support for both CDF and non-CDF panels. It's just more complex, and
they cannot be mixed freely. For example, if I have CDF support in my
SoC driver and a panel driver, I can easily add a CDF encoder driver in
between. But if the panel driver is a custom one, then I need a custom
encoder driver there also.
> Indeed. The fundamental point here seems to be that we don't represent
> things in DT which we don't need. In the same way that we don't add
Surely we should represent the things about the HW we don't need now, if
we know they might be needed in the future? (But that's not the case
here, see below)
> device nodes for enumerable devices, why would we need to explicitly add
> information about connections between devices that cannot be controlled
> anyway. If the board connects an RGB/LVDS output to a panel directly, it
> is only required that the output knows what it is connected to because
> there is no other way besides DT to obtain any information about the
> panel. Therefore a unidirectional connection is more than enough. Using
> that the output can access the panel and query it for information such
> as the mode timings as well as switch it on or off as required.
Right, in that case, you do have all the necessary information in the
DT. I don't think it really matters if the link in DT is from the SoC to
the panel, vice versa, or both ways. It still describes the same thing.
Even with unidirectional link you can always find the reverse link, you
just need to do more work going throught the DT data.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 11:15 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017110517.GC10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6391 bytes --]
Hi Thierry,
On Thursday 17 October 2013 13:05:18 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> > On 17/10/13 11:53, Thierry Reding wrote:
> > > I keep wondering why we absolutely must have compatibility between CDF
> > > and this simple panel binding. DT content is supposed to concern itself
> > > with the description of hardware only. What about the following isn't an
> > > accurate description of the panel hardware?
> > >
> > > panel: panel {
> > > compatible = "cptt,claa101wb01";
> > >
> > > power-supply = <&vdd_pnl_reg>;
> > > enable-gpios = <&gpio 90 0>;
> > >
> > > backlight = <&backlight>;
> > > };
> > >
> > > dsi-controller {
> > > panel = <&panel>;
> > > };
> >
> > That's quite similar to how my current out-of-mainline OMAP DSS DT
> > bindings work. The difference is that I have a reference from the panel
> > node to the video source (dsi-controller), instead of the other way
> > around. I just find it more natural. It works the same way as, say,
> > regulators, gpios, etc.
>
> I suppose that depends on the way you look at it. In the above proposal
> I consider the output (DSI controller) to use the panel as a resource
> that provides a certain set of services (query mode timings, provide a
> way to enable or disable the panel). Similarly the panel uses the
> backlight as a resource that provides a certain set of services (such as
> changing the brightness).
>
> The above also follows the natural order of control. The panel has no
> way to control the DSI output. However, it is the output that controls
> when a panel is required and turn it on as appropriate.
I'm no DSI expert, but I know enough about it to be sure that Tomi will
disagree. DSI panels can have complex power sequences that require the input
stream to be finely controlled (for instance it might require a clock without
video frames for some time, switch a GPIO or regulator, send a command to the
panel, and then only get video frames). For that reason all developers I've
talked to who have an in-depth knowledge of DSI and DSI panels told me that
the panel needs to control the video bus, and request the video source to
start/stop the video stream.
> > However, one thing unclear is where the panel node should be. You seem
> > to have a DSI panel here. If the panel is truly not controlled in any
> > way, maybe having the panel as a normal platform device is ok. But if
> > the DSI panel is controlled with DSI messages, should it be a child of
> > the dsi-controller node, the same way i2c peripherals are children of
> > i2c master?
>
> That's one possibility. In this particular case it's not even necessary
> to use any of DSIs control methods to talk to the panel. The panel only
> receives the data stream and "just works".
>
> Even if the panel were to be controlled via DSI, I think keeping it as
> a separate device node is equally valid. It's really boils down to what
> I already said above. The output uses the panel as a resource, similar
> to how other devices might use a GPIO controller to use a GPIO pin, or
> an interrupt controller to request an IRQ.
Except that the display controller can also provides resources to the panel.
For DSI panels that support DSI commands, the control bus is provided by the
display controller. Much like an I2C device must be a child of its I2C
controller node, the DSI panel device should then be a child of the display
controller node.
> > >>> +static const struct drm_display_mode auo_b101aw03_mode = {
> > >>> + .clock = 51450,
> > >>> + .hdisplay = 1024,
> > >>> + .hsync_start = 1024 + 156,
> > >>> + .hsync_end = 1024 + 156 + 8,
> > >>> + .htotal = 1024 + 156 + 8 + 156,
> > >>> + .vdisplay = 600,
> > >>> + .vsync_start = 600 + 16,
> > >>> + .vsync_end = 600 + 16 + 6,
> > >>> + .vtotal = 600 + 16 + 6 + 16,
> > >>> + .vrefresh = 60,
> > >>> +};
> > >>
> > >> Instead of hardcoding the modes in the driver, which would then require
> > >> to be updated for every new simple panel model (and we know there are
> > >> lots of them), why don't you specify the modes in the panel DT node ?
> > >> The simple panel driver would then become much more generic. It would
> > >> also allow board designers to tweak h/v sync timings depending on the
> > >> system requirements.
> > >
> > > Sigh... we keep second-guessing ourselves. Back at the time when power
> > > sequences were designed (and NAKed at the last minute), we all decided
> > > that the right thing to do would be to use specific compatible values
> > > for individual panels, because that would allow us to encode the power
> > > sequencing within the driver. And when we already have the panel model
> > > encoded in the compatible value, we might just as well encode the mode
> > > within the driver for that panel. Otherwise we'll have to keep adding
> > > the same mode timings for every board that uses the same panel.
> >
> > Oh, I didn't feel "we all decided that the right thing..." =).
> >
> > > I do agree though that it might be useful to tweak the mode in case the
> > > default one doesn't work. How about we provide a means to override the
> > > mode encoded in the driver using one specified in the DT? That's
> > > obviously a backwards-compatible change, so it could be added if or when
> > > it becomes necessary.
> >
> > This sounds good to me.
> >
> > Although maybe it'd be good to have the driver compatible with something
> > like "generic-panel", so that you could have:
> >
> > compatible = "foo,specific-panel", "generic-panel";
> >
> > and if there's no need for any power/gpio setup for your board, you may
> > skip adding "foo,specific-panel" support to the panel driver. Later,
> > somebody else may need to implement fine grained power/gpio support for
> > "foo,specific-panel", and it would just work.
> >
> > Maybe that would help us with the problem of adding support in the
> > driver for a hundred different simple panels each used only once on a
> > specific board.
>
> Sure, that all sounds like reasonable additions. All of the suggestions
> are even backwards-compatible and hence can be added when needed without
> breaking compatibility with existing users of the binding.
What's wrong with moving the three hardcoded modes we already have in the
driver to DT before pushing this to mainline ?
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 11:07 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Dave Airlie, Laurent Pinchart, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <20131017104856.GA10993-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6238 bytes --]
Hi Thierry,
On Thursday 17 October 2013 12:48:57 Thierry Reding wrote:
> On Thu, Oct 17, 2013 at 12:52:40PM +0300, Tomi Valkeinen wrote:
> > On 17/10/13 12:16, Thierry Reding wrote:
> > > On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote:
> > >> Just my opinion, but yes, DT is that inflexible. DT bindings are like a
> > >> syscall. Once they are there, you need to support them forever. That
> > >> support can, of course, include some kind of DT data converters or
> > >> such, that try to convert old bindings to new bindings at kernel boot.
> > >
> > > That's not entirely true. DT bindings are allowed to change, the only
> > > restriction being that they don't break backwards compatibility. So if
> >
> > True, but afaik the same goes for syscalls. But yes, it's probably
> > easier to add, say, new properties to DT bindings than adding new flags
> > to a syscall.
> >
> > > Am I the only one refusing to accept that we can't provide even the most
> > > basic functionality simply because we haven't been able to come up with
> > > the ultimately "perfect" DT binding yet? By definition it's not very
> > > likely that any binding will ever be perfect.
> >
> > With "perfect" I meant without any obvious features missing. I agree
> > that DT bindings will never be perfect, but I at least would like them
> > to be good enough to cover all the cases we know of.
>
> As far as the simple panel device tree binding is concerned, it covers
> all the cases I know of. I can't seriously be expected to do any better
> than that. These panels are dumb. They only come with a power supply
> that needs to be switched on and an additional GPIO to enable it once
> powered up. All three panels just work with that set of properties. So
> anything that doesn't can arguably be covered by some other, not-so-
> simple binding.
>
> > > I don't think we're doing ourselves any good by letting DT actively
> > > hinder us in merging any of these features. I would personally prefer
> > > having to support several bindings rather than not be able to provide
> > > the functionality at all.
> >
> > I'd say the driver writer is of course free to create basic bindings for
> > the device in question as long as the bindings are sane, and he agrees
> > to later create any needed converters. I don't see any problem with
> > having a driver supporting several versions of the bindings.
> >
> > But I don't think it's fair to push a driver with basic bindings,
> > knowing that the bindings won't be enough in the future, and leave all
> > the converter-mess to other people (not saying that's the case here).
> >
> > As a fbdev maintainer, I'm ok with adding DT bindings to device specific
> > fb drivers, as those can be just left as they are. Even when CDF is
> > merged, the specific fb drivers just work with the old bindings. If the
> > driver maintainer wants to use CDF, it's up to him to create any needed
> > binding-converters.
> >
> > But a generic driver for panels is more problematic, as that one should
> > be maintained and refreshed to new features like CDF. If such was
> > proposed to be merged to fbdev, I'd be very reluctant to merge it if
> > there were any concerns about it.
>
> I'm still missing the point here. For one, I don't see any reason that
> this driver would ever need to gain CDF support. At the same time, CDF
> could easily be supported by just adding a few required properties and
> it should be easy to support any non-CDF cases just as well to remain
> backwards-compatible.
>
> > I guess one option there would be to implement a new driver for the same
> > panel devices, one that supports CDF, and leave the old one as is. Not
> > so nice option, though.
>
> As I said, anything that really needs a CDF binding to work likely isn't
> "simple" anymore, therefore a separate driver can easily be justified.
The system as a whole would be more complex, but the panel could be the same.
We can't have two drivers for the same piece of hardware in the DT world, as
there will be a single compatible string and no way to choose between the
drivers (unlike the board code world that could set device names to "foo-
encoder-v4l2" or "foo-encoder-drm" and live happily with that ever after).
> > > For crying out loud, we can use the 3D engine to render to a framebuffer
> > > but we can't look at the result because we can't make the bloody panel
> > > light up! Seriously!?
> >
> > Ah, but do you have DT bindings for the 3D engine yet, which represent
> > how different internal blocks in the 3D engine are connected? If not,
> > better start designing it! Then the problem is solved, you won't even
> > have working 3D engine. ;)
> >
> > But seriously speaking, I'm with you. I don't get this DT stuff.
> >
> > And the funny thing is, DT should just represent the hardware, it
> > doesn't matter what kind of SW drivers there are, so in a sense talking
> > about DT problems with a SW framework like CDF is a bit silly. How hard
> > can it be to describe the hardware properly?
> >
> > Very, obviously =).
>
> Indeed. The fundamental point here seems to be that we don't represent
> things in DT which we don't need. In the same way that we don't add device
> nodes for enumerable devices, why would we need to explicitly add
> information about connections between devices that cannot be controlled
> anyway. If the board connects an RGB/LVDS output to a panel directly, it is
> only required that the output knows what it is connected to because there is
> no other way besides DT to obtain any information about the panel. Therefore
> a unidirectional connection is more than enough. Using that the output can
> access the panel and query it for information such as the mode timings as
> well as switch it on or off as required.
Please note that the idea of putting the modes in DT in no way implies that
the device controller should parse the modes from the panel node. It must
instead query the panel at runtime through whatever API is provided. Going
directly to the DT data would be a layering violation. The reason why I
propose to add modes to the panel DT node is to make the geenric panel driver
more generic and simpler.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 11:05 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, Laurent Pinchart, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <525FBA5D.9000001-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5174 bytes --]
On Thu, Oct 17, 2013 at 01:22:21PM +0300, Tomi Valkeinen wrote:
> On 17/10/13 11:53, Thierry Reding wrote:
>
> > I keep wondering why we absolutely must have compatibility between CDF
> > and this simple panel binding. DT content is supposed to concern itself
> > with the description of hardware only. What about the following isn't an
> > accurate description of the panel hardware?
> >
> > panel: panel {
> > compatible = "cptt,claa101wb01";
> >
> > power-supply = <&vdd_pnl_reg>;
> > enable-gpios = <&gpio 90 0>;
> >
> > backlight = <&backlight>;
> > };
> >
> > dsi-controller {
> > panel = <&panel>;
> > };
>
> That's quite similar to how my current out-of-mainline OMAP DSS DT
> bindings work. The difference is that I have a reference from the panel
> node to the video source (dsi-controller), instead of the other way
> around. I just find it more natural. It works the same way as, say,
> regulators, gpios, etc.
I suppose that depends on the way you look at it. In the above proposal
I consider the output (DSI controller) to use the panel as a resource
that provides a certain set of services (query mode timings, provide a
way to enable or disable the panel). Similarly the panel uses the
backlight as a resource that provides a certain set of services (such as
changing the brightness).
The above also follows the natural order of control. The panel has no
way to control the DSI output. However, it is the output that controls
when a panel is required and turn it on as appropriate.
> However, one thing unclear is where the panel node should be. You seem
> to have a DSI panel here. If the panel is truly not controlled in any
> way, maybe having the panel as a normal platform device is ok. But if
> the DSI panel is controlled with DSI messages, should it be a child of
> the dsi-controller node, the same way i2c peripherals are children of
> i2c master?
That's one possibility. In this particular case it's not even necessary
to use any of DSIs control methods to talk to the panel. The panel only
receives the data stream and "just works".
Even if the panel were to be controlled via DSI, I think keeping it as
a separate device node is equally valid. It's really boils down to what
I already said above. The output uses the panel as a resource, similar
to how other devices might use a GPIO controller to use a GPIO pin, or
an interrupt controller to request an IRQ.
> >>> +static const struct drm_display_mode auo_b101aw03_mode = {
> >>> + .clock = 51450,
> >>> + .hdisplay = 1024,
> >>> + .hsync_start = 1024 + 156,
> >>> + .hsync_end = 1024 + 156 + 8,
> >>> + .htotal = 1024 + 156 + 8 + 156,
> >>> + .vdisplay = 600,
> >>> + .vsync_start = 600 + 16,
> >>> + .vsync_end = 600 + 16 + 6,
> >>> + .vtotal = 600 + 16 + 6 + 16,
> >>> + .vrefresh = 60,
> >>> +};
> >>
> >> Instead of hardcoding the modes in the driver, which would then require to be
> >> updated for every new simple panel model (and we know there are lots of them),
> >> why don't you specify the modes in the panel DT node ? The simple panel driver
> >> would then become much more generic. It would also allow board designers to
> >> tweak h/v sync timings depending on the system requirements.
> >
> > Sigh... we keep second-guessing ourselves. Back at the time when power
> > sequences were designed (and NAKed at the last minute), we all decided
> > that the right thing to do would be to use specific compatible values
> > for individual panels, because that would allow us to encode the power
> > sequencing within the driver. And when we already have the panel model
> > encoded in the compatible value, we might just as well encode the mode
> > within the driver for that panel. Otherwise we'll have to keep adding
> > the same mode timings for every board that uses the same panel.
>
> Oh, I didn't feel "we all decided that the right thing..." =).
>
> > I do agree though that it might be useful to tweak the mode in case the
> > default one doesn't work. How about we provide a means to override the
> > mode encoded in the driver using one specified in the DT? That's
> > obviously a backwards-compatible change, so it could be added if or when
> > it becomes necessary.
>
> This sounds good to me.
>
> Although maybe it'd be good to have the driver compatible with something
> like "generic-panel", so that you could have:
>
> compatible = "foo,specific-panel", "generic-panel";
>
> and if there's no need for any power/gpio setup for your board, you may
> skip adding "foo,specific-panel" support to the panel driver. Later,
> somebody else may need to implement fine grained power/gpio support for
> "foo,specific-panel", and it would just work.
>
> Maybe that would help us with the problem of adding support in the
> driver for a hundred different simple panels each used only once on a
> specific board.
Sure, that all sounds like reasonable additions. All of the suggestions
are even backwards-compatible and hence can be added when needed without
breaking compatibility with existing users of the binding.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Laurent Pinchart @ 2013-10-17 11:02 UTC (permalink / raw)
To: Thierry Reding
Cc: Laurent Pinchart, Tomi Valkeinen, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie
In-Reply-To: <20131017085342.GB2502-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 10797 bytes --]
Hi Thierry,
On Thursday 17 October 2013 10:53:43 Thierry Reding wrote:
> Adding the dri-devel mailing list on Cc, since this discussion is
> outgrowing its original intent.
>
> On Thu, Oct 17, 2013 at 02:47:52AM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> >
> > Thank you for the patch.
> >
> > On Wednesday 16 October 2013 20:25:12 Thierry Reding wrote:
> > > Add a driver for simple panels. Such panels can have a regulator that
> > > provides the supply voltage and a separate GPIO to enable the panel.
> > > Optionally the panels can have a backlight associated with them so it
> > > can be enabled or disabled according to the panel's power management
> > > mode.
> > >
> > > Support is added for three panels: An AU Optronics 10.1" WSVGA, a
> > > Chunghwa Picture Tubes 10.1" WXGA and a Panasonic 10.1 WUXGA TFT LCD
> > > panel.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >
> > > .../devicetree/bindings/panel/auo,b101aw03.txt | 7 +
> > > .../bindings/panel/chunghwa,claa101wb03.txt | 7 +
> > > .../bindings/panel/panasonic,vvx10f004b00.txt | 7 +
> > > .../devicetree/bindings/panel/simple-panel.txt | 21 ++
> > > drivers/gpu/drm/Makefile | 1 +
> > > drivers/gpu/drm/panel/Kconfig | 13 +
> > > drivers/gpu/drm/panel/Makefile | 1 +
> > > drivers/gpu/drm/panel/panel-simple.c | 335 ++++++++++++++
> > > 8 files changed, 392 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/panel/auo,b101aw03.txt
> > > create mode 100644
> > > Documentation/devicetree/bindings/panel/chunghwa,claa101wb03.txt
> > > create mode 100644
> > > Documentation/devicetree/bindings/panel/panasonic,vvx10f004b00.txt
> > > create mode 100644
> > > Documentation/devicetree/bindings/panel/simple-panel.txt
> > > create mode 100644 drivers/gpu/drm/panel/Makefile
> > > create mode 100644 drivers/gpu/drm/panel/panel-simple.c
[snip]
> > > diff --git a/Documentation/devicetree/bindings/panel/simple-panel.txt
> > > b/Documentation/devicetree/bindings/panel/simple-panel.txt new file mode
> > > 100644
> > > index 0000000..1341bbf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/panel/simple-panel.txt
> > > @@ -0,0 +1,21 @@
> > > +Simple display panel
> > > +
> > > +Required properties:
> > > +- power-supply: regulator to provide the supply voltage
> > > +
> > > +Optional properties:
> > > +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> > > +- enable-gpios: GPIO pin to enable or disable the panel
> > > +- backlight: phandle of the backlight device attached to the panel
> > > +
> > > +Example:
> > > +
> > > + panel: panel {
> > > + compatible = "cptt,claa101wb01";
> > > + ddc-i2c-bus = <&panelddc>;
> > > +
> > > + power-supply = <&vdd_pnl_reg>;
> > > + enable-gpios = <&gpio 90 0>;
> > > +
> > > + backlight = <&backlight>;
> > > + };
> >
> > My biggest concern here is that this will not be compatible with the CDF
> > DT bindings. They're not complete yet, but they will require connections
> > between entities to be described in DT, in a way very similar (or
> > actually identical) to the V4L2 DT bindings, documented in
> > Documentation/devicetree/bindings/media/video-interfaces.txt. Could you
> > have a look at that ? Please ignore all optional properties beside
> > remote-endpoint, as they're V4L2 specific.
>
> Okay, so if I understand correctly, translating those bindings to panel
> nodes would look somewhat like this:
>
> dc: display-controller {
> ports {
> port@0 {
> remote-endpoint = <&panel>;
> };
> };
> };
>
> panel: panel {
> ports {
> port@0 {
> remote-endpoint = <&dc>;
> };
> };
> };
>
> The above leaves out any of the other, non-relevant properties. Does
> that sound about right?
Yes it does.
> That seems like an overly complex amount of data to describe just a simple
> panel where the only connection between it and the display hardware is the
> data bus and I was sort of expecting the CDF to provide some sort of
> shortcut for setups where the connection is 1:1 with no means to perform any
> configuration of the bus.
CDF needs to model video pipelines that can be more complex than just a
display controller with a single output connected to a panel with a single
input. For that reason the DT bindings need to describe connections in a
generic enough way. The above DT fragment might be slightly more complex than
one would expect when thinking about the really simple case only, but it's
nowhere near overly complex in my opinion.
Please note that, when a device has as single port, the ports node can be
omitted, and the port doesn't need to be numbered. You would then end up with
dc: display-controller {
port {
remote-endpoint = <&panel>;
};
};
panel: panel {
port {
remote-endpoint = <&dc>;
};
};
I don't think there's a way to simplify it further.
> > I also plan to specify video bus parameters in DT for CDF, but this hasn't
> > been finalized yet.
>
> That effectively blocks any of the code that I've been working on until
> CDF has been merged. It's been discussed for over a year now, and there
> has even been significant pushback on using CDF in DRM, so even if CDF
> is eventually merged, that will still leave DRM with no way to turn on
> a simple panel.
I'm probably even more concerned about the long time all this took and will
still take than you are, sincerely. I'm obviously to blame for not being able
to explain the problems and solutions correctly, as I see no way why people
would push back if they realized that the other solutions that have been
proposed, far from being future-proof, won't even be present-proof. We would
be shooting ourselves in the foot, but what can I do when too many people want
to do so ? I'll still keep trying of course, but not for years.I have yet to
see someone proposing a viable solution to share drivers between DRM and V4L2,
which is a real pressing requirement, partly due to DT.
Regarding the use of CDF in DRM, please note that it will be optional for
drivers to switch to the new framework. Nothing should hopefully require
existing drivers to be converted, drivers that want to make use of CDF to
support panels will be welcome to do so, but they big DRM drivers for desktop
GPUs won't be affected.
> I keep wondering why we absolutely must have compatibility between CDF
> and this simple panel binding. DT content is supposed to concern itself
> with the description of hardware only. What about the following isn't an
> accurate description of the panel hardware?
>
> panel: panel {
> compatible = "cptt,claa101wb01";
>
> power-supply = <&vdd_pnl_reg>;
> enable-gpios = <&gpio 90 0>;
>
> backlight = <&backlight>;
> };
>
> dsi-controller {
> panel = <&panel>;
> };
>
> Note how the above is also not incompatible with anything that the CDF
> (to be) mandates, so it could easily be extended with any nodes or
> properties that CDF needs to work properly without breaking backwards-
> compatibility.
As mentioned above, CDF needs to describe more complex pipelines. To make the
driver life as simple as possible the core code will handle the pipeline
description in the DT bindings, and it does require it to be generic enough.
You can of course describe connections in various ways depending on the
hardware, which would make the DT-related code needlessly overcomplex. One of
the issue with the above description is that there's no way to go from the
panel to the display controller, which might be a show stopper in the future
when code requires that. A workaround would be to look through the whole DT
for the reverse link, which would be pretty inefficient.
> > > diff --git a/drivers/gpu/drm/panel/panel-simple.c
[...]
> > > +static void panel_simple_enable(struct drm_panel *panel)
> > > +{
> > > + struct panel_simple *p = to_panel_simple(panel);
> > > + int err;
> > > +
> > > + if (p->enabled)
> > > + return;
> > > +
> > > + err = regulator_enable(p->supply);
> > > + if (err < 0)
> > > + dev_err(panel->dev, "failed to enable supply: %d\n", err);
> >
> > Is that really a non-fatal error ? Shouldn't the enable operation return
> > an int ?
>
> There's no way to propagate this in DRM, so why go through the trouble
> of returning the error? Furthermore, there's nothing that the caller
> could do to remedy the situation anyway.
That's a DRM issue, which could be fixed. While the caller can't remedy the
situation, it should at least report the error to the application instead of
silently ignoring it.
> > > +static const struct drm_display_mode auo_b101aw03_mode = {
> > > + .clock = 51450,
> > > + .hdisplay = 1024,
> > > + .hsync_start = 1024 + 156,
> > > + .hsync_end = 1024 + 156 + 8,
> > > + .htotal = 1024 + 156 + 8 + 156,
> > > + .vdisplay = 600,
> > > + .vsync_start = 600 + 16,
> > > + .vsync_end = 600 + 16 + 6,
> > > + .vtotal = 600 + 16 + 6 + 16,
> > > + .vrefresh = 60,
> > > +};
> >
> > Instead of hardcoding the modes in the driver, which would then require to
> > be updated for every new simple panel model (and we know there are lots
> > of them), why don't you specify the modes in the panel DT node ? The
> > simple panel driver would then become much more generic. It would also
> > allow board designers to tweak h/v sync timings depending on the system
> > requirements.
>
> Sigh... we keep second-guessing ourselves. Back at the time when power
> sequences were designed (and NAKed at the last minute), we all decided
> that the right thing to do would be to use specific compatible values
> for individual panels, because that would allow us to encode the power
> sequencing within the driver. And when we already have the panel model
> encoded in the compatible value, we might just as well encode the mode
> within the driver for that panel. Otherwise we'll have to keep adding
> the same mode timings for every board that uses the same panel.
>
> I do agree though that it might be useful to tweak the mode in case the
> default one doesn't work. How about we provide a means to override the
> mode encoded in the driver using one specified in the DT? That's
> obviously a backwards-compatible change, so it could be added if or when
> it becomes necessary.
I share Tomi's point of view here. Your three panels use the same power
sequence, so I believe we should have a generic panel compatible string that
would use modes described in DT for the common case. Only if a panel required
something more complex which can't (or which could, but won't for any reason)
accurately be described in DT would you need to extend the driver.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [RFR 2/2] drm/panel: Add simple panel support
From: Thierry Reding @ 2013-10-17 10:48 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Dave Airlie, Laurent Pinchart, Laurent Pinchart, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
devicetree-u79uwXL29TY76Z2rM5mHXA, Linux Fbdev development list
In-Reply-To: <525FB368.6020003-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5264 bytes --]
On Thu, Oct 17, 2013 at 12:52:40PM +0300, Tomi Valkeinen wrote:
> On 17/10/13 12:16, Thierry Reding wrote:
> > On Thu, Oct 17, 2013 at 11:49:52AM +0300, Tomi Valkeinen wrote:
>
> >> Just my opinion, but yes, DT is that inflexible. DT bindings are like a
> >> syscall. Once they are there, you need to support them forever. That
> >> support can, of course, include some kind of DT data converters or such,
> >> that try to convert old bindings to new bindings at kernel boot.
> >
> > That's not entirely true. DT bindings are allowed to change, the only
> > restriction being that they don't break backwards compatibility. So if
>
> True, but afaik the same goes for syscalls. But yes, it's probably
> easier to add, say, new properties to DT bindings than adding new flags
> to a syscall.
>
> > Am I the only one refusing to accept that we can't provide even the most
> > basic functionality simply because we haven't been able to come up with
> > the ultimately "perfect" DT binding yet? By definition it's not very
> > likely that any binding will ever be perfect.
>
> With "perfect" I meant without any obvious features missing. I agree
> that DT bindings will never be perfect, but I at least would like them
> to be good enough to cover all the cases we know of.
As far as the simple panel device tree binding is concerned, it covers
all the cases I know of. I can't seriously be expected to do any better
than that. These panels are dumb. They only come with a power supply
that needs to be switched on and an additional GPIO to enable it once
powered up. All three panels just work with that set of properties. So
anything that doesn't can arguably be covered by some other, not-so-
simple binding.
> > I don't think we're doing ourselves any good by letting DT actively
> > hinder us in merging any of these features. I would personally prefer
> > having to support several bindings rather than not be able to provide
> > the functionality at all.
>
> I'd say the driver writer is of course free to create basic bindings for
> the device in question as long as the bindings are sane, and he agrees
> to later create any needed converters. I don't see any problem with
> having a driver supporting several versions of the bindings.
>
> But I don't think it's fair to push a driver with basic bindings,
> knowing that the bindings won't be enough in the future, and leave all
> the converter-mess to other people (not saying that's the case here).
>
> As a fbdev maintainer, I'm ok with adding DT bindings to device specific
> fb drivers, as those can be just left as they are. Even when CDF is
> merged, the specific fb drivers just work with the old bindings. If the
> driver maintainer wants to use CDF, it's up to him to create any needed
> binding-converters.
>
> But a generic driver for panels is more problematic, as that one should
> be maintained and refreshed to new features like CDF. If such was
> proposed to be merged to fbdev, I'd be very reluctant to merge it if
> there were any concerns about it.
I'm still missing the point here. For one, I don't see any reason that
this driver would ever need to gain CDF support. At the same time, CDF
could easily be supported by just adding a few required properties and
it should be easy to support any non-CDF cases just as well to remain
backwards-compatible.
> I guess one option there would be to implement a new driver for the same
> panel devices, one that supports CDF, and leave the old one as is. Not
> so nice option, though.
As I said, anything that really needs a CDF binding to work likely isn't
"simple" anymore, therefore a separate driver can easily be justified.
> > For crying out loud, we can use the 3D engine to render to a framebuffer
> > but we can't look at the result because we can't make the bloody panel
> > light up! Seriously!?
>
> Ah, but do you have DT bindings for the 3D engine yet, which represent
> how different internal blocks in the 3D engine are connected? If not,
> better start designing it! Then the problem is solved, you won't even
> have working 3D engine. ;)
>
> But seriously speaking, I'm with you. I don't get this DT stuff.
>
> And the funny thing is, DT should just represent the hardware, it
> doesn't matter what kind of SW drivers there are, so in a sense talking
> about DT problems with a SW framework like CDF is a bit silly. How hard
> can it be to describe the hardware properly?
>
> Very, obviously =).
Indeed. The fundamental point here seems to be that we don't represent
things in DT which we don't need. In the same way that we don't add
device nodes for enumerable devices, why would we need to explicitly add
information about connections between devices that cannot be controlled
anyway. If the board connects an RGB/LVDS output to a panel directly, it
is only required that the output knows what it is connected to because
there is no other way besides DT to obtain any information about the
panel. Therefore a unidirectional connection is more than enough. Using
that the output can access the panel and query it for information such
as the mode timings as well as switch it on or off as required.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox