[4/6] pwm: sun4i: Add support for H6 PWM
diff mbox series

Message ID 20190726184045.14669-5-jernej.skrabec@siol.net
State New
Headers show
Series
  • pwm: sun4i: Add support for Allwinner H6
Related show

Commit Message

Jernej Skrabec July 26, 2019, 6:40 p.m. UTC
Now that sun4i PWM driver supports deasserting reset line and enabling
bus clock, support for H6 PWM can be added.

Note that while H6 PWM has two channels, only first one is wired to
output pin. Second channel is used as a clock source to companion AC200
chip which is bundled into same package.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/pwm/pwm-sun4i.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Uwe Kleine-König July 29, 2019, 6:40 a.m. UTC | #1
On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> Now that sun4i PWM driver supports deasserting reset line and enabling
> bus clock, support for H6 PWM can be added.
> 
> Note that while H6 PWM has two channels, only first one is wired to
> output pin. Second channel is used as a clock source to companion AC200
> chip which is bundled into same package.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/pwm/pwm-sun4i.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 7d3ac3f2dc3f..9e0eca79ff88 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data sun4i_pwm_single_bypass = {
>  	.npwm = 1,
>  };
>  
> +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> +	.has_bus_clock = true,
> +	.has_prescaler_bypass = true,
> +	.has_reset = true,
> +	.npwm = 2,
> +};
> +
>  static const struct of_device_id sun4i_pwm_dt_ids[] = {
>  	{
>  		.compatible = "allwinner,sun4i-a10-pwm",
> @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
>  	}, {
>  		.compatible = "allwinner,sun8i-h3-pwm",
>  		.data = &sun4i_pwm_single_bypass,
> +	}, {
> +		.compatible = "allwinner,sun50i-h6-pwm",
> +		.data = &sun50i_pwm_dual_bypass_clk_rst,

If you follow my suggestion for the two previous patches, you can just
use:

	compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";

and drop this patch.

Best regards
Uwe

>  	}, {
>  		/* sentinel */
>  	},
> -- 
> 2.22.0
> 
>
Jernej Skrabec July 29, 2019, 3:55 p.m. UTC | #2
Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König 
napisal(a):
> On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > Now that sun4i PWM driver supports deasserting reset line and enabling
> > bus clock, support for H6 PWM can be added.
> > 
> > Note that while H6 PWM has two channels, only first one is wired to
> > output pin. Second channel is used as a clock source to companion AC200
> > chip which is bundled into same package.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/pwm/pwm-sun4i.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > sun4i_pwm_single_bypass = {> 
> >  	.npwm = 1,
> >  
> >  };
> > 
> > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > +	.has_bus_clock = true,
> > +	.has_prescaler_bypass = true,
> > +	.has_reset = true,
> > +	.npwm = 2,
> > +};
> > +
> > 
> >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> >  
> >  	{
> >  	
> >  		.compatible = "allwinner,sun4i-a10-pwm",
> > 
> > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > {
> > 
> >  	}, {
> >  	
> >  		.compatible = "allwinner,sun8i-h3-pwm",
> >  		.data = &sun4i_pwm_single_bypass,
> > 
> > +	}, {
> > +		.compatible = "allwinner,sun50i-h6-pwm",
> > +		.data = &sun50i_pwm_dual_bypass_clk_rst,
> 
> If you follow my suggestion for the two previous patches, you can just
> use:
> 
> 	compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> 
> and drop this patch.

Maxime found out that it's not compatible with A10s due to difference in bypass 
bit, but yes, I know what you mean.

Since H6 requires reset line and bus clock to be specified, it's not compatible 
from DT binding side. New yaml based binding must somehow know that in order 
to be able to validate DT node, so it needs standalone compatible. However, 
depending on conclusions of other discussions, this new compatible can be 
associated with already available quirks structure or have it's own.

Best regards,
Jernej

> 
> Best regards
> Uwe
> 
> >  	}, {
> >  	
> >  		/* sentinel */
> >  	
> >  	},
Uwe Kleine-König July 29, 2019, 4:07 p.m. UTC | #3
On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> Hi Uwe,
> 
> Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König 
> napisal(a):
> > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > Now that sun4i PWM driver supports deasserting reset line and enabling
> > > bus clock, support for H6 PWM can be added.
> > > 
> > > Note that while H6 PWM has two channels, only first one is wired to
> > > output pin. Second channel is used as a clock source to companion AC200
> > > chip which is bundled into same package.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/pwm/pwm-sun4i.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > sun4i_pwm_single_bypass = {> 
> > >  	.npwm = 1,
> > >  
> > >  };
> > > 
> > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > +	.has_bus_clock = true,
> > > +	.has_prescaler_bypass = true,
> > > +	.has_reset = true,
> > > +	.npwm = 2,
> > > +};
> > > +
> > > 
> > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > >  
> > >  	{
> > >  	
> > >  		.compatible = "allwinner,sun4i-a10-pwm",
> > > 
> > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > > {
> > > 
> > >  	}, {
> > >  	
> > >  		.compatible = "allwinner,sun8i-h3-pwm",
> > >  		.data = &sun4i_pwm_single_bypass,
> > > 
> > > +	}, {
> > > +		.compatible = "allwinner,sun50i-h6-pwm",
> > > +		.data = &sun50i_pwm_dual_bypass_clk_rst,
> > 
> > If you follow my suggestion for the two previous patches, you can just
> > use:
> > 
> > 	compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > 
> > and drop this patch.
> 
> Maxime found out that it's not compatible with A10s due to difference in bypass 
> bit, but yes, I know what you mean.
> 
> Since H6 requires reset line and bus clock to be specified, it's not compatible 
> from DT binding side. New yaml based binding must somehow know that in order 
> to be able to validate DT node, so it needs standalone compatible. However, 
> depending on conclusions of other discussions, this new compatible can be 
> associated with already available quirks structure or have it's own.

I cannot follow. You should be able to specify in the binding that the
reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
without a reset line and bus clock also verifies, but this doesn't
really hurt (and who knows, maybe the next allwinner chip needs exactly
this).

Best regards
Uwe
Chen-Yu Tsai July 29, 2019, 4:09 p.m. UTC | #4
On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > Hi Uwe,
> >
> > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > napisal(a):
> > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > Now that sun4i PWM driver supports deasserting reset line and enabling
> > > > bus clock, support for H6 PWM can be added.
> > > >
> > > > Note that while H6 PWM has two channels, only first one is wired to
> > > > output pin. Second channel is used as a clock source to companion AC200
> > > > chip which is bundled into same package.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > >
> > > >  drivers/pwm/pwm-sun4i.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index 7d3ac3f2dc3f..9e0eca79ff88 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > sun4i_pwm_single_bypass = {>
> > > >   .npwm = 1,
> > > >
> > > >  };
> > > >
> > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > > + .has_bus_clock = true,
> > > > + .has_prescaler_bypass = true,
> > > > + .has_reset = true,
> > > > + .npwm = 2,
> > > > +};
> > > > +
> > > >
> > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > >
> > > >   {
> > > >
> > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > >
> > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > > > {
> > > >
> > > >   }, {
> > > >
> > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > >           .data = &sun4i_pwm_single_bypass,
> > > >
> > > > + }, {
> > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > >
> > > If you follow my suggestion for the two previous patches, you can just
> > > use:
> > >
> > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > >
> > > and drop this patch.
> >
> > Maxime found out that it's not compatible with A10s due to difference in bypass
> > bit, but yes, I know what you mean.
> >
> > Since H6 requires reset line and bus clock to be specified, it's not compatible
> > from DT binding side. New yaml based binding must somehow know that in order
> > to be able to validate DT node, so it needs standalone compatible. However,
> > depending on conclusions of other discussions, this new compatible can be
> > associated with already available quirks structure or have it's own.
>
> I cannot follow. You should be able to specify in the binding that the
> reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> without a reset line and bus clock also verifies, but this doesn't
> really hurt (and who knows, maybe the next allwinner chip needs exactly
> this).

It is not optional. It will not work if either the clocks or reset controls
are missing. How would these be optional anyway? Either it's connected and
thus required, or it's not and therefore should be omitted from the
description.

ChenYu
Uwe Kleine-König July 29, 2019, 4:24 p.m. UTC | #5
Hello,

On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > napisal(a):
> > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > sun4i_pwm_single_bypass = {>
> > > > >   .npwm = 1,
> > > > >
> > > > >  };
> > > > >
> > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
> > > > > + .has_bus_clock = true,
> > > > > + .has_prescaler_bypass = true,
> > > > > + .has_reset = true,
> > > > > + .npwm = 2,
> > > > > +};
> > > > > +
> > > > >
> > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > >
> > > > >   {
> > > > >
> > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > >
> > > > > @@ -347,6 +354,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] =
> > > > > {
> > > > >
> > > > >   }, {
> > > > >
> > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > >           .data = &sun4i_pwm_single_bypass,
> > > > >
> > > > > + }, {
> > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > >
> > > > If you follow my suggestion for the two previous patches, you can just
> > > > use:
> > > >
> > > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > >
> > > > and drop this patch.
> > >
> > > Maxime found out that it's not compatible with A10s due to difference in bypass
> > > bit, but yes, I know what you mean.
> > >
> > > Since H6 requires reset line and bus clock to be specified, it's not compatible
> > > from DT binding side. New yaml based binding must somehow know that in order
> > > to be able to validate DT node, so it needs standalone compatible. However,
> > > depending on conclusions of other discussions, this new compatible can be
> > > associated with already available quirks structure or have it's own.
> >
> > I cannot follow. You should be able to specify in the binding that the
> > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > without a reset line and bus clock also verifies, but this doesn't
> > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > this).
> 
> It is not optional. It will not work if either the clocks or reset controls
> are missing. How would these be optional anyway? Either it's connected and
> thus required, or it's not and therefore should be omitted from the
> description.

[Just arguing about the clock here, the argumentation is analogous for
the reset control.]

From the driver's perspective it's optional: There are devices with and
without a bus clock. This doesn't mean that you can just ignore this
clock if it's specified. It's optional in the sense "If dt doesn't
specify it, then assume this is a device that doesn't have it and so you
don't need to handle it." but not in the sense "it doesn't matter if
you handle it or not.".

Other than that I'm on your side. So for example I think it's not
optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
because this hides exactly the kind of problem you point out here.

Best regards
Uwe
Jernej Skrabec July 29, 2019, 4:40 p.m. UTC | #6
Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König 
napisal(a):
> Hello,
> 
> On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > 
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > 
> > > > napisal(a):
> > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > sun4i_pwm_single_bypass = {>
> > > > > > 
> > > > > >   .npwm = 1,
> > > > > >  
> > > > > >  };
> > > > > > 
> > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst
> > > > > > = {
> > > > > > + .has_bus_clock = true,
> > > > > > + .has_prescaler_bypass = true,
> > > > > > + .has_reset = true,
> > > > > > + .npwm = 2,
> > > > > > +};
> > > > > > +
> > > > > > 
> > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > >  
> > > > > >   {
> > > > > >   
> > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > 
> > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > sun4i_pwm_dt_ids[] =
> > > > > > {
> > > > > > 
> > > > > >   }, {
> > > > > >   
> > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > 
> > > > > > + }, {
> > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > 
> > > > > If you follow my suggestion for the two previous patches, you can
> > > > > just
> > > > > 
> > > > > use:
> > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > 
> > > > > and drop this patch.
> > > > 
> > > > Maxime found out that it's not compatible with A10s due to difference
> > > > in bypass bit, but yes, I know what you mean.
> > > > 
> > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > compatible from DT binding side. New yaml based binding must somehow
> > > > know that in order to be able to validate DT node, so it needs
> > > > standalone compatible. However, depending on conclusions of other
> > > > discussions, this new compatible can be associated with already
> > > > available quirks structure or have it's own.> > 
> > > I cannot follow. You should be able to specify in the binding that the
> > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > without a reset line and bus clock also verifies, but this doesn't
> > > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > > this).
> > 
> > It is not optional. It will not work if either the clocks or reset
> > controls
> > are missing. How would these be optional anyway? Either it's connected and
> > thus required, or it's not and therefore should be omitted from the
> > description.
> 
> [Just arguing about the clock here, the argumentation is analogous for
> the reset control.]
> 
> From the driver's perspective it's optional: There are devices with and
> without a bus clock. This doesn't mean that you can just ignore this
> clock if it's specified. It's optional in the sense "If dt doesn't
> specify it, then assume this is a device that doesn't have it and so you
> don't need to handle it." but not in the sense "it doesn't matter if
> you handle it or not.".
> 
> Other than that I'm on your side. So for example I think it's not
> optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> because this hides exactly the kind of problem you point out here.
>

I think there's misunderstanding. I only argued that we can't use

compatible = "allwinner,sun50i-h6-pwm",
	 "allwinner,sun5i-a10s-pwm";

as you suggested and only 

compatible = "allwinner,sun50i-h6-pwm"; 

will work. Not because of driver itself (it can still use _optional() 
variants), but because of DT binding, which should be able to validate H6 PWM 
node - reset and bus clock references are required in this case.

Best regards,
Jernej
 
> Best regards
> Uwe
Uwe Kleine-König July 29, 2019, 6:40 p.m. UTC | #7
On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König 
> napisal(a):
> > Hello,
> > 
> > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > 
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > 
> > > > > napisal(a):
> > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > 
> > > > > > >   .npwm = 1,
> > > > > > >  
> > > > > > >  };
> > > > > > > 
> > > > > > > +static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst
> > > > > > > = {
> > > > > > > + .has_bus_clock = true,
> > > > > > > + .has_prescaler_bypass = true,
> > > > > > > + .has_reset = true,
> > > > > > > + .npwm = 2,
> > > > > > > +};
> > > > > > > +
> > > > > > > 
> > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > >  
> > > > > > >   {
> > > > > > >   
> > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > 
> > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > {
> > > > > > > 
> > > > > > >   }, {
> > > > > > >   
> > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > 
> > > > > > > + }, {
> > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > 
> > > > > > If you follow my suggestion for the two previous patches, you can
> > > > > > just
> > > > > > 
> > > > > > use:
> > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > 
> > > > > > and drop this patch.
> > > > > 
> > > > > Maxime found out that it's not compatible with A10s due to difference
> > > > > in bypass bit, but yes, I know what you mean.
> > > > > 
> > > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > > compatible from DT binding side. New yaml based binding must somehow
> > > > > know that in order to be able to validate DT node, so it needs
> > > > > standalone compatible. However, depending on conclusions of other
> > > > > discussions, this new compatible can be associated with already
> > > > > available quirks structure or have it's own.> > 
> > > > I cannot follow. You should be able to specify in the binding that the
> > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > without a reset line and bus clock also verifies, but this doesn't
> > > > really hurt (and who knows, maybe the next allwinner chip needs exactly
> > > > this).
> > > 
> > > It is not optional. It will not work if either the clocks or reset
> > > controls
> > > are missing. How would these be optional anyway? Either it's connected and
> > > thus required, or it's not and therefore should be omitted from the
> > > description.
> > 
> > [Just arguing about the clock here, the argumentation is analogous for
> > the reset control.]
> > 
> > From the driver's perspective it's optional: There are devices with and
> > without a bus clock. This doesn't mean that you can just ignore this
> > clock if it's specified. It's optional in the sense "If dt doesn't
> > specify it, then assume this is a device that doesn't have it and so you
> > don't need to handle it." but not in the sense "it doesn't matter if
> > you handle it or not.".
> > 
> > Other than that I'm on your side. So for example I think it's not
> > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > because this hides exactly the kind of problem you point out here.
> >
> 
> I think there's misunderstanding. I only argued that we can't use
> 
> compatible = "allwinner,sun50i-h6-pwm",
> 	 "allwinner,sun5i-a10s-pwm";
> 
> as you suggested and only 
> 
> compatible = "allwinner,sun50i-h6-pwm"; 
> 
> will work. Not because of driver itself (it can still use _optional() 
> variants), but because of DT binding, which should be able to validate H6 PWM 
> node - reset and bus clock references are required in this case.

I think I understood. In my eyes there is no need to let validation of
the DT bindings catch a missing "optional" property that is needed on
H6.

You have to draw the line somewhere which information the driver has
hard-coded and what is only provided by the device tree and just assumed
to be correct by the driver. You argue the driver should know that if it
cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
that there is a clock named "bus" and a resets property that links to a
reset controller. How is that different from checking that the base
address is 0x0300a000 or that the "pwm" clock is the osc24M clock
running at 24 MHz? This isn't checked in the driver or the dt schema.
Still if the device tree got one of them wrong this yields an
non-working pwm device that isn't catched in the driver.

Best regards
Uwe
Jernej Skrabec July 29, 2019, 6:46 p.m. UTC | #8
Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König 
napisal(a):
> On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > 
> > napisal(a):
> > > Hello,
> > > 
> > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > 
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > Kleine-König
> > > > > > 
> > > > > > napisal(a):
> > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > 
> > > > > > > >   .npwm = 1,
> > > > > > > >  
> > > > > > > >  };
> > > > > > > > 
> > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > = {
> > > > > > > > + .has_bus_clock = true,
> > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > + .has_reset = true,
> > > > > > > > + .npwm = 2,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > 
> > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > >  
> > > > > > > >   {
> > > > > > > >   
> > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > 
> > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > {
> > > > > > > > 
> > > > > > > >   }, {
> > > > > > > >   
> > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > 
> > > > > > > > + }, {
> > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > 
> > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > can
> > > > > > > just
> > > > > > > 
> > > > > > > use:
> > > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > > 
> > > > > > > and drop this patch.
> > > > > > 
> > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > difference
> > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > 
> > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > not
> > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > somehow
> > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > discussions, this new compatible can be associated with already
> > > > > > available quirks structure or have it's own.> >
> > > > > 
> > > > > I cannot follow. You should be able to specify in the binding that
> > > > > the
> > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > exactly
> > > > > this).
> > > > 
> > > > It is not optional. It will not work if either the clocks or reset
> > > > controls
> > > > are missing. How would these be optional anyway? Either it's connected
> > > > and
> > > > thus required, or it's not and therefore should be omitted from the
> > > > description.
> > > 
> > > [Just arguing about the clock here, the argumentation is analogous for
> > > the reset control.]
> > > 
> > > From the driver's perspective it's optional: There are devices with and
> > > without a bus clock. This doesn't mean that you can just ignore this
> > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > specify it, then assume this is a device that doesn't have it and so you
> > > don't need to handle it." but not in the sense "it doesn't matter if
> > > you handle it or not.".
> > > 
> > > Other than that I'm on your side. So for example I think it's not
> > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > because this hides exactly the kind of problem you point out here.
> > 
> > I think there's misunderstanding. I only argued that we can't use
> > 
> > compatible = "allwinner,sun50i-h6-pwm",
> > 
> > 	 "allwinner,sun5i-a10s-pwm";
> > 
> > as you suggested and only
> > 
> > compatible = "allwinner,sun50i-h6-pwm";
> > 
> > will work. Not because of driver itself (it can still use _optional()
> > variants), but because of DT binding, which should be able to validate H6
> > PWM node - reset and bus clock references are required in this case.
> 
> I think I understood. In my eyes there is no need to let validation of
> the DT bindings catch a missing "optional" property that is needed on
> H6.
> 
> You have to draw the line somewhere which information the driver has
> hard-coded and what is only provided by the device tree and just assumed
> to be correct by the driver. You argue the driver should know that 

No, in this thread I argue that DT validation tool, executed by

make ARCH=arm64 dtbs_check

should catch that. This is not a driver, but DT binding described in YAML.

Best regards,
Jernej

> if it
> cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
> that there is a clock named "bus" and a resets property that links to a
> reset controller. How is that different from checking that the base
> address is 0x0300a000 or that the "pwm" clock is the osc24M clock
> running at 24 MHz? This isn't checked in the driver or the dt schema.
> Still if the device tree got one of them wrong this yields an
> non-working pwm device that isn't catched in the driver.
> 
> Best regards
> Uwe
Uwe Kleine-König July 29, 2019, 6:51 p.m. UTC | #9
On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König 
> napisal(a):
> > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > 
> > > napisal(a):
> > > > Hello,
> > > > 
> > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > 
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > Kleine-König
> > > > > > > 
> > > > > > > napisal(a):
> > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > 
> > > > > > > > >   .npwm = 1,
> > > > > > > > >  
> > > > > > > > >  };
> > > > > > > > > 
> > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > = {
> > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > + .has_reset = true,
> > > > > > > > > + .npwm = 2,
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > 
> > > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > >  
> > > > > > > > >   {
> > > > > > > > >   
> > > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > 
> > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > {
> > > > > > > > > 
> > > > > > > > >   }, {
> > > > > > > > >   
> > > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > > 
> > > > > > > > > + }, {
> > > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > 
> > > > > > > > If you follow my suggestion for the two previous patches, you
> > > > > > > > can
> > > > > > > > just
> > > > > > > > 
> > > > > > > > use:
> > > > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > > > 
> > > > > > > > and drop this patch.
> > > > > > > 
> > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > difference
> > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > 
> > > > > > > Since H6 requires reset line and bus clock to be specified, it's
> > > > > > > not
> > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > somehow
> > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > > discussions, this new compatible can be associated with already
> > > > > > > available quirks structure or have it's own.> >
> > > > > > 
> > > > > > I cannot follow. You should be able to specify in the binding that
> > > > > > the
> > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > exactly
> > > > > > this).
> > > > > 
> > > > > It is not optional. It will not work if either the clocks or reset
> > > > > controls
> > > > > are missing. How would these be optional anyway? Either it's connected
> > > > > and
> > > > > thus required, or it's not and therefore should be omitted from the
> > > > > description.
> > > > 
> > > > [Just arguing about the clock here, the argumentation is analogous for
> > > > the reset control.]
> > > > 
> > > > From the driver's perspective it's optional: There are devices with and
> > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > specify it, then assume this is a device that doesn't have it and so you
> > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > you handle it or not.".
> > > > 
> > > > Other than that I'm on your side. So for example I think it's not
> > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > because this hides exactly the kind of problem you point out here.
> > > 
> > > I think there's misunderstanding. I only argued that we can't use
> > > 
> > > compatible = "allwinner,sun50i-h6-pwm",
> > > 
> > > 	 "allwinner,sun5i-a10s-pwm";
> > > 
> > > as you suggested and only
> > > 
> > > compatible = "allwinner,sun50i-h6-pwm";
> > > 
> > > will work. Not because of driver itself (it can still use _optional()
> > > variants), but because of DT binding, which should be able to validate H6
> > > PWM node - reset and bus clock references are required in this case.
> > 
> > I think I understood. In my eyes there is no need to let validation of
> > the DT bindings catch a missing "optional" property that is needed on
> > H6.
> > 
> > You have to draw the line somewhere which information the driver has
> > hard-coded and what is only provided by the device tree and just assumed
> > to be correct by the driver. You argue the driver should know that 
> 
> No, in this thread I argue that DT validation tool, executed by
> 
> make ARCH=arm64 dtbs_check
> 
> should catch that. This is not a driver, but DT binding described in YAML.

The argumentation is the same. dtbs_check doesn't notice if the base
address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
it catch a missing reset controller phandle?

Best regards
Uwe
Jernej Skrabec July 29, 2019, 10:04 p.m. UTC | #10
Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König 
napisal(a):
> On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> > 
> > napisal(a):
> > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > > 
> > > > napisal(a):
> > > > > Hello,
> > > > > 
> > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > 
> > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > > Kleine-König
> > > > > > > > 
> > > > > > > > napisal(a):
> > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec 
wrote:
> > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > > 
> > > > > > > > > >   .npwm = 1,
> > > > > > > > > >  
> > > > > > > > > >  };
> > > > > > > > > > 
> > > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > > = {
> > > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > > + .has_reset = true,
> > > > > > > > > > + .npwm = 2,
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > 
> > > > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > > >  
> > > > > > > > > >   {
> > > > > > > > > >   
> > > > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > > 
> > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > > {
> > > > > > > > > > 
> > > > > > > > > >   }, {
> > > > > > > > > >   
> > > > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > > > 
> > > > > > > > > > + }, {
> > > > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > > 
> > > > > > > > > If you follow my suggestion for the two previous patches,
> > > > > > > > > you
> > > > > > > > > can
> > > > > > > > > just
> > > > > > > > > 
> > > > > > > > > use:
> > > > > > > > >     compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > >     "allwinner,sun5i-a10s-pwm";
> > > > > > > > > 
> > > > > > > > > and drop this patch.
> > > > > > > > 
> > > > > > > > Maxime found out that it's not compatible with A10s due to
> > > > > > > > difference
> > > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > > 
> > > > > > > > Since H6 requires reset line and bus clock to be specified,
> > > > > > > > it's
> > > > > > > > not
> > > > > > > > compatible from DT binding side. New yaml based binding must
> > > > > > > > somehow
> > > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > > standalone compatible. However, depending on conclusions of
> > > > > > > > other
> > > > > > > > discussions, this new compatible can be associated with
> > > > > > > > already
> > > > > > > > available quirks structure or have it's own.> >
> > > > > > > 
> > > > > > > I cannot follow. You should be able to specify in the binding
> > > > > > > that
> > > > > > > the
> > > > > > > reset line and bus clock is optional. Then
> > > > > > > allwinner,sun50i-h6-pwm
> > > > > > > without a reset line and bus clock also verifies, but this
> > > > > > > doesn't
> > > > > > > really hurt (and who knows, maybe the next allwinner chip needs
> > > > > > > exactly
> > > > > > > this).
> > > > > > 
> > > > > > It is not optional. It will not work if either the clocks or reset
> > > > > > controls
> > > > > > are missing. How would these be optional anyway? Either it's
> > > > > > connected
> > > > > > and
> > > > > > thus required, or it's not and therefore should be omitted from
> > > > > > the
> > > > > > description.
> > > > > 
> > > > > [Just arguing about the clock here, the argumentation is analogous
> > > > > for
> > > > > the reset control.]
> > > > > 
> > > > > From the driver's perspective it's optional: There are devices with
> > > > > and
> > > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > > specify it, then assume this is a device that doesn't have it and so
> > > > > you
> > > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > > you handle it or not.".
> > > > > 
> > > > > Other than that I'm on your side. So for example I think it's not
> > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > > because this hides exactly the kind of problem you point out here.
> > > > 
> > > > I think there's misunderstanding. I only argued that we can't use
> > > > 
> > > > compatible = "allwinner,sun50i-h6-pwm",
> > > > 
> > > > 	 "allwinner,sun5i-a10s-pwm";
> > > > 
> > > > as you suggested and only
> > > > 
> > > > compatible = "allwinner,sun50i-h6-pwm";
> > > > 
> > > > will work. Not because of driver itself (it can still use _optional()
> > > > variants), but because of DT binding, which should be able to validate
> > > > H6
> > > > PWM node - reset and bus clock references are required in this case.
> > > 
> > > I think I understood. In my eyes there is no need to let validation of
> > > the DT bindings catch a missing "optional" property that is needed on
> > > H6.
> > > 
> > > You have to draw the line somewhere which information the driver has
> > > hard-coded and what is only provided by the device tree and just assumed
> > > to be correct by the driver. You argue the driver should know that
> > 
> > No, in this thread I argue that DT validation tool, executed by
> > 
> > make ARCH=arm64 dtbs_check
> > 
> > should catch that. This is not a driver, but DT binding described in YAML.
> 
> The argumentation is the same. dtbs_check doesn't notice if the base
> address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
> it catch a missing reset controller phandle?

Of course checking actual values of node properties doesn't make sense in 
dtbs_check, otherwise we would have million DT bindings. If you have 10 copies 
of the same IP core, of course you would use same compatible, but actual 
register ranges, interrupts, etc. would be different in DT nodes.

At this point I would make same argument as were made before, but there is no 
point going in circles. I'm interested what have DT maintainers to say.

Best regards,
Jernej
Uwe Kleine-König July 30, 2019, 8:09 a.m. UTC | #11
Hello Rob and Frank,

Maxime and Jernej on one side and me on the other cannot agree about a
detail in the change to the bindings here. I'm trying to objectively
summarize the situation for you to help deciding what is the right thing
to do here.

TLDR: The sun4i pwm driver is extended to support a new variant of that
device on the H6 SoC. Compared to the earlier supported variants
allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
additional clock. 

The two positions are:

 - We need a new compatible because only then the driver and/or the dt
   schema checker can check that each "allwinner,sun50i-h6-pwm" device
   has a reset property and a "bus" clock; and the earlier variants
   don't.

 - The driver can be simpler and the device specific knowledge is only
   in a single place (the dt) if the device tree is considered valid and
   a reset property and "bus" clock is used iff it's provided in the
   device tree without additional comparison for the compatible.

Now our arguments seem to go in circles and Jernej was interested in
your position. That's something I agree with ;-) Can you please share
your view?

Find below some context about the arguments.

Best regards
Uwe

On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König 
> napisal(a):
> > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> > > napisal(a):
> > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > > > napisal(a):
> > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > > > > > napisal(a):
> > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > > > 
> > > > > > > > > > >   .npwm = 1,
> > > > > > > > > > >  
> > > > > > > > > > >  };
> > > > > > > > > > > 
> > > > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > > > = {
> > > > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > > > + .has_reset = true,
> > > > > > > > > > > + .npwm = 2,
> > > > > > > > > > > +};
> > > > > > > > > > > +
> > > > > > > > > > > 
> > > > > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > > > >  
> > > > > > > > > > >   {
> > > > > > > > > > >   
> > > > > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > > > 
> > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > > > {
> > > > > > > > > > > 
> > > > > > > > > > >   }, {
> > > > > > > > > > >   
> > > > > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > > > > 
> > > > > > > > > > > + }, {
> > > > > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > > > 
> > > > > > > > > > If you follow my suggestion for the two previous patches,

(i.e. use devm_clk_get_optional instead of using devm_clk_get iff the
compatible is allwinner,sun50i-h6-pwm; analogous for the reset
controller.)

> > > > > > > > > > you can just use:
> > > > > > > > > >
> > > > > > > > > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > > > > > > 
> > > > > > > > > > and drop this patch.
> > > > > > > > > 
> > > > > > > > > Maxime found out that it's not compatible with A10s due to difference
> > > > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > > > 
> > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > > > > > > compatible from DT binding side. New yaml based binding must somehow
> > > > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > > > > discussions, this new compatible can be associated with already
> > > > > > > > > available quirks structure or have it's own.
> > > > > > > > 
> > > > > > > > I cannot follow. You should be able to specify in the binding that the
> > > > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs exactly this).
> > > > > > > 
> > > > > > > It is not optional. It will not work if either the clocks or reset controls
> > > > > > > are missing. How would these be optional anyway? Either it's connected and
> > > > > > > thus required, or it's not and therefore should be omitted from the description.
> > > > > > 
> > > > > > [Just arguing about the clock here, the argumentation is analogous for
> > > > > > the reset control.]
> > > > > > 
> > > > > > From the driver's perspective it's optional: There are devices with and
> > > > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > > > specify it, then assume this is a device that doesn't have it and so you
> > > > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > > > you handle it or not.".
> > > > > > 
> > > > > > Other than that I'm on your side. So for example I think it's not
> > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > > > because this hides exactly the kind of problem you point out here.
> > > > > 
> > > > > I think there's misunderstanding. I only argued that we can't use
> > > > > 
> > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > 
> > > > > as you suggested and only
> > > > > 
> > > > > compatible = "allwinner,sun50i-h6-pwm";
> > > > > 
> > > > > will work. Not because of driver itself (it can still use _optional()
> > > > > variants), but because of DT binding, which should be able to validate H6
> > > > > PWM node - reset and bus clock references are required in this case.
> > > > 
> > > > I think I understood. In my eyes there is no need to let validation of
> > > > the DT bindings catch a missing "optional" property that is needed on
> > > > H6.
> > > > 
> > > > You have to draw the line somewhere which information the driver has
> > > > hard-coded and what is only provided by the device tree and just assumed
> > > > to be correct by the driver. You argue the driver should know that
> > > 
> > > No, in this thread I argue that DT validation tool, executed by
> > > 
> > > make ARCH=arm64 dtbs_check
> > > 
> > > should catch that. This is not a driver, but DT binding described in YAML.
> > 
> > The argumentation is the same. dtbs_check doesn't notice if the base
> > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
> > it catch a missing reset controller phandle?
> 
> Of course checking actual values of node properties doesn't make sense in 
> dtbs_check, otherwise we would have million DT bindings. If you have 10 copies 
> of the same IP core, of course you would use same compatible, but actual 
> register ranges, interrupts, etc. would be different in DT nodes.
> 
> At this point I would make same argument as were made before, but there is no 
> point going in circles. I'm interested what have DT maintainers to say.
Chen-Yu Tsai July 30, 2019, 8:32 a.m. UTC | #12
On Tue, Jul 30, 2019 at 4:09 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Rob and Frank,
>
> Maxime and Jernej on one side and me on the other cannot agree about a
> detail in the change to the bindings here. I'm trying to objectively
> summarize the situation for you to help deciding what is the right thing
> to do here.
>
> TLDR: The sun4i pwm driver is extended to support a new variant of that
> device on the H6 SoC. Compared to the earlier supported variants
> allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> additional clock.
>
> The two positions are:
>
>  - We need a new compatible because only then the driver and/or the dt
>    schema checker can check that each "allwinner,sun50i-h6-pwm" device
>    has a reset property and a "bus" clock; and the earlier variants
>    don't.
>
>  - The driver can be simpler and the device specific knowledge is only
>    in a single place (the dt) if the device tree is considered valid and
>    a reset property and "bus" clock is used iff it's provided in the
>    device tree without additional comparison for the compatible.
>
> Now our arguments seem to go in circles and Jernej was interested in
> your position. That's something I agree with ;-) Can you please share
> your view?
>
> Find below some context about the arguments.

A bit more context on the failure modes:

If the reset control is missing, anything done to hardware will be
silently ignored, since any writes to the registers are ignored.

On the other hand, if the bus clock is missing and otherwise not enabled,
accessing the device's registers could actually stall the whole system.

ChenYu

> Best regards
> Uwe
>
> On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König
> > napisal(a):
> > > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> > > > napisal(a):
> > > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > > > > napisal(a):
> > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > > > > > > napisal(a):
> > > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > > > > > > > @@ -331,6 +331,13 @@ static const struct sun4i_pwm_data
> > > > > > > > > > > > sun4i_pwm_single_bypass = {>
> > > > > > > > > > > >
> > > > > > > > > > > >   .npwm = 1,
> > > > > > > > > > > >
> > > > > > > > > > > >  };
> > > > > > > > > > > >
> > > > > > > > > > > > +static const struct sun4i_pwm_data
> > > > > > > > > > > > sun50i_pwm_dual_bypass_clk_rst
> > > > > > > > > > > > = {
> > > > > > > > > > > > + .has_bus_clock = true,
> > > > > > > > > > > > + .has_prescaler_bypass = true,
> > > > > > > > > > > > + .has_reset = true,
> > > > > > > > > > > > + .npwm = 2,
> > > > > > > > > > > > +};
> > > > > > > > > > > > +
> > > > > > > > > > > >
> > > > > > > > > > > >  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > > > > > > > > > > >
> > > > > > > > > > > >   {
> > > > > > > > > > > >
> > > > > > > > > > > >           .compatible = "allwinner,sun4i-a10-pwm",
> > > > > > > > > > > >
> > > > > > > > > > > > @@ -347,6 +354,9 @@ static const struct of_device_id
> > > > > > > > > > > > sun4i_pwm_dt_ids[] =
> > > > > > > > > > > > {
> > > > > > > > > > > >
> > > > > > > > > > > >   }, {
> > > > > > > > > > > >
> > > > > > > > > > > >           .compatible = "allwinner,sun8i-h3-pwm",
> > > > > > > > > > > >           .data = &sun4i_pwm_single_bypass,
> > > > > > > > > > > >
> > > > > > > > > > > > + }, {
> > > > > > > > > > > > +         .compatible = "allwinner,sun50i-h6-pwm",
> > > > > > > > > > > > +         .data = &sun50i_pwm_dual_bypass_clk_rst,
> > > > > > > > > > >
> > > > > > > > > > > If you follow my suggestion for the two previous patches,
>
> (i.e. use devm_clk_get_optional instead of using devm_clk_get iff the
> compatible is allwinner,sun50i-h6-pwm; analogous for the reset
> controller.)
>
> > > > > > > > > > > you can just use:
> > > > > > > > > > >
> > > > > > > > > > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > > > > > > >
> > > > > > > > > > > and drop this patch.
> > > > > > > > > >
> > > > > > > > > > Maxime found out that it's not compatible with A10s due to difference
> > > > > > > > > > in bypass bit, but yes, I know what you mean.
> > > > > > > > > >
> > > > > > > > > > Since H6 requires reset line and bus clock to be specified, it's not
> > > > > > > > > > compatible from DT binding side. New yaml based binding must somehow
> > > > > > > > > > know that in order to be able to validate DT node, so it needs
> > > > > > > > > > standalone compatible. However, depending on conclusions of other
> > > > > > > > > > discussions, this new compatible can be associated with already
> > > > > > > > > > available quirks structure or have it's own.
> > > > > > > > >
> > > > > > > > > I cannot follow. You should be able to specify in the binding that the
> > > > > > > > > reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
> > > > > > > > > without a reset line and bus clock also verifies, but this doesn't
> > > > > > > > > really hurt (and who knows, maybe the next allwinner chip needs exactly this).
> > > > > > > >
> > > > > > > > It is not optional. It will not work if either the clocks or reset controls
> > > > > > > > are missing. How would these be optional anyway? Either it's connected and
> > > > > > > > thus required, or it's not and therefore should be omitted from the description.
> > > > > > >
> > > > > > > [Just arguing about the clock here, the argumentation is analogous for
> > > > > > > the reset control.]
> > > > > > >
> > > > > > > From the driver's perspective it's optional: There are devices with and
> > > > > > > without a bus clock. This doesn't mean that you can just ignore this
> > > > > > > clock if it's specified. It's optional in the sense "If dt doesn't
> > > > > > > specify it, then assume this is a device that doesn't have it and so you
> > > > > > > don't need to handle it." but not in the sense "it doesn't matter if
> > > > > > > you handle it or not.".
> > > > > > >
> > > > > > > Other than that I'm on your side. So for example I think it's not
> > > > > > > optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
> > > > > > > devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
> > > > > > > because this hides exactly the kind of problem you point out here.
> > > > > >
> > > > > > I think there's misunderstanding. I only argued that we can't use
> > > > > >
> > > > > > compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > >
> > > > > > as you suggested and only
> > > > > >
> > > > > > compatible = "allwinner,sun50i-h6-pwm";
> > > > > >
> > > > > > will work. Not because of driver itself (it can still use _optional()
> > > > > > variants), but because of DT binding, which should be able to validate H6
> > > > > > PWM node - reset and bus clock references are required in this case.
> > > > >
> > > > > I think I understood. In my eyes there is no need to let validation of
> > > > > the DT bindings catch a missing "optional" property that is needed on
> > > > > H6.
> > > > >
> > > > > You have to draw the line somewhere which information the driver has
> > > > > hard-coded and what is only provided by the device tree and just assumed
> > > > > to be correct by the driver. You argue the driver should know that
> > > >
> > > > No, in this thread I argue that DT validation tool, executed by
> > > >
> > > > make ARCH=arm64 dtbs_check
> > > >
> > > > should catch that. This is not a driver, but DT binding described in YAML.
> > >
> > > The argumentation is the same. dtbs_check doesn't notice if the base
> > > address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
> > > it catch a missing reset controller phandle?
> >
> > Of course checking actual values of node properties doesn't make sense in
> > dtbs_check, otherwise we would have million DT bindings. If you have 10 copies
> > of the same IP core, of course you would use same compatible, but actual
> > register ranges, interrupts, etc. would be different in DT nodes.
> >
> > At this point I would make same argument as were made before, but there is no
> > point going in circles. I'm interested what have DT maintainers to say.
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de.
Maxime Ripard July 30, 2019, 5:06 p.m. UTC | #13
On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote:
> Hello Rob and Frank,
>
> Maxime and Jernej on one side and me on the other cannot agree about a
> detail in the change to the bindings here. I'm trying to objectively
> summarize the situation for you to help deciding what is the right thing
> to do here.
>
> TLDR: The sun4i pwm driver is extended to support a new variant of that
> device on the H6 SoC. Compared to the earlier supported variants
> allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> additional clock.
>
> The two positions are:
>
>  - We need a new compatible because only then the driver and/or the dt
>    schema checker can check that each "allwinner,sun50i-h6-pwm" device
>    has a reset property and a "bus" clock; and the earlier variants
>    don't.

There is two topics here, really. The binding itself really must have
those properties as required.

You had an analogy before that we shouldn't really do that, since it
would be verification and that it would be similar to checking whether
the register range was right. This analogy isn't correct, a better one
would be checking that the register range exists in the first place.

Indeed, if you don't have a register range, you have no register to
write to, and that's a showstopper for any driver. I'm pretty sure we
all agree on that. But on those SoCs, like Chen-Yu said, having no
resets or clocks properties result in an equally bad result: either
any write to that device is completely ignored (missing reset), or the
system completely (and silently) locks up (missing bus clock).

We *have* to catch that somehow and not let anything like that happen.

That being said, we can't really validate that the clock pointed is
the right one, just like we can't really check that the register range
is the proper one. Or rather, we could, but it's completely
impractical and we do agree on that as well.

Having the bus clock and reset line being required however is only a
few lines in the binding though, and is very practical.

>  - The driver can be simpler and the device specific knowledge is only
>    in a single place (the dt) if the device tree is considered valid and
>    a reset property and "bus" clock is used iff it's provided in the
>    device tree without additional comparison for the compatible.

And now we have the discussion on how it's implemented in a
driver. Since it's pretty cheap to implement (only a couple of lines:
two for the if block, one for the additional field in the structure,
one for each SoC using that) and have huge benefits (not silently
locking up the system at boot), then I'd say we should go for it.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Uwe Kleine-König July 31, 2019, 6:52 a.m. UTC | #14
On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote:
> On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote:
> > Hello Rob and Frank,
> >
> > Maxime and Jernej on one side and me on the other cannot agree about a
> > detail in the change to the bindings here. I'm trying to objectively
> > summarize the situation for you to help deciding what is the right thing
> > to do here.
> >
> > TLDR: The sun4i pwm driver is extended to support a new variant of that
> > device on the H6 SoC. Compared to the earlier supported variants
> > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> > additional clock.
> >
> > The two positions are:
> >
> >  - We need a new compatible because only then the driver and/or the dt
> >    schema checker can check that each "allwinner,sun50i-h6-pwm" device
> >    has a reset property and a "bus" clock; and the earlier variants
> >    don't.
> 
> There is two topics here, really. The binding itself really must have
> those properties as required.
> 
> You had an analogy before that we shouldn't really do that, since it
> would be verification and that it would be similar to checking whether
> the register range was right. This analogy isn't correct, a better one
> would be checking that the register range exists in the first place.

The relevant difference is that *all* devices supported by the driver
have to have a register range. Compared to that only a subset of the
devices have to have a bus clock.

> Indeed, if you don't have a register range, you have no register to
> write to, and that's a showstopper for any driver. I'm pretty sure we
> all agree on that. But on those SoCs, like Chen-Yu said, having no
> resets or clocks properties result in an equally bad result: either
> any write to that device is completely ignored (missing reset), or the
> system completely (and silently) locks up (missing bus clock).
> 
> We *have* to catch that somehow and not let anything like that happen.

IIUC both the clock and the reset stuff is SoC specific, so it's the
same for all machines with the H6, right? So assuming this is correctly
contained in the h6.dtsi, in which cases can this go wrong? I only see
the cases that the dts author includes the wrong dtsi or overrides
stuff. In the first case a non-working PWM is probably one of the
smaller problems and the second is something we're not really able to
catch.

But even if each machine's dts author has to get this right, I don't
think the dts schema is the right place to assert this.

> That being said, we can't really validate that the clock pointed is
> the right one, just like we can't really check that the register range
> is the proper one. Or rather, we could, but it's completely
> impractical and we do agree on that as well.
> 
> Having the bus clock and reset line being required however is only a
> few lines in the binding though, and is very practical.
> 
> >  - The driver can be simpler and the device specific knowledge is only
> >    in a single place (the dt) if the device tree is considered valid and
> >    a reset property and "bus" clock is used iff it's provided in the
> >    device tree without additional comparison for the compatible.
> 
> And now we have the discussion on how it's implemented in a
> driver. Since it's pretty cheap to implement (only a couple of lines:
> two for the if block, one for the additional field in the structure,
> one for each SoC using that) and have huge benefits (not silently
> locking up the system at boot), then I'd say we should go for it.

Right, it's only a few lines. Still it (IMHO needlessly) complicates the
driver. From the driver's POV the device tree defines the
characteristics of the device and if the dts defines an h6-pwm without a
bus clock then maybe this is the PWM on the next generation SoC that
doesn't need it. And maybe you're happy in a few year's time when you
don't have to touch the driver again for this next generation SoC
because the driver is not only simpler but also flexible enough to
handle the new PWM without adaptions.

All in all I don't care much about the dt schema stuff, I want to keep
the driver simple. So if we agree that the schema ensures that the h6
pwms have a reset and a bus clock and we just use reset_get_optional and
clk_get_optional that's a compromise I can agree to.

Best regards
Uwe
Maxime Ripard Aug. 12, 2019, 9:56 a.m. UTC | #15
On Wed, Jul 31, 2019 at 08:52:30AM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote:
> > On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote:
> > > Hello Rob and Frank,
> > >
> > > Maxime and Jernej on one side and me on the other cannot agree about a
> > > detail in the change to the bindings here. I'm trying to objectively
> > > summarize the situation for you to help deciding what is the right thing
> > > to do here.
> > >
> > > TLDR: The sun4i pwm driver is extended to support a new variant of that
> > > device on the H6 SoC. Compared to the earlier supported variants
> > > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> > > additional clock.
> > >
> > > The two positions are:
> > >
> > >  - We need a new compatible because only then the driver and/or the dt
> > >    schema checker can check that each "allwinner,sun50i-h6-pwm" device
> > >    has a reset property and a "bus" clock; and the earlier variants
> > >    don't.
> >
> > There is two topics here, really. The binding itself really must have
> > those properties as required.
> >
> > You had an analogy before that we shouldn't really do that, since it
> > would be verification and that it would be similar to checking whether
> > the register range was right. This analogy isn't correct, a better one
> > would be checking that the register range exists in the first place.
>
> The relevant difference is that *all* devices supported by the driver
> have to have a register range. Compared to that only a subset of the
> devices have to have a bus clock.

That's true, but it still have nothing to do with validating its
presence vs its content. We never even mentionned the latter.

> > Indeed, if you don't have a register range, you have no register to
> > write to, and that's a showstopper for any driver. I'm pretty sure we
> > all agree on that. But on those SoCs, like Chen-Yu said, having no
> > resets or clocks properties result in an equally bad result: either
> > any write to that device is completely ignored (missing reset), or the
> > system completely (and silently) locks up (missing bus clock).
> >
> > We *have* to catch that somehow and not let anything like that happen.
>
> IIUC both the clock and the reset stuff is SoC specific, so it's the
> same for all machines with the H6, right?

Indeed

> So assuming this is correctly contained in the h6.dtsi, in which
> cases can this go wrong? I only see the cases that the dts author
> includes the wrong dtsi or overrides stuff.

The bootloader passed by the bootloader is not meant for Linux but for
another OS, the bootloader loaded a DT not meant for mainline but some
BSP that happen to have the same compatible, the user has applied a
work in progress patch to its DT, and then updates the kernel, the
user applied a poorly written overlay, etc...

We really shouldn't support those cases in the first place, but a
silent lockup of the system is the worst way to treat those errors.

> In the first case a non-working PWM is probably one of the smaller
> problems and the second is something we're not really able to catch.
>
> But even if each machine's dts author has to get this right, I don't
> think the dts schema is the right place to assert this.

We shouldn't assert this *only* in the schema, but if it's cheap and
it can catch some mistakes, then why not?

Worst case scenario, the DTSI will be correct all the time, and it
will never generate any error. Just like 90% of all the constraints in
the schemas.

> > That being said, we can't really validate that the clock pointed is
> > the right one, just like we can't really check that the register range
> > is the proper one. Or rather, we could, but it's completely
> > impractical and we do agree on that as well.
> >
> > Having the bus clock and reset line being required however is only a
> > few lines in the binding though, and is very practical.
> >
> > >  - The driver can be simpler and the device specific knowledge is only
> > >    in a single place (the dt) if the device tree is considered valid and
> > >    a reset property and "bus" clock is used iff it's provided in the
> > >    device tree without additional comparison for the compatible.
> >
> > And now we have the discussion on how it's implemented in a
> > driver. Since it's pretty cheap to implement (only a couple of lines:
> > two for the if block, one for the additional field in the structure,
> > one for each SoC using that) and have huge benefits (not silently
> > locking up the system at boot), then I'd say we should go for it.
>
> Right, it's only a few lines. Still it (IMHO needlessly) complicates the
> driver. From the driver's POV the device tree defines the
> characteristics of the device and if the dts defines an h6-pwm without a
> bus clock then maybe this is the PWM on the next generation SoC that
> doesn't need it. And maybe you're happy in a few year's time when you
> don't have to touch the driver again for this next generation SoC
> because the driver is not only simpler but also flexible enough to
> handle the new PWM without adaptions.

You've been doing SoC support for a while, how many times did this
truly happen to you, whithout a single change to the driver?

> All in all I don't care much about the dt schema stuff, I want to keep
> the driver simple. So if we agree that the schema ensures that the h6
> pwms have a reset and a bus clock and we just use reset_get_optional and
> clk_get_optional that's a compromise I can agree to.

Fine, let's do that then

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Uwe Kleine-König Aug. 12, 2019, 10:47 a.m. UTC | #16
Hello Maxime,

the idea of my mail was to summarize quickly the discussion for the dt
people to give their judgement to stop us circling in a discussion about
the always same points.

I suggest we stop the discussion here now and wait for a reply from them
instead.

Best regards
Uwe
Jernej Skrabec Aug. 12, 2019, 10:51 a.m. UTC | #17
Dne ponedeljek, 12. avgust 2019 ob 12:47:00 CEST je Uwe Kleine-König 
napisal(a):
> Hello Maxime,
> 
> the idea of my mail was to summarize quickly the discussion for the dt
> people to give their judgement to stop us circling in a discussion about
> the always same points.
> 
> I suggest we stop the discussion here now and wait for a reply from them
> instead.

Shouldn't we just go with compromise solution you suggested and Maxime 
accepted? I would like to send new version in time for 5.4.

Best regards,
Jernej

> 
> Best regards
> Uwe

Patch
diff mbox series

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 7d3ac3f2dc3f..9e0eca79ff88 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -331,6 +331,13 @@  static const struct sun4i_pwm_data sun4i_pwm_single_bypass = {
 	.npwm = 1,
 };
 
+static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
+	.has_bus_clock = true,
+	.has_prescaler_bypass = true,
+	.has_reset = true,
+	.npwm = 2,
+};
+
 static const struct of_device_id sun4i_pwm_dt_ids[] = {
 	{
 		.compatible = "allwinner,sun4i-a10-pwm",
@@ -347,6 +354,9 @@  static const struct of_device_id sun4i_pwm_dt_ids[] = {
 	}, {
 		.compatible = "allwinner,sun8i-h3-pwm",
 		.data = &sun4i_pwm_single_bypass,
+	}, {
+		.compatible = "allwinner,sun50i-h6-pwm",
+		.data = &sun50i_pwm_dual_bypass_clk_rst,
 	}, {
 		/* sentinel */
 	},