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

Groups > linux.kernel > #1476213 > unrolled thread

Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

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 v3 1/4] pwm: Add support for Meson PWM Controller Thierry Reding <thierry.reding@gmail.com> - 2016-09-05 11:10 +0200
    Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller Neil Armstrong <narmstrong@baylibre.com> - 2016-09-05 11:30 +0200
    Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller Neil Armstrong <narmstrong@baylibre.com> - 2016-09-06 10:40 +0200
      Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller Thierry Reding <thierry.reding@gmail.com> - 2016-09-06 11:10 +0200
        Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller Neil Armstrong <narmstrong@baylibre.com> - 2016-09-06 11:20 +0200
          Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller Thierry Reding <thierry.reding@gmail.com> - 2016-09-06 12:10 +0200
            Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller jbrunet <jbrunet@baylibre.com> - 2016-09-06 14:20 +0200

#1476213 — Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
> Add support for the PWM controller found in the Amlogic SoCs.
> This driver supports the Meson8b and GXBB SoCs.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/pwm/Kconfig     |   9 +
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 538 insertions(+)
>  create mode 100644 drivers/pwm/pwm-meson.c

Hi Neil,

sorry for taking so long to review this. I had actually started to write
a review email since I had noticed a couple of slight oddities about the
driver structure (primarily this was about how channel-specific data was
split between struct meson_pwm_channel and struct meson_pwm_chip), but I
ended up making some changes to the driver in order to see what my
suggestions would look like, and if they would indeed improve things.
But once I had done that, I thought it a bit pointless to make that into
review comments and decided to just push what I had done and ask you to
take a look, and if you had no objections to the changes take the driver
for a spin to see if it still worked as expected.

One other thing I noticed is that your ->get_state() implementation only
reads the enable state, but from the looks of it it should be possible
to read period and duty cycle information from hardware as well. I'm not
going to reject the driver for that reason, just saying that it'd be
good to have that implemented sometime in the future.

I've pushed my modifications to the driver to the linux-pwm repository:

	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next

Alternatively you can also take a look at the for-4.9/drivers branch,
but they're currently the same thing.

Thierry

[toc] | [next] | [standalone]

#1476229

On 09/05/2016 11:00 AM, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
>> Add support for the PWM controller found in the Amlogic SoCs.
>> This driver supports the Meson8b and GXBB SoCs.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/pwm/Kconfig     |   9 +
>>  drivers/pwm/Makefile    |   1 +
>>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 538 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-meson.c

Hi Thierry,
> Hi Neil,
> 
> sorry for taking so long to review this. I had actually started to write
> a review email since I had noticed a couple of slight oddities about the
> driver structure (primarily this was about how channel-specific data was
> split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> ended up making some changes to the driver in order to see what my
> suggestions would look like, and if they would indeed improve things.
> But once I had done that, I thought it a bit pointless to make that into
> review comments and decided to just push what I had done and ask you to
> take a look, and if you had no objections to the changes take the driver
> for a spin to see if it still worked as expected.

Well, thanks ! I was wondering why it took so long, but the result look far best than what I achieved.

The road was very long since the original Amlogic driver...

I will try it out ASAP, but it looks very good for me.
Your changes seems quite obvious, and such rework was necessary.

> 
> One other thing I noticed is that your ->get_state() implementation only
> reads the enable state, but from the looks of it it should be possible
> to read period and duty cycle information from hardware as well. I'm not
> going to reject the driver for that reason, just saying that it'd be
> good to have that implemented sometime in the future.

Yes, it was delayed for later since it's not a functional feature, I will certainly push
an update with this later on.

> 
> I've pushed my modifications to the driver to the linux-pwm repository:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next
> 
> Alternatively you can also take a look at the for-4.9/drivers branch,
> but they're currently the same thing.

Great, I will give you a functional update ASAP.

> Thierry

Thanks for the review, aww, s/review/rework/ !
Neil
> 

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

#1477159

Hi Thierry,

On 09/05/2016 11:00 AM, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
>> Add support for the PWM controller found in the Amlogic SoCs.
>> This driver supports the Meson8b and GXBB SoCs.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/pwm/Kconfig     |   9 +
>>  drivers/pwm/Makefile    |   1 +
>>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 538 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-meson.c
> 
> Hi Neil,
> 
> sorry for taking so long to review this. I had actually started to write
> a review email since I had noticed a couple of slight oddities about the
> driver structure (primarily this was about how channel-specific data was
> split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> ended up making some changes to the driver in order to see what my
> suggestions would look like, and if they would indeed improve things.
> But once I had done that, I thought it a bit pointless to make that into
> review comments and decided to just push what I had done and ask you to
> take a look, and if you had no objections to the changes take the driver
> for a spin to see if it still worked as expected.

We re-run our tests and I found 2 bugs, the first one is in meson_pwm_enable(),
only the channel A was setup, the fix is :

static void meson_pwm_enable(...)
-	u32 value, clk_shift, clk_enable, enable;
+	u32 reg, value, clk_shift, clk_enable, enable;

 	switch (id) {
 	case 0:
[...]
+		reg = REG_PWM_A;
 		break;
 	case 1:
[...]
+		reg = REG_PWM_B;
 		break;
 	}
[...]
-	writel(value, meson->base + REG_PWM_A);
+	writel(value, meson->base + reg);

The second bug is in probe(), I understand the point to allocate dynamically the channels
and attach them to each pwm chip, but when calling meson_pwm_init_channels() we get an OOPS
because meson->chip.pwms[i] are allocated in pwmchip_add().
Moving meson_pwm_init_channels() would fix this, but in case of a clk PROBE_DEFER, we would need
to remove back the pwmchip, which is a quite a bad design decision....

The smartest fix I found was to allocate channels in probe, init them them attach them after pwmchip_add():

static int meson_pwm_init_channels(..., struct meson_pwm_channel *channels)
{
+	struct meson_pwm_channel *channels;
[...]
-	for (i = 0; i < meson->chip.npwm; i++) {
-		struct pwm_device *pwm = &meson->chip.pwms[i];
-		struct meson_pwm_channel *channel;
-
-		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
-		if (!channel)
-			return -ENOMEM;
+	if (!channels)
+		return -EINVAL;

+	for (i = 0; i < meson->chip.npwm; i++) {
[...]
+		memset(&channels[i], 0, sizeof(struct meson_pwm_channel));
[...]
//Rename "channel->" into "channels[i]."//
[...]
-		pwm_set_chip_data(pwm, channel);
 	}

 	return 0;
}

+static void meson_pwm_add_channels_data(struct meson_pwm *meson,
+					struct meson_pwm_channel *channels)
+{
+	unsigned int i;
+
+	for (i = 0; i < meson->chip.npwm; i++)
+		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
+}

static int meson_pwm_probe(struct platform_device *pdev)
{
+	struct meson_pwm_channel *channels;
[...]
-	err = meson_pwm_init_channels(meson);
-	if (err < 0)
-		return err;
-
 	meson->chip.dev = &pdev->dev;
[...]
 	meson->chip.of_pwm_n_cells = 3;

+	channels = devm_kmalloc_array(&pdev->dev, 2, sizeof(*meson),
+				      GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	err = meson_pwm_init_channels(meson, channels);
+	if (err < 0)
+		return err;
+
 	err = pwmchip_add(&meson->chip);
[...]
+	meson_pwm_add_channels_data(meson, channels);
+
 	platform_set_drvdata(pdev, meson);

 	return 0;
}

The fix driver is in a separate branch, rebased on your for-next :
https://github.com/superna9999/linux/tree/amlogic/v4.8/pwm-for-next

and in a signed tag I can transform in a pull request if needed :
https://github.com/superna9999/linux/releases/tag/amlogic/v4.8/pwm-for-next-for-v4

[...]
> 
> I've pushed my modifications to the driver to the linux-pwm repository:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next
> 
> Alternatively you can also take a look at the for-4.9/drivers branch,
> but they're currently the same thing.
> 
> Thierry
> 

Thanks,
Neil

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

#1477191

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> Hi Thierry,
> 
> On 09/05/2016 11:00 AM, Thierry Reding wrote:
> > On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
> >> Add support for the PWM controller found in the Amlogic SoCs.
> >> This driver supports the Meson8b and GXBB SoCs.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >>  drivers/pwm/Kconfig     |   9 +
> >>  drivers/pwm/Makefile    |   1 +
> >>  drivers/pwm/pwm-meson.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 538 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-meson.c
> > 
> > Hi Neil,
> > 
> > sorry for taking so long to review this. I had actually started to write
> > a review email since I had noticed a couple of slight oddities about the
> > driver structure (primarily this was about how channel-specific data was
> > split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> > ended up making some changes to the driver in order to see what my
> > suggestions would look like, and if they would indeed improve things.
> > But once I had done that, I thought it a bit pointless to make that into
> > review comments and decided to just push what I had done and ask you to
> > take a look, and if you had no objections to the changes take the driver
> > for a spin to see if it still worked as expected.
> 
> We re-run our tests and I found 2 bugs, the first one is in meson_pwm_enable(),
> only the channel A was setup, the fix is :
> 
> static void meson_pwm_enable(...)
> -	u32 value, clk_shift, clk_enable, enable;
> +	u32 reg, value, clk_shift, clk_enable, enable;
> 
>  	switch (id) {
>  	case 0:
> [...]
> +		reg = REG_PWM_A;
>  		break;
>  	case 1:
> [...]
> +		reg = REG_PWM_B;
>  		break;
>  	}
> [...]
> -	writel(value, meson->base + REG_PWM_A);
> +	writel(value, meson->base + reg);

Ah indeed. Good catch.

> 
> The second bug is in probe(), I understand the point to allocate
> dynamically the channels and attach them to each pwm chip, but when
> calling meson_pwm_init_channels() we get an OOPS because
> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> meson_pwm_init_channels() would fix this, but in case of a clk
> PROBE_DEFER, we would need to remove back the pwmchip, which is a
> quite a bad design decision....

Ah yes... that one again. I remember running into that a while ago with
some other driver. To be honest, I think that's a short-coming of the
PWM subsystem and the fix would be for PWM chip registration to be split
into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
would be initialized using pwm_chip_init() where the pwms array would be
allocated, and pwm_chip_add() would register the chip with the system.

Currently a few drivers might be vulnerable to a race condition between
registration and implementation (i.e. PWM channels aren't fully set up
when they are exposed to users and sysfs).

> The smartest fix I found was to allocate channels in probe, init them
> them attach them after pwmchip_add():
> 
> static int meson_pwm_init_channels(..., struct meson_pwm_channel *channels)
> {
> +	struct meson_pwm_channel *channels;
> [...]
> -	for (i = 0; i < meson->chip.npwm; i++) {
> -		struct pwm_device *pwm = &meson->chip.pwms[i];
> -		struct meson_pwm_channel *channel;
> -
> -		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> -		if (!channel)
> -			return -ENOMEM;
> +	if (!channels)
> +		return -EINVAL;
> 
> +	for (i = 0; i < meson->chip.npwm; i++) {
> [...]
> +		memset(&channels[i], 0, sizeof(struct meson_pwm_channel));
> [...]
> //Rename "channel->" into "channels[i]."//
> [...]
> -		pwm_set_chip_data(pwm, channel);
>  	}
> 
>  	return 0;
> }
> 
> +static void meson_pwm_add_channels_data(struct meson_pwm *meson,
> +					struct meson_pwm_channel *channels)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < meson->chip.npwm; i++)
> +		pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
> +}
> 
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> +	struct meson_pwm_channel *channels;
> [...]
> -	err = meson_pwm_init_channels(meson);
> -	if (err < 0)
> -		return err;
> -
>  	meson->chip.dev = &pdev->dev;
> [...]
>  	meson->chip.of_pwm_n_cells = 3;
> 
> +	channels = devm_kmalloc_array(&pdev->dev, 2, sizeof(*meson),
> +				      GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	err = meson_pwm_init_channels(meson, channels);
> +	if (err < 0)
> +		return err;
> +
>  	err = pwmchip_add(&meson->chip);
> [...]
> +	meson_pwm_add_channels_data(meson, channels);
> +
>  	platform_set_drvdata(pdev, meson);
> 
>  	return 0;
> }

That's the race I was talking about above. I suppose it's not too big an
issue since other drivers seem to manage, so I'm going to merge your
fixed driver.

Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
split, in which case your driver would be the first to be race-free. =)

Thierry

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

#1477200

On 09/06/2016 11:07 AM, Thierry Reding wrote:
> On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
>> Hi Thierry,
>>
[...]
> 
>>
>> The second bug is in probe(), I understand the point to allocate
>> dynamically the channels and attach them to each pwm chip, but when
>> calling meson_pwm_init_channels() we get an OOPS because
>> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
>> meson_pwm_init_channels() would fix this, but in case of a clk
>> PROBE_DEFER, we would need to remove back the pwmchip, which is a
>> quite a bad design decision....
> 
> Ah yes... that one again. I remember running into that a while ago with
> some other driver. To be honest, I think that's a short-coming of the
> PWM subsystem and the fix would be for PWM chip registration to be split
> into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
> would be initialized using pwm_chip_init() where the pwms array would be
> allocated, and pwm_chip_add() would register the chip with the system.
> 
> Currently a few drivers might be vulnerable to a race condition between
> registration and implementation (i.e. PWM channels aren't fully set up
> when they are exposed to users and sysfs).
> 
>> The smartest fix I found was to allocate channels in probe, init them
>> them attach them after pwmchip_add():
>>
[...]

> 
> That's the race I was talking about above. I suppose it's not too big an
> issue since other drivers seem to manage, so I'm going to merge your
> fixed driver.

ok thanks !

> 
> Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
> split, in which case your driver would be the first to be race-free. =)

Having he driver upstream is a priority, but having it completely race-free would be great!
I'll be happy to collaborate to a race-free pwmchip probe somehow !

But there is still a glitch, when pwmadd_chip() returns, pwm_get_chip_data(pwm) will still
return crap until meson_pwm_add_channels_data() is called in the following instructions...

The pwm_chip_init()/pwm_chip_add() would be the only solution !

> Thierry
> 

Thanks,
Neil

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

#1477240

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote:
> On 09/06/2016 11:07 AM, Thierry Reding wrote:
> > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> >> Hi Thierry,
> >>
> [...]
> > 
> >>
> >> The second bug is in probe(), I understand the point to allocate
> >> dynamically the channels and attach them to each pwm chip, but when
> >> calling meson_pwm_init_channels() we get an OOPS because
> >> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> >> meson_pwm_init_channels() would fix this, but in case of a clk
> >> PROBE_DEFER, we would need to remove back the pwmchip, which is a
> >> quite a bad design decision....
> > 
> > Ah yes... that one again. I remember running into that a while ago with
> > some other driver. To be honest, I think that's a short-coming of the
> > PWM subsystem and the fix would be for PWM chip registration to be split
> > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip
> > would be initialized using pwm_chip_init() where the pwms array would be
> > allocated, and pwm_chip_add() would register the chip with the system.
> > 
> > Currently a few drivers might be vulnerable to a race condition between
> > registration and implementation (i.e. PWM channels aren't fully set up
> > when they are exposed to users and sysfs).
> > 
> >> The smartest fix I found was to allocate channels in probe, init them
> >> them attach them after pwmchip_add():
> >>
> [...]
> 
> > 
> > That's the race I was talking about above. I suppose it's not too big an
> > issue since other drivers seem to manage, so I'm going to merge your
> > fixed driver.
> 
> ok thanks !

I've made a few tiny changes (reg -> offset, temporary variable to track
&channels[i], ...) and pushed it all out. Hopefully that now fixes any
of the remaining issues.

> > Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add()
> > split, in which case your driver would be the first to be race-free. =)
> 
> Having he driver upstream is a priority, but having it completely
> race-free would be great! I'll be happy to collaborate to a race-free
> pwmchip probe somehow !

Fair enough. I'll do some prototyping and keep you in the loop if I come
up with something that I think will do.

Thierry

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

#1477313

On Tue, 2016-09-06 at 12:04 +0200, Thierry Reding wrote:
> On Tue, Sep 06, 2016 at 11:14:45AM +0200, Neil Armstrong wrote:
> > 
> > On 09/06/2016 11:07 AM, Thierry Reding wrote:
> > > 
> > > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> > > > 
> > > > Hi Thierry,
> > > > 
> > [...]
> > > 
> > > 
> > > > 
> > > > 
> > > > The second bug is in probe(), I understand the point to
> > > > allocate
> > > > dynamically the channels and attach them to each pwm chip, but
> > > > when
> > > > calling meson_pwm_init_channels() we get an OOPS because
> > > > meson->chip.pwms[i] are allocated in pwmchip_add(). Moving
> > > > meson_pwm_init_channels() would fix this, but in case of a clk
> > > > PROBE_DEFER, we would need to remove back the pwmchip, which is
> > > > a
> > > > quite a bad design decision....
> > > 
> > > Ah yes... that one again. I remember running into that a while
> > > ago with
> > > some other driver. To be honest, I think that's a short-coming of
> > > the
> > > PWM subsystem and the fix would be for PWM chip registration to
> > > be split
> > > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a
> > > chip
> > > would be initialized using pwm_chip_init() where the pwms array
> > > would be
> > > allocated, and pwm_chip_add() would register the chip with the
> > > system.
> > > 
> > > Currently a few drivers might be vulnerable to a race condition
> > > between
> > > registration and implementation (i.e. PWM channels aren't fully
> > > set up
> > > when they are exposed to users and sysfs).
> > > 
> > > > 
> > > > The smartest fix I found was to allocate channels in probe,
> > > > init them
> > > > them attach them after pwmchip_add():
> > > > 
> > [...]
> > 
> > > 
> > > 
> > > That's the race I was talking about above. I suppose it's not too
> > > big an
> > > issue since other drivers seem to manage, so I'm going to merge
> > > your
> > > fixed driver.
> > 
> > ok thanks !
> 
> I've made a few tiny changes (reg -> offset, temporary variable to
> track
> &channels[i], ...) and pushed it all out. Hopefully that now fixes
> any
> of the remaining issues.
> 
> > 
> > > 
> > > Unless you feel like taking a stab at the
> > > pwm_chip_init()/pwm_chip_add()
> > > split, in which case your driver would be the first to be race-
> > > free. =)
> > 
> > Having he driver upstream is a priority, but having it completely
> > race-free would be great! I'll be happy to collaborate to a race-
> > free
> > pwmchip probe somehow !
> 
> Fair enough. I'll do some prototyping and keep you in the loop if I
> come
> up with something that I think will do.
> 
> Thierry

Hi Thierry,

I have tested the latest version on the P200 (S905), channels E and F.
It works as expected.

Regards

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web