Re: [PATCH 1/6] ARM: dts: rockchip: add basic dtsi file for RK3229 SoC

[Huang, Tao]

Hi Jacob and Heiko:
On 2017年06月21日 12:11, Jacob Chen wrote:
> Hi,
>
> 2017-06-20 18:48 GMT+08:00 Heiko Stübner  >:
> > Hi Frank,
> >
> > Am Dienstag, 20. Juni 2017, 15:13:00 CEST schrieb Frank Wang:
> >> Hi Heiko,
> >>
> >> On 2017/6/19 20:30, Heiko Stübner wrote:
> >> > Hi Frank,
> >> >
> >> > Am Montag, 19. Juni 2017, 18:34:27 CEST schrieb Frank Wang:
> >> >> On 2017/6/18 2:12, Heiko Stuebner wrote:
> >> >>> Am Donnerstag, 15. Juni 2017, 15:16:15 CEST schrieb Frank Wang:
> >>  Due to some tiny differences between RK3228 and RK3229, this patch
> >>  adds a basic dtsi file which includes a new CPU opp table and PSCI
> >>  brought up support for RK3229.
> >> 
> >>  Signed-off-by: Frank Wang >
> >> >
> >> > [...]
> >> >
> >>  +psci {
> >>  +compatible = "arm,psci-1.0", "arm,psci-0.2";
> >>  +method = "smc";
> >>  +};
> >>  +};
> >>  +
> >>  + {
> >>  +enable-method = "psci";
> >>  +};
> >> >>>
> >> >>> Hmm, I don't really understand this.
> >> >>> What method of core-bringup does the rk3228 use? In the current
> >> >>> rk322x.dtsi there is no enable-method at all defined.
> >> >>
> >> >> For non-security, the same with rk3036 SoC, we use rk3036-smp
> method to
> >> >> bring-up cores, and for security, we use arm-psci method.
> >> >> As security become more and more important and required, we
> would prefer
> >> >> using arm-psci method, and it is also an easy way to use.
> >> >>
> >> >>> So is the rk3228 firmware using a different method than the rk3229?
> >> >>
> >> >> No, they are the same. How about I move these changes to
> rk322x.dtsi?
> >> >
> >> > yep, that is what I was getting at with my question ;-)
> >> >
> >> >>> And out of curiosity as this is a arm32 without atf, is the psci
> >> >>> implementation (for uboot?) you're using available somewhere?
> >> >>
> >> >> Ah, it is included in op-tee :-)
> >> >
> >> > Is that super secret or will this be part of the official op-tee [0]
> >> > at some point (Similar to the ATF stuff on arm64)?
> >>
> >> Hmm, the op-tee itself must keep secure, but the psci part in it can be
> >> extracted to public, although it may have a bit of secure risk.
> >> Due to Rockchip have amended the frame of op-tee to support psci,
> we can
> >> try to upstream these changes to official op-tee or push them to source
> >> codes of Rockchip in git-hub.
> >
> > I just want to make sure, people can actually create a working system
> > with this, as there is mainline u-boot support for the rk3228/rk3229 in
> > the works - hopefully also with SPL support later on.
> > So I'm wondering how this is supposed to be setup?
> >
> > On arm64 we now have the SPL load the ATF, which in turn loads
> uboot, so I
> > guess the mechanism for the op-tee would be somehow similar? And
> there all
> > necessary components are available to build everything from source.
> >
> > I really don't care about all the other super-secret stuff happening in
> > that op-tee thingy, but I really want people to be able to build a
> complete
> > firmware on their machine, without having to rely on arbitary binary
> blobs.
> >
> > Which is something companies adopting Rockchip socs seem to rely on
> > a lot these days ;-) .
> >
> >
> > One alternative I can think of, would be to also create a u-boot psci
> > implementation for arm32, like sunxi [0] and others use for example.
> > That way people could choose where their psci should come from (u-boot
> > itself or the op-tee).
> >
> >
> > Heiko
> >
> > [0]
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/sunxi/psci.c
> >
> >>
> >>
> >> BR.
> >> Frank
> >>
> >> > Heiko
> >> >
> >> > [0]https://github.com/OP-TEE/optee_os/tree/master/core/arch/arm
> >
> >
>
>  Implement psci in upstream u-boot  sounds a good idea. 
>
> I don't like the bundled solution, like if I want  to enable  power
> management in my board,  i have to use OP-TEE, then i have to use
> vendor u-boot, then vendor kernel, rootfs, tools..
>
No. The kernel only depends on PSCI. Anyone can implement PSCI firmware
through
OPTEE/Trusty/U-Boot/UEFI or other open source implementation. We don't limit
people use vendor software or not. As Frank said, we will open source
OPTEE which
support PSCI for our chips.
BTW people can implement SMP/suspend function builtin in kernel as
usual. We just
hope use PSCI by default, so we can support TEE more easy as arm64.

Re: [PATCH 1/6] ARM: dts: rockchip: add basic dtsi file for RK3229 SoC

[Huang, Tao]

Hi Jacob and Heiko:
On 2017年06月21日 12:11, Jacob Chen wrote:
> Hi,
>
> 2017-06-20 18:48 GMT+08:00 Heiko Stübner  >:
> > Hi Frank,
> >
> > Am Dienstag, 20. Juni 2017, 15:13:00 CEST schrieb Frank Wang:
> >> Hi Heiko,
> >>
> >> On 2017/6/19 20:30, Heiko Stübner wrote:
> >> > Hi Frank,
> >> >
> >> > Am Montag, 19. Juni 2017, 18:34:27 CEST schrieb Frank Wang:
> >> >> On 2017/6/18 2:12, Heiko Stuebner wrote:
> >> >>> Am Donnerstag, 15. Juni 2017, 15:16:15 CEST schrieb Frank Wang:
> >>  Due to some tiny differences between RK3228 and RK3229, this patch
> >>  adds a basic dtsi file which includes a new CPU opp table and PSCI
> >>  brought up support for RK3229.
> >> 
> >>  Signed-off-by: Frank Wang >
> >> >
> >> > [...]
> >> >
> >>  +psci {
> >>  +compatible = "arm,psci-1.0", "arm,psci-0.2";
> >>  +method = "smc";
> >>  +};
> >>  +};
> >>  +
> >>  + {
> >>  +enable-method = "psci";
> >>  +};
> >> >>>
> >> >>> Hmm, I don't really understand this.
> >> >>> What method of core-bringup does the rk3228 use? In the current
> >> >>> rk322x.dtsi there is no enable-method at all defined.
> >> >>
> >> >> For non-security, the same with rk3036 SoC, we use rk3036-smp
> method to
> >> >> bring-up cores, and for security, we use arm-psci method.
> >> >> As security become more and more important and required, we
> would prefer
> >> >> using arm-psci method, and it is also an easy way to use.
> >> >>
> >> >>> So is the rk3228 firmware using a different method than the rk3229?
> >> >>
> >> >> No, they are the same. How about I move these changes to
> rk322x.dtsi?
> >> >
> >> > yep, that is what I was getting at with my question ;-)
> >> >
> >> >>> And out of curiosity as this is a arm32 without atf, is the psci
> >> >>> implementation (for uboot?) you're using available somewhere?
> >> >>
> >> >> Ah, it is included in op-tee :-)
> >> >
> >> > Is that super secret or will this be part of the official op-tee [0]
> >> > at some point (Similar to the ATF stuff on arm64)?
> >>
> >> Hmm, the op-tee itself must keep secure, but the psci part in it can be
> >> extracted to public, although it may have a bit of secure risk.
> >> Due to Rockchip have amended the frame of op-tee to support psci,
> we can
> >> try to upstream these changes to official op-tee or push them to source
> >> codes of Rockchip in git-hub.
> >
> > I just want to make sure, people can actually create a working system
> > with this, as there is mainline u-boot support for the rk3228/rk3229 in
> > the works - hopefully also with SPL support later on.
> > So I'm wondering how this is supposed to be setup?
> >
> > On arm64 we now have the SPL load the ATF, which in turn loads
> uboot, so I
> > guess the mechanism for the op-tee would be somehow similar? And
> there all
> > necessary components are available to build everything from source.
> >
> > I really don't care about all the other super-secret stuff happening in
> > that op-tee thingy, but I really want people to be able to build a
> complete
> > firmware on their machine, without having to rely on arbitary binary
> blobs.
> >
> > Which is something companies adopting Rockchip socs seem to rely on
> > a lot these days ;-) .
> >
> >
> > One alternative I can think of, would be to also create a u-boot psci
> > implementation for arm32, like sunxi [0] and others use for example.
> > That way people could choose where their psci should come from (u-boot
> > itself or the op-tee).
> >
> >
> > Heiko
> >
> > [0]
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/sunxi/psci.c
> >
> >>
> >>
> >> BR.
> >> Frank
> >>
> >> > Heiko
> >> >
> >> > [0]https://github.com/OP-TEE/optee_os/tree/master/core/arch/arm
> >
> >
>
>  Implement psci in upstream u-boot  sounds a good idea. 
>
> I don't like the bundled solution, like if I want  to enable  power
> management in my board,  i have to use OP-TEE, then i have to use
> vendor u-boot, then vendor kernel, rootfs, tools..
>
No. The kernel only depends on PSCI. Anyone can implement PSCI firmware
through
OPTEE/Trusty/U-Boot/UEFI or other open source implementation. We don't limit
people use vendor software or not. As Frank said, we will open source
OPTEE which
support PSCI for our chips.
BTW people can implement SMP/suspend function builtin in kernel as
usual. We just
hope use PSCI by default, so we can support TEE more easy as arm64.

Re: [PATCH v2 0/4] clocksource: rockchip/timer: Support rktimer for rk3399

[Huang, Tao]

Hi Daniel:
On 2016年06月13日 21:06, Daniel Lezcano wrote:
> On Tue, Jun 07, 2016 at 12:54:29PM +0800, Caesar Wang wrote:
>> This series patches had been tested on rockchip inside kernel.
>> In order to support the rk3399 SoC timer and turn off interrupts and IPIs to
>> save power in idle.
> 
> For my personnal information, are the arch_timer in the same power domain 
> than the CPU ? IOW, what is the 'always-on' property in the DT ?

Yes. In our SoC design, all arch (generic) timer in the same power
domain of CPU core. So if one CPU core power down, the arch (generic)
timer will lose it's state and stop working.
While rk timer maybe in peri power domain or pmu power domain, so the
timer will still work when CPU power down.

But before RK3399, all SoCs with CPU power domain, do not support auto
power down while cpu idle. So the arch timer can be seem as always on,
i.e. we don't need a broadcast timer at all.

> 
>> Okay, it still works bootup on rk3288/other SoCs, even though many socs 
>> hasn't used
>> the broadcast timer.
> 
> Yes, unfortunately the SoC design on rk3288 and the previous ones do not 
> allow to use a cpuidle driver with cpu/cluster power down, so obviously the 
> broadcast timer is pointless on these boards :)
> 

You are right.

>> History version:
>> v1:
>> https://lkml.org/lkml/2016/5/25/186
>>
>> Easy to test for my borad.
>> localhost / # cat /proc/interrupts
>> CPU0   CPU1   CPU2   CPU3   CPU4   CPU5
>> 1:  0  0  0  0  0  0 
>> GICv3  29 Edge  arch_timer
>> ...
>> 5:  0  0  0  0  0  0 
>> GICv3 113 Level rk_timer
>> ..
>>
>> localhost / # cat /proc/timer_list | grep event_handler
>> get "event_handler:  hrtimer_interrupt"
>> event_handler:  tick_handle_oneshot_broadcast
>> event_handler:  hrtimer_interrupt
> 
> What are you trying to demonstrate here ? There are no interrupts for both 
> arch_timer and rk_timer.

I don't know. Maybe Caesar do something wrong :(
This is my output:
   CPU0   CPU1   CPU2   CPU3   CPU4   CPU5

...
  2:   2911   1967   1588   1608   1295   1606
   GICv3  30 Edge  arch_timer
  5:578637684626161165
   GICv3 113 Level rk_timer

Re: [PATCH v2 0/4] clocksource: rockchip/timer: Support rktimer for rk3399

[Huang, Tao]

Hi Daniel:
On 2016年06月13日 21:06, Daniel Lezcano wrote:
> On Tue, Jun 07, 2016 at 12:54:29PM +0800, Caesar Wang wrote:
>> This series patches had been tested on rockchip inside kernel.
>> In order to support the rk3399 SoC timer and turn off interrupts and IPIs to
>> save power in idle.
> 
> For my personnal information, are the arch_timer in the same power domain 
> than the CPU ? IOW, what is the 'always-on' property in the DT ?

Yes. In our SoC design, all arch (generic) timer in the same power
domain of CPU core. So if one CPU core power down, the arch (generic)
timer will lose it's state and stop working.
While rk timer maybe in peri power domain or pmu power domain, so the
timer will still work when CPU power down.

But before RK3399, all SoCs with CPU power domain, do not support auto
power down while cpu idle. So the arch timer can be seem as always on,
i.e. we don't need a broadcast timer at all.

> 
>> Okay, it still works bootup on rk3288/other SoCs, even though many socs 
>> hasn't used
>> the broadcast timer.
> 
> Yes, unfortunately the SoC design on rk3288 and the previous ones do not 
> allow to use a cpuidle driver with cpu/cluster power down, so obviously the 
> broadcast timer is pointless on these boards :)
> 

You are right.

>> History version:
>> v1:
>> https://lkml.org/lkml/2016/5/25/186
>>
>> Easy to test for my borad.
>> localhost / # cat /proc/interrupts
>> CPU0   CPU1   CPU2   CPU3   CPU4   CPU5
>> 1:  0  0  0  0  0  0 
>> GICv3  29 Edge  arch_timer
>> ...
>> 5:  0  0  0  0  0  0 
>> GICv3 113 Level rk_timer
>> ..
>>
>> localhost / # cat /proc/timer_list | grep event_handler
>> get "event_handler:  hrtimer_interrupt"
>> event_handler:  tick_handle_oneshot_broadcast
>> event_handler:  hrtimer_interrupt
> 
> What are you trying to demonstrate here ? There are no interrupts for both 
> arch_timer and rk_timer.

I don't know. Maybe Caesar do something wrong :(
This is my output:
   CPU0   CPU1   CPU2   CPU3   CPU4   CPU5

...
  2:   2911   1967   1588   1608   1295   1606
   GICv3  30 Edge  arch_timer
  5:578637684626161165
   GICv3 113 Level rk_timer

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Huang, Tao]

Hi Doug:
On 2016年06月01日 01:03, Doug Anderson wrote:
> Hi,
> 
> On Mon, May 30, 2016 at 4:09 PM, Daniel Lezcano
> <daniel.lezc...@linaro.org> wrote:
>> On 05/25/2016 11:49 AM, Caesar Wang wrote:
>>>
>>> From: Huang Tao <huang...@rock-chips.com>
>>>
>>> rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
>>> request_irq. Timer should keep disabled before booting Linux.
>>
>>
>> That's true in the perfect world :/ Some version has u-boot letting the
>> timer with irq enabled, therefore as soon as request_irq is done, an irq
>> fires and leads to a kernel panic.
>>
>> On the other side, this timer is not used on the other rockchip version than
>> rk3399 because of no need of a broadcast timer, so removing these two lines
>> may be acceptable.
>>
>> Can try the changes with another board, eg rk3288 (and forcing to use this
>> timer). Can you do the test and confirm it does not break with different
>> version of u-boot ?
> 
> Actually, I'm not even sure that's true in a perfect world.  ;)  There
> are two main problems that might be lurking here:
> 
> 1. On exynos5 devices I've worked with, the private timer (MCT)
> actually shared the same physical counter with the ARM Architected
> Timer.  IIRC stopping or resetting the MCT had the effect of stopping
> / resetting the Arch Timer.  Is it the same for you?  As I understand
> it the Arch Timer isn't supposed to ever be stopped or reset.  If
> firmware left the timer stopped and the kernel happened to be compiled
> without support for the Rockchip timer (but had the Arch Timers) then
> things would be very broken.  Also the early kernel boot might be
> broken if the Arch Timer inits before the Rockchip timer.
> 
> NOTE: If your timer and the Arch Timer are totally separate then point
> #1 is not important.

We never use the timer which provide clock source of arch timer as
clockevent timer. If we do such stupid thing, when rk timer disabled,
the arch timer will stop too. Generally, we use this special timer as
clocksouce or never touch it again when it is running.

> 
> 
> 2. Historically in Chrome OS there's been an unofficial agreement that
> the firmware would start its high speed timer as soon as possible at
> bootup and that this could be used to (roughly) measure the time
> between the start of firmware and the start of the kernel.  That means
> that the kernel was expecting the timer to actually be running when it
> started up.  Yup, this is a bit of a hack and I'm not sure it's
> terribly well documented, but it does provide a reason that firmware
> might have left the timer running.

Why you chose the timer shared with Linux kernel, there are so many
timer? I think loader should do the right thing, uninit the resources
when it boot the kernel. I believe this code is lagacy from very old
chip such as rk2908 which is Cortex-A8. There are not arch timer, so the
loader may keep the timer running when enter kernel. Any way, if we
adopt the code suggested by Daniel, it is safe to keep the code.

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Huang, Tao]

Hi Doug:
On 2016年06月01日 01:03, Doug Anderson wrote:
> Hi,
> 
> On Mon, May 30, 2016 at 4:09 PM, Daniel Lezcano
>  wrote:
>> On 05/25/2016 11:49 AM, Caesar Wang wrote:
>>>
>>> From: Huang Tao 
>>>
>>> rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
>>> request_irq. Timer should keep disabled before booting Linux.
>>
>>
>> That's true in the perfect world :/ Some version has u-boot letting the
>> timer with irq enabled, therefore as soon as request_irq is done, an irq
>> fires and leads to a kernel panic.
>>
>> On the other side, this timer is not used on the other rockchip version than
>> rk3399 because of no need of a broadcast timer, so removing these two lines
>> may be acceptable.
>>
>> Can try the changes with another board, eg rk3288 (and forcing to use this
>> timer). Can you do the test and confirm it does not break with different
>> version of u-boot ?
> 
> Actually, I'm not even sure that's true in a perfect world.  ;)  There
> are two main problems that might be lurking here:
> 
> 1. On exynos5 devices I've worked with, the private timer (MCT)
> actually shared the same physical counter with the ARM Architected
> Timer.  IIRC stopping or resetting the MCT had the effect of stopping
> / resetting the Arch Timer.  Is it the same for you?  As I understand
> it the Arch Timer isn't supposed to ever be stopped or reset.  If
> firmware left the timer stopped and the kernel happened to be compiled
> without support for the Rockchip timer (but had the Arch Timers) then
> things would be very broken.  Also the early kernel boot might be
> broken if the Arch Timer inits before the Rockchip timer.
> 
> NOTE: If your timer and the Arch Timer are totally separate then point
> #1 is not important.

We never use the timer which provide clock source of arch timer as
clockevent timer. If we do such stupid thing, when rk timer disabled,
the arch timer will stop too. Generally, we use this special timer as
clocksouce or never touch it again when it is running.

> 
> 
> 2. Historically in Chrome OS there's been an unofficial agreement that
> the firmware would start its high speed timer as soon as possible at
> bootup and that this could be used to (roughly) measure the time
> between the start of firmware and the start of the kernel.  That means
> that the kernel was expecting the timer to actually be running when it
> started up.  Yup, this is a bit of a hack and I'm not sure it's
> terribly well documented, but it does provide a reason that firmware
> might have left the timer running.

Why you chose the timer shared with Linux kernel, there are so many
timer? I think loader should do the right thing, uninit the resources
when it boot the kernel. I believe this code is lagacy from very old
chip such as rk2908 which is Cortex-A8. There are not arch timer, so the
loader may keep the timer running when enter kernel. Any way, if we
adopt the code suggested by Daniel, it is safe to keep the code.

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 22:06, Daniel Lezcano wrote:
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>  return rk_timer(ce)->base;
>>   }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +return rk_timer(ce)->base + rk_timer(ce)->ctrl;
> 
> You can do a small optimization by pre-computing 'ctrl' at init time, so 
> no need to do this addition each time.

I understand what you mean, please see comment below.
And even we use ctrl as pointer, we still will get addition LDR other
then ADD.
This is disassemble code before:
   0:   f9408021ldr x1, [x1,#256]
   4:   5283mov w3, #0x0
   8:   91004022add x2, x1, #0x10
   c:   b943str w3, [x2]
This is disassemble code after change:
   0:   5283mov w3, #0x0
   4:   f9408422ldr x2, [x1,#264]
   8:   b943str w3, [x2]
   c:   f9408021ldr x1, [x1,#256]
Of course we can assume cache hit.

> 
>> +}
>> +
>>   static inline void rk_timer_disable(struct clock_event_device *ce)
>>   {
>> -writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>   }
>>
>>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>   {
>>  writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -   rk_base(ce) + TIMER_CONTROL_REG);
>> +   rk_ctrl(ce));
>>   }
>>
>>   static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,19 @@ out_unmap:
>>  iounmap(bc_timer.base);
>>   }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +rk_timer_init(np);
> 
>   rk_timer_init(np);
>   bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

No. It's not such simple. You will access null pointer when
rk_timer_init, if we keep rk_timer_disable call in init or after
request_irq/clockevents_config_and_register and interrupt happen
immediately.

So the code maybe:
static void __init rk3288_timer_init(struct device_node *np)
{
bc_timer.base = of_iomap(np, 0);
if (!bc_timer.base) {
pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
return;
}
bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
rk_imter_init(np); // of course remove of_iomap from init.

Is this what you want?

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 22:06, Daniel Lezcano wrote:
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>  return rk_timer(ce)->base;
>>   }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +return rk_timer(ce)->base + rk_timer(ce)->ctrl;
> 
> You can do a small optimization by pre-computing 'ctrl' at init time, so 
> no need to do this addition each time.

I understand what you mean, please see comment below.
And even we use ctrl as pointer, we still will get addition LDR other
then ADD.
This is disassemble code before:
   0:   f9408021ldr x1, [x1,#256]
   4:   5283mov w3, #0x0
   8:   91004022add x2, x1, #0x10
   c:   b943str w3, [x2]
This is disassemble code after change:
   0:   5283mov w3, #0x0
   4:   f9408422ldr x2, [x1,#264]
   8:   b943str w3, [x2]
   c:   f9408021ldr x1, [x1,#256]
Of course we can assume cache hit.

> 
>> +}
>> +
>>   static inline void rk_timer_disable(struct clock_event_device *ce)
>>   {
>> -writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>   }
>>
>>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>   {
>>  writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -   rk_base(ce) + TIMER_CONTROL_REG);
>> +   rk_ctrl(ce));
>>   }
>>
>>   static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,19 @@ out_unmap:
>>  iounmap(bc_timer.base);
>>   }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +rk_timer_init(np);
> 
>   rk_timer_init(np);
>   bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

No. It's not such simple. You will access null pointer when
rk_timer_init, if we keep rk_timer_disable call in init or after
request_irq/clockevents_config_and_register and interrupt happen
immediately.

So the code maybe:
static void __init rk3288_timer_init(struct device_node *np)
{
bc_timer.base = of_iomap(np, 0);
if (!bc_timer.base) {
pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
return;
}
bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
rk_imter_init(np); // of course remove of_iomap from init.

Is this what you want?

Re: [PATCH 3/5] clocksource: rockchip: add dynamic irq flag to the timer

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:16, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao <huang...@rock-chips.com>
>>
>> The rockchip timer is broadcast timer. Add CLOCK_EVT_FEAT_DYNIRQ
>> flag and set cpumask to all cpu to save power by avoid unnecessary
>> wakeups and IPIs.
>>
>> Signed-off-by: Huang Tao <huang...@rock-chips.com>
>> Cc: Daniel Lezcano <daniel.lezc...@linaro.org>
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Cc: Heiko Stuebner <he...@sntech.de>
>> Tested-by: Jianqun Xu <jay...@rock-chips.com>
>> Signed-off-by: Caesar Wang <w...@rock-chips.com>
>> ---
>>
>>   drivers/clocksource/rockchip_timer.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clocksource/rockchip_timer.c 
>> b/drivers/clocksource/rockchip_timer.c
>> index b93fed6..f3dfb1a 100644
>> --- a/drivers/clocksource/rockchip_timer.c
>> +++ b/drivers/clocksource/rockchip_timer.c
>> @@ -150,12 +150,13 @@ static void __init rk_timer_init(struct device_node 
>> *np)
>>  }
>>
>>  ce->name = TIMER_NAME;
>> -ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>> +ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>> +   CLOCK_EVT_FEAT_DYNIRQ;
>>  ce->set_next_event = rk_timer_set_next_event;
>>  ce->set_state_shutdown = rk_timer_shutdown;
>>  ce->set_state_periodic = rk_timer_set_periodic;
>>  ce->irq = irq;
>> -ce->cpumask = cpumask_of(0);
>> +ce->cpumask = cpu_all_mask;
> 
> s/cpu_all_mask/cpu_possible_mask/

Okay for me.
It seems drivers in drivers/clocksource are very confusing about use
cpu_all_mask or cpu_possible_mask. For example arm_arch_timer.c use
cpu_all_mask, while mtk_timer.c use cpu_possible_mask. I think all just
work.

Thanks,
Huang, Tao

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:28, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao <huang...@rock-chips.com>
>>
>> The CONTROL register offset is different from old SoCs.
>> For Linux driver, there are not functional changes at all.
>> Let's call it v2.
>>
>> Signed-off-by: Huang Tao <huang...@rock-chips.com>
>> Cc: Daniel Lezcano <daniel.lezc...@linaro.org>
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Cc: Heiko Stuebner <he...@sntech.de>
>> Tested-by: Jianqun Xu <jay...@rock-chips.com>
>> Signed-off-by: Caesar Wang <w...@rock-chips.com>
>> ---
> 
> That's hackish.
Yes:( I blamed our IC guy.
> 
> Please consider something like:
> 
> diff --git a/drivers/clocksource/rockchip_timer.c 
> b/drivers/clocksource/rockchip_timer.c
> index b991b28..b6ba6f9 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -19,7 +19,8 @@
> 
>   #define TIMER_LOAD_COUNT0   0x00
>   #define TIMER_LOAD_COUNT1   0x04
> -#define TIMER_CONTROL_REG0x10
> +#define TIMER_CONTROL_REG32880x10
> +#define TIMER_CONTROL_REG33990x1C
>   #define TIMER_INT_STATUS0x18
> 
>   #define TIMER_DISABLE   0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   struct clock_event_device ce;
>   void __iomem *base;
> + void __iomem *ctrl;
>   u32 freq;
>   };
> 
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
> clock_event_device *ce)
>   return rk_timer(ce)->base;
>   }
> 
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +return rk_timer(ce)->ctrl;
> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> - writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> + writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
> 
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32 
> flags)
>   {
>   writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -rk_base(ce) + TIMER_CONTROL_REG);
> +rk_ctrl(ce));
>   }
> 
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,18 @@ out_unmap:
>   iounmap(bc_timer.base);
>   }
> 
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> + bc_timer.ctrl = TIMER_CONTROL_REG3288;
> + rk_timer_init(np);
> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +bc_timer.ctrl = TIMER_CONTROL_REG3399;
> + rk_timer_init(np);
> +}
> +
> +
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
> rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
> rk3399_timer_init);
> 
> 

I think you mean this patch otherwise compile will fail:
@@ -19,7 +19,8 @@

 #define TIMER_LOAD_COUNT0  0x00
 #define TIMER_LOAD_COUNT1  0x04
-#define TIMER_CONTROL_REG  0x10
+#define TIMER_CONTROL_REG3288  0x10
+#define TIMER_CONTROL_REG3399  0x1C
 #define TIMER_INT_STATUS   0x18

 #define TIMER_DISABLE  0x0
@@ -31,6 +32,7 @@
 struct bc_timer {
struct clock_event_device ce;
void __iomem *base;
+   u32 ctrl;
u32 freq;
 };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
clock_event_device *ce)
return rk_timer(ce)->base;
 }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+   return rk_timer(ce)->base + rk_timer(ce)->ctrl;
+}
+
 static inline void rk_timer_disable(struct clock_event_device *ce)
 {
-   writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+   writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
 }

 static inline void rk_timer_enable(struct clock_event_device *ce, u32
flags)
 {
writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-  rk_base(ce) + TIMER_CONTROL_REG);
+  rk_ctrl(ce));
 }

 static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,19 @@ out_unmap:
iounmap(bc_timer.base);
 }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+   bc_timer.ctrl = TIMER_CONTROL_REG3288;
+   rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+   bc_timer.ctrl = TIMER_CONTROL_REG3399;
+   rk_timer_init(np);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
+  rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
+  rk3399_timer_init);

This patch will give us a little lager text size. If we do disassemble,
we can see additional LDR is called. I can accept this performance drop.
So we will send new patches.
BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
before request_irq" can drop if we use this patch.

Thanks,
Huang, Tao

Re: [PATCH 3/5] clocksource: rockchip: add dynamic irq flag to the timer

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:16, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao 
>>
>> The rockchip timer is broadcast timer. Add CLOCK_EVT_FEAT_DYNIRQ
>> flag and set cpumask to all cpu to save power by avoid unnecessary
>> wakeups and IPIs.
>>
>> Signed-off-by: Huang Tao 
>> Cc: Daniel Lezcano 
>> Cc: Thomas Gleixner 
>> Cc: Heiko Stuebner 
>> Tested-by: Jianqun Xu 
>> Signed-off-by: Caesar Wang 
>> ---
>>
>>   drivers/clocksource/rockchip_timer.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clocksource/rockchip_timer.c 
>> b/drivers/clocksource/rockchip_timer.c
>> index b93fed6..f3dfb1a 100644
>> --- a/drivers/clocksource/rockchip_timer.c
>> +++ b/drivers/clocksource/rockchip_timer.c
>> @@ -150,12 +150,13 @@ static void __init rk_timer_init(struct device_node 
>> *np)
>>  }
>>
>>  ce->name = TIMER_NAME;
>> -ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>> +ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>> +   CLOCK_EVT_FEAT_DYNIRQ;
>>  ce->set_next_event = rk_timer_set_next_event;
>>  ce->set_state_shutdown = rk_timer_shutdown;
>>  ce->set_state_periodic = rk_timer_set_periodic;
>>  ce->irq = irq;
>> -ce->cpumask = cpumask_of(0);
>> +ce->cpumask = cpu_all_mask;
> 
> s/cpu_all_mask/cpu_possible_mask/

Okay for me.
It seems drivers in drivers/clocksource are very confusing about use
cpu_all_mask or cpu_possible_mask. For example arm_arch_timer.c use
cpu_all_mask, while mtk_timer.c use cpu_possible_mask. I think all just
work.

Thanks,
Huang, Tao

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:28, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao 
>>
>> The CONTROL register offset is different from old SoCs.
>> For Linux driver, there are not functional changes at all.
>> Let's call it v2.
>>
>> Signed-off-by: Huang Tao 
>> Cc: Daniel Lezcano 
>> Cc: Thomas Gleixner 
>> Cc: Heiko Stuebner 
>> Tested-by: Jianqun Xu 
>> Signed-off-by: Caesar Wang 
>> ---
> 
> That's hackish.
Yes:( I blamed our IC guy.
> 
> Please consider something like:
> 
> diff --git a/drivers/clocksource/rockchip_timer.c 
> b/drivers/clocksource/rockchip_timer.c
> index b991b28..b6ba6f9 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -19,7 +19,8 @@
> 
>   #define TIMER_LOAD_COUNT0   0x00
>   #define TIMER_LOAD_COUNT1   0x04
> -#define TIMER_CONTROL_REG0x10
> +#define TIMER_CONTROL_REG32880x10
> +#define TIMER_CONTROL_REG33990x1C
>   #define TIMER_INT_STATUS0x18
> 
>   #define TIMER_DISABLE   0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   struct clock_event_device ce;
>   void __iomem *base;
> + void __iomem *ctrl;
>   u32 freq;
>   };
> 
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
> clock_event_device *ce)
>   return rk_timer(ce)->base;
>   }
> 
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +return rk_timer(ce)->ctrl;
> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> - writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> + writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
> 
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32 
> flags)
>   {
>   writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -rk_base(ce) + TIMER_CONTROL_REG);
> +rk_ctrl(ce));
>   }
> 
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,18 @@ out_unmap:
>   iounmap(bc_timer.base);
>   }
> 
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> + bc_timer.ctrl = TIMER_CONTROL_REG3288;
> + rk_timer_init(np);
> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +bc_timer.ctrl = TIMER_CONTROL_REG3399;
> + rk_timer_init(np);
> +}
> +
> +
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
> rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
> rk3399_timer_init);
> 
> 

I think you mean this patch otherwise compile will fail:
@@ -19,7 +19,8 @@

 #define TIMER_LOAD_COUNT0  0x00
 #define TIMER_LOAD_COUNT1  0x04
-#define TIMER_CONTROL_REG  0x10
+#define TIMER_CONTROL_REG3288  0x10
+#define TIMER_CONTROL_REG3399  0x1C
 #define TIMER_INT_STATUS   0x18

 #define TIMER_DISABLE  0x0
@@ -31,6 +32,7 @@
 struct bc_timer {
struct clock_event_device ce;
void __iomem *base;
+   u32 ctrl;
u32 freq;
 };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
clock_event_device *ce)
return rk_timer(ce)->base;
 }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+   return rk_timer(ce)->base + rk_timer(ce)->ctrl;
+}
+
 static inline void rk_timer_disable(struct clock_event_device *ce)
 {
-   writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+   writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
 }

 static inline void rk_timer_enable(struct clock_event_device *ce, u32
flags)
 {
writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-  rk_base(ce) + TIMER_CONTROL_REG);
+  rk_ctrl(ce));
 }

 static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,19 @@ out_unmap:
iounmap(bc_timer.base);
 }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+   bc_timer.ctrl = TIMER_CONTROL_REG3288;
+   rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+   bc_timer.ctrl = TIMER_CONTROL_REG3399;
+   rk_timer_init(np);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
+  rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
+  rk3399_timer_init);

This patch will give us a little lager text size. If we do disassemble,
we can see additional LDR is called. I can accept this performance drop.
So we will send new patches.
BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
before request_irq" can drop if we use this patch.

Thanks,
Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月25日 18:47, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 06:19:28PM +0800, Huang, Tao wrote:
>> Hi, Mark:
>> On 2016年04月25日 18:05, Mark Rutland wrote:
>>> On Mon, Apr 25, 2016 at 05:48:51PM +0800, Huang, Tao wrote:
>>>> and pmu define as:
>>>> pmu_a53 {
>>>> compatible = "arm,cortex-a53-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_l0>,
>>>>  <_l1>,
>>>>  <_l2>,
>>>>  <_l3>;
>>>> };
>>>>
>>>> pmu_a72 {
>>>> compatible = "arm,cortex-a72-pmu", "arm,cortex-a57-pmu";
>>> That Cortex-A57 PMU fallback should just go. We already have Cortex-A72
>>> PMU support upstream, and I believe there are sufficient differences
>>> such that the Cortex-A72 PMU is not a strict superset of the Cortex-A57
>>> PMU.
>> As I say, I tested on v4.4, I don't back port
>> arch/arm64/kernel/perf_event.c, so I use "arm,cortex-a57-pmu". Upstream
>> will use "arm,cortex-a72-pmu" only.
>> BTW, I don't see any differences between A72/A57 in source code:
> The PMU name is exposed to userspace, so the user will be told they have
> a Cortex-A57 PMU, with all of the IMPLEMENTATION DEFINED events that
> implies.
>
> We don't handle those IMPLEMENTATION DEFINED events in the kernel, but
> for the sake of the userspace ABI, we should not expose the Cortex-A72
> PMU as a Cortex-A57 PMU.
>
> Given the code is otherwise identical, it should be relatively simple to
> backport the A72 support.
>
Understood, thank you!

Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月25日 18:47, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 06:19:28PM +0800, Huang, Tao wrote:
>> Hi, Mark:
>> On 2016年04月25日 18:05, Mark Rutland wrote:
>>> On Mon, Apr 25, 2016 at 05:48:51PM +0800, Huang, Tao wrote:
>>>> and pmu define as:
>>>> pmu_a53 {
>>>> compatible = "arm,cortex-a53-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_l0>,
>>>>  <_l1>,
>>>>  <_l2>,
>>>>  <_l3>;
>>>> };
>>>>
>>>> pmu_a72 {
>>>> compatible = "arm,cortex-a72-pmu", "arm,cortex-a57-pmu";
>>> That Cortex-A57 PMU fallback should just go. We already have Cortex-A72
>>> PMU support upstream, and I believe there are sufficient differences
>>> such that the Cortex-A72 PMU is not a strict superset of the Cortex-A57
>>> PMU.
>> As I say, I tested on v4.4, I don't back port
>> arch/arm64/kernel/perf_event.c, so I use "arm,cortex-a57-pmu". Upstream
>> will use "arm,cortex-a72-pmu" only.
>> BTW, I don't see any differences between A72/A57 in source code:
> The PMU name is exposed to userspace, so the user will be told they have
> a Cortex-A57 PMU, with all of the IMPLEMENTATION DEFINED events that
> implies.
>
> We don't handle those IMPLEMENTATION DEFINED events in the kernel, but
> for the sake of the userspace ABI, we should not expose the Cortex-A72
> PMU as a Cortex-A57 PMU.
>
> Given the code is otherwise identical, it should be relatively simple to
> backport the A72 support.
>
Understood, thank you!

Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Marc:
On 2016年04月25日 18:39, Marc Zyngier wrote:
> On 25/04/16 11:06, Marc Zyngier wrote:
>> On 25/04/16 10:48, Huang, Tao wrote:
>>> Hi, Marc:
>>> On 2016年04月21日 19:30, Marc Zyngier wrote:
>>>> On Thu, 21 Apr 2016 18:47:20 +0800
>>>> "Huang, Tao" <huang...@rock-chips.com> wrote:
>>>>
>>>>> Hi, Mark:
>>>>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>>>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>>>>> +   cpu_l0: cpu@0 {
>>>>>>> +   device_type = "cpu";
>>>>>>> +   compatible = "arm,cortex-a53", "arm,armv8";
>>>>>>> +   reg = <0x0 0x0>;
>>>>>>> +   enable-method = "psci";
>>>>>>> +   #cooling-cells = <2>; /* min followed by max */
>>>>>>> +   clocks = < ARMCLKL>;
>>>>>>> +   };
>>>>>>> +   cpu_b0: cpu@100 {
>>>>>>> +   device_type = "cpu";
>>>>>>> +   compatible = "arm,cortex-a72", "arm,armv8";
>>>>>>> +   reg = <0x0 0x100>;
>>>>>>> +   enable-method = "psci";
>>>>>>> +   #cooling-cells = <2>; /* min followed by max */
>>>>>>> +   clocks = < ARMCLKB>;
>>>>>>> +   };
>>>>>>> +
>>>>>>> +   arm-pmu {
>>>>>>> +   compatible = "arm,armv8-pmuv3";
>>>>>>> +   interrupts = ;
>>>>>>> +   };
>>>>>> This is wrong, and must go. There should be a separate node for the PMU
>>>>>> of each microarchitecture, with the appropriate compatible string to
>>>>>> represent that (see the juno dts).
>>>>> You are right. The first version we wrote is:
>>>>> pmu_a53 {
>>>>> compatible = "arm,cortex-a53-pmu";
>>>>> interrupts = ;
>>>>> interrupt-affinity = <_l0>,
>>>>>  <_l1>,
>>>>>  <_l2>,
>>>>>  <_l3>;
>>>>> };
>>>>>
>>>>> pmu_a72 {
>>>>> compatible = "arm,cortex-a72-pmu";
>>>>> interrupts = ;
>>>>> interrupt-affinity = <_b0>,
>>>>>  <_b1>;
>>>>> };
>>>>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>>>>> well,
>>>>> so we have to replace with this implementation.
>>>>>> In this case things are messier as the same PPI number is being used
>>>>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>>>>> should allow us to support that.
>>>>> Great! So what we can do right now? Wait this feature, and delete
>>>>> arm-pmu node?
>>>> I'd rather you have a look at the patches, test them with your HW,
>>>> and comment on what doesn't work!
>>>>
>>>> You can find the patches over there:
>>>>
>>>> https://lkml.org/lkml/2016/4/11/182
>>>>
>>>> and on the following branch:
>>>>
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>>> irq/percpu-partition
>>> I tested these patches. Because our kernel is based on v4.4, so I back
>>> port most changes about
>>> include/linux/irqdomain.h
>>> kernel/irq/irqdomain.c
>>> drivers/irqchip/irq-gic-v3.c
>>> and change rk3399.dtsi base on your arm,gic-v3.txt:
>>>
>>>  gic: interrupt-controller@fee0 {
>>>  compatible = "arm,gic-v3";
>>> -#interrupt-cells = <3>;
>>> +#interrupt-cells = <4>;
>>>  #address-cells = <2>;
>>>  #size-cells = <2>;
>>> ...
>>> +
>>> +ppi-partitions {
>>> +part0: interrupt-partition-0 {

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Marc:
On 2016年04月25日 18:39, Marc Zyngier wrote:
> On 25/04/16 11:06, Marc Zyngier wrote:
>> On 25/04/16 10:48, Huang, Tao wrote:
>>> Hi, Marc:
>>> On 2016年04月21日 19:30, Marc Zyngier wrote:
>>>> On Thu, 21 Apr 2016 18:47:20 +0800
>>>> "Huang, Tao"  wrote:
>>>>
>>>>> Hi, Mark:
>>>>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>>>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>>>>> +   cpu_l0: cpu@0 {
>>>>>>> +   device_type = "cpu";
>>>>>>> +   compatible = "arm,cortex-a53", "arm,armv8";
>>>>>>> +   reg = <0x0 0x0>;
>>>>>>> +   enable-method = "psci";
>>>>>>> +   #cooling-cells = <2>; /* min followed by max */
>>>>>>> +   clocks = < ARMCLKL>;
>>>>>>> +   };
>>>>>>> +   cpu_b0: cpu@100 {
>>>>>>> +   device_type = "cpu";
>>>>>>> +   compatible = "arm,cortex-a72", "arm,armv8";
>>>>>>> +   reg = <0x0 0x100>;
>>>>>>> +   enable-method = "psci";
>>>>>>> +   #cooling-cells = <2>; /* min followed by max */
>>>>>>> +   clocks = < ARMCLKB>;
>>>>>>> +   };
>>>>>>> +
>>>>>>> +   arm-pmu {
>>>>>>> +   compatible = "arm,armv8-pmuv3";
>>>>>>> +   interrupts = ;
>>>>>>> +   };
>>>>>> This is wrong, and must go. There should be a separate node for the PMU
>>>>>> of each microarchitecture, with the appropriate compatible string to
>>>>>> represent that (see the juno dts).
>>>>> You are right. The first version we wrote is:
>>>>> pmu_a53 {
>>>>> compatible = "arm,cortex-a53-pmu";
>>>>> interrupts = ;
>>>>> interrupt-affinity = <_l0>,
>>>>>  <_l1>,
>>>>>  <_l2>,
>>>>>  <_l3>;
>>>>> };
>>>>>
>>>>> pmu_a72 {
>>>>> compatible = "arm,cortex-a72-pmu";
>>>>> interrupts = ;
>>>>> interrupt-affinity = <_b0>,
>>>>>  <_b1>;
>>>>> };
>>>>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>>>>> well,
>>>>> so we have to replace with this implementation.
>>>>>> In this case things are messier as the same PPI number is being used
>>>>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>>>>> should allow us to support that.
>>>>> Great! So what we can do right now? Wait this feature, and delete
>>>>> arm-pmu node?
>>>> I'd rather you have a look at the patches, test them with your HW,
>>>> and comment on what doesn't work!
>>>>
>>>> You can find the patches over there:
>>>>
>>>> https://lkml.org/lkml/2016/4/11/182
>>>>
>>>> and on the following branch:
>>>>
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>>> irq/percpu-partition
>>> I tested these patches. Because our kernel is based on v4.4, so I back
>>> port most changes about
>>> include/linux/irqdomain.h
>>> kernel/irq/irqdomain.c
>>> drivers/irqchip/irq-gic-v3.c
>>> and change rk3399.dtsi base on your arm,gic-v3.txt:
>>>
>>>  gic: interrupt-controller@fee0 {
>>>  compatible = "arm,gic-v3";
>>> -#interrupt-cells = <3>;
>>> +#interrupt-cells = <4>;
>>>  #address-cells = <2>;
>>>  #size-cells = <2>;
>>> ...
>>> +
>>> +ppi-partitions {
>>> +part0: interrupt-partition-0 {
>>> +affin

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月25日 18:05, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 05:48:51PM +0800, Huang, Tao wrote:
>> Hi, Marc:
>> On 2016年04月21日 19:30, Marc Zyngier wrote:
>>> On Thu, 21 Apr 2016 18:47:20 +0800
>>> "Huang, Tao" <huang...@rock-chips.com> wrote:
>>>
>>>> Hi, Mark:
>>>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>>>> +cpu_l0: cpu@0 {
>>>>>> +device_type = "cpu";
>>>>>> +compatible = "arm,cortex-a53", "arm,armv8";
>>>>>> +reg = <0x0 0x0>;
>>>>>> +enable-method = "psci";
>>>>>> +#cooling-cells = <2>; /* min followed by max */
>>>>>> +clocks = < ARMCLKL>;
>>>>>> +};
>>>>>> +cpu_b0: cpu@100 {
>>>>>> +device_type = "cpu";
>>>>>> +compatible = "arm,cortex-a72", "arm,armv8";
>>>>>> +reg = <0x0 0x100>;
>>>>>> +enable-method = "psci";
>>>>>> +#cooling-cells = <2>; /* min followed by max */
>>>>>> +clocks = < ARMCLKB>;
>>>>>> +};
>>>>>> +
>>>>>> +arm-pmu {
>>>>>> +compatible = "arm,armv8-pmuv3";
>>>>>> +interrupts = ;
>>>>>> +};
>>>>> This is wrong, and must go. There should be a separate node for the PMU
>>>>> of each microarchitecture, with the appropriate compatible string to
>>>>> represent that (see the juno dts).
>>>> You are right. The first version we wrote is:
>>>> pmu_a53 {
>>>> compatible = "arm,cortex-a53-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_l0>,
>>>>  <_l1>,
>>>>  <_l2>,
>>>>  <_l3>;
>>>> };
>>>>
>>>> pmu_a72 {
>>>> compatible = "arm,cortex-a72-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_b0>,
>>>>  <_b1>;
>>>> };
>>>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>>>> well,
>>>> so we have to replace with this implementation.
>>>>> In this case things are messier as the same PPI number is being used
>>>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>>>> should allow us to support that.
>>>> Great! So what we can do right now? Wait this feature, and delete
>>>> arm-pmu node?
>>> I'd rather you have a look at the patches, test them with your HW,
>>> and comment on what doesn't work!
>>>
>>> You can find the patches over there:
>>>
>>> https://lkml.org/lkml/2016/4/11/182
>>>
>>> and on the following branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>> irq/percpu-partition
>> I tested these patches. Because our kernel is based on v4.4, so I back
>> port most changes about
>> include/linux/irqdomain.h
>> kernel/irq/irqdomain.c
>> drivers/irqchip/irq-gic-v3.c
>> and change rk3399.dtsi base on your arm,gic-v3.txt:
>>
>>  gic: interrupt-controller@fee0 {
>>  compatible = "arm,gic-v3";
>> -#interrupt-cells = <3>;
>> +#interrupt-cells = <4>;
>>  #address-cells = <2>;
>>  #size-cells = <2>;
>> ...
>> +
>> +ppi-partitions {
>> +part0: interrupt-partition-0 {
>> +affinity = <_l0 _l1 _l2 _l3>;
>> +};
>> +
>> +part1: interrupt-partition-1 {
>> +affinity = <_b0 _b1>;
>> +};
>> +};
>>
>> and change every interrupts from three cells to four cells,

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月25日 18:05, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 05:48:51PM +0800, Huang, Tao wrote:
>> Hi, Marc:
>> On 2016年04月21日 19:30, Marc Zyngier wrote:
>>> On Thu, 21 Apr 2016 18:47:20 +0800
>>> "Huang, Tao"  wrote:
>>>
>>>> Hi, Mark:
>>>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>>>> +cpu_l0: cpu@0 {
>>>>>> +device_type = "cpu";
>>>>>> +compatible = "arm,cortex-a53", "arm,armv8";
>>>>>> +reg = <0x0 0x0>;
>>>>>> +enable-method = "psci";
>>>>>> +#cooling-cells = <2>; /* min followed by max */
>>>>>> +clocks = < ARMCLKL>;
>>>>>> +};
>>>>>> +cpu_b0: cpu@100 {
>>>>>> +device_type = "cpu";
>>>>>> +compatible = "arm,cortex-a72", "arm,armv8";
>>>>>> +reg = <0x0 0x100>;
>>>>>> +enable-method = "psci";
>>>>>> +#cooling-cells = <2>; /* min followed by max */
>>>>>> +clocks = < ARMCLKB>;
>>>>>> +};
>>>>>> +
>>>>>> +arm-pmu {
>>>>>> +compatible = "arm,armv8-pmuv3";
>>>>>> +interrupts = ;
>>>>>> +};
>>>>> This is wrong, and must go. There should be a separate node for the PMU
>>>>> of each microarchitecture, with the appropriate compatible string to
>>>>> represent that (see the juno dts).
>>>> You are right. The first version we wrote is:
>>>> pmu_a53 {
>>>> compatible = "arm,cortex-a53-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_l0>,
>>>>  <_l1>,
>>>>  <_l2>,
>>>>  <_l3>;
>>>> };
>>>>
>>>> pmu_a72 {
>>>> compatible = "arm,cortex-a72-pmu";
>>>> interrupts = ;
>>>> interrupt-affinity = <_b0>,
>>>>  <_b1>;
>>>> };
>>>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>>>> well,
>>>> so we have to replace with this implementation.
>>>>> In this case things are messier as the same PPI number is being used
>>>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>>>> should allow us to support that.
>>>> Great! So what we can do right now? Wait this feature, and delete
>>>> arm-pmu node?
>>> I'd rather you have a look at the patches, test them with your HW,
>>> and comment on what doesn't work!
>>>
>>> You can find the patches over there:
>>>
>>> https://lkml.org/lkml/2016/4/11/182
>>>
>>> and on the following branch:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>> irq/percpu-partition
>> I tested these patches. Because our kernel is based on v4.4, so I back
>> port most changes about
>> include/linux/irqdomain.h
>> kernel/irq/irqdomain.c
>> drivers/irqchip/irq-gic-v3.c
>> and change rk3399.dtsi base on your arm,gic-v3.txt:
>>
>>  gic: interrupt-controller@fee0 {
>>  compatible = "arm,gic-v3";
>> -#interrupt-cells = <3>;
>> +#interrupt-cells = <4>;
>>  #address-cells = <2>;
>>  #size-cells = <2>;
>> ...
>> +
>> +ppi-partitions {
>> +part0: interrupt-partition-0 {
>> +affinity = <_l0 _l1 _l2 _l3>;
>> +};
>> +
>> +part1: interrupt-partition-1 {
>> +affinity = <_b0 _b1>;
>> +};
>> +};
>>
>> and change every interrupts from three cells to four cells, such as
>>  saradc: saradc@ff10 {
>> 

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Marc:
On 2016年04月21日 19:30, Marc Zyngier wrote:
> On Thu, 21 Apr 2016 18:47:20 +0800
> "Huang, Tao" <huang...@rock-chips.com> wrote:
>
>> Hi, Mark:
>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>> +  cpu_l0: cpu@0 {
>>>> +  device_type = "cpu";
>>>> +  compatible = "arm,cortex-a53", "arm,armv8";
>>>> +  reg = <0x0 0x0>;
>>>> +  enable-method = "psci";
>>>> +  #cooling-cells = <2>; /* min followed by max */
>>>> +  clocks = < ARMCLKL>;
>>>> +  };
>>>> +  cpu_b0: cpu@100 {
>>>> +  device_type = "cpu";
>>>> +  compatible = "arm,cortex-a72", "arm,armv8";
>>>> +  reg = <0x0 0x100>;
>>>> +  enable-method = "psci";
>>>> +  #cooling-cells = <2>; /* min followed by max */
>>>> +  clocks = < ARMCLKB>;
>>>> +  };
>>>> +
>>>> +  arm-pmu {
>>>> +  compatible = "arm,armv8-pmuv3";
>>>> +  interrupts = ;
>>>> +  };
>>> This is wrong, and must go. There should be a separate node for the PMU
>>> of each microarchitecture, with the appropriate compatible string to
>>> represent that (see the juno dts).
>> You are right. The first version we wrote is:
>> pmu_a53 {
>> compatible = "arm,cortex-a53-pmu";
>> interrupts = ;
>> interrupt-affinity = <_l0>,
>>  <_l1>,
>>  <_l2>,
>>  <_l3>;
>> };
>>
>> pmu_a72 {
>> compatible = "arm,cortex-a72-pmu";
>> interrupts = ;
>> interrupt-affinity = <_b0>,
>>  <_b1>;
>> };
>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>> well,
>> so we have to replace with this implementation.
>>> In this case things are messier as the same PPI number is being used
>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>> should allow us to support that.
>> Great! So what we can do right now? Wait this feature, and delete
>> arm-pmu node?
> I'd rather you have a look at the patches, test them with your HW,
> and comment on what doesn't work!
>
> You can find the patches over there:
>
> https://lkml.org/lkml/2016/4/11/182
>
> and on the following branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> irq/percpu-partition

I tested these patches. Because our kernel is based on v4.4, so I back
port most changes about
include/linux/irqdomain.h
kernel/irq/irqdomain.c
drivers/irqchip/irq-gic-v3.c
and change rk3399.dtsi base on your arm,gic-v3.txt:

 gic: interrupt-controller@fee0 {
 compatible = "arm,gic-v3";
-#interrupt-cells = <3>;
+#interrupt-cells = <4>;
 #address-cells = <2>;
 #size-cells = <2>;
...
+
+ppi-partitions {
+part0: interrupt-partition-0 {
+affinity = <_l0 _l1 _l2 _l3>;
+};
+
+part1: interrupt-partition-1 {
+affinity = <_b0 _b1>;
+};
+};

and change every interrupts from three cells to four cells, such as
 saradc: saradc@ff10 {
 compatible = "rockchip,rk3399-saradc";
 reg = <0x0 0xff10 0x0 0x100>;
-interrupts = ;
+interrupts = ;
 #io-channel-cells = <1>;
 clocks = < SCLK_SARADC>, < PCLK_SARADC>;
 clock-names = "saradc", "apb_pclk";

and pmu define as:
pmu_a53 {
compatible = "arm,cortex-a53-pmu";
interrupts = ;
interrupt-affinity = <_l0>,
 <_l1>,
 <_l2>,
 <_l3>;
};

pmu_a72 {
compatible = "arm,cortex-a72-pmu", "arm,cortex-a57-pmu";
interrupts = ;
interrupt-affinity = <_b0>,
 <_b1>;
};

It can boot. And I test with Android simpleperf stat and perf top, it works!
So these patches work on RK3399.

But as I mentioned, we must change every interrupt in dts, do you think
this is acceptable?
>
> Of course, you'll have to hack a bit in the PMU code to make it
> understand per-PMU affinity together with percpu interrupts, but it
> wouldn't be fun if there was nothing to do...
I don't change drivers/perf/arm_pmu.c, it just work.

Thanks,
Huang Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Marc:
On 2016年04月21日 19:30, Marc Zyngier wrote:
> On Thu, 21 Apr 2016 18:47:20 +0800
> "Huang, Tao"  wrote:
>
>> Hi, Mark:
>> On 2016年04月21日 18:19, Mark Rutland wrote:
>>> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>>>> +  cpu_l0: cpu@0 {
>>>> +  device_type = "cpu";
>>>> +  compatible = "arm,cortex-a53", "arm,armv8";
>>>> +  reg = <0x0 0x0>;
>>>> +  enable-method = "psci";
>>>> +  #cooling-cells = <2>; /* min followed by max */
>>>> +  clocks = < ARMCLKL>;
>>>> +  };
>>>> +  cpu_b0: cpu@100 {
>>>> +  device_type = "cpu";
>>>> +  compatible = "arm,cortex-a72", "arm,armv8";
>>>> +  reg = <0x0 0x100>;
>>>> +  enable-method = "psci";
>>>> +  #cooling-cells = <2>; /* min followed by max */
>>>> +  clocks = < ARMCLKB>;
>>>> +  };
>>>> +
>>>> +  arm-pmu {
>>>> +  compatible = "arm,armv8-pmuv3";
>>>> +  interrupts = ;
>>>> +  };
>>> This is wrong, and must go. There should be a separate node for the PMU
>>> of each microarchitecture, with the appropriate compatible string to
>>> represent that (see the juno dts).
>> You are right. The first version we wrote is:
>> pmu_a53 {
>> compatible = "arm,cortex-a53-pmu";
>> interrupts = ;
>> interrupt-affinity = <_l0>,
>>  <_l1>,
>>  <_l2>,
>>  <_l3>;
>> };
>>
>> pmu_a72 {
>> compatible = "arm,cortex-a72-pmu";
>> interrupts = ;
>> interrupt-affinity = <_b0>,
>>  <_b1>;
>> };
>> but unfortunately, the arm pmu driver do not support PPI in two cluster
>> well,
>> so we have to replace with this implementation.
>>> In this case things are messier as the same PPI number is being used
>>> across clusters. Marc (Cc'd) has been working on PPI partitions, which
>>> should allow us to support that.
>> Great! So what we can do right now? Wait this feature, and delete
>> arm-pmu node?
> I'd rather you have a look at the patches, test them with your HW,
> and comment on what doesn't work!
>
> You can find the patches over there:
>
> https://lkml.org/lkml/2016/4/11/182
>
> and on the following branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> irq/percpu-partition

I tested these patches. Because our kernel is based on v4.4, so I back
port most changes about
include/linux/irqdomain.h
kernel/irq/irqdomain.c
drivers/irqchip/irq-gic-v3.c
and change rk3399.dtsi base on your arm,gic-v3.txt:

 gic: interrupt-controller@fee0 {
 compatible = "arm,gic-v3";
-#interrupt-cells = <3>;
+#interrupt-cells = <4>;
 #address-cells = <2>;
 #size-cells = <2>;
...
+
+ppi-partitions {
+part0: interrupt-partition-0 {
+affinity = <_l0 _l1 _l2 _l3>;
+};
+
+part1: interrupt-partition-1 {
+affinity = <_b0 _b1>;
+};
+};

and change every interrupts from three cells to four cells, such as
 saradc: saradc@ff10 {
 compatible = "rockchip,rk3399-saradc";
 reg = <0x0 0xff10 0x0 0x100>;
-interrupts = ;
+interrupts = ;
 #io-channel-cells = <1>;
 clocks = < SCLK_SARADC>, < PCLK_SARADC>;
 clock-names = "saradc", "apb_pclk";

and pmu define as:
pmu_a53 {
compatible = "arm,cortex-a53-pmu";
interrupts = ;
interrupt-affinity = <_l0>,
 <_l1>,
 <_l2>,
 <_l3>;
};

pmu_a72 {
compatible = "arm,cortex-a72-pmu", "arm,cortex-a57-pmu";
interrupts = ;
interrupt-affinity = <_b0>,
 <_b1>;
};

It can boot. And I test with Android simpleperf stat and perf top, it works!
So these patches work on RK3399.

But as I mentioned, we must change every interrupt in dts, do you think
this is acceptable?
>
> Of course, you'll have to hack a bit in the PMU code to make it
> understand per-PMU affinity together with percpu interrupts, but it
> wouldn't be fun if there was nothing to do...
I don't change drivers/perf/arm_pmu.c, it just work.

Thanks,
Huang Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Doug:
On 2016年04月22日 06:38, Doug Anderson wrote:
>
>>> + i2c1: i2c@ff11 {
>>> + compatible = "rockchip,rk3399-i2c";
>> David respun the rk3399 i2c-support on tuesday, so this and the others below
>> are waiting on Wolfram to take a look.
> I think it can work with the rk3288-i2c as a fallback, at least for
> low speed stuff, right?  Should this be:
>
> compatible = "rockchip,rk3399-i2c", "rockchip,rk3288-i2c"
>
> Looks like that was done for rk3368.
No. For RK3399 I2C controller:
The I2C controller uses the APB clock/clk_i2c as the working clock. The
APB clock will determine the I2C bus clock, clk_i2c is the function clk,
up to 200MHz.
Chips such as RK3288/3368 only uses APB clock. So old driver do not work
on RK3399.

Thanks,
Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Doug:
On 2016年04月22日 06:38, Doug Anderson wrote:
>
>>> + i2c1: i2c@ff11 {
>>> + compatible = "rockchip,rk3399-i2c";
>> David respun the rk3399 i2c-support on tuesday, so this and the others below
>> are waiting on Wolfram to take a look.
> I think it can work with the rk3288-i2c as a fallback, at least for
> low speed stuff, right?  Should this be:
>
> compatible = "rockchip,rk3399-i2c", "rockchip,rk3288-i2c"
>
> Looks like that was done for rk3368.
No. For RK3399 I2C controller:
The I2C controller uses the APB clock/clk_i2c as the working clock. The
APB clock will determine the I2C bus clock, clk_i2c is the function clk,
up to 200MHz.
Chips such as RK3288/3368 only uses APB clock. So old driver do not work
on RK3399.

Thanks,
Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月21日 18:19, Mark Rutland wrote:
> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>> +cpu_l0: cpu@0 {
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a53", "arm,armv8";
>> +reg = <0x0 0x0>;
>> +enable-method = "psci";
>> +#cooling-cells = <2>; /* min followed by max */
>> +clocks = < ARMCLKL>;
>> +};
>> +cpu_b0: cpu@100 {
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a72", "arm,armv8";
>> +reg = <0x0 0x100>;
>> +enable-method = "psci";
>> +#cooling-cells = <2>; /* min followed by max */
>> +clocks = < ARMCLKB>;
>> +};
>> +
>> +arm-pmu {
>> +compatible = "arm,armv8-pmuv3";
>> +interrupts = ;
>> +};
> This is wrong, and must go. There should be a separate node for the PMU
> of each microarchitecture, with the appropriate compatible string to
> represent that (see the juno dts).
You are right. The first version we wrote is:
pmu_a53 {
compatible = "arm,cortex-a53-pmu";
interrupts = ;
interrupt-affinity = <_l0>,
 <_l1>,
 <_l2>,
 <_l3>;
};

pmu_a72 {
compatible = "arm,cortex-a72-pmu";
interrupts = ;
interrupt-affinity = <_b0>,
 <_b1>;
};
but unfortunately, the arm pmu driver do not support PPI in two cluster
well,
so we have to replace with this implementation.
> In this case things are messier as the same PPI number is being used
> across clusters. Marc (Cc'd) has been working on PPI partitions, which
> should allow us to support that.
Great! So what we can do right now? Wait this feature, and delete
arm-pmu node?

Thanks,
Huang, Tao

Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

[Huang, Tao]

Hi, Mark:
On 2016年04月21日 18:19, Mark Rutland wrote:
> On Thu, Apr 21, 2016 at 11:58:12AM +0800, Jianqun Xu wrote:
>> +cpu_l0: cpu@0 {
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a53", "arm,armv8";
>> +reg = <0x0 0x0>;
>> +enable-method = "psci";
>> +#cooling-cells = <2>; /* min followed by max */
>> +clocks = < ARMCLKL>;
>> +};
>> +cpu_b0: cpu@100 {
>> +device_type = "cpu";
>> +compatible = "arm,cortex-a72", "arm,armv8";
>> +reg = <0x0 0x100>;
>> +enable-method = "psci";
>> +#cooling-cells = <2>; /* min followed by max */
>> +clocks = < ARMCLKB>;
>> +};
>> +
>> +arm-pmu {
>> +compatible = "arm,armv8-pmuv3";
>> +interrupts = ;
>> +};
> This is wrong, and must go. There should be a separate node for the PMU
> of each microarchitecture, with the appropriate compatible string to
> represent that (see the juno dts).
You are right. The first version we wrote is:
pmu_a53 {
compatible = "arm,cortex-a53-pmu";
interrupts = ;
interrupt-affinity = <_l0>,
 <_l1>,
 <_l2>,
 <_l3>;
};

pmu_a72 {
compatible = "arm,cortex-a72-pmu";
interrupts = ;
interrupt-affinity = <_b0>,
 <_b1>;
};
but unfortunately, the arm pmu driver do not support PPI in two cluster
well,
so we have to replace with this implementation.
> In this case things are messier as the same PPI number is being used
> across clusters. Marc (Cc'd) has been working on PPI partitions, which
> should allow us to support that.
Great! So what we can do right now? Wait this feature, and delete
arm-pmu node?

Thanks,
Huang, Tao

Re: [PATCH] spi/rockchip: Make sure spi clk is on in rockchip_spi_set_cs

[Huang, Tao]

On 2016年02月26日 09:58, Mark Brown wrote:
> 
>> Another way to solve this bug is add runtime PM support while spi setup.
>> Some other chips may have some problem, for example mt65xx and orion,
>> which access hardware register too.
> 
> No, this is telling you you're doing something wrong - setup() might be
> called while another transfer is in progress so you shouldn't be
> changing the hardware setup (see Documentation/spi/spi-summary).  This
> means that you normally shouldn't be writing to registers there.  Moving
> this to prepare_message() instead is probably what you want.
> 
You misunderstand me. I talk about spi_setup, as
Documentation/spi/spi-summary, which would normally be called from
probe() before the first I/O is done to the device.
spi_setup will call spi_set_cs(spi, false), which introduced with commit
1a7b7ee72c21 ("spi: Ensure that CS line is in non-active state after
spi_setup()"). And spi_set_cs will call spi->master->set_cs, and set_cs
callback will access register. Without clk enable, I believe some
drivers will failed to run.

Best Regards,

Huang, Tao

Re: [PATCH] spi/rockchip: Make sure spi clk is on in rockchip_spi_set_cs

[Huang, Tao]

On 2016年02月26日 09:58, Mark Brown wrote:
> 
>> Another way to solve this bug is add runtime PM support while spi setup.
>> Some other chips may have some problem, for example mt65xx and orion,
>> which access hardware register too.
> 
> No, this is telling you you're doing something wrong - setup() might be
> called while another transfer is in progress so you shouldn't be
> changing the hardware setup (see Documentation/spi/spi-summary).  This
> means that you normally shouldn't be writing to registers there.  Moving
> this to prepare_message() instead is probably what you want.
> 
You misunderstand me. I talk about spi_setup, as
Documentation/spi/spi-summary, which would normally be called from
probe() before the first I/O is done to the device.
spi_setup will call spi_set_cs(spi, false), which introduced with commit
1a7b7ee72c21 ("spi: Ensure that CS line is in non-active state after
spi_setup()"). And spi_set_cs will call spi->master->set_cs, and set_cs
callback will access register. Without clk enable, I believe some
drivers will failed to run.

Best Regards,

Huang, Tao

Re: [PATCH] spi/rockchip: Make sure spi clk is on in rockchip_spi_set_cs

[Huang, Tao]

Hi, Mark:

Another way to solve this bug is add runtime PM support while spi setup.
Some other chips may have some problem, for example mt65xx and orion,
which access hardware register too.

On 2016年02月24日 18:00, Huibin Hong wrote:
> Rockchip_spi_set_cs could be called by spi_setup, but
> spi_setup may be called by device driver after runtime suspend.
> Then the spi clock is closed, rockchip_spi_set_cs may access the
> spi registers, which causes cpu block in some socs.
> 
> Fixes: 64e36824b32 ("spi/rockchip: add driver for Rockchip RK3xxx")
> Cc: Addy Ke 
> Cc: shawn.lin 
> Signed-off-by: Huibin Hong 
> ---
> 
>  drivers/spi/spi-rockchip.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 79a8bc4..035767c 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -265,7 +265,10 @@ static inline u32 rx_max(struct rockchip_spi *rs)
>  static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
>  {
>   u32 ser;
> - struct rockchip_spi *rs = spi_master_get_devdata(spi->master);
> + struct spi_master *master = spi->master;
> + struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> + pm_runtime_get_sync(rs->dev);
> 
>   ser = readl_relaxed(rs->regs + ROCKCHIP_SPI_SER) & SER_MASK;
> 
> @@ -290,6 +293,8 @@ static void rockchip_spi_set_cs(struct spi_device *spi, 
> bool enable)
>   ser &= ~(1 << spi->chip_select);
> 
>   writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER);
> +
> + pm_runtime_put_sync(rs->dev);
>  }
> 
>  static int rockchip_spi_prepare_message(struct spi_master *master,
> --
> 1.9.1
> 
> 
> 
> ___
> Linux-rockchip mailing list
> linux-rockc...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
> 

Re: [PATCH] spi/rockchip: Make sure spi clk is on in rockchip_spi_set_cs

[Huang, Tao]

Hi, Mark:

Another way to solve this bug is add runtime PM support while spi setup.
Some other chips may have some problem, for example mt65xx and orion,
which access hardware register too.

On 2016年02月24日 18:00, Huibin Hong wrote:
> Rockchip_spi_set_cs could be called by spi_setup, but
> spi_setup may be called by device driver after runtime suspend.
> Then the spi clock is closed, rockchip_spi_set_cs may access the
> spi registers, which causes cpu block in some socs.
> 
> Fixes: 64e36824b32 ("spi/rockchip: add driver for Rockchip RK3xxx")
> Cc: Addy Ke 
> Cc: shawn.lin 
> Signed-off-by: Huibin Hong 
> ---
> 
>  drivers/spi/spi-rockchip.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index 79a8bc4..035767c 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -265,7 +265,10 @@ static inline u32 rx_max(struct rockchip_spi *rs)
>  static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
>  {
>   u32 ser;
> - struct rockchip_spi *rs = spi_master_get_devdata(spi->master);
> + struct spi_master *master = spi->master;
> + struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> + pm_runtime_get_sync(rs->dev);
> 
>   ser = readl_relaxed(rs->regs + ROCKCHIP_SPI_SER) & SER_MASK;
> 
> @@ -290,6 +293,8 @@ static void rockchip_spi_set_cs(struct spi_device *spi, 
> bool enable)
>   ser &= ~(1 << spi->chip_select);
> 
>   writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER);
> +
> + pm_runtime_put_sync(rs->dev);
>  }
> 
>  static int rockchip_spi_prepare_message(struct spi_master *master,
> --
> 1.9.1
> 
> 
> 
> ___
> Linux-rockchip mailing list
> linux-rockc...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
> 

Re: [PATCH] i2c: rk3x: init module as subsys call

[Huang, Tao]

Hi, Heiko:

On 2016年01月05日 16:00, Heiko Stuebner wrote:
> Hi Tao,
> 
> Am Dienstag, 5. Januar 2016, 15:42:32 schrieb Huang, Tao:

>> I don't think this is a good idea. This will trigger a lots of init call
>> failed. Before pmic init, all i2c device driver transmit will failed,
>> and because i2c is slow bus, and i2c transmit may failed by other
>> reasons, so the i2c driver and i2c device driver will try many times to
>> make sure the transmit completion. These unnecessary transmission will
>> make Linux boot very slow.
> 
> In general, the slowdown won't be _this_ much if touchscreen drivers need 
> one deferral-round before i2c is available. I'm also only pointing out 
> things I remember from the last time this came up. 
> 
> rk3x-i2c even was here already:
> http://www.spinics.net/lists/linux-i2c/msg16680.html

OK. I don't agree with the rule, but we will follow it.

> 
> 
>> I2C bus should be subsys, and we can easy resolve this problem, why we
>> depends on a complicated and slow implementation?
> 
> because it's the only safe way to do that. Because now you need i2c-init at 
> subsys-init time, some months later some other soc may need some other 
> ordering, especially needing i2c-init later/earlier.
> 
> Going through the deferral mechanism is the only way currently available to 
> actually make this work on all socs.
> 
> Tomeu from Collabora is working on some better scheme to optimize device 
> probing order but it looks like this may be a bit off still.
> 
> 
>>> Your touchscreen will have a "xyz-supply" property and I think the
>>> regulator-framework should already emit a -EPROBE_DEFER at
>>> regulator_get,
>>> when the regulator is specified but not available yet.
>>
>> Unfortunately, mostly driver do not support regulator api. They are
>> suppose power is on.
> 
> Having touchscreen drivers support its proper supply-regulators is not 
> rocket science ;-) [0] , so I would consider this a bug in the touchscreen 
> driver itself.

I don't just talk about touch screen driver, most i2c device driver such
as input sensor/camera/rtc/battery will suffer. So people will see their
drivers do not work or slow down on rk3368 platform :(

Thanks!
Huang, Tao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c

[Huang, Tao]

Hi, Alessandro:

On 2016年01月04日 21:59, Alessandro Zummo wrote:
> On Mon, 4 Jan 2016 10:45:46 +0100
> Alexandre Belloni  wrote:
> 
>> I'm not sure it is useful to do that renaming. It is usual to have one
>> driver that supports multiple chips named with the forst chip it
>> supported.
>>
>> Also, what would happen if for example rk855 is not compatible at all
>> with the previous implementations?
> 
>  Alexandre is absolutely right. There's no need to rename a driver,
>  it would just piss off people who are used to that name and
>  have it in their scripts. Like when your eth0 gets renamed
>  to some obscure enXXX .
> 

You and Alexandre are right. The rename is just make the driver more
readable, i.e. let people know this driver suit for more PMIC no just
rk808. In fact, I don't care the name is rk808 or rk8xx.

The key change of this patch is try to dis-coupling rk808 driver and RTC
driver. Because of register offset and function is vary between
different PMIC, we believe it is hard to write one PMIC driver to suit
all PMIC. So we hope RTC driver can share between all PMIC from rockchip.

Please review this code:

-static int rk808_rtc_probe(struct platform_device *pdev)
+static int rk8xx_rtc_probe(struct platform_device *pdev)
 {
-   struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
...
+   struct i2c_client *client = to_i2c_client(pdev->dev.parent);

...

-   rk808_rtc->rk808 = rk808;
+   rk8xx_rtc->regmap = devm_regmap_init_i2c(client,
+_rtc_regmap_config);
...
+   rk8xx_rtc->i2c = client;

Old driver have struct rk808 pointer, which defined on
include/linux/mfd/rk808.h
If we write new PMIC driver, for example rk818, define a new struct
rk818. How can we get this pointer from RTC driver?

So another way to solve this problem is introduce common struct share
between all PMIC driver. For example rk8xx.

We solve this problem by create new regmap to access PMIC. As I say
before, it make RTC driver independent of PMIC driver. Do you agree this
change?

Thanks!
Huang, Tao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] rtc: rk808: rename rtc-rk808.c to rtc-rk8xx.c

[Huang, Tao]

Hi, Alessandro:

On 2016年01月04日 21:59, Alessandro Zummo wrote:
> On Mon, 4 Jan 2016 10:45:46 +0100
> Alexandre Belloni <alexandre.bell...@free-electrons.com> wrote:
> 
>> I'm not sure it is useful to do that renaming. It is usual to have one
>> driver that supports multiple chips named with the forst chip it
>> supported.
>>
>> Also, what would happen if for example rk855 is not compatible at all
>> with the previous implementations?
> 
>  Alexandre is absolutely right. There's no need to rename a driver,
>  it would just piss off people who are used to that name and
>  have it in their scripts. Like when your eth0 gets renamed
>  to some obscure enXXX .
> 

You and Alexandre are right. The rename is just make the driver more
readable, i.e. let people know this driver suit for more PMIC no just
rk808. In fact, I don't care the name is rk808 or rk8xx.

The key change of this patch is try to dis-coupling rk808 driver and RTC
driver. Because of register offset and function is vary between
different PMIC, we believe it is hard to write one PMIC driver to suit
all PMIC. So we hope RTC driver can share between all PMIC from rockchip.

Please review this code:

-static int rk808_rtc_probe(struct platform_device *pdev)
+static int rk8xx_rtc_probe(struct platform_device *pdev)
 {
-   struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
...
+   struct i2c_client *client = to_i2c_client(pdev->dev.parent);

...

-   rk808_rtc->rk808 = rk808;
+   rk8xx_rtc->regmap = devm_regmap_init_i2c(client,
+_rtc_regmap_config);
...
+   rk8xx_rtc->i2c = client;

Old driver have struct rk808 pointer, which defined on
include/linux/mfd/rk808.h
If we write new PMIC driver, for example rk818, define a new struct
rk818. How can we get this pointer from RTC driver?

So another way to solve this problem is introduce common struct share
between all PMIC driver. For example rk8xx.

We solve this problem by create new regmap to access PMIC. As I say
before, it make RTC driver independent of PMIC driver. Do you agree this
change?

Thanks!
Huang, Tao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c: rk3x: init module as subsys call

[Huang, Tao]

Hi, Heiko:

On 2016年01月05日 16:00, Heiko Stuebner wrote:
> Hi Tao,
> 
> Am Dienstag, 5. Januar 2016, 15:42:32 schrieb Huang, Tao:

>> I don't think this is a good idea. This will trigger a lots of init call
>> failed. Before pmic init, all i2c device driver transmit will failed,
>> and because i2c is slow bus, and i2c transmit may failed by other
>> reasons, so the i2c driver and i2c device driver will try many times to
>> make sure the transmit completion. These unnecessary transmission will
>> make Linux boot very slow.
> 
> In general, the slowdown won't be _this_ much if touchscreen drivers need 
> one deferral-round before i2c is available. I'm also only pointing out 
> things I remember from the last time this came up. 
> 
> rk3x-i2c even was here already:
> http://www.spinics.net/lists/linux-i2c/msg16680.html

OK. I don't agree with the rule, but we will follow it.

> 
> 
>> I2C bus should be subsys, and we can easy resolve this problem, why we
>> depends on a complicated and slow implementation?
> 
> because it's the only safe way to do that. Because now you need i2c-init at 
> subsys-init time, some months later some other soc may need some other 
> ordering, especially needing i2c-init later/earlier.
> 
> Going through the deferral mechanism is the only way currently available to 
> actually make this work on all socs.
> 
> Tomeu from Collabora is working on some better scheme to optimize device 
> probing order but it looks like this may be a bit off still.
> 
> 
>>> Your touchscreen will have a "xyz-supply" property and I think the
>>> regulator-framework should already emit a -EPROBE_DEFER at
>>> regulator_get,
>>> when the regulator is specified but not available yet.
>>
>> Unfortunately, mostly driver do not support regulator api. They are
>> suppose power is on.
> 
> Having touchscreen drivers support its proper supply-regulators is not 
> rocket science ;-) [0] , so I would consider this a bug in the touchscreen 
> driver itself.

I don't just talk about touch screen driver, most i2c device driver such
as input sensor/camera/rtc/battery will suffer. So people will see their
drivers do not work or slow down on rk3368 platform :(

Thanks!
Huang, Tao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c: rk3x: init module as subsys call

[Huang, Tao]

Hi, Heiko:

On 2016年01月05日 15:02, Heiko Stuebner wrote:
> Hi Jianqun,
> 
> Am Dienstag, 5. Januar 2016, 11:02:18 schrieb jianqun.xu:
>> From: Xu Jianqun 
>>
>> There is a requirement from pmic device, which is on the i2c bus,
>> that the pmic needs to be called earlier then devices powered by
>> the outputs of the pmic, if not, the devices maybe fail to probe.
>>
>> For example, a pmic on i2c0, and touchscreen device on i2c2,
>> i2c0: - pmic(rk818)
>> i2c2: - ts(gt911), powered by rk818 on i2c0
>>
>> The problem will happen if the i2c2 node in dts file is ordered
>> before i2c0 node, then ts(gt911) will be probed before pmic(rk818),
>> since the power from the pmic(rk818) for ts(gt911) hasn't enabled,
>> so ts(gt911) will fail to probe due to the failure of i2c test.
>>
>> But if we set the i2c0 node before i2c2, there is no this issue.
>>
>> The stable way to make sure that pmic can be intalized before other
>> peripher devices is to make the pmic module be subsys call, the i2c
>> module need to be subsys call firstly.
> 
> I do believe that came up in the past already and the direction from then 
> was (and most likely still is) that drivers should make use of the probe-
> deferral mechanism instead of wiggling with the initcall ordering.


I don't think this is a good idea. This will trigger a lots of init call
failed. Before pmic init, all i2c device driver transmit will failed,
and because i2c is slow bus, and i2c transmit may failed by other
reasons, so the i2c driver and i2c device driver will try many times to
make sure the transmit completion. These unnecessary transmission will
make Linux boot very slow.

I2C bus should be subsys, and we can easy resolve this problem, why we
depends on a complicated and slow implementation?

> 
> Your touchscreen will have a "xyz-supply" property and I think the 
> regulator-framework should already emit a -EPROBE_DEFER at regulator_get, 
> when the regulator is specified but not available yet.

Unfortunately, mostly driver do not support regulator api. They are
suppose power is on.

Thanks.
Huang, Tao

> 
> 
> Heiko
> 
>>
>> Signed-off-by: Xu Jianqun 
>> ---
>>  drivers/i2c/busses/i2c-rk3x.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>> index c1935eb..00e5959 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -1037,7 +1037,17 @@ static struct platform_driver rk3x_i2c_driver = {
>>  },
>>  };
>>
>> -module_platform_driver(rk3x_i2c_driver);
>> +static int __init rk3x_i2c_init(void)
>> +{
>> +return platform_driver_register(_i2c_driver);
>> +}
>> +subsys_initcall(rk3x_i2c_init);
>> +
>> +static void __exit rk3x_i2c_exit(void)
>> +{
>> +platform_driver_unregister(_i2c_driver);
>> +}
>> +module_exit(rk3x_i2c_exit);
>>
>>  MODULE_DESCRIPTION("Rockchip RK3xxx I2C Bus driver");
>>  MODULE_AUTHOR("Max Schwarz ");
> 
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c: rk3x: init module as subsys call

[Huang, Tao]

Hi, Heiko:

On 2016年01月05日 15:02, Heiko Stuebner wrote:
> Hi Jianqun,
> 
> Am Dienstag, 5. Januar 2016, 11:02:18 schrieb jianqun.xu:
>> From: Xu Jianqun <jay...@rock-chips.com>
>>
>> There is a requirement from pmic device, which is on the i2c bus,
>> that the pmic needs to be called earlier then devices powered by
>> the outputs of the pmic, if not, the devices maybe fail to probe.
>>
>> For example, a pmic on i2c0, and touchscreen device on i2c2,
>> i2c0: - pmic(rk818)
>> i2c2: - ts(gt911), powered by rk818 on i2c0
>>
>> The problem will happen if the i2c2 node in dts file is ordered
>> before i2c0 node, then ts(gt911) will be probed before pmic(rk818),
>> since the power from the pmic(rk818) for ts(gt911) hasn't enabled,
>> so ts(gt911) will fail to probe due to the failure of i2c test.
>>
>> But if we set the i2c0 node before i2c2, there is no this issue.
>>
>> The stable way to make sure that pmic can be intalized before other
>> peripher devices is to make the pmic module be subsys call, the i2c
>> module need to be subsys call firstly.
> 
> I do believe that came up in the past already and the direction from then 
> was (and most likely still is) that drivers should make use of the probe-
> deferral mechanism instead of wiggling with the initcall ordering.


I don't think this is a good idea. This will trigger a lots of init call
failed. Before pmic init, all i2c device driver transmit will failed,
and because i2c is slow bus, and i2c transmit may failed by other
reasons, so the i2c driver and i2c device driver will try many times to
make sure the transmit completion. These unnecessary transmission will
make Linux boot very slow.

I2C bus should be subsys, and we can easy resolve this problem, why we
depends on a complicated and slow implementation?

> 
> Your touchscreen will have a "xyz-supply" property and I think the 
> regulator-framework should already emit a -EPROBE_DEFER at regulator_get, 
> when the regulator is specified but not available yet.

Unfortunately, mostly driver do not support regulator api. They are
suppose power is on.

Thanks.
Huang, Tao

> 
> 
> Heiko
> 
>>
>> Signed-off-by: Xu Jianqun <jay...@rock-chips.com>
>> ---
>>  drivers/i2c/busses/i2c-rk3x.c | 12 +++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>> index c1935eb..00e5959 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -1037,7 +1037,17 @@ static struct platform_driver rk3x_i2c_driver = {
>>  },
>>  };
>>
>> -module_platform_driver(rk3x_i2c_driver);
>> +static int __init rk3x_i2c_init(void)
>> +{
>> +return platform_driver_register(_i2c_driver);
>> +}
>> +subsys_initcall(rk3x_i2c_init);
>> +
>> +static void __exit rk3x_i2c_exit(void)
>> +{
>> +platform_driver_unregister(_i2c_driver);
>> +}
>> +module_exit(rk3x_i2c_exit);
>>
>>  MODULE_DESCRIPTION("Rockchip RK3xxx I2C Bus driver");
>>  MODULE_AUTHOR("Max Schwarz <max.schw...@online.de>");
> 
> 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH 0/1] Fix the "hard LOCKUP" when running a heavy loading

[Huang, Tao]

Hello Russell:

在 2015年11月03日 19:14, Russell King - ARM Linux 写道:
> On Tue, Nov 03, 2015 at 04:10:08PM +0800, Caesar Wang wrote:
>> As the Russell said:
>> "in other words, which can be handled by updating a control register in
>> the firmware or boot loader"
>> Maybe the better solution is in firmware.
> 
> The full quote is:
> 
> "I think we're at the point where we start insisting that workarounds
> which are simple enable/disable feature bit operations (in other words,
> which can be handled by updating a control register in the firmware or
> boot loader) must be done that way, and we are not going to add such
> workarounds to the kernel anymore."
> 
> The position hasn't changed.  Workarounds such as this should be handled
> in the firmware/boot loader before control is passed to the kernel.
> 
> The reason is very simple: if the C compiler can generate code which
> triggers the bug, it can generate code which triggers the bug in the
> boot loader.  So, the only place such workarounds can be done is before
> any C code gets executed.  Putting such workarounds in the kernel is
> completely inappropriate.

I agree with your reason for CPU0. But how about CPU1~3 if we don't use
any firmware such as ARM Trusted Firmware to take control of CPU power
on? If the CPU1~3 will run on Linux when its first instruction is running?

BTW I don't want to argue with you the workaround is right or wrong
because I know the errata just happen on r0p0 not r0p1.

> 
> Sorry, I'm not going to accept this workaround into the kernel.

It seems we should introduce some code outside the kernel to do such
initialization?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH 0/1] Fix the "hard LOCKUP" when running a heavy loading

[Huang, Tao]

Hello Russell:

在 2015年11月03日 19:14, Russell King - ARM Linux 写道:
> On Tue, Nov 03, 2015 at 04:10:08PM +0800, Caesar Wang wrote:
>> As the Russell said:
>> "in other words, which can be handled by updating a control register in
>> the firmware or boot loader"
>> Maybe the better solution is in firmware.
> 
> The full quote is:
> 
> "I think we're at the point where we start insisting that workarounds
> which are simple enable/disable feature bit operations (in other words,
> which can be handled by updating a control register in the firmware or
> boot loader) must be done that way, and we are not going to add such
> workarounds to the kernel anymore."
> 
> The position hasn't changed.  Workarounds such as this should be handled
> in the firmware/boot loader before control is passed to the kernel.
> 
> The reason is very simple: if the C compiler can generate code which
> triggers the bug, it can generate code which triggers the bug in the
> boot loader.  So, the only place such workarounds can be done is before
> any C code gets executed.  Putting such workarounds in the kernel is
> completely inappropriate.

I agree with your reason for CPU0. But how about CPU1~3 if we don't use
any firmware such as ARM Trusted Firmware to take control of CPU power
on? If the CPU1~3 will run on Linux when its first instruction is running?

BTW I don't want to argue with you the workaround is right or wrong
because I know the errata just happen on r0p0 not r0p1.

> 
> Sorry, I'm not going to accept this workaround into the kernel.

It seems we should introduce some code outside the kernel to do such
initialization?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: rockchip: restore dapswjdp after suspend

[Huang, Tao]

Hi, Doug:

在 2015年05月21日 04:34, Doug Anderson 写道:
> In the commit (0ea001d ARM: rockchip: disable dapswjdp during suspend)
> we made the assumption that we didn't need to restore dapswjdp after
> suspend because "the MASKROM will enable it back".
> 
> It turns out that's not a safe assumption.  In some cases (pending
> interrupts) it's possible that the WFI might act as a no-op and the
> MaskROM will never run.


I don't think this can happen. It seems we set PMU_GLOBAL_INT_DISABLE
bit, which means in power off flow, the IRQ will not accepted until the
ARM is power off. Do you see the SGRF_CPU_CON0[0] is 0 after resume?

Anyway, restore the value is okay, which make the code more symmetrically



  Since we're changing the bit, we should
> restore it ourselves.
> 
> Signed-off-by: Doug Anderson 
> ---
>  arch/arm/mach-rockchip/pm.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
> index b0dcbe2..a7be465 100644
> --- a/arch/arm/mach-rockchip/pm.c
> +++ b/arch/arm/mach-rockchip/pm.c
> @@ -48,6 +48,7 @@ static struct regmap *sgrf_regmap;
>  
>  static u32 rk3288_pmu_pwr_mode_con;
>  static u32 rk3288_sgrf_soc_con0;
> +static u32 rk3288_sgrf_cpu_con0;
>  
>  static inline u32 rk3288_l2_config(void)
>  {
> @@ -70,6 +71,7 @@ static void rk3288_slp_mode_set(int level)
>  {
>   u32 mode_set, mode_set1;
>  
> + regmap_read(sgrf_regmap, RK3288_SGRF_CPU_CON0, _sgrf_cpu_con0);
>   regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, _sgrf_soc_con0);
>  
>   regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON,
> @@ -129,6 +131,9 @@ static void rk3288_slp_mode_set(int level)
>  
>  static void rk3288_slp_mode_set_resume(void)
>  {
> + regmap_write(sgrf_regmap, RK3288_SGRF_CPU_CON0,
> +  rk3288_sgrf_cpu_con0 | SGRF_DAPDEVICEEN_WRITE);
> +
>   regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON,
>rk3288_pmu_pwr_mode_con);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ARM: rockchip: restore dapswjdp after suspend

[Huang, Tao]

Hi, Doug:

在 2015年05月21日 04:34, Doug Anderson 写道:
 In the commit (0ea001d ARM: rockchip: disable dapswjdp during suspend)
 we made the assumption that we didn't need to restore dapswjdp after
 suspend because the MASKROM will enable it back.
 
 It turns out that's not a safe assumption.  In some cases (pending
 interrupts) it's possible that the WFI might act as a no-op and the
 MaskROM will never run.


I don't think this can happen. It seems we set PMU_GLOBAL_INT_DISABLE
bit, which means in power off flow, the IRQ will not accepted until the
ARM is power off. Do you see the SGRF_CPU_CON0[0] is 0 after resume?

Anyway, restore the value is okay, which make the code more symmetrically



  Since we're changing the bit, we should
 restore it ourselves.
 
 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
  arch/arm/mach-rockchip/pm.c | 5 +
  1 file changed, 5 insertions(+)
 
 diff --git a/arch/arm/mach-rockchip/pm.c b/arch/arm/mach-rockchip/pm.c
 index b0dcbe2..a7be465 100644
 --- a/arch/arm/mach-rockchip/pm.c
 +++ b/arch/arm/mach-rockchip/pm.c
 @@ -48,6 +48,7 @@ static struct regmap *sgrf_regmap;
  
  static u32 rk3288_pmu_pwr_mode_con;
  static u32 rk3288_sgrf_soc_con0;
 +static u32 rk3288_sgrf_cpu_con0;
  
  static inline u32 rk3288_l2_config(void)
  {
 @@ -70,6 +71,7 @@ static void rk3288_slp_mode_set(int level)
  {
   u32 mode_set, mode_set1;
  
 + regmap_read(sgrf_regmap, RK3288_SGRF_CPU_CON0, rk3288_sgrf_cpu_con0);
   regmap_read(sgrf_regmap, RK3288_SGRF_SOC_CON0, rk3288_sgrf_soc_con0);
  
   regmap_read(pmu_regmap, RK3288_PMU_PWRMODE_CON,
 @@ -129,6 +131,9 @@ static void rk3288_slp_mode_set(int level)
  
  static void rk3288_slp_mode_set_resume(void)
  {
 + regmap_write(sgrf_regmap, RK3288_SGRF_CPU_CON0,
 +  rk3288_sgrf_cpu_con0 | SGRF_DAPDEVICEEN_WRITE);
 +
   regmap_write(pmu_regmap, RK3288_PMU_PWRMODE_CON,
rk3288_pmu_pwr_mode_con);
  
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/4] thermal: rockchip: add driver for thermal

[Huang Tao]

Dear Eduardo Bezerra Valentin:

在 2014年08月29日 19:39, edubez...@gmail.com 写道:
> Hello Zhao,
>
> On Thu, Aug 28, 2014 at 9:54 PM, 赵仪峰  wrote:
>> Hi Heiko，
>>
>>The TS-ADC on RK3288 has two component, a tsadc and a tsadc controller.
>> The tsadc controller is similar like the thermal manager unit on other SOCs.
>> We followed the naming of 3066, but not named as the Thermal Manager.
>>
>>Moreover,there is only one set of apb registers to access the tsadc
>> controller,and the tsadc is controlled by the tsadc controller,could not
>> access directly. If we write a general tsadc driver by accessed tsadc
>> controller registers, and it hardly to write a driver for the tsadc
>> controller.
> As suggested by Arnd, you can use the generic driver as interface
> between thermal framework and IIO layer. The driver you are going to
> write to your ADC is going to be in the IIO subsystem. The only
> difference is that the generic driver would work for any ADC driver in
> the IIO subsystem.
> https://lkml.org/lkml/2014/2/5/810
>
> In fact, there is already a generic driver. We just need to get it up
> to date.  I see some testing has been already done, and results sound
> promising.
This iio_thermal driver only support get temperature from the ADC IIO
channel.
It dose not support any advanced feature such as configure the
temperature when system should be reset.
So if we adapt it, we will access the register form both TSADC IIO
driver and  thermal driver.
I don't thank this is a good ideal.

In fact, we should ignore the "ADC" stuff. This sensor use ADC internal,
but it is transparent to software, especially in automatic mode.
>
>>So, I do not agree to write a generic adc driver for the rk3288-tsadc.
>>
>> 
>> Yifeng Zhao
>>
>>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi,

在 2014年08月29日 19:22, Mark Rutland 写道:
>> 在 2014年08月28日 23:11, Mark Rutland 写道:
>>> To clarify: if there are low power states that the CPU can enter where
>>> we lose state, then this patch isn't correct.
>> Right now, the software of RK3288 SoC only support CPU hotplug
>> (cpu_on/off) and power off all CPUs on suspend.
> Sure, but that's a Linux implementation detail rather than a fixed
> property of the hardware. Given those states exist, the "always-on"
> property is not appropriate.
>
>> We do not implement cpuidle to power off CPU. Do you think we should
>> introduce a broadcast timer?
> If one is present, yes. 
>
>> On our early kernel, I never see any interrupt on a broadcast timer
>> (yes, we implement it with a external timer).
> That's fine; Linux doesn't need to use it just yet. However, when we
> want to use low power states later, it will be necessary to enable
> placing all CPUS into a low power state.
>
> If your external timer is already supported by an existing driver, there
> is no reason not to add it now.
>
>>> A more general approach would be to enable the broadcast hrtimer for
>>> arm, as has been done for arm64.
>> Yes. I think it should be done by arm framework.
> Patches welcome.
>
> I also think it would make sense to enable this for arm.
>
> Thanks,
> Mark.
>
>
Okay, so this patch is wrong as I expected.
Thank you!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi,

在 2014年08月29日 19:22, Mark Rutland 写道:
 在 2014年08月28日 23:11, Mark Rutland 写道:
 To clarify: if there are low power states that the CPU can enter where
 we lose state, then this patch isn't correct.
 Right now, the software of RK3288 SoC only support CPU hotplug
 (cpu_on/off) and power off all CPUs on suspend.
 Sure, but that's a Linux implementation detail rather than a fixed
 property of the hardware. Given those states exist, the always-on
 property is not appropriate.

 We do not implement cpuidle to power off CPU. Do you think we should
 introduce a broadcast timer?
 If one is present, yes. 

 On our early kernel, I never see any interrupt on a broadcast timer
 (yes, we implement it with a external timer).
 That's fine; Linux doesn't need to use it just yet. However, when we
 want to use low power states later, it will be necessary to enable
 placing all CPUS into a low power state.

 If your external timer is already supported by an existing driver, there
 is no reason not to add it now.

 A more general approach would be to enable the broadcast hrtimer for
 arm, as has been done for arm64.
 Yes. I think it should be done by arm framework.
 Patches welcome.

 I also think it would make sense to enable this for arm.

 Thanks,
 Mark.


Okay, so this patch is wrong as I expected.
Thank you!

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 1/4] thermal: rockchip: add driver for thermal

[Huang Tao]

Dear Eduardo Bezerra Valentin:

在 2014年08月29日 19:39, edubez...@gmail.com 写道:
 Hello Zhao,

 On Thu, Aug 28, 2014 at 9:54 PM, 赵仪峰 z...@rock-chips.com wrote:
 Hi Heiko，

The TS-ADC on RK3288 has two component, a tsadc and a tsadc controller.
 The tsadc controller is similar like the thermal manager unit on other SOCs.
 We followed the naming of 3066, but not named as the Thermal Manager.

Moreover,there is only one set of apb registers to access the tsadc
 controller,and the tsadc is controlled by the tsadc controller,could not
 access directly. If we write a general tsadc driver by accessed tsadc
 controller registers, and it hardly to write a driver for the tsadc
 controller.
 As suggested by Arnd, you can use the generic driver as interface
 between thermal framework and IIO layer. The driver you are going to
 write to your ADC is going to be in the IIO subsystem. The only
 difference is that the generic driver would work for any ADC driver in
 the IIO subsystem.
 https://lkml.org/lkml/2014/2/5/810

 In fact, there is already a generic driver. We just need to get it up
 to date.  I see some testing has been already done, and results sound
 promising.
This iio_thermal driver only support get temperature from the ADC IIO
channel.
It dose not support any advanced feature such as configure the
temperature when system should be reset.
So if we adapt it, we will access the register form both TSADC IIO
driver and  thermal driver.
I don't thank this is a good ideal.

In fact, we should ignore the ADC stuff. This sensor use ADC internal,
but it is transparent to software, especially in automatic mode.

So, I do not agree to write a generic adc driver for the rk3288-tsadc.

 
 Yifeng Zhao




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi, Mark:

在 2014年08月28日 23:11, Mark Rutland 写道:
> To clarify: if there are low power states that the CPU can enter where
> we lose state, then this patch isn't correct.
Right now, the software of RK3288 SoC only support CPU hotplug
(cpu_on/off) and power off all CPUs on suspend.
We do not implement cpuidle to power off CPU. Do you think we should
introduce a broadcast timer?
On our early kernel, I never see any interrupt on a broadcast timer
(yes, we implement it with a external timer).
>
> A more general approach would be to enable the broadcast hrtimer for
> arm, as has been done for arm64.
Yes. I think it should be done by arm framework.
>
> See commit 5d1638acb9f6 (tick: Introduce hrtimer based broadcast) which
> introduced the broadcast hrtimer, and commit 9358d755bd5c (arm64:
> kernel: initialize broadcast hrtimer based clock event device) which
> added the requisite plumbing for arm64.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi, Mark:

在 2014年08月28日 23:11, Mark Rutland 写道:
 To clarify: if there are low power states that the CPU can enter where
 we lose state, then this patch isn't correct.
Right now, the software of RK3288 SoC only support CPU hotplug
(cpu_on/off) and power off all CPUs on suspend.
We do not implement cpuidle to power off CPU. Do you think we should
introduce a broadcast timer?
On our early kernel, I never see any interrupt on a broadcast timer
(yes, we implement it with a external timer).

 A more general approach would be to enable the broadcast hrtimer for
 arm, as has been done for arm64.
Yes. I think it should be done by arm framework.

 See commit 5d1638acb9f6 (tick: Introduce hrtimer based broadcast) which
 introduced the broadcast hrtimer, and commit 9358d755bd5c (arm64:
 kernel: initialize broadcast hrtimer based clock event device) which
 added the requisite plumbing for arm64.



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ASoC: add driver for Rockchip RK3xxx I2S controller

[Huang Tao]

Hi, Mark:

于 2014年07月02日 01:07, Mark Brown 写道:
>> +static inline void i2s_writel(struct rk_i2s_dev *i2s, u32 value,
>> > +unsigned int offset)
>> > +{
>> > +  writel_relaxed(value, i2s->regs + offset);
>> > +}
>> > +
>> > +static inline u32 i2s_readl(struct rk_i2s_dev *i2s, unsigned int offset)
>> > +{
>> > +  return readl_relaxed(i2s->regs + offset);
>> > +}
> Perhaps use regmap?  The main advantage would be the debug
> infrastructure, though you could also use _update_bits() to reduce the
> amount of time spent locked.
>
Are you sure? This is a I2S driver, we can write the register directly,
do not through I2C or SPI bus.
Write a register is only a few instructions on ARM, but write through
regmap, it may take a long path.
I think it will just consume CPU power and make the whole thing more
complex.
Could you tell me what benefits we can get if use regmap? Or something I
just missing?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] ASoC: add driver for Rockchip RK3xxx I2S controller

[Huang Tao]

Hi, Mark:

于 2014年07月02日 01:07, Mark Brown 写道:
 +static inline void i2s_writel(struct rk_i2s_dev *i2s, u32 value,
  +unsigned int offset)
  +{
  +  writel_relaxed(value, i2s-regs + offset);
  +}
  +
  +static inline u32 i2s_readl(struct rk_i2s_dev *i2s, unsigned int offset)
  +{
  +  return readl_relaxed(i2s-regs + offset);
  +}
 Perhaps use regmap?  The main advantage would be the debug
 infrastructure, though you could also use _update_bits() to reduce the
 amount of time spent locked.

Are you sure? This is a I2S driver, we can write the register directly,
do not through I2C or SPI bus.
Write a register is only a few instructions on ARM, but write through
regmap, it may take a long path.
I think it will just consume CPU power and make the whole thing more
complex.
Could you tell me what benefits we can get if use regmap? Or something I
just missing?

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/