Hi Simon, On Wed, Jul 19, 2017 at 11:06 AM, Simon Glass <s...@chromium.org> wrote: > Hi Mario, > > On 14 July 2017 at 05:55, Mario Six <mario....@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....@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....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot