Re: [U-Boot] [PATCH 37/51] drivers: Add ihs_video_out driver

Hi Simon,

On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass sjg@chromium.org wrote:

Hi Mario,

On 14 July 2017 at 05:55, Mario Six mario.six@gdsys.cc wrote:

...

Add a driver for IHS OSDs on IHS FPGAs.

Signed-off-by: Mario Six mario.six@gdsys.cc

drivers/misc/Kconfig | 6 ++ drivers/misc/Makefile | 1 + drivers/misc/ihs_video_out.c | 243 +++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/ihs_video_out.h | 146 ++++++++++++++++++++++++++ 5 files changed, 397 insertions(+) create mode 100644 drivers/misc/ihs_video_out.c create mode 100644 include/ihs_video_out.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index c53f9f195e..c0e4b921b4 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -209,4 +209,10 @@ config IHS_AXI help Support for IHS AXI bus.

+config IHS_VIDEO_OUT

•   bool "Enable IHS video out driver"
  

•   depends on MISC
  

•   help
  

•     Support for IHS video out.
  

endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index a6c71acedd..eab539b739 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -54,3 +54,4 @@ obj-$(CONFIG_QFW) += qfw.o obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o obj-$(CONFIG_IHS_FPGA) += ihs_fpga.o gdsys_soc.o obj-$(CONFIG_IHS_AXI) += ihs_axi.o +obj-$(CONFIG_IHS_VIDEO_OUT) += ihs_video_out.o diff --git a/drivers/misc/ihs_video_out.c b/drivers/misc/ihs_video_out.c new file mode 100644 index 0000000000..a7e855f054 --- /dev/null +++ b/drivers/misc/ihs_video_out.c @@ -0,0 +1,243 @@ +/*

• • (C) Copyright 2017
• • Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
• • based on the gdsys osd driver, which is
• • (C) Copyright 2010
• • Dirk Eibach, Guntermann & Drunck GmbH, eibach@gdsys.de
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/uclass-internal.h> +#include <gdsys_soc.h> +#include <ihs_fpga.h> +#include <ihs_video_out.h> +#include <transmitter.h> +#include <asm/gpio.h>

+#include "../transmitter/logicore_dp_tx.h"

+DECLARE_GLOBAL_DATA_PTR;

+enum {

•   REG_VERSION = 0x00,
  

•   REG_FEATURES = 0x02,
  

•   REG_CONTROL = 0x04,
  

•   REG_XY_SIZE = 0x06,
  

•   REG_XY_SCALE = 0x08,
  

•   REG_X_POS = 0x0A,
  

•   REG_Y_POS = 0x0C,
  

+};

+struct ihs_video_out_priv {

•   int addr;
  

•   int osd_base;
  

•   int osd_buffer_base;
  

•   uint base_width;
  

•   uint base_height;
  

•   int sync_src;
  

•   struct udevice *dp_tx;
  

•   struct udevice *clk_gen;
  

+};

+UCLASS_DRIVER(ihs_video_out) = {

•   .id             = UCLASS_IHS_VIDEO_OUT,
  

•   .name           = "ihs_video_out",
  

•   .flags          = DM_UC_FLAG_SEQ_ALIAS,
  

+};

+static const struct udevice_id ihs_video_out_ids[] = {

•   { .compatible = "gdsys,ihs_video_out" },
  

•   { }
  

+};

+int video_out_get_data(struct udevice *dev, struct ihs_video_out_data *data) +{

•   struct ihs_video_out_ops *ops = ihs_video_out_get_ops(dev);
  

•   return ops->get_data(dev, data);
  

+}

+int video_out_set_mem(struct udevice *dev, uint x, uint y, u16 *buf,

•                 size_t buflen)
  

+{

•   struct ihs_video_out_ops *ops = ihs_video_out_get_ops(dev);
  

•   return ops->set_mem(dev, x, y, buf, buflen);
  

+}

+int video_out_set_control(struct udevice *dev, u16 value) +{

•   struct ihs_video_out_ops *ops = ihs_video_out_get_ops(dev);
  

•   return ops->set_control(dev, value);
  

+}

+int video_out_set_size(struct udevice *dev, u16 xy_size, u16 x_pos, u16 y_pos) +{

•   struct ihs_video_out_ops *ops = ihs_video_out_get_ops(dev);
  

•   return ops->set_size(dev, xy_size, x_pos, y_pos);
  

+}

+void print_info(struct udevice *dev) +{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   struct gdsys_soc_child_platdata *pplat = dev_get_parent_platdata(dev);
  

•   u16 version;
  

•   u16 features;
  

•   u16 control = 0x49;
  

•   version = fpga_in_le16(pplat->fpga, priv->osd_base + REG_VERSION);
  

•   features = fpga_in_le16(pplat->fpga, priv->osd_base + REG_FEATURES);
  

•   if (priv->sync_src)
  

•           control |= ((priv->sync_src & 0x7) << 8);
  

•   fpga_out_le16(pplat->fpga, priv->osd_base + REG_CONTROL, control);
  

•   priv->base_width = ((features & 0x3f00) >> 8) + 1;
  

•   priv->base_height = (features & 0x001f) + 1;
  

•   printf("OSD-%s: Digital-OSD version %01d.%02d, %d x %d characters\n",
  

•          dev->name, version / 100, version % 100,
  

•          priv->base_width, priv->base_height);
  

+}

+int ihs_video_out_get_data(struct udevice *dev, struct ihs_video_out_data *data) +{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   data->width = priv->base_width;
  

•   data->height = priv->base_height;
  

•   return 0;
  

+}

+int ihs_video_out_set_mem(struct udevice *dev, uint x, uint y, u16 *buf,

•                     size_t buflen)
  

+{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   struct gdsys_soc_child_platdata *pplat = dev_get_parent_platdata(dev);
  

•   uint offset = y * priv->base_width + x;
  

•   uint k;
  

•   for (k = 0; k < buflen; ++k) {
  

•           if (offset + k >= priv->base_width * priv->base_height)
  

•                   return -ENXIO;
  

•           fpga_out_le16(pplat->fpga,
  

•                         priv->osd_buffer_base + offset + k, buf[k]);
  

•   }
  

•   return buflen;
  

+}

+int ihs_video_out_set_control(struct udevice *dev, u16 value) +{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   struct gdsys_soc_child_platdata *pplat = dev_get_parent_platdata(dev);
  

•   fpga_out_le16(pplat->fpga, priv->addr + REG_CONTROL, value);
  

•   return 0;
  

+}

+int ihs_video_out_set_size(struct udevice *dev, u16 xy_size,

•                      u16 x_pos, u16 y_pos)
  

+{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   struct gdsys_soc_child_platdata *pplat = dev_get_parent_platdata(dev);
  

•   fpga_out_le16(pplat->fpga, priv->addr + REG_XY_SIZE, xy_size);
  

•   fpga_out_le16(pplat->fpga, priv->addr + REG_X_POS, x_pos);
  

•   fpga_out_le16(pplat->fpga, priv->addr + REG_Y_POS, y_pos);
  

•   return 0;
  

+}

+static const struct ihs_video_out_ops ihs_video_out_ops = {

•   .get_data = ihs_video_out_get_data,
  

•   .set_mem = ihs_video_out_set_mem,
  

•   .set_control = ihs_video_out_set_control,
  

•   .set_size = ihs_video_out_set_size,
  

+};

+int ihs_video_out_probe(struct udevice *dev) +{

•   struct ihs_video_out_priv *priv = dev_get_priv(dev);
  

•   int len = 0;
  

•   struct fdtdec_phandle_args phandle_args;
  

•   char *mode;
  

•   priv->addr = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
  

•                               "reg", -1);
  

Can we use dev_read_...() API?

I'll switch to the new API in v2 (again, older code so it still used the old API).

...

•   priv->osd_base = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
  

•                                   "osd_base", -1);
  

•   priv->osd_buffer_base = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
  

•                                          "osd_buffer_base", -1);
  

•   priv->sync_src = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
  

•                   "sync-source");
  

•   print_info(dev);
  

•   if (fdtdec_parse_phandle_with_args(gd->fdt_blob, dev_of_offset(dev),
  

•                                      "clk_gen", NULL, 0, 0,
  

•                                      &phandle_args)) {
  

•           printf("%s: Could not get clk_gen node.\n", dev->name);
  

•           return -1;
  

These should return -EINVAL I think.

OK, will change in v2.

...

•   }
  

•   if (device_get_global_by_of_offset(phandle_args.node, &priv->clk_gen)) {
  

•           printf("%s: Could not get clk_gen dev.\n", dev->name);
  

•           return -1;
  

•   }
  

•   if (fdtdec_parse_phandle_with_args(gd->fdt_blob, dev_of_offset(dev),
  

•                                      "dp_tx", NULL, 0, 0,
  

•                                      &phandle_args)) {
  

•           printf("%s: Could not get dp_tx.\n", dev->name);
  

•           return -1;
  

•   }
  

•   if (device_get_global_by_of_offset(phandle_args.node, &priv->dp_tx)) {
  

•           printf("%s: Could not get dp_tx dev.\n", dev->name);
  

•           return -1;
  

•   }
  

•   mode = (char *)fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
  

•                              "mode", &len);
  

•   if (!mode) {
  

•           printf("%s: Could not read mode property.\n", dev->name);
  

•           return -1;
  

•   }
  

•   if (!strcmp(mode, "1024_768_60"))
  

•           priv->sync_src = 2;
  

•   else if (!strcmp(mode, "720_400_70"))
  

•           priv->sync_src = 1;
  

•   else
  

•           priv->sync_src = 0;
  

•   transmitter_power_on(priv->dp_tx, mode);
  

•   return 0;
  

+}

+U_BOOT_DRIVER(ihs_video_out_drv) = {

•   .name           = "ihs_video_out_drv",
  

•   .id             = UCLASS_IHS_VIDEO_OUT,
  

•   .ops            = &ihs_video_out_ops,
  

•   .of_match       = ihs_video_out_ids,
  

•   .probe          = ihs_video_out_probe,
  

•   .priv_auto_alloc_size = sizeof(struct ihs_video_out_priv),
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 1a24de10b4..a40185d2bb 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -44,6 +44,7 @@ enum uclass_id { UCLASS_I2C_MUX, /* I2C multiplexer */ UCLASS_IHS_AXI, /* gdsys IHS AXI bus */ UCLASS_IHS_FPGA, /* gdsys IHS FPGAs */

•   UCLASS_IHS_VIDEO_OUT,   /* gdsys IHS video output */
  

Is this video out? It seems to me it is more about an on-screen display/ So how about a new uclass like VIDEO_OSD?

Then you can make it generic so that other drivers might use it. Your operations don't seem too specific to this driver...?

Yes, the naming convention is a bit weird here. The video_out device controls both the actual video registers and the OSD functionalities, but only the OSD stuff is used in U-Boot.

A new uclass would be fine with me, but is it OK to assume that all OSDs are text-based (like ours is)? What about those that are pixel-based? Would they just use a different set of functions in the same uclass?

...

    UCLASS_IRQ,             /* Interrupt controller */
    UCLASS_KEYBOARD,        /* Keyboard input device */
    UCLASS_LED,             /* Light-emitting diode (LED) */

diff --git a/include/ihs_video_out.h b/include/ihs_video_out.h new file mode 100644 index 0000000000..e3dcdad8c5 --- /dev/null +++ b/include/ihs_video_out.h @@ -0,0 +1,146 @@ +/*

• • (C) Copyright 2017
• • Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _IHS_VIDEO_OUT_H_ +#define _IHS_VIDEO_OUT_H_

+/*

• • The IHS OSD is a character-oriented on-screen display of gdsys devices built
• • into a IHS FPGA. It is controlled via a set of registers in the FPGA's
• • register space, as well as a memory buffer that holds character data
• • (line-wise, left-to-right, 16 bit per character) that constitute the data to
• • be displayed by the OSD.
• */

+/**

• • struct ihs_video_out_data - information about a IHS OSD instance
• • @width Maximum width of the OSD screen in characters.
• • @heigth Maximum height of the OSD screen in characters.
• */

+struct ihs_video_out_data {

•   uint width;
  

•   uint height;
  

+};

+/**

• • struct ihs_video_out_ops - driver operations for IHS OSD uclass
• • Drivers should support these operations unless otherwise noted. These
• • operations are intended to be used by uclass code, not directly from
• • other code.
• */

+struct ihs_video_out_ops {

•   /**
  

•    * get_data() - Get information about a IHS OSD instance
  

•    *
  

•    * A IHS OSD instance keeps some internal data about itself. This
  

•    * function can be used to access this data.
  

•    *
  

•    * @dev:        IHS OSD instance to query.
  

•    * @data:       Pointer to a struct ihs_video_out_data structure that
  

•    *              takes the information read from the OSD instance.
  

•    * @return 0 if OK, -ve on error.
  

•    */
  

•   int (*get_data)(struct udevice *dev, struct ihs_video_out_data *data);
  

•   /**
  

•    * set_mem() - Write pixel data to OSD memory
  

•    *
  

•    * Each 16 bit word of data encodes the display information for a
  

•    * single character. The data are written to a given character position
  

•    * (x/y coordinates), which are written left-to-right, line-wise,
  

•    * continuing to the next line if the data are longer than the current
  

•    * line.
  

•    *
  

•    * @dev:        IHS OSD instance to query.
  

query?

Copy-paste error, will fix in v2.

...

•    * @x:          Horizontal character coordinate to write to.
  

•    * @y:          Vertical character coordinate to write to.
  

•    * @buf:        Array containing 16 bit words to write to the specified
  

•    *              address in the IHS OSD memory.
  

Here I wonder if we can use a u8 * ?

For a generic osd uclass definitely; I used u16 here, since our OSD only supports 16-bit accesses.

...

•    * @buflen:     Length of the data in the passed buffer (in 16 bit
  

•    *              words).
  

•    * @return 0 if OK, -ve on error.
  

•    */
  

•   int (*set_mem)(struct udevice *dev, uint x, uint y, u16 *buf, size_t buflen);
  

•   /**
  

•    * set_control() - Set the value of the IHS OSD's control register
  

•    *
  

•    * @dev:        IHS OSD instance to write to.
  

•    * @value:      Value to write to the IHS OSD's control register.
  

•    * @return 0 if OK, -ve on error.
  

•    */
  

•   int (*set_control)(struct udevice *dev, u16 value);
  

•   /**
  

•    * set_size() - Set the position and dimension of the IHS OSD's
  

•    *              writeable window
  

•    *
  

•    * @dev:        IHS OSD instance to write to.
  

•    * @xy_size:    The number of characters in a row and a column,
  

•    *              combined into a 16 bit value.
  

Can you separate out the values?

OK, will separate them in v2.

...

•    * @x_pos:      The horizontal position of the upper left corner of the
  

•    *              OSD's writeable window in pixels.
  

•    * @y_pos:      The vertical position of the upper left corner of the
  

•    *              OSD's writeable window in pixels.
  

•    * @return 0 if OK, -ve on error.
  

•    */
  

•   int (*set_size)(struct udevice *dev, u16 xy_size, u16 x_pos, u16 y_pos);
  

+};

+#define ihs_video_out_get_ops(dev) ((struct ihs_video_out_ops *)(dev)->driver->ops)

+/**

• • video_out_get_data() - Get information about a IHS OSD instance
• • @dev: IHS OSD instance to query.
• • @data: Pointer to a struct ihs_video_out_data structure that takes the

• •         information read from the OSD instance.
    

• • @return 0 if OK, -ve on error.
• */

+int video_out_get_data(struct udevice *dev, struct ihs_video_out_data *data);

+/**

• • video_out_set_mem() - Write pixel data to OSD memory
• • @dev: IHS OSD instance to query.
• • @x: Horizontal character coordinate to write to.
• • @y: Vertical character coordinate to write to.
• • @buf: Array containing 16 bit words to write to the specified address

• •         in the IHS OSD memory.
    

u8 * ?

For a generic osd uclass, yes. Will fix in v2.

...

• • @buflen: Length of the data in the passed buffer (in 16 bit words).
• • @return 0 if OK, -ve on error.
• */

+int video_out_set_mem(struct udevice *dev, uint x, uint y, u16 *buf,

•                 size_t buflen);
  

+/**

• • video_out_set_control() - Set the value of the IHS OSD's control register
• • @dev: IHS OSD instance to write to.
• • @value: Value to write to the IHS OSD's control register.
• • @return 0 if OK, -ve on error.
• */

+int video_out_set_control(struct udevice *dev, u16 value);

+/**

• • video_out_set_size() - Set the position and dimension of the IHS OSD's

• •                   writeable window
    

• • @dev: IHS OSD instance to write to.
• • @xy_size: The number of characters in a row and a column, combined into a

• •         16 bit value.
    

• • @x_pos: The horizontal position of the upper left corner of the OSD's

• •         writeable window in pixels.
    

• • @y_pos: The vertical position of the upper left corner of the OSD's

• •         writeable window in pixels.
    

• • @return 0 if OK, -ve on error.
• */

+int video_out_set_size(struct udevice *dev, u16 xy_size, u16 x_pos, u16 y_pos);

+#endif /* !_IHS_VIDEO_OUT_H_ */

2.11.0

Regards, Simon

Best regards,

Mario