[U-Boot] [PATCH 32/51] drivers: Add ihs_fpga and gdsys_soc drivers
Simon Glass
sjg at chromium.org
Wed Jul 19 09:06:07 UTC 2017
Hi Mario,
On 14 July 2017 at 05:55, Mario Six <mario.six at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list