Hi Marek,
> On 6/17/19 3:01 PM, Lukasz Majewski wrote:
> > 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.
>
> Linux is continuously improving, so is U-Boot, code is being
> constantly rewritten and improved. So this isn't really a convincing
> argument.
>
> > 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?
>
> It also adds churn into the driver which does not have to be there. In
> fact, DT is a hardware description, it should describe hardware and it
> has facilities to do so -- gpio-ranges.
> Will you keep adding more and
> more new macros into the code with every new/old iteration of this
> GPIO block ?
>
> If you were to put that information into DT, where it belongs, the
> driver would be much simple(r) and wouldn't grow unnecessarily.
>
> > 3. It is easily reusable in SPL.
>
> And with gpio-ranges it's not ?
>
> >> 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 :-).
>
> Linux GPIO maintainer is real friendly.
> >>> 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.
>
> See above -- add support for it and it'd even simplify the driver.
>
> >>
> >> [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.
> Are there any users which will actually have to use the old stuff ? If
> not, just don't add it.
>
> [...]
> To have a consensus regarding the gpio-ranges and mxs_gpio - here is my proposition: 1. I can provide "gpio-ranges" properties to imx28-u-boot.dtsi With support similar to one from drivers/gpio/gpio-rcar.c, which only extracts the info regarding pin mapping, but omits the phandle for pinmux (pinmux will not be supported). 2. I will not upstream those properties to Linux (and adjust the working mxs gpio driver in any way) - they would be only U-Boot specific 3. The IMX_GPIO_NR() macro will not be defined for i.MX28 GPIO. 4. Considering point 3 - all converted boards will have to use dm_gpio* API. 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
pgpjWH139GJ8p.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
