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:
This patch adds DM drivers for IHS FPGAs and their associated busses, as well as uclasses for both.
Signed-off-by: Mario Six mario.six@gdsys.cc
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, &spec)) {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, &spec)) {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, &args)) {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, &priv->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().
I since found this error myself; the probe should really fail if we can't get the GPIOs (since we can't start the FPGA in this case). Will be fixed in v2.
printf("%s: Could not get reset-GPIO.\n", dev->name);gpio_request_by_name(dev, "done-gpios", 0, &priv->done_gpio,GPIOD_IS_IN);if (!priv->done_gpio.dev)printf("%s: Could not get done-GPIO.\n", dev->name);dm_gpio_set_value(&priv->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.
Dito, should make the probe fail. Will fix in v2.
signal_startup_finished(dev);if (!do_reflection_test(dev)) {int ctr = 0;dm_gpio_set_value(&priv->reset_gpio, 0);while (!dm_gpio_get_value(&priv->done_gpio)) {mdelay(100);if (ctr++ > 5) {printf("Initializing FPGA failed\n");break;}}udelay(10);dm_gpio_set_value(&priv->reset_gpio, 1);if (!do_reflection_test(dev)) {printf("%s: Reflection test FAILED.\n", dev->name);return -1;This is -EPERM. Perhaps use -EIO? Better to return vaue from the do_reflection_tests() call.
Will fix in v2.
}}printf("%s: Reflection test passed.\n", dev->name);debug()?
That's actually another status message: If it's printed, we know that at least the FPGA is alive.
if (fdtdec_parse_phandle_with_args(gd->fdt_blob, dev_of_offset(dev), "bus",NULL, 0, 0, &phandle_args))return -1;lists_bind_fdt(dev, offset_to_ofnode(phandle_args.node), &busdev);Should this be dm_scan_fdt_dev,()?
Also should go in bind() method
I could use dm_scan_fdt_dev here, but if you take a look in the device tree, you'll see that the FPGA's bus is not actually a child of the FPGA itself, but of the root node (that's a quirk from the Linux driver; you will probably see more of this in the course of the series, I'm afraid). So to make it less confusing in U-Boot, I use lists_bind_fdt to explicitly bind the bus as a child of the FPGA. It's pretty much a cosmetic thing, though.
I'll move it to bind() in v2.
fpga_print_info(dev);/* TODO: Check if this should be gazerbeam-specific */if (priv->has_osd) {/* Disable softcore, select external pixclock */fpga_set_reg(dev, "control", 0x8000);}device_probe(busdev);for (device_find_first_child(busdev, &child);child;device_find_next_child(&child))device_probe(child);The children should be probed when used by things like uclass_get_device()...
We should not be probing them here.
The way the old drivers (board/gdsys/common) worked was that everything was explicitly initialized/probed/turned on during startup unconditionally, and I tried to replicate this behavior here.
For the Gazerbeam CON device, DM's lazy probing would probably work as well, since we display a startup screen on the OSD via some OSD commands in a U-Boot script (which would in turn probe all devices we need). The Gazerbeam CPU in contrast doesn't have a OSD, but we need to program the FPGA regardless.
So, how do I make sure that the FPGA is activated in this case (without explicit probing, that is)?
return 0;+}
+static const struct udevice_id ihs_fpga_ids[] = {
{ .compatible = "gdsys,iocon_fpga" },{ .compatible = "gdsys,iocpu_fpga" },{ }+};
+U_BOOT_DRIVER(ihs_fpga_bus) = {
.name = "ihs_fpga_bus",.id = UCLASS_IHS_FPGA,.ops = &ihs_fpga_ops,.of_match = ihs_fpga_ids,.probe = ihs_fpga_probe,.priv_auto_alloc_size = sizeof(struct ihs_fpga_priv),+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 2e6498b7dc..56fbedaa9d 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -35,12 +35,14 @@ enum uclass_id { UCLASS_DISPLAY, /* Display (e.g. DisplayPort, HDMI) */ UCLASS_DMA, /* Direct Memory Access */ UCLASS_ETH, /* Ethernet device */
UCLASS_GDSYS_SOC, /* gdsys soc buses */ UCLASS_GPIO, /* Bank of general-purpose I/O pins */ UCLASS_FIRMWARE, /* Firmware */ UCLASS_I2C, /* I2C bus */ UCLASS_I2C_EEPROM, /* I2C EEPROM device */ UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_I2C_MUX, /* I2C multiplexer */UCLASS_IHS_FPGA, /* gdsys IHS FPGAs */ UCLASS_IRQ, /* Interrupt controller */ UCLASS_KEYBOARD, /* Keyboard input device */ UCLASS_LED, /* Light-emitting diode (LED) */diff --git a/include/gdsys_soc.h b/include/gdsys_soc.h new file mode 100644 index 0000000000..2164651f62 --- /dev/null +++ b/include/gdsys_soc.h @@ -0,0 +1,29 @@ +/*
- (C) Copyright 2017
- Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _GDSYS_SOC_H_ +#define _GDSYS_SOC_H_
+/**
- struct gdsys_soc_child_platdata - platform data for devices on gdsys soc
buses
- To access their register maps, devices on gdsys soc buses usually have
- facilitate the accessor function of the IHS FPGA their parent bus is
- attached to. To pass the FPGA device down to the bus' children, a pointer to
- it is contained in this structure.
- To obtain this structure, use dev_get_parent_platdata(dev) where dev is a
- device on the gdsys soc bus.
- @fpga: The IHS FPGA that controls the bus the device is attached to.
- */
+struct gdsys_soc_child_platdata {
struct udevice *fpga;+};
+#endif /* _GDSYS_SOC_H_ */ diff --git a/include/ihs_fpga.h b/include/ihs_fpga.h new file mode 100644 index 0000000000..552583cbe1 --- /dev/null +++ b/include/ihs_fpga.h @@ -0,0 +1,111 @@ +/*
- (C) Copyright 2017
- Mario Six, Guntermann & Drunck GmbH, mario.six@gdsys.cc
- SPDX-License-Identifier: GPL-2.0+
- */
+#ifndef _IHS_FPGA_H_ +#define _IHS_FPGA_H_
+/**
- struct ihs_fpga_ops - driver operations for IHS FPGA 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_fpga_ops {
/*** get_reg() - Get value of a named FPGA register** To accommodate possible future memory layout changes, the registers* of a IHS FPGA are described in the device tree, and each is given a* distinct compatible string to address them with.** @dev: FPGA device to read from.* @compat: The compatible string of the register to write to.* @value: Pointer to a variable that takes the value read from the* register.* @return 0 if OK, -ENODEV if no named register with the given* compatibility string exists.*/int (*get_reg)(struct udevice *dev, const char *compat, uint *value);/*** set_reg() - Set value of a named FPGA register** To accommodate possible future memory layout changes, the registers* of a IHS FPGA are described in the device tree, and each is given a* distinct compatible string to address them with.** @dev: FPGA device to write to.* @compat: The compatible string of the register to write to.* @value: Value to write to the register.* @return 0 if OK, -ENODEV if no named register with the given* compatibility string exists.*/int (*set_reg)(struct udevice *dev, const char *compat, uint value);/*** in_le16() - Read 16 bit value from address in FPGA register space** @dev: FPGA device to read from.* @addr: Address in the FPGA register space to read from.* @return 0 if OK, -ve on error.*/u16 (*in_le16)(struct udevice *dev, phys_addr_t addr);/*** out_le16() - Write 16 bit value to address in FPGA register space** @dev: FPGA device to write to.* @addr: Address in the FPGA register space to write to.* @return 0 if OK, -ve on error.*/void (*out_le16)(struct udevice *dev, phys_addr_t addr, u16 value);+};
+#define ihs_fpga_get_ops(dev) ((struct ihs_fpga_ops *)(dev)->driver->ops)
+/**
- fpga_get_reg() - Get value of a named FPGA register
- @dev: FPGA device to read from.
- @compat: The compatible string of the register to write to.
- @value: Pointer to a variable that takes the value read from the register.
- @return 0 if OK, -ENODEV if no named register with the given compatibility
- string exists.
- */
+int fpga_get_reg(struct udevice *dev, const char *compat, uint *value);
+/**
- fpga_set_reg() - Set value of a named FPGA register
- @dev: FPGA device to write to.
- @compat: The compatible string of the register to write to.
- @value: Value to write to the register.
- @return 0 if OK, -ENODEV if no named register with the given compatibility
- string exists.
- */
+int fpga_set_reg(struct udevice *dev, const char *compat, uint value);
+/**
- fpga_in_le16() - Read 16 bit value from address in FPGA register space
- @dev: FPGA device to read from.
- @addr: Address in the FPGA register space to read from.
- @return 0 if OK, -ve on error.
- */
+u16 fpga_in_le16(struct udevice *dev, phys_addr_t addr);
+/**
- fpga_out_le16() - Write 16 bit value to address in FPGA register space
- @dev: FPGA device to write to.
- @addr: Address in the FPGA register space to write to.
- @return 0 if OK, -ve on error.
- */
+void fpga_out_le16(struct udevice *dev, phys_addr_t addr, u16 value);
+#endif /* !_IHS_FPGA_H_ */
2.11.0
Regards, Simon
Best regards,
Mario