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/bind...
+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) >> 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_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@denx.de