[U-Boot] [PATCH 1/6] mips: mscc_sgpio: Add the MSCC serial GPIO device (SIO)

[Lars.Povlsen at microchip.com]

Hi Daniel!

Thank you for the input. I am about to have a new v2 ready.

Comments below.

---Lars

> -----Original Message-----
> From: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> Sent: Thursday, December 20, 2018 21:53
> To: Lars Povlsen - M31675 <Lars.Povlsen at microchip.com>; u-
> boot at lists.denx.de
> Cc: gregory.clement at bootlin.com; Horatiu Vultur - M31836
> <Horatiu.Vultur at microchip.com>
> Subject: Re: [PATCH 1/6] mips: mscc_sgpio: Add the MSCC serial GPIO
> device (SIO)
> 
> 
> 
> Am 20.12.18 um 21:11 schrieb Lars Povlsen:
> > This add support for the the MSCC serial GPIO driver in MSCC
> > VCoreIII-based SOCs.
> >
> > By using a serial interface, the SIO controller significantly extends
> > the number of available GPIOs with a minimum number of additional pins
> > on the device. The primary purpose of the SIO controller is to connect
> > control signals from SFP modules and to act as an LED controller.
> >
> > This adds the base driver.

[snip]

> > diff --git a/drivers/gpio/mscc_sgpio.c b/drivers/gpio/mscc_sgpio.c
> > new file mode 100644
> > index 0000000000..28f60498fd
> > --- /dev/null
> > +++ b/drivers/gpio/mscc_sgpio.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> > +/*
> > + * Microsemi SoCs serial gpio driver
> > + *
> > + * Author: <lars.povlsen at microchip.com>
> > + * License: Dual MIT/GPL
> 
> this line is redundant due to the SPDX identifier

Removed.

> 
> > + * Copyright (c) 2018 Microsemi Corporation
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/gpio.h>
> > +#include <asm/io.h>
> > +#include <errno.h>
> > +#include <clk.h>
> > +
> > +#define MSCC_SGPIOS_PER_BANK	32
> > +#define MSCC_SGPIO_BANK_DEPTH	4
> > +

[snip]

> > +static int mscc_sgpio_probe(struct udevice *dev)
> > +{
> > +	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +	struct mscc_sgpio_priv *priv = dev_get_priv(dev);
> > +	const void *fdt = gd->fdt_blob;
> > +	int err, div_clock = 0, port, node = dev_of_offset(dev);
> > +	u32 val;
> > +	struct clk clk;
> > +
> > +	err = clk_get_by_index(dev, 0, &clk);
> > +	if (!err) {
> > +		err = clk_get_rate(&clk);
> > +		if (!IS_ERR_VALUE(err))
> > +			div_clock = err;
> > +	} else {
> > +		debug("mscc_sgpio: failed to get clock\n");
> > +		return err;
> > +	}
> > +
> > +	priv->props = (const struct mscc_sgpio_props
> *)dev_get_driver_data(dev);
> > +	priv->ports = fdtdec_get_int(fdt, node, "mscc,sgpio-ports",
> 0xFFFFFFFF);
> > +	priv->clock = fdtdec_get_int(fdt, node,
> > +				     "mscc,sgpio-frequency", 12500000);
> > +	if (priv->clock <= 0 || priv->clock > div_clock) {
> > +		printf("mscc_sgpio: Invalid frequency %d\n", priv->clock);
> > +		return -EINVAL;
> > +	}
> > +	priv->bitcount = fdtdec_get_int(fdt, node,
> > +					"mscc,sgpio-bitcount", 2);
> 
> you should use the newer DM API from include/dm/read.h which is more
> readable and you don't need to poke on gd->fdt_blob anymore. This would
> also make your driver live-tree compatible in case you want to enable
> that in the future.

Now using dev_read_u32_default() instead.

> 
> > +	if (priv->bitcount < 1 || priv->bitcount > 4) {
> > +		printf("mscc_sgpio: bit count %d\n", priv->bitcount);
> > +		return -EINVAL;
> > +	}
> > +	priv->regs = (u32 __iomem *)devfdt_get_addr(dev);
> 
> don't you need some mapping to a virtual address? Or is this mapped
> through TLB? Actually the address from device tree should be a physical
> one.

There is a 1:1 phys to virtual TLB mapping. Also, most of the other GPIO
drivers do it the same, but I added a map_physmem() call.

---Lars

> 
> > +	uc_priv->gpio_count = MSCC_SGPIOS_PER_BANK * priv->bitcount;
> > +	uc_priv->bank_name = "sgpio";
> > +
> > +	sgpio_clrsetbits(priv, REG_SIO_CONFIG, 0,
> > +			 MSCC_M_CFG_SIO_PORT_WIDTH(priv),
> > +			 MSCC_F_CFG_SIO_PORT_WIDTH(priv, priv->bitcount - 1) |
> > +			 MSCC_M_CFG_SIO_AUTO_REPEAT(priv));
> > +	val = div_clock / priv->clock;
> > +	debug("probe: div-clock = %d KHz, freq = %d KHz, div = %d\n",
> > +	      div_clock / 1000, priv->clock / 1000, val);
> > +	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0,
> > +			 MSCC_M_CLOCK_SIO_CLK_FREQ(priv),
> > +			 MSCC_F_CLOCK_SIO_CLK_FREQ(priv, val));
> > +
> > +	for (port = 0; port < 32; port++)
> > +		sgpio_writel(priv, 0, REG_PORT_CONFIG, port);
> > +	sgpio_writel(priv, priv->ports, REG_PORT_ENABLE, 0);
> > +
> > +	debug("probe: sgpio regs = %p\n", priv->regs);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops mscc_sgpio_ops = {
> > +	.direction_input	= mscc_sgpio_direction_input,
> > +	.direction_output	= mscc_sgpio_direction_output,
> > +	.get_function		= mscc_sgpio_get_function,
> > +	.get_value		= mscc_sgpio_get_value,
> > +	.set_value		= mscc_sgpio_set_value,
> > +};
> > +
> > +static const struct udevice_id mscc_sgpio_ids[] = {
> > +	{ .compatible = "mscc,luton-sgpio", .data = (ulong)&props_luton },
> > +	{ .compatible = "mscc,ocelot-sgpio", .data = (ulong)&props_ocelot
> },
> > +	{ }
> > +};
> > +
> > +U_BOOT_DRIVER(gpio_mscc_sgpio) = {
> > +	.name			= "mscc-sgpio",
> > +	.id			= UCLASS_GPIO,
> > +	.of_match		= mscc_sgpio_ids,
> > +	.ops			= &mscc_sgpio_ops,
> > +	.probe			= mscc_sgpio_probe,
> > +	.priv_auto_alloc_size	= sizeof(struct mscc_sgpio_priv),
> > +};
> >
> 
> --
> - Daniel