[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Jean-Francois Moine]

On Wed, 16 Nov 2016 22:33:06 +0100
Maxime Ripard  wrote:

> > > > The Device Engine just handles the planes of the LCDs, but, indeed,
> > > > the LCDs must know about the DE and the DE must know about the LCDs.
> > > > There are 2 ways to realize this knowledge in the DT:
> > > > 1) either the DE has one or two phandle's to the LCDs,
> > > > 2) or the LCDs have a phandle to the DE.
> > > > 
> > > > I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
> > > > which is part of the video link (OF-graph LCD <-> connector).
> > > > It would be possible to have phandles to the LCDs themselves, but this
> > > > asks for more code.
> > > > 
> > > > The second way is also possible, but it also complexifies a bit the
> > > > exchanges DE <-> LCD.
> > > 
> > > I'm still not sure how it would complexify anything, and why you can't
> > > use the display graph to model the relation between the display engine
> > > and the TCON (and why you want to use a generic property that refers
> > > to the of-graph while it really isn't).
> > 
> > Complexification:
> > 1- my solution:
> >   At startup time, the DE device is the DRM device.
> 
> How do you deal with SoCs with multiple display engines then?

In the H3, A83T and A64, there is only one DE.
If many DEs in a SoC, there may be either one DRM device per DE or one
DRM device for the whole system. In this last case, the (global) DE
would have many resources (many I/O memory maps / IRQs..) and the
physical DE of each endpoint would be indicated by the position of its
phandle in the 'ports' array (DT documentation).

> > It has to know the devices entering in the video chains.
> >   The CRTCs (LCD/TCON) are found by
> > ports[i] -> parent
> >   The connectors are found by
> > ports[i] -> endpoint -> remote_endpoint -> parent
> > 2- with ports pointing to the LCDs:
> >   The CRTCs (LCD/TCON) are simply
> > ports[i]
> >   The connectors are found by
> > loop on all ports of ports[i]
> > ports[i][j] -> endpoint -> remote_endpoint -> parent
> > 3- with a phandle to the DE in the LCDs:
> 
> What do you mean with LCD? Panels? Why would panels have a phandle to
> the display engine?

LCD is the same as CRTC. I don't think people will connect old CRT's to
their new ARM boards. LCD's may be panels, modern TV sets, or any
digital display. The word LCD seems clearer to me in this context, even
if there may a DAC as an ancoder.

> >   The DE cannot be the DRM device because there is no information about
> >   the video devices in the DT. Then, the DRM devices are the LCDs.
> >   These LCDs must give their indices to the DE. So, the DE must implement
> >   some callback function to accept a LCD definition, and there must be
> >   a list of DEs in the driver to make the association DE <-> LCD[i]
> >   Some more problem may be raised if a user wants to have the same frame
> >   buffer on the 2 LCDs of a DE.
> 
> I have no idea what you're talking about, sorry.

Here is the DT (I am using back 'CRTC'):

de: display-controller@xxx {
...
};
crtc0: crt-controller@xxx{
...
display-controller = <>;
ports {
... /* to the crtc0 connectors */
}
};
crtc1: crt-controller@xxx{
...
display-controller = <>;
ports {
... /* to the crtc1 connectors */
};
};

There are 2 DRM devices: one on crtc0, the other one on crtc1.
The DE device is isolated. But, to treat the planes, it must receive
information about the CRTCs. How?

> > Anyway, my solution is already used in the IMX Soc.
> > See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.
> 
> Pointing out random example in the tree doesn't make a compelling
> argument.

This is not a random example. There was a thread about the 'ports'
phandle in the DT definition of the IMX video subsystem, and what kind
of OF function should be used (see one of my previous mails). In the DRI
list, nobody objected about the phandle by itself.

> > > > > > > Panel functions? In the CRTC driver?
> > > > > > 
> > > > > > Yes, dumb panel.
> > > > > 
> > > > > What do you mean by that? Using a Parallel/RGB interface?
> > > > 
> > > > Sorry, I though this was a well-known name. The 'dump panel' was used
> > > > in the documentation of my previous ARM machine as the video frame sent
> > > > to the HDMI controller. 'video_frame' is OK for you?
> > > 
> > > If it's the frame sent to the encoder, then it would be the CRTC by
> > > DRM's nomenclature.
> > 
> > The CRTC is a software entity. The frame buffer is a hardware entity.
> 
> Why are you about framebuffer now, this is nowhere in that
> discussion. Any way, the framebuffer is also what is put in a plane,
> so there's a name collision here, and you'll probably want to change
> 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Maxime Ripard]

On Tue, Nov 08, 2016 at 03:37:52PM +0100, Jean-Francois Moine wrote:
> On Mon, 7 Nov 2016 23:37:41 +0100
> Maxime Ripard  wrote:
> 
> > Hi,
> > 
> > On Fri, Oct 28, 2016 at 07:34:20PM +0200, Jean-Francois Moine wrote:
> > > On Fri, 28 Oct 2016 00:03:16 +0200
> > > Maxime Ripard  wrote:
>   [snip]
> > > > > > We've been calling them bus and mod.
> > > > > 
> > > > > I can understand "bus" (which is better than "apb"), but why "mod"?
> > > > 
> > > > Allwinner has been calling the clocks that are supposed to generate
> > > > the external signals (depending on where you were looking) module or
> > > > mod clocks (which is also why we have mod in the clock
> > > > compatibles). The module 1 clocks being used for the audio and the
> > > > module 0 for the rest (SPI, MMC, NAND, display, etc.)
> > > 
> > > I did not find any 'module' in the H3 documentation.
> > > So, is it really a good name?
> > 
> > It's true that they use it less nowadays, but they still do,
> > ie. 
> > https://github.com/allwinner-zh/linux-3.4-sunxi/blob/master/drivers/clk/sunxi/clk-sun8iw7.c#L513
> 
> There is a 'mod' suffix, but it is used for the bus gates only, not for
> the main clocks.
> 
> > And we have to remain consistent anyway.
> 
> I don't see any consistency in the H3 DT:
> - the bus gates are named "ahb" and apb"

We've been inconsistent in the past, hence why it would be great to
have bus instead.

> - the (main) clocks are named "mmc", "usb0_phy" and "ir"
> There is no "bus" nor "mod".

Look into the other devices, for other SoCs. Pointing out that we've
been bad in the past doesn't mean that we can't improve.

> > > > > > > +
> > > > > > > +- resets: phandle to the reset of the device
> > > > > > > +
> > > > > > > +- ports: phandle's to the LCD ports
> > > > > > 
> > > > > > Please use the OF graph.
> > > > > 
> > > > > These ports are references to the graph of nodes. See
> > > > >   http://www.kernelhub.org/?msg=911825=2
> > > > 
> > > > In an OF-graph, your phandle to the LCD controller would be replaced
> > > > by an output endpoint.
> > > 
> > > This is the DE controller. There is no endpoint link at this level.
> > 
> > The display engine definitely has an endpoint: the TCON.
> 
> Not at all. The video chain is simply
>   CRTC (TCON) -> connector (HDMI/LCD/DAC/..)
> The DE is an ancillary device which handles the planes.

And what does it do exactly with those planes? It outputs it to the
TCON.

> > > The Device Engine just handles the planes of the LCDs, but, indeed,
> > > the LCDs must know about the DE and the DE must know about the LCDs.
> > > There are 2 ways to realize this knowledge in the DT:
> > > 1) either the DE has one or two phandle's to the LCDs,
> > > 2) or the LCDs have a phandle to the DE.
> > > 
> > > I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
> > > which is part of the video link (OF-graph LCD <-> connector).
> > > It would be possible to have phandles to the LCDs themselves, but this
> > > asks for more code.
> > > 
> > > The second way is also possible, but it also complexifies a bit the
> > > exchanges DE <-> LCD.
> > 
> > I'm still not sure how it would complexify anything, and why you can't
> > use the display graph to model the relation between the display engine
> > and the TCON (and why you want to use a generic property that refers
> > to the of-graph while it really isn't).
> 
> Complexification:
> 1- my solution:
>   At startup time, the DE device is the DRM device.

How do you deal with SoCs with multiple display engines then?

> It has to know the devices entering in the video chains.
>   The CRTCs (LCD/TCON) are found by
>   ports[i] -> parent
>   The connectors are found by
>   ports[i] -> endpoint -> remote_endpoint -> parent
> 2- with ports pointing to the LCDs:
>   The CRTCs (LCD/TCON) are simply
>   ports[i]
>   The connectors are found by
>   loop on all ports of ports[i]
>   ports[i][j] -> endpoint -> remote_endpoint -> parent
> 3- with a phandle to the DE in the LCDs:

What do you mean with LCD? Panels? Why would panels have a phandle to
the display engine?

>   The DE cannot be the DRM device because there is no information about
>   the video devices in the DT. Then, the DRM devices are the LCDs.
>   These LCDs must give their indices to the DE. So, the DE must implement
>   some callback function to accept a LCD definition, and there must be
>   a list of DEs in the driver to make the association DE <-> LCD[i]
>   Some more problem may be raised if a user wants to have the same frame
>   buffer on the 2 LCDs of a DE.

I have no idea what you're talking about, sorry.

> Anyway, my solution is already used in the IMX Soc.
> See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.

Pointing out random example in the tree doesn't make a compelling
argument.

> > > > > > > +void de2_disable_vblank(struct drm_device *drm, unsigned 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Jean-Francois Moine]

On Mon, 7 Nov 2016 23:37:41 +0100
Maxime Ripard  wrote:

> Hi,
> 
> On Fri, Oct 28, 2016 at 07:34:20PM +0200, Jean-Francois Moine wrote:
> > On Fri, 28 Oct 2016 00:03:16 +0200
> > Maxime Ripard  wrote:
[snip]
> > > > > We've been calling them bus and mod.
> > > > 
> > > > I can understand "bus" (which is better than "apb"), but why "mod"?
> > > 
> > > Allwinner has been calling the clocks that are supposed to generate
> > > the external signals (depending on where you were looking) module or
> > > mod clocks (which is also why we have mod in the clock
> > > compatibles). The module 1 clocks being used for the audio and the
> > > module 0 for the rest (SPI, MMC, NAND, display, etc.)
> > 
> > I did not find any 'module' in the H3 documentation.
> > So, is it really a good name?
> 
> It's true that they use it less nowadays, but they still do,
> ie. 
> https://github.com/allwinner-zh/linux-3.4-sunxi/blob/master/drivers/clk/sunxi/clk-sun8iw7.c#L513

There is a 'mod' suffix, but it is used for the bus gates only, not for
the main clocks.

> And we have to remain consistent anyway.

I don't see any consistency in the H3 DT:
- the bus gates are named "ahb" and apb"
- the (main) clocks are named "mmc", "usb0_phy" and "ir"
There is no "bus" nor "mod".

> > > > > > +
> > > > > > +- resets: phandle to the reset of the device
> > > > > > +
> > > > > > +- ports: phandle's to the LCD ports
> > > > > 
> > > > > Please use the OF graph.
> > > > 
> > > > These ports are references to the graph of nodes. See
> > > > http://www.kernelhub.org/?msg=911825=2
> > > 
> > > In an OF-graph, your phandle to the LCD controller would be replaced
> > > by an output endpoint.
> > 
> > This is the DE controller. There is no endpoint link at this level.
> 
> The display engine definitely has an endpoint: the TCON.

Not at all. The video chain is simply
CRTC (TCON) -> connector (HDMI/LCD/DAC/..)
The DE is an ancillary device which handles the planes.

> > The Device Engine just handles the planes of the LCDs, but, indeed,
> > the LCDs must know about the DE and the DE must know about the LCDs.
> > There are 2 ways to realize this knowledge in the DT:
> > 1) either the DE has one or two phandle's to the LCDs,
> > 2) or the LCDs have a phandle to the DE.
> > 
> > I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
> > which is part of the video link (OF-graph LCD <-> connector).
> > It would be possible to have phandles to the LCDs themselves, but this
> > asks for more code.
> > 
> > The second way is also possible, but it also complexifies a bit the
> > exchanges DE <-> LCD.
> 
> I'm still not sure how it would complexify anything, and why you can't
> use the display graph to model the relation between the display engine
> and the TCON (and why you want to use a generic property that refers
> to the of-graph while it really isn't).

Complexification:
1- my solution:
  At startup time, the DE device is the DRM device. It has to know the
  devices entering in the video chains.
  The CRTCs (LCD/TCON) are found by
ports[i] -> parent
  The connectors are found by
ports[i] -> endpoint -> remote_endpoint -> parent
2- with ports pointing to the LCDs:
  The CRTCs (LCD/TCON) are simply
ports[i]
  The connectors are found by
loop on all ports of ports[i]
ports[i][j] -> endpoint -> remote_endpoint -> parent
3- with a phandle to the DE in the LCDs:
  The DE cannot be the DRM device because there is no information about
  the video devices in the DT. Then, the DRM devices are the LCDs.
  These LCDs must give their indices to the DE. So, the DE must implement
  some callback function to accept a LCD definition, and there must be
  a list of DEs in the driver to make the association DE <-> LCD[i]
  Some more problem may be raised if a user wants to have the same frame
  buffer on the 2 LCDs of a DE.

Anyway, my solution is already used in the IMX Soc.
See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.

> > > > > > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > > > > > +{
> > > > > > +   struct priv *priv = drm->dev_private;
> > > > > > +   struct lcd *lcd = priv->lcds[crtc];
> > > > > > +
> > > > > > +   tcon_write(lcd->mmio, gint0,
> > > > > > +tcon_read(lcd->mmio, gint0) &
> > > > > > +   ~TCON_GINT0_TCON1_Vb_Int_En);
> > > > > > +}
> > > > > > +
> > > > > > +/* panel functions */
> > > > > 
> > > > > Panel functions? In the CRTC driver?
> > > > 
> > > > Yes, dumb panel.
> > > 
> > > What do you mean by that? Using a Parallel/RGB interface?
> > 
> > Sorry, I though this was a well-known name. The 'dump panel' was used
> > in the documentation of my previous ARM machine as the video frame sent
> > to the HDMI controller. 'video_frame' is OK for you?
> 
> If it's the frame sent to the encoder, then it 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Maxime Ripard]

Hi,

On Fri, Oct 28, 2016 at 07:34:20PM +0200, Jean-Francois Moine wrote:
> On Fri, 28 Oct 2016 00:03:16 +0200
> Maxime Ripard  wrote:
> 
> > On Tue, Oct 25, 2016 at 04:14:41PM +0200, Jean-Francois Moine wrote:
> > > > > +Display controller
> > > > > +==
> > > > > +
> > > > > +Required properties:
> > > > > +
> > > > > +- compatible: value should be one of the following
> > > > > + "allwinner,sun8i-a83t-display-engine"
> > > > > + "allwinner,sun8i-h3-display-engine"
> > > > > +
> > > > > +- clocks: must include clock specifiers corresponding to entries in 
> > > > > the
> > > > > + clock-names property.
> > > > > +
> > > > > +- clock-names: must contain
> > > > > + "gate": for DE activation
> > > > > + "clock": DE clock
> > > > 
> > > > We've been calling them bus and mod.
> > > 
> > > I can understand "bus" (which is better than "apb"), but why "mod"?
> > 
> > Allwinner has been calling the clocks that are supposed to generate
> > the external signals (depending on where you were looking) module or
> > mod clocks (which is also why we have mod in the clock
> > compatibles). The module 1 clocks being used for the audio and the
> > module 0 for the rest (SPI, MMC, NAND, display, etc.)
> 
> I did not find any 'module' in the H3 documentation.
> So, is it really a good name?

It's true that they use it less nowadays, but they still do,
ie. 
https://github.com/allwinner-zh/linux-3.4-sunxi/blob/master/drivers/clk/sunxi/clk-sun8iw7.c#L513

And we have to remain consistent anyway.

> > > > > +
> > > > > +- resets: phandle to the reset of the device
> > > > > +
> > > > > +- ports: phandle's to the LCD ports
> > > > 
> > > > Please use the OF graph.
> > > 
> > > These ports are references to the graph of nodes. See
> > >   http://www.kernelhub.org/?msg=911825=2
> > 
> > In an OF-graph, your phandle to the LCD controller would be replaced
> > by an output endpoint.
> 
> This is the DE controller. There is no endpoint link at this level.

The display engine definitely has an endpoint: the TCON.

> The Device Engine just handles the planes of the LCDs, but, indeed,
> the LCDs must know about the DE and the DE must know about the LCDs.
> There are 2 ways to realize this knowledge in the DT:
> 1) either the DE has one or two phandle's to the LCDs,
> 2) or the LCDs have a phandle to the DE.
> 
> I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
> which is part of the video link (OF-graph LCD <-> connector).
> It would be possible to have phandles to the LCDs themselves, but this
> asks for more code.
> 
> The second way is also possible, but it also complexifies a bit the
> exchanges DE <-> LCD.

I'm still not sure how it would complexify anything, and why you can't
use the display graph to model the relation between the display engine
and the TCON (and why you want to use a generic property that refers
to the of-graph while it really isn't).

> > > > > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > > > > +{
> > > > > + struct priv *priv = drm->dev_private;
> > > > > + struct lcd *lcd = priv->lcds[crtc];
> > > > > +
> > > > > + tcon_write(lcd->mmio, gint0,
> > > > > +  tcon_read(lcd->mmio, gint0) &
> > > > > + ~TCON_GINT0_TCON1_Vb_Int_En);
> > > > > +}
> > > > > +
> > > > > +/* panel functions */
> > > > 
> > > > Panel functions? In the CRTC driver?
> > > 
> > > Yes, dumb panel.
> > 
> > What do you mean by that? Using a Parallel/RGB interface?
> 
> Sorry, I though this was a well-known name. The 'dump panel' was used
> in the documentation of my previous ARM machine as the video frame sent
> to the HDMI controller. 'video_frame' is OK for you?

If it's the frame sent to the encoder, then it would be the CRTC by
DRM's nomenclature.

> > > > > +static const struct {
> > > > > + char chan;
> > > > > + char layer;
> > > > > + char pipe;
> > > > > +} plane2layer[DE2_N_PLANES] = {
> > > > > + [DE2_PRIMARY_PLANE] =   {0, 0, 0},
> > > > > + [DE2_CURSOR_PLANE] ={1, 0, 1},
> > > > > + [DE2_VI_PLANE] ={0, 1, 0},
> > > > > +};
> > > > 
> > > > Comments?
> > > 
> > > This
> > >   primary plane is channel 0 (VI), layer 0, pipe 0
> > >   cursor plane is channel 1 (UI), layer 0, pipe 1
> > >   overlay plane is channel 0 (VI), layer 1, pipe 0
> > > or the full explanation:
> > > Constraints:
> > >   The VI channels can do RGB or YUV, while UI channels can do RGB
> > >   only.
> > >   The LCD 0 has 1 VI channel and 4 UI channels, while
> > >   LCD 1 has only 1 VI channel and 1 UI channel.
> > >   The cursor must go to a channel bigger than the primary channel,
> > >   otherwise it is not transparent.
> > > First try:
> > >   Letting the primary plane (usually RGB) in the 2nd channel (UI),
> > >   as this is done in the legacy driver, asks for the cursor to go
> > >   to the 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Rob Herring]

On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> engine, DE2.
> This patch adds a DRM video driver for this device.
> 
> Signed-off-by: Jean-Francois Moine 
> ---
>  .../bindings/display/sunxi/sunxi-de2.txt   |  83 +++
>  drivers/gpu/drm/Kconfig|   2 +
>  drivers/gpu/drm/Makefile   |   1 +
>  drivers/gpu/drm/sunxi/Kconfig  |  21 +
>  drivers/gpu/drm/sunxi/Makefile |   7 +
>  drivers/gpu/drm/sunxi/de2_crtc.c   | 475 +
>  drivers/gpu/drm/sunxi/de2_crtc.h   |  63 +++
>  drivers/gpu/drm/sunxi/de2_de.c | 591 
> +
>  drivers/gpu/drm/sunxi/de2_drm.h|  47 ++
>  drivers/gpu/drm/sunxi/de2_drv.c| 378 +
>  drivers/gpu/drm/sunxi/de2_plane.c  | 119 +
>  11 files changed, 1787 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
>  create mode 100644 drivers/gpu/drm/sunxi/Kconfig
>  create mode 100644 drivers/gpu/drm/sunxi/Makefile
>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
>  create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
>  create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt 
> b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> new file mode 100644
> index 000..f9cd67a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt

> + hdmi: hdmi@01ee {
> + ...
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port {
> + type = "video";

This is proposed, but not an accepted property. Please drop.

> + hdmi_ep: endpoint {
> + remote-endpoint = <_ep>;
> + };
> + };
> + };

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Rob Herring]

On Tue, Oct 25, 2016 at 04:14:41PM +0200, Jean-Francois Moine wrote:
> On Mon, 24 Oct 2016 16:04:19 +0200
> Maxime Ripard  wrote:
> 
> > Hi,
> 
> Hi Maxime,
> 
> > On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > > Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> > > engine, DE2.
> > > This patch adds a DRM video driver for this device.
> > > 
> > > Signed-off-by: Jean-Francois Moine 
> > 
> > Output from checkpatch:
> > total: 0 errors, 20 warnings, 83 checks, 1799 lines checked
> > 
> > > ---
> > >  .../bindings/display/sunxi/sunxi-de2.txt   |  83 +++
> > >  drivers/gpu/drm/Kconfig|   2 +
> > >  drivers/gpu/drm/Makefile   |   1 +
> > >  drivers/gpu/drm/sunxi/Kconfig  |  21 +
> > >  drivers/gpu/drm/sunxi/Makefile |   7 +
> > >  drivers/gpu/drm/sunxi/de2_crtc.c   | 475 
> > > +
> > >  drivers/gpu/drm/sunxi/de2_crtc.h   |  63 +++
> > >  drivers/gpu/drm/sunxi/de2_de.c | 591 
> > > +
> > >  drivers/gpu/drm/sunxi/de2_drm.h|  47 ++
> > >  drivers/gpu/drm/sunxi/de2_drv.c| 378 +
> > >  drivers/gpu/drm/sunxi/de2_plane.c  | 119 +
> > >  11 files changed, 1787 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > >  create mode 100644 drivers/gpu/drm/sunxi/Kconfig
> > >  create mode 100644 drivers/gpu/drm/sunxi/Makefile
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
> > >  create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt 
> > > b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > > new file mode 100644
> > > index 000..f9cd67a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt

[...]

> > > +
> > > +- resets: phandle to the reset of the device
> > > +
> > > +- ports: phandle's to the LCD ports
> > 
> > Please use the OF graph.
> 
> These ports are references to the graph of nodes. See
>   http://www.kernelhub.org/?msg=911825=2

I think what Maxime means is describe the DE to LCD connection with OF 
graph, not just a phandle.

Rob

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Jean-Francois Moine]

On Fri, 28 Oct 2016 00:03:16 +0200
Maxime Ripard  wrote:

> On Tue, Oct 25, 2016 at 04:14:41PM +0200, Jean-Francois Moine wrote:
> > > > +Display controller
> > > > +==
> > > > +
> > > > +Required properties:
> > > > +
> > > > +- compatible: value should be one of the following
> > > > +   "allwinner,sun8i-a83t-display-engine"
> > > > +   "allwinner,sun8i-h3-display-engine"
> > > > +
> > > > +- clocks: must include clock specifiers corresponding to entries in the
> > > > +   clock-names property.
> > > > +
> > > > +- clock-names: must contain
> > > > +   "gate": for DE activation
> > > > +   "clock": DE clock
> > > 
> > > We've been calling them bus and mod.
> > 
> > I can understand "bus" (which is better than "apb"), but why "mod"?
> 
> Allwinner has been calling the clocks that are supposed to generate
> the external signals (depending on where you were looking) module or
> mod clocks (which is also why we have mod in the clock
> compatibles). The module 1 clocks being used for the audio and the
> module 0 for the rest (SPI, MMC, NAND, display, etc.)

I did not find any 'module' in the H3 documentation.
So, is it really a good name?

> > > > +
> > > > +- resets: phandle to the reset of the device
> > > > +
> > > > +- ports: phandle's to the LCD ports
> > > 
> > > Please use the OF graph.
> > 
> > These ports are references to the graph of nodes. See
> > http://www.kernelhub.org/?msg=911825=2
> 
> In an OF-graph, your phandle to the LCD controller would be replaced
> by an output endpoint.

This is the DE controller. There is no endpoint link at this level.
The Device Engine just handles the planes of the LCDs, but, indeed,
the LCDs must know about the DE and the DE must know about the LCDs.
There are 2 ways to realize this knowledge in the DT:
1) either the DE has one or two phandle's to the LCDs,
2) or the LCDs have a phandle to the DE.

I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
which is part of the video link (OF-graph LCD <-> connector).
It would be possible to have phandles to the LCDs themselves, but this
asks for more code.

The second way is also possible, but it also complexifies a bit the
exchanges DE <-> LCD.

> > [snip]
> > > > +struct tcon {
> > > > +   u32 gctl;
> > > > +#defineTCON_GCTL_TCON_En BIT(31)
[snip]
> > > > +   u32 fill_ctl;   /* 0x300 */
> > > > +   u32 fill_start0;
> > > > +   u32 fill_end0;
> > > > +   u32 fill_data0;
> > > > +};
> > > 
> > > Please use defines instead of the structures.
> > 
> > I think that structures are more readable.
> 
> That's not really the point. No one in the kernel uses it (and even
> you use defines for registers offset in some places of that
> patch). And then you have André arguments.

I am not convinced, but I'll do as you said.

> > > > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > > > +{
> > > > +   struct priv *priv = drm->dev_private;
> > > > +   struct lcd *lcd = priv->lcds[crtc];
> > > > +
> > > > +   tcon_write(lcd->mmio, gint0,
> > > > +tcon_read(lcd->mmio, gint0) &
> > > > +   ~TCON_GINT0_TCON1_Vb_Int_En);
> > > > +}
> > > > +
> > > > +/* panel functions */
> > > 
> > > Panel functions? In the CRTC driver?
> > 
> > Yes, dumb panel.
> 
> What do you mean by that? Using a Parallel/RGB interface?

Sorry, I though this was a well-known name. The 'dump panel' was used
in the documentation of my previous ARM machine as the video frame sent
to the HDMI controller. 'video_frame' is OK for you?

[snip]
> > > > +   ret = clk_prepare_enable(lcd->clk);
> > > > +   if (ret)
> > > > +   goto err2;
> > > 
> > > Is there any reason not to do that in the enable / disable? Leaving
> > > clocks running while the device has no guarantee that it's going to be
> > > used seems like a waste of resources.
> > 
> > If the machine does not need video (network server, router..), it is simpler
> > to prevent the video driver to be loaded (DT, module black list...).
> 
> You might not have control on any of it, or you might just have no
> monitor attached for example. Recompiling the kernel or updating the
> DT when you want to plug an HDMI monitor seems like a poor UX :)

OK, I will check if this works.

> > > > +static const struct {
> > > > +   char chan;
> > > > +   char layer;
> > > > +   char pipe;
> > > > +} plane2layer[DE2_N_PLANES] = {
> > > > +   [DE2_PRIMARY_PLANE] =   {0, 0, 0},
> > > > +   [DE2_CURSOR_PLANE] ={1, 0, 1},
> > > > +   [DE2_VI_PLANE] ={0, 1, 0},
> > > > +};
> > > 
> > > Comments?
> > 
> > This
> > primary plane is channel 0 (VI), layer 0, pipe 0
> > cursor plane is channel 1 (UI), layer 0, pipe 1
> > overlay plane is channel 0 (VI), layer 1, pipe 0
> > or the 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Maxime Ripard]

On Tue, Oct 25, 2016 at 04:14:41PM +0200, Jean-Francois Moine wrote:
> > > +Display controller
> > > +==
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: value should be one of the following
> > > + "allwinner,sun8i-a83t-display-engine"
> > > + "allwinner,sun8i-h3-display-engine"
> > > +
> > > +- clocks: must include clock specifiers corresponding to entries in the
> > > + clock-names property.
> > > +
> > > +- clock-names: must contain
> > > + "gate": for DE activation
> > > + "clock": DE clock
> > 
> > We've been calling them bus and mod.
> 
> I can understand "bus" (which is better than "apb"), but why "mod"?

Allwinner has been calling the clocks that are supposed to generate
the external signals (depending on where you were looking) module or
mod clocks (which is also why we have mod in the clock
compatibles). The module 1 clocks being used for the audio and the
module 0 for the rest (SPI, MMC, NAND, display, etc.)

> > > +
> > > +- resets: phandle to the reset of the device
> > > +
> > > +- ports: phandle's to the LCD ports
> > 
> > Please use the OF graph.
> 
> These ports are references to the graph of nodes. See
>   http://www.kernelhub.org/?msg=911825=2

In an OF-graph, your phandle to the LCD controller would be replaced
by an output endpoint.

> 
>   [snip]
> > > +struct tcon {
> > > + u32 gctl;
> > > +#define  TCON_GCTL_TCON_En BIT(31)
> > > + u32 gint0;
> > > +#define  TCON_GINT0_TCON1_Vb_Int_En BIT(30)
> > > +#define  TCON_GINT0_TCON1_Vb_Int_Flag BIT(14)
> > > + u32 gint1;
> > > + u32 dum0[13];
> > > + u32 tcon0_ctl;  /* 0x40 */
> > > +#define  TCON0_CTL_TCON_En BIT(31)
> > > + u32 dum1[19];
> > > + u32 tcon1_ctl;  /* 0x90 */
> > > +#define  TCON1_CTL_TCON_En BIT(31)
> > > +#define  TCON1_CTL_Interlace_En BIT(20)
> > > +#define  TCON1_CTL_Start_Delay_SHIFT 4
> > > +#define  TCON1_CTL_Start_Delay_MASK GENMASK(8, 4)
> > > + u32 basic0; /* XI/YI */
> > > + u32 basic1; /* LS_XO/LS_YO */
> > > + u32 basic2; /* XO/YO */
> > > + u32 basic3; /* HT/HBP */
> > > + u32 basic4; /* VT/VBP */
> > > + u32 basic5; /* HSPW/VSPW */
> > > + u32 dum2;
> > > + u32 ps_sync;/* 0xb0 */
> > > + u32 dum3[15];
> > > + u32 io_pol; /* 0xf0 */
> > > +#define  TCON1_IO_POL_IO0_inv BIT(24)
> > > +#define  TCON1_IO_POL_IO1_inv BIT(25)
> > > +#define  TCON1_IO_POL_IO2_inv BIT(26)
> > > + u32 io_tri;
> > > + u32 dum4[2];
> > > +
> > > + u32 ceu_ctl;/* 0x100 */
> > > +#define TCON_CEU_CTL_ceu_en BIT(31)
> > > + u32 dum5[3];
> > > + u32 ceu_rr;
> > > + u32 ceu_rg;
> > > + u32 ceu_rb;
> > > + u32 ceu_rc;
> > > + u32 ceu_gr;
> > > + u32 ceu_gg;
> > > + u32 ceu_gb;
> > > + u32 ceu_gc;
> > > + u32 ceu_br;
> > > + u32 ceu_bg;
> > > + u32 ceu_bb;
> > > + u32 ceu_bc;
> > > + u32 ceu_rv;
> > > + u32 ceu_gv;
> > > + u32 ceu_bv;
> > > + u32 dum6[45];
> > > +
> > > + u32 mux_ctl;/* 0x200 */
> > > + u32 dum7[63];
> > > +
> > > + u32 fill_ctl;   /* 0x300 */
> > > + u32 fill_start0;
> > > + u32 fill_end0;
> > > + u32 fill_data0;
> > > +};
> > 
> > Please use defines instead of the structures.
> 
> I think that structures are more readable.

That's not really the point. No one in the kernel uses it (and even
you use defines for registers offset in some places of that
patch). And then you have André arguments.

> > > +void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
> > > +{
> > > + struct priv *priv = drm->dev_private;
> > > + struct lcd *lcd = priv->lcds[crtc];
> > > +
> > > + tcon_write(lcd->mmio, gint0,
> > > +  tcon_read(lcd->mmio, gint0) &
> > > + ~TCON_GINT0_TCON1_Vb_Int_En);
> > > +}
> > > +
> > > +/* panel functions */
> > 
> > Panel functions? In the CRTC driver?
> 
> Yes, dumb panel.

What do you mean by that? Using a Parallel/RGB interface?

> 
> > > +static void de2_set_frame_timings(struct lcd *lcd)
> > > +{
> > > + struct drm_crtc *crtc = >crtc;
> > > + const struct drm_display_mode *mode = >mode;
> > > + int interlace = mode->flags & DRM_MODE_FLAG_INTERLACE ? 2 : 1;
> > > + int start_delay;
> > > + u32 data;
> > > +
> > > + data = XY(mode->hdisplay - 1, mode->vdisplay / interlace - 1);
> > > + tcon_write(lcd->mmio, basic0, data);
> > > + tcon_write(lcd->mmio, basic1, data);
> > > + tcon_write(lcd->mmio, basic2, data);
> > > + tcon_write(lcd->mmio, basic3,
> > > + XY(mode->htotal - 1,
> > > + mode->htotal - mode->hsync_start - 1));
> > > + tcon_write(lcd->mmio, basic4,
> > > + XY(mode->vtotal * (3 - interlace),
> > > + 

Re: [linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[André Przywara]

On 25/10/16 15:14, Jean-Francois Moine wrote:
> On Mon, 24 Oct 2016 16:04:19 +0200
> Maxime Ripard  wrote:
> 
>> Hi,
> 
> Hi Maxime,
> 
>> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
>>> Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
>>> engine, DE2.
>>> This patch adds a DRM video driver for this device.
>>>
>>> Signed-off-by: Jean-Francois Moine 
>>
>> Output from checkpatch:
>> total: 0 errors, 20 warnings, 83 checks, 1799 lines checked
>>
>>> ---
>>>  .../bindings/display/sunxi/sunxi-de2.txt   |  83 +++
>>>  drivers/gpu/drm/Kconfig|   2 +
>>>  drivers/gpu/drm/Makefile   |   1 +
>>>  drivers/gpu/drm/sunxi/Kconfig  |  21 +
>>>  drivers/gpu/drm/sunxi/Makefile |   7 +
>>>  drivers/gpu/drm/sunxi/de2_crtc.c   | 475 +
>>>  drivers/gpu/drm/sunxi/de2_crtc.h   |  63 +++
>>>  drivers/gpu/drm/sunxi/de2_de.c | 591 
>>> +
>>>  drivers/gpu/drm/sunxi/de2_drm.h|  47 ++
>>>  drivers/gpu/drm/sunxi/de2_drv.c| 378 +
>>>  drivers/gpu/drm/sunxi/de2_plane.c  | 119 +
>>>  11 files changed, 1787 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
>>>  create mode 100644 drivers/gpu/drm/sunxi/Kconfig
>>>  create mode 100644 drivers/gpu/drm/sunxi/Makefile
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
>>>  create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt 
>>> b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
>>> new file mode 100644
>>> index 000..f9cd67a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
>>> @@ -0,0 +1,83 @@
>>> +Allwinner sunxi Display Engine 2 subsystem
>>> +==
>>> +
>>> +The sunxi DE2 subsystem contains a display controller (DE2),
>>
>> sunxi is a made up name, and doesn't really mean anything. You can
>> call it either sun8i (because it was introduced in that family).
> 
> OK.
> 
>>> +one or two LCD controllers (TCON) and their external interfaces.
>>> +
>>> +Display controller
>>> +==
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: value should be one of the following
>>> +   "allwinner,sun8i-a83t-display-engine"
>>> +   "allwinner,sun8i-h3-display-engine"
>>> +
>>> +- clocks: must include clock specifiers corresponding to entries in the
>>> +   clock-names property.
>>> +
>>> +- clock-names: must contain
>>> +   "gate": for DE activation
>>> +   "clock": DE clock
>>
>> We've been calling them bus and mod.
> 
> I can understand "bus" (which is better than "apb"), but why "mod"?
> 
>>> +
>>> +- resets: phandle to the reset of the device
>>> +
>>> +- ports: phandle's to the LCD ports
>>
>> Please use the OF graph.
> 
> These ports are references to the graph of nodes. See
>   http://www.kernelhub.org/?msg=911825=2
> 
>   [snip]
>>> diff --git a/drivers/gpu/drm/sunxi/de2_crtc.c 
>>> b/drivers/gpu/drm/sunxi/de2_crtc.c
>>> new file mode 100644
>>> index 000..dae0fab
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/sunxi/de2_crtc.c
>>> @@ -0,0 +1,475 @@
>>> +/*
>>> + * Allwinner DRM driver - DE2 CRTC
>>> + *
>>> + * Copyright (C) 2016 Jean-Francois Moine 
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation; either version 2 of
>>> + * the License, or (at your option) any later version.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "de2_drm.h"
>>> +#include "de2_crtc.h"
>>> +
>>> +/* I/O map */
>>> +
>>> +struct tcon {
>>> +   u32 gctl;
>>> +#defineTCON_GCTL_TCON_En BIT(31)
>>> +   u32 gint0;
>>> +#defineTCON_GINT0_TCON1_Vb_Int_En BIT(30)
>>> +#defineTCON_GINT0_TCON1_Vb_Int_Flag BIT(14)
>>> +   u32 gint1;
>>> +   u32 dum0[13];
>>> +   u32 tcon0_ctl;  /* 0x40 */
>>> +#defineTCON0_CTL_TCON_En BIT(31)
>>> +   u32 dum1[19];
>>> +   u32 tcon1_ctl;  /* 0x90 */
>>> +#defineTCON1_CTL_TCON_En BIT(31)
>>> +#defineTCON1_CTL_Interlace_En BIT(20)
>>> +#defineTCON1_CTL_Start_Delay_SHIFT 4
>>> +#defineTCON1_CTL_Start_Delay_MASK GENMASK(8, 4)
>>> +   u32 basic0; 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Jean-Francois Moine]

On Tue, 25 Oct 2016 08:44:22 +0200
Daniel Vetter  wrote:

> > +   /* start the subdevices */
> > +   ret = component_bind_all(dev, drm);
> > +   if (ret < 0)
> > +   goto out2;
> > +
> > +   ret = drm_dev_register(drm, 0);
> 
> This needs to be the very last step in your driver load sequence.
> Kerneldoc explains why. Similar, but inverted for unloading:
> drm_dev_unregister is the very first thing you must call.

Thanks, and also for embedding the drm device.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Jean-Francois Moine]

On Mon, 24 Oct 2016 16:04:19 +0200
Maxime Ripard  wrote:

> Hi,

Hi Maxime,

> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> > engine, DE2.
> > This patch adds a DRM video driver for this device.
> > 
> > Signed-off-by: Jean-Francois Moine 
> 
> Output from checkpatch:
> total: 0 errors, 20 warnings, 83 checks, 1799 lines checked
> 
> > ---
> >  .../bindings/display/sunxi/sunxi-de2.txt   |  83 +++
> >  drivers/gpu/drm/Kconfig|   2 +
> >  drivers/gpu/drm/Makefile   |   1 +
> >  drivers/gpu/drm/sunxi/Kconfig  |  21 +
> >  drivers/gpu/drm/sunxi/Makefile |   7 +
> >  drivers/gpu/drm/sunxi/de2_crtc.c   | 475 +
> >  drivers/gpu/drm/sunxi/de2_crtc.h   |  63 +++
> >  drivers/gpu/drm/sunxi/de2_de.c | 591 
> > +
> >  drivers/gpu/drm/sunxi/de2_drm.h|  47 ++
> >  drivers/gpu/drm/sunxi/de2_drv.c| 378 +
> >  drivers/gpu/drm/sunxi/de2_plane.c  | 119 +
> >  11 files changed, 1787 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> >  create mode 100644 drivers/gpu/drm/sunxi/Kconfig
> >  create mode 100644 drivers/gpu/drm/sunxi/Makefile
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
> >  create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> > 
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt 
> > b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > new file mode 100644
> > index 000..f9cd67a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> > @@ -0,0 +1,83 @@
> > +Allwinner sunxi Display Engine 2 subsystem
> > +==
> > +
> > +The sunxi DE2 subsystem contains a display controller (DE2),
> 
> sunxi is a made up name, and doesn't really mean anything. You can
> call it either sun8i (because it was introduced in that family).

OK.

> > +one or two LCD controllers (TCON) and their external interfaces.
> > +
> > +Display controller
> > +==
> > +
> > +Required properties:
> > +
> > +- compatible: value should be one of the following
> > +   "allwinner,sun8i-a83t-display-engine"
> > +   "allwinner,sun8i-h3-display-engine"
> > +
> > +- clocks: must include clock specifiers corresponding to entries in the
> > +   clock-names property.
> > +
> > +- clock-names: must contain
> > +   "gate": for DE activation
> > +   "clock": DE clock
> 
> We've been calling them bus and mod.

I can understand "bus" (which is better than "apb"), but why "mod"?

> > +
> > +- resets: phandle to the reset of the device
> > +
> > +- ports: phandle's to the LCD ports
> 
> Please use the OF graph.

These ports are references to the graph of nodes. See
http://www.kernelhub.org/?msg=911825=2

[snip]
> > diff --git a/drivers/gpu/drm/sunxi/de2_crtc.c 
> > b/drivers/gpu/drm/sunxi/de2_crtc.c
> > new file mode 100644
> > index 000..dae0fab
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sunxi/de2_crtc.c
> > @@ -0,0 +1,475 @@
> > +/*
> > + * Allwinner DRM driver - DE2 CRTC
> > + *
> > + * Copyright (C) 2016 Jean-Francois Moine 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "de2_drm.h"
> > +#include "de2_crtc.h"
> > +
> > +/* I/O map */
> > +
> > +struct tcon {
> > +   u32 gctl;
> > +#defineTCON_GCTL_TCON_En BIT(31)
> > +   u32 gint0;
> > +#defineTCON_GINT0_TCON1_Vb_Int_En BIT(30)
> > +#defineTCON_GINT0_TCON1_Vb_Int_Flag BIT(14)
> > +   u32 gint1;
> > +   u32 dum0[13];
> > +   u32 tcon0_ctl;  /* 0x40 */
> > +#defineTCON0_CTL_TCON_En BIT(31)
> > +   u32 dum1[19];
> > +   u32 tcon1_ctl;  /* 0x90 */
> > +#defineTCON1_CTL_TCON_En BIT(31)
> > +#defineTCON1_CTL_Interlace_En BIT(20)
> > +#defineTCON1_CTL_Start_Delay_SHIFT 4
> > +#defineTCON1_CTL_Start_Delay_MASK GENMASK(8, 4)
> > +   u32 basic0; /* XI/YI */
> > +   u32 basic1; /* LS_XO/LS_YO */
> > +   u32 

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Daniel Vetter]

On Tue, Oct 25, 2016 at 08:44:22AM +0200, Daniel Vetter wrote:
> On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> > +static int de2_drm_bind(struct device *dev)
> > +{
> > +   struct drm_device *drm;
> > +   struct priv *priv;
> > +   int ret;
> > +
> > +   drm = drm_dev_alloc(_drm_driver, dev);

Oh and you might want to look into drm_dev_init, allows you to use
subclassing instead of pointer chasing for your driver-private data.
-Daniel

> > +   if (!drm)
> > +   return -ENOMEM;
> > +
> > +   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +   if (!priv) {
> > +   dev_err(dev, "failed to allocate private area\n");
> > +   ret = -ENOMEM;
> > +   goto out1;
> > +   }
> > +
> > +   dev_set_drvdata(dev, drm);
> > +   drm->dev_private = priv;
> > +
> > +   drm_mode_config_init(drm);
> > +   drm->mode_config.min_width = 32;/* needed for cursor */
> > +   drm->mode_config.min_height = 32;
> > +   drm->mode_config.max_width = 1920;
> > +   drm->mode_config.max_height = 1080;
> > +   drm->mode_config.funcs = _mode_config_funcs;
> > +
> > +   drm->irq_enabled = true;
> > +
> > +   /* initialize the display engine */
> > +   priv->soc_type = (int) of_match_device(de2_drm_of_match, dev)->data;
> > +   ret = de2_de_init(priv, dev);
> > +   if (ret)
> > +   goto out2;
> > +
> > +   /* start the subdevices */
> > +   ret = component_bind_all(dev, drm);
> > +   if (ret < 0)
> > +   goto out2;
> > +
> > +   ret = drm_dev_register(drm, 0);
> 
> This needs to be the very last step in your driver load sequence.
> Kerneldoc explains why. Similar, but inverted for unloading:
> drm_dev_unregister is the very first thing you must call.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Daniel Vetter]

On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> +static int de2_drm_bind(struct device *dev)
> +{
> + struct drm_device *drm;
> + struct priv *priv;
> + int ret;
> +
> + drm = drm_dev_alloc(_drm_driver, dev);
> + if (!drm)
> + return -ENOMEM;
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + dev_err(dev, "failed to allocate private area\n");
> + ret = -ENOMEM;
> + goto out1;
> + }
> +
> + dev_set_drvdata(dev, drm);
> + drm->dev_private = priv;
> +
> + drm_mode_config_init(drm);
> + drm->mode_config.min_width = 32;/* needed for cursor */
> + drm->mode_config.min_height = 32;
> + drm->mode_config.max_width = 1920;
> + drm->mode_config.max_height = 1080;
> + drm->mode_config.funcs = _mode_config_funcs;
> +
> + drm->irq_enabled = true;
> +
> + /* initialize the display engine */
> + priv->soc_type = (int) of_match_device(de2_drm_of_match, dev)->data;
> + ret = de2_de_init(priv, dev);
> + if (ret)
> + goto out2;
> +
> + /* start the subdevices */
> + ret = component_bind_all(dev, drm);
> + if (ret < 0)
> + goto out2;
> +
> + ret = drm_dev_register(drm, 0);

This needs to be the very last step in your driver load sequence.
Kerneldoc explains why. Similar, but inverted for unloading:
drm_dev_unregister is the very first thing you must call.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

[Maxime Ripard]

Hi,

On Fri, Oct 21, 2016 at 09:26:18AM +0200, Jean-Francois Moine wrote:
> Allwinner's recent SoCs, as A64, A83T and H3, contain a new display
> engine, DE2.
> This patch adds a DRM video driver for this device.
> 
> Signed-off-by: Jean-Francois Moine 

Output from checkpatch:
total: 0 errors, 20 warnings, 83 checks, 1799 lines checked

> ---
>  .../bindings/display/sunxi/sunxi-de2.txt   |  83 +++
>  drivers/gpu/drm/Kconfig|   2 +
>  drivers/gpu/drm/Makefile   |   1 +
>  drivers/gpu/drm/sunxi/Kconfig  |  21 +
>  drivers/gpu/drm/sunxi/Makefile |   7 +
>  drivers/gpu/drm/sunxi/de2_crtc.c   | 475 +
>  drivers/gpu/drm/sunxi/de2_crtc.h   |  63 +++
>  drivers/gpu/drm/sunxi/de2_de.c | 591 
> +
>  drivers/gpu/drm/sunxi/de2_drm.h|  47 ++
>  drivers/gpu/drm/sunxi/de2_drv.c| 378 +
>  drivers/gpu/drm/sunxi/de2_plane.c  | 119 +
>  11 files changed, 1787 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
>  create mode 100644 drivers/gpu/drm/sunxi/Kconfig
>  create mode 100644 drivers/gpu/drm/sunxi/Makefile
>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_crtc.h
>  create mode 100644 drivers/gpu/drm/sunxi/de2_de.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_drm.h
>  create mode 100644 drivers/gpu/drm/sunxi/de2_drv.c
>  create mode 100644 drivers/gpu/drm/sunxi/de2_plane.c
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt 
> b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> new file mode 100644
> index 000..f9cd67a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/sunxi-de2.txt
> @@ -0,0 +1,83 @@
> +Allwinner sunxi Display Engine 2 subsystem
> +==
> +
> +The sunxi DE2 subsystem contains a display controller (DE2),

sunxi is a made up name, and doesn't really mean anything. You can
call it either sun8i (because it was introduced in that family).

> +one or two LCD controllers (TCON) and their external interfaces.
> +
> +Display controller
> +==
> +
> +Required properties:
> +
> +- compatible: value should be one of the following
> + "allwinner,sun8i-a83t-display-engine"
> + "allwinner,sun8i-h3-display-engine"
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> + clock-names property.
> +
> +- clock-names: must contain
> + "gate": for DE activation
> + "clock": DE clock

We've been calling them bus and mod.

> +
> +- resets: phandle to the reset of the device
> +
> +- ports: phandle's to the LCD ports

Please use the OF graph.

> +
> +LCD controller
> +==
> +
> +Required properties:
> +
> +- compatible: value should be one of the following
> + "allwinner,sun8i-a83t-lcd"
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> + clock-names property.
> +
> +- clock-names: must contain
> + "gate": for LCD activation
> + "clock": pixel clock
> +
> +- resets: phandle to the reset of the device
> +
> +- port: port node with endpoint definitions as defined in
> + Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +
> + de: de-controller@0100 {
> + compatible = "allwinner,sun8i-h3-display-engine";
> + ...
> + clocks = <& CLK_BUS_DE>, < CLK_DE>;
> + clock-names = "gate", "clock";
> + resets = < RST_BUS_DE>;
> + ports = <_p>;
> + };
> +
> + lcd0: lcd-controller@01c0c000 {
> + compatible = "allwinner,sun8i-a83t-lcd";
> + ...
> + clocks = < CLK_BUS_TCON0>, < CLK_TCON0>;
> + clock-names = "gate", "clock";
> + resets = < RST_BUS_TCON0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + lcd0_p: port {
> + lcd0_ep: endpoint {
> + remote-endpoint = <_ep>;
> + };
> + };
> + };
> +
> + hdmi: hdmi@01ee {
> + ...
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port {
> + type = "video";
> + hdmi_ep: endpoint {
> + remote-endpoint = <_ep>;
> + };
> + };
> + };

> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 483059a..afd576f 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -187,6 +187,8 @@ source "drivers/gpu/drm/shmobile/Kconfig"
>  
>  source