Re: [PATCH] pinctrl: imx5: start numbering pad from 0

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Wed, Aug 15, 2012 at 02:55:27PM +0800, Dong Aisheng wrote:
> On Wed, Aug 15, 2012 at 07:44:36AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > (Cc += devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org)
> > 
> > On Wed, Aug 15, 2012 at 11:59:18AM +0800, Dong Aisheng wrote:
> > > On 15 August 2012 03:37, Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org> wrote:
> > > > On Tue, Aug 14, 2012 at 2:20 AM, Dong Aisheng <dong.aisheng@linaro.org> wrote:
> > > >> On 13 August 2012 23:12, Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org> wrote:
> > > >>> I have a minor nit; while renumbering the pinctrl definitions is
> > > >>> laudable and device trees can match, there are 16 other pin controls
> > > >>> below EIM_D16 - this makes the start of the IOMUXC pin definitions
> > > >>> actually be the value calculated by (iomuxc_base + 0x300 + (4*16) +
> > > >>> (pin_id*4)).
> > > >>
> > > >> What do you mean of this value calculated?
> > > >
> > > > Well, it just seems the datasheet has been read from the point of view
> > > > of the SW_CTRL settings (pad pullups, drive strength etc.) to define
> > > > the enums; every pin setting in order from IOMUXC_BASE + 0x3f0 (sorry
> > > > I mistyped) has been entered as an enum into this list. Then a table
> > > > has been generated using these enums to supply a pin id. Then this pin
> > > > id is not relevant anymore; it actually gets referenced by it's
> > > > *offset into the table*.
> > > >
> > > Not exactly, probably.
> > > Basically we follow the mux reigster offset to define pin id, however,
> > > there're also some pads may not have mux function but only config
> > > which also has a pin id
> > > and vice versa.
> > > Those pins are not ordered by it's mux regsiter offset.
> > > 
> > > Actually I did not manually search all those pins, instead, i just
> > > using the exist ones:
> > > arch/arm/plat-mxc/include/mach/iomux-mx51.h
> > > Since this exist headfile already covers this case.
> > > 
> > > > In this case, basically the code misses a few pins which have ALT
> > > > settings but perhaps not always the ability to change pad control -
> > > > EIM_DA5 to 15 are a good example.
> > > >
> > > As i said, we already defined those pin ids.
> > > 
> > > > Probably better would be to define the binding almost exactly as it
> > > > was done in the old method; iomux_v3_cfg_t took into account the three
> > > > register offsets possible for the pin (SW_MUX_CTL, SW_PAD_CTL and IPP)
> > > > and the three values to be put into those registers.
> > > >
> > > > This has been reduced down in pinctrl to what I find quite strange,
> > > > which is a completely arbitrary pin "id" mapping, which indirectly
> > > > references an entry in a table by index which is why pin 1 as a start
> > > > looks odd.
> > > >
> > > Start with pin 1 is a bug which caused by my mistake during the early
> > > data conversion.
> > > 
> > > > If the binding is entirely i.MX-specific or even i.MX51-specific then
> > > > why not define the pins in the device tree the "old" iomux_v3_cfg_t
> > > > way, which is the way they are in the table of imx_pin_regs;
> > > >
> > > >       IMX_PIN_REG(MX51_PAD_EIM_D16, 0x3f0, 0x05c, 5, 0x000, 0), /*
> > > > MX51_PAD_EIM_D16__AUD4_RXFS */
> > > >
> > > > 0x3f0 is the SW_PAD_CTL, 0x05c is the SW_MUX_CTL, 5 is the ALT mode,
> > > > 0x000 is the IPP reg, and 0 the IPP value.
> > > >
> > > > So fsl,iomux-pins = <0x3f0 0x85 0x05c 5 0 0 ... > (PAD, settings, MUX,
> > > > mode/sion, IPP, select - in that order) would be a valid pin
> > > > definition and require no table. Sure, the offsets are device specific
> > > > but then the arbitrary numbering is too.
> > > > This way old iomux controls
> > > > can be re-derived for people who use the old method in old kernels, or
> > > > use U-Boot with iomux-v3.h taken from Linux (MX6 got ported, I am
> > > > porting the Efika to it on MX5 for U-Boot right now). Also that huge
> > > > table goes away - it can be built at runtime, along with the
> > > > PAD_NAME__ALT_MODE_NAME description.
> > > >
> > > I can't agree this is better than the exist one now.
> > > It's hard to use in device tree.
> > > And how would you parse the pin id from this data array?
> > I admit I didn't follow completely the calculation suggestion by Matt,
> > but I think in general he has a point. This is more or less what I
> > thought when I built the imx35 pinctrl driver.
> > 
> > With the current approach (i.e. put the pinfuncs in some order that
> > seems sensible today and then referencing them by index according to
> > that ordering) you really get into troubles if you have to add a new
> > function. You have to add it to the end to not break existing device
> > trees and so the ordering is broken.
> > 
> Actually, we do not mean to maintain the ordering in driver.
> It just needs to be align with the pin func ids definitions in binding
> doc. So the ordering broken in driver is not an issue, it just looks
> not so better if we really have to add a new pin function to the end
> due to mistake but this does not break anything to work.
Well, if you add a function between say current function 11 and 12 all
device trees need fixing, i.e. increase all function ids >= 12 by one.
You obviously cannot fix out-of-tree users and even fixing the in-tree
users is ugly.
 
> > Also "11" has no obvious relation to MX35_PAD_COMPARE__SDMA_EXTDMA_2
> > other than a match if you grep over the binding docs (or the driver).
> > While "0x32c, 0x008, 7, 0x0, 0" is longer and looks more ugly the
> > mapping is obvious and so IMHO preferable.
> > 
> Also, don't you think this is hard to use for devicetree?
Not harder than the current approach. Obviously macro support would be
nice, but this applies to both approaches.

> And we need to reference datasheet to fill these values.
I'd list the explicit values in the binding docs as the indexes are
documented there now. So the difference for a device-tree writer is just
the content that is copy-and-pasted.

> I just keep using as the old way at best and i remember Sascha also suggested
> before that we don't want to lose using the macro way as
> MX35_PAD_COMPARE__SDMA_EXTDMA_2 to make easy use.
> In the future, we will switch to macro when dt supports.
> For the way "0x32c, 0x008, 7, 0x0, 0", it's very unconveniently to use macro.
That is just

	#define MX35_PAD_COMPARE__SDMA_EXTDMA_2 11

vs.

	#define MX35_PAD_COMPARE__SDMA_EXTDMA_2 0x32c, 0x008, 7, 0x0, 0

isn't it? (Actually I'd not go for cpp as preprocessor, but that's a
different story.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |