| Message ID | 359d62c1f2f7ef61c6160df9b60c1af4bfcea89a.1496664241.git.baolin.wang@spreadtrum.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 5, 2017 at 2:11 PM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > This patch adds the binding documentation for Spreadtrum SC9860 pin > controller device. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> Good improvements since last iteration! (...) > +The Spreadtrum pin controller are organized in 3 blocks (types). > + > +The first block comprises some global control registers, and each > +register contains several bit fields with one bit or several bits > +to configure for some global common configuration, such as domain > +pad driving level, system control select and so on. Insert your explanations about domain pads and system control from the other mail here. > + We recognise > +every fields comprising one bit or several bits in one global control > +register as one pin, thus we should record every pin's bit offset, > +bit width and register offset to configure this field (pin). Since > +this type pins' configuration are very tricky and different for each > +register, we introduce "sprd,ctrl" property to set the various global > +control configuration. Hm I still don't fully understand this property. > + In some situation we have some pin-sleep related > +configuration need to set when one of system goes into deep sleep > +mode. For example, if we set the pin sleep mode as AP_SLEEP, which > +means when AP system goes into deep sleep mode, this pin's sleep > +related properties (input/output or pullup/down) will be set > +automatically. Thus we intoduce "sprd,sleep_mode" to set pin sleep > +mode. So what you mean is that there is a special register for the sleep mode, so that we do not need to write the sleep mode explicitly, it will instead hit the hardware automatically when the system switches to sleep mode on some global level? Please detail this. This is not a spreadtrum-specific feature. Look for example in: include/dt-bindings/pinctrl/nomadik.h We have things like this: #define SLPM_DISABLED 0 #define SLPM_ENABLED 1 #define SLPM_INPUT_NOPULL 0 #define SLPM_INPUT_PULLUP 1 #define SLPM_INPUT_PULLDOWN 2 #define SLPM_DIR_INPUT 3 #define SLPM_OUTPUT_LOW 0 #define SLPM_OUTPUT_HIGH 1 #define SLPM_DIR_OUTPUT 2 #define SLPM_WAKEUP_DISABLE 0 #define SLPM_WAKEUP_ENABLE 1 These (also custom) properties predate the generic pin config, and that is why it was done like this. But moving forward, we may need to think about coming up with something generic for this. I think this kind of overlaps the pin control states "default" and "sleep". What we want to do is read out both "default" and "sleep" modes from the device tree and program *both* into the hardware when "default" is normally initialized. This is better, because then the device tree looks the same whether we use hardware-backed switch from "default" to "sleep" and back or not. I don't know how to do this practically unfortunately, it may need some modifications of the pin control core code so that drivers can get the "sleep" mode configuration out and program it. > +The last block comprises some misc registers which also have unified > +register definition, and each register described one pin is used to > +configure drive strength, pull up/down and so on. OK > + Especially for pull > +up, we introduce "sprd,pullup" property for two kind configuration: > +PULLUP_20K or PULLUP_4_7K, which means the pullup resistor is 20K or > +4.7K. Do not use this. The generic bias-pull-up already supports an argument. Use this: bias-pull-up = <20000>; bias-pull-up = <4700>; Just unpack the argument in the .set_config() callback and deny it setting anything else than 20000 or 4700 Ohms. > +Optional properties: > +- function: Specified the function name. > +- drive-strength: Drive strength in mA. > +- input-schmitt-disable: Enable schmitt-trigger mode. > +- input-schmitt-enable: Disable schmitt-trigger mode. > +- bias-disable: Disable pin bias. > +- bias-pull-down: Pull down on pin. OK > +- sprd,ctrl: Control values referring to databook for global control pins. Uncertain on this. > +- sprd,sleep_mode: Sleep mode selection. > +- sprd,pullup: Pull up on pin. Switch to bias-pull-up as per above. > +- sprd,input-sleep: Input enable when system goes into deep sleep mode. > +- sprd,output-sleep: Output enable when system goes into deep sleep mode. > +- sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. > +- sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. This is not good. We need to handle these using the standard input-enable output-enable bias-pull-up bias-pull-down Just inside a state named "sleep" and then let the driver read and program this state into the sleep registers at first convenience. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 五, 6月 09, 2017 at 01:00:17下午 +0200, Linus Walleij wrote: > On Mon, Jun 5, 2017 at 2:11 PM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > > > This patch adds the binding documentation for Spreadtrum SC9860 pin > > controller device. > > > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > > Good improvements since last iteration! > > (...) > > > +The Spreadtrum pin controller are organized in 3 blocks (types). > > + > > +The first block comprises some global control registers, and each > > +register contains several bit fields with one bit or several bits > > +to configure for some global common configuration, such as domain > > +pad driving level, system control select and so on. > > Insert your explanations about domain pads and system > control from the other mail here. OK. > > > + We recognise > > +every fields comprising one bit or several bits in one global control > > +register as one pin, thus we should record every pin's bit offset, > > +bit width and register offset to configure this field (pin). Since > > +this type pins' configuration are very tricky and different for each > > +register, we introduce "sprd,ctrl" property to set the various global > > +control configuration. > > Hm I still don't fully understand this property. I try to explain these special pins definition on Spreadtrum platform. This type pins (containing one bit or several bits) are used to configure various global control, not only domain pad driving level, system control, and also some other configuration: EIC (external interrupt controller) source select, debug mode select, camera pd signal select, camera reset signal select, ADI sync signal require, watchdog reset select ....... There are too much various configuration that I can not list all of them, so we can not make every special configuration as one generic configuration, and maybe it will add more strange globle configuration in future. Then we add one "sprd,ctrl" (maybe "sprd,control" will be better) to set these various global control configuration, and we need use magic number for this property. > > > + In some situation we have some pin-sleep related > > +configuration need to set when one of system goes into deep sleep > > +mode. For example, if we set the pin sleep mode as AP_SLEEP, which > > +means when AP system goes into deep sleep mode, this pin's sleep > > +related properties (input/output or pullup/down) will be set > > +automatically. Thus we intoduce "sprd,sleep_mode" to set pin sleep > > +mode. > > So what you mean is that there is a special register for the sleep > mode, so that we do not need to write the sleep mode explicitly, > it will instead hit the hardware automatically when the system > switches to sleep mode on some global level? No. Sorry for confusing. What I mean is if we set one pin's sleep mode as AP_SLEEP, when the AP system goes into deep sleep mode, then the sleep related configuration of this pin will be set automatically. If we set the sleep mode as PUBCP_SLEEP, which means when the pubcp system goes into deep sleep mode, this pin's sleep related configuration will be set. The sleep related configuration of one pin are described (need modify as you pointed) as below: - sprd,input-sleep: Input enable when system goes into deep sleep mode. - sprd,output-sleep: Output enable when system goes into deep sleep mode. - sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. - sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. So if we set one pin's sleep related configuration, we also should choose the sleep mode (means which system). > > Please detail this. > > This is not a spreadtrum-specific feature. Look for example in: > include/dt-bindings/pinctrl/nomadik.h > > We have things like this: > > #define SLPM_DISABLED 0 > #define SLPM_ENABLED 1 > #define SLPM_INPUT_NOPULL 0 > #define SLPM_INPUT_PULLUP 1 > #define SLPM_INPUT_PULLDOWN 2 > #define SLPM_DIR_INPUT 3 > #define SLPM_OUTPUT_LOW 0 > #define SLPM_OUTPUT_HIGH 1 > #define SLPM_DIR_OUTPUT 2 > #define SLPM_WAKEUP_DISABLE 0 > #define SLPM_WAKEUP_ENABLE 1 > > These (also custom) properties predate the generic pin config, > and that is why it was done like this. > > But moving forward, we may need to think about coming up with > something generic for this. > > I think this kind of overlaps the pin control states "default" > and "sleep". > > What we want to do is read out both "default" and "sleep" modes > from the device tree and program *both* into the hardware > when "default" is normally initialized. > > This is better, because then the device tree looks the same > whether we use hardware-backed switch from "default" to > "sleep" and back or not. > > I don't know how to do this practically unfortunately, it may need > some modifications of the pin control core code so that drivers > can get the "sleep" mode configuration out and program it. Yes, I think these "sleep" related configuration are not like what "sprd,sleep-mode" describes, it looks like below properties: - sprd,input-sleep: Input enable when system goes into deep sleep mode. - sprd,output-sleep: Output enable when system goes into deep sleep mode. - sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. - sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. I agree we should add these into core code, something like: static const struct pinconf_generic_params dt_params[] = { { "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 }, { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, { "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 }, { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 }, { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 }, { "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 }, { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 }, { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 }, { "output-high", PIN_CONFIG_OUTPUT, 1, }, { "output-low", PIN_CONFIG_OUTPUT, 0, }, { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN 1}, ...... }; If you agree that, I can help to add these configuration. > > > +The last block comprises some misc registers which also have unified > > +register definition, and each register described one pin is used to > > +configure drive strength, pull up/down and so on. > > OK > > > + Especially for pull > > +up, we introduce "sprd,pullup" property for two kind configuration: > > +PULLUP_20K or PULLUP_4_7K, which means the pullup resistor is 20K or > > +4.7K. > > Do not use this. The generic bias-pull-up already supports an > argument. > > Use this: > > bias-pull-up = <20000>; > bias-pull-up = <4700>; > > Just unpack the argument in the .set_config() callback and deny > it setting anything else than 20000 or 4700 Ohms. Yes, you are right. I missed this and will fix in next version. > > > +Optional properties: > > +- function: Specified the function name. > > +- drive-strength: Drive strength in mA. > > +- input-schmitt-disable: Enable schmitt-trigger mode. > > +- input-schmitt-enable: Disable schmitt-trigger mode. > > +- bias-disable: Disable pin bias. > > +- bias-pull-down: Pull down on pin. > > OK > > > +- sprd,ctrl: Control values referring to databook for global control pins. > > Uncertain on this. > > > +- sprd,sleep_mode: Sleep mode selection. > > +- sprd,pullup: Pull up on pin. > > Switch to bias-pull-up as per above. OK. > > > +- sprd,input-sleep: Input enable when system goes into deep sleep mode. > > +- sprd,output-sleep: Output enable when system goes into deep sleep mode. > > +- sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. > > +- sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. > > This is not good. We need to handle these using the standard > > input-enable > output-enable > bias-pull-up > bias-pull-down > > Just inside a state named "sleep" and then let the driver read and program > this state into the sleep registers at first convenience. I am afraid I can not use the "sleep" state. Since if we set the "sleep" state, and usually we will issue "pinctrl_force_sleep()" in suspend function, but I am afraid it will be too late for pin sleep related configuration when system goes into deep sleep. As I explained above, these 4 sleep related configuration bind with the pin sleep mode, we should set them as generic config before system goes into deep sleep. We can add these sleep related configuration into pinctrl core like I suggested above, which will be more reasonable. What do you think? static const struct pinconf_generic_params dt_params[] = { ...... { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN, 1}, + { "sleep-bias-input-enable", PIN_CONFIG_SLEEP_INPUT_ENABLE, 1}, + { "sleep-bias-output-enable", PIN_CONFIG_SLEEP_OUTPUT_ENABLE, 1}, Really appreciate for your comments. }; > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 一, 6月 12, 2017 at 02:07:18下午 +0800, Baolin Wang wrote: > Hi Linus, > > On 五, 6月 09, 2017 at 01:00:17下午 +0200, Linus Walleij wrote: > > On Mon, Jun 5, 2017 at 2:11 PM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > > > > > This patch adds the binding documentation for Spreadtrum SC9860 pin > > > controller device. > > > > > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > > > > Good improvements since last iteration! > > > > (...) > > > > > +The Spreadtrum pin controller are organized in 3 blocks (types). > > > + > > > +The first block comprises some global control registers, and each > > > +register contains several bit fields with one bit or several bits > > > +to configure for some global common configuration, such as domain > > > +pad driving level, system control select and so on. > > > > Insert your explanations about domain pads and system > > control from the other mail here. > > OK. > > > > > > + We recognise > > > +every fields comprising one bit or several bits in one global control > > > +register as one pin, thus we should record every pin's bit offset, > > > +bit width and register offset to configure this field (pin). Since > > > +this type pins' configuration are very tricky and different for each > > > +register, we introduce "sprd,ctrl" property to set the various global > > > +control configuration. > > > > Hm I still don't fully understand this property. > > I try to explain these special pins definition on Spreadtrum platform. > This type pins (containing one bit or several bits) are used to configure > various global control, not only domain pad driving level, system control, > and also some other configuration: > EIC (external interrupt controller) source select, debug mode select, > camera pd signal select, camera reset signal select, ADI sync signal > require, watchdog reset select ....... > > There are too much various configuration that I can not list all of them, > so we can not make every special configuration as one generic configuration, > and maybe it will add more strange globle configuration in future. Then > we add one "sprd,ctrl" (maybe "sprd,control" will be better) to set these > various global control configuration, and we need use magic number for this > property. > > > > > > + In some situation we have some pin-sleep related > > > +configuration need to set when one of system goes into deep sleep > > > +mode. For example, if we set the pin sleep mode as AP_SLEEP, which > > > +means when AP system goes into deep sleep mode, this pin's sleep > > > +related properties (input/output or pullup/down) will be set > > > +automatically. Thus we intoduce "sprd,sleep_mode" to set pin sleep > > > +mode. > > > > So what you mean is that there is a special register for the sleep > > mode, so that we do not need to write the sleep mode explicitly, > > it will instead hit the hardware automatically when the system > > switches to sleep mode on some global level? > > No. Sorry for confusing. What I mean is if we set one pin's sleep mode > as AP_SLEEP, when the AP system goes into deep sleep mode, then the sleep > related configuration of this pin will be set automatically. If we set > the sleep mode as PUBCP_SLEEP, which means when the pubcp system goes into > deep sleep mode, this pin's sleep related configuration will be set. > > The sleep related configuration of one pin are described (need modify as > you pointed) as below: > - sprd,input-sleep: Input enable when system goes into deep sleep mode. > - sprd,output-sleep: Output enable when system goes into deep sleep mode. > - sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. > - sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. > > So if we set one pin's sleep related configuration, we also should choose > the sleep mode (means which system). > > > > > Please detail this. > > > > This is not a spreadtrum-specific feature. Look for example in: > > include/dt-bindings/pinctrl/nomadik.h > > > > We have things like this: > > > > #define SLPM_DISABLED 0 > > #define SLPM_ENABLED 1 > > #define SLPM_INPUT_NOPULL 0 > > #define SLPM_INPUT_PULLUP 1 > > #define SLPM_INPUT_PULLDOWN 2 > > #define SLPM_DIR_INPUT 3 > > #define SLPM_OUTPUT_LOW 0 > > #define SLPM_OUTPUT_HIGH 1 > > #define SLPM_DIR_OUTPUT 2 > > #define SLPM_WAKEUP_DISABLE 0 > > #define SLPM_WAKEUP_ENABLE 1 > > > > These (also custom) properties predate the generic pin config, > > and that is why it was done like this. > > > > But moving forward, we may need to think about coming up with > > something generic for this. > > > > I think this kind of overlaps the pin control states "default" > > and "sleep". > > > > What we want to do is read out both "default" and "sleep" modes > > from the device tree and program *both* into the hardware > > when "default" is normally initialized. > > > > This is better, because then the device tree looks the same > > whether we use hardware-backed switch from "default" to > > "sleep" and back or not. > > > > I don't know how to do this practically unfortunately, it may need > > some modifications of the pin control core code so that drivers > > can get the "sleep" mode configuration out and program it. > > Yes, I think these "sleep" related configuration are not like > what "sprd,sleep-mode" describes, it looks like below properties: > - sprd,input-sleep: Input enable when system goes into deep sleep mode. > - sprd,output-sleep: Output enable when system goes into deep sleep mode. > - sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. > - sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. > > I agree we should add these into core code, something like: > static const struct pinconf_generic_params dt_params[] = { > { "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 }, > { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, > { "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 }, > { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, > { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 }, > { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, > { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, > { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, > { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 }, > { "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 }, > { "input-schmitt", PIN_CONFIG_INPUT_SCHMITT, 0 }, > { "input-schmitt-disable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 0 }, > { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 }, > { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 }, > { "output-high", PIN_CONFIG_OUTPUT, 1, }, > { "output-low", PIN_CONFIG_OUTPUT, 0, }, > { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, > + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN 1}, > ...... > }; > > If you agree that, I can help to add these configuration. > > > > > > +The last block comprises some misc registers which also have unified > > > +register definition, and each register described one pin is used to > > > +configure drive strength, pull up/down and so on. > > > > OK > > > > > + Especially for pull > > > +up, we introduce "sprd,pullup" property for two kind configuration: > > > +PULLUP_20K or PULLUP_4_7K, which means the pullup resistor is 20K or > > > +4.7K. > > > > Do not use this. The generic bias-pull-up already supports an > > argument. > > > > Use this: > > > > bias-pull-up = <20000>; > > bias-pull-up = <4700>; > > > > Just unpack the argument in the .set_config() callback and deny > > it setting anything else than 20000 or 4700 Ohms. > > Yes, you are right. I missed this and will fix in next version. > > > > > > +Optional properties: > > > +- function: Specified the function name. > > > +- drive-strength: Drive strength in mA. > > > +- input-schmitt-disable: Enable schmitt-trigger mode. > > > +- input-schmitt-enable: Disable schmitt-trigger mode. > > > +- bias-disable: Disable pin bias. > > > +- bias-pull-down: Pull down on pin. > > > > OK > > > > > +- sprd,ctrl: Control values referring to databook for global control pins. > > > > Uncertain on this. > > > > > +- sprd,sleep_mode: Sleep mode selection. > > > +- sprd,pullup: Pull up on pin. > > > > Switch to bias-pull-up as per above. > > OK. > > > > > > +- sprd,input-sleep: Input enable when system goes into deep sleep mode. > > > +- sprd,output-sleep: Output enable when system goes into deep sleep mode. > > > +- sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. > > > +- sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. > > > > This is not good. We need to handle these using the standard > > > > input-enable > > output-enable > > bias-pull-up > > bias-pull-down > > > > Just inside a state named "sleep" and then let the driver read and program > > this state into the sleep registers at first convenience. > > I am afraid I can not use the "sleep" state. Since if we set the "sleep" state, > and usually we will issue "pinctrl_force_sleep()" in suspend function, but I am > afraid it will be too late for pin sleep related configuration when system goes > into deep sleep. > > As I explained above, these 4 sleep related configuration bind with the pin sleep > mode, we should set them as generic config before system goes into deep sleep. > We can add these sleep related configuration into pinctrl core like I suggested > above, which will be more reasonable. What do you think? > static const struct pinconf_generic_params dt_params[] = { > ...... > { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, > + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN, 1}, > + { "sleep-bias-input-enable", PIN_CONFIG_SLEEP_INPUT_ENABLE, 1}, > + { "sleep-bias-output-enable", PIN_CONFIG_SLEEP_OUTPUT_ENABLE, 1}, > > Really appreciate for your comments. I forgot one most important reason why we can not use the "sleep" state. As I explained above, the sleep related configuration will bind with the pin's sleep mode. If we set the pin's sleep mode as AP_SLEEP, then we can select "sleep" state when AP system goes into deep sleep mode by issuing "pinctrl_force_sleep()" in pinctrl suspend function. But if we set the pin's sleep mode as PUBCP_SLEEP and pubcp system doesn't run linux kernel (it run another thread OS), then we can not select "sleep" state since the AP system does not go into deep sleep mode (AP system run linux kernel OS). > }; > > > > Yours, > > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 12, 2017 at 8:07 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > I agree we should add these into core code, something like: > static const struct pinconf_generic_params dt_params[] = { (...) > { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, > + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, > + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN 1}, > ...... > }; > > If you agree that, I can help to add these configuration. Actually, yes indeed, that is a nice thing. Very nice. I think my Nomadik driver could use this too. Just need to hear what the DT maintainers have to say about it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 13, 2017 at 5:15 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > I forgot one most important reason why we can not use the "sleep" state. As I explained > above, the sleep related configuration will bind with the pin's sleep mode. If we set the > pin's sleep mode as AP_SLEEP, then we can select "sleep" state when AP system goes into > deep sleep mode by issuing "pinctrl_force_sleep()" in pinctrl suspend function. > > But if we set the pin's sleep mode as PUBCP_SLEEP and pubcp system doesn't run linux kernel > (it run another thread OS), then we can not select "sleep" state since the AP system does > not go into deep sleep mode (AP system run linux kernel OS). Allright yes it makes sense, and also there are systems that just go into "hardware sleep" and just put the pin into some pre-programmed mode. I'm a bit back-and-forth. I didn't mean that some code would actually switch the state to "sleep" when we go to sleep, I meant that when the system configures "default" mode it should also look up and program the "sleep" mode, but this approach with a special property is just another way of achieveing the same thing. But then we should add a whole slew of sleep states. I was thinking whether we could avoid having a special DT property by parsing ahead to states we do not currently use and programming that into the sleep mode registers. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 June 2017 at 17:10, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jun 12, 2017 at 8:07 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > >> I agree we should add these into core code, something like: >> static const struct pinconf_generic_params dt_params[] = { > (...) >> { "slew-rate", PIN_CONFIG_SLEW_RATE, 0 }, >> + { "sleep-bias-pull-up", PIN_CONFIG_SLEEP_BIAS_PULL_UP, 1}, >> + { "sleep-bias-pull-down", PIN_CONFIG_SLEEP_BIAS_PULL_DOWN 1}, >> ...... >> }; >> >> If you agree that, I can help to add these configuration. > > Actually, yes indeed, that is a nice thing. Very nice. I think my Nomadik > driver could use this too. > > Just need to hear what the DT maintainers have to say about it. OK, I will submit one patch to review. Thanks.
On 20 June 2017 at 17:31, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jun 13, 2017 at 5:15 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: > >> I forgot one most important reason why we can not use the "sleep" state. As I explained >> above, the sleep related configuration will bind with the pin's sleep mode. If we set the >> pin's sleep mode as AP_SLEEP, then we can select "sleep" state when AP system goes into >> deep sleep mode by issuing "pinctrl_force_sleep()" in pinctrl suspend function. >> >> But if we set the pin's sleep mode as PUBCP_SLEEP and pubcp system doesn't run linux kernel >> (it run another thread OS), then we can not select "sleep" state since the AP system does >> not go into deep sleep mode (AP system run linux kernel OS). > > Allright yes it makes sense, and also there are systems that just go into > "hardware sleep" and just put the pin into some pre-programmed mode. > > I'm a bit back-and-forth. I didn't mean that some code would actually > switch the state to "sleep" when we go to sleep, I meant that when > the system configures "default" mode it should also look up and > program the "sleep" mode, but this approach with a special property > is just another way of achieveing the same thing. > > But then we should add a whole slew of sleep states. > > I was thinking whether we could avoid having a special DT property > by parsing ahead to states we do not currently use and programming > that into the sleep mode registers. Yes, for most scenarios, it can work with the "sleep" state to set sleep-related config. But for our Spreadtrum platform scenario (I do not know if there are other platforms need this feature), we can not select the "sleep" state when pubcp system goes into deep sleep mode but ap system does not go into deep sleep mode. So I think we still need these "sleep-bias-pull-up", "sleep-bias-pull-down", "sleep-input-enable" and "sleep-output-enable" properties.
On Wed, Jun 21, 2017 at 10:10 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 20 June 2017 at 17:31, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Tue, Jun 13, 2017 at 5:15 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: >> >>> I forgot one most important reason why we can not use the "sleep" state. As I explained >>> above, the sleep related configuration will bind with the pin's sleep mode. If we set the >>> pin's sleep mode as AP_SLEEP, then we can select "sleep" state when AP system goes into >>> deep sleep mode by issuing "pinctrl_force_sleep()" in pinctrl suspend function. >>> >>> But if we set the pin's sleep mode as PUBCP_SLEEP and pubcp system doesn't run linux kernel >>> (it run another thread OS), then we can not select "sleep" state since the AP system does >>> not go into deep sleep mode (AP system run linux kernel OS). >> >> Allright yes it makes sense, and also there are systems that just go into >> "hardware sleep" and just put the pin into some pre-programmed mode. >> >> I'm a bit back-and-forth. I didn't mean that some code would actually >> switch the state to "sleep" when we go to sleep, I meant that when >> the system configures "default" mode it should also look up and >> program the "sleep" mode, but this approach with a special property >> is just another way of achieveing the same thing. >> >> But then we should add a whole slew of sleep states. >> >> I was thinking whether we could avoid having a special DT property >> by parsing ahead to states we do not currently use and programming >> that into the sleep mode registers. > > Yes, for most scenarios, it can work with the "sleep" state to set > sleep-related config. But for our Spreadtrum platform scenario (I do > not know if there are other platforms need this feature), we can not > select the "sleep" state when pubcp system goes into deep sleep mode > but ap system does not go into deep sleep mode. So I think we still > need these "sleep-bias-pull-up", "sleep-bias-pull-down", > "sleep-input-enable" and "sleep-output-enable" properties. I don't really mean you should select the "sleep" state. I meant, as part of setting the "default" state or even the "init" state, we would inspect the "sleep" state, use those settings, and program them into the registers at this early point. Then never touch the registers again, and never really go to the sleep state by software, just by hardware. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 June 2017 at 06:19, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jun 21, 2017 at 10:10 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 20 June 2017 at 17:31, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Tue, Jun 13, 2017 at 5:15 AM, Baolin Wang <baolin.wang@spreadtrum.com> wrote: >>> >>>> I forgot one most important reason why we can not use the "sleep" state. As I explained >>>> above, the sleep related configuration will bind with the pin's sleep mode. If we set the >>>> pin's sleep mode as AP_SLEEP, then we can select "sleep" state when AP system goes into >>>> deep sleep mode by issuing "pinctrl_force_sleep()" in pinctrl suspend function. >>>> >>>> But if we set the pin's sleep mode as PUBCP_SLEEP and pubcp system doesn't run linux kernel >>>> (it run another thread OS), then we can not select "sleep" state since the AP system does >>>> not go into deep sleep mode (AP system run linux kernel OS). >>> >>> Allright yes it makes sense, and also there are systems that just go into >>> "hardware sleep" and just put the pin into some pre-programmed mode. >>> >>> I'm a bit back-and-forth. I didn't mean that some code would actually >>> switch the state to "sleep" when we go to sleep, I meant that when >>> the system configures "default" mode it should also look up and >>> program the "sleep" mode, but this approach with a special property >>> is just another way of achieveing the same thing. >>> >>> But then we should add a whole slew of sleep states. >>> >>> I was thinking whether we could avoid having a special DT property >>> by parsing ahead to states we do not currently use and programming >>> that into the sleep mode registers. >> >> Yes, for most scenarios, it can work with the "sleep" state to set >> sleep-related config. But for our Spreadtrum platform scenario (I do >> not know if there are other platforms need this feature), we can not >> select the "sleep" state when pubcp system goes into deep sleep mode >> but ap system does not go into deep sleep mode. So I think we still >> need these "sleep-bias-pull-up", "sleep-bias-pull-down", >> "sleep-input-enable" and "sleep-output-enable" properties. > > I don't really mean you should select the "sleep" state. > > I meant, as part of setting the "default" state or even the "init" > state, we would inspect the "sleep" state, use those settings, and > program them into the registers at this early point. I understood your points. But we can not program all into the registers at one early point, sometimes these sleep-related configs need depend on some conditions in users' drivers, like on condition 1: driver need to set one pin as input-enable when specified system goes into deep sleep, on condition 2: driver need set this pin as output-enable when specified system goes into deep sleep. So I still think it is better if we introduce some standard sleep related configs. > > Then never touch the registers again, and never really go to the > sleep state by software, just by hardware. > > Yours, > Linus Walleij
On Tue, Jun 27, 2017 at 9:59 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 26 June 2017 at 06:19, Linus Walleij <linus.walleij@linaro.org> wrote: >> I meant, as part of setting the "default" state or even the "init" >> state, we would inspect the "sleep" state, use those settings, and >> program them into the registers at this early point. > > I understood your points. But we can not program all into the > registers at one early point, sometimes these sleep-related configs > need depend on some conditions in users' drivers, like on condition 1: > driver need to set one pin as input-enable when specified system goes > into deep sleep, on condition 2: driver need set this pin as > output-enable when specified system goes into deep sleep. So I still > think it is better if we introduce some standard sleep related > configs. So you mean you need special runtime states to set up sleep states? Do you have an example of how this would look in the device tree? I am sorry but I have a hard time following the use case :( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt new file mode 100644 index 0000000..2f544cd --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt @@ -0,0 +1,56 @@ +* Spreadtrum Pin Controller + +The Spreadtrum pin controller are organized in 3 blocks (types). + +The first block comprises some global control registers, and each +register contains several bit fields with one bit or several bits +to configure for some global common configuration, such as domain +pad driving level, system control select and so on. We recognise +every fields comprising one bit or several bits in one global control +register as one pin, thus we should record every pin's bit offset, +bit width and register offset to configure this field (pin). Since +this type pins' configuration are very tricky and different for each +register, we introduce "sprd,ctrl" property to set the various global +control configuration. + +The second block comprises some common registers which have unified +register definition, and each register described one pin is used +to configure the pin sleep mode and function select. Now we have 4 +systems on SC9860 SoC: AP system, PUBCP system, TGLDSP system and +AGDSP system. In some situation we have some pin-sleep related +configuration need to set when one of system goes into deep sleep +mode. For example, if we set the pin sleep mode as AP_SLEEP, which +means when AP system goes into deep sleep mode, this pin's sleep +related properties (input/output or pullup/down) will be set +automatically. Thus we intoduce "sprd,sleep_mode" to set pin sleep +mode. + +The last block comprises some misc registers which also have unified +register definition, and each register described one pin is used to +configure drive strength, pull up/down and so on. Especially for pull +up, we introduce "sprd,pullup" property for two kind configuration: +PULLUP_20K or PULLUP_4_7K, which means the pullup resistor is 20K or +4.7K. + +Required properties for Spreadtrum pin controller: +- compatible: "sprd,<soc>-pinctrl" + Please refer to each sprd,<soc>-pinctrl.txt binding doc for supported SoCs. +- reg: The register address of pin controller device. +- pins : An array of pin names. + +Optional properties: +- function: Specified the function name. +- drive-strength: Drive strength in mA. +- input-schmitt-disable: Enable schmitt-trigger mode. +- input-schmitt-enable: Disable schmitt-trigger mode. +- bias-disable: Disable pin bias. +- bias-pull-down: Pull down on pin. +- sprd,ctrl: Control values referring to databook for global control pins. +- sprd,sleep_mode: Sleep mode selection. +- sprd,pullup: Pull up on pin. +- sprd,input-sleep: Input enable when system goes into deep sleep mode. +- sprd,output-sleep: Output enable when system goes into deep sleep mode. +- sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. +- sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. + +Please refer to each sprd,<soc>-pinctrl.txt binding doc for supported values. diff --git a/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt new file mode 100644 index 0000000..10b77e1 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt @@ -0,0 +1,66 @@ +* Spreadtrum SC9860 Pin Controller + +Please refer to sprd,pinctrl.txt in this directory for common binding part +and usage. + +Required properties: +- compatible: Must be "sprd,sc9860-pinctrl". +- reg: The register address of pin controller device. +- pins : An array of strings, each string containing the name of a pin. + +Optional properties: +- function: A string containing the name of the function, values must be + one of: "func1", "func2", "func3" and "func4". +- drive-strength: Drive strength in mA. Supported values: 2, 4, 6, 8, 10, + 12, 14, 16, 20, 21, 24, 25, 27, 29, 31 and 33. +- input-schmitt-disable: Enable schmitt-trigger mode. +- input-schmitt-enable: Disable schmitt-trigger mode. +- bias-disable: Disable pin bias. +- bias-pull-down: Pull down on pin. +- sprd,ctrl: Control values referring to databook for global control pins. +- sprd,sleep_mode: Choose the pin sleep mode, and supported values are: + AP_SLEEP, PUBCP_SLEEP, TGLDSP_SLEEP and AGDSP_SLEEP. +- sprd,pullup: Pull up on pin, the value should be PULLUP_20K or PULLUP_4_7K. +- sprd,input-sleep: Input enable when system goes into deep sleep mode. +- sprd,output-sleep: Output enable when system goes into deep sleep mode. +- sprd,pullup_sleep: Pull up enable when system goes into deep sleep mode. +- sprd,pulldown_sleep: Pull down enable when system goes into deep sleep mode. + +Pin sleep mode definition: +enum pin_sleep_mode { + AP_SLEEP = BIT(0), + PUBCP_SLEEP = BIT(1), + TGLDSP_SLEEP = BIT(2), + AGDSP_SLEEP = BIT(3), +}; + +Pin pullup mode definition: +enum pin_pullup_sel { + PULLUP_20K, + PULLUP_4_7K, +}; + +Example: +pin_controller: pinctrl@402a0000 { + compatible = "sprd,sc9860-pinctrl"; + reg = <0x402a0000 0x10000>; + + grp1: sd0 { + pins = "SC9860_VIO_SD2_IRTE", "SC9860_VIO_SD0_IRTE"; + sprd,ctrl = <0x1>; + }; + + grp2: rfctl_33 { + pins = "SC9860_RFCTL33"; + function = "func2"; + sprd,sleep_mode = <AP_SLEEP | PUBCP_SLEEP>; + sprd,output-sleep; + }; + + grp3: rfctl_misc_20 { + pins = "SC9860_RFCTL20_MISC"; + drive-strength = <10>; + sprd,pullup = <PULLUP_4_7K>; + sprd,pullup-sleep; + }; +};
This patch adds the binding documentation for Spreadtrum SC9860 pin controller device. Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> --- Changes since v1: - Remove magic numbers and get to use the standard bindings. - Fix some typos. --- .../devicetree/bindings/pinctrl/sprd,pinctrl.txt | 56 +++++++++++++++++ .../bindings/pinctrl/sprd,sc9860-pinctrl.txt | 66 ++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/sprd,pinctrl.txt create mode 100644 Documentation/devicetree/bindings/pinctrl/sprd,sc9860-pinctrl.txt