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

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

[Uwe Kleine-König]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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.

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.

Best regards
Uwe

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

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

[Dong Aisheng]

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-QSEj5FYQhm5QFI55V6+gNQ@public.gmane.orgg> 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.

> 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?
And we need to reference datasheet to fill these values.

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.

Regards
Dong Aisheng

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

[Uwe Kleine-König]

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/  |

Re: [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-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.
>  
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

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

[Dong Aisheng]

On 15 August 2012 17:25, Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 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-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.
>>
> 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?
>
Sorry, forget to add Grant.
Cc Grant.

Regards
Dong Aisheng

> 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
>

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

[Shawn Guo]

On Wed, Aug 15, 2012 at 05:25:40PM +0800, Dong Aisheng wrote:
> On Wed, Aug 15, 2012 at 03:51:17PM +0800, Uwe Kleine-König wrote:
> > 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?
> 
I have the same doubt there.  Copied Stephen who might have a better
insight on this.

> 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

In case of NO_MUX, mux_reg_offset is 0, so the formula becomes:

	PIN_NO_MUX_ID_BASE + config_reg_offset / 4

The question comes to how PIN_NO_MUX_ID_BASE gets determined?

> 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.
> 
I do not quite follow on this.  The "enum imx51_pads" will still be
there, so all the pads will still be visible to the pinctrl system.

-- 
Regards,
Shawn

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

[Shawn Guo]

On Wed, Aug 15, 2012 at 09:59:50PM +0800, Shawn Guo wrote:
> On Wed, Aug 15, 2012 at 05:25:40PM +0800, Dong Aisheng wrote:
> > On Wed, Aug 15, 2012 at 03:51:17PM +0800, Uwe Kleine-König wrote:
> > > 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?
> > 
> I have the same doubt there.  Copied Stephen who might have a better
> insight on this.
> 
Rather than betting how DTC will implement macro, we'd better make the
the safest assumption - it will not support that syntax.  But we can
still work our issue, I guess.  Actually, the goal is all about
encoding the data that is currently defined in driver as a big array of
struct imx_pin_reg in device tree.

struct imx_pin_reg {
        u16 pid;
        u16 mux_reg;
        u16 conf_reg;
        u8 mux_mode;
        u16 input_reg;
        u8 input_val;
};

As we will figure out the pid from the mux_reg and conf_reg as below,
it becomes how we encode other fields.  An u64 can just cover them.
That said, the line below in binding doc

  MX35_PAD_COMPARE__SDMA_EXTDMA_2 11

becomes 

  MX35_PAD_COMPARE__SDMA_EXTDMA_2 0x032c000807000000

We should probably have it be mux_reg + mux_mode + conf_reg +
input_reg + input_val or something to have the offset and value coupled.
Anyway, we will still have fsl,pins formatted as <PIN_FUNC_ID CONFIG>,
and only difference is PIN_FUNC_ID becomes an u64 integer.  But it can
save us that big array from the driver.

> > 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
> 
> In case of NO_MUX, mux_reg_offset is 0, so the formula becomes:
> 
> 	PIN_NO_MUX_ID_BASE + config_reg_offset / 4
> 
> The question comes to how PIN_NO_MUX_ID_BASE gets determined?
> 
So PIN_NO_MUX_ID_BASE will be: the largest mux_reg_offset / 4 + 1, right?

-- 
Regards,
Shawn

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

[Stephen Warren]

On 08/15/2012 07:59 AM, Shawn Guo wrote:
> On Wed, Aug 15, 2012 at 05:25:40PM +0800, Dong Aisheng wrote:
>> On Wed, Aug 15, 2012 at 03:51:17PM +0800, Uwe Kleine-König wrote:
>>> 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?
>>
> I have the same doubt there.  Copied Stephen who might have a better
> insight on this.

You mean like C pre-processor on .dts files? There's currently no
support for anything like that at all, with either syntax above. I
honestly don't know if/when there will be.

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

[Matt Sealey]

On Wed, Aug 15, 2012 at 10:31 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Wed, Aug 15, 2012 at 09:59:50PM +0800, Shawn Guo wrote:
>> On Wed, Aug 15, 2012 at 05:25:40PM +0800, Dong Aisheng wrote:
>> > On Wed, Aug 15, 2012 at 03:51:17PM +0800, Uwe Kleine-König wrote:
>> > 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?
>>
>> I have the same doubt there.  Copied Stephen who might have a better
>> insight on this.
>>
> Rather than betting how DTC will implement macro, we'd better make the
> the safest assumption - it will not support that syntax.

It won't. DT is defined by properties and those properties have cells.
Properties
that have more than one cell or multiple definitions in them have to have a
#size-cells property to go with it somehow.

I don't see what the problem would be here; you want to define pin properties,
sure, they are intrinsically defined by 3 possible registers and value pairs. In
actual fact if you drill down to this, what happens is you get a pin property
which defines offsets into the unit, and values to stuff into those
registers, so
you could either dictate a size-cell of 6 (3 pairs of reg-value) or 2
(reg, value).
It would be more convenient for a driver to know what those pairs are maybe,
so 6 is better.

> still work our issue, I guess.  Actually, the goal is all about
> encoding the data that is currently defined in driver as a big array of
> struct imx_pin_reg in device tree.

Absolutely!

> struct imx_pin_reg {
>         u16 pid;
>         u16 mux_reg;
>         u16 conf_reg;
>         u8 mux_mode;
>         u16 input_reg;
>         u8 input_val;
> };
>
> As we will figure out the pid from the mux_reg and conf_reg as below,
> it becomes how we encode other fields.  An u64 can just cover them.

Or 3 pairs of values can encode them. Don't needlessly encode data in
the device tree that can just exist in a cell of a property.

> That said, the line below in binding doc
>
>   MX35_PAD_COMPARE__SDMA_EXTDMA_2 11
>
> becomes
>
>   MX35_PAD_COMPARE__SDMA_EXTDMA_2 0x032c000807000000

I can only give a quick example on MX51, MX53 or MX6 since that's my
experience, but essentially.. you're overthinking it.

What it becomes is

iomuxc@0x73f08000 {
  fsl,iomux-pins = <mux value ctrl value ipp value
/* MX53_PIN_SD4_CMD__SD4_CMD */
                            mux value ctrl value ipp value
       /* etc. */
                            ...>;

You can encode those values *in the driver* as a hash or lookup to get
names for them if you like in the driver.. or build them at runtime
for truly odd combinations or not-usual pin configurations. In any
case, those reg can be "valid" 0 since 0 is IOMUXC_GPR0.. which you do
not want to set. That said, setting IOMUXC_GPR can be useful! Ethernet
may need (on MX6 examples) ENET_CLK_SEL set, IPU may need to select
MIPI gasket or IOMUX. Usually though I would assume that you would
have properties for these though as you do not want to "trash"
bootloader configuration of any clock or mux settings, just update
them (so, fsl,ethernet-clock-sel =1 or fsl,ipu-iomux = <1 0>)

That said, the usual thing is to set them to "-1" explicitly in the
tree and let the DT code sort out what -1 actually means (usually
0xffffffff since by default OF cells are 32-bit, but it's the same
difference if it's a 64-bit device tree binding) since this is totally
invalid. That way a pin with no ctrl or ipp, or ctrl but no ipp or mux
would be

fsl,iomux-pin = <-1 -1 0x05c 0x85 -1 -1
                        ...>;

> We should probably have it be mux_reg + mux_mode + conf_reg +
> input_reg + input_val or something to have the offset and value coupled.
> Anyway, we will still have fsl,pins formatted as <PIN_FUNC_ID CONFIG>,
> and only difference is PIN_FUNC_ID becomes an u64 integer.  But it can
> save us that big array from the driver.

As above. Please do not encode the data. Don't assume you can encode a
64-bit integer either.. that's not in any core binding. What you need
here is a multi-cell property as above. That  way it can be expanded
if MX7 has an extra iomux register set for something else ("enable
awesome mode" or so).

>> The question comes to how PIN_NO_MUX_ID_BASE gets determined?
>>
> So PIN_NO_MUX_ID_BASE will be: the largest mux_reg_offset / 4 + 1, right?

Nope. It should be -1. Do it properly, please. Do not "hack" numbers,
as this can mean the driver has to intimately know the hardware when
it is needless to do so. The driver MAY however  assume that
IOMUXC_BASE_ADDR + 0xffffffff is not really possible - check the
offset generated and if it goes beyond the reg = <start length> range,
it is obviously invalid. Or just check the offset against the length..
walking into unmapped memory invalid is invalid across the board.

One thing though, I don't understand why Dong thinks it would be more
difficult to look up a register number in the docs (PDF search, Dong,
it's easy!) versus looking up the binding number in the binding
document. These pairs are ALREADY encoded in older kernels too in that
64 bit integer. Just split them back out again (there's code in
iomux-v3.h to do this still in the kernel) and you can encode your
device tree. Including headers, macros is unecessary. You may want to
petition Freescale to create a plugin for their IOMUX tool though that
not only spits out the reg/pair values in C source format (this is
very useful, though, since you can paste it into anything that can use
writel() to set up all pins exactly as your board designer dictated),
but also does it in device-tree binding pairs as above. That would
make it a lot easier all round, too.. nobody needs to look up a
binding, the binding is "fsl,iomux-pins defines pairs of offsets and
values inside the IOMUXC unit. These offsets and values are defined in
the manual. <some examples here>"

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Dong Aisheng]

On Wed, Aug 15, 2012 at 09:59:50PM +0800, Shawn Guo wrote:
...
> > 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.
> > 
> I do not quite follow on this.  The "enum imx51_pads" will still be
> there, so all the pads will still be visible to the pinctrl system.
> 
Sorry, i did not describe it accurately.
The problem is that those pins are visible to pinctrl subystem, but the pinctrl
subsystem can not manage them all.
Not sure this meet Linus's original design purpose.
Because for the way proposed, all the pin's basic properties like mux_reg, config_reg
are parsed from device tree at runtime.
If a pin is not used in device tree, the driver can not know this pin's
corresponding registers.
Thus, for those unused pins, we can not manage it on mux or config in pinctrl
subsystem and driver.

For example, the pin_config_get/pin_config_set API in
include/linux/pinctrl/consumer.h can not work for such pins.
Then imx_pinconf_dbg_show in current driver may need change since it does not
support show all pins's config value.
It looks like not a big issue currently since i did not see any client driver
using this API.
But i'm not sure if we may have this requirement in the future.
For example, is it possible that pinctrl subsystem may support configure pins
via sysfs dynamically?

Linus, Stephen,
Any comment on it?

If it supports, will imx driver also support config unused pins in devicetree
via sysfs?
Probably it may be rarely used and imx just does not support it.
Then we can get register data from devicetree.

Regards
Dong Aisheng

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

[Shawn Guo]

On Wed, Aug 15, 2012 at 11:49:01AM -0500, Matt Sealey wrote:
> > Rather than betting how DTC will implement macro, we'd better make the
> > the safest assumption - it will not support that syntax.
> 
> It won't.

There were a lot of pushing on macro support for DTC.  Though it's
going to be very slow, I guess someday we will have it at least for
those simple and straight-forward syntax.

But right, we shouldn't make any assumption on that.

> DT is defined by properties and those properties have cells.
> Properties
> that have more than one cell or multiple definitions in them have to have a
> #size-cells property to go with it somehow.
> 
> I don't see what the problem would be here; you want to define pin properties,
> sure, they are intrinsically defined by 3 possible registers and value pairs. In
> actual fact if you drill down to this, what happens is you get a pin property
> which defines offsets into the unit, and values to stuff into those
> registers, so
> you could either dictate a size-cell of 6 (3 pairs of reg-value) or 2
> (reg, value).
> It would be more convenient for a driver to know what those pairs are maybe,
> so 6 is better.

<snip>

> iomuxc@0x73f08000 {
>   fsl,iomux-pins = <mux value ctrl value ipp value
> /* MX53_PIN_SD4_CMD__SD4_CMD */
>                             mux value ctrl value ipp value
>        /* etc. */
>                             ...>;
> 
I do not see problem with this approach either.  But, this is nothing
IMX specific.  Essentially, this becomes a common binding for
configuring a pin with arbitrary number of register/value pairs.

-- 
Regards,
Shawn

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

[Matt Sealey]

On Wed, Aug 15, 2012 at 10:45 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Aug 15, 2012 at 11:49:01AM -0500, Matt Sealey wrote:
>> > Rather than betting how DTC will implement macro, we'd better make the
>> > the safest assumption - it will not support that syntax.
>>
>> It won't.
>
> There were a lot of pushing on macro support for DTC.  Though it's
> going to be very slow, I guess someday we will have it at least for
> those simple and straight-forward syntax.
>
> But right, we shouldn't make any assumption on that.

I'm not sure you'd want a "programmable" device tree that expanded itself out
using macros. Just imagine what one tiny mistake in syntax could do - stop a
board from booting, for example.

You never ran into this on real OF, you never needed it, why would precompiled
DTs need it?

>> iomuxc@0x73f08000 {
>>   fsl,iomux-pins = <mux value ctrl value ipp value
>> /* MX53_PIN_SD4_CMD__SD4_CMD */
>>                             mux value ctrl value ipp value
>>        /* etc. */
>>                             ...>;
>
> I do not see problem with this approach either.  But, this is nothing
> IMX specific.  Essentially, this becomes a common binding for
> configuring a pin with arbitrary number of register/value pairs.

And indeed, I don't see how this could possibly be a bad thing when
multiple boards can share such a binding and ease device tree
generation by making it a task of "cross reference the manual" or
"read the automated tool output and copy it to the device tree". So
the name changes to something non-fsl-specific or something and all
the boards that support it this way can enable it this way, job done,
common binding for multiple systems?

Side nit: how do you propose in the device tree to enable certain bits
in certain IOMUXC registers on MX6 or so?

The current solution where pinctrl holds IOMUXC to ransom (it ioremaps
the register set until _remove) is fine for now, but what if I want to
change my ENET_CLK_CTL or MIPI_IPU1_MUX or USB_OTG_ID or OCRAM
pipeline control bits in IOMUXC_GPRn for correct board operation? Is
this a bootloader responsibility or device tree? It seems odd to make
pad mux and control a device tree job, but internal path or unit setup
a bootloader job.

If we're doing this to configure the correct internal paths in the
SoC, the "correct" place might be in the ethernet@ node, or ipu-csi@
node as they define the operation and paths of these units, but they
would be better served to be defined at the pinctrl level and not at
the unit, since pinctrl "owns" IOMUXC. But having iomuxc@foo contain a
lot of very strange internal path definitions seems too odd and
cluttered.

In this case, pinctrl-imx implementation is too generic in claiming
this entire unit for the purpose. Probably what needs to be done is
fsl,imx6q-iomuxc gets claimed by an MFD which allows setting these
feature bits, and pinctrl simply depends on it to implement a pinctrl@
node underneath it (like regulators are done now).

As for requiring drivers to fetch and use pinctrl data without
possibility of dummies to allow it to "fail gracefully", I would say
pinctrl in i.MX drivers at least should not be a requirement - by all
means, search for the pin definitions, instruct pinctrl to perform
these operations, but if it does not find a compatible property..
carry on regardless, maybe with a warning that you MUST have trusted
your bootloader. Or define a linux,pinctrl-already-done property (this
would be useful for MX51 uart1 which is muxed to ALT0 by default, so
pinctrl needn't bother doing anything - it was always there and always
correctly configured from mask rom time up to Linux anyway..) and as
such we can omit the warning and just carry on regardless.

-- 
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.

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

[Matt Sealey]

On Wed, Aug 15, 2012 at 10:30 PM, Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Wed, Aug 15, 2012 at 09:59:50PM +0800, Shawn Guo wrote:
> ...
>> > 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.
>> >
>> I do not quite follow on this.  The "enum imx51_pads" will still be
>> there, so all the pads will still be visible to the pinctrl system.
>
> Sorry, i did not describe it accurately.
> The problem is that those pins are visible to pinctrl subystem, but the pinctrl
> subsystem can not manage them all.

Why not? It only needs to "manage" the ones the DT defines or the ones
you specify.

The rest of the data is there as a convenience for debug and naming of
pins, a cross-reference that could be compiled out for non-debug
kernels since there's not a lot of point wasting 64kb or possibly more
of memory on naming every pin on the chip statically. The name of the
pad is in pin_desc and can be marked dynamic (a small function at
get_pin_desc can work out the string and vsprintf the correct string,
and put_pin_desc can free it again). The name of the groups/configs
etc. is also dynamic, no?

> Because for the way proposed, all the pin's basic properties like mux_reg, config_reg
> are parsed from device tree at runtime.

The ones you want to perform anyway.

> If a pin is not used in device tree, the driver can not know this pin's
> corresponding registers.

It would. I'm not saying get rid of the table, just remove the
redundancy - if you don't reference pins by "arbitrary id" anymore,
why include an arbitrary id?

At the moment you've got a redundant list of enums to define offsets
into an array which is.. an array anyway. You can always work out the
position in the array from it's address and the structure size
(offsetof or some clever math).

If you're not referencing the pin by index into the array anyway which
is bad behavior, the other data serves as cross-reference - the
register offsets defined are unique to the pin in question. So the
enums go away, and the first element of imx_pin_reg goes away. The
rest is just debug data.

The static pinctrl_pin_desc array can go away as well. As it stands
it's a waste of memory - MX6Q_PAD_ in every string cannot be merged by
the compiler, that's 329 multiplied 9 bytes you're compiling into the
kernel redundantly. The name can be referenced - I'm gonna use
MX6Q_PAD_SD4_CMD__SD4_CMD as an example here to reduce confusion.

> Then imx_pinconf_dbg_show in current driver may need change since it does not
> support show all pins's config value.

You will need the huge static array to cross-reference the pins
canonical name and generated configuration, sure, I don't want to
remove it, just.. not use array index to reference it from the device
tree.

If you imagine building a string like "MX6Q_PAD_" "SD4_CMD" "__"
"SD4_CMD" - this is defined by the SoC in use and the DT binding, the
default pad name from the docs, a couple underscores to match the old
iomux-v3 definition which we are still clinging on to (and should
since this is what's used in U-Boot right now) and the ALT mode from
the MUX_MODE bits.

The enum id can be replaced with a pin name string pointer "SD4_CMD"
which is static per pin. The imx_pin_reg struct would become

struct imx_pin_reg {
   const char *pad_name;
   u32 mux_reg;
   u32 ctl_reg;
   u32 ipp_reg;
   const char *mux_modes;
   u32 nmodes;
};

This can be used to build strings for debug. You're not using much
more space than the existing model, and the array contains strings
defining the last part of the debug string representing the alt mode -
saving a little bit of memory besides. And of course, it's not needed
to actually perform the muxing since that's just adding register
offsets to bases and writel() a value direct from the DT (although,
imx_pin_reg could be used to verify that the 3 pairs correspond to
each other...)

And you'd need strings to define the CTL settings like HYS, ODE, PKE,
PUE settings from the registers and give them human readable names as
per the device tree binding, as they were set, but this is not in the
imx_pin_regs array (or maybe it is, someone should put all the default
pad settings in there?).

Build the string like you want and you reproduce the naming in the
comments. Some other debug data needs to be exposed to define the bits
in the register as above, which isn't done right now. As such the
debug currently isn't so useful - it only describes the pad itself,
the MUX_MODE in an i.MX IOMUXC and the rest of the information is lost
since it does not take into account the pad control itself. This is
lucky as it's less work for now.

> If it supports, will imx driver also support config unused pins in devicetree
> via sysfs?

Setting up the stuff from the device tree wouldn't involve the
imx_pin_reg array so you could compile the whole thing out if you
didn't want the space usage or the chattiness of debug logs.

> Probably it may be rarely used and imx just does not support it.
> Then we can get register data from devicetree.

-- 
Matt

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

[Stephen Warren]

On 08/15/2012 09:30 PM, Dong Aisheng wrote:
> On Wed, Aug 15, 2012 at 09:59:50PM +0800, Shawn Guo wrote:
> ...
>>> 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.
>>>
>> I do not quite follow on this.  The "enum imx51_pads" will still be
>> there, so all the pads will still be visible to the pinctrl system.
>>
> Sorry, i did not describe it accurately.
> The problem is that those pins are visible to pinctrl subystem, but the pinctrl
> subsystem can not manage them all.
> Not sure this meet Linus's original design purpose.
> Because for the way proposed, all the pin's basic properties like mux_reg, config_reg
> are parsed from device tree at runtime.
> If a pin is not used in device tree, the driver can not know this pin's
> corresponding registers.
> Thus, for those unused pins, we can not manage it on mux or config in pinctrl
> subsystem and driver.
> 
> For example, the pin_config_get/pin_config_set API in
> include/linux/pinctrl/consumer.h can not work for such pins.

Hmmm. Given we support all pin mux/config options from the mapping
table, I'd question whether that API should even exist any more...

> Then imx_pinconf_dbg_show in current driver may need change since it does not
> support show all pins's config value.

The pinctrl core assumes that the pin numbering space could be sparse.
For example, see that pinconf.c:pinconf_pins_show() "continues" the loop
if pin_desc_get() fails for a particular pin number. So just based on
what I've read in this one email, I think this is fine.

> It looks like not a big issue currently since i did not see any client driver
> using this API.
> But i'm not sure if we may have this requirement in the future.
> For example, is it possible that pinctrl subsystem may support configure pins
> via sysfs dynamically?

I can't comment on sysfs specifically, but I believe it would be
generally true that a pin that isn't know to the pinctrl subsystem can't
be manipulated in any way.

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

[Linus Walleij]

On Thu, Aug 16, 2012 at 5:30 AM, Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> The problem is that those pins are visible to pinctrl subystem, but the pinctrl
> subsystem can not manage them all.
> Not sure this meet Linus's original design purpose.

The U300 driver has *lots* of pins it cannot manage. I have no strong
opinions on this, often if there are lots of pins and say some ranges
here and there are controllable, you may want to register them all
(with names) just so that you can get something intuitive out of the
pin numbers.

But the descriptors are actually sparse, so you don't have to have
a consecutive pin range and can actually just register the pins you
want with the names you want. It's your pick...

> But i'm not sure if we may have this requirement in the future.
> For example, is it possible that pinctrl subsystem may support configure pins
> via sysfs dynamically?

Not sysfs. For GPIO this ended up in a bad place.
Maybe /dev/pinctrl0, /dev/pinctrl1

But I don't think we want to expose individual pin configs to
userspace. Maybe pinctrl states. Dunno, has to be designed,
and surely pins can be totally numb and unconfigurable as well....

> If it supports, will imx driver also support config unused pins in devicetree
> via sysfs?

DT *or* /dev/foo I guess.

Yours,
Linus Walleij

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

[Linus Walleij]

On Thu, Aug 16, 2012 at 11:12 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 08/15/2012 09:30 PM, Dong Aisheng wrote:

>> For example, the pin_config_get/pin_config_set API in
>> include/linux/pinctrl/consumer.h can not work for such pins.
>
> Hmmm. Given we support all pin mux/config options from the mapping
> table, I'd question whether that API should even exist any more...

If there are no in-kernel users we can delete it. But I can surely see
things like PCI lab boards etc wanting to have full control over pins
from userspace, just like they can hammer GPIO's on/off today
from userspace (i.e. embedded SoCs is not the only use case)
so I wouldn't rule it out. I think this use case is quite real for
automatic control, robotics, factory lines... those guys do use GPIO
sysfs like that already today.

>> It looks like not a big issue currently since i did not see any client driver
>> using this API.
>> But i'm not sure if we may have this requirement in the future.
>> For example, is it possible that pinctrl subsystem may support configure pins
>> via sysfs dynamically?
>
> I can't comment on sysfs specifically, but I believe it would be
> generally true that a pin that isn't know to the pinctrl subsystem can't
> be manipulated in any way.

sysfs over my dead body ... but exposing pin names to userspace is
another thing, and might be interesting, in case we want to just
sort of list all pins on the system from some command line tool or
HW-info control panel. But that requires designing the userspace
device interface first, so no need to haste.

Yours,
Linus Walleij