[v6,5/8] led: led_cortina: Add CAxxx LED support

From: Jway Lin <jway.lin at cortina-access.com>

Add Cortina Access LED controller support for CAxxxx SOCs

Signed-off-by: Jway Lin <jway.lin at cortina-access.com>
Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
CC: Simon Glass <sjg at chromium.org>

---

Changes in v6: None
Changes in v5: None
Changes in v4:
- remove unused macros
- remove cortina prefix from macros
- remove use BSS variable
- further cleanup to meet code style guidelines
- add additinal struct comments
- rename DT blink rate symbol

Changes in v3: None
Changes in v2: None

 MAINTAINERS               |   2 +
 drivers/led/Kconfig       |   8 ++
 drivers/led/Makefile      |   1 +
 drivers/led/led_cortina.c | 268 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 279 insertions(+)
 create mode 100644 drivers/led/led_cortina.c

Hi Alex,

On Mon, 20 Apr 2020 at 23:30, Alex Nemirovsky
<alex.nemirovsky at cortina-access.com> wrote:
>
> From: Jway Lin <jway.lin at cortina-access.com>
>
> Add Cortina Access LED controller support for CAxxxx SOCs
>
> Signed-off-by: Jway Lin <jway.lin at cortina-access.com>
> Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
> CC: Simon Glass <sjg at chromium.org>
>
> ---
>
> Changes in v6: None
> Changes in v5: None
> Changes in v4:
> - remove unused macros
> - remove cortina prefix from macros
> - remove use BSS variable
> - further cleanup to meet code style guidelines
> - add additinal struct comments
> - rename DT blink rate symbol
>
> Changes in v3: None
> Changes in v2: None
>
>  MAINTAINERS               |   2 +
>  drivers/led/Kconfig       |   8 ++
>  drivers/led/Makefile      |   1 +
>  drivers/led/led_cortina.c | 268 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 279 insertions(+)
>  create mode 100644 drivers/led/led_cortina.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 307e620..e9b6bfa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -183,6 +183,7 @@ F:  drivers/serial/serial_cortina.c
>  F:     drivers/mmc/ca_dw_mmc.c
>  F:     drivers/i2c/i2c-cortina.c
>  F:     drivers/i2c/i2c-cortina.h
> +F:     drivers/led/led_cortina.c
>
>  ARM/CZ.NIC TURRIS MOX SUPPORT
>  M:     Marek Behun <marek.behun at nic.cz>
> @@ -724,6 +725,7 @@ F:  drivers/serial/serial_cortina.c
>  F:     drivers/mmc/ca_dw_mmc.c
>  F:     drivers/i2c/i2c-cortina.c
>  F:     drivers/i2c/i2c-cortina.h
> +F:     drivers/led/led_cortina.c
>
>  MIPS MSCC
>  M:     Gregory CLEMENT <gregory.clement at bootlin.com>
> diff --git a/drivers/led/Kconfig b/drivers/led/Kconfig
> index 6675934..cc87fbf 100644
> --- a/drivers/led/Kconfig
> +++ b/drivers/led/Kconfig
> @@ -35,6 +35,14 @@ config LED_BCM6858
>           This option enables support for LEDs connected to the BCM6858
>           HW has blinking capabilities and up to 32 LEDs can be controlled.
>
> +config LED_CORTINA
> +       bool "LED Support for Cortina Access CAxxxx SoCs"
> +       depends on LED && (CORTINA_PLATFORM)
> +       help
> +         This option enables support for LEDs connected to the Cortina
> +         Access CAxxxx SOCs.
> +
> +
>  config LED_BLINK
>         bool "Support LED blinking"
>         depends on LED
> diff --git a/drivers/led/Makefile b/drivers/led/Makefile
> index 3654dd3..8e3ae7f 100644
> --- a/drivers/led/Makefile
> +++ b/drivers/led/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_LED_BCM6328) += led_bcm6328.o
>  obj-$(CONFIG_LED_BCM6358) += led_bcm6358.o
>  obj-$(CONFIG_LED_BCM6858) += led_bcm6858.o
>  obj-$(CONFIG_$(SPL_)LED_GPIO) += led_gpio.o
> +obj-$(CONFIG_LED_CORTINA) += led_cortina.o
> diff --git a/drivers/led/led_cortina.c b/drivers/led/led_cortina.c
> new file mode 100644
> index 0000000..f873d30
> --- /dev/null
> +++ b/drivers/led/led_cortina.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright (C) 2020 Cortina-Access
> + * Author: Jway Lin <jway.lin at cortina-access.com>
> + *
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <led.h>
> +#include <asm/io.h>
> +#include <dm/lists.h>
> +
> +#define LED_MAX_HW_BLINK               127
> +#define LED_MAX_COUNT                  16
> +
> +/* LED_CONTROL fields */
> +#define LED_BLINK_RATE1_OFFSET 0
> +#define LED_BLINK_RATE1_MASK   0xff
> +#define LED_BLINK_RATE2_OFFSET 8

Suggestion: I would call that a SHIFT rather than an offset.

> +#define LED_BLINK_RATE2_MASK   0xff
> +#define LED_CLK_TEST                   BIT(16)
> +#define LED_CLK_POLARITY               BIT(17)
> +#define LED_CLK_TEST_MODE              BIT(16)
> +#define LED_CLK_TEST_RX_TEST   BIT(30)
> +#define LED_CLK_TEST_TX_TEST   BIT(31)
> +
> +/* LED_CONFIG fields */
> +#define LED_EVENT_ON_OFFSET            0
> +#define LED_EVENT_ON_MASK              0x7
> +#define LED_EVENT_BLINK_OFFSET 3
> +#define LED_EVENT_BLINK_MASK   0x7
> +#define LED_EVENT_OFF_OFFSET   6
> +#define LED_EVENT_OFF_MASK             0x7
> +#define LED_OFF_ON_OFFSET              9
> +#define LED_OFF_ON_MASK                        0x3
> +#define LED_PORT_OFFSET                        11
> +#define LED_PORT_MASK                  0x7
> +#define LED_OFF_VAL                            BIT(14)
> +#define LED_SW_EVENT                   BIT(15)
> +#define LED_BLINK_SEL                  BIT(16)
> +
> +/* LED_CONFIG structures */
> +struct cortina_led_cfg {

struct cortina_led_priv

> +       void __iomem *regs;
> +       int idx;

comment?

> +       bool active_low;
> +       int off_event;          /* set led off event */
> +       int blink_event;        /* set led blink event */
> +       int on_event;   /* set led on event */
> +       int port;               /* corresponding ethernet port */
> +       int blink;              /* blink rate sel */
> +};
> +
> +static void cortina_led_write(void __iomem *reg, unsigned long data)
> +{
> +       writel(data, reg);
> +}
> +
> +static unsigned long cortina_led_read(void __iomem *reg)
> +{
> +       return readl(reg);
> +}
> +
> +static enum led_state_t cortina_led_get_state(struct udevice *dev)
> +{
> +       struct cortina_led_cfg *priv = dev_get_priv(dev);
> +       enum led_state_t state = LEDST_OFF;
> +       u32 val;
> +
> +       val = readl(priv->regs);
> +
> +       if (val & LED_SW_EVENT)
> +               state = LEDST_ON;
> +
> +       return state;
> +}
> +
> +static int cortina_led_set_state(struct udevice *dev, enum led_state_t state)
> +{
> +       u32 val;
> +       struct cortina_led_cfg *priv = dev_get_priv(dev);
> +
> +       val = readl(priv->regs);
> +
> +       switch (state) {
> +       case LEDST_OFF:
> +               val &= ~LED_SW_EVENT;
> +               val &= ~(LED_OFF_ON_MASK << LED_OFF_ON_OFFSET);

Those two lines look like they could move above the switch()

> +               val |= 0x3 << LED_OFF_ON_OFFSET;

Perhaps 3 (and the 1 below) should be in an enum since you are using
ones for everything else?

> +               cortina_led_write(priv->regs, val);
> +               break;
> +       case LEDST_ON:
> +               val |= LED_SW_EVENT;
> +               val &= ~(LED_OFF_ON_MASK << LED_OFF_ON_OFFSET);
> +               val |= 0x1 << LED_OFF_ON_OFFSET;
> +               cortina_led_write(priv->regs, val);

that line looks like it could be below the switch.

> +               break;
> +       case LEDST_TOGGLE:
> +               if (cortina_led_get_state(dev) == LEDST_OFF)
> +                       return cortina_led_set_state(dev, LEDST_ON);
> +               else
> +                       return cortina_led_set_state(dev, LEDST_OFF);
> +               break;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct led_ops cortina_led_ops = {
> +       .get_state = cortina_led_get_state,
> +       .set_state = cortina_led_set_state,
> +};
> +
> +static int cortina_led_probe(struct udevice *dev)
> +{
> +       struct led_uc_plat *uc_plat = dev_get_uclass_platdata(dev);

Reading the DT should really happen in the ofdata_to_platdata method.

> +
> +       /* Top-level LED node */
> +       if (!uc_plat->label) {
> +               void __iomem *regs;
> +               u32 reg_value, val;
> +               u16 rate1, rate2;
> +
> +               regs = dev_remap_addr(dev);
> +               if (!regs)
> +                       return -EINVAL;
> +
> +               reg_value = 0;
> +               reg_value |= LED_CLK_POLARITY;
> +
> +               rate1 = dev_read_u32_default(dev, "cortina,blink-rate1", 256);
> +               rate2 = dev_read_u32_default(dev, "cortina,blink-rate2", 512);
> +

If you put these into your platdata then you can have a probe() method
which just talks to the hardware, not also reading DT.

> +               val = rate1 / 16 - 1;
> +               rate1 = val > LED_MAX_HW_BLINK ?
> +                                       LED_MAX_HW_BLINK : val;
> +               reg_value |= (rate1 & LED_BLINK_RATE1_MASK) <<
> +                                       LED_BLINK_RATE1_OFFSET;
> +
> +               val = rate2 / 16 - 1;
> +               rate2 = val > LED_MAX_HW_BLINK ?
> +                                       LED_MAX_HW_BLINK : val;
> +               reg_value |= (rate2 & LED_BLINK_RATE2_MASK) <<
> +                                       LED_BLINK_RATE2_OFFSET;
> +
> +               cortina_led_write(regs, reg_value);

That call belongs in probe() though.

> +
> +       } else {
> +               struct cortina_led_cfg *priv = dev_get_priv(dev);
> +               void __iomem *regs;
> +               u32 pin, val, blink, port, off_event, blink_event, on_event;
> +
> +               regs = dev_remap_addr(dev_get_parent(dev));
> +               if (!regs)
> +                       return -EINVAL;
> +
> +               pin = dev_read_u32_default(dev, "pin", LED_MAX_COUNT);
> +
> +               if (pin >= LED_MAX_COUNT)
> +                       return -EINVAL;
> +
> +               priv->regs = regs + 4 + pin * 4;
> +
> +               val = cortina_led_read(priv->regs);
> +
> +               if (dev_read_bool(dev, "active-low")) {
> +                       priv->active_low = true;
> +                       val |= LED_OFF_VAL;
> +               } else {
> +                       priv->active_low = false;
> +                       val &= ~LED_OFF_VAL;
> +               }
> +
> +               blink = dev_read_u32_default(dev, "blink-sel", 0);
> +
> +               if (blink == 0) {
> +                       priv->blink = 0;
> +                       val &= ~LED_BLINK_SEL;
> +               } else if (blink == 1) {
> +                       priv->blink = 1;
> +                       val |= LED_BLINK_SEL;
> +               }
> +
> +               off_event = dev_read_u32_default(dev, "off-event", 3);
> +               val &= ~(LED_EVENT_OFF_MASK << LED_EVENT_OFF_OFFSET);
> +               if (off_event != 3) {
> +                       priv->off_event = off_event;
> +                       val |= BIT(off_event) << LED_EVENT_OFF_OFFSET;
> +               }
> +
> +               blink_event = dev_read_u32_default(dev, "blink-event", 3);
> +               val &= ~(LED_EVENT_BLINK_MASK << LED_EVENT_BLINK_OFFSET);
> +               if (blink_event != 3) {
> +                       priv->blink_event = blink_event;
> +                       val |= BIT(blink_event) << LED_EVENT_BLINK_OFFSET;
> +               }
> +
> +               on_event = dev_read_u32_default(dev, "on-event", 3);
> +               val &= ~(LED_EVENT_ON_MASK << LED_EVENT_ON_OFFSET);
> +               if (on_event != 3) {
> +                       priv->on_event = on_event;
> +                       val |= BIT(on_event) << LED_EVENT_ON_OFFSET;
> +               }
> +
> +               port = dev_read_u32_default(dev, "port", 0);
> +               priv->port = port;
> +               val &= ~(LED_PORT_MASK << LED_PORT_OFFSET);
> +               val |= port << LED_PORT_OFFSET;
> +
> +               /* force off */
> +               val &= ~(LED_OFF_ON_MASK << LED_OFF_ON_OFFSET);
> +               val |= 0x3 << LED_OFF_ON_OFFSET;
> +
> +               cortina_led_write(priv->regs, val);
> +       }
> +
> +       return 0;
> +}
> +
> +static int cortina_led_bind(struct udevice *parent)
> +{
> +       ofnode node;
> +
> +       dev_for_each_subnode(node, parent) {
> +               struct led_uc_plat *uc_plat;
> +               struct udevice *dev;
> +               const char *label;
> +               int ret;
> +
> +               label = ofnode_read_string(node, "label");
> +               if (!label) {
> +                       debug("%s: node %s has no label\n", __func__,
> +                             ofnode_get_name(node));
> +                       return -EINVAL;
> +               }
> +
> +               ret = device_bind_driver_to_node(parent, "ca-leds",
> +                                                ofnode_get_name(node),
> +                                                node, &dev);

If you have your own binding, could you add a compatible string for
the child nodes so that DM can bind drivers automatically?

> +               if (ret)
> +                       return ret;
> +               uc_plat = dev_get_uclass_platdata(dev);
> +               uc_plat->label = label;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id ca_led_ids[] = {
> +       { .compatible = "cortina,ca-leds" },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(cortina_led) = {
> +       .name = "ca-leds",
> +       .id = UCLASS_LED,
> +       .of_match = ca_led_ids,
> +       .bind = cortina_led_bind,
> +       .probe = cortina_led_probe,
> +       .priv_auto_alloc_size = sizeof(struct cortina_led_cfg),
> +       .ops = &cortina_led_ops,
> +};
> --
> 2.7.4
>

Regards,
Simon