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
