Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

2016-10-17 Thread Maxime Ripard 
Hi Icenowy,

On Sun, Oct 16, 2016 at 10:29:17PM +0800, Icenowy Zheng wrote:
> Hi,
> 
> 14.10.2016, 20:58, "Maxime Ripard" :
> > Hi,
> >
> > On Wed, Oct 12, 2016 at 04:10:03PM +0800, Icenowy Zheng wrote:
> >>  > On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng  
> >> wrote:
> >>  > > The PWM controller in A31 is different with other Allwinner SoCs, with
> >>  > > a control register per channel (in other SoCs the control register is
> >>  > > shared), and each channel are allocated 16 bytes of address (but only 
> >> 8
> >>  > > bytes are used.). The register map in one channel is just like a
> >>  > > single-channel A10 PWM controller, however, A31 have a different
> >>  > > prescaler table than other SoCs.
> >>  > >
> >>  > > In order to use the driver for all 4 channels, device nodes should be
> >>  > > created per channel.
> >>  >
> >>  > I think Maxime wants you to support the different register offsets
> >>  > in this driver, and have all 4 channels in the same device (node).
> >>
> >>  I think that will make the code much more complex... And in
> >>  hardware there may also be 4 controllers... as the register is
> >>  aligned at 0x10.
> >
> > You also have to think about it the other way around. This is exposed
> > everywhere as a single device. There may be some undocumented
> > registers hidden somewhere in the memory space of that device. How
> > would you deal with that without touching the device tree?
> 
> Is the reason only they're listed in the one chapter of user manual?

Well, yes, because it is one single device.

> >
> > Exposing this as a single device is the best solution both from the
> > philosophical point of view, but also from a maintainance aspect.
> 
> If we really do so, I will go back to the original patch (pwm-sun6i)
> and merge 4 channels.
> 
> No other PWM block of Allwinner devices uses 4 control registers, and it's
> a significant difference to make it a dedicated driver.
> 
> However, I still think we should have 4 nodes, since the 4 channels can work
> very dedicatedly, with different control register... This can be a reason to 
> see
> them as 4 dedicated controllers.
> 
> (And as PWM uses only oscXX, we cannot judge it according to the clock tree,
> and Occam's Razor will apply to think it's 4 A10-like PWM controller...)
> 
> If we just think it's because it's a whole part, why don't we combine ehci 
> and ohci
> to one driver? Just because we can reuse {e,o}hci-platform...
> 
> It's the same reason to see it as 4 controllers.

Duplicating code is usually not a good idea. In this case, this will
be easier for you to deal with the A31 PWM, but all the rest of the
code (how to enable, disable it, setup the rates, when to enable the
clocks, when to disable them) will be duplicated, even though part of
them are really non-trivial.

Have you looked at reg_field? It really allows you to deal quite
cleanly with those kinds of quirks.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

2016-10-14 Thread Maxime Ripard 
Hi,

On Wed, Oct 12, 2016 at 04:10:03PM +0800, Icenowy Zheng wrote:
> > On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng  wrote:
> > > The PWM controller in A31 is different with other Allwinner SoCs, with
> > > a control register per channel (in other SoCs the control register is
> > > shared), and each channel are allocated 16 bytes of address (but only 8
> > > bytes are used.). The register map in one channel is just like a
> > > single-channel A10 PWM controller, however, A31 have a different
> > > prescaler table than other SoCs.
> > > 
> > > In order to use the driver for all 4 channels, device nodes should be
> > > created per channel.
> > 
> > I think Maxime wants you to support the different register offsets
> > in this driver, and have all 4 channels in the same device (node).
> 
> I think that will make the code much more complex...  And in
> hardware there may also be 4 controllers... as the register is
> aligned at 0x10.

You also have to think about it the other way around. This is exposed
everywhere as a single device. There may be some undocumented
registers hidden somewhere in the memory space of that device. How
would you deal with that without touching the device tree?

Exposing this as a single device is the best solution both from the
philosophical point of view, but also from a maintainance aspect.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

2016-10-14 Thread Maxime Ripard 
On Wed, Oct 12, 2016 at 03:30:07PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng  wrote:
> > The PWM controller in A31 is different with other Allwinner SoCs, with a
> > control register per channel (in other SoCs the control register is
> > shared), and each channel are allocated 16 bytes of address (but only 8
> > bytes are used.). The register map in one channel is just like a
> > single-channel A10 PWM controller, however, A31 have a different
> > prescaler table than other SoCs.
> >
> > In order to use the driver for all 4 channels, device nodes should be
> > created per channel.
> 
> I think Maxime wants you to support the different register offsets
> in this driver, and have all 4 channels in the same device (node).

Indeed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

2016-10-12 Thread Icenowy Zheng 
Hi,

- 原件 -
> Hi,
> 
> On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng  wrote:
> > The PWM controller in A31 is different with other Allwinner SoCs, with
> > a control register per channel (in other SoCs the control register is
> > shared), and each channel are allocated 16 bytes of address (but only 8
> > bytes are used.). The register map in one channel is just like a
> > single-channel A10 PWM controller, however, A31 have a different
> > prescaler table than other SoCs.
> > 
> > In order to use the driver for all 4 channels, device nodes should be
> > created per channel.
> 
> I think Maxime wants you to support the different register offsets
> in this driver, and have all 4 channels in the same device (node).

I think that will make the code much more complex...
And in hardware there may also be 4 controllers... as the register is aligned 
at 0x10.

The error I made in my previous patch set is making
a dedicatded pwm-sun6i driver. So I reworked this.
(This also why I do not call the current patchset PATCH v2 -- it's a new 
patchset)

Icenowy Zheng

> 
> ChenYu
> 
> 
> > Signed-off-by: Icenowy Zheng 
> > ---
> > drivers/pwm/pwm-sun4i.c | 37 -
> > 1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 03a99a5..3e93bdf 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -46,7 +46,7 @@
> > 
> > #define BIT_CH(bit, chan)           ((bit) << ((chan) * PWMCH_OFFSET))
> > 
> > -static const u32 prescaler_table[] = {
> > +static const u32 prescaler_table_a10[] = {
> > 120,
> > 180,
> > 240,
> > @@ -65,10 +65,30 @@ static const u32 prescaler_table[] = {
> > 0, /* Actually 1 but tested separately */
> > };
> > 
> > +static const u32 prescaler_table_a31[] = {
> > +             1,
> > +             2,
> > +             4,
> > +             8,
> > +             16,
> > +             32,
> > +             64,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +             0,
> > +};
> > +
> > struct sun4i_pwm_data {
> > bool has_prescaler_bypass;
> > bool has_rdy;
> > unsigned int npwm;
> > +             const u32 *prescaler_table;
> > };
> > 
> > struct sun4i_pwm_chip {
> > @@ -100,6 +120,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > +             const u32 *prescaler_table = sun4i_pwm->data->prescaler_table;
> > u32 prd, dty, val, clk_gate;
> > u64 clk_rate, div = 0;
> > unsigned int prescaler = 0;
> > @@ -264,24 +285,35 @@ static const struct sun4i_pwm_data
> > sun4i_pwm_data_a10 = { .has_prescaler_bypass = false,
> > .has_rdy = false,
> > .npwm = 2,
> > +             .prescaler_table = prescaler_table_a10,
> > };
> > 
> > static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
> > .has_prescaler_bypass = true,
> > .has_rdy = true,
> > .npwm = 2,
> > +             .prescaler_table = prescaler_table_a10,
> > };
> > 
> > static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
> > .has_prescaler_bypass = true,
> > .has_rdy = true,
> > .npwm = 1,
> > +             .prescaler_table = prescaler_table_a10,
> > };
> > 
> > static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
> > .has_prescaler_bypass = true,
> > .has_rdy = true,
> > .npwm = 2,
> > +             .prescaler_table = prescaler_table_a10,
> > +};
> > +
> > +static const struct sun4i_pwm_data sun4i_pwm_data_a31 = {
> > +             .has_prescaler_bypass = false,
> > +             .has_rdy = true,
> > +             .npwm = 1,
> > +             .prescaler_table = prescaler_table_a31,
> > };
> > 
> > static const struct of_device_id sun4i_pwm_dt_ids[] = {
> > @@ -298,6 +330,9 @@ static const struct of_device_id
> > sun4i_pwm_dt_ids[] = { .compatible = "allwinner,sun7i-a20-pwm",
> > .data = _pwm_data_a20,
> > }, {
> > +                             .compatible = "allwinner,sun6i-a31-pwm",
> > +                             .data = _pwm_data_a31
> > +             }, {
> > /* sentinel */
> > },
> > };
> > --
> > 2.10.1
> > 
> > --
> > 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+unsubscr...@googlegroups.com. For more options, visit
> > https://groups.google.com/d/optout.
> 
> -- 
> 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+unsubscr...@googlegroups.com. For more options, visit
> https://groups.google.com/d/optout.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.

Re: [linux-sunxi] [PATCH 2/5] pwm: sun4i: Add support for PWM controller on sun6i SoCs

2016-10-12 Thread Chen-Yu Tsai 
Hi,

On Wed, Oct 12, 2016 at 12:20 PM, Icenowy Zheng  wrote:
> The PWM controller in A31 is different with other Allwinner SoCs, with a
> control register per channel (in other SoCs the control register is
> shared), and each channel are allocated 16 bytes of address (but only 8
> bytes are used.). The register map in one channel is just like a
> single-channel A10 PWM controller, however, A31 have a different
> prescaler table than other SoCs.
>
> In order to use the driver for all 4 channels, device nodes should be
> created per channel.

I think Maxime wants you to support the different register offsets
in this driver, and have all 4 channels in the same device (node).

ChenYu


> Signed-off-by: Icenowy Zheng 
> ---
>  drivers/pwm/pwm-sun4i.c | 37 -
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..3e93bdf 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -46,7 +46,7 @@
>
>  #define BIT_CH(bit, chan)  ((bit) << ((chan) * PWMCH_OFFSET))
>
> -static const u32 prescaler_table[] = {
> +static const u32 prescaler_table_a10[] = {
> 120,
> 180,
> 240,
> @@ -65,10 +65,30 @@ static const u32 prescaler_table[] = {
> 0, /* Actually 1 but tested separately */
>  };
>
> +static const u32 prescaler_table_a31[] = {
> +   1,
> +   2,
> +   4,
> +   8,
> +   16,
> +   32,
> +   64,
> +   0,
> +   0,
> +   0,
> +   0,
> +   0,
> +   0,
> +   0,
> +   0,
> +   0,
> +};
> +
>  struct sun4i_pwm_data {
> bool has_prescaler_bypass;
> bool has_rdy;
> unsigned int npwm;
> +   const u32 *prescaler_table;
>  };
>
>  struct sun4i_pwm_chip {
> @@ -100,6 +120,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct 
> pwm_device *pwm,
> int duty_ns, int period_ns)
>  {
> struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> +   const u32 *prescaler_table = sun4i_pwm->data->prescaler_table;
> u32 prd, dty, val, clk_gate;
> u64 clk_rate, div = 0;
> unsigned int prescaler = 0;
> @@ -264,24 +285,35 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a10 = 
> {
> .has_prescaler_bypass = false,
> .has_rdy = false,
> .npwm = 2,
> +   .prescaler_table = prescaler_table_a10,
>  };
>
>  static const struct sun4i_pwm_data sun4i_pwm_data_a10s = {
> .has_prescaler_bypass = true,
> .has_rdy = true,
> .npwm = 2,
> +   .prescaler_table = prescaler_table_a10,
>  };
>
>  static const struct sun4i_pwm_data sun4i_pwm_data_a13 = {
> .has_prescaler_bypass = true,
> .has_rdy = true,
> .npwm = 1,
> +   .prescaler_table = prescaler_table_a10,
>  };
>
>  static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
> .has_prescaler_bypass = true,
> .has_rdy = true,
> .npwm = 2,
> +   .prescaler_table = prescaler_table_a10,
> +};
> +
> +static const struct sun4i_pwm_data sun4i_pwm_data_a31 = {
> +   .has_prescaler_bypass = false,
> +   .has_rdy = true,
> +   .npwm = 1,
> +   .prescaler_table = prescaler_table_a31,
>  };
>
>  static const struct of_device_id sun4i_pwm_dt_ids[] = {
> @@ -298,6 +330,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
> .compatible = "allwinner,sun7i-a20-pwm",
> .data = _pwm_data_a20,
> }, {
> +   .compatible = "allwinner,sun6i-a31-pwm",
> +   .data = _pwm_data_a31
> +   }, {
> /* sentinel */
> },
>  };
> --
> 2.10.1
>
> --
> 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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.