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

Mon Jun 17 10:20:17 UTC 2019

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 at 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 , and submit patch for mainline
Linux, it's easy.

> 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 . Read the
documentation for it at [2] . It even explicitly states it's used for
interaction between pincontrol and gpio controllers.

[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.

>>> +#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,
Marek Vasut