diff mbox

[13/26] ARM: omap3.dtsi: add omapdss information

Message ID 1386160133-24026-14-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen Dec. 4, 2013, 12:28 p.m. UTC
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/boot/dts/omap3.dtsi | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Comments

Tony Lindgren Dec. 5, 2013, 5:05 p.m. UTC | #1
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131204 04:31]:

Description missing.. But other than that can you please check that
the latest patch I posted in thread "[PATCH] ARM: OMAP2+: Fix populating
the hwmod data from device" works with this?

The test to do is to remove the related reg, interrupt and dma entries
from omap_hwmod_*_data.c, and make sure the related hwmod data is initialized
from DT properly.

I don't know if it makes sense to have them as children of dss_core, they
really all seem to be completely independent devices?

BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
property and do the lookup based on the compatible property instead ;)
So from that point of view we need to get the device mapping right in
the .dtsi files, and don't want to start mixing up separate devices into
single .dtsi entry.

Regards,

Tony

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  arch/arm/boot/dts/omap3.dtsi | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> index f3a0c26ed0c2..6fc163201cbd 100644
> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -588,5 +588,48 @@
>  			num-eps = <16>;
>  			ram-bits = <12>;
>  		};
> +
> +		dss@48050000 {
> +			compatible = "ti,omap3-dss", "simple-bus";
> +			reg = <0x48050000 0x200>;
> +			ti,hwmods = "dss_core";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			dispc@48050400 {
> +				compatible = "ti,omap3-dispc";
> +				reg = <0x48050400 0x400>;
> +				interrupts = <25>;
> +				ti,hwmods = "dss_dispc";
> +			};
> +
> +			dpi: encoder@0 {
> +				compatible = "ti,omap3-dpi";
> +			};
> +
> +			sdi: encoder@1 {
> +				compatible = "ti,omap3-sdi";
> +			};
> +
> +			dsi: encoder@4804fc00 {
> +				compatible = "ti,omap3-dsi";
> +				reg = <0x4804fc00 0x400>;
> +				interrupts = <25>;
> +				ti,hwmods = "dss_dsi1";
> +			};
> +
> +			rfbi: encoder@48050800 {
> +				compatible = "ti,omap3-rfbi";
> +				reg = <0x48050800 0x100>;
> +				ti,hwmods = "dss_rfbi";
> +			};
> +
> +			venc: encoder@48050c00 {
> +				compatible = "ti,omap3-venc";
> +				reg = <0x48050c00 0x100>;
> +				ti,hwmods = "dss_venc";
> +			};
> +		};
>  	};
>  };
> -- 
> 1.8.3.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Dec. 9, 2013, 12:45 p.m. UTC | #2
On 2013-12-05 19:05, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [131204 04:31]:
> 
> Description missing.. But other than that can you please check that
> the latest patch I posted in thread "[PATCH] ARM: OMAP2+: Fix populating
> the hwmod data from device" works with this?
>
> The test to do is to remove the related reg, interrupt and dma entries
> from omap_hwmod_*_data.c, and make sure the related hwmod data is initialized
> from DT properly.

I made a quick test with panda, by applying your patch and reverting
b38911f3472be89551bfca740adf0009562b9873. That only effectively tests
the DISPC IRQ, but that worked fine.

> I don't know if it makes sense to have them as children of dss_core, they
> really all seem to be completely independent devices?

The DSS subdevices depend on the dss_core. dss_core has to be powered up
for any of the subdevices to work. This is done automatically by the
runtime PM when the subdevices are children of the dss_core.

> BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
> property and do the lookup based on the compatible property instead ;)
> So from that point of view we need to get the device mapping right in
> the .dtsi files, and don't want to start mixing up separate devices into
> single .dtsi entry.

Hmm, was that just a general comment, or something that affects the DSS
DT data I have in my patch? As far as I understand, the DSS nodes
reflect the current hwmods correctly.

With the exception that DPI and SDI do not have a matching hwmod, as
they are really part of dss_core/dispc. They are separate nodes as they
are "video outputs" the same way as the other subnodes.

I could perhaps remove the DPI and SDI nodes, and have them as direct
video ports from DISPC, but... That's easier said than done.

 Tomi
Tony Lindgren Dec. 9, 2013, 6:04 p.m. UTC | #3
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131209 04:46]:
> On 2013-12-05 19:05, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [131204 04:31]:
> > 
> > Description missing.. But other than that can you please check that
> > the latest patch I posted in thread "[PATCH] ARM: OMAP2+: Fix populating
> > the hwmod data from device" works with this?
> >
> > The test to do is to remove the related reg, interrupt and dma entries
> > from omap_hwmod_*_data.c, and make sure the related hwmod data is initialized
> > from DT properly.
> 
> I made a quick test with panda, by applying your patch and reverting
> b38911f3472be89551bfca740adf0009562b9873. That only effectively tests
> the DISPC IRQ, but that worked fine.

OK I've finally pushed a real branch for the mach-omap2 board-*.c file
removal patches at omap-for-v3.14/omap3-board-removal so you can use
that as a base for testing. I did not apply the dpi panel pdata-quirks.c
patch as we discussed earlier.
 
> > I don't know if it makes sense to have them as children of dss_core, they
> > really all seem to be completely independent devices?
> 
> The DSS subdevices depend on the dss_core. dss_core has to be powered up
> for any of the subdevices to work. This is done automatically by the
> runtime PM when the subdevices are children of the dss_core.

OK thanks. Care to also check that it plays along nicely with the comments
starting at line 3222 in omap_hwmod_3xxx_data.c? We should set up things
so we can eventually remove those kind of hwmod workarounds.
 
> > BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
> > property and do the lookup based on the compatible property instead ;)
> > So from that point of view we need to get the device mapping right in
> > the .dtsi files, and don't want to start mixing up separate devices into
> > single .dtsi entry.
> 
> Hmm, was that just a general comment, or something that affects the DSS
> DT data I have in my patch? As far as I understand, the DSS nodes
> reflect the current hwmods correctly.

Yes that's what we want if there is a dependency to the dss_core at the
hardware level and the children cannot be used independently. However,
if the children can be enabled and clocked independently, then they
should not be children of the dss_core.
 
> With the exception that DPI and SDI do not have a matching hwmod, as
> they are really part of dss_core/dispc. They are separate nodes as they
> are "video outputs" the same way as the other subnodes.
> 
> I could perhaps remove the DPI and SDI nodes, and have them as direct
> video ports from DISPC, but... That's easier said than done.

If you need a dev entry created for those where the phandle of that dev
is used to select the output for a board, then it makes sense to have
them. I guess you could also set them as a pinctrl mux controller and
then the board specific .dts file could request those outputs. But there
may be more than just mux involved like regulators.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Dec. 11, 2013, 11:44 p.m. UTC | #4
Hi Tomi,

On Monday 09 December 2013 14:45:25 Tomi Valkeinen wrote:
> On 2013-12-05 19:05, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [131204 04:31]:
> > 
> > Description missing.. But other than that can you please check that
> > the latest patch I posted in thread "[PATCH] ARM: OMAP2+: Fix populating
> > the hwmod data from device" works with this?
> > 
> > The test to do is to remove the related reg, interrupt and dma entries
> > from omap_hwmod_*_data.c, and make sure the related hwmod data is
> > initialized from DT properly.
> 
> I made a quick test with panda, by applying your patch and reverting
> b38911f3472be89551bfca740adf0009562b9873. That only effectively tests
> the DISPC IRQ, but that worked fine.
> 
> > I don't know if it makes sense to have them as children of dss_core, they
> > really all seem to be completely independent devices?
> 
> The DSS subdevices depend on the dss_core. dss_core has to be powered up
> for any of the subdevices to work. This is done automatically by the
> runtime PM when the subdevices are children of the dss_core.

I'd like to get a clearer picture of the hardware here. The OMAP3 ISP is 
organized in a seemingly similar way, with the hardware subdivided in high-
level more-or-less independent modules. However, from a system point of view, 
the ISP submodules are not standalone: they're part of the same power domain 
as the ISP core, with subclocks management and interrupts being handled by the 
ISP core. For those reasons we've modeled the ISP as a single platform device.

Are the DSS submodules really independent, or are they organized similarly ? 
For instance do they each have a clock handled by the OMAP core clock IP, or 
are the clocks gated by the DSS core ? Do they have interrupts routed to the 
GIC, or does the DSS core driver demux the interrupts ?

If the submodules are not independent, would it make sense to have a single DT 
node that would be matched with the DSS core driver ? You could list 
information about the submodules in subnodes, and possibly create platform 
devices internally in the DSS core, but a single platform device would be 
instantiated from DT, and the DSS core wouldn't need a "simple-bus" compatible 
string. My gut feeling is that this would be a better representation of the 
hardware, but I might not known enough about the DSS and be completely wrong.

> > BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
> > property and do the lookup based on the compatible property instead ;)
> > So from that point of view we need to get the device mapping right in
> > the .dtsi files, and don't want to start mixing up separate devices into
> > single .dtsi entry.
> 
> Hmm, was that just a general comment, or something that affects the DSS
> DT data I have in my patch? As far as I understand, the DSS nodes
> reflect the current hwmods correctly.
> 
> With the exception that DPI and SDI do not have a matching hwmod, as
> they are really part of dss_core/dispc. They are separate nodes as they
> are "video outputs" the same way as the other subnodes.
> 
> I could perhaps remove the DPI and SDI nodes, and have them as direct
> video ports from DISPC, but... That's easier said than done.

DPI and SDI indeed seem like ports to me, node devices. Have you given the 
implementation a thought ? How difficult would it be ?
Tomi Valkeinen Dec. 12, 2013, 8:38 a.m. UTC | #5
On 2013-12-12 01:44, Laurent Pinchart wrote:

>> The DSS subdevices depend on the dss_core. dss_core has to be powered up
>> for any of the subdevices to work. This is done automatically by the
>> runtime PM when the subdevices are children of the dss_core.
> 
> I'd like to get a clearer picture of the hardware here. The OMAP3 ISP is 
> organized in a seemingly similar way, with the hardware subdivided in high-
> level more-or-less independent modules. However, from a system point of view, 
> the ISP submodules are not standalone: they're part of the same power domain 
> as the ISP core, with subclocks management and interrupts being handled by the 
> ISP core. For those reasons we've modeled the ISP as a single platform device.
> 
> Are the DSS submodules really independent, or are they organized similarly ? 
> For instance do they each have a clock handled by the OMAP core clock IP, or 
> are the clocks gated by the DSS core ? Do they have interrupts routed to the 
> GIC, or does the DSS core driver demux the interrupts ?

The DSS is "interesting". The TRM for various OMAP versions are the best
source of information, there's integration section in the very beginning
of the DSS chapter.

We have the main dss_core (just DSS in the TRM, but for clarity we use
dss_core) module, which is kind of a wrapper/glue for all the
submodules. dss_core contains things like controlling muxes for signals
between submodules, or clocks coming from outside. And there's the DSS
power domain, containing all the DSS modules.

Then we have DISPC, which reads the pixel data, manipulates it, and
outputs raw RGB data to encoder submodules.

Then we have DSI, HDMI, RFBI, VENC encoder submodules. They all have
separate address spaces, some have dedicated PLLs, PHYs, and interrupts.

Then DPI, which I think is mostly just level shifters. It's really just
a port, as you say.

SDI is a bit unclear to me. The registers for it are in the dss_core.
There's only a few registers, but it does have a PHY and a PLL. But I
guess it's also more of a port than a separate module.

As for the clocks, I'm not sure what the actual point is that you want
to clarify. DSS gets one "main" func clock from PRCM, which is used by
DISPC and in some cases other submodules. But then we have dedicated DSI
and HDMI PLLs, which, at least in DSI's case, can be used to fully
satisfy DSI's clock needs. The PLLs can also be used for DISPC, so the
PRCM clock is not needed in all cases.

The interrupts, then. In OMAP4, DISPC, DSI1, DSI2 and HDMI each have
their own interrupt line. In OMAP3, DISPC and DSI shared the same
interrupt line. But in both OMAP4 and OMAP3 DISPC and DSI interrupt
status/enable is handled via the respective IP.

The DSS submodules also are not really designed together. For example,
the HDMI IP is from one vendor, not TI. And the HDMI IP is different in
OMAP4 and OMAP5. Most of the DSS IPs are, I believe, from TI. But it's
not like all the IPs were designed to work together, that's why we have
wrappers/glue blocks (e.g. around HDMI).

So, are they independent? I don't know =). I think they lean on the
independent side. dss_core is always needed for the submodules to work,
but for example DSI could be used without DISPC, using system DMA to
transfer data from memory to DSI. Not a very useful thing to do, but
still, there are dedicated DMA channels for that.

> If the submodules are not independent, would it make sense to have a single DT 
> node that would be matched with the DSS core driver ? You could list 
> information about the submodules in subnodes, and possibly create platform 
> devices internally in the DSS core, but a single platform device would be 
> instantiated from DT, and the DSS core wouldn't need a "simple-bus" compatible 
> string. My gut feeling is that this would be a better representation of the 
> hardware, but I might not known enough about the DSS and be completely wrong.

I have been wondering about this for a long time. The DSS modules have
dependencies, and splitting them into separate devices/drivers brings
the issue of probe order. We side-step that by having the virtual
omapdss driver add the drivers for DSS modules in proper order.

But then, I feel that they are quite independent and probably should be
separate devices. And we've had omap hwmods, which I believe force us to
have separate devices (although afaik hwmods are going away).

>>> BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
>>> property and do the lookup based on the compatible property instead ;)
>>> So from that point of view we need to get the device mapping right in
>>> the .dtsi files, and don't want to start mixing up separate devices into
>>> single .dtsi entry.
>>
>> Hmm, was that just a general comment, or something that affects the DSS
>> DT data I have in my patch? As far as I understand, the DSS nodes
>> reflect the current hwmods correctly.
>>
>> With the exception that DPI and SDI do not have a matching hwmod, as
>> they are really part of dss_core/dispc. They are separate nodes as they
>> are "video outputs" the same way as the other subnodes.
>>
>> I could perhaps remove the DPI and SDI nodes, and have them as direct
>> video ports from DISPC, but... That's easier said than done.
> 
> DPI and SDI indeed seem like ports to me, node devices. Have you given the 
> implementation a thought ? How difficult would it be ?

I have not though too much about the implementation. I'll spend some
time on that to see how it goes.

There's also the question where do the ports belong to. DISPC outputs
the pixels.

For DPI, I don't see dss_core really doing anything.

For SDI, the dss_core contains the control for the SDI PLL and PHY. But
SDI PLL and PHY are not parts of dss_core, just the control is done via
dss_core.

 Tomi
Tony Lindgren Dec. 12, 2013, 9:59 p.m. UTC | #6
On Thu, Dec 12, 2013 at 10:38:34AM +0200, Tomi Valkeinen wrote:
> On 2013-12-12 01:44, Laurent Pinchart wrote:
> 
> So, are they independent? I don't know =). I think they lean on the
> independent side. dss_core is always needed for the submodules to work,
> but for example DSI could be used without DISPC, using system DMA to
> transfer data from memory to DSI. Not a very useful thing to do, but
> still, there are dedicated DMA channels for that.

If they have separate hwmod entries, they should be considered separate
independent devices for sure.

To summarize, here are few reasons why they need to be treated as
separate devices:

1. The modules maybe clocked/powered/idled separately and can have their
   own idle configuration so they can do the hardware based idling
   separately.

2. Doing a readback after a write to one module will not flush the write
   to the other modules on the (bus depending on the SoC version AFAIK).
   That can lead to nasty bugs caused by the ordering.

3. If the devices are described in a different way in the .dts files
   from the hwmod data, we will not have 1-to-1 mapping and will never
   be able to replace ti,hwmods with just the compatible string.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Dec. 13, 2013, 3:24 a.m. UTC | #7
Hi Tomi,

On Thursday 12 December 2013 10:38:34 Tomi Valkeinen wrote:
> On 2013-12-12 01:44, Laurent Pinchart wrote:
> >> The DSS subdevices depend on the dss_core. dss_core has to be powered up
> >> for any of the subdevices to work. This is done automatically by the
> >> runtime PM when the subdevices are children of the dss_core.
> > 
> > I'd like to get a clearer picture of the hardware here. The OMAP3 ISP is
> > organized in a seemingly similar way, with the hardware subdivided in
> > high-level more-or-less independent modules. However, from a system point
> > of view, the ISP submodules are not standalone: they're part of the same
> > power domain as the ISP core, with subclocks management and interrupts
> > being handled by the ISP core. For those reasons we've modeled the ISP as
> > a single platform device.
> > 
> > Are the DSS submodules really independent, or are they organized similarly
> > ? For instance do they each have a clock handled by the OMAP core clock
> > IP, or are the clocks gated by the DSS core ? Do they have interrupts
> > routed to the GIC, or does the DSS core driver demux the interrupts ?
> 
> The DSS is "interesting". The TRM for various OMAP versions are the best
> source of information, there's integration section in the very beginning
> of the DSS chapter.
> 
> We have the main dss_core (just DSS in the TRM, but for clarity we use
> dss_core) module, which is kind of a wrapper/glue for all the
> submodules. dss_core contains things like controlling muxes for signals
> between submodules, or clocks coming from outside. And there's the DSS
> power domain, containing all the DSS modules.
> 
> Then we have DISPC, which reads the pixel data, manipulates it, and
> outputs raw RGB data to encoder submodules.
> 
> Then we have DSI, HDMI, RFBI, VENC encoder submodules. They all have
> separate address spaces, some have dedicated PLLs, PHYs, and interrupts.

The separate address spaces are not by themselves a clear indication that the 
submodules should be considered as separate devices, as the hardware might 
just group registers related to submodules together.

The dedicated interrupts (for DSI and HDMI) and PRCM clocks (for VENC if I'm 
not mistaken, and HDMI on the OMAP4) are a clearer sign. 

> Then DPI, which I think is mostly just level shifters. It's really just
> a port, as you say.
> 
> SDI is a bit unclear to me. The registers for it are in the dss_core.
> There's only a few registers, but it does have a PHY and a PLL. But I
> guess it's also more of a port than a separate module.

After a quick look at the documentation I would say so. I would be tempted to 
consider RFBI as part of the DSS core, but that's less clear.

> As for the clocks, I'm not sure what the actual point is that you want
> to clarify. DSS gets one "main" func clock from PRCM, which is used by
> DISPC and in some cases other submodules. But then we have dedicated DSI
> and HDMI PLLs, which, at least in DSI's case, can be used to fully
> satisfy DSI's clock needs. The PLLs can also be used for DISPC, so the
> PRCM clock is not needed in all cases.
> 
> The interrupts, then. In OMAP4, DISPC, DSI1, DSI2 and HDMI each have
> their own interrupt line. In OMAP3, DISPC and DSI shared the same
> interrupt line. But in both OMAP4 and OMAP3 DISPC and DSI interrupt
> status/enable is handled via the respective IP.
> 
> The DSS submodules also are not really designed together. For example,
> the HDMI IP is from one vendor, not TI. And the HDMI IP is different in
> OMAP4 and OMAP5. Most of the DSS IPs are, I believe, from TI. But it's
> not like all the IPs were designed to work together, that's why we have
> wrappers/glue blocks (e.g. around HDMI).
> 
> So, are they independent? I don't know =). I think they lean on the
> independent side.

I agree with that, except for DPI, SDI and possibly RFBI.

> dss_core is always needed for the submodules to work, but for example DSI
> could be used without DISPC, using system DMA to transfer data from memory
> to DSI. Not a very useful thing to do, but still, there are dedicated DMA
> channels for that.

Right. The real question is whether the DSI or HDMI IPs can be used in a 
system without the DSS core. If not, it might make sense to just merge the 
drivers into a single module (of course with a clear interface between the 
different parts to avoid spaghetti code).

> > If the submodules are not independent, would it make sense to have a
> > single DT node that would be matched with the DSS core driver ? You could
> > list information about the submodules in subnodes, and possibly create
> > platform devices internally in the DSS core, but a single platform device
> > would be instantiated from DT, and the DSS core wouldn't need a
> > "simple-bus" compatible string. My gut feeling is that this would be a
> > better representation of the hardware, but I might not known enough about
> > the DSS and be completely wrong.
>
> I have been wondering about this for a long time. The DSS modules have
> dependencies, and splitting them into separate devices/drivers brings
> the issue of probe order. We side-step that by having the virtual
> omapdss driver

Given that the DSS core has a set of registers not dedicated to any of the 
submodules I believe it should be represented by a device. The omapdss driver 
thus doesn't look virtual to me, it supports a real piece of hardware.

> add the drivers for DSS modules in proper order.
>
> But then, I feel that they are quite independent and probably should be
> separate devices.

Even if they're separate devices they could be instantiated by DSS core based 
on DT nodes. I'm not sure whether that's the best idea, but it might be worth 
thinking about it.

> And we've had omap hwmods, which I believe force us to have separate devices
> (although afaik hwmods are going away).
> 
> >>> BTW, for v3.15, I'm hoping to do patches where we deprecate ti,hwmods
> >>> property and do the lookup based on the compatible property instead ;)
> >>> So from that point of view we need to get the device mapping right in
> >>> the .dtsi files, and don't want to start mixing up separate devices into
> >>> single .dtsi entry.
> >> 
> >> Hmm, was that just a general comment, or something that affects the DSS
> >> DT data I have in my patch? As far as I understand, the DSS nodes
> >> reflect the current hwmods correctly.
> >> 
> >> With the exception that DPI and SDI do not have a matching hwmod, as
> >> they are really part of dss_core/dispc. They are separate nodes as they
> >> are "video outputs" the same way as the other subnodes.
> >> 
> >> I could perhaps remove the DPI and SDI nodes, and have them as direct
> >> video ports from DISPC, but... That's easier said than done.
> > 
> > DPI and SDI indeed seem like ports to me, node devices. Have you given the
> > implementation a thought ? How difficult would it be ?
> 
> I have not though too much about the implementation. I'll spend some time on
> that to see how it goes.
> 
> There's also the question where do the ports belong to. DISPC outputs the
> pixels.
> 
> For DPI, I don't see dss_core really doing anything.
> 
> For SDI, the dss_core contains the control for the SDI PLL and PHY. But
> SDI PLL and PHY are not parts of dss_core, just the control is done via
> dss_core.

If the PLL and PHY are solely controlled through registers part of the DSS 
core register space, they would seem like part of the DSS core to me.
Laurent Pinchart Dec. 13, 2013, 3:27 a.m. UTC | #8
Hi Tony,

On Thursday 12 December 2013 21:59:13 Tony Lindgren wrote:
> On Thu, Dec 12, 2013 at 10:38:34AM +0200, Tomi Valkeinen wrote:
> > On 2013-12-12 01:44, Laurent Pinchart wrote:
> > 
> > So, are they independent? I don't know =). I think they lean on the
> > independent side. dss_core is always needed for the submodules to work,
> > but for example DSI could be used without DISPC, using system DMA to
> > transfer data from memory to DSI. Not a very useful thing to do, but
> > still, there are dedicated DMA channels for that.
> 
> If they have separate hwmod entries, they should be considered separate
> independent devices for sure.
> 
> To summarize, here are few reasons why they need to be treated as
> separate devices:

Are you talking generally here, or about the DSS modules in particular ?

> 1. The modules maybe clocked/powered/idled separately and can have their
>    own idle configuration so they can do the hardware based idling
>    separately.

I don't think this applies to the DSS modules.

> 2. Doing a readback after a write to one module will not flush the write
>    to the other modules on the (bus depending on the SoC version AFAIK).
>    That can lead to nasty bugs caused by the ordering.

How does separate devices solve this ?

> 3. If the devices are described in a different way in the .dts files
>    from the hwmod data, we will not have 1-to-1 mapping and will never
>    be able to replace ti,hwmods with just the compatible string.
Tomi Valkeinen Dec. 13, 2013, 9:29 a.m. UTC | #9
On 2013-12-13 05:24, Laurent Pinchart wrote:

> Right. The real question is whether the DSI or HDMI IPs can be used in a 
> system without the DSS core. If not, it might make sense to just merge the 
> drivers into a single module (of course with a clear interface between the 
> different parts to avoid spaghetti code).

The drivers are already in single kernel module, omapdss.ko.

The HDMI IP is used on another SoC, without DSS. They have their own
display architecture. DSI IP might need some small modifications to work
without DSS, but not much. It doesn't have any strict DSS/DISPC
dependencies.

> Given that the DSS core has a set of registers not dedicated to any of the 
> submodules I believe it should be represented by a device. The omapdss driver 
> thus doesn't look virtual to me, it supports a real piece of hardware.

As noted in another mail, dss_core and omapdss devices are different
things. dss_core is not virtual, omapdss is.

>> But then, I feel that they are quite independent and probably should be
>> separate devices.
> 
> Even if they're separate devices they could be instantiated by DSS core based 
> on DT nodes. I'm not sure whether that's the best idea, but it might be worth 
> thinking about it.

What would be the difference to the one in this series? In this series,
the submodules are instantiated automatically by the driver framework.
The only difference I see is that the submodule devices would
appear/disappear dynamically, but... what would be the benefit?

 Tomi
Tomi Valkeinen Dec. 13, 2013, 10:18 a.m. UTC | #10
On 2013-12-13 05:27, Laurent Pinchart wrote:
> Hi Tony,
> 
> On Thursday 12 December 2013 21:59:13 Tony Lindgren wrote:
>> On Thu, Dec 12, 2013 at 10:38:34AM +0200, Tomi Valkeinen wrote:
>>> On 2013-12-12 01:44, Laurent Pinchart wrote:
>>>
>>> So, are they independent? I don't know =). I think they lean on the
>>> independent side. dss_core is always needed for the submodules to work,
>>> but for example DSI could be used without DISPC, using system DMA to
>>> transfer data from memory to DSI. Not a very useful thing to do, but
>>> still, there are dedicated DMA channels for that.
>>
>> If they have separate hwmod entries, they should be considered separate
>> independent devices for sure.
>>
>> To summarize, here are few reasons why they need to be treated as
>> separate devices:
> 
> Are you talking generally here, or about the DSS modules in particular ?
> 
>> 1. The modules maybe clocked/powered/idled separately and can have their
>>    own idle configuration so they can do the hardware based idling
>>    separately.
> 
> I don't think this applies to the DSS modules.

The DSS submodules have their own SYSCONFIG register, and idle settings
can be set per module. So I think they idle separately, even if they are
in a common power domain.

 Tomi
Tony Lindgren Dec. 13, 2013, 5:10 p.m. UTC | #11
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131213 02:19]:
> On 2013-12-13 05:27, Laurent Pinchart wrote:
> > Hi Tony,
> > 
> > On Thursday 12 December 2013 21:59:13 Tony Lindgren wrote:
> >> On Thu, Dec 12, 2013 at 10:38:34AM +0200, Tomi Valkeinen wrote:
> >>> On 2013-12-12 01:44, Laurent Pinchart wrote:
> >>>
> >>> So, are they independent? I don't know =). I think they lean on the
> >>> independent side. dss_core is always needed for the submodules to work,
> >>> but for example DSI could be used without DISPC, using system DMA to
> >>> transfer data from memory to DSI. Not a very useful thing to do, but
> >>> still, there are dedicated DMA channels for that.
> >>
> >> If they have separate hwmod entries, they should be considered separate
> >> independent devices for sure.
> >>
> >> To summarize, here are few reasons why they need to be treated as
> >> separate devices:
> > 
> > Are you talking generally here, or about the DSS modules in particular ?
> > 
> >> 1. The modules maybe clocked/powered/idled separately and can have their
> >>    own idle configuration so they can do the hardware based idling
> >>    separately.
> > 
> > I don't think this applies to the DSS modules.
> 
> The DSS submodules have their own SYSCONFIG register, and idle settings
> can be set per module. So I think they idle separately, even if they are
> in a common power domain.

Yes. Please see the current omap_hwmod_*_data.c files, if they are separate
entries there, that means we need to treat them as separate devices to
avoid the issues I listed.

We do have some entries still missing from omap_hwmod_*_data.c files, like
the SSI entries as they are undocumented. But for the existing ones there
please follow the same layout for the .dts entries.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Dec. 16, 2013, 10:49 a.m. UTC | #12
On 2013-12-13 05:24, Laurent Pinchart wrote:

>> Then DPI, which I think is mostly just level shifters. It's really just
>> a port, as you say.
>>
>> SDI is a bit unclear to me. The registers for it are in the dss_core.
>> There's only a few registers, but it does have a PHY and a PLL. But I
>> guess it's also more of a port than a separate module.
> 
> After a quick look at the documentation I would say so. I would be tempted to 
> consider RFBI as part of the DSS core, but that's less clear.

I had a look at this, mainly the DPI side so far. There's one extra
complication, which actually affects all other outputs also (and CDF):
pinctrl.

In the current series, I just have pinctrl for each device, with
"default" name, which ends up being used by default without any code on
my part.

However, if DPI is no longer a device, it can't have pinctrl entry. But
this is a wider issue, as the pinctrl should really be per endpoint, not
per device. When an endpoint is selected to be used, a particular
pinmuxing should be taken into use.

I'm not sure what would be the cleanest solution to this. I currently
have this working:

&dss {
	pinctrl-names = "default-0-0";
	pinctrl-0 = <&dss_dpi_pins>;

	port@0 {
		dpi_out: endpoint {
			remote-endpoint = <&tfp410_in>;
			data-lines = <24>;
		};
	};
};

So here I have 'port@0' for DSS, which is the DPI output, and it has a
single endpoint. For DSS device, I have pinctrl data.

When the DPI endpoint is initialized, the code looks for pinctrl with
name "default-<portnum>-<endpointnum>". As the DPI is port 0, and just
one endpoint, the code looks for "default-0-0".

For omap3 board with both DPI and SDI as options (they can't be used at
the same time, though), I imagine it'd be like:

&dss {
	vdds_dsi-supply = <&vpll2>;
	vdds_sdi-supply = <&vpll2>;

	pinctrl-names = "default-0-0", "default-1-0";
	pinctrl-0 = <&dss_dpi_pins>;
	pinctrl-1 = <&dss_sdi_pins>;

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

		port@0 {
			reg = <0>;
			dpi_out: endpoint {
			};
		};

		port@1 {
			reg = <1>;
			sdi_out: endpoint {
			};
		};
	};
};

Any thoughts?

Every time I work with ports/endpoints, I feel that this is needlessly
complex. But I have never come up with any cleaner or simpler way to
handle this.

I also think this multiple-peripherals-per-single-port is not really
display related, although, for some reason, it seems like display is the
most abused hardware. Maybe ports/endpoints or similar should be in the
common driver framework?

 Tomi
Laurent Pinchart Dec. 16, 2013, 1:55 p.m. UTC | #13
Hi Tomi,

On Monday 16 December 2013 12:49:03 Tomi Valkeinen wrote:
> On 2013-12-13 05:24, Laurent Pinchart wrote:
> >> Then DPI, which I think is mostly just level shifters. It's really just
> >> a port, as you say.
> >> 
> >> SDI is a bit unclear to me. The registers for it are in the dss_core.
> >> There's only a few registers, but it does have a PHY and a PLL. But I
> >> guess it's also more of a port than a separate module.
> > 
> > After a quick look at the documentation I would say so. I would be tempted
> > to consider RFBI as part of the DSS core, but that's less clear.
> 
> I had a look at this, mainly the DPI side so far. There's one extra
> complication, which actually affects all other outputs also (and CDF):
> pinctrl.
> 
> In the current series, I just have pinctrl for each device, with
> "default" name, which ends up being used by default without any code on
> my part.
> 
> However, if DPI is no longer a device, it can't have pinctrl entry. But
> this is a wider issue, as the pinctrl should really be per endpoint, not
> per device. When an endpoint is selected to be used, a particular
> pinmuxing should be taken into use.
> 
> I'm not sure what would be the cleanest solution to this. I currently
> have this working:
> 
> &dss {
> 	pinctrl-names = "default-0-0";
> 	pinctrl-0 = <&dss_dpi_pins>;
> 
> 	port@0 {
> 		dpi_out: endpoint {
> 			remote-endpoint = <&tfp410_in>;
> 			data-lines = <24>;
> 		};
> 	};
> };
> 
> So here I have 'port@0' for DSS, which is the DPI output, and it has a
> single endpoint. For DSS device, I have pinctrl data.
> 
> When the DPI endpoint is initialized, the code looks for pinctrl with
> name "default-<portnum>-<endpointnum>". As the DPI is port 0, and just
> one endpoint, the code looks for "default-0-0".
> 
> For omap3 board with both DPI and SDI as options (they can't be used at
> the same time, though), I imagine it'd be like:
> 
> &dss {
> 	vdds_dsi-supply = <&vpll2>;
> 	vdds_sdi-supply = <&vpll2>;
> 
> 	pinctrl-names = "default-0-0", "default-1-0";
> 	pinctrl-0 = <&dss_dpi_pins>;
> 	pinctrl-1 = <&dss_sdi_pins>;
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		port@0 {
> 			reg = <0>;
> 			dpi_out: endpoint {
> 			};
> 		};
> 
> 		port@1 {
> 			reg = <1>;
> 			sdi_out: endpoint {
> 			};
> 		};
> 	};
> };
> 
> Any thoughts?

Would it be feasible to put the pinctrl properties in the port or endpoint 
nodes ? That could require changes to the pinctrl core, most probably just 
exporting a few internal functions (possibly requiring a bit of refactoring), 
but it might make the result simpler.

> Every time I work with ports/endpoints, I feel that this is needlessly
> complex. But I have never come up with any cleaner or simpler way to
> handle this.
>
> I also think this multiple-peripherals-per-single-port is not really
> display related, although, for some reason, it seems like display is the
> most abused hardware. Maybe ports/endpoints or similar should be in the
> common driver framework?

Ports and endpoints is the way we have come up with to describe a graph in DT. 
I wouldn't call it needlessly complex, as I believe it's both generic and 
simple, but I agree it's a bit on the verbose side. Omitting the ports and 
port nodes as a shortcut might be a good way to reduce the verbosity.

Regarding moving this to the device core, I'm not opposed to it, but I'd like 
to see interest from other users first.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index f3a0c26ed0c2..6fc163201cbd 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -588,5 +588,48 @@ 
 			num-eps = <16>;
 			ram-bits = <12>;
 		};
+
+		dss@48050000 {
+			compatible = "ti,omap3-dss", "simple-bus";
+			reg = <0x48050000 0x200>;
+			ti,hwmods = "dss_core";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			dispc@48050400 {
+				compatible = "ti,omap3-dispc";
+				reg = <0x48050400 0x400>;
+				interrupts = <25>;
+				ti,hwmods = "dss_dispc";
+			};
+
+			dpi: encoder@0 {
+				compatible = "ti,omap3-dpi";
+			};
+
+			sdi: encoder@1 {
+				compatible = "ti,omap3-sdi";
+			};
+
+			dsi: encoder@4804fc00 {
+				compatible = "ti,omap3-dsi";
+				reg = <0x4804fc00 0x400>;
+				interrupts = <25>;
+				ti,hwmods = "dss_dsi1";
+			};
+
+			rfbi: encoder@48050800 {
+				compatible = "ti,omap3-rfbi";
+				reg = <0x48050800 0x100>;
+				ti,hwmods = "dss_rfbi";
+			};
+
+			venc: encoder@48050c00 {
+				compatible = "ti,omap3-venc";
+				reg = <0x48050c00 0x100>;
+				ti,hwmods = "dss_venc";
+			};
+		};
 	};
 };