Hi Simon,

On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Mario,
>
> On 14 July 2017 at 05:55, Mario Six <mario....@gdsys.cc> wrote:
>> Add a driver for IHS OSDs on IHS FPGAs.
>>
>> Signed-off-by: Mario Six <mario....@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....@gdsys.cc
>> + *
>> + * based on the gdsys osd driver, which is
>> + *
>> + * (C) Copyright 2010
>> + * Dirk Eibach,  Guntermann & Drunck GmbH, eib...@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....@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to