Re: [U-Boot] [PATCH 32/51] drivers: Add ihs_fpga and gdsys_soc drivers

[Mario Six]

Hi Simon,

On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass  wrote:
> Hi Mario,
>
> On 14 July 2017 at 05:55, Mario Six  wrote:
>> This patch adds DM drivers for IHS FPGAs and their associated busses, as
>> well as uclasses for both.
>>
>> Signed-off-by: Mario Six 
>> ---
>>
>>  drivers/misc/Kconfig |   6 +
>>  drivers/misc/Makefile|   1 +
>>  drivers/misc/gdsys_soc.c |  51 +++
>>  drivers/misc/ihs_fpga.c  | 871 
>> +++
>>  include/dm/uclass-id.h   |   2 +
>>  include/gdsys_soc.h  |  29 ++
>>  include/ihs_fpga.h   | 111 ++
>>  7 files changed, 1071 insertions(+)
>>  create mode 100644 drivers/misc/gdsys_soc.c
>>  create mode 100644 drivers/misc/ihs_fpga.c
>>  create mode 100644 include/gdsys_soc.h
>>  create mode 100644 include/ihs_fpga.h
>
> There are definitely cases where you we might have a unique peripheral
> and want to add a uclass for it.
>
> But for what you have here, could we use UCLASS_SYSCON for the soc bus?
>

OK, I wasn't aware that the syscon uclass could be used in such a case.

> For the FPGA could we add a new FPGA uclass? It could have read and
> write methods a bit like struct pci_ops where you specify the size of
> the read/write. I think that would be useful generally.
>
> [..]
>

OK, I guess I can make a general FPGA class. But wouldn't that collide with the
type of functionality we have in drivers/fpga now? That code is more concerned
with getting a FPGA to run (load a bit file from somewhere and similar
functions) than accessing the register interface of a running FPGA from what I
can see. Would that be eventually absorbed into the new uclass? Or should those
be different uclasses? Should I put the "new" fpga-uclass.c file in
drivers/fpga, or make a new directory?

>> +static int ihs_fpga_set_reg(struct udevice *dev, const char *compat,
>> +   uint value)
>> +{
>> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
>> +   struct reg_spec spec;
>> +
>> +   if (get_reg_spec(dev, compat, )) {
>> +   printf("%s: Could not get %s regspec for '%s'.\n", __func__,
>> +  dev->name, compat);
>> +   return -ENODEV;
>
> Can you return the error from get_reg_spec()? Also -ENODEV means no
> device which cannot be true if you have  a dev pointer. Perhaps
> -ENXIO?
>

Yes, that's a copy-paste-error; I'll fix it in v2.

>> +   }
>> +
>> +   out_le16((void *)(priv->regs + spec.addr), value << spec.shift);
>> +
>> +   return 0;
>> +}
>> +
>> +static int ihs_fpga_get_reg(struct udevice *dev, const char *compat,
>> +   uint *value)
>> +{
>> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
>> +   struct reg_spec spec;
>> +   uint tmp;
>> +
>> +   if (get_reg_spec(dev, compat, )) {
>> +   printf("%s: Could not get %s regspec for '%s'.\n", __func__,
>> +  dev->name, compat);
>
> Can we use debug() in drivers?
>

Will fix in v2.

>> +   return -ENODEV;
>> +   }
>> +
>> +   tmp = in_le16((void *)(priv->regs + spec.addr));
>> +   *value = (tmp & spec.mask) >> spec.shift;
>> +
>> +   return 0;
>> +}
>> +
>> +static u16 ihs_fpga_in_le16(struct udevice *dev, phys_addr_t addr)
>> +{
>> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
>> +
>> +   /* TODO: MCLink transfer */
>> +
>> +   return in_le16((void *)(priv->regs + addr));
>> +}
>> +
>> +static void ihs_fpga_out_le16(struct udevice *dev, phys_addr_t addr, u16 
>> value)
>> +{
>> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
>> +
>> +   /* TODO: MCLink transfer */
>> +
>> +   out_le16((void *)(priv->regs + addr), value);
>> +}
>> +
>> +static const struct ihs_fpga_ops ihs_fpga_ops = {
>> +   .set_reg = ihs_fpga_set_reg,
>> +   .get_reg = ihs_fpga_get_reg,
>> +   .in_le16 = ihs_fpga_in_le16,
>> +   .out_le16 = ihs_fpga_out_le16,
>> +};
>> +
>> +static int ihs_fpga_probe(struct udevice *dev)
>> +{
>> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
>> +   fdt_addr_t addr;
>> +   struct fdtdec_phandle_args phandle_args;
>
> Since this is new code I think using the dev_read_...() functions is better
>

OK, I'll switch it to the new API in v2. I had this code lying around for a
bit, so it still uses the old interface.

>> +   struct fdtdec_phandle_args args;
>> +   struct udevice *busdev = NULL;
>> +   struct udevice *child = NULL;
>> +   uint mem_width;
>> +
>> +   if (fdtdec_parse_phandle_with_args(gd->fdt_blob, dev_of_offset(dev),
>> +  "regmap", NULL, 0, 0, )) {
>> +   printf("%s: Could not get regmap.\n", dev->name);
>> +   return 1;
>> +   }
>> +
>> +   priv->regmap_node = args.node;
>> +
>> +   addr = devfdt_get_addr(dev);
>> +
>> +   priv->addr = addr;
>> +   priv->regs = 

Re: [U-Boot] [PATCH 32/51] drivers: Add ihs_fpga and gdsys_soc drivers

[Simon Glass]

Hi Mario,

On 14 July 2017 at 05:55, Mario Six  wrote:
> This patch adds DM drivers for IHS FPGAs and their associated busses, as
> well as uclasses for both.
>
> Signed-off-by: Mario Six 
> ---
>
>  drivers/misc/Kconfig |   6 +
>  drivers/misc/Makefile|   1 +
>  drivers/misc/gdsys_soc.c |  51 +++
>  drivers/misc/ihs_fpga.c  | 871 
> +++
>  include/dm/uclass-id.h   |   2 +
>  include/gdsys_soc.h  |  29 ++
>  include/ihs_fpga.h   | 111 ++
>  7 files changed, 1071 insertions(+)
>  create mode 100644 drivers/misc/gdsys_soc.c
>  create mode 100644 drivers/misc/ihs_fpga.c
>  create mode 100644 include/gdsys_soc.h
>  create mode 100644 include/ihs_fpga.h

There are definitely cases where you we might have a unique peripheral
and want to add a uclass for it.

But for what you have here, could we use UCLASS_SYSCON for the soc bus?

For the FPGA could we add a new FPGA uclass? It could have read and
write methods a bit like struct pci_ops where you specify the size of
the read/write. I think that would be useful generally.

[..]

> +static int ihs_fpga_set_reg(struct udevice *dev, const char *compat,
> +   uint value)
> +{
> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
> +   struct reg_spec spec;
> +
> +   if (get_reg_spec(dev, compat, )) {
> +   printf("%s: Could not get %s regspec for '%s'.\n", __func__,
> +  dev->name, compat);
> +   return -ENODEV;

Can you return the error from get_reg_spec()? Also -ENODEV means no
device which cannot be true if you have  a dev pointer. Perhaps
-ENXIO?

> +   }
> +
> +   out_le16((void *)(priv->regs + spec.addr), value << spec.shift);
> +
> +   return 0;
> +}
> +
> +static int ihs_fpga_get_reg(struct udevice *dev, const char *compat,
> +   uint *value)
> +{
> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
> +   struct reg_spec spec;
> +   uint tmp;
> +
> +   if (get_reg_spec(dev, compat, )) {
> +   printf("%s: Could not get %s regspec for '%s'.\n", __func__,
> +  dev->name, compat);

Can we use debug() in drivers?

> +   return -ENODEV;
> +   }
> +
> +   tmp = in_le16((void *)(priv->regs + spec.addr));
> +   *value = (tmp & spec.mask) >> spec.shift;
> +
> +   return 0;
> +}
> +
> +static u16 ihs_fpga_in_le16(struct udevice *dev, phys_addr_t addr)
> +{
> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
> +
> +   /* TODO: MCLink transfer */
> +
> +   return in_le16((void *)(priv->regs + addr));
> +}
> +
> +static void ihs_fpga_out_le16(struct udevice *dev, phys_addr_t addr, u16 
> value)
> +{
> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
> +
> +   /* TODO: MCLink transfer */
> +
> +   out_le16((void *)(priv->regs + addr), value);
> +}
> +
> +static const struct ihs_fpga_ops ihs_fpga_ops = {
> +   .set_reg = ihs_fpga_set_reg,
> +   .get_reg = ihs_fpga_get_reg,
> +   .in_le16 = ihs_fpga_in_le16,
> +   .out_le16 = ihs_fpga_out_le16,
> +};
> +
> +static int ihs_fpga_probe(struct udevice *dev)
> +{
> +   struct ihs_fpga_priv *priv = dev_get_priv(dev);
> +   fdt_addr_t addr;
> +   struct fdtdec_phandle_args phandle_args;

Since this is new code I think using the dev_read_...() functions is better

> +   struct fdtdec_phandle_args args;
> +   struct udevice *busdev = NULL;
> +   struct udevice *child = NULL;
> +   uint mem_width;
> +
> +   if (fdtdec_parse_phandle_with_args(gd->fdt_blob, dev_of_offset(dev),
> +  "regmap", NULL, 0, 0, )) {
> +   printf("%s: Could not get regmap.\n", dev->name);
> +   return 1;
> +   }
> +
> +   priv->regmap_node = args.node;
> +
> +   addr = devfdt_get_addr(dev);
> +
> +   priv->addr = addr;
> +   priv->regs = map_sysmem(addr, mem_width);
> +
> +   gpio_request_by_name(dev, "reset-gpios", 0, >reset_gpio,
> +GPIOD_IS_OUT);
> +   if (!priv->reset_gpio.dev)

You should check the return value of the function. How come you don't
return the error code here? If it is OK to not have a GPIO then use
dm_gpio_is_valid() - and hopefully debug() instead of printf().

> +   printf("%s: Could not get reset-GPIO.\n", dev->name);
> +
> +   gpio_request_by_name(dev, "done-gpios", 0, >done_gpio,
> +GPIOD_IS_IN);
> +   if (!priv->done_gpio.dev)
> +   printf("%s: Could not get done-GPIO.\n", dev->name);
> +
> +   dm_gpio_set_value(>reset_gpio, 1);

Error check? This returns -ENOENT if the GPIO is not valid (as above)
so you could filter that out, or use dm_gpo_is_valid() to avoid
calling it.

> +
> +   signal_startup_finished(dev);
> +
> +   if (!do_reflection_test(dev)) {
> +   int ctr = 

[U-Boot] [PATCH 32/51] drivers: Add ihs_fpga and gdsys_soc drivers

[Mario Six]

This patch adds DM drivers for IHS FPGAs and their associated busses, as
well as uclasses for both.

Signed-off-by: Mario Six 
---

 drivers/misc/Kconfig |   6 +
 drivers/misc/Makefile|   1 +
 drivers/misc/gdsys_soc.c |  51 +++
 drivers/misc/ihs_fpga.c  | 871 +++
 include/dm/uclass-id.h   |   2 +
 include/gdsys_soc.h  |  29 ++
 include/ihs_fpga.h   | 111 ++
 7 files changed, 1071 insertions(+)
 create mode 100644 drivers/misc/gdsys_soc.c
 create mode 100644 drivers/misc/ihs_fpga.c
 create mode 100644 include/gdsys_soc.h
 create mode 100644 include/ihs_fpga.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index d1ddbbe157..8b59a444ce 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -196,4 +196,10 @@ config I2C_EEPROM
depends on MISC
help
  Enable a generic driver for EEPROMs attached via I2C.
+
+config IHS_FPGA
+   bool "Enable IHS FPGA driver"
+   depends on MISC
+   help
+ Support for IHS FPGA.
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 10265c8fb4..d2e46fc7d6 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,3 +52,4 @@ obj-$(CONFIG_FSL_DEVICE_DISABLE) += fsl_devdis.o
 obj-$(CONFIG_WINBOND_W83627) += winbond_w83627.o
 obj-$(CONFIG_QFW) += qfw.o
 obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
+obj-$(CONFIG_IHS_FPGA) += ihs_fpga.o gdsys_soc.o
diff --git a/drivers/misc/gdsys_soc.c b/drivers/misc/gdsys_soc.c
new file mode 100644
index 00..34b06d44bd
--- /dev/null
+++ b/drivers/misc/gdsys_soc.c
@@ -0,0 +1,51 @@
+/*
+ * (C) Copyright 2017
+ * Mario Six,  Guntermann & Drunck GmbH, mario@gdsys.cc
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int gdsys_soc_child_post_bind(struct udevice *dev)
+{
+   struct gdsys_soc_child_platdata *pplat = dev_get_parent_platdata(dev);
+   struct udevice *fpga;
+
+   if (uclass_get_device_by_phandle(UCLASS_IHS_FPGA, dev->parent, "fpga",
+)) {
+   printf("%s: Could not find 'fpga' phandle.\n",
+  dev->parent->name);
+   return 1;
+   }
+
+   pplat->fpga = fpga;
+
+   return 0;
+}
+
+UCLASS_DRIVER(gdsys_soc) = {
+   .id = UCLASS_GDSYS_SOC,
+   .name   = "gdsys_soc",
+   .post_probe = dm_scan_fdt_dev,
+   .child_post_bind = gdsys_soc_child_post_bind,
+   .per_child_platdata_auto_alloc_size =
+   sizeof(struct gdsys_soc_child_platdata),
+};
+
+static const struct udevice_id gdsys_soc_ids[] = {
+   { .compatible = "gdsys,soc" },
+   { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(gdsys_soc_bus) = {
+   .name   = "gdsys_soc_bus",
+   .id = UCLASS_GDSYS_SOC,
+   .of_match   = gdsys_soc_ids,
+};
diff --git a/drivers/misc/ihs_fpga.c b/drivers/misc/ihs_fpga.c
new file mode 100644
index 00..a2b5d3f58c
--- /dev/null
+++ b/drivers/misc/ihs_fpga.c
@@ -0,0 +1,871 @@
+/*
+ * (C) Copyright 2017
+ * Mario Six,  Guntermann & Drunck GmbH, mario@gdsys.cc
+ *
+ * based on the ioep-fpga driver, which is
+ *
+ * (C) Copyright 2014
+ * Dirk Eibach,  Guntermann & Drunck GmbH, eib...@gdsys.de
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct reg_spec {
+   uint addr;
+   uint shift;
+   ulong mask;
+};
+
+struct ihs_fpga_priv {
+   u8 *regs;
+   fdt_addr_t addr;
+   struct gpio_desc reset_gpio;
+   struct gpio_desc done_gpio;
+   struct gpio_desc startupfin_gpios[2];
+   int regmap_node;
+   bool has_osd;
+};
+
+const u16 reflection_testpattern = 0xdede;
+
+enum pcb_video_type {
+   PCB_DVI_SL,
+   PCB_DP_165MPIX,
+   PCB_DP_300MPIX,
+   PCB_HDMI,
+   PCB_DP_1_2,
+   PCB_HDMI_2_0,
+};
+
+enum pcb_transmission_type {
+   PCB_CAT_1G,
+   PCB_FIBER_3G,
+   PCB_CAT_10G,
+   PCB_FIBER_10G,
+};
+
+enum carrier_speed {
+   CARRIER_SPEED_1G,
+   CARRIER_SPEED_3G,
+   CARRIER_SPEED_2_5G = CARRIER_SPEED_3G,
+   CARRIER_SPEED_10G,
+};
+
+enum ram_config {
+   RAM_DDR2_32BIT_295MBPS,
+   RAM_DDR3_32BIT_590MBPS,
+   RAM_DDR3_48BIT_590MBPS,
+   RAM_DDR3_64BIT_1800MBPS,
+   RAM_DDR3_48BIT_1800MBPS,
+};
+
+enum sysclock {
+   SYSCLK_147456,
+};
+
+struct fpga_versions {
+   bool video_channel;
+   bool con_side;
+   enum pcb_video_type pcb_video_type;
+   enum pcb_transmission_type pcb_transmission_type;
+   unsigned int hw_version;
+};
+
+struct fpga_features {
+   u8 video_channels;
+   u8 carriers;
+   enum carrier_speed carrier_speed;
+   enum ram_config ram_config;
+   enum sysclock sysclock;
+