Pulls and drive strengths in the pinctrl world

[Tomasz Figa]

On Sunday 19 of May 2013 11:17:36 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > PULL_UP         (1 << 0): indicate this pin need a pull up.
> > > > > MULTIDRIVE      (1 << 1): indicate this pin need to be
> > > > > configured as
> > > > > multidrive. DEGLITCH        (1 << 2): indicate this pin need
> > > > > deglitch.
> > > > > PULL_DOWN       (1 << 3): indicate this pin need a pull down.
> > > > > DIS_SCHMIT      (1 << 4): indicate this pin need to disable
> > > > > schmit
> > > > > trigger. DEBOUNCE        (1 << 16): indicate this pin need
> > > > > debounce.
> > > > > DEBOUNCE_VAL    (0x3fff << 17): debounce val.
> > > > > 
> > > > > today I was planning to update the binding to allow to this
> > > > > 
> > > > > instead of writing this
> > > > > 
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_A 
AT91_PINCTRL_NONE
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP>;
> > > > > 	
> > > > > 	};
> > > > > 
> > > > > };
> > > > > 
> > > > > we will write this
> > > > > 
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu: dbgu-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_A>,
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A 
AT91_PINCTRL_PULL_UP>;
> > > > > 	
> > > > > 	};
> > > > > 
> > > > > };
> > > > > 
> > > > > so a pin can have 3 or more parameter
> > > > > 
> > > > > so as a generic binding only the 3 first will be namdatory (bank
> > > > > pinnp
> > > > > muxid) the rest will driver specific
> > > > > 
> > > > > for power down I plan to define an other node
> > > > > dbgu {
> > > > > 
> > > > > 	pinctrl_dbgu_sleep: dbgu_sleep-0 {
> > > > > 	
> > > > > 		atmel,pins =
> > > > > 		
> > > > > 			<AT91_PIOB 30 AT91_PERIPH_GPIO>,
> > > > > 			
> > > > > 			 AT91_PIOB 31 AT91_PERIPH_A
> > > > 
> > > > AT91_PINCTRL_PULL_DOWN>;
> > > > 
> > > > > 	};
> > > > > 
> > > > > };
> > > > 
> > > > I'm afraid this won't work for Samsung SoCs. In our case normal
> > > > and
> > > > power down settings are completely unrelated, i.e. stored in
> > > > separate
> > > > registers and one doesn't affect another (a system controller
> > > > automatically switches between normal and power down settings when
> > > > entering or leaving low power modes, like SoC-level suspend).
> > > 
> > > and?
> > 
> > Pin configuration node on Exynos SoCs will need 7 values for each pin
> > in samsung,pins property, just like in following example:
> > 
> > mmc0 {
> > 
> > 	mmc0_bus1: mmc0-bus1 {
> > 	
> > 		pins = <GPA0 4 SFN3 PULL_UP DRV4 PDN_IN PDN_PULL_UP>;
> > 	
> > 	};
> > 	/* ... */
> > 
> > };
> > 
> > Now if I just want to enable pull-up for a single pin, I will have to
> > add following node:
> > 
> > foo {
> > 
> > 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>;
> > 
> > };
> 
> no you will not
> 
> foo {
> 	pins = <GPK1 2 NONE PULL_UP>;
> };

OK, this will work in case of one pin, but if you need two it starts to 
become problematic. Let's look at an example:

We have two pins for which we don't need to specify power down mode 
settings (e.g. they are in alive banks):

foo {
 	pins = <GPK1 2 NONE PULL_UP>,
		<GPK1 3 NONE PULL_UP>;
};

After compilation you will get just a series of u32 values, like

foo {
	pins = <1 2 255 3 1 3 255 3>;
};

How are you going to distinguish such setup with:

foo {
 	pins = <GPK1 2 NONE PULL_UP NONE NONE NONE>,
		<GPK1 3 NONE PULL_UP NONE NONE NONE>;
};

which translates to

foo {
	pins = <1 2 255 3 255 255 255 1 3 255 3 255 255 255>;
};

I mean, you don't know where one entry ends and another starts, if you 
allow to left out some values.

> how a pin can not have mux?

I don't always want to change the mux. Sometimes I just want to change one 
of the other parameters. For example, I don't want to switch the pin to 
interrupt mode on driver probe (it is a separate pin mux value), but 
rather after the trigger type gets configured, which is done by 
request_irq() based on trigger flags.

> > while with current bindings I can simply omit properties that I don't
> > care about (or are going to be set up correctly by other means - e.g.
> > gpio_direction_input() or request_irq(), ending with following node:
> > 
> > foo {
> > 
> > 	samsung,pins = "gpk1-2";
> > 	samsung,pin-pud = <3>;
> > 
> > };
> 
> except here you will 100s of NODE which we do NOT want in the dtb

Is this really an issue?

We are already using this method to describe really complex boards (not 
necessarily in mainline) and we don't have any problems.

> > This is all I need to configure for simple things like open-drain
> > interrupt lines.
> > 
> > Another thing is that we're using one driver for many SoCs, which have
> > different variants of the controller. So for example some of them
> > don't
> > have driver strength configuration (S3C24xx, S3C64xx), other don't
> > have
> > power down mode configuration (S3C24xx) and even some of the banks on
> > some SoCs don't support particular type of configuration (alive banks
> > of SoCs
> same on at91 some IP have less feature
> and some SoC have the IP/die but not the same pins package
> 
> the key point is to share the pin DT handling between at91, ST and
> Samsung ofcourse all the IP will have more or less parameter per pin
> but the basic is the same for DT and C code
> 
> > >= S3C64xx don't have power down mode configuration, because they are
> > 
> > always on).
> > 
> > > on at91 I've x banks of registers to handle each gpio bank
> > > 
> > > on ST with have same situation but the controller work the same way
> > > at
> > > the end
> > > 
> > > so we need to factorise code
> > > 
> > > > Personally I'd prefer a solution with separate property for each
> > > > parameter, because it's much more flexible and allows shorter
> > > > lines,
> > > > making device tree sources more readable.
> > > 
> > > we already discuss this on the ML the one property perline will
> > > endup
> > > with 100s of node if not 1000s so we all do agree we do not want
> > > this
> > > in the DT
> > 
> > Could you point me to this discussion, please? I'd really like to take
> > a look.
> 
> you have to search this on the dt ML, it was about the clk bindings IIRC
> and agree on this at Prague durring kernel Summit

OK. Will look for it.

Best regards,
Tomasz