[linux-sunxi] Re: [PATCH] regulator: axp20x: support AXP813 variant

2016-07-28 Thread Jean-Francois Moine 
On Thu, 28 Jul 2016 22:19:44 +0200
Maxime Ripard  wrote:

> >  Documentation/devicetree/bindings/mfd/axp20x.txt | 32 -
> >  drivers/mfd/axp20x-rsb.c |  1 +
> >  drivers/mfd/axp20x.c |  3 +
> >  drivers/regulator/axp20x-regulator.c | 82 
> > +++-
> >  include/linux/mfd/axp20x.h   | 38 +++
> >  5 files changed, 153 insertions(+), 3 deletions(-)
> 
> And this needs to be split per logical changes.

There is only one logical change: support the AXP813.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
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.

[linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

2016-07-28 Thread Ondřej Jirman 
On 28.7.2016 23:00, Maxime Ripard wrote:
> Hi Ondrej,
> 
> On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
>> Hi Maxime,
>>
>> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
>> to this.
> 
> You can find it in the clock maintainers tree:
> https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng
> 
>> On 26.7.2016 08:32, Maxime Ripard wrote:
>>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>>> If so, then yes, trying to switch to the 24MHz oscillator before
>>> applying the factors, and then switching back when the PLL is stable
>>> would be a nice solution.
>>>
>>> I just checked, and all the SoCs we've had so far have that
>>> possibility, so if it works, for now, I'd like to stick to that.
>>
>> It would need to be tested. U-boot does the change only once, while the
>> kernel would be doing it all the time and between various frequencies
>> and PLL settings. So the issues may show up with this solution too.
>
> That would have the benefit of being quite easy to document, not be a
> huge amount of code and it would work on all the CPUs PLLs we have so
> far, so still, a pretty big win. If it doesn't, of course, we don't
> really have the choice.

 It's probably more code though. It has to access different register from
 the one that is already defined in dts, which would add a lot of code
 and require dts changes. The original patch I sent is simpler than that.
>>>
>>> Why?
>>>
>>> You can use container_of to retrieve the parent structure of the clock
>>> notifier, and then you get a ccu_common structure pointer, with the
>>> CCU base address, the clock register, its lock, etc.
>>>
>>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
>>>
>>> I don't really get why anything should be changed in the DT, or why it
>>> would add a lot of code. Or maybe we're not talking about the same
>>> thing?
>>
>> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
>> it very liberally uses divider parameters, even in situations that are
>> out of spec compared to the current code in the kernel.
>>
>> In the current code and especially in the original vendor code, divider
>> parameters are used as last resort only. Presumably because, of the
>> inherent trouble in changing them, as I described to you in other email.
>>
>> The new ccu code uses dividers often and even at very high frequencies,
>> which goes against the spec.
>>
>> In the vendor code M is never anything else but 0, and P is used only
>> for frequencies below 288MHz, which matches the H3 datasheet, which says:
> 
> In the vendor code, P is never used either. All the boards we had so
> far don't go that low, so we cannot make any of these assumptions,
> especially since the vendor code has had the bad habit of doing
> something wrong and / or useless in the past.

P is used in the arisc firmware according to the spec for the lower
frequencies.

> However, this implementation is a new thing, and it was using the H3
> precisely because of its early stage of support to use it as a testbed
> for the more established.
> 
> If you feel like we should use a different formula to favour the
> multipliers over the dividers (or want to change the class of the CPU
> PLL to an NKM or something else, this is totally doable.

I think the original formula that's currently in the mainline kernel is
better and avoids fiddling with dividers too much.

>> "The P factor only use in the condition that PLL output less than 288
>> MHz."
> 
> And the datasheet also had some issues, either misleading or wrong
> comments in the past. Don't get me wrong, I'm not saying that this is
> wrong, just that we should not follow it religiously, and that we
> should trust more the experiments than the datasheet.

I can believe that. :) Regardless, I think the reasons given for
avoiding dividers are quite reasonable. It's based on how PLL block
works, not what manual says.

>> Also other datasheets of similar socs from Allwinner state that M should
>> not be used in production code.
> 
> Which ones specifically?

A83T for example. You can search for "only for test" phrase.

https://github.com/allwinner-zh/documents/blob/master/A83T/A83T_User_Manual_v1.5.1_20150513.pdf

Those PLLs are a bit different though.


regards,
  Ondrej

>> So it may be that they either forgot to state it in the H3
>> datasheet, or it can be used. In any case, they never use M in their
>> code, so it may be wise to keep to that.
>>
>> When I boot with the new CCU code I get this:
>>
>> PLL_CPUX = 0x1B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
>> PLL_CPUX : freq = 1008MHz
>>
>> Mathematically it works, but it is against the spec.
>>
>> Also, this:
>>
>> analyzing CPU 0:
>>   driver: cpufreq-dt
>>   CPUs which run at the same hardware frequency: 0 1 2 3
>>   CPUs which need to have their frequency coordinated by software: 0 1

[linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

2016-07-28 Thread Maxime Ripard 
Hi Ondrej,

On Thu, Jul 28, 2016 at 01:27:05PM +0200, Ondřej Jirman wrote:
> Hi Maxime,
> 
> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
> to this.

You can find it in the clock maintainers tree:
https://git.kernel.org/cgit/linux/kernel/git/clk/linux.git/log/?h=clk-sunxi-ng

> On 26.7.2016 08:32, Maxime Ripard wrote:
> > On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> > If so, then yes, trying to switch to the 24MHz oscillator before
> > applying the factors, and then switching back when the PLL is stable
> > would be a nice solution.
> >
> > I just checked, and all the SoCs we've had so far have that
> > possibility, so if it works, for now, I'd like to stick to that.
> 
>  It would need to be tested. U-boot does the change only once, while the
>  kernel would be doing it all the time and between various frequencies
>  and PLL settings. So the issues may show up with this solution too.
> >>>
> >>> That would have the benefit of being quite easy to document, not be a
> >>> huge amount of code and it would work on all the CPUs PLLs we have so
> >>> far, so still, a pretty big win. If it doesn't, of course, we don't
> >>> really have the choice.
> >>
> >> It's probably more code though. It has to access different register from
> >> the one that is already defined in dts, which would add a lot of code
> >> and require dts changes. The original patch I sent is simpler than that.
> > 
> > Why?
> > 
> > You can use container_of to retrieve the parent structure of the clock
> > notifier, and then you get a ccu_common structure pointer, with the
> > CCU base address, the clock register, its lock, etc.
> > 
> > Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> > 
> > I don't really get why anything should be changed in the DT, or why it
> > would add a lot of code. Or maybe we're not talking about the same
> > thing?
> 
> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
> it very liberally uses divider parameters, even in situations that are
> out of spec compared to the current code in the kernel.
> 
> In the current code and especially in the original vendor code, divider
> parameters are used as last resort only. Presumably because, of the
> inherent trouble in changing them, as I described to you in other email.
> 
> The new ccu code uses dividers often and even at very high frequencies,
> which goes against the spec.
> 
> In the vendor code M is never anything else but 0, and P is used only
> for frequencies below 288MHz, which matches the H3 datasheet, which says:

In the vendor code, P is never used either. All the boards we had so
far don't go that low, so we cannot make any of these assumptions,
especially since the vendor code has had the bad habit of doing
something wrong and / or useless in the past.

However, this implementation is a new thing, and it was using the H3
precisely because of its early stage of support to use it as a testbed
for the more established.

If you feel like we should use a different formula to favour the
multipliers over the dividers (or want to change the class of the CPU
PLL to an NKM or something else, this is totally doable.

> "The P factor only use in the condition that PLL output less than 288
> MHz."

And the datasheet also had some issues, either misleading or wrong
comments in the past. Don't get me wrong, I'm not saying that this is
wrong, just that we should not follow it religiously, and that we
should trust more the experiments than the datasheet.

> Also other datasheets of similar socs from Allwinner state that M should
> not be used in production code.

Which ones specifically?

> So it may be that they either forgot to state it in the H3
> datasheet, or it can be used. In any case, they never use M in their
> code, so it may be wise to keep to that.
> 
> When I boot with the new CCU code I get this:
> 
> PLL_CPUX = 0x1B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
> PLL_CPUX : freq = 1008MHz
> 
> Mathematically it works, but it is against the spec.
> 
> Also, this:
> 
> analyzing CPU 0:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1 2 3
>   CPUs which need to have their frequency coordinated by software: 0 1 2 3
>   maximum transition latency: 1.74 ms
>   hardware limits: 120 MHz - 1.37 GHz
>   available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
>   available cpufreq governors: conservative ondemand userspace powersave
> performance
>   current policy: frequency should be within 240 MHz and 240 MHz.
>   The governor "performance" may decide which speed to use
>   within this range.
>   current CPU frequency: 24.0 MHz (asserted by call to hardware)
> 
> 
> Somehow, the new CCU code sets the CPUX to 24MHz no matter what.
> 
> I'm using your

Re: [linux-sunxi] [PATCH] regulator: axp20x: support AXP813 variant

2016-07-28 Thread Maxime Ripard 
On Thu, Jul 28, 2016 at 08:24:41PM +1000, Julian Calaby wrote:
> Hi All,
> 
> On Thu, Jul 28, 2016 at 5:28 PM, Jean-Francois Moine  wrote:
> > The X-Powers AXP813 PMIC is close enough to the AXP809 with some
> > more outputs.
> >
> > Signed-off-by: Jean-Francois Moine 
> > ---
> >  Documentation/devicetree/bindings/mfd/axp20x.txt | 32 -
> >  drivers/mfd/axp20x-rsb.c |  1 +
> >  drivers/mfd/axp20x.c |  3 +
> >  drivers/regulator/axp20x-regulator.c | 82 
> > +++-
> >  include/linux/mfd/axp20x.h   | 38 +++
> >  5 files changed, 153 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/regulator/axp20x-regulator.c 
> > b/drivers/regulator/axp20x-regulator.c
> > index 6d9ac76..c3287c9 100644
> > --- a/drivers/regulator/axp20x-regulator.c
> > +++ b/drivers/regulator/axp20x-regulator.c
> > @@ -467,7 +543,8 @@ static int axp20x_regulator_probe(struct 
> > platform_device *pdev)
> >  * name.
> >  */
> > if ((regulators == axp22x_regulators && i == AXP22X_DC1SW) 
> > ||
> > -   (regulators == axp809_regulators && i == AXP809_DC1SW)) 
> > {
> > +   (regulators == axp809_regulators && i == AXP809_DC1SW) 
> > ||
> > +   (regulators == axp813_regulators && i == AXP813_DC1SW)) 
> > {
> 
> Are we getting to the point now where we need some other way to flag
> these? Maybe a set of flags per regulator?

I'd be inclined to say yes. We can also associate a structure to the
compatible, and store a bunch of infos there (like does it have a
DC1SW regulator in this case).

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

[linux-sunxi] Re: [PATCH] regulator: axp20x: support AXP813 variant

2016-07-28 Thread Maxime Ripard 
Hi,

This patch must be sent to the MFD, regulators and DT maintainers, and
the relevant mailing lists.

On Thu, Jul 28, 2016 at 09:28:14AM +0200, Jean-Francois Moine wrote:
> The X-Powers AXP813 PMIC is close enough to the AXP809 with some
> more outputs.
> 
> Signed-off-by: Jean-Francois Moine 
> ---
>  Documentation/devicetree/bindings/mfd/axp20x.txt | 32 -
>  drivers/mfd/axp20x-rsb.c |  1 +
>  drivers/mfd/axp20x.c |  3 +
>  drivers/regulator/axp20x-regulator.c | 82 
> +++-
>  include/linux/mfd/axp20x.h   | 38 +++
>  5 files changed, 153 insertions(+), 3 deletions(-)

And this needs to be split per logical changes.

> diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt 
> b/Documentation/devicetree/bindings/mfd/axp20x.txt
> index 585a955..2a8ec61 100644
> --- a/Documentation/devicetree/bindings/mfd/axp20x.txt
> +++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
> @@ -7,10 +7,12 @@ axp209 (X-Powers)
>  axp221 (X-Powers)
>  axp223 (X-Powers)
>  axp809 (X-Powers)
> +axp813 (X-Powers)
>  
>  Required properties:
>  - compatible: "x-powers,axp152", "x-powers,axp202", "x-powers,axp209",
> -   "x-powers,axp221", "x-powers,axp223", "x-powers,axp809"
> +   "x-powers,axp221", "x-powers,axp223", "x-powers,axp809",
> +   "x-powers,axp813"
>  - reg: The I2C slave address or RSB hardware address for the AXP chip
>  - interrupt-parent: The parent interrupt controller
>  - interrupts: SoC NMI / GPIO interrupt connected to the PMIC's IRQ pin
> @@ -110,6 +112,34 @@ LDO_IO1  : LDO   : ips-supply
> : GPIO 1
>  RTC_LDO  : LDO   : ips-supply: always on
>  SW   : On/Off Switch : swin-supply
>  
> +AXP813 regulators, type, and corresponding input supply names:
> +
> +Regulator  TypeSupply Name Notes
> +-  --- -
> +DCDC1: DC-DC buck: vin1-supply
> +DCDC2: DC-DC buck: vin2-supply
> +DCDC3: DC-DC buck: vin3-supply
> +DCDC4: DC-DC buck: vin4-supply
> +DCDC5: DC-DC buck: vin5-supply
> +DCDC6: DC-DC buck: vin6-supply
> +DCDC7: DC-DC buck: vin7-supply
> +RTC_LDO  : LDO   : ips-supply: always on
> +ALDO1: LDO   : aldoin-supply : shared supply
> +ALDO2: LDO   : aldoin-supply : shared supply
> +ALDO3: LDO   : aldoin-supply : shared supply
> +DLDO1: LDO   : dldoin-supply : shared supply
> +DLDO2: LDO   : dldoin-supply : shared supply
> +DLDO3: LDO   : dldoin-supply : shared supply
> +DLDO4: LDO   : dldoin-supply : shared supply
> +ELDO1: LDO   : eldoin-supply : shared supply
> +ELDO2: LDO   : eldoin-supply : shared supply
> +ELDO3: LDO   : eldoin-supply : shared supply
> +FLDO1: LDO   : fldoin-supply : shared supply
> +FLDO2: LDO   : fldoin-supply : shared supply
> +LDO_IO0  : LDO   : ips-supply: GPIO 0
> +LDO_IO1  : LDO   : ips-supply: GPIO 1
> +DC1SW: On/Off Switch :   : DCDC1 
> secondary output
> +
>  Example:
>  
>  axp209: pmic@34 {
> diff --git a/drivers/mfd/axp20x-rsb.c b/drivers/mfd/axp20x-rsb.c
> index a407527..e34643b 100644
> --- a/drivers/mfd/axp20x-rsb.c
> +++ b/drivers/mfd/axp20x-rsb.c
> @@ -62,6 +62,7 @@ static int axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
>  static const struct of_device_id axp20x_rsb_of_match[] = {
>   { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
>   { .compatible = "x-powers,axp809", .data = (void *)AXP809_ID },
> + { .compatible = "x-powers,axp813", .data = (void *)AXP813_ID },
>   { },
>  };
>  MODULE_DEVICE_TABLE(of, axp20x_rsb_of_match);
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index fd80b09..e62d56f 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
>   "AXP223",
>   "AXP288",
>   "AXP809",
> + "AXP813",
>  };
>  
>  static const struct regmap_range axp152_writeable_ranges[] = {
> @@ -494,6 +495,7 @@ static const struct regmap_irq_chip 
> axp288_regmap_irq_chip = {
>  
>  };
>  
> +/* common 803/809/813 */
>  static const struct regmap_irq_chip axp809_regmap_irq_chip = {
>   .name   = "axp809",
>   .status_base= AXP20X_IRQ1_STATE,
> @@

[linux-sunxi] Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

2016-07-28 Thread Maxime Ripard 
On Thu, Jul 28, 2016 at 03:40:31PM +0200, LABBE Corentin wrote:
> On Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote:
> > > This patch adds documentation for Device-Tree bindings for the
> > > Allwinner sun8i-emac driver.
> > > 
> > > Signed-off-by: LABBE Corentin 
> > > ---
> > >  .../bindings/net/allwinner,sun8i-emac.txt  | 65 
> > > ++
> > >  1 file changed, 65 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> > > b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > new file mode 100644
> > > index 000..4bf4e53
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > @@ -0,0 +1,65 @@
> > > +* Allwinner sun8i EMAC ethernet controller
> > > +
> > > +Required properties:
> > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac",
> > > + or "allwinner,sun50i-a64-emac"
> > > +- reg: address and length of the register sets for the device.
> > > +- reg-names: should be "emac" and "syscon", matching the register sets
> > 
> > Blindly mapping a register of some other device on the SoC doesn't
> > look very reasonable.
> 
> As we discuss after this mail on IRC, this register is dedicated to EMAC.

I don't think we did. It's still right in the middle of some other
hardware block register space. You actually have a syscon driver to do
just that, why not use it?

> > > +See ethernet.txt in the same directory for generic bindings for ethernet
> > > +controllers.
> > > +
> > > +The device node referenced by "phy" or "phy-handle" should be a child 
> > > node
> > > +of this node. See phy.txt for the generic PHY bindings.
> > > +
> > > +Optional properties:
> > > +- phy-supply: phandle to a regulator if the PHY needs one
> > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one 
> > > for I/O.
> > > +  This is sometimes found with RGMII PHYs, which use a second
> > > +  regulator for the lower I/O voltage.
> > > +- allwinner,tx-delay: The setting of the TX clock delay chain
> > > +- allwinner,rx-delay: The setting of the RX clock delay chain
> > 
> > In which unit? What is the default value?
> 
> The unit is unknown to me, but I have added a comment for the
> default and acceptable range value.

That's unfortunate. We'll see how the DT maintainers feel about that.

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

[linux-sunxi] Re: [PATCH 4/9] clk: sunxi-ng: mux: Add support for mux tables

2016-07-28 Thread Jean-Francois Moine 
On Thu, 28 Jul 2016 15:28:42 +0200
Maxime Ripard  wrote:

> On Wed, Jul 27, 2016 at 10:36:49AM +0200, Jean-Francois Moine wrote:
> > On Wed, 27 Jul 2016 09:40:20 +0200
> > Maxime Ripard  wrote:
> > 
> > > > > Parenting functions would also not work as expected,
> > > > > clk_hw_get_parent_by_index being the obvious example, in that case
> > > > > returning the empty string for an invalid parent, while it should
> > > > > really return NULL.
> > > > 
> > > > I don't see why the clock should be orphan.
> > > > Then, when a parent is "", clk_hw_get_parent_by_index() returns NULL.
> > > 
> > > Why? It should return NULL when there's no parent, while you
> > > explicitly registered a parent.
> > 
> > "" is not an existing parent. It could be "none" / "dum" / "toto" / ...
> > with the same result: 'this index cannot be used in mux'.
> 
> And the clock is marked as orphan, while it really isn't.

Sorry for I don't follow you.

A clock is orphan when it has no parent. In our case, there are many
possible parents and, at startup time, the hardware or the boot sets
the mux to point to a real parent, with an index out of the usused
values.
Yes, the clock may be orphan, as the other clocks, but just the time
this real parent becomes visible.

So, how could such a clock stay marked as orphan?

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
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.

[linux-sunxi] Re: [PATCH v4] ARM: dts: sun8i: Add dts file for Olimex A33-OLinuXino

2016-07-28 Thread Maxime Ripard 
On Wed, Jul 27, 2016 at 01:02:41PM +0300, stefan.mavrod...@gmail.com wrote:
> On Wednesday, July 27, 2016 8:21:46 AM EEST Maxime Ripard wrote:
> > On Wed, Jul 27, 2016 at 08:12:29AM +0300, stefan.mavrod...@gmail.com wrote:
> > > > > +_dcdc1 {
> > > > > + regulator-always-on;
> > > > > + regulator-min-microvolt = <330>;
> > > > > + regulator-max-microvolt = <330>;
> > > > > + regulator-name = "vcc-dsi";
> > > > > +};
> > > > 
> > > > What is it used for? Is it really necessary to keep it on at all time?
> > > 
> > > I think so.
> > > This is the supply for the MMC.
> > 
> > Then it's poorly named, and you should tie it to the MMC, and remove
> > the always-on if it's only used by the mmc. always-on is supposed to
> > be for regulators that shouldn't but turned off for the system to stay
> > running. Some MMC regulator doesn't fit that description.
>
> It's named upon the A33 power pin - "VCC-DSI".

Usually, it's based on the name of the output in the schematics,
precisely because, from one board to another, it might have different
usage.

> If I remove "always-on" the board still will work, since dcdc1 is tied to 
> mmc0. 
> vmmc-supply = <_dcdc1>;
> 
> We assume this voltage will be always present and there are some pullups that 
> are tied to it (on i2c0 and i2c1 bus).  In this case should I remove "always-
> on" from the regulator node?

No, you should just tie that regulator to the i2c buses as well.

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

[linux-sunxi] Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

2016-07-28 Thread LABBE Corentin 
On Mon, Jul 25, 2016 at 09:54:55PM +0200, Maxime Ripard wrote:
> On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
> > This patch add support for sun8i-emac ethernet MAC hardware.
> > It could be found in Allwinner H3/A83T/A64 SoCs.
> > 
> > It supports 10/100/1000 Mbit/s speed with half/full duplex.
> > It can use an internal PHY (MII 10/100) or an external PHY
> > via RGMII/RMII.
> > 
> > Signed-off-by: LABBE Corentin 
> > ---
> >  drivers/net/ethernet/allwinner/Kconfig  |   13 +
> >  drivers/net/ethernet/allwinner/Makefile |1 +
> >  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 
> > +++
> >  3 files changed, 2143 insertions(+)
> >  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c
> > 
> > diff --git a/drivers/net/ethernet/allwinner/Kconfig 
> > b/drivers/net/ethernet/allwinner/Kconfig
> > index 47da7e7..060569c 100644
> > --- a/drivers/net/ethernet/allwinner/Kconfig
> > +++ b/drivers/net/ethernet/allwinner/Kconfig
> > @@ -33,4 +33,17 @@ config SUN4I_EMAC
> >To compile this driver as a module, choose M here.  The module
> >will be called sun4i-emac.
> >  
> > +config SUN8I_EMAC
> > +   tristate "Allwinner sun8i EMAC support"
> > +   depends on ARCH_SUNXI || COMPILE_TEST
> > +   depends on OF
> > +   select MII
> > +   select PHYLIB
> > +---help---
> > + This driver support the sun8i EMAC ethernet driver present on
> > + H3/A83T/A64 Allwinner SoCs.
> > +
> > +  To compile this driver as a module, choose M here.  The module
> > +  will be called sun8i-emac.
> > +
> >  endif # NET_VENDOR_ALLWINNER
> > diff --git a/drivers/net/ethernet/allwinner/Makefile 
> > b/drivers/net/ethernet/allwinner/Makefile
> > index 03129f7..8bd1693c 100644
> > --- a/drivers/net/ethernet/allwinner/Makefile
> > +++ b/drivers/net/ethernet/allwinner/Makefile
> > @@ -3,3 +3,4 @@
> >  #
> >  
> >  obj-$(CONFIG_SUN4I_EMAC) += sun4i-emac.o
> > +obj-$(CONFIG_SUN8I_EMAC) += sun8i-emac.o
> > diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c 
> > b/drivers/net/ethernet/allwinner/sun8i-emac.c
> > new file mode 100644
> > index 000..fc0c1dd
> > --- /dev/null
> > +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
> > @@ -0,0 +1,2129 @@
> > +/*
> > + * sun8i-emac driver
> > + *
> > + * Copyright (C) 2015-2016 Corentin LABBE 
> > + *
> > + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC
> > + *
> > + * TODO:
> > + * - MAC filtering
> > + * - Jumbo frame
> > + * - features rx-all (NETIF_F_RXALL_BIT)
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define SUN8I_EMAC_BASIC_CTL0  0x00
> > +#define SUN8I_EMAC_BASIC_CTL1  0x04
> > +#define SUN8I_EMAC_INT_STA 0x08
> > +#define SUN8I_EMAC_INT_EN  0x0C
> > +#define SUN8I_EMAC_TX_CTL0 0x10
> > +#define SUN8I_EMAC_TX_CTL1 0x14
> > +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C
> > +#define SUN8I_EMAC_RX_CTL0 0x24
> > +#define SUN8I_EMAC_RX_CTL1 0x28
> > +#define SUN8I_EMAC_RX_FRM_FLT  0x38
> > +#define SUN8I_EMAC_MDIO_CMD0x48
> > +#define SUN8I_EMAC_MDIO_DATA   0x4C
> > +#define SUN8I_EMAC_TX_DMA_STA  0xB0
> > +#define SUN8I_EMAC_TX_CUR_DESC 0xB4
> > +#define SUN8I_EMAC_TX_CUR_BUF  0xB8
> > +#define SUN8I_EMAC_RX_DMA_STA  0xC0
> > +
> > +#define MDIO_CMD_MII_BUSY  BIT(0)
> > +#define MDIO_CMD_MII_WRITE BIT(1)
> > +#define MDIO_CMD_MII_PHY_REG_ADDR_MASK GENMASK(8, 4)
> > +#define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT4
> > +#define MDIO_CMD_MII_PHY_ADDR_MASK GENMASK(16, 12)
> > +#define MDIO_CMD_MII_PHY_ADDR_SHIFT12
> > +
> > +#define SUN8I_EMAC_MACADDR_HI  0x50
> > +#define SUN8I_EMAC_MACADDR_LO  0x54
> > +
> > +#define SUN8I_EMAC_RX_DESC_LIST 0x34
> > +#define SUN8I_EMAC_TX_DESC_LIST 0x20
> > +
> > +#define SUN8I_EMAC_RX_DO_CRC BIT(27)
> > +#define SUN8I_EMAC_RX_STRIP_FCS BIT(28)
> > +
> > +#define SUN8I_COULD_BE_USED_BY_DMA BIT(31)
> > +
> > +/* Used in RX_CTL1*/
> > +#define RX_DMA_EN  BIT(30)
> > +#define RX_DMA_START   BIT(31)
> > +/* Used in TX_CTL1*/
> > +#define TX_DMA_EN  BIT(30)
> > +#define TX_DMA_START   BIT(31)
> > +
> > +/* Used in RX_CTL0 */
> > +#define RX_RECEIVER_EN BIT(31)
> > +/* Used in TX_CTL0 */
> > +#define TX_TRANSMITTER_EN  BIT(31)
> > +
> > +/* Basic CTL0 */
> > +#define BCTL0_FD BIT(0)
> > +#define BCTL0_SPEED_10 2
> > +#define BCTL0_SPEED_1003
> > +#define BCTL0_SPEED_MASK   GENMASK(3, 2)
> > +#define BCTL0_SPEED_SHIFT  2
> > +
> > +#define FLOW_RX 1
> > +#define FLOW_TX 2
> > +
> > +#define RX_INT  BIT(8)
> > +#define TX_INT  BIT(0)
> > +
> > +/* Bits used in frame RX status */
> > +#define

[linux-sunxi] Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

2016-07-28 Thread LABBE Corentin 
On Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote:
> > This patch adds documentation for Device-Tree bindings for the
> > Allwinner sun8i-emac driver.
> > 
> > Signed-off-by: LABBE Corentin 
> > ---
> >  .../bindings/net/allwinner,sun8i-emac.txt  | 65 
> > ++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> > b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > new file mode 100644
> > index 000..4bf4e53
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > @@ -0,0 +1,65 @@
> > +* Allwinner sun8i EMAC ethernet controller
> > +
> > +Required properties:
> > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac",
> > +   or "allwinner,sun50i-a64-emac"
> > +- reg: address and length of the register sets for the device.
> > +- reg-names: should be "emac" and "syscon", matching the register sets
> 
> Blindly mapping a register of some other device on the SoC doesn't
> look very reasonable.
> 

As we discuss after this mail on IRC, this register is dedicated to EMAC.

> > +- interrupts: interrupt for the device
> > +- clocks: A phandle to the reference clock for this device
> > +- clock-names: should be "ahb"
> > +- resets: A phandle to the reset control for this device
> > +- reset-names: should be "ahb"
> > +- phy-mode: See ethernet.txt
> > +- phy or phy-handle: See ethernet.txt
> > +- #address-cells: shall be 1
> > +- #size-cells: shall be 0
> > +
> > +"allwinner,sun8i-h3-emac" also requires:
> > +- clocks: an extra phandle to the reference clock for the EPHY
> > +- clock-names: an extra "ephy" entry matching the clocks property
> > +- resets: an extra phandle to the reset control for the EPHY
> > +- resets-names: an extra "ephy" entry matching the resets property
> 
> Shouldn't that be attached to the phy itself?
> 

Ok I will move them.

> > +See ethernet.txt in the same directory for generic bindings for ethernet
> > +controllers.
> > +
> > +The device node referenced by "phy" or "phy-handle" should be a child node
> > +of this node. See phy.txt for the generic PHY bindings.
> > +
> > +Optional properties:
> > +- phy-supply: phandle to a regulator if the PHY needs one
> > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for 
> > I/O.
> > +This is sometimes found with RGMII PHYs, which use a second
> > +regulator for the lower I/O voltage.
> > +- allwinner,tx-delay: The setting of the TX clock delay chain
> > +- allwinner,rx-delay: The setting of the RX clock delay chain
> 
> In which unit? What is the default value?
> 

The unit is unknown to me, but I have added a comment for the default and 
acceptable range value.

> > +
> > +The TX/RX clock delay chain settings are board specific.
> > +
> > +Optional properties for "allwinner,sun8i-h3-emac":
> > +- allwinner,use-internal-phy: Use the H3 SoC's internal E(thernet) PHY
> 
> Can't that be derived from the presence of the phy property?
> 

Yes, I have reworked the "variant" of the driver for easily handling this.

> > +- allwinner,leds-active-low: EPHY LEDs are active low
> 
> That also seems PHY related. Overall, I feel like we really need a phy
> node for the internal phy.
> 

Moved also in the phy node.

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

Best regards

Thanks

LABBE Corentin

-- 
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.

[linux-sunxi] Re: [PATCH 8/9] clk: sunxi-ng: Add A31/A31s clocks

2016-07-28 Thread Maxime Ripard 
On Tue, Jul 26, 2016 at 03:04:30PM +0800, Chen-Yu Tsai wrote:
> Add a new style driver for the clock control unit in Allwinner A31/A31s.
> 
> A few clocks are still missing:
> 
> - EMAC clock
> 
> Signed-off-by: Chen-Yu Tsai 

Looks good to me, aside from the comment made by Jean-Francois.

I guess I'd prefer to have the SoC names in the define, just in case
we need to introduce support for another SoC, but this can always be
done later.

Thanks!
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

[linux-sunxi] Re: [PATCH 4/9] clk: sunxi-ng: mux: Add support for mux tables

2016-07-28 Thread Maxime Ripard 
On Wed, Jul 27, 2016 at 10:36:49AM +0200, Jean-Francois Moine wrote:
> On Wed, 27 Jul 2016 09:40:20 +0200
> Maxime Ripard  wrote:
> 
> > > > Parenting functions would also not work as expected,
> > > > clk_hw_get_parent_by_index being the obvious example, in that case
> > > > returning the empty string for an invalid parent, while it should
> > > > really return NULL.
> > > 
> > > I don't see why the clock should be orphan.
> > > Then, when a parent is "", clk_hw_get_parent_by_index() returns NULL.
> > 
> > Why? It should return NULL when there's no parent, while you
> > explicitly registered a parent.
> 
> "" is not an existing parent. It could be "none" / "dum" / "toto" / ...
> with the same result: 'this index cannot be used in mux'.

And the clock is marked as orphan, while it really isn't.

-- 
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

[linux-sunxi] Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

2016-07-28 Thread LABBE Corentin 
On Wed, Jul 20, 2016 at 02:15:33PM -0500, Rob Herring wrote:
> On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote:
> > This patch adds documentation for Device-Tree bindings for the
> > Allwinner sun8i-emac driver.
> > 
> > Signed-off-by: LABBE Corentin 
> > ---
> >  .../bindings/net/allwinner,sun8i-emac.txt  | 65 
> > ++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> > b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > new file mode 100644
> > index 000..4bf4e53
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > @@ -0,0 +1,65 @@
> > +* Allwinner sun8i EMAC ethernet controller
> > +
> > +Required properties:
> > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac",
> > +   or "allwinner,sun50i-a64-emac"
> 
> List one per line.
> 
ok

> > +- reg: address and length of the register sets for the device.
> > +- reg-names: should be "emac" and "syscon", matching the register sets
> > +- interrupts: interrupt for the device
> > +- clocks: A phandle to the reference clock for this device
> > +- clock-names: should be "ahb"
> > +- resets: A phandle to the reset control for this device
> > +- reset-names: should be "ahb"
> > +- phy-mode: See ethernet.txt
> > +- phy or phy-handle: See ethernet.txt
> > +- #address-cells: shall be 1
> > +- #size-cells: shall be 0
> > +
> > +"allwinner,sun8i-h3-emac" also requires:
> > +- clocks: an extra phandle to the reference clock for the EPHY
> > +- clock-names: an extra "ephy" entry matching the clocks property
> > +- resets: an extra phandle to the reset control for the EPHY
> > +- resets-names: an extra "ephy" entry matching the resets property
> > +
> > +See ethernet.txt in the same directory for generic bindings for ethernet
> > +controllers.
> > +
> > +The device node referenced by "phy" or "phy-handle" should be a child node
> > +of this node. See phy.txt for the generic PHY bindings.
> > +
> > +Optional properties:
> > +- phy-supply: phandle to a regulator if the PHY needs one
> > +- phy-io-supply: phandle to a regulator if the PHY needs a another one for 
> > I/O.
> > +This is sometimes found with RGMII PHYs, which use a second
> > +regulator for the lower I/O voltage.
> 
> I previously said these should go in the phy node, and you said you 
> would remove them.
> 
> Rob

Sorry, I forgot to remove them.
I have removed them now.

Regards

Thanks

LABBE Corentin

-- 
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.

[linux-sunxi] Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

2016-07-28 Thread LABBE Corentin 
On Wed, Jul 20, 2016 at 11:56:12AM +0200, Arnd Bergmann wrote:
> On Wednesday, July 20, 2016 10:03:16 AM CEST LABBE Corentin wrote:
> > +
> > +   /* Benched on OPIPC with 100M, setting more than 256 does not give 
> > any
> > +* perf boost
> > +*/
> > +   priv->nbdesc_rx = 128;
> > +   priv->nbdesc_tx = 256;
> > +
> > 
> 
> 256 tx descriptors can introduce a significant latency. Can you add
> support for BQL (netdev_sent_queue/netdev_completed_queue) to limit
> the queue size to the minimum?

Done, since setting below 256 give lower performance with iperf.

> 
> I also noticed that your tx_lock() prevents you from concurrently
> running sun8i_emac_complete_xmit() and sun8i_emac_xmit(). Is that
> necessary? I'd think that you can find a way to make them work
> concurrently.
> 
>   Arnd

I will reworked locking and it seems that no locking is necessary.
I have added the following comment about the locking strategy:

/* Locking strategy:
 * RX queue does not need any lock since only sun8i_emac_poll() access it.
 * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so 
sun8i_emac_poll())
 * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and 
sun8i_emac_tx_timeout()
 * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue)
 *
 * sun8i_emac_xmit() could fire only once (netif_tx_lock)
 * sun8i_emac_complete_xmit() could fire only once (called from NAPI)
 * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt
 * race with sun8i_emac_xmit (due to netif_tx_lock) and with 
sun8i_emac_complete_xmit which disable NAPI.
 *
 * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the same 
time.
 * But they never could modify the same descriptors:
 * - sun8i_emac_complete_xmit() will modify only descriptors with empty status
 * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN
 * Proper memory barriers ensure that descriptor set to DCLEAN could not be
 * modified latter by sun8i_emac_complete_xmit().
 * */

Does I am right ?

Thanks for your review.

Regards

LABBE Corentin

-- 
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.

Re: [linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

2016-07-28 Thread Chen-Yu Tsai 
Hi,

On Thu, Jul 28, 2016 at 7:27 PM, Ondřej Jirman  wrote:
> Hi Maxime,
>
> I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
> to this.
>
> On 26.7.2016 08:32, Maxime Ripard wrote:
>> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
>> If so, then yes, trying to switch to the 24MHz oscillator before
>> applying the factors, and then switching back when the PLL is stable
>> would be a nice solution.
>>
>> I just checked, and all the SoCs we've had so far have that
>> possibility, so if it works, for now, I'd like to stick to that.
>
> It would need to be tested. U-boot does the change only once, while the
> kernel would be doing it all the time and between various frequencies
> and PLL settings. So the issues may show up with this solution too.

 That would have the benefit of being quite easy to document, not be a
 huge amount of code and it would work on all the CPUs PLLs we have so
 far, so still, a pretty big win. If it doesn't, of course, we don't
 really have the choice.
>>>
>>> It's probably more code though. It has to access different register from
>>> the one that is already defined in dts, which would add a lot of code
>>> and require dts changes. The original patch I sent is simpler than that.
>>
>> Why?
>>
>> You can use container_of to retrieve the parent structure of the clock
>> notifier, and then you get a ccu_common structure pointer, with the
>> CCU base address, the clock register, its lock, etc.
>>
>> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
>>
>> I don't really get why anything should be changed in the DT, or why it
>> would add a lot of code. Or maybe we're not talking about the same
>> thing?
>
> I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
> it very liberally uses divider parameters, even in situations that are
> out of spec compared to the current code in the kernel.
>
> In the current code and especially in the original vendor code, divider
> parameters are used as last resort only. Presumably because, of the
> inherent trouble in changing them, as I described to you in other email.
>
> The new ccu code uses dividers often and even at very high frequencies,
> which goes against the spec.
>
> In the vendor code M is never anything else but 0, and P is used only
> for frequencies below 288MHz, which matches the H3 datasheet, which says:
>
> "The P factor only use in the condition that PLL output less than 288
> MHz."
>
> Also other datasheets of similar socs from Allwinner state that M should
> not be used in production code. So it may be that they either forgot to
> state it in the H3 datasheet, or it can be used. In any case, they never
> use M in their code, so it may be wise to keep to that.
>
> When I boot with the new CCU code I get this:
>
> PLL_CPUX = 0x1B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
> PLL_CPUX : freq = 1008MHz
>
> Mathematically it works, but it is against the spec.
>
> Also, this:
>
> analyzing CPU 0:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1 2 3
>   CPUs which need to have their frequency coordinated by software: 0 1 2 3
>   maximum transition latency: 1.74 ms
>   hardware limits: 120 MHz - 1.37 GHz
>   available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
> MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
> GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
>   available cpufreq governors: conservative ondemand userspace powersave
> performance
>   current policy: frequency should be within 240 MHz and 240 MHz.
>   The governor "performance" may decide which speed to use
>   within this range.
>   current CPU frequency: 24.0 MHz (asserted by call to hardware)
>
>
> Somehow, the new CCU code sets the CPUX to 24MHz no matter what.
>
> I'm using your pen/clk-rework branch without any other patches that were
> later sent to the mailing list.

The H3 CPUX clock does not have the CLK_SET_RATE_PARENT flag set, which
means cpufreq's attempts to set the clk rate will, in some cases, be
ignored as the change is not propagated up to the parent PLL. In the
worst case it will select the 24 MHz oscillator. The fix is to add
CLK_SET_RATE_PARENT. And then you'll have to deal with the PLL potentially
going too high, as you stated in your other mail, and can be mitigated
with my clk notifier patch.

ChenYu

> regards,
>   Ondrej
>
>>
>> Maxime
>>
>
> --
> 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

[linux-sunxi] Re: Changed: sunxi-ng clock code - NKMP clock implementation is wrong

2016-07-28 Thread Ondřej Jirman 
Hi Maxime,

I don't have your sunxi-ng clock patches in my mailbox, so I'm replying
to this.

On 26.7.2016 08:32, Maxime Ripard wrote:
> On Thu, Jul 21, 2016 at 11:52:15AM +0200, Ondřej Jirman wrote:
> If so, then yes, trying to switch to the 24MHz oscillator before
> applying the factors, and then switching back when the PLL is stable
> would be a nice solution.
>
> I just checked, and all the SoCs we've had so far have that
> possibility, so if it works, for now, I'd like to stick to that.

 It would need to be tested. U-boot does the change only once, while the
 kernel would be doing it all the time and between various frequencies
 and PLL settings. So the issues may show up with this solution too.
>>>
>>> That would have the benefit of being quite easy to document, not be a
>>> huge amount of code and it would work on all the CPUs PLLs we have so
>>> far, so still, a pretty big win. If it doesn't, of course, we don't
>>> really have the choice.
>>
>> It's probably more code though. It has to access different register from
>> the one that is already defined in dts, which would add a lot of code
>> and require dts changes. The original patch I sent is simpler than that.
> 
> Why?
> 
> You can use container_of to retrieve the parent structure of the clock
> notifier, and then you get a ccu_common structure pointer, with the
> CCU base address, the clock register, its lock, etc.
> 
> Look at what is done in drivers/clk/meson/clk-cpu.c. It's like 20 LoC.
> 
> I don't really get why anything should be changed in the DT, or why it
> would add a lot of code. Or maybe we're not talking about the same
> thing?

I've looked at the new CCU code, particularly ccu_nkmp.c, and found that
it very liberally uses divider parameters, even in situations that are
out of spec compared to the current code in the kernel.

In the current code and especially in the original vendor code, divider
parameters are used as last resort only. Presumably because, of the
inherent trouble in changing them, as I described to you in other email.

The new ccu code uses dividers often and even at very high frequencies,
which goes against the spec.

In the vendor code M is never anything else but 0, and P is used only
for frequencies below 288MHz, which matches the H3 datasheet, which says:

"The P factor only use in the condition that PLL output less than 288
MHz."

Also other datasheets of similar socs from Allwinner state that M should
not be used in production code. So it may be that they either forgot to
state it in the H3 datasheet, or it can be used. In any case, they never
use M in their code, so it may be wise to keep to that.

When I boot with the new CCU code I get this:

PLL_CPUX = 0x1B21 : M = 2, K = 3, N = 28, P = 1, EN = 0
PLL_CPUX : freq = 1008MHz

Mathematically it works, but it is against the spec.

Also, this:

analyzing CPU 0:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1 2 3
  CPUs which need to have their frequency coordinated by software: 0 1 2 3
  maximum transition latency: 1.74 ms
  hardware limits: 120 MHz - 1.37 GHz
  available frequency steps:  120 MHz, 240 MHz, 480 MHz, 648 MHz, 816
MHz, 960 MHz, 1.01 GHz, 1.06 GHz, 1.10 GHz, 1.15 GHz, 1.20 GHz, 1.22
GHz, 1.25 GHz, 1.30 GHz, 1.34 GHz, 1.37 GHz
  available cpufreq governors: conservative ondemand userspace powersave
performance
  current policy: frequency should be within 240 MHz and 240 MHz.
  The governor "performance" may decide which speed to use
  within this range.
  current CPU frequency: 24.0 MHz (asserted by call to hardware)


Somehow, the new CCU code sets the CPUX to 24MHz no matter what.

I'm using your pen/clk-rework branch without any other patches that were
later sent to the mailing list.

regards,
  Ondrej

> 
> Maxime
> 

-- 
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: OpenPGP digital signature

Re: [linux-sunxi] [PATCH] regulator: axp20x: support AXP813 variant

2016-07-28 Thread Julian Calaby 
Hi All,

On Thu, Jul 28, 2016 at 5:28 PM, Jean-Francois Moine  wrote:
> The X-Powers AXP813 PMIC is close enough to the AXP809 with some
> more outputs.
>
> Signed-off-by: Jean-Francois Moine 
> ---
>  Documentation/devicetree/bindings/mfd/axp20x.txt | 32 -
>  drivers/mfd/axp20x-rsb.c |  1 +
>  drivers/mfd/axp20x.c |  3 +
>  drivers/regulator/axp20x-regulator.c | 82 
> +++-
>  include/linux/mfd/axp20x.h   | 38 +++
>  5 files changed, 153 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c 
> b/drivers/regulator/axp20x-regulator.c
> index 6d9ac76..c3287c9 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -467,7 +543,8 @@ static int axp20x_regulator_probe(struct platform_device 
> *pdev)
>  * name.
>  */
> if ((regulators == axp22x_regulators && i == AXP22X_DC1SW) ||
> -   (regulators == axp809_regulators && i == AXP809_DC1SW)) {
> +   (regulators == axp809_regulators && i == AXP809_DC1SW) ||
> +   (regulators == axp813_regulators && i == AXP813_DC1SW)) {

Are we getting to the point now where we need some other way to flag
these? Maybe a set of flags per regulator?

> new_desc = devm_kzalloc(>dev, sizeof(*desc),
> GFP_KERNEL);
> *new_desc = regulators[i];
> @@ -505,7 +582,8 @@ static int axp20x_regulator_probe(struct platform_device 
> *pdev)
>  * Save AXP22X DCDC1 / DCDC5 regulator names for later.
>  */
> if ((regulators == axp22x_regulators && i == AXP22X_DCDC1) ||
> -   (regulators == axp809_regulators && i == AXP809_DCDC1))
> +   (regulators == axp809_regulators && i == AXP809_DCDC1) ||
> +   (regulators == axp813_regulators && i == AXP813_DCDC1))

Ditto.

> of_property_read_string(rdev->dev.of_node,
> "regulator-name",
> _name);

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

-- 
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.

[linux-sunxi] [PATCH] regulator: axp20x: support AXP813 variant

2016-07-28 Thread Jean-Francois Moine 
The X-Powers AXP813 PMIC is close enough to the AXP809 with some
more outputs.

Signed-off-by: Jean-Francois Moine 
---
 Documentation/devicetree/bindings/mfd/axp20x.txt | 32 -
 drivers/mfd/axp20x-rsb.c |  1 +
 drivers/mfd/axp20x.c |  3 +
 drivers/regulator/axp20x-regulator.c | 82 +++-
 include/linux/mfd/axp20x.h   | 38 +++
 5 files changed, 153 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt 
b/Documentation/devicetree/bindings/mfd/axp20x.txt
index 585a955..2a8ec61 100644
--- a/Documentation/devicetree/bindings/mfd/axp20x.txt
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -7,10 +7,12 @@ axp209 (X-Powers)
 axp221 (X-Powers)
 axp223 (X-Powers)
 axp809 (X-Powers)
+axp813 (X-Powers)
 
 Required properties:
 - compatible: "x-powers,axp152", "x-powers,axp202", "x-powers,axp209",
- "x-powers,axp221", "x-powers,axp223", "x-powers,axp809"
+ "x-powers,axp221", "x-powers,axp223", "x-powers,axp809",
+ "x-powers,axp813"
 - reg: The I2C slave address or RSB hardware address for the AXP chip
 - interrupt-parent: The parent interrupt controller
 - interrupts: SoC NMI / GPIO interrupt connected to the PMIC's IRQ pin
@@ -110,6 +112,34 @@ LDO_IO1: LDO   : ips-supply
: GPIO 1
 RTC_LDO: LDO   : ips-supply: always on
 SW : On/Off Switch : swin-supply
 
+AXP813 regulators, type, and corresponding input supply names:
+
+RegulatorTypeSupply Name Notes
+---- -
+DCDC1  : DC-DC buck: vin1-supply
+DCDC2  : DC-DC buck: vin2-supply
+DCDC3  : DC-DC buck: vin3-supply
+DCDC4  : DC-DC buck: vin4-supply
+DCDC5  : DC-DC buck: vin5-supply
+DCDC6  : DC-DC buck: vin6-supply
+DCDC7  : DC-DC buck: vin7-supply
+RTC_LDO: LDO   : ips-supply: always on
+ALDO1  : LDO   : aldoin-supply : shared supply
+ALDO2  : LDO   : aldoin-supply : shared supply
+ALDO3  : LDO   : aldoin-supply : shared supply
+DLDO1  : LDO   : dldoin-supply : shared supply
+DLDO2  : LDO   : dldoin-supply : shared supply
+DLDO3  : LDO   : dldoin-supply : shared supply
+DLDO4  : LDO   : dldoin-supply : shared supply
+ELDO1  : LDO   : eldoin-supply : shared supply
+ELDO2  : LDO   : eldoin-supply : shared supply
+ELDO3  : LDO   : eldoin-supply : shared supply
+FLDO1  : LDO   : fldoin-supply : shared supply
+FLDO2  : LDO   : fldoin-supply : shared supply
+LDO_IO0: LDO   : ips-supply: GPIO 0
+LDO_IO1: LDO   : ips-supply: GPIO 1
+DC1SW  : On/Off Switch :   : DCDC1 secondary output
+
 Example:
 
 axp209: pmic@34 {
diff --git a/drivers/mfd/axp20x-rsb.c b/drivers/mfd/axp20x-rsb.c
index a407527..e34643b 100644
--- a/drivers/mfd/axp20x-rsb.c
+++ b/drivers/mfd/axp20x-rsb.c
@@ -62,6 +62,7 @@ static int axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
 static const struct of_device_id axp20x_rsb_of_match[] = {
{ .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
{ .compatible = "x-powers,axp809", .data = (void *)AXP809_ID },
+   { .compatible = "x-powers,axp813", .data = (void *)AXP813_ID },
{ },
 };
 MODULE_DEVICE_TABLE(of, axp20x_rsb_of_match);
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index fd80b09..e62d56f 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
"AXP223",
"AXP288",
"AXP809",
+   "AXP813",
 };
 
 static const struct regmap_range axp152_writeable_ranges[] = {
@@ -494,6 +495,7 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip 
= {
 
 };
 
+/* common 803/809/813 */
 static const struct regmap_irq_chip axp809_regmap_irq_chip = {
.name   = "axp809",
.status_base= AXP20X_IRQ1_STATE,
@@ -733,6 +735,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
axp20x->regmap_irq_chip = _regmap_irq_chip;
break;
case AXP809_ID:
+   case AXP813_ID:
axp20x->nr_cells = ARRAY_SIZE(axp809_cells);
axp20x->cells = axp809_cells;
axp20x->regmap_cfg = _regmap_config;
diff --git a/drivers/regulator/axp20x-regulator.c 
b/drivers/regulator/axp20x-regulator.c
index 6d9ac76..c3287c9 100644
---