[U-Boot] [PATCH v3 3/5] ARM: dm: mxs: gpio: Add support for DM/DTS in the mxs_gpio.c driver (DM_GPIO)

[Lukasz Majewski <lukma@denx.de>]

Hi Marek,

> On 6/16/19 12:34 AM, Lukasz Majewski wrote:
> > This commit  
> 
> This is not a commit, it's a change. It only becomes a commit when
> applied.
> 
> > adds support for DM in the mxs_gpio.c driver when DM_GPIO
> > is enabled in Kconfig.  
> 
> But this also adds support for DT probing, which is orthogonal to DM
> support, yet it's not documented in the commit message.

Ok. Will rewrite the commit message to reflect the changes in the
commit.

> 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > 
> > ---
> > 
> > Changes in v3:
> > - Set more apropriate tags
> > 
> > Changes in v2:
> > - Use #if !CONFIG_IS_ENABLED(DM_GPIO) instead of plain #ifdef
> > CONFIG_DM_GPIO
> > 
> >  arch/arm/include/asm/arch-mxs/gpio.h |  28 +++++++
> >  drivers/gpio/mxs_gpio.c              | 149
> > +++++++++++++++++++++++++++++++++++ 2 files changed, 177
> > insertions(+)
> > 
> > diff --git a/arch/arm/include/asm/arch-mxs/gpio.h
> > b/arch/arm/include/asm/arch-mxs/gpio.h index 34fa421945..0c152e25e2
> > 100644 --- a/arch/arm/include/asm/arch-mxs/gpio.h
> > +++ b/arch/arm/include/asm/arch-mxs/gpio.h
> > @@ -15,4 +15,32 @@ void mxs_gpio_init(void);
> >  inline void mxs_gpio_init(void) {}
> >  #endif
> >  
> > +#if defined(CONFIG_MX28) && CONFIG_IS_ENABLED(DM_GPIO)
> > +/*
> > + * According to i.MX28 Reference Manual:
> > + * 'i.MX28 Applications Processor Reference Manual, Rev. 1, 2010'
> > + * The i.MX28 has following number of GPIOs available:
> > + * Bank 0: 0-28 -> 29 PINS
> > + * Bank 1: 0-31 -> 32 PINS
> > + * Bank 2: 0-27 -> 28 PINS
> > + * Bank 3: 0-30 -> 31 PINS
> > + * Bank 4: 0-20 -> 21 PINS
> > + */
> > +#define IMX28_GPIO_BANK0_PIN_NR 29
> > +#define IMX28_GPIO_BANK1_PIN_NR 32
> > +#define IMX28_GPIO_BANK2_PIN_NR 28
> > +#define IMX28_GPIO_BANK3_PIN_NR 31
> > +#define IMX28_GPIO_BANK4_PIN_NR 21
> > +#define IMX28_GPIO_BANK_NR 5  
> 
> Please make a complete conversion, not partial one.
> You want to use gpio-ranges in DT and then parse the size of each GPIO
> bank from those gpio-ranges , not hard-code it into the driver.

I would have used the gpio-ranges, but the original Linux code [1]
(imx28.dtsi) doesn't define them.

Also, the dts files which include [1] don't extend original gpio nodes
to have one.

As it is not available in the Linux kernel, I don't see any good reason
to add the gpio-ranges to U-Boot. The same approach, as presented here,
is used in zynq_gpio.c file (which also uses DM/DTS).

Adding per u-boot property - like 'u-boot,mx28-gpio-ranges' is also less
appealing than 24 lines of code, which can be then easily re-used with
OF_PLATDATA (without DM/DTS suport) in SPL (u-boot.sb to be precise).


> 
> > +struct mxs_gpio_platdata {
> > +	u32 gpio_nr_of_bank_pins[IMX28_GPIO_BANK_NR];
> > +	u32 gpio_base_nr[IMX28_GPIO_BANK_NR];
> > +	u32 ngpio;
> > +};
> > +
> > +extern const struct mxs_gpio_platdata mxs_gpio_def;
> > +#define IMX_GPIO_NR(port, index)
> > (mxs_gpio_def.gpio_base_nr[(port)] + (index))  
> 
> So this should be completely unnecessary when using the DM GPIO, this
> was only needed for non-DM-GPIO .

This is a compatibility layer - for some reason ALL iMX ports define it
- i.e. imx8, imx7 - which shall use DM/DTS for gpio from the outset.

With the in-board code the dm_gpio_* set of functions shall (and
will) be used (as done in opos6ul.c file).

> 
> > +#endif
> > +
> >  #endif	/* __MX28_GPIO_H__ */
> > diff --git a/drivers/gpio/mxs_gpio.c b/drivers/gpio/mxs_gpio.c
> > index c2c8a25886..f386235ff1 100644
> > --- a/drivers/gpio/mxs_gpio.c
> > +++ b/drivers/gpio/mxs_gpio.c
> > @@ -51,6 +51,7 @@ void mxs_gpio_init(void)
> >  	}
> >  }
> >  
> > +#if !CONFIG_IS_ENABLED(DM_GPIO)
> >  int gpio_get_value(unsigned gpio)
> >  {
> >  	uint32_t bank = PAD_BANK(gpio);
> > @@ -127,3 +128,151 @@ int name_to_gpio(const char *name)
> >  
> >  	return (bank << MXS_PAD_BANK_SHIFT) | (pin <<
> > MXS_PAD_PIN_SHIFT); }
> > +#else /* CONFIG_DM_GPIO */
> > +#include <dm.h>
> > +#include <asm/gpio.h>
> > +#include <asm/arch/gpio.h>
> > +#ifdef CONFIG_MX28
> > +const struct mxs_gpio_platdata mxs_gpio_def = {
> > +	.gpio_nr_of_bank_pins[0] = IMX28_GPIO_BANK0_PIN_NR,
> > +	.gpio_nr_of_bank_pins[1] = IMX28_GPIO_BANK1_PIN_NR,
> > +	.gpio_nr_of_bank_pins[2] = IMX28_GPIO_BANK2_PIN_NR,
> > +	.gpio_nr_of_bank_pins[3] = IMX28_GPIO_BANK3_PIN_NR,
> > +	.gpio_nr_of_bank_pins[4] = IMX28_GPIO_BANK4_PIN_NR,
> > +	.gpio_base_nr[0] = 0,
> > +	.gpio_base_nr[1] = IMX28_GPIO_BANK0_PIN_NR,
> > +	.gpio_base_nr[2] = (IMX28_GPIO_BANK0_PIN_NR + \
> > +			   IMX28_GPIO_BANK1_PIN_NR),
> > +	.gpio_base_nr[3] = (IMX28_GPIO_BANK0_PIN_NR + \
> > +			   IMX28_GPIO_BANK1_PIN_NR + \
> > +			   IMX28_GPIO_BANK2_PIN_NR),
> > +	.gpio_base_nr[4] = (IMX28_GPIO_BANK0_PIN_NR + \
> > +			   IMX28_GPIO_BANK1_PIN_NR + \
> > +			   IMX28_GPIO_BANK2_PIN_NR + \
> > +			   IMX28_GPIO_BANK3_PIN_NR),
> > +	.ngpio = (IMX28_GPIO_BANK0_PIN_NR + \
> > +		  IMX28_GPIO_BANK1_PIN_NR + \
> > +		  IMX28_GPIO_BANK2_PIN_NR + \
> > +		  IMX28_GPIO_BANK3_PIN_NR + \
> > +		  IMX28_GPIO_BANK4_PIN_NR),
> > +};  
> 
> So please use gpio-ranges in DT.

Please see the above comment regarding gpio-ranges.

> 
> > +#else
> > +#error "Only i.MX28 supported with DM_GPIO"
> > +#endif
> > +
> > +struct mxs_gpio_priv {
> > +	unsigned int bank;
> > +};
> > +
> > +static int mxs_gpio_get_value(struct udevice *dev, unsigned offset)
> > +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct mxs_register_32 *reg =
> > +		(struct mxs_register_32 *)(MXS_PINCTRL_BASE +
> > +
> > PINCTRL_DIN(priv->bank)); +
> > +	return (readl(&reg->reg) >> offset) & 1;
> > +}
> > +
> > +static int mxs_gpio_set_value(struct udevice *dev, unsigned offset,
> > +			      int value)
> > +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct mxs_register_32 *reg =
> > +		(struct mxs_register_32 *)(MXS_PINCTRL_BASE +
> > +
> > PINCTRL_DOUT(priv->bank));
> > +	if (value)
> > +		writel(1 << offset, &reg->reg_set);  
> 
> BIT(), fix globally.

I took it from the original implementation :-).

Ok. I will fix it and replace 1 << offset with BIT().

> 
> > +	else
> > +		writel(1 << offset, &reg->reg_clr);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mxs_gpio_direction_input(struct udevice *dev, unsigned
> > offset) +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct mxs_register_32 *reg =
> > +		(struct mxs_register_32 *)(MXS_PINCTRL_BASE +
> > +
> > PINCTRL_DOE(priv->bank)); +
> > +	writel(1 << offset, &reg->reg_clr);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mxs_gpio_direction_output(struct udevice *dev, unsigned
> > offset,
> > +				     int value)
> > +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct mxs_register_32 *reg =
> > +		(struct mxs_register_32 *)(MXS_PINCTRL_BASE +
> > +
> > PINCTRL_DOE(priv->bank)); +
> > +	mxs_gpio_set_value(dev, offset, value);
> > +
> > +	writel(1 << offset, &reg->reg_set);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mxs_gpio_get_function(struct udevice *dev, unsigned
> > offset) +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct mxs_register_32 *reg =
> > +		(struct mxs_register_32 *)(MXS_PINCTRL_BASE +
> > +
> > PINCTRL_DOE(priv->bank));
> > +	bool is_output = !!(readl(&reg->reg) >> offset);
> > +
> > +	return is_output ? GPIOF_OUTPUT : GPIOF_INPUT;
> > +}
> > +
> > +static const struct dm_gpio_ops gpio_mxs_ops = {
> > +	.direction_input	= mxs_gpio_direction_input,
> > +	.direction_output	= mxs_gpio_direction_output,
> > +	.get_value		= mxs_gpio_get_value,
> > +	.set_value		= mxs_gpio_set_value,
> > +	.get_function		= mxs_gpio_get_function,
> > +};
> > +
> > +static int mxs_gpio_probe(struct udevice *dev)
> > +{
> > +	struct mxs_gpio_priv *priv = dev_get_priv(dev);
> > +	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +	struct mxs_gpio_platdata *pdata =
> > +		(struct mxs_gpio_platdata
> > *)dev_get_driver_data(dev);
> > +	char name[18], *str;
> > +	int ret;
> > +
> > +	ret = dev_read_u32(dev, "reg", &priv->bank);  
> 
> devfdt_get_addr()

Good point - thanks.

> 
> > +	if (ret) {
> > +		printf("%s: No 'reg' property defined!\n",
> > __func__);
> > +		return ret;
> > +	}
> > +
> > +	sprintf(name, "GPIO%d_", priv->bank);  
> 
> Name cannot conceivably have more than 16 characters, why is the array
> 18 characters large ? 

It shall be 16. 

> Also, snprintf() would likely be better here.

Ok, I will replace it.

> 
> > +	str = strdup(name);
> > +	if (!str)
> > +		return -ENOMEM;
> > +
> > +	uc_priv->bank_name = str;
> > +	uc_priv->gpio_count =
> > pdata->gpio_nr_of_bank_pins[priv->bank]; +
> > +	return 0;
> > +}
> > +
> > +static const struct udevice_id mxs_gpio_ids[] = {
> > +	{ .compatible = "fsl,imx28-gpio", .data =
> > (ulong)&mxs_gpio_def },
> > +	{ }
> > +};
> > +
> > +U_BOOT_DRIVER(gpio_mxs) = {
> > +	.name	= "gpio_mxs",
> > +	.id	= UCLASS_GPIO,
> > +	.ops	= &gpio_mxs_ops,
> > +	.probe	= mxs_gpio_probe,
> > +	.priv_auto_alloc_size = sizeof(struct mxs_gpio_priv),
> > +	.of_match = mxs_gpio_ids,
> > +};
> > +#endif /* CONFIG_DM_GPIO */
> >   
> 
> 

Note:

[1] -
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx28.dtsi


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190617/fc4b1d64/attachment.sig>