* [GIT PULL] ARM: SoC fixes for 3.16-rc
From: Olof Johansson @ 2014-07-19 5:49 UTC (permalink / raw)
To: linux-arm-kernel
Hi Linus,
The following changes since commit cacadb4ff969a82628d47db87b5a531be466b134:
Merge tag 'samsung-fixes-3' of git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung into fixes (2014-07-12 21:19:21 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git tags/fixes-for-linus
for you to fetch changes up to 9637f30e6b7bc394c08fa9d27d63622f141142e9:
ARM: EXYNOS: Fix core ID used by platsmp and hotplug code (2014-07-18 17:12:57 -0700)
----------------------------------------------------------------
ARM: SoC fixes for 3.16-rc
A smaller set of fixes this week, and all regression fixes:
- a handful of issues fixed on at91 with common clock conversion
- a set of fixes for Marvell mvebu (SMP, coherency, PM)
- a clock fix for i.MX6Q.
- ... and a SMP/hotplug fix for Exynos
----------------------------------------------------------------
Bo Shen (1):
ARM: at91: at91sam9x5: correct typo error for ohci clock
Boris BREZILLON (2):
ARM: at91/dt: fix usb0 clocks definition in sam9n12 dtsi
ARM: at91/dt: add missing clocks property to pwm node in sam9x5.dtsi
Ezequiel Garcia (1):
ARM: mvebu: Fix coherency bus notifiers by using separate notifiers
Gregory CLEMENT (1):
ARM: mvebu: Fix the operand list in the inline asm of armada_370_xp_pmsu_idle_enter
Lucas Stach (1):
ARM: clk-imx6q: parent lvds_sel input from upstream clock gates
Olof Johansson (3):
Merge tag 'mvebu-fixes-3.16-3' of git://git.infradead.org/linux-mvebu into fixes
Merge tag 'at91-fixes' of git://github.com/at91linux/linux-at91 into fixes
Merge tag 'imx-fixes-3.16-2' of git://git.kernel.org/.../shawnguo/linux into fixes
Thomas Petazzoni (1):
ARM: mvebu: fix SMP boot for Armada 38x and Armada 375 Z1 in big endian
Tomasz Figa (1):
ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
arch/arm/boot/dts/at91sam9n12.dtsi | 2 +-
arch/arm/boot/dts/at91sam9x5.dtsi | 4 ++--
arch/arm/mach-exynos/hotplug.c | 10 ++++++----
arch/arm/mach-exynos/platsmp.c | 34 +++++++++++++++++++---------------
arch/arm/mach-imx/clk-imx6q.c | 4 ++--
arch/arm/mach-mvebu/coherency.c | 6 +++++-
arch/arm/mach-mvebu/headsmp-a9.S | 9 ++++++++-
arch/arm/mach-mvebu/pmsu.c | 10 +++++-----
8 files changed, 48 insertions(+), 31 deletions(-)
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read, write}s*()
From: Arnd Bergmann @ 2014-07-19 7:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140718210631.GA22109@ravnborg.org>
On Friday 18 July 2014 23:06:31 Sam Ravnborg wrote:
> >
> > All these things are not introduced by your patch but now that
> > you show some love and care for this file maybe we could take
> > the next step and bring more order to the current semi chaos?
>
> And I just noticed...
>
> These are never used - and only defined a few places:
> #define inb_p(addr) inb(addr)
> #define inw_p(addr) inw(addr)
> #define inl_p(addr) inl(addr)
> #define outb_p(x, addr) outb((x), (addr))
> #define outw_p(x, addr) outw((x), (addr))
> #define outl_p(x, addr) outl((x), (addr))
>
> Likewise for these:
> #define insb_p(port,to,len) insb(port,to,len)
> #define insw_p(port,to,len) insw(port,to,len)
> #define insl_p(port,to,len) insl(port,to,len)
>
> #define outsb_p(port,from,len) outsb(port,from,len)
> #define outsw_p(port,from,len) outsw(port,from,len)
> #define outsl_p(port,from,len) outsl(port,from,len)
>
>
>
> This set:
> #define inb_p(addr) inb(addr)
> #define inw_p(addr) inw(addr)
> #define inl_p(addr) inl(addr)
> #define outb_p(x, addr) outb((x), (addr))
> #define outw_p(x, addr) outw((x), (addr))
> #define outl_p(x, addr) outl((x), (addr))
>
> Should have a comment that say they are deprecated.
> Especially the "b" variants still have many users.
Are they? I don't remember ever seeing a reason to deprecate
them. We could perhaps enclose them in #ifdef CONFIG_ISA, but
there may also be some drivers that use the same code for ISA
and PCI, and it doesn't really hurt on PCI.
Arnd
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read, write}s*()
From: Arnd Bergmann @ 2014-07-19 7:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140718205953.GA21964@ravnborg.org>
On Friday 18 July 2014 22:59:53 Sam Ravnborg wrote:
> On Wed, Jul 16, 2014 at 01:01:22PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Currently driver writers need to use io{read,write}{8,16,32}_rep() when
> > accessing FIFO registers portably. This is bad for two reasons: it is
> > inconsistent with how other registers are accessed using the standard
> > {read,write}{b,w,l}() functions, which can lead to confusion. On some
> > architectures the io{read,write}*() functions also need to perform some
> > extra checks to determine whether an address is memory-mapped or refers
> > to I/O space. Drivers which can be expected to never use I/O can safely
> > use the {read,write}s{b,w,l,q}(), just like they use their non-string
> > variants and there's no need for these extra checks.
> >
> > This patch implements generic versions of readsb(), readsw(), readsl(),
> > readsq(), writesb(), writesw(), writesl() and writesq(). Variants of
> > these string functions for I/O accesses (ins*() and outs*() as well as
> > ioread*_rep() and iowrite*_rep()) are now implemented in terms of the
> > new functions.
> >
> > Going forward, {read,write}{,s}{b,w,l,q}() should be used consistently
> > by drivers for devices that will only ever be memory-mapped and hence
> > don't need to access I/O space, whereas io{read,write}{8,16,32}_rep()
> > should be used by drivers for devices that can be either memory-mapped
> > or I/O-mapped.
> >
> > While at it, also make sure that any of the functions provided as
> > fallback for architectures that don't override them can't be overridden
> > subsequently.
> >
> > This is compile- and runtime-tested on 32-bit and 64-bit ARM and compile
> > tested on Microblaze, s390, SPARC and Xtensa. For ARC, Blackfin, Metag,
> > OpenRISC, Score and Unicore32 which also use asm-generic/io.h I couldn't
> > find or build a cross-compiler that would run on my system. But by code
> > inspection they shouldn't break with this patch.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
>
> Hi Thierry.
>
> While browsing the resulting asm-generic/io.h it is irritating
> that the functions are messed up like they are.
>
> From the start of the file the IO accessors are defined in following order:
> __raw_readb
> __raw_readw
> __raw_readl
>
> readb
> readw
> readl
>
> __raw_writeb
> __raw_writew
> __raw_writel
>
> writeb
> writew
> writel
>
> __raw_readq
>
> readq
>
> __raw_writeq
>
> writeq
>
>
> See how strange it looks?
It saves one #ifdef CONFIG_64BIT to do it like this, but I guess
you are right that reordering them slightly would be nice here.
> A semi related question.
> Why do we play all these macro tricks in io.h?
>
> Example:
>
> #define writeb __raw_writeb
>
> The consensus these days is to use static inline to have the
> correct prototype but this file is contains a lot of these
> macro conversions.
>
> All these things are not introduced by your patch but now that
> you show some love and care for this file maybe we could take
> the next step and bring more order to the current semi chaos?
The macros are mainly there so we can test their presence with
#ifdef. The interface is complex enough that there is probably
an architecture for any combination of overrides: most should
override the __raw_*() functions with inline assembly, but a lot
don't do that and it works because of implementation details of
the compiler. Some may need to override either readl() or
readl_relaxed() but not the other one.
For this reason, we want architecture-level files that include
the asm-generic version to use macros (or macro + inline function)
rather than a plain inline.
I was arguing earlier that we don't need the extra macros in the
asm-generic version, but it also doesn't hurt and it can make
it slightly easier for new architectures to copy the bits from
asm-generic they want to override.
Arnd
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read,write}s*()
From: Sam Ravnborg @ 2014-07-19 8:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <43703371.jktNRIezc3@wuerfel>
> >
> > This set:
> > #define inb_p(addr) inb(addr)
> > #define inw_p(addr) inw(addr)
> > #define inl_p(addr) inl(addr)
> > #define outb_p(x, addr) outb((x), (addr))
> > #define outw_p(x, addr) outw((x), (addr))
> > #define outl_p(x, addr) outl((x), (addr))
> >
> > Should have a comment that say they are deprecated.
> > Especially the "b" variants still have many users.
>
> Are they? I don't remember ever seeing a reason to deprecate
> them. We could perhaps enclose them in #ifdef CONFIG_ISA, but
> there may also be some drivers that use the same code for ISA
> and PCI, and it doesn't really hurt on PCI.
It is my understanding that inl and inl_p are the same these days.
A quick grep indicate that only m68k define the
_p variant different from the other.
But I failed to find and description of the difference between the
two which is why I assumed they were identical and thus no need for both.
Sam
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read,write}s*()
From: Sam Ravnborg @ 2014-07-19 8:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5913515.3g98ODuH6W@wuerfel>
> > A semi related question.
> > Why do we play all these macro tricks in io.h?
> >
> > Example:
> >
> > #define writeb __raw_writeb
> >
> > The consensus these days is to use static inline to have the
> > correct prototype but this file is contains a lot of these
> > macro conversions.
> >
> > All these things are not introduced by your patch but now that
> > you show some love and care for this file maybe we could take
> > the next step and bring more order to the current semi chaos?
>
> The macros are mainly there so we can test their presence with
> #ifdef.
There are two types of macros in io.h:
#define __raw_writeb __raw_writeb
#ifndef __raw_writeb
This is the one you talk about which allows an architecture to
provide a local implementation of a function.
These are prefectly OK and are required.
Then there are the other type where one IO access function
may re-use the implementation of another IO access function:
#ifndef writeb
#define writeb __raw_writeb
#endif
This could have been implmented like this:
#ifndef writeb
#define writeb writeb
static inline void writeb(u8 b, volatile void __iomem *addr)
{
__raw_writeb(b, addr);
}
#endif
In this way the prototype of the function is easy to understand and
we avoid the macro tricks were we blindly replace one function name,
with another function name.
And we also use the same pattarn all over for the various functions.
Concerning the efficiency the compiler should be smart enough to
do the same independent on the two implmentations.
The only drawback I see is that the above is 6 lines of codes,
where the macro expansion is 3 lines of code.
And when we talk about simpler expansion like ioread8()
then we replace one line with 4 lines.
Sam
^ permalink raw reply
* [PATCH v10 0/8] ARM: berlin: add AHCI support
From: Sebastian Hesselbarth @ 2014-07-19 9:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140718135758.GB13012@htj.dyndns.org>
On 07/18/2014 03:57 PM, Tejun Heo wrote:
> On Fri, Jul 18, 2014 at 02:29:59PM +0200, Antoine T?nart wrote:
>> In both cases we do not have time to do this for the next release, as
>> the request popped up quite late.
>>
>> So as of now:
>> - Either the series is merged as is and changes to the AHCI framework
>> can be made for 3.18, as it's not particularly linked to this
>> series.
>> - Or you really do not want it. Then that would be great if patches
>> 1-2 and 7-8 could be merged so that we do not end up with this big
>> series going for yet another cycle... I think Kishon already took
>> patches 1-2.
>
> I don't wanna take in code which isn't in the shape that it should be.
> Things like this accumulate to become a large maintenance burden over
> time. Sure, urgent things can slip in and then later be fixed up but
> who are gonna do that here? You guys already seem to be under time
> pressure as it is.
>
> If you guys can figure something out with Hans regarding how to
> proceed on this, I'll be happy take the code as is.
I cannot say much about AHCI, but if the PHY driver is fine I can
take the DT patches adding it to BG2Q and BG2Q-DMP at any time.
Sebastian
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read, write}s*()
From: Arnd Bergmann @ 2014-07-19 9:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140719084152.GA31564@ravnborg.org>
On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:
> > >
> > > This set:
> > > #define inb_p(addr) inb(addr)
> > > #define inw_p(addr) inw(addr)
> > > #define inl_p(addr) inl(addr)
> > > #define outb_p(x, addr) outb((x), (addr))
> > > #define outw_p(x, addr) outw((x), (addr))
> > > #define outl_p(x, addr) outl((x), (addr))
> > >
> > > Should have a comment that say they are deprecated.
> > > Especially the "b" variants still have many users.
> >
> > Are they? I don't remember ever seeing a reason to deprecate
> > them. We could perhaps enclose them in #ifdef CONFIG_ISA, but
> > there may also be some drivers that use the same code for ISA
> > and PCI, and it doesn't really hurt on PCI.
>
> It is my understanding that inl and inl_p are the same these days.
> A quick grep indicate that only m68k define the
> _p variant different from the other.
> But I failed to find and description of the difference between the
> two which is why I assumed they were identical and thus no need for both.
I don't know why m68k needs it, it's really an x86-specific
thing, see slow_down_io() in arch/x86/include/asm/io.h.
Arnd
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read, write}s*()
From: Arnd Bergmann @ 2014-07-19 9:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140719085338.GB31564@ravnborg.org>
On Saturday 19 July 2014 10:53:38 Sam Ravnborg wrote:
>
> Then there are the other type where one IO access function
> may re-use the implementation of another IO access function:
>
> #ifndef writeb
> #define writeb __raw_writeb
> #endif
>
> This could have been implmented like this:
>
> #ifndef writeb
> #define writeb writeb
> static inline void writeb(u8 b, volatile void __iomem *addr)
> {
> __raw_writeb(b, addr);
> }
> #endif
>
> In this way the prototype of the function is easy to understand and
> we avoid the macro tricks were we blindly replace one function name,
> with another function name.
> And we also use the same pattarn all over for the various functions.
>
> Concerning the efficiency the compiler should be smart enough to
> do the same independent on the two implmentations.
I really don't have a strong opinion on those, as you say one is a
little shorter and the other is a little more readable, so my
preference in a case like this is to leave it up to the person
who last touches the code and let them decide.
Arnd
^ permalink raw reply
* [PATCH v3 1/3] asm-generic/io.h: Implement generic {read,write}s*()
From: Sam Ravnborg @ 2014-07-19 9:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4967546.K5Yf5hcJE1@wuerfel>
On Sat, Jul 19, 2014 at 11:05:33AM +0200, Arnd Bergmann wrote:
> On Saturday 19 July 2014 10:41:52 Sam Ravnborg wrote:
> > > >
> > > > This set:
> > > > #define inb_p(addr) inb(addr)
> > > > #define inw_p(addr) inw(addr)
> > > > #define inl_p(addr) inl(addr)
> > > > #define outb_p(x, addr) outb((x), (addr))
> > > > #define outw_p(x, addr) outw((x), (addr))
> > > > #define outl_p(x, addr) outl((x), (addr))
> > > >
> > > > Should have a comment that say they are deprecated.
> > > > Especially the "b" variants still have many users.
> > >
> > > Are they? I don't remember ever seeing a reason to deprecate
> > > them. We could perhaps enclose them in #ifdef CONFIG_ISA, but
> > > there may also be some drivers that use the same code for ISA
> > > and PCI, and it doesn't really hurt on PCI.
> >
> > It is my understanding that inl and inl_p are the same these days.
> > A quick grep indicate that only m68k define the
> > _p variant different from the other.
> > But I failed to find and description of the difference between the
> > two which is why I assumed they were identical and thus no need for both.
>
> I don't know why m68k needs it, it's really an x86-specific
> thing, see slow_down_io() in arch/x86/include/asm/io.h.
I had missed the x86 versions when grepping.
Hmm, and with the macro tricks they play in asm/io.h this
file is not at all grep friendly.
So xxx_p is for pause (or something like that).
This also matches that m68k do some tricks with delay() in the _p variants.
Thanks for the explanation.
Sam
^ permalink raw reply
* [PATCH 1/5] arm: dts: omap3-gta04: Add missing nodes to fully describe gta04 board
From: Dr. H. Nikolaus Schaller @ 2014-07-19 9:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABxcv==sj5ZZ_f1gqgREFDNoiVv71k3rjQLPq6CpZbB-643Z4g@mail.gmail.com>
Hi all,
Am 18.07.2014 um 11:21 schrieb Javier Martinez Canillas:
> Hello Marek and Dr. H. Nikolaus,
>
> On Fri, Jul 18, 2014 at 8:55 AM, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 16 July 2014 09:17, Dr. H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>> Am 15.07.2014 um 14:45 schrieb Joachim Eastwood:
>>>
>>>> Hi Marek,
>>>>
>>>> You seem to add some DT nodes for hw that doesn't have drivers in
>>>> mainline. I think you should leave those out until the driver itself
>>>> is upstream and the bindings for it is documented.
>>> is there some policy for only having nodes for existing drivers in DT files?
>>
>
> I strongly agree with Joachim on this regard.
>
>>> If I understand the device tree concept correctly, it should not describe drivers
>>> (and hence nothing about the state of them being mainlined), but it should statically
>>> describe the given hardware in a way that kernel and drivers can read it (or ignore).
>>> And even other kernels can use it (because they run on the same hardware).
>>>
>
> Yes, it should describe hardware but that description should be
> previously agreed and properly documented as was mentioned before.
Ok, it is a little a hen and egg problem - who should start with the documentation.
The device tree file, more or less describing what the hardware requirements
are. Or the drivers which describe what they support.
And you are right - these two ends must match and that can only be resolved by
discussions and negotiations between both ends.
>
>>> So unless there is an important reason not to have "currently unused" nodes
>>> in the DT source/binary (loading time is IMHO neglectable), I would ask that we
>>> can keep with the complete DT instead of splitting it up artificially and getting back
>>> to the same status (because the hardware does not change over time).
>>
>
> I understand your motivation since it will allow you to not have to
> maintain a patch-set for your downstream DTS changes but I would like
> to ask you the opposite question. What's the benefit for the kernel
> community to keep in a mainline DTS a bunch of device nodes with DT
> bindings that have not been neither discussed nor documented?
Less patches to review individually. Just a single one instead.
But this might be a weak benefit.
>
> Developers doing a board bring-up usually use the DTS in mainline as a
> reference for their boards and having non-documented/agreed DT binding
> on the upstream DTS will only bring confusion and frustration to them
> IMHO.
Well, my experience (at least for the current status) is that in most times new
boards need new drivers and DT nodes anyways. But this might change.
>
> We already have some issues with Device Trees (bindings that broke
> backward compatibility, drivers implementing custom DT binding instead
> of using standard ones, bindings that don't really reflect the actual
> H/W, etc), please don't make this even more complicated to developers.
Ok, then I think we will do the way as suggested and only submit DT nodes
for already existing drivers. Or submit new ones bundled with driver and
documentation patches.
Marek will update the patches anyways, so that they apply without errors.
>
> Thanks a lot and best regards,
> Javier
BR,
Nikolaus
^ permalink raw reply
* [PATCH v10 0/8] ARM: berlin: add AHCI support
From: Hans de Goede @ 2014-07-19 10:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140718135758.GB13012@htj.dyndns.org>
Hi,
On 07/18/2014 03:57 PM, Tejun Heo wrote:
> (cc'ing Hans who's now maintaining libahci-platform.)
Note I was already following this thread as I'm subscribed to
linux-ide now.
>
> On Fri, Jul 18, 2014 at 02:29:59PM +0200, Antoine T?nart wrote:
>> Tejun, Kishon, Sebastian,
>>
>> I looked into the AHCI framework to see how to map PHYs and ports
>> information. I see two ways of doing this:
>> - We can attach the ahci_port_priv to the ahci_host_priv structure,
>> but that would require quite a lot of changes since the
>> ahci_port_priv is initialized at the very end (in port_start()) and
>> because ahci_port_priv is currently retrieved from the ata_port
>> structure in libahci functions. We do want to parse the dt ports
>> early in the AHCI initialization to be able to generate the right
>> port_map mask. Tests would be needed to ensure nothing is broken.
>> - We can move the PHY handling to where the ports are handled, moving
>> PHYs from ahci_host_priv to ahci_port_priv. This also would require
>> to perform some tests as PHY operations would be moved from
>> libahci_platform to libahci.
>
> I don't get the last part. Why would it have to be moved from
> libahci_platform to libahci? Can't we break up the init steps so that
> PHY handling can be put inbetween? The last time I suggested that,
> Hans seemed to agree.
Yes as it sounded good, but I did not look at the code to closely,
looking closer at the code I can see the problem.
ahci_port_priv gets allocated from ahci_port_start in
libahci.c and that same function also starts the port.
If we want to store the phy in ahci_port_priv then we need to do
some work between ahci_port_priv getting allocated and the port getting
started.
ahci_port_start gets allocated from ata_host_start, with the relevant
bit being:
for (i = 0; i < host->n_ports; i++) {
struct ata_port *ap = host->ports[i];
if (ap->ops->port_start) {
rc = ap->ops->port_start(ap);
We could move the allocating of ahci_port_priv to libahci.c:
ahci_init_controller() which has a similar loop. But then the
ahci_port_priv info still is allocated after we call
ahci_platform_enable_resources()
The problem is that:
1) We need to enable resources before we can do ahci_save_initial_config()
2) We must do ahci_save_initial_config() before we can do ata_host_alloc_pinfo()
3) Therefor we don't have port_info at enable_resources time, which is when we
want to enable the phys (and we cannot just enable the phys elsewhere as
enable_resouces gets used on e.g. resume too).
So I think it is best to just make the phy pointers an array inside
ahci_host_priv, with a comment that the array indexes must match port
indexes. Which brings us back to square one, sorry for long dance to
get there.
I know I initially agreed that it would be good to store the phy
pointer inside ahci_port_priv, but in practice this just does not
work well as we get and enable resources before we have ahci_port_priv.
Which I see is exactly what you've done in patch 4 of v10 of this
series :)
>> In both cases we do not have time to do this for the next release, as
>> the request popped up quite late.
>>
>> So as of now:
>> - Either the series is merged as is and changes to the AHCI framework
>> can be made for 3.18, as it's not particularly linked to this
>> series.
>> - Or you really do not want it. Then that would be great if patches
>> 1-2 and 7-8 could be merged so that we do not end up with this big
>> series going for yet another cycle... I think Kishon already took
>> patches 1-2.
>
> I don't wanna take in code which isn't in the shape that it should be.
> Things like this accumulate to become a large maintenance burden over
> time. Sure, urgent things can slip in and then later be fixed up but
> who are gonna do that here? You guys already seem to be under time
> pressure as it is.
>
> If you guys can figure something out with Hans regarding how to
> proceed on this, I'll be happy take the code as is.
I think the way to proceed with this is just leaving things as they are,
see above.
As for taking the ahci parts of this series, except for the minor comments
from you to patch 3 and from me to patch 5 that is fine with me.
Regards,
Hans
>
> Thanks.
>
^ permalink raw reply
* [PATCH v10 0/8] ARM: berlin: add AHCI support
From: Hans de Goede @ 2014-07-19 10:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CA4608.3040208@redhat.com>
Hi,
On 07/19/2014 12:18 PM, Hans de Goede wrote:
<snip>
> The problem is that:
>
> 1) We need to enable resources before we can do ahci_save_initial_config()
> 2) We must do ahci_save_initial_config() before we can do ata_host_alloc_pinfo()
> 3) Therefor we don't have port_info at enable_resources time, which is when we
> want to enable the phys (and we cannot just enable the phys elsewhere as
> enable_resouces gets used on e.g. resume too).
>
> So I think it is best to just make the phy pointers an array inside
> ahci_host_priv, with a comment that the array indexes must match port
> indexes.
So looking at "[PATCH v10 4/8] ata: libahci: allow to use multiple PHYs"
I see that currently the phy array indexes do not necessarily match the
port indexes. Since you already allocate the phys array at nports size,
I suggest simply making the array sparse, leaving in NULL entries for
unused ports, and adjusting enable / disable_phys to check for NULL
pointers. This way we still have a 1:1 way to map ports <-> phys if
we want to do something with phys on a per port basis in the future.
Note please also add a check that reg < nports so that we don't use
the array out of bounds if there is an error in the dts.
<snip>
Regards,
Hans
^ permalink raw reply
* [PATCH 00/14] clk: sunxi: Improve MMC clocks support
From: Hans de Goede @ 2014-07-19 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405588134-2396-1-git-send-email-maxime.ripard@free-electrons.com>
Hi,
On 07/17/2014 11:08 AM, Maxime Ripard wrote:
> Hi everyone,
>
> Here is an attempt at improving the MMC clock support in the Allwinner
> SoCs.
>
> Until now, the MMC clocks were having a custom phase function that was
> directly setting an obscure value in the right register, because we
> were not really having any idea of what these values were.
>
> Now that we have more informations, we can introduce a common function
> call to get and set the phase of a particular clock, and use this in
> both our provider and our client.
>
> Another issue we had so far on the A13 was that, out of reset, the
> PLL6 driving the MMC was running too high to be working. We can solve
> that by adding two new properties in the DT to setup the rate
> constraints we might have on a clock.
Looks good to me, thanks for working on this.
Question have you dumped the raw mmc0 clk reg before and after this
patch set to verify that the end result is the same ?
Regards,
Hans
^ permalink raw reply
* [PATCH v7 1/6] clk: samsung: add infrastructure to register cpu clocks
From: Tomasz Figa @ 2014-07-19 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-2-git-send-email-thomas.ab@samsung.com>
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
[snip]
> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> new file mode 100644
> index 0000000..0d62968
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-cpu.c
> @@ -0,0 +1,576 @@
> +/*
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Author: Thomas Abraham <thomas.ab@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This file contains the utility functions to register the CPU clocks
> + * for Samsung platforms.
I'd expect few words here or in separate comment on how the code works,
i.e. assumptions made, most important procedures, etc. This is a complex
piece of code for quite complex hardware, so proper documentation is
essential.
> +*/
> +
> +#include <linux/errno.h>
> +#include "clk.h"
> +
> +#define E4210_SRC_CPU 0x0
> +#define E4210_STAT_CPU 0x200
> +#define E4210_DIV_CPU0 0x300
> +#define E4210_DIV_CPU1 0x304
> +#define E4210_DIV_STAT_CPU0 0x400
> +#define E4210_DIV_STAT_CPU1 0x404
> +
> +#define MAX_DIV 8
> +#define DIV_MASK 7
> +#define DIV_MASK_ALL 0xffffffff
> +#define MUX_MASK 7
> +
> +#define E4210_DIV0_RATIO0_MASK 0x7
> +#define E4210_DIV1_HPM_MASK ((0x7 << 4) | (0x7 << 0))
This mask contains two fields, doesn't it? I'd say it would be better
for readability if you split it.
> +#define E4210_MUX_HPM_MASK (1 << 20)
> +#define E4210_DIV0_ATB_SHIFT 16
> +#define E4210_DIV0_ATB_MASK (DIV_MASK << E4210_DIV0_ATB_SHIFT)
> +
> +#define E4210_CPU_DIV0(apll, pclk_dbg, atb, periph, corem1, corem0) \
> + (((apll) << 24) | ((pclk_dbg) << 20) | ((atb) << 16) | \
> + ((periph) << 12) | ((corem1) << 8) | ((corem0) << 4))
> +#define E4210_CPU_DIV1(hpm, copy) \
> + (((hpm) << 4) | ((copy) << 0))
> +
> +#define E5250_CPU_DIV0(apll, pclk_dbg, atb, periph, acp, cpud) \
> + (((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> + (periph << 12) | (acp << 8) | (cpud << 4)))
> +#define E5250_CPU_DIV1(hpm, copy) \
> + (((hpm) << 4) | (copy))
> +
> +#define E5420_EGL_DIV0(apll, pclk_dbg, atb, cpud) \
> + (((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> + (cpud << 4)))
> +#define E5420_KFC_DIV(kpll, pclk, aclk) \
> + (((kpll << 24) | (pclk << 20) | (aclk << 4)))
Again, used macro arguments should always be surrounded with parentheses.
> +
> +enum cpuclk_type {
> + EXYNOS4210,
> + EXYNOS5250,
> + EXYNOS5420,
> +};
> +
> +/**
> + * struct exynos4210_cpuclk_data: config data to setup cpu clocks.
It seems like this could be used for all Exynos SoCs, so probably should
be called exynos_cpuclk_data.
> + * @prate: frequency of the primary parent clock (in KHz).
> + * @div0: value to be programmed in the div_cpu0 register.
> + * @div1: value to be programmed in the div_cpu1 register.
> + *
> + * This structure holds the divider configuration data for dividers in the CPU
> + * clock domain. The parent frequency at which these divider values are valid is
> + * specified in @prate. The @prate is the frequency of the primary parent clock.
> + * For CPU clock domains that do not have a DIV1 register, the @div1 member
> + * is optional.
> + */
> +struct exynos4210_cpuclk_data {
> + unsigned long prate;
> + unsigned int div0;
> + unsigned int div1;
> +};
> +
> +/**
> + * struct exynos_cpuclk: information about clock supplied to a CPU core.
> + * @hw: handle between CCF and CPU clock.
> + * @alt_parent: alternate parent clock to use when switching the speed
> + * of the primary parent clock.
> + * @ctrl_base: base address of the clock controller.
> + * @offset: offset from the ctrl_base address where the CPU clock div/mux
> + * registers can be accessed.
> + * @lock: cpu clock domain register access lock.
> + * @type: type of the CPU clock.
> + * @data: optional data which the actual instantiation of this clock
> + * can use.
> + * @clk_nb: clock notifier registered for changes in clock speed of the
> + * primary parent clock.
> + * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
> + * of the primary parent clock.
> + * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
> + * of the primary parent clock.
> + *
> + * This structure holds information required for programming the cpu clock for
> + * various clock speeds.
nit: s/cpu/CPU/
> + */
> +struct exynos_cpuclk {
> + struct clk_hw hw;
> + struct clk *alt_parent;
> + void __iomem *ctrl_base;
> + unsigned long offset;
> + spinlock_t *lock;
> + enum cpuclk_type type;
> + const void *data;
The code always expects this to be const struct exynos4210_cpuclk_data.
Why not make this field so?
Also this is not some plain data, but an array of operating points, so
probably a name like "rates" would be better.
> + struct notifier_block clk_nb;
> + int (*pre_rate_cb)(struct clk_notifier_data *,
> + struct exynos_cpuclk *,
> + void __iomem *base);
> + int (*post_rate_cb)(struct clk_notifier_data *,
> + struct exynos_cpuclk *,
> + void __iomem *base);
All the Exynos SoCs supported by this patch seem to be using exactly the
same notifiers. We don't know what changes in further SoCs, so there is
no guarantee that having these as pointer here will give us any
benefits. I'd recommend just getting rid of this indirection for now. If
it turns out to be needed, it will be trivial to add it back.
> +};
> +
> +#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
> +#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)
Please make these static inlines for type safety.
> +
> +/**
> + * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
> + * @ops: clock operations to be used for this clock.
> + * @offset: optional offset from base of clock controller register base, to
> + * be used when accessing clock controller registers related to the
> + * CPU clock.
> + * @data: SoC specific data for cpuclk configuration (optional).
How is this optional? Can this code work without a list of operating points?
> + * @data_size: size of the data contained in @data member.
Both fields could be probably named "rates" and "num_rates", with the
meaning of the latter changed to mean the number of entries, not size in
bytes.
> + * @type: type of the CPU clock.
> + * @pre_rate_cb: callback function to handle PRE_RATE_CHANGE notification
> + * of the primary parent clock.
> + * @post_rate_cb: callback function to handle POST_RATE_CHANGE notification
> + * of the primary parent clock.
> + *
> + * This structure provides SoC specific data for CPU clocks. Based on
> + * the compatible value of the clock controller node, the value of the
> + * fields in this structure can be populated.
> + */
> +struct exynos_cpuclk_soc_data {
> + const struct clk_ops *ops;
> + unsigned int offset;
> + const void *data;
Same comments as for the data field above.
> + const unsigned int data_size;
If the same struct is always used it would be clearer to
> + enum cpuclk_type type;
> + int (*pre_rate_cb)(struct clk_notifier_data *,
> + struct exynos_cpuclk *,
> + void __iomem *base);
> + int (*post_rate_cb)(struct clk_notifier_data *,
> + struct exynos_cpuclk *,
> + void __iomem *base);
Same comment as above.
> +};
It looks like instead of duplicating most of the fields of this struct
in exynos_cpuclk struct the latter could simply have a pointer to an
instance of the former.
> +
> +/*
> + * Helper function to wait until divider(s) have stabilized after the divider
> + * value has changed.
> + */
How about a kernel doc like comments for functions as well? (same
comment for remaining functions)
> +static void wait_until_divider_stable(void __iomem *div_reg, unsigned long mask)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(10);
> +
> + do {
> + if (!(readl(div_reg) & mask))
> + return;
> + } while (time_before(jiffies, timeout));
> +
> + pr_err("%s: timeout in divider stablization\n", __func__);
> +}
[snip]
> +/* common recalc rate callback useable for all types of CPU clocks */
> +static unsigned long exynos_cpuclk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
This function is so trivial that it might be reasonable to explain why
nothing else is needed, e.g.
/*
* The CPU clock output (armclk) rate is the same as its parent
* rate. Although there exist certain dividers inside the CPU
* clock block that could be used to divide the parent clock,
* the driver does not make use of them currently, except during
* frequency transitions.
*/
> + return parent_rate;
> +}
> +
> +static const struct clk_ops exynos_cpuclk_clk_ops = {
> + .recalc_rate = exynos_cpuclk_recalc_rate,
> + .round_rate = exynos_cpuclk_round_rate,
> +};
> +
> +/*
> + * Calculates the divider value to be set for deriving drate from prate.
> + * Divider value is actual divider value - 1.
> + */
> +static unsigned long _calc_div(unsigned long prate, unsigned long drate)
> +{
> + unsigned long div = DIV_ROUND_UP(prate, drate) - 1;
> +
> + WARN_ON(div >= MAX_DIV);
> + return div;
> +}
This function seems to be used just once. Not even saying about its
strange semantics - the name would suggest a real divider being
returned, but it's not, it's divider minus one.
So I'd suggest to completely remove this function and simply paste its
contents instead, since it's only used once.
> +
[snip]
> +/* helper function to register a cpu clock */
> +static int __init exynos_cpuclk_register(struct samsung_clk_provider *ctx,
> + unsigned int lookup_id, const char *name, const char *parent,
> + const char *alt_parent, struct device_node *np,
> + const struct exynos_cpuclk_soc_data *soc_data)
> +{
> + struct exynos_cpuclk *cpuclk;
> + struct clk_init_data init;
> + struct clk *clk;
> + void *data;
> + int ret = 0;
> +
> + cpuclk = kzalloc(sizeof(*cpuclk), GFP_KERNEL);
> + if (!cpuclk)
> + return -ENOMEM;
> +
> + data = kmalloc(soc_data->data_size, GFP_KERNEL);
You could simply use kmemdup() here to duplicate the source array. Also
the return value could be already saved in cpuclk->data without the need
for a local variable.
> + if (!data) {
> + ret = -ENOMEM;
> + goto free_cpuclk;
> + }
> +
> + init.name = name;
> + init.flags = CLK_SET_RATE_PARENT;
> + init.parent_names = &parent;
> + init.num_parents = 1;
> + init.ops = soc_data->ops;
> +
> + cpuclk->hw.init = &init;
> + cpuclk->ctrl_base = ctx->reg_base;
> + cpuclk->lock = &ctx->lock;
> + cpuclk->offset = soc_data->offset;
> + cpuclk->type = soc_data->type;
> + cpuclk->pre_rate_cb = soc_data->pre_rate_cb;
> + cpuclk->post_rate_cb = soc_data->post_rate_cb;
> + memcpy(data, soc_data->data, soc_data->data_size);
> + cpuclk->data = data;
> +
> + cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
> + ret = clk_notifier_register(__clk_lookup(parent), &cpuclk->clk_nb);
It would be a good idea to check if __clk_lookup() succeeded and error
out otherwise.
> + if (ret) {
> + pr_err("%s: failed to register clock notifier for %s\n",
> + __func__, name);
> + goto free_cpuclk_data;
> + }
> +
[snip]
> +/*
> + * Helper function to set the 'safe' dividers for the CPU clock. The parameters
> + * div and mask contain the divider value and the register bit mask of the
> + * dividers to be programmed.
> + */
> +static void exynos4210_set_safe_div(void __iomem *base, unsigned long div,
> + unsigned long mask)
> +{
> + unsigned long div0;
> +
> + div0 = readl(base + E4210_DIV_CPU0);
> + div0 = (div0 & ~mask) | div;
There is nothing said in the comment above about the assumption that div
has bits not indicated by mask cleared, so to be safe it might be a good
idea to make this
div0 = (div0 & ~mask) | (div & mask);
instead.
> + writel(div0, base + E4210_DIV_CPU0);
> + wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, mask);
> +}
> +
> +/* handler for pre-rate change notification from parent clock */
> +static int exynos4210_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
> + struct exynos_cpuclk *cpuclk, void __iomem *base)
> +{
> + const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
> + unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
> + unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
> + unsigned long div0, div1 = 0, mux_reg;
> +
> + /* find out the divider values to use for clock data */
> + while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
> + if (cpuclk_data->prate == 0)
> + return -EINVAL;
> + cpuclk_data++;
> + }
> +
> + /* For the selected PLL clock frequency, get the pre-defined divider
> + * values. If the clock for sclk_hpm is not sourced from apll, then
> + * the values for DIV_COPY and DIV_HPM dividers need not be set.
> + */
> + div0 = cpuclk_data->div0;
> + if (cpuclk->type != EXYNOS5420) {
Rather than checking for Exynos5420 explicitly, it would be better to
add a boolean "has_mux_hpm" flag to cpuclk.
> + div1 = cpuclk_data->div1;
> + if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK) {
> + div1 = readl(base + E4210_DIV_CPU1) &
> + E4210_DIV1_HPM_MASK;
> + div1 |= ((cpuclk_data->div1) & ~E4210_DIV1_HPM_MASK);
> + }
> + }
> +
> + spin_lock(cpuclk->lock);
> +
> + /*
> + * If the new and old parent clock speed is less than the clock speed
> + * of the alternate parent, then it should be ensured that at no point
> + * the armclk speed is more than the old_prate until the dividers are
> + * set.
> + */
> + if (alt_prate > ndata->old_rate) {
> + alt_div = _calc_div(alt_prate, ndata->old_rate);
> + if (cpuclk->type == EXYNOS4210) {
Again, this could be handled by a boolean "needs_atb_alt_div" flag,
instead of checking for Exynos4210 explicitly.
> + /*
> + * In Exynos4210, ATB clock parent is also mout_core. So
> + * ATB clock also needs to be mantained at safe speed.
> + */
> + alt_div |= E4210_DIV0_ATB_MASK;
> + alt_div_mask |= E4210_DIV0_ATB_MASK;
> + }
> + exynos4210_set_safe_div(base, alt_div, alt_div_mask);
> + div0 |= alt_div;
> + }
> +
> + /* select sclk_mpll as the alternate parent */
> + mux_reg = readl(base + E4210_SRC_CPU);
> + writel(mux_reg | (1 << 16), base + E4210_SRC_CPU);
> + wait_until_mux_stable(base + E4210_STAT_CPU, 16, 2);
> +
> + /* alternate parent is active now. set the dividers */
> + writel(div0, base + E4210_DIV_CPU0);
> + wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, DIV_MASK_ALL);
> +
> + if (cpuclk->type != EXYNOS5420) {
This could be handled by "has_div_cpu1" boolean flag.
> + writel(div1, base + E4210_DIV_CPU1);
> + wait_until_divider_stable(base + E4210_DIV_STAT_CPU1,
> + DIV_MASK_ALL);
> + }
> +
> + spin_unlock(cpuclk->lock);
> + return 0;
> +}
> +
> +/* handler for post-rate change notification from parent clock */
> +static int exynos4210_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
> + struct exynos_cpuclk *cpuclk, void __iomem *base)
> +{
> + const struct exynos4210_cpuclk_data *cpuclk_data = cpuclk->data;
> + unsigned long div = 0, div_mask = DIV_MASK;
> + unsigned long mux_reg;
> +
> + spin_lock(cpuclk->lock);
> +
> + /* select mout_apll as the alternate parent */
> + mux_reg = readl(base + E4210_SRC_CPU);
> + writel(mux_reg & ~(1 << 16), base + E4210_SRC_CPU);
> + wait_until_mux_stable(base + E4210_STAT_CPU, 16, 1);
> +
> + if (cpuclk->type == EXYNOS4210) {
Here the "needs_atb_alt_div" flag could be used again.
> + /* find out the divider values to use for clock data */
> + while ((cpuclk_data->prate * 1000) != ndata->new_rate) {
> + if (cpuclk_data->prate == 0)
> + return -EINVAL;
> + cpuclk_data++;
> + }
> +
> + div |= (cpuclk_data->div0 & E4210_DIV0_ATB_MASK);
> + div_mask |= E4210_DIV0_ATB_MASK;
> + }
> +
> + exynos4210_set_safe_div(base, div, div_mask);
> + spin_unlock(cpuclk->lock);
> + return 0;
> +}
> +
> +static const struct exynos4210_cpuclk_data e4210_armclk_d[] __initconst = {
> + { 1200000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 5), },
> + { 1000000, E4210_CPU_DIV0(7, 1, 4, 3, 7, 3), E4210_CPU_DIV1(0, 4), },
> + { 800000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
> + { 500000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
> + { 400000, E4210_CPU_DIV0(7, 1, 3, 3, 7, 3), E4210_CPU_DIV1(0, 3), },
> + { 200000, E4210_CPU_DIV0(0, 1, 1, 1, 3, 1), E4210_CPU_DIV1(0, 3), },
> + { 0 },
> +};
[snip]
> +static const struct exynos_cpuclk_soc_data e4210_clk_soc_data __initconst = {
> + .ops = &exynos_cpuclk_clk_ops,
> + .offset = 0x14200,
> + .data = e4210_armclk_d,
> + .data_size = sizeof(e4210_armclk_d),
> + .type = EXYNOS4210,
> + .pre_rate_cb = exynos4210_cpuclk_pre_rate_change,
> + .post_rate_cb = exynos4210_cpuclk_post_rate_change,
> +};
[snip]
> +/**
> + * exynos_register_cpu_clock: register cpu clock with ccf.
> + * @ctx: driver context.
> + * @cluster_id: cpu cluster number to which this clock is connected.
> + * @lookup_id: cpuclk clock output id for the clock controller.
> + * @name: the name of the cpu clock.
> + * @parent: name of the parent clock for cpuclk.
> + * @alt_parent: name of the alternate clock parent.
> + * @np: device tree node pointer of the clock controller.
> + */
> +int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
> + unsigned int cluster_id, unsigned int lookup_id,
> + const char *name, const char *parent,
> + const char *alt_parent, struct device_node *np)
> +{
> + const struct of_device_id *match;
> + const struct exynos_cpuclk_soc_data *data = NULL;
> +
> + if (!np)
> + return -EINVAL;
> +
> + match = of_match_node(exynos_cpuclk_ids, np);
> + if (!match)
> + return -EINVAL;
> +
> + data = match->data;
> + data += cluster_id;
> + return exynos_cpuclk_register(ctx, lookup_id, name, parent,
> + alt_parent, np, data);
> +}
We now have SoC-specific data hardcoded here, so (as opposed to my
earlier comments when we did not have such) it's now reasonable to move
such data to SoC-specific source files and then just call
exynos_register_cpu_clock() with a pointer to such data. This would also
eliminate the not so good idea of indexing internal data array with
cluster_id passed as an argument from external code.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v7 2/6] clk: samsung: register exynos5420 apll/kpll configuration data
From: Tomasz Figa @ 2014-07-19 12:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-3-git-send-email-thomas.ab@samsung.com>
On 14.07.2014 15:38, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Register the PLL configuration data for APLL and KPLL on Exynos5420. This
> configuration data table specifies PLL coefficients for supported PLL
> clock speeds when a 24MHz clock is supplied as the input clock source
> for these PLLs.
>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Tested-by: Arjun K.V <arjun.kv@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos5420.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
Looks good. Will apply.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH] asm-generic/io.h: reorder funtions to form logical groups
From: Sam Ravnborg @ 2014-07-19 12:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140718205953.GA21964@ravnborg.org>
>From 929c64c1aaf378b767e0ed89826b6bb12449df15 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 19 Jul 2014 14:47:43 +0200
Subject: [PATCH] asm-generic/io.h: reorder funtions to form logical groups
Reoder the functions so the various functions are grouped
according to how they access memoy.
For example __raw_{read,write}* are now all grouped.
The benefit of this grouping is that one can easier find all
IO accessors of one type.
To do so a few more #ifdef CONFIG_64BIT had to be used.
Add a small boiler plate comment for some of the groups to
better let them stand out.
Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
Hi Thierry.
This is my attempt to bring some order into io.h
with respect to the order the functions are defined in.
In a follow-up mail I also said we should delete the _p variants
of some methods but I then learned they are for slow IO access.
So these I have left as is.
And introducing static inline for all functions that are pure macro
substitution is also left out for now.
Please consider if you will take this as a follow-on patch.
Sam
include/asm-generic/io.h | 126 +++++++++++++++++++++++++++--------------------
1 file changed, 73 insertions(+), 53 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index b2ea16b..5c84db4 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -24,10 +24,10 @@
#define mmiowb() do {} while (0)
#endif
-/*****************************************************************************/
/*
- * readX/writeX() are used to access memory mapped devices. On some
- * architectures the memory mapped IO stuff needs to be accessed
+ * raw_{read,write}{b,w,l,q} access memory in native endian.
+ *
+ * On some architectures the memory mapped IO stuff needs to be accessed
* differently. On the simple architectures, we just read/write the
* memory location directly.
*/
@@ -55,25 +55,16 @@ static inline u32 __raw_readl(const volatile void __iomem *addr)
}
#endif
-#ifndef readb
-#define readb __raw_readb
-#endif
-
-#ifndef readw
-#define readw readw
-static inline u16 readw(const volatile void __iomem *addr)
+#ifdef CONFIG_64BIT
+#ifndef __raw_readq
+#define __raw_readq __raw_readq
+static inline u64 __raw_readq(const volatile void __iomem *addr)
{
- return __le16_to_cpu(__raw_readw(addr));
+ return *(const volatile u64 __force *) addr;
}
#endif
+#endif /* CONFIG_64BIT */
-#ifndef readl
-#define readl readl
-static inline u32 readl(const volatile void __iomem *addr)
-{
- return __le32_to_cpu(__raw_readl(addr));
-}
-#endif
#ifndef __raw_writeb
#define __raw_writeb __raw_writeb
@@ -99,27 +90,42 @@ static inline void __raw_writel(u32 b, volatile void __iomem *addr)
}
#endif
-#ifndef writeb
-#define writeb __raw_writeb
+#ifdef CONFIG_64BIT
+#ifndef __raw_writeq
+#define __raw_writeq __raw_writeq
+static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
+{
+ *(volatile u64 __force *) addr = b;
+}
#endif
+#endif /* CONFIG_64BIT */
-#ifndef writew
-#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+
+/*
+ * {read,write}{b,w,l,q} access little endian memory
+ * and return result in native endian
+ */
+#ifndef readb
+#define readb __raw_readb
#endif
-#ifndef writel
-#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
+#ifndef readw
+#define readw readw
+static inline u16 readw(const volatile void __iomem *addr)
+{
+ return __le16_to_cpu(__raw_readw(addr));
+}
#endif
-#ifdef CONFIG_64BIT
-#ifndef __raw_readq
-#define __raw_readq __raw_readq
-static inline u64 __raw_readq(const volatile void __iomem *addr)
+#ifndef readl
+#define readl readl
+static inline u32 readl(const volatile void __iomem *addr)
{
- return *(const volatile u64 __force *) addr;
+ return __le32_to_cpu(__raw_readl(addr));
}
#endif
+#ifdef CONFIG_64BIT
#ifndef readq
#define readq readq
static inline u64 readq(const volatile void __iomem *addr)
@@ -127,20 +133,31 @@ static inline u64 readq(const volatile void __iomem *addr)
return __le64_to_cpu(__raw_readq(addr));
}
#endif
+#endif /* CONFIG_64BIT */
-#ifndef __raw_writeq
-#define __raw_writeq __raw_writeq
-static inline void __raw_writeq(u64 b, volatile void __iomem *addr)
-{
- *(volatile u64 __force *) addr = b;
-}
+
+#ifndef writeb
+#define writeb __raw_writeb
+#endif
+
+#ifndef writew
+#define writew(b,addr) __raw_writew(__cpu_to_le16(b),addr)
+#endif
+
+#ifndef writel
+#define writel(b,addr) __raw_writel(__cpu_to_le32(b),addr)
#endif
+#ifdef CONFIG_64BIT
#ifndef writeq
#define writeq(b, addr) __raw_writeq(__cpu_to_le64(b), addr)
#endif
#endif /* CONFIG_64BIT */
+
+/*
+ * {read,write}s{b,w,l.q}b access native memory in chunks specified by count
+ */
#ifndef readsb
#define readsb readsb
static inline void readsb(const void __iomem *addr, void *buffer, int count)
@@ -183,6 +200,23 @@ static inline void readsl(const void __iomem *addr, void *buffer, int count)
}
#endif
+#ifdef CONFIG_64BIT
+#ifndef readsq
+#define readsq readsq
+static inline void readsq(const void __iomem *addr, void *buffer, int count)
+{
+ if (count) {
+ u64 *buf = buffer;
+ do {
+ u64 x = __raw_readq(addr);
+ *buf++ = x;
+ } while (--count);
+ }
+}
+#endif
+#endif /* CONFIG_64BIT */
+
+
#ifndef writesb
#define writesb writesb
static inline void writesb(void __iomem *addr, const void *buffer, int count)
@@ -223,20 +257,6 @@ static inline void writesl(void __iomem *addr, const void *buffer, int count)
#endif
#ifdef CONFIG_64BIT
-#ifndef readsq
-#define readsq readsq
-static inline void readsq(const void __iomem *addr, void *buffer, int count)
-{
- if (count) {
- u64 *buf = buffer;
- do {
- u64 x = __raw_readq(addr);
- *buf++ = x;
- } while (--count);
- }
-}
-#endif
-
#ifndef writesq
#define writesq writesq
static inline void writesq(void __iomem *addr, const void *buffer, int count)
@@ -356,6 +376,10 @@ static inline void outl(u32 b, unsigned long addr)
#define ioread16(addr) readw(addr)
#define ioread32(addr) readl(addr)
+#define iowrite8(v, addr) writeb((v), (addr))
+#define iowrite16(v, addr) writew((v), (addr))
+#define iowrite32(v, addr) writel((v), (addr))
+
#ifndef ioread16be
#define ioread16be(addr) __be16_to_cpu(__raw_readw(addr))
#endif
@@ -364,10 +388,6 @@ static inline void outl(u32 b, unsigned long addr)
#define ioread32be(addr) __be32_to_cpu(__raw_readl(addr))
#endif
-#define iowrite8(v, addr) writeb((v), (addr))
-#define iowrite16(v, addr) writew((v), (addr))
-#define iowrite32(v, addr) writel((v), (addr))
-
#ifndef iowrite16be
#define iowrite16be(v, addr) __raw_writew(__cpu_to_be16(v), addr)
#endif
--
1.9.3
^ permalink raw reply related
* [PATCH v2] ARM: rockchip: Add cpu hotplug support for RK3XXX SoCs
From: Romain Perier @ 2014-07-19 13:03 UTC (permalink / raw)
To: linux-arm-kernel
Adds ability to shutdown all CPUs except the first one
(since it might be special for a lot of platforms).
It is now possible to use kexec which requires such a feature.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
arch/arm/mach-rockchip/platsmp.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index 910835d..189684f 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -21,6 +21,7 @@
#include <linux/of_address.h>
#include <asm/cacheflush.h>
+#include <asm/cp15.h>
#include <asm/smp_scu.h>
#include <asm/smp_plat.h>
#include <asm/mach/map.h>
@@ -178,8 +179,27 @@ static void __init rockchip_smp_prepare_cpus(unsigned int max_cpus)
pmu_set_power_domain(0 + i, false);
}
+#ifdef CONFIG_HOTPLUG_CPU
+static int rockchip_cpu_kill(unsigned int cpu)
+{
+ pmu_set_power_domain(0 + cpu, false);
+ return 1;
+}
+
+static void rockchip_cpu_die(unsigned int cpu)
+{
+ v7_exit_coherency_flush(louis);
+ while(1)
+ cpu_do_idle();
+}
+#endif
+
static struct smp_operations rockchip_smp_ops __initdata = {
.smp_prepare_cpus = rockchip_smp_prepare_cpus,
.smp_boot_secondary = rockchip_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+ .cpu_kill = rockchip_cpu_kill,
+ .cpu_die = rockchip_cpu_die,
+#endif
};
CPU_METHOD_OF_DECLARE(rk3066_smp, "rockchip,rk3066-smp", &rockchip_smp_ops);
--
1.9.1
^ permalink raw reply related
* [PATCH v7 3/6] clk: exynos: use cpu-clock provider type to represent arm clock
From: Tomasz Figa @ 2014-07-19 13:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-4-git-send-email-thomas.ab@samsung.com>
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Register cpu clocks using the new cpu-clock provider type for exynos platforms.
> The differnt clock blocks that are now encapsulated within the cpu-clock can be
> marked with read-only attribute.
>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Tested-by: Arjun K.V <arjun.kv@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos4.c | 25 +++++++++++++++++--------
> drivers/clk/samsung/clk-exynos5250.c | 16 +++++++++++-----
> drivers/clk/samsung/clk-exynos5420.c | 32 +++++++++++++++++++++++---------
> include/dt-bindings/clock/exynos5250.h | 1 +
> include/dt-bindings/clock/exynos5420.h | 2 ++
> 5 files changed, 54 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index 75573a4..10f1818 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -471,7 +471,8 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
> MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
> MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
> MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
> - MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
> + MUX_F(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1, 0,
> + CLK_MUX_READ_ONLY),
> MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
> MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
> MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
> @@ -530,7 +531,8 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
> MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
> MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
> MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
> - MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
> + MUX_F(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1, 0,
> + CLK_MUX_READ_ONLY),
> MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
> MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
> MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
> @@ -572,8 +574,10 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>
> /* list of divider clocks supported in all exynos4 soc's */
> static struct samsung_div_clock exynos4_div_clks[] __initdata = {
> - DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
> - DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
> + DIV_F(0, "div_core", "mout_core", DIV_CPU0, 0, 3,
> + CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
> + DIV_F(0, "div_core2", "div_core", DIV_CPU0, 28, 3,
> + CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
> DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
> DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
> DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
> @@ -619,8 +623,10 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
> DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
> DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
> DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
> - DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
> - DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
> + DIV_F(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3,
> + CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
> + DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
> + CLK_GET_RATE_NOCACHE, CLK_DIVIDER_READ_ONLY),
> DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
> CLK_SET_RATE_PARENT, 0),
> DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
> @@ -1016,7 +1022,6 @@ static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
>
> static struct samsung_clock_alias exynos4_aliases[] __initdata = {
> ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
> - ALIAS(CLK_ARM_CLK, NULL, "armclk"),
> ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
> };
These aliases are used only by the old cpufreq driver. If we are
removing support for it, we should also drop them.
However I wonder if we shouldn't rather initially allow both drivers to
coexist with the old still being the default for one release. We want to
avoid regressions so we should first test the new driver for a while and
then, if it proves to be fine, we could drop the old one.
This would probably mean some ifdefs in clock drivers until the old
driver gets removed, but I don't think it's a problem.
>
> @@ -1255,6 +1260,8 @@ static void __init exynos4_clk_init(struct device_node *np,
> ARRAY_SIZE(exynos4210_gate_clks));
> samsung_clk_register_alias(ctx, exynos4210_aliases,
> ARRAY_SIZE(exynos4210_aliases));
> + exynos_register_cpu_clock(ctx, 0, CLK_ARM_CLK, "armclk",
> + mout_core_p4210[0], mout_core_p4210[1], np);
> } else {
> samsung_clk_register_mux(ctx, exynos4x12_mux_clks,
> ARRAY_SIZE(exynos4x12_mux_clks));
> @@ -1264,6 +1271,8 @@ static void __init exynos4_clk_init(struct device_node *np,
> ARRAY_SIZE(exynos4x12_gate_clks));
> samsung_clk_register_alias(ctx, exynos4x12_aliases,
> ARRAY_SIZE(exynos4x12_aliases));
> + exynos_register_cpu_clock(ctx, 0, CLK_ARM_CLK, "armclk",
> + mout_core_p4x12[0], mout_core_p4x12[1], np);
> }
>
> samsung_clk_register_alias(ctx, exynos4_aliases,
> @@ -1278,7 +1287,7 @@ static void __init exynos4_clk_init(struct device_node *np,
> exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
> _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
> _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
> - _get_rate("arm_clk"));
> + _get_rate("armclk"));
> }
>
>
> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
> index 5861183..2f78bb0 100644
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -292,7 +292,8 @@ static struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
> */
> MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
> CLK_SET_RATE_PARENT, 0, "mout_apll"),
> - MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1, "mout_cpu"),
> + MUX_FA(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1,
> + 0, CLK_MUX_READ_ONLY, "mout_cpu"),
>
Similar comment about aliases here and for few clocks below.
Otherwise the patch looks fine.
Best regards,
Tomasz
^ permalink raw reply
* [GIT PULL] Samsung fixes-4 for v3.16
From: Tomasz Figa @ 2014-07-19 13:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53C9AF23.6030007@samsung.com>
Hi Kukjin,
On 19.07.2014 01:34, Kukjin Kim wrote:
> Hi,
>
> This is a regression for v3.16 so please take this in -rc.
>
> Thanks,
> Kukjin
>
> The following changes since commit
> 1795cd9b3a91d4b5473c97f491d63892442212ab:
>
> Linux 3.16-rc5 (2014-07-13 14:04:33 -0700)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git
> tags/samsung-fixes-4
>
> for you to fetch changes up to 4e3a498d6fc438d8c203c51aadc6ca8fb47b0577:
>
> ARM: EXYNOS: Fix core ID used by platsmp and hotplug code (2014-07-16
> 02:59:18 +0900)
>
> ----------------------------------------------------------------
> Samsung fixes-4 for v3.16
> - Fix core ID for platsmp and hotplug
> This fixes existing calculation of PMU register offsets on
> exynos SoCs and this also fixes CPU hotplug with more than
> 2 cores, because it removes incorrect condition check in
> platform_do_lowpower().
>
> ----------------------------------------------------------------
> Tomasz Figa (1):
> ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
>
> arch/arm/mach-exynos/hotplug.c | 10 ++++++----
> arch/arm/mach-exynos/platsmp.c | 34 +++++++++++++++++++---------------
> 2 files changed, 25 insertions(+), 19 deletions(-)
Could you also merge this branch to your for-next so we can base other
patches for 3.17 on this fix?
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v7 4/6] ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
From: Tomasz Figa @ 2014-07-19 13:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-5-git-send-email-thomas.ab@samsung.com>
Hi Thomas,
Please see my comments inline.
On 14.07.2014 15:38, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> For Exynos 4210/5250/5420 based platforms, add CPU nodes, operating points and
> cpu clock data for migrating from Exynos specific cpufreq driver to using
> generic cpufreq drivers.
[snip]
> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index ee3001f..c3a73bf 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -31,6 +31,33 @@
> pinctrl2 = &pinctrl_2;
> };
>
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + cpu at 0 {
nit: Missing blank line after last property.
The cluster ID field of MPIDR on Exynos4210 is 0x9 not zero, which means
that this should be cpu at 900.
> + device_type = "cpu";
> + compatible = "arm,cortex-a9";
> + reg = <0>;
reg = <0x900>;
> + clocks = <&clock CLK_ARM_CLK>;
> + clock-names = "cpu";
> +
> + operating-points = <
> + 1200000 1250000
> + 1000000 1150000
> + 800000 1075000
> + 500000 975000
> + 400000 975000
> + 200000 950000
> + >;
> + };
> +
> + cpu at 1 {
cpu at 901
> + device_type = "cpu";
> + compatible = "arm,cortex-a9";
> + reg = <1>;
reg = <0x901>;
> + };
In general this wouldn't have even booted, because there were several
places where code relied on CPUs being 0, 1, 2... However I have sent
necessary fixes and they should hit linux-next in few days.
> + };
> +
> sysram at 02020000 {
> compatible = "mmio-sram";
> reg = <0x02020000 0x20000>;
[snip]
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 834fb5a..66b0f98 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -63,6 +63,29 @@
> compatible = "arm,cortex-a15";
> reg = <0>;
> clock-frequency = <1700000000>;
> +
> + clocks = <&clock CLK_ARM_CLK>;
> + clock-names = "cpu";
> +
> + operating-points = <
> + 1700000 1300000
> + 1600000 1250000
> + 1500000 1225000
> + 1400000 1200000
> + 1300000 1150000
> + 1200000 1125000
> + 1100000 1100000
> + 1000000 1075000
> + 900000 1050000
> + 800000 1025000
> + 700000 1012500
> + 600000 1000000
> + 500000 975000
> + 400000 950000
> + 300000 937500
> + 200000 925000
> + >;
> + clock-latency = <200000>;
I don't see this property specified for Exynos4210. Have you missed it
there?
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v7 5/6] ARM: Exynos: switch to using generic cpufreq driver for exynos4210/5250/5420
From: Tomasz Figa @ 2014-07-19 13:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-6-git-send-email-thomas.ab@samsung.com>
On 14.07.2014 15:38, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Remove the platform device instantiation for exynos cpufreq driver and add the
> platform device for generic cpufreq drivers.
>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Tested-by: Arjun K.V <arjun.kv@samsung.com>
> ---
> arch/arm/mach-exynos/exynos.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index 2a43a17..5028b35 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -183,7 +183,20 @@ void __init exynos_cpuidle_init(void)
>
> void __init exynos_cpufreq_init(void)
> {
> - platform_device_register_simple("exynos-cpufreq", -1, NULL, 0);
> + char *dev_name;
> +
> + if (of_machine_is_compatible("samsung,exynos5440"))
> + return;
The original code registers the device unconditionally. Why Exynos5440
is excluded now?
> + if (of_machine_is_compatible("samsung,exynos5420"))
> + dev_name = "arm-bL-cpufreq-dt";
> + else
> + if (of_machine_is_compatible("samsung,exynos4412") ||
> + of_machine_is_compatible("samsung,exynos4212"))
> + dev_name = "exynos-cpufreq";
> + else
> + dev_name = "cpufreq-cpu0";
> +
> + platform_device_register_simple(dev_name, -1, NULL, 0);
> }
How about rewriting this to:
static const struct of_device_id exynos_cpufreq_matches[] = {
{ .compatible = "samsung,exynos5420",
.data = "arm-bL-cpufreq-dt" },
{ .compatible = "samsung,exynos5250",
.data = "cpufreq-cpu0" },
{ .compatible = "samsung,exynos4210",
.data = "cpufreq-cpu0" },
{ /* sentinel */ }
};
void __init exynos_cpufreq_init(void)
{
struct device_node *root = of_find_node_by_path("/");
const struct of_device_id *match;
match = of_match_node(exynos_cpufreq_matches, root);
if (!match) {
platform_device_register_simple("exynos-cpufreq", -1,
NULL, 0);
return;
}
platform_device_register_simple(match->data, -1, NULL, 0);
}
This way it is much more readable and original behavior is preserved for
any SoCs not supported by new drivers.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v7 6/6] cpufreq: exynos: remove exynos4210/5250 specific cpufreq driver support
From: Tomasz Figa @ 2014-07-19 13:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405345118-4269-7-git-send-email-thomas.ab@samsung.com>
Hi,
On 14.07.2014 15:38, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
>
> Exynos4210 and Exynos5250 based platforms have switched over to use generic
> cpufreq drivers for cpufreq functionality. So the Exynos specific cpufreq
> drivers for these platforms can be removed.
>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Tested-by: Arjun K.V <arjun.kv@samsung.com>
> ---
> drivers/cpufreq/Kconfig.arm | 22 ----
> drivers/cpufreq/Makefile | 2 -
> drivers/cpufreq/exynos4210-cpufreq.c | 184 -----------------------------
> drivers/cpufreq/exynos5250-cpufreq.c | 210 ----------------------------------
> 4 files changed, 418 deletions(-)
> delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
> delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c
>
I would hesitate with removing this driver straightaway and replacing it
with a new one that isn't yet well tested. For at least one release I
would keep the old code in place with the possibility of switching
between old and new one in Kconfig.
Best regards,
Tomasz
^ permalink raw reply
* [GIT PULL] ARM: SPEAr13xx PCIe updates for v3.17
From: Viresh Kumar @ 2014-07-19 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140719045804.GC20064@quad.lixom.net>
On 19 July 2014 10:28, Olof Johansson <olof@lixom.net> wrote:
> Looks good. Thanks for following up on this and fixing up stuff.
Thanks for picking it up.
^ permalink raw reply
* [GIT PULL] ARM: SPEAr13xx PCIe updates for v3.17
From: Viresh Kumar @ 2014-07-19 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140719053359.GF20064@quad.lixom.net>
On 19 July 2014 11:03, Olof Johansson <olof@lixom.net> wrote:
> Also, there's a conflict with one of the fixes that were sent earlier.
> It'd been nice to keep this based on the -rc that had that fix to avoid the
> conflict. Please check the contents in our for-next.
I wasn't aware of any such issues, otherwise I would have rebased
it over latest rc.
I kept it on rc3 as I thought people might want to look at diffs
between v8, 9 and 10 and v8 was rebased over rc3. So never changed
it.
^ permalink raw reply
* [RFC] cpufreq: Add bindings for CPU clock sharing topology
From: Viresh Kumar @ 2014-07-19 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMhuNzVtUkaUjF+JjNgHcgf08WiM0DG-kzwtcyUxkK_zow@mail.gmail.com>
On 19 July 2014 03:22, Olof Johansson <olof@lixom.net> wrote:
> What is the current API that is being broken, in your opinion?
So, currently the nodes doesn't have any such property. And drivers
consider all of them as sharing clocks, for eg: cpufreq-cpu0.
Now, if we use those older DT's after the new changes, drivers would
consider CPUs as having separate clocks. And that would be opposite
of what currently happens.
Not sure if this counts as broken.
>> But if that isn't the case, the bindings are very simple & clear to handle.
>> Diff for new bindings:
>
> It's somewhat confusing to see a diff to the patch instead of a new
> version. It seems to remove the cpu 0 entry now?
Not really, I removed an unwanted example. This is how it looks:
* Generic CPUFreq clock bindings
Clock lines may or may not be shared among different CPUs on a platform.
Possible configurations:
1.) All CPUs share a single clock line
2.) All CPUs have independent clock lines
3.) CPUs within a group/cluster share clock line but each group/cluster have a
separate line for itself
Optional Properties:
- clock-master: Contains phandle of the master cpu controlling clocks.
Ideally there is nothing like a "master" CPU as any CPU can play with DVFS
settings. But we have to choose one cpu out of a group, so that others can
point to it.
If there is no "clock-master" property for a cpu node, it is considered as
master. It may or may not have other slave CPUs pointing towards it.
Examples:
1.) All CPUs share a single clock line
cpus {
#address-cells = <1>;
#size-cells = <0>;
cpu0: cpu at 0 {
compatible = "arm,cortex-a15";
reg = <0>;
next-level-cache = <&L2>;
operating-points = <
/* kHz uV */
792000 1100000
396000 950000
198000 850000
>;
clock-latency = <61036>; /* two CLK32 periods */
};
cpu1: cpu at 1 {
compatible = "arm,cortex-a15";
reg = <1>;
next-level-cache = <&L2>;
clock-master = <&cpu0>;
};
};
2.) All CPUs have independent clock lines
cpus {
#address-cells = <1>;
#size-cells = <0>;
cpu0: cpu at 0 {
compatible = "arm,cortex-a15";
reg = <0>;
next-level-cache = <&L2>;
operating-points = <
/* kHz uV */
792000 1100000
396000 950000
198000 850000
>;
clock-latency = <61036>; /* two CLK32 periods */
};
cpu1: cpu at 1 {
compatible = "arm,cortex-a15";
reg = <1>;
next-level-cache = <&L2>;
operating-points = <
/* kHz uV */
792000 1100000
396000 950000
198000 850000
>;
clock-latency = <61036>; /* two CLK32 periods */
};
};
3.) CPUs within a group/cluster share single clock line but each group/cluster
have a separate line for itself
cpus {
#address-cells = <1>;
#size-cells = <0>;
cpu0: cpu at 0 {
compatible = "arm,cortex-a15";
reg = <0>;
next-level-cache = <&L2>;
operating-points = <
/* kHz uV */
792000 1100000
396000 950000
198000 850000
>;
clock-latency = <61036>; /* two CLK32 periods */
};
cpu1: cpu at 1 {
compatible = "arm,cortex-a15";
reg = <1>;
next-level-cache = <&L2>;
clock-master = <&cpu0>;
};
cpu2: cpu at 100 {
compatible = "arm,cortex-a7";
reg = <100>;
next-level-cache = <&L2>;
operating-points = <
/* kHz uV */
792000 950000
396000 750000
198000 450000
>;
clock-latency = <61036>; /* two CLK32 periods */
};
cpu3: cpu at 101 {
compatible = "arm,cortex-a7";
reg = <101>;
next-level-cache = <&L2>;
clock-master = <&cpu2>;
};
};
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox