This is an attempt to deal with i.MX SoC PWM HW limitation.
When a pad is configured as a PWM output the output level is always 0V
if the PWM block is disabled. This can cause problems when inverted PWM
signal is needed to drive the connected circuit. With inverted output
duty cycle = 0% corresponds to high output level and duty cycle = 100%
corresponds to low output level. This means that whenever the PWM block
is disabled the connected circuit is fed with 100% duty cycle.
This binding is totally optional and current users are not affected.
The idea is to use two new pinctrl states, "pwm" and "gpio", instead of
the "default" pinctrl state. The gpio state is selected when PWM is
disabled and the pwm pinctrl state is selected when PWM is enabled. In the
gpio state level on the output is controlled by the internal pull-up.
Michal Vokáč (2):
dt-bindings: pwm: imx: Allow switching PWM output between PWM and GPIO
pwm: imx: Configure output to GPIO in disabled state
Documentation/devicetree/bindings/pwm/imx-pwm.txt | 49 +++++++++++++++
drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++
2 files changed, 126 insertions(+)
--
2.1.4
Output of the PWM block on i.MX SoCs is always low when the block is
disabled. This can cause issues when inverted PWM polarity is needed.
With inverted polarity a duty cycle = 0% corresponds to high level on
the output. Now, when PWM is disabled its output instantly goes low
which corresponds to duty cycle = 100%.
To get a truly inverted PWM output two pinctrl states of the PWM pin
can be used. Configure the pin to GPIO function when PWM is disabled
and switch back to PWM function whenever non-zero duty cycle is needed.
Signed-off-by: Michal Vokáč <[email protected]>
---
Changes in v3:
- Slightly different description of the pinctrl and pwm-gpio.
Changes in v2:
- Do not use the "default" pinctrl state for GPIO.
- Use two new "pwm" and "gpio" pinctrl states.
- Add a new pwm-gpios signal.
Documentation/devicetree/bindings/pwm/imx-pwm.txt | 49 +++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index c61bdf8..2a555b6 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -14,6 +14,16 @@ See the clock consumer binding,
Documentation/devicetree/bindings/clock/clock-bindings.txt
- interrupts: The interrupt for the pwm controller
+Optional properties:
+- pinctrl: For i.MX27 and newer SoCs. Use "pwm" and "gpio" specific pinctrls
+ instead of the "default" to configure the PWM pin to GPIO and PWM function.
+ It allows control over the pin output level when the PWM block is disabled.
+ This is useful if you use the PWM for single purpose and you need inverted
+ polarity of the PWM signal. See "Inverted PWM output" section bellow.
+- pwm-gpios: Specify the GPIO pin that will act as the PWM output. This should
+ be the same pin as is used for normal PWM output. See "Inverted PWM output"
+ section bellow.
+
Example:
pwm1: pwm@53fb4000 {
@@ -25,3 +35,42 @@ pwm1: pwm@53fb4000 {
clock-names = "ipg", "per";
interrupts = <61>;
};
+
+Inverted PWM output
+-------------------
+
+The i.MX SoC has such limitation that whenever a pin is configured as a PWM
+output, the output level on the pin is always low when the PWM block is
+disabled. The low output level is actively driven by the output stage of the
+PWM block and it does not matter what polarity a PWM client (e.g. backlight)
+requested.
+
+To gain control of the output level in PWM disabled state two pinctrl states
+can be used. A "pwm" state and a "gpio" state. In the pwm state the pin is
+configured as a normal PWM output. In the gpio state the pin is configured as
+a GPIO input. In the gpio state the output level is controlled by the pull-up
+setting. This setup assures that the PWM output is at the required level that
+corresponds to duty cycle = 0 when PWM is disabled.
+
+Example:
+
+&pwm1 {
+ pinctrl-names = "pwm", "gpio";
+ pinctrl-0 = <&pinctrl_backlight_pwm>;
+ pinctrl-1 = <&pinctrl_backlight_gpio>;
+ pwm-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>
+}
+
+pinctrl_backlight_gpio: pwm1grp-gpio {
+ fsl,pins = <
+ /* GPIO with 100kOhm pull-up */
+ MX6QDL_PAD_GPIO_9__GPIO1_IO09 0xb000
+ >;
+};
+
+pinctrl_backlight_pwm: pwm1grp-pwm {
+ fsl,pins = <
+ /* PWM output */
+ MX6QDL_PAD_GPIO_9__PWM1_OUT 0x8
+ >;
+};
--
2.1.4
Normally the PWM output is held LOW when PWM is disabled. This can cause
problems when inverted PWM signal polarity is needed. With this behavior
the connected circuit is fed by 100% duty cycle instead of being shut-off.
Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
state is selected when PWM is enabled and the gpio pinctrl state is
selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
configured as input and the internal pull-up resistor is used to pull the
output level high.
If all the pinctrl states and the pwm-gpios GPIO are not correctly
specified in DT the PWM work as usual.
As an example, with this patch a PWM controlled backlight with inversed
signal polarity can be used in full brightness range. Without this patch
the backlight can not be turned off as brightness = 0 disables the PWM
and that in turn set PWM output LOW, that is full brightness.
Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
+--------------+------------+---------------+----------- +-------------+
| After reset | Bootloader | PWM probe | PWM | PWM |
| 100k pull-up | | | enable 30% | disable |
+--------------+------------+---------------+------------+-------------+
| pinctrl | none | default | default | default |
| out H __________________ __ __ |
| out L \_________________/ \_/ \_/\____________ |
| ^ ^ ^ |
+--------------+------------+---------------+------------+-------------+
| pinctrl | none | gpio | pwm | gpio |
| out H __________________________________ __ __ _____________ |
| out L \_/ \_/ \_/ |
| ^ ^ ^ |
+----------------------------------------------------------------------+
Signed-off-by: Michal Vokáč <[email protected]>
---
Changes in v3:
- Commit message update.
- Minor fix in code comment (Uwe)
- Align function arguments to the opening parentheses. (Uwe)
- Do not test devm_pinctrl_get for NULL. (Thierry)
- Convert all messages to dev_dbg() (Thierry)
- Do not actively drive the pin in gpio state. Configure it as input
and rely solely on the internal pull-up. (Thierry)
Changes in v2:
- Utilize the "pwm" and "gpio" pinctrl states.
- Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
- Select the right pinctrl state in probe.
drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 1d5242c..84eaec8 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -12,10 +12,12 @@
#include <linux/err.h>
#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
#include <linux/io.h>
#include <linux/pwm.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
/* i.MX1 and i.MX21 share the same PWM function block: */
@@ -52,10 +54,44 @@ struct imx_chip {
void __iomem *mmio_base;
struct pwm_chip chip;
+
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pinctrl_pins_gpio;
+ struct pinctrl_state *pinctrl_pins_pwm;
+ struct gpio_desc *pwm_gpiod;
};
#define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
+static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
+ struct platform_device *pdev)
+{
+ imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(imx_chip->pinctrl)) {
+ dev_dbg(&pdev->dev, "can not get pinctrl\n");
+ return PTR_ERR(imx_chip->pinctrl);
+ }
+
+ imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
+ "pwm");
+ imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
+ "gpio");
+ imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
+ GPIOD_IN);
+
+ if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(imx_chip->pwm_gpiod) ||
+ IS_ERR(imx_chip->pinctrl_pins_pwm) ||
+ IS_ERR(imx_chip->pinctrl_pins_gpio)) {
+ dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
+ devm_pinctrl_put(imx_chip->pinctrl);
+ imx_chip->pinctrl = NULL;
+ }
+
+ return 0;
+}
+
static int imx_pwm_config_v1(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -216,7 +252,25 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
cr |= MX3_PWMCR_POUTC;
writel(cr, imx->mmio_base + MX3_PWMCR);
+
+ /*
+ * If we are in charge of pinctrl then switch output to
+ * the PWM signal.
+ */
+ if (imx->pinctrl)
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_pwm);
} else if (cstate.enabled) {
+ /*
+ * PWM block will be disabled. Normally its output will be set
+ * low no matter what output polarity is configured. Let's use
+ * pinctrl to switch the output pin to GPIO functon and keep
+ * the output at the same level as for duty-cycle = 0.
+ */
+ if (imx->pinctrl)
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_gpio);
+
writel(0, imx->mmio_base + MX3_PWMCR);
clk_disable_unprepare(imx->clk_per);
@@ -263,6 +317,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
const struct of_device_id *of_id =
of_match_device(imx_pwm_dt_ids, &pdev->dev);
const struct imx_pwm_data *data;
+ struct pwm_state cstate;
struct imx_chip *imx;
struct resource *r;
int ret = 0;
@@ -294,6 +349,10 @@ static int imx_pwm_probe(struct platform_device *pdev)
imx->chip.of_pwm_n_cells = 3;
}
+ ret = imx_pwm_init_pinctrl_info(imx, pdev);
+ if (ret)
+ return ret;
+
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
if (IS_ERR(imx->mmio_base))
@@ -303,6 +362,24 @@ static int imx_pwm_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
+ if (imx->pinctrl) {
+ /*
+ * Update cstate after pwmchip_add() call as the core might
+ * call the get_state() function to read the PWM registers
+ * to get the actual HW state.
+ */
+ pwm_get_state(imx->chip.pwms, &cstate);
+ if (cstate.enabled) {
+ dev_dbg(&pdev->dev,
+ "PWM entered probe in enabled state\n");
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_pwm);
+ } else {
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_gpio);
+ }
+ }
+
platform_set_drvdata(pdev, imx);
return 0;
}
--
2.1.4
On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> Normally the PWM output is held LOW when PWM is disabled. This can cause
> problems when inverted PWM signal polarity is needed. With this behavior
> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>
> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> state is selected when PWM is enabled and the gpio pinctrl state is
> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> configured as input and the internal pull-up resistor is used to pull the
> output level high.
>
> If all the pinctrl states and the pwm-gpios GPIO are not correctly
> specified in DT the PWM work as usual.
>
> As an example, with this patch a PWM controlled backlight with inversed
> signal polarity can be used in full brightness range. Without this patch
> the backlight can not be turned off as brightness = 0 disables the PWM
> and that in turn set PWM output LOW, that is full brightness.
>
> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>
> +--------------+------------+---------------+----------- +-------------+
> | After reset | Bootloader | PWM probe | PWM | PWM |
> | 100k pull-up | | | enable 30% | disable |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | default | default | default |
> | out H __________________ __ __ |
> | out L \_________________/ \_/ \_/\____________ |
> | ^ ^ ^ |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | gpio | pwm | gpio |
> | out H __________________________________ __ __ _____________ |
> | out L \_/ \_/ \_/ |
> | ^ ^ ^ |
> +----------------------------------------------------------------------+
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> Changes in v3:
> - Commit message update.
> - Minor fix in code comment (Uwe)
> - Align function arguments to the opening parentheses. (Uwe)
> - Do not test devm_pinctrl_get for NULL. (Thierry)
> - Convert all messages to dev_dbg() (Thierry)
> - Do not actively drive the pin in gpio state. Configure it as input
> and rely solely on the internal pull-up. (Thierry)
>
> Changes in v2:
> - Utilize the "pwm" and "gpio" pinctrl states.
> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
> - Select the right pinctrl state in probe.
>
> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 1d5242c..84eaec8 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -12,10 +12,12 @@
> #include <linux/err.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/io.h>
> #include <linux/pwm.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>
> /* i.MX1 and i.MX21 share the same PWM function block: */
>
> @@ -52,10 +54,44 @@ struct imx_chip {
> void __iomem *mmio_base;
>
> struct pwm_chip chip;
> +
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *pinctrl_pins_gpio;
> + struct pinctrl_state *pinctrl_pins_pwm;
> + struct gpio_desc *pwm_gpiod;
> };
>
> #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>
> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
> + struct platform_device *pdev)
Please indent the follow up line to the opening parenthesis.
> +{
> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(imx_chip->pinctrl)) {
> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
> + return PTR_ERR(imx_chip->pinctrl);
> + }
> +
> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
> + "pwm");
> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
> + "gpio");
> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
> + GPIOD_IN);
> +
> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(imx_chip->pwm_gpiod) ||
> + IS_ERR(imx_chip->pinctrl_pins_pwm) ||
> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
> + devm_pinctrl_put(imx_chip->pinctrl);
> + imx_chip->pinctrl = NULL;
Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
-EIO? I think you want to put the gpio if the failure is because there
is a pinctrl related error.
> + }
> +
> + return 0;
> +}
> +
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -216,7 +252,25 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> cr |= MX3_PWMCR_POUTC;
>
> writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> + /*
> + * If we are in charge of pinctrl then switch output to
> + * the PWM signal.
> + */
> + if (imx->pinctrl)
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_pwm);
> } else if (cstate.enabled) {
> + /*
> + * PWM block will be disabled. Normally its output will be set
> + * low no matter what output polarity is configured. Let's use
> + * pinctrl to switch the output pin to GPIO functon and keep
> + * the output at the same level as for duty-cycle = 0.
> + */
> + if (imx->pinctrl)
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_gpio);
> +
> writel(0, imx->mmio_base + MX3_PWMCR);
>
> clk_disable_unprepare(imx->clk_per);
> @@ -263,6 +317,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
> const struct of_device_id *of_id =
> of_match_device(imx_pwm_dt_ids, &pdev->dev);
> const struct imx_pwm_data *data;
> + struct pwm_state cstate;
> struct imx_chip *imx;
> struct resource *r;
> int ret = 0;
> @@ -294,6 +349,10 @@ static int imx_pwm_probe(struct platform_device *pdev)
> imx->chip.of_pwm_n_cells = 3;
> }
>
> + ret = imx_pwm_init_pinctrl_info(imx, pdev);
> + if (ret)
> + return ret;
> +
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> if (IS_ERR(imx->mmio_base))
> @@ -303,6 +362,24 @@ static int imx_pwm_probe(struct platform_device *pdev)
> if (ret < 0)
> return ret;
>
> + if (imx->pinctrl) {
> + /*
> + * Update cstate after pwmchip_add() call as the core might
> + * call the get_state() function to read the PWM registers
> + * to get the actual HW state.
> + */
> + pwm_get_state(imx->chip.pwms, &cstate);
> + if (cstate.enabled) {
> + dev_dbg(&pdev->dev,
> + "PWM entered probe in enabled state\n");
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_pwm);
> + } else {
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_gpio);
> + }
> + }
> +
ISTR that there was a patch that implements get_state for imx. Is there
a dependency on that one? Otherwise the state returned by
pwm_get_state() might not be what is actually configured.
Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
I ask because of https://patchwork.ozlabs.org/patch/1000071/
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 6.12.2018 14:59, Uwe Kleine-König wrote:
> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>
>> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
>> + struct platform_device *pdev)
>
> Please indent the follow up line to the opening parenthesis.
Meh, I overlooked that one. I will fix it.
>> +{
>> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
>> + if (IS_ERR(imx_chip->pinctrl)) {
>> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
>> + return PTR_ERR(imx_chip->pinctrl);
>> + }
>> +
>> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "pwm");
>> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "gpio");
>> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
>> + GPIOD_IN);
>> +
>> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(imx_chip->pwm_gpiod) ||
>> + IS_ERR(imx_chip->pinctrl_pins_pwm) ||
>> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
>> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
>> + devm_pinctrl_put(imx_chip->pinctrl);
>> + imx_chip->pinctrl = NULL;
>
> Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
No. The pinctrl_lookup_state either returns pointer to the pinctrl state
or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
for PTR_ERR(-EPROBE_DEFER), or do I?
> Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
> -EIO? I think you want to put the gpio if the failure is because there
> is a pinctrl related error.
I think that is what I am doing. In case the GPIO is not ready the probe
is deferred. In case of any other error with the GPIO or pinctrl failure
I put the pinctrl. Or maybe I do not really understand what you mean.
>> + }
>> +
>> + return 0;
>> +}
>> +
[...]
>> @@ -303,6 +362,24 @@ static int imx_pwm_probe(struct platform_device *pdev)
>> if (ret < 0)
>> return ret;
>>
>> + if (imx->pinctrl) {
>> + /*
>> + * Update cstate after pwmchip_add() call as the core might
>> + * call the get_state() function to read the PWM registers
>> + * to get the actual HW state.
>> + */
>> + pwm_get_state(imx->chip.pwms, &cstate);
>> + if (cstate.enabled) {
>> + dev_dbg(&pdev->dev,
>> + "PWM entered probe in enabled state\n");
>> + pinctrl_select_state(imx->pinctrl,
>> + imx->pinctrl_pins_pwm);
>> + } else {
>> + pinctrl_select_state(imx->pinctrl,
>> + imx->pinctrl_pins_gpio);
>> + }
>> + }
>> +
>
> ISTR that there was a patch that implements get_state for imx. Is there
> a dependency on that one? Otherwise the state returned by
> pwm_get_state() might not be what is actually configured.
No, it should be independent. One can go without the other. I tested all
three combinations (mainline with .get_state, mainline with this series,
mainline with .get_state AND this series) and got the expected results.
Without the .get_state() patch the core always returns the default which
is disabled state so the gpio pinctrl state is selected in probe.
> Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
> I ask because of https://patchwork.ozlabs.org/patch/1000071/
Yep, I am aware of that patch. IMHO this is not needed for the v1 on
older i.MX SoCs but I do not have a hands-on experience with those.
Thank you for the review Uwe!
Michal
On Thu, Dec 06, 2018 at 03:37:55PM +0000, Vokáč Michal wrote:
> On 6.12.2018 14:59, Uwe Kleine-König wrote:
> > On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> >> +{
> >> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> >> + if (IS_ERR(imx_chip->pinctrl)) {
> >> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
> >> + return PTR_ERR(imx_chip->pinctrl);
> >> + }
> >> +
> >> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
> >> + "pwm");
> >> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
> >> + "gpio");
> >> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
> >> + GPIOD_IN);
> >> +
> >> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
> >> + return -EPROBE_DEFER;
> >> + } else if (IS_ERR(imx_chip->pwm_gpiod) ||
> >> + IS_ERR(imx_chip->pinctrl_pins_pwm) ||
> >> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
> >> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
> >> + devm_pinctrl_put(imx_chip->pinctrl);
> >> + imx_chip->pinctrl = NULL;
> >
> > Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
>
> No. The pinctrl_lookup_state either returns pointer to the pinctrl state
> or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
> for PTR_ERR(-EPROBE_DEFER), or do I?
You don't, I just wondered if this could happen and the function should
return -EPROBE_DEFER in this case, too.
> > Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
> > -EIO? I think you want to put the gpio if the failure is because there
> > is a pinctrl related error.
>
> I think that is what I am doing. In case the GPIO is not ready the probe
> is deferred. In case of any other error with the GPIO or pinctrl failure
> I put the pinctrl. Or maybe I do not really understand what you mean.
Yes, you put the pinctrl, but not the GPIO. I.e. you're not undoing
devm_gpiod_get_optional(). Maybe only do this if the pinctrl stuff
succeeded to not touch the GPIO if it won't be used?
> > ISTR that there was a patch that implements get_state for imx. Is there
> > a dependency on that one? Otherwise the state returned by
> > pwm_get_state() might not be what is actually configured.
>
> No, it should be independent. One can go without the other. I tested all
> three combinations (mainline with .get_state, mainline with this series,
> mainline with .get_state AND this series) and got the expected results.
> Without the .get_state() patch the core always returns the default which
> is disabled state so the gpio pinctrl state is selected in probe.
Without .get_state it won't be possible to smoothly take over a running
PWM. It doesn't hurt if the PWM isn't running though. Still I'd like to
see the .get_state patch to go in first to not get this (admittedly
small) regression.
> > Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
> > I ask because of https://patchwork.ozlabs.org/patch/1000071/
>
> Yep, I am aware of that patch. IMHO this is not needed for the v1 on
> older i.MX SoCs but I do not have a hands-on experience with those.
OK. If you agree with my split and as you have to rework your patch
anyhow: Would you mind to rebase on top of my patch series? (Unless
Thierry disagrees with my patches, but unfortunately he didn't comment
yet.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Dec 10, 2018 at 11:15:05AM +0000, Vokáč Michal wrote:
> On 6.12.2018 17:16, Uwe Kleine-König wrote:
> > On Thu, Dec 06, 2018 at 03:37:55PM +0000, Vokáč Michal wrote:
> >> On 6.12.2018 14:59, Uwe Kleine-König wrote:
> >>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> >>>
> >>> Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
> >>
> >> No. The pinctrl_lookup_state either returns pointer to the pinctrl state
> >> or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
> >> for PTR_ERR(-EPROBE_DEFER), or do I?
> >
> > You don't, I just wondered if this could happen and the function should
> > return -EPROBE_DEFER in this case, too.
>
> OK.
>
> >>> Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
> >>> -EIO? I think you want to put the gpio if the failure is because there
> >>> is a pinctrl related error.
> >>
> >> I think that is what I am doing. In case the GPIO is not ready the probe
> >> is deferred. In case of any other error with the GPIO or pinctrl failure
> >> I put the pinctrl. Or maybe I do not really understand what you mean.
> >
> > Yes, you put the pinctrl, but not the GPIO. I.e. you're not undoing
> > devm_gpiod_get_optional(). Maybe only do this if the pinctrl stuff
> > succeeded to not touch the GPIO if it won't be used?
>
> OK, I agree it seems better to get the pinctrl first and if it succeeds
> only then try to get the GPIO. In that case I need to use the non-optional
> variant of devm_gpio_get(). Note that then I do not really need to put the
> GPIO in the error path as it means I did not get it.
> The code would look like:
>
> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
> + struct platform_device *pdev)
> +{
> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(imx_chip->pinctrl)) {
> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
> + return PTR_ERR(imx_chip->pinctrl);
> + }
> +
> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
> + "pwm");
> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
> + "gpio");
> +
> + if (IS_ERR(imx_chip->pinctrl_pins_pwm) ||
> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
> + dev_dbg(&pdev->dev, "pinctrl information incomplete\n");
> + goto out;
> + }
> +
> + imx_chip->pwm_gpiod = devm_gpiod_get(&pdev->dev, "pwm", GPIOD_IN);
> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(imx_chip->pwm_gpiod)) {
> + dev_dbg(&pdev->dev, "GPIO information incomplete\n");
> + goto out;
> + }
> +
> + return 0;
> +
> +out:
> + devm_pinctrl_put(imx_chip->pinctrl);
> + imx_chip->pinctrl = NULL;
> +
> + return 0;
> +}
This looks right.
> >>> ISTR that there was a patch that implements get_state for imx. Is there
> >>> a dependency on that one? Otherwise the state returned by
> >>> pwm_get_state() might not be what is actually configured.
> >>
> >> No, it should be independent. One can go without the other. I tested all
> >> three combinations (mainline with .get_state, mainline with this series,
> >> mainline with .get_state AND this series) and got the expected results.
> >> Without the .get_state() patch the core always returns the default which
> >> is disabled state so the gpio pinctrl state is selected in probe.
> >
> > Without .get_state it won't be possible to smoothly take over a running
> > PWM.
>
> But that is exactly how the current pwm-imx code works, right?
But then at least the pwm would run until the first consumer
reconfigures it.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 10.12.2018 12:17, Uwe Kleine-König wrote:
> On Mon, Dec 10, 2018 at 11:15:05AM +0000, Vokáč Michal wrote:
>> On 6.12.2018 17:16, Uwe Kleine-König wrote:
>>> On Thu, Dec 06, 2018 at 03:37:55PM +0000, Vokáč Michal wrote:
>>>> On 6.12.2018 14:59, Uwe Kleine-König wrote:
>>>>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>>>>
>>>>> Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
>>>>
>>>> No. The pinctrl_lookup_state either returns pointer to the pinctrl state
>>>> or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
>>>> for PTR_ERR(-EPROBE_DEFER), or do I?
>>>
>>> You don't, I just wondered if this could happen and the function should
>>> return -EPROBE_DEFER in this case, too.
>>
>> OK.
>>
>>>>> Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
>>>>> -EIO? I think you want to put the gpio if the failure is because there
>>>>> is a pinctrl related error.
>>>>
>>>> I think that is what I am doing. In case the GPIO is not ready the probe
>>>> is deferred. In case of any other error with the GPIO or pinctrl failure
>>>> I put the pinctrl. Or maybe I do not really understand what you mean.
>>>
>>> Yes, you put the pinctrl, but not the GPIO. I.e. you're not undoing
>>> devm_gpiod_get_optional(). Maybe only do this if the pinctrl stuff
>>> succeeded to not touch the GPIO if it won't be used?
>>
>> OK, I agree it seems better to get the pinctrl first and if it succeeds
>> only then try to get the GPIO. In that case I need to use the non-optional
>> variant of devm_gpio_get(). Note that then I do not really need to put the
>> GPIO in the error path as it means I did not get it.
>> The code would look like:
>>
>> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
>> + struct platform_device *pdev)
>> +{
>> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
>> + if (IS_ERR(imx_chip->pinctrl)) {
>> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
>> + return PTR_ERR(imx_chip->pinctrl);
>> + }
>> +
>> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "pwm");
>> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "gpio");
>> +
>> + if (IS_ERR(imx_chip->pinctrl_pins_pwm) ||
>> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
>> + dev_dbg(&pdev->dev, "pinctrl information incomplete\n");
>> + goto out;
>> + }
>> +
>> + imx_chip->pwm_gpiod = devm_gpiod_get(&pdev->dev, "pwm", GPIOD_IN);
>> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(imx_chip->pwm_gpiod)) {
>> + dev_dbg(&pdev->dev, "GPIO information incomplete\n");
>> + goto out;
>> + }
>> +
>> + return 0;
>> +
>> +out:
>> + devm_pinctrl_put(imx_chip->pinctrl);
>> + imx_chip->pinctrl = NULL;
>> +
>> + return 0;
>> +}
>
> This looks right.
Wow, you're quick! OK, thanks.
>>>>> ISTR that there was a patch that implements get_state for imx. Is there
>>>>> a dependency on that one? Otherwise the state returned by
>>>>> pwm_get_state() might not be what is actually configured.
>>>>
>>>> No, it should be independent. One can go without the other. I tested all
>>>> three combinations (mainline with .get_state, mainline with this series,
>>>> mainline with .get_state AND this series) and got the expected results.
>>>> Without the .get_state() patch the core always returns the default which
>>>> is disabled state so the gpio pinctrl state is selected in probe.
>>>
>>> Without .get_state it won't be possible to smoothly take over a running
>>> PWM.
>>
>> But that is exactly how the current pwm-imx code works, right?
>
> But then at least the pwm would run until the first consumer
> reconfigures it.
That is right. I think it is actually possible (and maybe good idea?)
to drop the probe part from this pinctrl/GPIO series and put it into
the .get_state series if the .get_state series would land in later.
What do you think?
Michal
On 6.12.2018 17:16, Uwe Kleine-König wrote:
> On Thu, Dec 06, 2018 at 03:37:55PM +0000, Vokáč Michal wrote:
>> On 6.12.2018 14:59, Uwe Kleine-König wrote:
>>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>>
>>> Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
>>
>> No. The pinctrl_lookup_state either returns pointer to the pinctrl state
>> or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
>> for PTR_ERR(-EPROBE_DEFER), or do I?
>
> You don't, I just wondered if this could happen and the function should
> return -EPROBE_DEFER in this case, too.
OK.
>>> Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
>>> -EIO? I think you want to put the gpio if the failure is because there
>>> is a pinctrl related error.
>>
>> I think that is what I am doing. In case the GPIO is not ready the probe
>> is deferred. In case of any other error with the GPIO or pinctrl failure
>> I put the pinctrl. Or maybe I do not really understand what you mean.
>
> Yes, you put the pinctrl, but not the GPIO. I.e. you're not undoing
> devm_gpiod_get_optional(). Maybe only do this if the pinctrl stuff
> succeeded to not touch the GPIO if it won't be used?
OK, I agree it seems better to get the pinctrl first and if it succeeds
only then try to get the GPIO. In that case I need to use the non-optional
variant of devm_gpio_get(). Note that then I do not really need to put the
GPIO in the error path as it means I did not get it.
The code would look like:
+static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
+ struct platform_device *pdev)
+{
+ imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(imx_chip->pinctrl)) {
+ dev_dbg(&pdev->dev, "can not get pinctrl\n");
+ return PTR_ERR(imx_chip->pinctrl);
+ }
+
+ imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
+ "pwm");
+ imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
+ "gpio");
+
+ if (IS_ERR(imx_chip->pinctrl_pins_pwm) ||
+ IS_ERR(imx_chip->pinctrl_pins_gpio)) {
+ dev_dbg(&pdev->dev, "pinctrl information incomplete\n");
+ goto out;
+ }
+
+ imx_chip->pwm_gpiod = devm_gpiod_get(&pdev->dev, "pwm", GPIOD_IN);
+ if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(imx_chip->pwm_gpiod)) {
+ dev_dbg(&pdev->dev, "GPIO information incomplete\n");
+ goto out;
+ }
+
+ return 0;
+
+out:
+ devm_pinctrl_put(imx_chip->pinctrl);
+ imx_chip->pinctrl = NULL;
+
+ return 0;
+}
>>> ISTR that there was a patch that implements get_state for imx. Is there
>>> a dependency on that one? Otherwise the state returned by
>>> pwm_get_state() might not be what is actually configured.
>>
>> No, it should be independent. One can go without the other. I tested all
>> three combinations (mainline with .get_state, mainline with this series,
>> mainline with .get_state AND this series) and got the expected results.
>> Without the .get_state() patch the core always returns the default which
>> is disabled state so the gpio pinctrl state is selected in probe.
>
> Without .get_state it won't be possible to smoothly take over a running
> PWM.
But that is exactly how the current pwm-imx code works, right?
> It doesn't hurt if the PWM isn't running though. Still I'd like to
> see the .get_state patch to go in first to not get this (admittedly
> small) regression.
I would not mind that. The problem is that the .get_state patch have
not received any comments yet. I planned to resend it once this series
is in. If we choose to fine-tune the .get_state patch first and then
put this series on top of that I am afraid we will miss another few
releases.
>>> Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
>>> I ask because of https://patchwork.ozlabs.org/patch/1000071/
>>
>> Yep, I am aware of that patch. IMHO this is not needed for the v1 on
>> older i.MX SoCs but I do not have a hands-on experience with those.
>
> OK. If you agree with my split and as you have to rework your patch
> anyhow: Would you mind to rebase on top of my patch series? (Unless
> Thierry disagrees with my patches, but unfortunately he didn't comment
> yet.)
No problem for me to rebase on top of your series. But it really
depends on Thierry. And it generates the same problem as I mentioned
above. Unlike your split and my .get_state series the pinctrl/GPIO
series has already been thoroughly discussed and seems to be quite
close to be accepted. Maybe it is not a good idea to postpone that
with the current pace of patches merged per release into PWM subsystem.
The other thing is we mostly discussed the concept so far. So now
as it looks like this pinctrl/GPIO series may be eventually merged
a review on the dt-binding part is also needed. Hopefully it gets
Rob's attention once submitted as non-RFC.
Best regards,
Michal
On Thu, 6 Dec 2018 13:41:30 +0000, =?utf-8?B?Vm9rw6HEjSBNaWNoYWw=?= wrote:
> Output of the PWM block on i.MX SoCs is always low when the block is
> disabled. This can cause issues when inverted PWM polarity is needed.
> With inverted polarity a duty cycle = 0% corresponds to high level on
> the output. Now, when PWM is disabled its output instantly goes low
> which corresponds to duty cycle = 100%.
>
> To get a truly inverted PWM output two pinctrl states of the PWM pin
> can be used. Configure the pin to GPIO function when PWM is disabled
> and switch back to PWM function whenever non-zero duty cycle is needed.
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> Changes in v3:
> - Slightly different description of the pinctrl and pwm-gpio.
>
> Changes in v2:
> - Do not use the "default" pinctrl state for GPIO.
> - Use two new "pwm" and "gpio" pinctrl states.
> - Add a new pwm-gpios signal.
>
> Documentation/devicetree/bindings/pwm/imx-pwm.txt | 49 +++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
Hello,
On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> Normally the PWM output is held LOW when PWM is disabled. This can cause
> problems when inverted PWM signal polarity is needed. With this behavior
> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>
> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> state is selected when PWM is enabled and the gpio pinctrl state is
> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> configured as input and the internal pull-up resistor is used to pull the
> output level high.
>
> If all the pinctrl states and the pwm-gpios GPIO are not correctly
> specified in DT the PWM work as usual.
>
> As an example, with this patch a PWM controlled backlight with inversed
> signal polarity can be used in full brightness range. Without this patch
> the backlight can not be turned off as brightness = 0 disables the PWM
> and that in turn set PWM output LOW, that is full brightness.
>
> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>
> +--------------+------------+---------------+----------- +-------------+
> | After reset | Bootloader | PWM probe | PWM | PWM |
> | 100k pull-up | | | enable 30% | disable |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | default | default | default |
> | out H __________________ __ __ |
> | out L \_________________/ \_/ \_/\____________ |
> | ^ ^ ^ |
> +--------------+------------+---------------+------------+-------------+
> | pinctrl | none | gpio | pwm | gpio |
> | out H __________________________________ __ __ _____________ |
> | out L \_/ \_/ \_/ |
> | ^ ^ ^ |
> +----------------------------------------------------------------------+
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> Changes in v3:
> - Commit message update.
> - Minor fix in code comment (Uwe)
> - Align function arguments to the opening parentheses. (Uwe)
> - Do not test devm_pinctrl_get for NULL. (Thierry)
> - Convert all messages to dev_dbg() (Thierry)
> - Do not actively drive the pin in gpio state. Configure it as input
> and rely solely on the internal pull-up. (Thierry)
>
> Changes in v2:
> - Utilize the "pwm" and "gpio" pinctrl states.
> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
> - Select the right pinctrl state in probe.
>
> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 1d5242c..84eaec8 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -12,10 +12,12 @@
> #include <linux/err.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/io.h>
> #include <linux/pwm.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>
> /* i.MX1 and i.MX21 share the same PWM function block: */
>
> @@ -52,10 +54,44 @@ struct imx_chip {
> void __iomem *mmio_base;
>
> struct pwm_chip chip;
> +
> + struct pinctrl *pinctrl;
> + struct pinctrl_state *pinctrl_pins_gpio;
> + struct pinctrl_state *pinctrl_pins_pwm;
> + struct gpio_desc *pwm_gpiod;
> };
>
> #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>
> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
> + struct platform_device *pdev)
> +{
> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(imx_chip->pinctrl)) {
> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
> + return PTR_ERR(imx_chip->pinctrl);
> + }
> +
> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
> + "pwm");
> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
> + "gpio");
> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
> + GPIOD_IN);
> +
> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else if (IS_ERR(imx_chip->pwm_gpiod) ||
> + IS_ERR(imx_chip->pinctrl_pins_pwm) ||
> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
> + devm_pinctrl_put(imx_chip->pinctrl);
> + imx_chip->pinctrl = NULL;
> + }
> +
> + return 0;
> +}
> +
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -216,7 +252,25 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> cr |= MX3_PWMCR_POUTC;
>
> writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> + /*
> + * If we are in charge of pinctrl then switch output to
> + * the PWM signal.
> + */
> + if (imx->pinctrl)
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_pwm);
> } else if (cstate.enabled) {
> + /*
> + * PWM block will be disabled. Normally its output will be set
> + * low no matter what output polarity is configured. Let's use
> + * pinctrl to switch the output pin to GPIO functon and keep
> + * the output at the same level as for duty-cycle = 0.
> + */
> + if (imx->pinctrl)
> + pinctrl_select_state(imx->pinctrl,
> + imx->pinctrl_pins_gpio);
> +
On thing I noticed while looking at the rcar driver is: This doesn't
wait for the current period to end. Is this supposed to happen? Also for
the enable case the hardware is configured for the desired duty cycle
and only then the pinctrl is switched to pwm. Both might result in a
spike that is not desired.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 12.12.2018 09:01, Uwe Kleine-König wrote:
> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>> Normally the PWM output is held LOW when PWM is disabled. This can cause
>> problems when inverted PWM signal polarity is needed. With this behavior
>> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>>
>> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
>> state is selected when PWM is enabled and the gpio pinctrl state is
>> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
>> configured as input and the internal pull-up resistor is used to pull the
>> output level high.
>>
>> If all the pinctrl states and the pwm-gpios GPIO are not correctly
>> specified in DT the PWM work as usual.
>>
>> As an example, with this patch a PWM controlled backlight with inversed
>> signal polarity can be used in full brightness range. Without this patch
>> the backlight can not be turned off as brightness = 0 disables the PWM
>> and that in turn set PWM output LOW, that is full brightness.
>>
>> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>>
>> +--------------+------------+---------------+----------- +-------------+
>> | After reset | Bootloader | PWM probe | PWM | PWM |
>> | 100k pull-up | | | enable 30% | disable |
>> +--------------+------------+---------------+------------+-------------+
>> | pinctrl | none | default | default | default |
>> | out H __________________ __ __ |
>> | out L \_________________/ \_/ \_/\____________ |
>> | ^ ^ ^ |
>> +--------------+------------+---------------+------------+-------------+
>> | pinctrl | none | gpio | pwm | gpio |
>> | out H __________________________________ __ __ _____________ |
>> | out L \_/ \_/ \_/ |
>> | ^ ^ ^ |
>> +----------------------------------------------------------------------+
>>
>> Signed-off-by: Michal Vokáč <[email protected]>
>> ---
>> Changes in v3:
>> - Commit message update.
>> - Minor fix in code comment (Uwe)
>> - Align function arguments to the opening parentheses. (Uwe)
>> - Do not test devm_pinctrl_get for NULL. (Thierry)
>> - Convert all messages to dev_dbg() (Thierry)
>> - Do not actively drive the pin in gpio state. Configure it as input
>> and rely solely on the internal pull-up. (Thierry)
>>
>> Changes in v2:
>> - Utilize the "pwm" and "gpio" pinctrl states.
>> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>> - Select the right pinctrl state in probe.
>>
>> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 77 insertions(+)
>>
[ snip ]
> On thing I noticed while looking at the rcar driver is: This doesn't
> wait for the current period to end. Is this supposed to happen? Also for
> the enable case the hardware is configured for the desired duty cycle
> and only then the pinctrl is switched to pwm. Both might result in a
> spike that is not desired.
The behavior should not change from how imx-pwm was working before.
When PWM is disabled the output is immediately gated off (put into
the idle state) independently on the period. I measured this.
For the enable case you would certainly not get any additional spikes.
The worst thing that may happen is that the first period will be
slightly shorter depending on how long it takes to test the pinctrl
and switch the muxing. And this is unavoidable, it would happen even
if you wait for the start of a period. The test + muxing still takes
some time.
Michal
On Wed, Dec 12, 2018 at 11:42:17AM +0000, Vokáč Michal wrote:
> On 12.12.2018 09:01, Uwe Kleine-König wrote:
> > On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> >> Normally the PWM output is held LOW when PWM is disabled. This can cause
> >> problems when inverted PWM signal polarity is needed. With this behavior
> >> the connected circuit is fed by 100% duty cycle instead of being shut-off.
> >>
> >> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> >> state is selected when PWM is enabled and the gpio pinctrl state is
> >> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> >> configured as input and the internal pull-up resistor is used to pull the
> >> output level high.
> >>
> >> If all the pinctrl states and the pwm-gpios GPIO are not correctly
> >> specified in DT the PWM work as usual.
> >>
> >> As an example, with this patch a PWM controlled backlight with inversed
> >> signal polarity can be used in full brightness range. Without this patch
> >> the backlight can not be turned off as brightness = 0 disables the PWM
> >> and that in turn set PWM output LOW, that is full brightness.
> >>
> >> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
> >>
> >> +--------------+------------+---------------+----------- +-------------+
> >> | After reset | Bootloader | PWM probe | PWM | PWM |
> >> | 100k pull-up | | | enable 30% | disable |
> >> +--------------+------------+---------------+------------+-------------+
> >> | pinctrl | none | default | default | default |
> >> | out H __________________ __ __ |
> >> | out L \_________________/ \_/ \_/\____________ |
> >> | ^ ^ ^ |
> >> +--------------+------------+---------------+------------+-------------+
> >> | pinctrl | none | gpio | pwm | gpio |
> >> | out H __________________________________ __ __ _____________ |
> >> | out L \_/ \_/ \_/ |
> >> | ^ ^ ^ |
> >> +----------------------------------------------------------------------+
> >>
> >> Signed-off-by: Michal Vokáč <[email protected]>
> >> ---
> >> Changes in v3:
> >> - Commit message update.
> >> - Minor fix in code comment (Uwe)
> >> - Align function arguments to the opening parentheses. (Uwe)
> >> - Do not test devm_pinctrl_get for NULL. (Thierry)
> >> - Convert all messages to dev_dbg() (Thierry)
> >> - Do not actively drive the pin in gpio state. Configure it as input
> >> and rely solely on the internal pull-up. (Thierry)
> >>
> >> Changes in v2:
> >> - Utilize the "pwm" and "gpio" pinctrl states.
> >> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
> >> - Select the right pinctrl state in probe.
> >>
> >> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 77 insertions(+)
> >>
>
> [ snip ]
>
> > On thing I noticed while looking at the rcar driver is: This doesn't
> > wait for the current period to end. Is this supposed to happen? Also for
> > the enable case the hardware is configured for the desired duty cycle
> > and only then the pinctrl is switched to pwm. Both might result in a
> > spike that is not desired.
>
> The behavior should not change from how imx-pwm was working before.
> When PWM is disabled the output is immediately gated off (put into
> the idle state) independently on the period. I measured this.
Oh really, I wasn't aware of that. This is another bug in the imx pwm
then (I think).
> For the enable case you would certainly not get any additional spikes.
Yes, there is a possibility for a spike: If you configure for say 40%:
_ _
pwm output : ___/ \__/ \__
muxing : GPIO| PWM_
actual output: ____/\__/ \__
> The worst thing that may happen is that the first period will be
> slightly shorter depending on how long it takes to test the pinctrl
> and switch the muxing. And this is unavoidable, it would happen even
> if you wait for the start of a period. The test + muxing still takes
> some time.
You could first configure for duty_cycle 0 and a short period, then mux
to PWM and then configure the correct duty cycle. Ditto for disable.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Dec 6, 2018 at 2:41 PM Vokáč Michal <[email protected]> wrote:
> Output of the PWM block on i.MX SoCs is always low when the block is
> disabled. This can cause issues when inverted PWM polarity is needed.
> With inverted polarity a duty cycle = 0% corresponds to high level on
> the output. Now, when PWM is disabled its output instantly goes low
> which corresponds to duty cycle = 100%.
>
> To get a truly inverted PWM output two pinctrl states of the PWM pin
> can be used. Configure the pin to GPIO function when PWM is disabled
> and switch back to PWM function whenever non-zero duty cycle is needed.
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> Changes in v3:
> - Slightly different description of the pinctrl and pwm-gpio.
This looks good to me.
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On 12.12.2018 13:12, Uwe Kleine-König wrote:
> On Wed, Dec 12, 2018 at 11:42:17AM +0000, Vokáč Michal wrote:
>> On 12.12.2018 09:01, Uwe Kleine-König wrote:
>>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>>> Normally the PWM output is held LOW when PWM is disabled. This can cause
>>>> problems when inverted PWM signal polarity is needed. With this behavior
>>>> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>>>>
>>>> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
>>>> state is selected when PWM is enabled and the gpio pinctrl state is
>>>> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
>>>> configured as input and the internal pull-up resistor is used to pull the
>>>> output level high.
>>>>
>>>> If all the pinctrl states and the pwm-gpios GPIO are not correctly
>>>> specified in DT the PWM work as usual.
>>>>
>>>> As an example, with this patch a PWM controlled backlight with inversed
>>>> signal polarity can be used in full brightness range. Without this patch
>>>> the backlight can not be turned off as brightness = 0 disables the PWM
>>>> and that in turn set PWM output LOW, that is full brightness.
>>>>
>>>> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>>>>
>>>> +--------------+------------+---------------+----------- +-------------+
>>>> | After reset | Bootloader | PWM probe | PWM | PWM |
>>>> | 100k pull-up | | | enable 30% | disable |
>>>> +--------------+------------+---------------+------------+-------------+
>>>> | pinctrl | none | default | default | default |
>>>> | out H __________________ __ __ |
>>>> | out L \_________________/ \_/ \_/\____________ |
>>>> | ^ ^ ^ |
>>>> +--------------+------------+---------------+------------+-------------+
>>>> | pinctrl | none | gpio | pwm | gpio |
>>>> | out H __________________________________ __ __ _____________ |
>>>> | out L \_/ \_/ \_/ |
>>>> | ^ ^ ^ |
>>>> +----------------------------------------------------------------------+
>>>>
>>>> Signed-off-by: Michal Vokáč <[email protected]>
>>>> ---
>>>> Changes in v3:
>>>> - Commit message update.
>>>> - Minor fix in code comment (Uwe)
>>>> - Align function arguments to the opening parentheses. (Uwe)
>>>> - Do not test devm_pinctrl_get for NULL. (Thierry)
>>>> - Convert all messages to dev_dbg() (Thierry)
>>>> - Do not actively drive the pin in gpio state. Configure it as input
>>>> and rely solely on the internal pull-up. (Thierry)
>>>>
>>>> Changes in v2:
>>>> - Utilize the "pwm" and "gpio" pinctrl states.
>>>> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>>>> - Select the right pinctrl state in probe.
>>>>
>>>> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 77 insertions(+)
>>>>
>>
>> [ snip ]
>>
>>> On thing I noticed while looking at the rcar driver is: This doesn't
>>> wait for the current period to end. Is this supposed to happen? Also for
>>> the enable case the hardware is configured for the desired duty cycle
>>> and only then the pinctrl is switched to pwm. Both might result in a
>>> spike that is not desired.
>>
>> The behavior should not change from how imx-pwm was working before.
>> When PWM is disabled the output is immediately gated off (put into
>> the idle state) independently on the period. I measured this.
>
> Oh really, I wasn't aware of that. This is another bug in the imx pwm
> then (I think).
I kind of expect that when hit a disable button the output is immediately
put into the idle state. To me it does not seem appropriate to wait the
whole period, or even just the active part of the period. If duty=100%
and period=4s (current maximum), in the worst case you would have to wait
4s until you stop the PWM. Quite a long time of driving something you
actually wanted to be shut off.
>> For the enable case you would certainly not get any additional spikes.
>
> Yes, there is a possibility for a spike: If you configure for say 40%:
> _ _
> pwm output : ___/ \__/ \__
> muxing : GPIO| PWM_
> actual output: ____/\__/ \__
OK, you are right.
>> The worst thing that may happen is that the first period will be
>> slightly shorter depending on how long it takes to test the pinctrl
>> and switch the muxing. And this is unavoidable, it would happen even
>> if you wait for the start of a period. The test + muxing still takes
>> some time.
>
> You could first configure for duty_cycle 0 and a short period, then mux
> to PWM and then configure the correct duty cycle. Ditto for disable.
I agree it can be solved for the enable case but I do not see the
point in doing so for the disable case. Can you elaborate on how it
could be useful?
This is what I came up with:
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 55666cc..f76a326 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -242,6 +284,15 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
else
period_cycles = 0;
+ cr = MX3_PWMCR_PRESCALER_SET(prescale) |
+ MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+ FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
+ MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
+
+ if (cstate.polarity == PWM_POLARITY_INVERSED)
+ cr |= FIELD_PREP(MX3_PWMCR_POUTC,
+ MX3_PWMCR_POUTC_INVERTED);
+
/*
* Wait for a free FIFO slot if the PWM is already enabled, and
* flush the FIFO if the PWM was disabled and is about to be
@@ -255,22 +306,36 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return ret;
pwm_imx27_sw_reset(chip);
+ if (imx->pinctrl) {
+ /*
+ * Smooth transition from disabled to enabled
+ * state. Configure duty=0 and requested period
+ * to assure that, later when the requested
+ * duty is configured, the duty cycle of the
+ * first period has correct length.
+ */
+ writel(0, imx->mmio_base + MX3_PWMSAR);
+ writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+ writel(cr, imx->mmio_base + MX3_PWMCR);
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_pwm);
+ }
}
writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
- cr = MX3_PWMCR_PRESCALER_SET(prescale) |
- MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
- FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
- MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
-
- if (state->polarity == PWM_POLARITY_INVERSED)
- cr |= FIELD_PREP(MX3_PWMCR_POUTC,
- MX3_PWMCR_POUTC_INVERTED);
-
writel(cr, imx->mmio_base + MX3_PWMCR);
} else if (cstate.enabled) {
+ /*
+ * PWM block will be disabled. Normally its output will be set
+ * low no matter what output polarity is configured. Let's use
+ * pinctrl to switch the output pin to GPIO functon and keep
+ * the output at the same level as for duty-cycle = 0.
+ */
+ if (imx->pinctrl)
+ pinctrl_select_state(imx->pinctrl,
+ imx->pinctrl_pins_gpio);
+
writel(0, imx->mmio_base + MX3_PWMCR);
pwm_imx27_clk_disable_unprepare(chip);
--
2.1.4
What do you think?
Best regards,
Michal
On Thu, Jan 24, 2019 at 09:59:47AM +0100, Michal Vokáč wrote:
> On 12.12.2018 13:12, Uwe Kleine-König wrote:
> > On Wed, Dec 12, 2018 at 11:42:17AM +0000, Vokáč Michal wrote:
> > > On 12.12.2018 09:01, Uwe Kleine-König wrote:
> > > > On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
> > > > > Normally the PWM output is held LOW when PWM is disabled. This can cause
> > > > > problems when inverted PWM signal polarity is needed. With this behavior
> > > > > the connected circuit is fed by 100% duty cycle instead of being shut-off.
> > > > >
> > > > > Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
> > > > > state is selected when PWM is enabled and the gpio pinctrl state is
> > > > > selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
> > > > > configured as input and the internal pull-up resistor is used to pull the
> > > > > output level high.
> > > > >
> > > > > If all the pinctrl states and the pwm-gpios GPIO are not correctly
> > > > > specified in DT the PWM work as usual.
> > > > >
> > > > > As an example, with this patch a PWM controlled backlight with inversed
> > > > > signal polarity can be used in full brightness range. Without this patch
> > > > > the backlight can not be turned off as brightness = 0 disables the PWM
> > > > > and that in turn set PWM output LOW, that is full brightness.
> > > > >
> > > > > Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
> > > > >
> > > > > +--------------+------------+---------------+----------- +-------------+
> > > > > | After reset | Bootloader | PWM probe | PWM | PWM |
> > > > > | 100k pull-up | | | enable 30% | disable |
> > > > > +--------------+------------+---------------+------------+-------------+
> > > > > | pinctrl | none | default | default | default |
> > > > > | out H __________________ __ __ |
> > > > > | out L \_________________/ \_/ \_/\____________ |
> > > > > | ^ ^ ^ |
> > > > > +--------------+------------+---------------+------------+-------------+
> > > > > | pinctrl | none | gpio | pwm | gpio |
> > > > > | out H __________________________________ __ __ _____________ |
> > > > > | out L \_/ \_/ \_/ |
> > > > > | ^ ^ ^ |
> > > > > +----------------------------------------------------------------------+
> > > > >
> > > > > Signed-off-by: Michal Vokáč <[email protected]>
> > > > > ---
> > > > > Changes in v3:
> > > > > - Commit message update.
> > > > > - Minor fix in code comment (Uwe)
> > > > > - Align function arguments to the opening parentheses. (Uwe)
> > > > > - Do not test devm_pinctrl_get for NULL. (Thierry)
> > > > > - Convert all messages to dev_dbg() (Thierry)
> > > > > - Do not actively drive the pin in gpio state. Configure it as input
> > > > > and rely solely on the internal pull-up. (Thierry)
> > > > >
> > > > > Changes in v2:
> > > > > - Utilize the "pwm" and "gpio" pinctrl states.
> > > > > - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
> > > > > - Select the right pinctrl state in probe.
> > > > >
> > > > > drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 77 insertions(+)
> > > > >
> > >
> > > [ snip ]
> > >
> > > > On thing I noticed while looking at the rcar driver is: This doesn't
> > > > wait for the current period to end. Is this supposed to happen? Also for
> > > > the enable case the hardware is configured for the desired duty cycle
> > > > and only then the pinctrl is switched to pwm. Both might result in a
> > > > spike that is not desired.
> > >
> > > The behavior should not change from how imx-pwm was working before.
> > > When PWM is disabled the output is immediately gated off (put into
> > > the idle state) independently on the period. I measured this.
> >
> > Oh really, I wasn't aware of that. This is another bug in the imx pwm
> > then (I think).
>
> I kind of expect that when hit a disable button the output is immediately
> put into the idle state. To me it does not seem appropriate to wait the
> whole period, or even just the active part of the period. If duty=100%
> and period=4s (current maximum), in the worst case you would have to wait
> 4s until you stop the PWM. Quite a long time of driving something you
> actually wanted to be shut off.
I think it might be beneficial to allow (or require) that disable acts
immediately. But this is not how it used to be and in my discussion with
Thierry (IIRC) he required to complete the currently running period.
Then if a user requires a "smooth disable" (i.e. with completing the
currently running period) they can do:
pwm_apply_state(pwm, { .duty_cycle = 0, ... });
pwm_apply_state(pwm, { .enabled = 0 });
to get the delayed disable.
But having said this, this should be a concious decision with the
requirements properly documented and the drivers users reviewed and adapted.
> > > For the enable case you would certainly not get any additional spikes.
> >
> > Yes, there is a possibility for a spike: If you configure for say 40%:
> > _ _
> > pwm output : ___/ \__/ \__
> > muxing : GPIO| PWM_
> > actual output: ____/\__/ \__
>
> OK, you are right.
>
> > > The worst thing that may happen is that the first period will be
> > > slightly shorter depending on how long it takes to test the pinctrl
> > > and switch the muxing. And this is unavoidable, it would happen even
> > > if you wait for the start of a period. The test + muxing still takes
> > > some time.
> >
> > You could first configure for duty_cycle 0 and a short period, then mux
> > to PWM and then configure the correct duty cycle. Ditto for disable.
>
> I agree it can be solved for the enable case but I do not see the
> point in doing so for the disable case. Can you elaborate on how it
> could be useful?
Assuming we stick to "disable should complete the currently running
period" it is all about preventing spikes. So if you switch the mux from
PWM to GPIO when the output just raised, that might (depending on the
use case) be bad and so it should be possible for the user to prevent
this.
> This is what I came up with:
>
> [...]
>
> What do you think?
I'm missing context. Your patch is on top of your last patch I assume.
Would you mind to send the squashed result?
(I'm unsure when I find time (and nerve) for a proper review, but I will
try to at least look over your suggestions.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 24.1.2019 10:22, Uwe Kleine-König wrote:
> On Thu, Jan 24, 2019 at 09:59:47AM +0100, Michal Vokáč wrote:
>> On 12.12.2018 13:12, Uwe Kleine-König wrote:
>>> On Wed, Dec 12, 2018 at 11:42:17AM +0000, Vokáč Michal wrote:
>>>> On 12.12.2018 09:01, Uwe Kleine-König wrote:
>>>>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>>>>> Normally the PWM output is held LOW when PWM is disabled. This can cause
>>>>>> problems when inverted PWM signal polarity is needed. With this behavior
>>>>>> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>>>>>>
>>>>>> Allow users to define a "pwm" and a "gpio" pinctrl states. The pwm pinctrl
>>>>>> state is selected when PWM is enabled and the gpio pinctrl state is
>>>>>> selected when PWM is disabled. In the gpio state the new pwm-gpios GPIO is
>>>>>> configured as input and the internal pull-up resistor is used to pull the
>>>>>> output level high.
>>>>>>
>>>>>> If all the pinctrl states and the pwm-gpios GPIO are not correctly
>>>>>> specified in DT the PWM work as usual.
>>>>>>
>>>>>> As an example, with this patch a PWM controlled backlight with inversed
>>>>>> signal polarity can be used in full brightness range. Without this patch
>>>>>> the backlight can not be turned off as brightness = 0 disables the PWM
>>>>>> and that in turn set PWM output LOW, that is full brightness.
>>>>>>
>>>>>> Inverted output of the PWM with "default" and with "pwm"+"gpio" pinctrl:
>>>>>>
>>>>>> +--------------+------------+---------------+----------- +-------------+
>>>>>> | After reset | Bootloader | PWM probe | PWM | PWM |
>>>>>> | 100k pull-up | | | enable 30% | disable |
>>>>>> +--------------+------------+---------------+------------+-------------+
>>>>>> | pinctrl | none | default | default | default |
>>>>>> | out H __________________ __ __ |
>>>>>> | out L \_________________/ \_/ \_/\____________ |
>>>>>> | ^ ^ ^ |
>>>>>> +--------------+------------+---------------+------------+-------------+
>>>>>> | pinctrl | none | gpio | pwm | gpio |
>>>>>> | out H __________________________________ __ __ _____________ |
>>>>>> | out L \_/ \_/ \_/ |
>>>>>> | ^ ^ ^ |
>>>>>> +----------------------------------------------------------------------+
>>>>>>
>>>>>> Signed-off-by: Michal Vokáč <[email protected]>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>> - Commit message update.
>>>>>> - Minor fix in code comment (Uwe)
>>>>>> - Align function arguments to the opening parentheses. (Uwe)
>>>>>> - Do not test devm_pinctrl_get for NULL. (Thierry)
>>>>>> - Convert all messages to dev_dbg() (Thierry)
>>>>>> - Do not actively drive the pin in gpio state. Configure it as input
>>>>>> and rely solely on the internal pull-up. (Thierry)
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Utilize the "pwm" and "gpio" pinctrl states.
>>>>>> - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>>>>>> - Select the right pinctrl state in probe.
>>>>>>
>>>>>> drivers/pwm/pwm-imx.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 77 insertions(+)
>>>>>>
>>>>
>>>> [ snip ]
>>>>
>>>>> On thing I noticed while looking at the rcar driver is: This doesn't
>>>>> wait for the current period to end. Is this supposed to happen? Also for
>>>>> the enable case the hardware is configured for the desired duty cycle
>>>>> and only then the pinctrl is switched to pwm. Both might result in a
>>>>> spike that is not desired.
>>>>
>>>> The behavior should not change from how imx-pwm was working before.
>>>> When PWM is disabled the output is immediately gated off (put into
>>>> the idle state) independently on the period. I measured this.
>>>
>>> Oh really, I wasn't aware of that. This is another bug in the imx pwm
>>> then (I think).
>>
>> I kind of expect that when hit a disable button the output is immediately
>> put into the idle state. To me it does not seem appropriate to wait the
>> whole period, or even just the active part of the period. If duty=100%
>> and period=4s (current maximum), in the worst case you would have to wait
>> 4s until you stop the PWM. Quite a long time of driving something you
>> actually wanted to be shut off.
>
> I think it might be beneficial to allow (or require) that disable acts
> immediately. But this is not how it used to be and in my discussion with
> Thierry (IIRC) he required to complete the currently running period.
I am confused here. IFAIK it always used to be that:
pwm_apply_state(pwm, { .enabled = 0 });
immediately stops the PWM with:
writel(0, imx->mmio_base + MX3_PWMCR);
regardless of the period (for pwm-imx).
> Then if a user requires a "smooth disable" (i.e. with completing the
> currently running period) they can do:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, ... });
> pwm_apply_state(pwm, { .enabled = 0 });
>
> to get the delayed disable.
>
> But having said this, this should be a concious decision with the
> requirements properly documented and the drivers users reviewed and adapted.
>
>>>> For the enable case you would certainly not get any additional spikes.
>>>
>>> Yes, there is a possibility for a spike: If you configure for say 40%:
>>> _ _
>>> pwm output : ___/ \__/ \__
>>> muxing : GPIO| PWM_
>>> actual output: ____/\__/ \__
>>
>> OK, you are right.
>>
>>>> The worst thing that may happen is that the first period will be
>>>> slightly shorter depending on how long it takes to test the pinctrl
>>>> and switch the muxing. And this is unavoidable, it would happen even
>>>> if you wait for the start of a period. The test + muxing still takes
>>>> some time.
>>>
>>> You could first configure for duty_cycle 0 and a short period, then mux
>>> to PWM and then configure the correct duty cycle. Ditto for disable.
>>
>> I agree it can be solved for the enable case but I do not see the
>> point in doing so for the disable case. Can you elaborate on how it
>> could be useful?
>
> Assuming we stick to "disable should complete the currently running
> period" it is all about preventing spikes. So if you switch the mux from
> PWM to GPIO when the output just raised, that might (depending on the
> use case) be bad and so it should be possible for the user to prevent
> this.
I understand. The issue is I do not tend to call this case "a spike".
To me it means that the first and last period may have a shorter duty
cycle than requested. I tend to think that shorter duty cycle is not
harmful/bad. It actually means that you enable/disable the device
more smoothly.
Off course everything depends on the device that is fed with the signal.
So I totaly agree that being able to produce the signal with all the
periods of the same length and with the same duty is a good thing.
>> This is what I came up with:
>>
>> [...]
>>
>> What do you think?
>
> I'm missing context. Your patch is on top of your last patch I assume.
Sorry, I had to add some context. It is against current linux-next and
it is just the relevant hunk for the pwm_imx27_apply() function.
> Would you mind to send the squashed result?
Of course I do not mind. I can respin and send what I have as v5
against linux-next as I already rebased on top of your patches with
the split driver, if you prefer that?
> (I'm unsure when I find time (and nerve) for a proper review, but I will
> try to at least look over your suggestions.)
Thank you very much Uwe!
Michal
On Thu, Jan 24, 2019 at 11:12:12AM +0100, Michal Vokáč wrote:
> On 24.1.2019 10:22, Uwe Kleine-König wrote:
> > I think it might be beneficial to allow (or require) that disable acts
> > immediately. But this is not how it used to be and in my discussion with
> > Thierry (IIRC) he required to complete the currently running period.
>
> I am confused here. IFAIK it always used to be that:
>
> pwm_apply_state(pwm, { .enabled = 0 });
>
> immediately stops the PWM with:
>
> writel(0, imx->mmio_base + MX3_PWMCR);
>
> regardless of the period (for pwm-imx).
Then is is a bug since forever (well, or a fact that resulted from the
intended semantic not being documented and the driver author not caring
or knowing better).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On 24.1.2019 11:44, Uwe Kleine-König wrote:
> On Thu, Jan 24, 2019 at 11:12:12AM +0100, Michal Vokáč wrote:
>> On 24.1.2019 10:22, Uwe Kleine-König wrote:
>>> I think it might be beneficial to allow (or require) that disable acts
>>> immediately. But this is not how it used to be and in my discussion with
>>> Thierry (IIRC) he required to complete the currently running period.
>>
>> I am confused here. IFAIK it always used to be that:
>>
>> pwm_apply_state(pwm, { .enabled = 0 });
>>
>> immediately stops the PWM with:
>>
>> writel(0, imx->mmio_base + MX3_PWMCR);
>>
>> regardless of the period (for pwm-imx).
>
> Then is is a bug since forever (well, or a fact that resulted from the
> intended semantic not being documented and the driver author not caring
> or knowing better).
Hi Uwe, I skimmed all the PWM drivers trying to find out how others are
waiting for the end of a period before disabling PWM.
There is 50 PWM drivers in total and I could find only two drivers that
do something that resembles waiting for an end of a period:
- pwm-atmel.c
https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-atmel.c#L176
- pwm-sun4i.c
https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-sun4i.c#L284
I did not delve into that too much but I believe there are some HW
requirements on those platforms to stop the PWM that way. Others
simply stop the PWM straight away. So I am confused even more
and wonder where your requirements came from?
Anyway, as I could not find any real example I tried to solve it
according to your earlier suggestion. That is configure duty_cycle=0
and some time later disable PWM.
It generates additional problems. The PWM block has a FIFO with four
slots. Before adding the sample with duty cycle=0 into the FIFO it
is wise to wait for a free slot in the FIFO. Then when the sample is
added it may actually happen that the sample is the fourth in the
FIFO. So it may take up to four periods until we can disable the PWM.
These four periods can take up to 16s. Hmmm :(
Reconfigure the period is not an option. According to the TRM, once you
write new value into the period register, the counter is cleared and
new period is started. That means you can produce the spikes this way
as well..
I agree there are bugs in the driver and it is far from providing
complete support of the i.MX PWM HW. Still, I believe those are totally
independent problems from the pinctrl stuff and so should not block
the discussion/inclusion of this series.
I am sorry if this is getting on your nerves Uwe. I am doing my
best to test all your suggestions and possible solutions to find
a good compromise. At least I learned a lot from doing all that stuff.
Thank you for your time,
Michal
Hello Michal,
On Wed, Jan 30, 2019 at 03:42:29PM +0100, Michal Vokáč wrote:
> On 24.1.2019 11:44, Uwe Kleine-König wrote:
> > On Thu, Jan 24, 2019 at 11:12:12AM +0100, Michal Vokáč wrote:
> > > On 24.1.2019 10:22, Uwe Kleine-König wrote:
> > > > I think it might be beneficial to allow (or require) that disable acts
> > > > immediately. But this is not how it used to be and in my discussion with
> > > > Thierry (IIRC) he required to complete the currently running period.
> > >
> > > I am confused here. IFAIK it always used to be that:
> > >
> > > pwm_apply_state(pwm, { .enabled = 0 });
> > >
> > > immediately stops the PWM with:
> > >
> > > writel(0, imx->mmio_base + MX3_PWMCR);
> > >
> > > regardless of the period (for pwm-imx).
> >
> > Then is is a bug since forever (well, or a fact that resulted from the
> > intended semantic not being documented and the driver author not caring
> > or knowing better).
>
> Hi Uwe, I skimmed all the PWM drivers trying to find out how others are
> waiting for the end of a period before disabling PWM.
>
> There is 50 PWM drivers in total and I could find only two drivers that
> do something that resembles waiting for an end of a period:
>
> - pwm-atmel.c
> https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-atmel.c#L176
>
> - pwm-sun4i.c
> https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/pwm/pwm-sun4i.c#L284
There are quite some drivers known (to me) being buggy here. My feeling
is that Thierry doesn't share that impression and I think the only
reasonable path forward is to first fix the requirements for drivers and
when this is done, look at the implementations and fix them or conclude
that the requirements are not practical and shift them accordingly.
From my POV the situation is as follows: I suggested a few changes that
seemed reasonable to me (e.g.: don't rely on the pin state to be
inactive after disable; don't rely on a driver to block in .apply until
the new configuration is active; thought about dropping .disable
altogether) and Thierry shot them down indicating that he is not willing
to change the API and all affected users for only one or a few
implementations. My consequence is to be strict about the requirements
because that's the only way to show the resulting pain (or see there
isn't so much pain).
> I did not delve into that too much but I believe there are some HW
> requirements on those platforms to stop the PWM that way. Others
> simply stop the PWM straight away. So I am confused even more
> and wonder where your requirements came from?
I'm aware that many drivers don't adhere to these requirements. IMHO
this is related to the poor situation regarding documentation for
pwm-driver authors and lack of time for in-deep reviews by Thierry.
Never the less: The requirement to complete the currently running period
comes from Thierry. There is a patch by me waiting for review that
targets improving the documentation and you already have to suffer from
my plan to spend some time on pwm-reviews :-)
> Anyway, as I could not find any real example I tried to solve it
> according to your earlier suggestion. That is configure duty_cycle=0
> and some time later disable PWM.
>
> It generates additional problems. The PWM block has a FIFO with four
> slots. Before adding the sample with duty cycle=0 into the FIFO it
> is wise to wait for a free slot in the FIFO. Then when the sample is
> added it may actually happen that the sample is the fourth in the
> FIFO. So it may take up to four periods until we can disable the PWM.
> These four periods can take up to 16s. Hmmm :(
No it cannot. Because if you put a new configuration into the FIFO you
have to block until the requested configuration is active, so it must
not happen that you hit the FIFO with 4 busy entries. (Unless a user
calls pwm_apply_state() in four contexts in parallel, which should not
happen. And if it does, we should implement serialization in the
pwm-framework such that pwm-drivers doesn't need to care.)
> I agree there are bugs in the driver and it is far from providing
> complete support of the i.MX PWM HW. Still, I believe those are totally
> independent problems from the pinctrl stuff and so should not block
> the discussion/inclusion of this series.
I think while there are people who care about a driver, the known
problems should be addressed before a change is "sneaked" in that makes
the contributor happy and care about other stuff.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi Uwe,
On 30. 01. 19 16:39, Uwe Kleine-König wrote:
> On Wed, Jan 30, 2019 at 03:42:29PM +0100, Michal Vokáč wrote:
[...]> There are quite some drivers known (to me) being buggy here. My feeling
> is that Thierry doesn't share that impression and I think the only
> reasonable path forward is to first fix the requirements for drivers and
> when this is done, look at the implementations and fix them or conclude
> that the requirements are not practical and shift them accordingly.
I can't really comment on this as my experience is limited to the
i.MX only, but what you say sounds quite reasonable. What I think might
be a problem is it seems there is not many active members in the "PWM
community". Namely I think it is you and Thierry. Others are just
occasional contributors to PWM hw drivers. So it is not going to be an
easy task to conclude on a new/better/fixed set of requirements.
> From my POV the situation is as follows: I suggested a few changes that
> seemed reasonable to me (e.g.: don't rely on the pin state to be
> inactive after disable; don't rely on a driver to block in .apply until
> the new configuration is active; thought about dropping .disable
> altogether) and Thierry shot them down indicating that he is not willing
> to change the API and all affected users for only one or a few
> implementations. My consequence is to be strict about the requirements
> because that's the only way to show the resulting pain (or see there
> isn't so much pain).
I remember those discussions. That is why I am worried if this is ever
going to move forward as the whole issue dates at least back to 2015.
Fingers crossed!
>> I did not delve into that too much but I believe there are some HW
>> requirements on those platforms to stop the PWM that way. Others
>> simply stop the PWM straight away. So I am confused even more
>> and wonder where your requirements came from?
>
> I'm aware that many drivers don't adhere to these requirements. IMHO
> this is related to the poor situation regarding documentation for
> pwm-driver authors and lack of time for in-deep reviews by Thierry.
> Never the less: The requirement to complete the currently running period
> comes from Thierry. There is a patch by me waiting for review that
> targets improving the documentation and you already have to suffer from
> my plan to spend some time on pwm-reviews :-)
Yeah, that is unpleasant if there are some requirements coming from
the maintainer and are not documented. The response times are not
really good as well. Except from you. That is why I actually welcome
the fact that you opted to spend your time on pwm reviews!
I looked at the documentation patch you submitted and at least it
clearly describes some of the questionable situations. Whether
I like them or not is irrelevant.
>> Anyway, as I could not find any real example I tried to solve it
>> according to your earlier suggestion. That is configure duty_cycle=0
>> and some time later disable PWM.
>>
>> It generates additional problems. The PWM block has a FIFO with four
>> slots. Before adding the sample with duty cycle=0 into the FIFO it
>> is wise to wait for a free slot in the FIFO. Then when the sample is
>> added it may actually happen that the sample is the fourth in the
>> FIFO. So it may take up to four periods until we can disable the PWM.
>> These four periods can take up to 16s. Hmmm :(
>
> No it cannot. Because if you put a new configuration into the FIFO you
> have to block until the requested configuration is active, so it must
> not happen that you hit the FIFO with 4 busy entries. (Unless a user
> calls pwm_apply_state() in four contexts in parallel, which should not
> happen. And if it does, we should implement serialization in the
> pwm-framework such that pwm-drivers doesn't need to care.)
I was describing what the current code does/allows. There is a function
to check there is a free slot in the FIFO. It returns without blocking
if there is at least one free slot out of the four. So currently it
is possible to run this script:
#!/bin/sh
echo 0 > enable
echo 1000000000 > period # 1 second
echo 100000000 > duty_cycle # 1. sample, 100ms
echo 1 > enable # clears FIFO and writes 1. sample
echo 200000000 > duty_cycle # 2. sample, 200ms
echo 400000000 > duty_cycle # 3. sample, 400ms
echo 800000000 > duty_cycle # 4. sample, 800ms
and the driver does not block on writing the samples 2-4 and I can see
a pulse train of all the configured samples on a scope. This definitely
does not call pwm_apply_state() in parallel contexts.
If I attempt to write another sample unless the first period passes
(when the FIFO is still full) the driver does block for msleep(period_ms)
and then the new sample is written and I see all 5 samples on the scope.
The last (fith one) is repeated as expected.
From what you say here and what I read in your proposed change to the
documentation you require the driver to always block until the first
sample is utilized. So effectively shrinking the FIFO from 4 to 1 sample,
right?
This makes me to think: How we can define a fixed set of requirements
for (not just) PWM HW drivers and at the same time allow to utilize
the maximum capabilities of each HW?
>> I agree there are bugs in the driver and it is far from providing
>> complete support of the i.MX PWM HW. Still, I believe those are totally
>> independent problems from the pinctrl stuff and so should not block
>> the discussion/inclusion of this series.
>
> I think while there are people who care about a driver, the known
> problems should be addressed before a change is "sneaked" in that makes
> the contributor happy and care about other stuff.
You can argue it is subjective but isn't the fact that the i.MX PWM
HW has some issues (and people are trying to solve them), a known
problem as well? I agree that there may be reasons to change priority
and first change/fix the requirements and then fix the drivers
accordingly. Unfortunately, even though I would love, I can not really
help much with that.
Best regards,
Michal