Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1622189 > unrolled thread

Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-04-12 16:40 +0200
    Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support Andrew Lunn <andrew@lunn.ch> - 2017-04-12 17:20 +0200
      Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-04-21 11:30 +0200
        Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support Linus Walleij <linus.walleij@linaro.org> - 2017-04-24 11:20 +0200

#1622189 — Re: [PATCH v5 1/4] gpio: mvebu: Add limited PWM support

Hello,

Sorry for the late feedback about this.

On Sun,  9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:

> +		gpio1: gpio@18140 {
> +			compatible = "marvell,armada-370-xp-gpio";
> +			reg = <0x18140 0x40>, <0x181c8 0x08>;

One issue I see is that you have only two counters A and B. You
associate counter A with the first bank of GPIOs, and counter B with
the second bank of GPIOs.

Which means that if you need to PWM a GPIO from the third bank of
GPIOs, you can't, even if the HW allows it.

While I'm fine with not supporting all the HW features, but it's a bit
sad that this gets encoded into the DT.

But I guess the only way to make this possible would be to have a
single node for all GPIOs rather than one per bank? Or do we have a way
to have those counter A/B registers bound to a separate PWM driver, and
then the GPIO driver being smart enough to select the counter to be
used? Seems not easy to do though :-/


> +struct mvebu_pwm {
> +	void __iomem		*membase;
> +	unsigned long		 clk_rate;
> +	bool			 used;
> +	struct pwm_chip		 chip;
> +	spinlock_t		 lock;
> +	struct mvebu_gpio_chip	*mvchip;
> +
> +	/* Used to preserve GPIO/PWM registers across suspend/resume */
> +	u32			 blink_select;
> +	u32			 blink_on_duration;
> +	u32			 blink_off_duration;
> +};
> +
>  struct mvebu_gpio_chip {
>  	struct gpio_chip   chip;
>  	spinlock_t	   lock;
> @@ -87,6 +113,10 @@ struct mvebu_gpio_chip {
>  	struct irq_domain *domain;
>  	int		   soc_variant;
>  
> +	/* Used for PWM support */
> +	struct clk	  *clk;
> +	struct mvebu_pwm  *mvpwm;

Why does mvpwm needs to be allocated separately? Why not directly embed
it inside the mvebu_gpio_chip structure?

Do we need a separate spinlock?


> @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
>  		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
>  	},
>  	{
> +		.compatible = "marvell,armada-370-xp-gpio",
> +		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,

Whum, what are you doing here?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1622234

On Wed, Apr 12, 2017 at 04:31:28PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> Sorry for the late feedback about this.
> 
> On Sun,  9 Apr 2017 20:09:27 +0200, Ralph Sennhauser wrote:
> 
> > +		gpio1: gpio@18140 {
> > +			compatible = "marvell,armada-370-xp-gpio";
> > +			reg = <0x18140 0x40>, <0x181c8 0x08>;
> 
> One issue I see is that you have only two counters A and B. You
> associate counter A with the first bank of GPIOs, and counter B with
> the second bank of GPIOs.
> 
> Which means that if you need to PWM a GPIO from the third bank of
> GPIOs, you can't, even if the HW allows it.
>
> 
> While I'm fine with not supporting all the HW features, but it's a bit
> sad that this gets encoded into the DT.
> 
> But I guess the only way to make this possible would be to have a
> single node for all GPIOs rather than one per bank? Or do we have a way
> to have those counter A/B registers bound to a separate PWM driver, and
> then the GPIO driver being smart enough to select the counter to be
> used? Seems not easy to do though :-/

Hi Thomas

Yep. It was a compromise. By adding a new binding for the GPIO driver,
this might be possible. But it did not seem worth such a major change.

The prime use of this feature is for controlling a fan. So far, i've
not seen any hardware with more than one fan, i.e. needs more than one
PWM. Nor have i seen any hardware with the GPIO for the fan being on
the third bank. A hardware manufacture could add multiple fans, but i
doubt it, they make noise and fail. And if a manufacture does place a
fan on the third bank, it can still be controlled as a plain GPIO fan,
as we have been doing for the last few years.

So i personally think it is an O.K. compromise.

> > @@ -555,6 +842,10 @@ static const struct of_device_id mvebu_gpio_of_match[] = {
> >  		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> >  	},
> >  	{
> > +		.compatible = "marvell,armada-370-xp-gpio",
> > +		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
> 
> Whum, what are you doing here?

Since you are late to the discussion, you probably missed this part.

Only 370 and XP have the hardware needed to do this. Kirkwood and
Orion5x does not. It was requested a compatible string was added to
indicate SoC has the needed hardware.

The driver was extended when XP was added, due it is per CPU
interrupts. However, that turned out to be broken. One CPU would
enable the interrupt, and it was delivered to another, causing it to
be missed. So XP was reverted to use the plan old ORION way of doing
interrupts.

So this patch adds in a new compatible string for 370 and XP, to
indicate the PWM hardware is available, and keeps using the ORION way
of handing interrupts.

   Andrew

[toc] | [prev] | [next] | [standalone]

#1628079

Hello,

On Wed, 12 Apr 2017 17:19:32 +0200, Andrew Lunn wrote:

> Yep. It was a compromise. By adding a new binding for the GPIO driver,
> this might be possible. But it did not seem worth such a major change.
> 
> The prime use of this feature is for controlling a fan. So far, i've
> not seen any hardware with more than one fan, i.e. needs more than one
> PWM. Nor have i seen any hardware with the GPIO for the fan being on
> the third bank. A hardware manufacture could add multiple fans, but i
> doubt it, they make noise and fail. And if a manufacture does place a
> fan on the third bank, it can still be controlled as a plain GPIO fan,
> as we have been doing for the last few years.

Right.

> So i personally think it is an O.K. compromise.

I clearly don't want to block this, but I believe this is a very good
illustration of why stable DT bindings simply don't work. We are
realizing here that having each GPIO bank represented as a separate DT
node doesn't work, because this blinking functionality is not per GPIO
bank, but global to all GPIO banks.

I am totally fine with compromise, and having things simple first, and
extend them later if needed. But this stable DT binding rule makes this
quite impossible: what is a compromise today might put you in big
troubles tomorrow.

Anyway, it's fine for me, I don't think it's worth the effort making a
much more complicated solution/change.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1629344

On Fri, Apr 21, 2017 at 11:19 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> I clearly don't want to block this, but I believe this is a very good
> illustration of why stable DT bindings simply don't work. We are
> realizing here that having each GPIO bank represented as a separate DT
> node doesn't work, because this blinking functionality is not per GPIO
> bank, but global to all GPIO banks.
>
> I am totally fine with compromise, and having things simple first, and
> extend them later if needed. But this stable DT binding rule makes this
> quite impossible: what is a compromise today might put you in big
> troubles tomorrow.

Really "stable bindings" I never believed in. It's just a pipe dream.

Well they might become stable when the system is "finished"
whenever that happens.

I think a better rationale is that of the IETF:
"rough consensus and running code", make deployed DTs work,
if they are not deployed, or only getting deployed together with the
kernel, changing the bindings are not a problem.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web