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

[Dong Aisheng]

On Wed, Aug 15, 2012 at 03:51:17PM +0800, Uwe Kleine-König wrote:
> 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 at lists.ozlabs.org)
> > > 
> > > On Wed, Aug 15, 2012 at 11:59:18AM +0800, Dong Aisheng wrote:
> > > > On 15 August 2012 03:37, Matt Sealey <matt at genesi-usa.com> wrote:
> > > > > On Tue, Aug 14, 2012 at 2:20 AM, Dong Aisheng <dong.aisheng at linaro.org> wrote:
> > > > >> On 13 August 2012 23:12, Matt Sealey <matt at genesi-usa.com> 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.
>  
For such case, we should add it to the end since the exist ids are already
taken. It will not break any user.

> > > 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.)
> 
Hmm, not sure if dt macro may support this kind of syntax but yes if it supports.
Grant,
Do you know if dt macro can support it?

Now I'm a bit intend to admit that we probably could do like that if it supports.
The benefit i see is that it could save much code lines in driver while having
no using experience downgrade.

The left question is that how do we get the pin id from this kind of format data
(0x32c, 0x008, 7, 0x0, 0), probably one way may be:
For normal pads and NO_CONFIG pads, we could get it via:
mux_reg_offset / 4
for NO_MUX pads but have CONFIG pads, we may get it via:
mux_reg_offset / 4 + PIN_NO_MUX_ID_BASE + config_reg_offset /4
That may fix getting pin id issue from dt.

Another known issue is that via this way, that means the pinctrl subsystem
can only see the using pads, this is a bit not align with the pinctrl
subsystem design. No sure if Linus would like to see it.

Regards
Dong Aisheng