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 <lu...@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) >> 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: lu...@denx.de
pgpv8yRNOICAq.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
