[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/17/19 10:37 AM, Lukasz Majewski wrote:
> > 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.  
> 
> You can add them to imx28-u-boot.dtsi , 

No, IMHO this is not the best solution. My point is:

1. i.MX28 driver in Linux is stable and it works (without gpio-ranges).
I do not have any interest in fixing code which is already working. If
that were new driver then no issue to use gpio-ranges from the outset.
Also if Linux kernel driver would support it - then also no problems
with adding it.


2. The proposed code is small - only 24 LOC and doesn't require any
extra parsing of DTS (including pinctrl driver's properties).

Why shall I make the driver more verbose if I can avoid it?

3. It is easily reusable in SPL.

> and submit patch for mainline
> Linux, it's easy.

Submitting patches to Linux is easy - but to have them already accepted
and pulled is not :-).

> 
> > 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).  
> 
> It is well possible the zynq DTs predate the gpio-ranges .

No, it is not.

> Read the
> documentation for it at [2] . It even explicitly states it's used for
> interaction between pincontrol and gpio controllers.

In those cases where both support it. The i.MX28 Linux GPIO driver is
NOT supporting gpio-ranges.

> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/gpio/gpio.txt#L239
> 
> >>> +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).  
> 
> Then use them and drop this.

I will use the new dm_gpio_* API where applicable. However, to be in
sync with other iMX ports the IMX_GPIO_NR() shall be also defined.

> 
> >>> +#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.  
> 
> DTTO
> 
> >>> +#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 :-).  
> 
> Original implementation is very old code.

:-)

> 
> [...]
> 




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/a02231d2/attachment.sig>