[U-Boot] [PATCH 32/51] drivers: Add ihs_fpga and gdsys_soc drivers
Mario Six
mario.six at gdsys.cc
Tue Jul 25 09:22:48 UTC 2017
Hi Simon,
On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass <sjg at chromium.org> wrote:
> 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?
>
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 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
Best regards,
Mario
More information about the U-Boot
mailing list