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?
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, &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?
}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?
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, &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().
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.
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.
}}printf("%s: Reflection test passed.\n", dev->name);
debug()?
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
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.
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