[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 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 <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.  
> 
> 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: 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/20190618/1ede9e67/attachment.sig>