* [PATCH v3 1/7] cpufreq: cpufreq-cpu0: allow use of optional boost mode frequencies
From: Tomasz Figa @ 2014-02-13 11:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJuA9agoK4vO+1mTktgc6BoHLO8qR2482uizP1LbG5zgy9Q4kA@mail.gmail.com>
On 13.02.2014 09:02, Thomas Abraham wrote:
> On Wed, Feb 12, 2014 at 8:28 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Thomas,
>>
>>
>> On 07.02.2014 16:55, Thomas Abraham wrote:
[snip]
>>> diff --git a/drivers/cpufreq/cpufreq-cpu0.c
>>> b/drivers/cpufreq/cpufreq-cpu0.c
>>> index 0c12ffc..06539eb 100644
>>> --- a/drivers/cpufreq/cpufreq-cpu0.c
>>> +++ b/drivers/cpufreq/cpufreq-cpu0.c
>>> @@ -195,6 +195,9 @@ static int cpu0_cpufreq_probe(struct platform_device
>>> *pdev)
>>> transition_latency += ret * 1000;
>>> }
>>>
>>> + if (of_find_property(cpu_dev->of_node, "boost-frequency", NULL))
>>> + cpu0_cpufreq_driver.boost_supported = true;
>>> +
>>> ret = cpufreq_register_driver(&cpu0_cpufreq_driver);
>>> if (ret) {
>>> pr_err("failed register driver: %d\n", ret);
>>>
>>
>> I'd say that boost should be enabled depending on user's preference, as done
>> before in Exynos cpufreq driver. So both presence of boost-frequency
>> property and state of CPU_FREQ_BOOST_SW should be considered.
>>
>> As for CPU_FREQ_BOOST_SW, I don't think it should be always selected, but
>> ather, either converted to a user-selectable bool entry or made selectable
>> by other entry, like current ARM_EXYNOS_CPU_FREQ_BOOST_SW.
>
> We still will have the same problem on Exynos multi-platform kernel
> where one Exynos platform needs it and others don't. Same with just
> using the CPU_FREQ_BOOST_SW config option. So that was the reason to
> just fallback on presence of boost property.
I don't think we really have a problem here, because we have well
defined semantics for particular enable methods:
- Kconfig is supposed to be a global enable - if an option is
disabled, it is not even built into the kernel and can be used in any
way - this is per-user choice, regardless of platform the image is going
to be running on,
- device tree is supposed to be telling us whether the hardware we are
running on supports given feature and all the required data to enable
it, if yes,
- then, for per system configuration, you should be able to
enable/disable given feature by a command line parameter, e.g.
cpufreq.boost_disable.
If you follow the above description, you should be able to get any
configuration you want on any system, as long as it's supported by hardware.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH 03/22] efi: add helper function to get UEFI params from FDT
From: Matt Fleming @ 2014-02-13 11:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391619853-10601-4-git-send-email-leif.lindholm@linaro.org>
On Wed, 05 Feb, at 05:03:54PM, Leif Lindholm wrote:
> From: Mark Salter <msalter@redhat.com>
>
> ARM and ARM64 architectures use the device tree to pass UEFI parameters
> from stub to kernel. These parameters are things known to the stub but
> not discoverable by the kernel after the stub calls ExitBootSerives().
> There is a helper function in:
>
> drivers/firmware/efi/fdt.c
>
> which the stub uses to add the UEFI parameters to the device tree.
> This patch adds a complimentary helper function which UEFI runtime
> support may use to retrieve the parameters from the device tree.
> If an architecture wants to use this helper, it should select
> CONFIG_UEFI_PARAMS_FROM_FDT.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> drivers/firmware/efi/Kconfig | 7 ++++
> drivers/firmware/efi/efi.c | 79 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/efi.h | 9 +++++
> 3 files changed, 95 insertions(+)
Is there a particular reason this change is being added to
drivers/firmware/efi/efi.c and not drivers/firmware/efi/fdt.c?
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply
* [PATCH 06/22] Add helper functions used by arm/arm64
From: Matt Fleming @ 2014-02-13 11:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1391619853-10601-7-git-send-email-leif.lindholm@linaro.org>
On Wed, 05 Feb, at 05:03:57PM, Leif Lindholm wrote:
> From: Roy Franz <roy.franz@linaro.org>
>
> Add the get_dram_base() function and efi_call_physN() macros
> that are shared by arm/arm64.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> drivers/firmware/efi/efi-stub-helper.c | 63 +++++++++++++++++++++++++-------
> 1 file changed, 50 insertions(+), 13 deletions(-)
These changes should be in drivers/firmware/efi/arm-stub.c -
efi-stub-helper.c is not a dumping ground for random
architecture-specific code.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply
* [PATCH] pci: Add support for creating a generic host_bridge from device tree
From: Arnd Bergmann @ 2014-02-13 11:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <000001cf2899$a6eb75b0$f4c26110$%han@samsung.com>
On Thursday 13 February 2014 17:57:41 Jingoo Han wrote:
> I want to use 'drivers/pci/host/pcie-designware.c' for both arm32
> and arm64, without any code changes. However, it looks impossible.
It is impossible at the moment, and I agree we have to fix that.
> I made 'drivers/pci/host/pcie-designware.c' based on 32bit arm PCI
> support. Then, with Liviu's patch, do I have to make new code for arm64,
> even though the same HW PCIe IP is used?
>
> - For arm32
> drivers/pci/host/pcie-designware.c
>
> - For arm64
> drivers/pci/host/pcie-designware-arm64.c
As a start, I'd suggest using "#ifdef CONFIG_ARM" in the driver,
but sharing as much code as you can. We should try to make the #else
section of the #ifdef architecture independent and get have the arm64
implementation shared with any architecture that doesn't have or want
its own pcibios32.c implementation.
> > > I am reviewing and compiling your patch.
> > > Would you consider adding 'struct pci_sys_data' and 'struct hw_pci'?
I would rather get rid of struct hw_pci for architecture independent
drivers and add a different registration method on arm32 that is
compatible with what we come up with on arm64. The main purpose of
hw_pci is to allow multiple PCI controllers to be initialized at
once, but we don't actually need that for any of the "modern" platforms
where we already have a probe function that gets called once for
each controller.
As a start, we could add a pci_host_bridge_register() function like
the one below to arm32 and migrate the drivers/pci/host/ drivers
over to use it with little effort. Instead of filling out hw_pci,
these drivers would allocate (by embedding in their device struct)
and fill out pci_sys_data directly. After that, we can gradually
move more code out of the arm32 implementation into common code, if
it doesn't already exist there, up to the point where a host driver
no longer has to call any function in bios32.c.
Arnd
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 317da88..12c2178 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -514,6 +514,26 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
}
}
+static void pci_common_bus_probe(struct pci_bus *bus)
+{
+ if (!pci_has_flag(PCI_PROBE_ONLY)) {
+ /*
+ * Size the bridge windows.
+ */
+ pci_bus_size_bridges(bus);
+
+ /*
+ * Assign resources.
+ */
+ pci_bus_assign_resources(bus);
+ }
+
+ /*
+ * Tell drivers about devices found.
+ */
+ pci_bus_add_devices(bus);
+}
+
void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
{
struct pci_sys_data *sys;
@@ -528,27 +548,38 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
- list_for_each_entry(sys, &head, node) {
- struct pci_bus *bus = sys->bus;
+ list_for_each_entry(sys, &head, node)
+ pci_common_bus_probe(sys->bus);
+}
- if (!pci_has_flag(PCI_PROBE_ONLY)) {
- /*
- * Size the bridge windows.
- */
- pci_bus_size_bridges(bus);
- /*
- * Assign resources.
- */
- pci_bus_assign_resources(bus);
- }
- /*
- * Tell drivers about devices found.
- */
- pci_bus_add_devices(bus);
- }
+
+int pci_host_bridge_register(struct device *parent, struct pci_sys_data *sys, struct pci_ops *ops, int (*setup)(int nr, struct pci_sys_data *))
+{
+ int ret;
+
+ pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+ INIT_LIST_HEAD(&sys->resources);
+
+ ret = setup(0, sys);
+ if (ret)
+ return ret;
+
+ ret = pcibios_init_resources(0, sys);
+ if (ret)
+ return ret;
+
+ sys->bus = pci_scan_root_bus(parent, sys->busnr, ops, sys, &sys->resources);
+ if (!sys->bus)
+ return -ENODEV;
+
+ pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq);
+
+ pci_common_bus_probe(sys->bus);
+ return ret;
}
+EXPORT_SYMBOL_GPL(pci_host_bridge_register);
#ifndef CONFIG_PCI_HOST_ITE8152
void pcibios_set_master(struct pci_dev *dev)
^ permalink raw reply related
* [PATCH v2 3/3] PCI: ARM: add support for generic PCI host controller
From: Arnd Bergmann @ 2014-02-13 11:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213110401.GB13576@mudshark.cambridge.arm.com>
On Thursday 13 February 2014 11:04:02 Will Deacon wrote:
> Hi Arnd,
>
> Thanks again for the comments.
>
> On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:
> > On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > > +- ranges : As described in IEEE Std 1275-1994, but must provide
> > > + at least a definition of one or both of IO and Memory
> > > + Space.
> >
> > I'd say *must* provide at least non-prefetchable memory. *may* provide
> > prefetchable memory and/or I/O space.
>
> Can do. Should I enforce this in the driver too? (i.e. complain if
> non-prefetchable memory is absent).
Yes, good idea.
> > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > > index 47d46c6d8468..491d74c36f6a 100644
> > > --- a/drivers/pci/host/Kconfig
> > > +++ b/drivers/pci/host/Kconfig
> > > @@ -33,4 +33,11 @@ config PCI_RCAR_GEN2
> > > There are 3 internal PCI controllers available with a single
> > > built-in EHCI/OHCI host controller present on each one.
> > >
> > > +config PCI_ARM_GENERIC
> > > + bool "ARM generic PCI host controller"
> > > + depends on ARM && OF
> > > + help
> > > + Say Y here if you want to support a simple generic PCI host
> > > + controller, such as the one emulated by kvmtool.
> > > +
> > > endmenu
> >
> > Looks good for now. In the long run, I'd hope to get rid of the 'ARM'
> > part here and make it work on any architecture, but that requires
> > significant work that we should not depend on here.
>
> Agreed. arm64 is the obvious next target (once Liviu's series is sorted out
> -- I'm currently using a simplified port of bios32.c for testing).
See also the reply I just sent on the previous thread for a migration
plan regarding the existing drivers.
> > > +struct gen_pci_cfg_window {
> > > + u64 cpu_phys;
> > > + void __iomem *base;
> > > + u8 bus;
> > > + spinlock_t lock;
> > > + const struct gen_pci_cfg_accessors *accessors;
> > > +};
> > > +
> > > +struct gen_pci_resource {
> > > + struct list_head list;
> > > + struct resource cpu_res;
> > > + resource_size_t offset;
> > > +};
> >
> > Your gen_pci_resource is actually identical to struct pci_host_bridge_window,
> > which I guess is coincidence, but it would be nice to actually use
> > the standard structure to make it easier to integrate with common
> > infrastructure later.
>
> Ha, at least I managed to come up with the same struct! I'll move to the
> generic version.
Hmm, I fear I was speaking too quickly, the pci_host_bridge_window actually
contains a pointer to the resource rather than the resource itself :(
There is probably a way to do this better, at least once we unify the
probe() and setup() functions.
> > > +static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
> > > + unsigned int devfn,
> > > + int where)
> > > +{
> > > + struct pci_sys_data *sys = bus->sysdata;
> > > + struct gen_pci *pci = sys->private_data;
> > > + u32 busn = bus->number;
> > > +
> > > + spin_lock(&pci->cfg.lock);
> > > + if (pci->cfg.bus != busn) {
> > > + resource_size_t offset;
> > > +
> > > + devm_iounmap(pci->dev, pci->cfg.base);
> > > + offset = pci->cfg.cpu_phys + (busn << 20);
> > > + pci->cfg.base = devm_ioremap(pci->dev, offset, SZ_1M);
> > > + pci->cfg.bus = busn;
> > > + }
> > > +
> > > + return pci->cfg.base + ((devfn << 12) | where);
> > > +}
> >
> > Nice idea, but unfortunately broken: we need config space access from
> > atomic context, since there are drivers doing that.
>
> That aside, I just took a spin_lock so this needs fixing regardless. The
> only solution I can think of then is to map all of the buses at setup time
> (making bus_ranges pretty important) and hope that I don't chew through all
> of vmalloc.
It's possible we have to go further and only map the buses that are
actually used, rather than the ones that are defined for the bus,
but just mapping the entire range is a reasonable start I think.
> > > +
> > > + hw = (struct hw_pci) {
> > > + .nr_controllers = 1,
> > > + .private_data = (void **)&pci,
> > > + .setup = gen_pci_setup,
> > > + .map_irq = of_irq_parse_and_map_pci,
> > > + .ops = &gen_pci_ops,
> > > + };
> > > +
> > > + pci_common_init_dev(dev, &hw);
> > > + return 0;
> > > +}
> >
> > A missing part here seems to be a way to propagate errors from
> > the pci_common_init_dev or gen_pci_setup back to the caller.
> >
> > This seems to be a result of the arm pcibios implementation not
> > being meant for loadable modules, but I suspect it can be fixed.
> > The nr_controllers>1 logic gets a bit in the way there, but it's
> > also a model that seems to be getting out of fashion:
> > kirkwood/dove/orion5x/mv78xx0 use it at the moment, but are
> > migrating over to the new pci-mvebu code that doesn't as they
> > get rid of the non-DT probing. pci-rcar-gen2.c uses it, but it
> > seems they already ran into problems with that and are changing
> > it. That pretty much leaves iop13xx as the only user, but at
> > that point we can probably move the loop into iop13xx specific
> > code.
>
> Makes sense once there are no users of the field.
With the patch I just suggested, we can simply keep
pci_common_init_dev() for older (non-multiplatform) controllers
and not change them at all but move on to something else for
the interesting ones, i.e. those we want to share with arm64.
Arnd
^ permalink raw reply
* [PATCH v3 01/13] ARM: mvebu: rename armada-370-xp.c to armada-mvebu.c
From: Arnd Bergmann @ 2014-02-13 11:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392289475-8902-2-git-send-email-thomas.petazzoni@free-electrons.com>
On Thursday 13 February 2014 12:04:23 Thomas Petazzoni wrote:
> In preparation to the introduction of the support of Armada 375 and
> Armada 38x, this commit renames arch/arm/mach-mvebu/armada-370-xp.c to
> arch/arm/mach-mvebu/armada-mvebu.c. The armada-mvebu.c name was chosen
> because:
>
> * As we are going to merge the support for Kirkwood and Dove into
> mach-mvebu, there will be other files with DT_MACHINE_START
> structures, so a generic name such as board-dt.c or mvebu.c does
> not work.
>
> * A simple armada.c does not work, because there are Marvell Armada
> SOCs that are not part of the MVEBU family. For example, the
> Marvell Armada 1500 are part of the mach-berlin family, which is a
> completely separate line of SOCs.
Your reasoning for the new name makes a lot of sense, but my personal
opinion is that I'd rather leave the name as it is and deal with the
fact that it's not the best name. Renaming files often causes unexpected
problems, in particular if someone else wants to modify the same file.
Arnd
^ permalink raw reply
* [PATCH] pci: Add support for creating a generic host_bridge from device tree
From: Russell King - ARM Linux @ 2014-02-13 11:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3204351.WykFFcX4zJ@wuerfel>
On Thu, Feb 13, 2014 at 12:27:05PM +0100, Arnd Bergmann wrote:
> I would rather get rid of struct hw_pci for architecture independent
> drivers and add a different registration method on arm32 that is
> compatible with what we come up with on arm64. The main purpose of
> hw_pci is to allow multiple PCI controllers to be initialized at
> once, but we don't actually need that for any of the "modern" platforms
> where we already have a probe function that gets called once for
> each controller.
No. The main purpose of hw_pci is as a container to support multiple
different platform specific PCI implementations in one kernel. It's
exactly what you need for single zImage.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply
* [PATCH v3 01/13] ARM: mvebu: rename armada-370-xp.c to armada-mvebu.c
From: Thomas Petazzoni @ 2014-02-13 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2036226.egxWL7qC66@wuerfel>
Dear Arnd Bergmann,
On Thu, 13 Feb 2014 12:50:15 +0100, Arnd Bergmann wrote:
> On Thursday 13 February 2014 12:04:23 Thomas Petazzoni wrote:
> > In preparation to the introduction of the support of Armada 375 and
> > Armada 38x, this commit renames arch/arm/mach-mvebu/armada-370-xp.c to
> > arch/arm/mach-mvebu/armada-mvebu.c. The armada-mvebu.c name was chosen
> > because:
> >
> > * As we are going to merge the support for Kirkwood and Dove into
> > mach-mvebu, there will be other files with DT_MACHINE_START
> > structures, so a generic name such as board-dt.c or mvebu.c does
> > not work.
> >
> > * A simple armada.c does not work, because there are Marvell Armada
> > SOCs that are not part of the MVEBU family. For example, the
> > Marvell Armada 1500 are part of the mach-berlin family, which is a
> > completely separate line of SOCs.
>
> Your reasoning for the new name makes a lot of sense, but my personal
> opinion is that I'd rather leave the name as it is and deal with the
> fact that it's not the best name. Renaming files often causes unexpected
> problems, in particular if someone else wants to modify the same file.
I believe it's a matter of taste here. Having a file named
armada-370-xp.c that handles Armada 375 and Armada 38x looks highly
confusing to me, and I believe both Gr?gory and Ezequiel were of the
same opinion.
The number of changes to this file is very limited, so the probability
of having a large number of complicated patches touching the same file
being in flight is fairly low.
Maybe we can leave this taste decision to the mach-mvebu maintainers?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v2 3/3] PCI: ARM: add support for generic PCI host controller
From: Will Deacon @ 2014-02-13 12:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <348076699.SKFqCqFVHd@wuerfel>
On Thu, Feb 13, 2014 at 11:47:46AM +0000, Arnd Bergmann wrote:
> On Thursday 13 February 2014 11:04:02 Will Deacon wrote:
> > On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:
> > > On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > > > +struct gen_pci_resource {
> > > > + struct list_head list;
> > > > + struct resource cpu_res;
> > > > + resource_size_t offset;
> > > > +};
> > >
> > > Your gen_pci_resource is actually identical to struct pci_host_bridge_window,
> > > which I guess is coincidence, but it would be nice to actually use
> > > the standard structure to make it easier to integrate with common
> > > infrastructure later.
> >
> > Ha, at least I managed to come up with the same struct! I'll move to the
> > generic version.
>
> Hmm, I fear I was speaking too quickly, the pci_host_bridge_window actually
> contains a pointer to the resource rather than the resource itself :(
I can allocate the resources dynamically as I parse them, not a problem at
all.
> There is probably a way to do this better, at least once we unify the
> probe() and setup() functions.
Yes, I fully expect this to be iterative.
> > > > + spin_lock(&pci->cfg.lock);
> > > > + if (pci->cfg.bus != busn) {
> > > > + resource_size_t offset;
> > > > +
> > > > + devm_iounmap(pci->dev, pci->cfg.base);
> > > > + offset = pci->cfg.cpu_phys + (busn << 20);
> > > > + pci->cfg.base = devm_ioremap(pci->dev, offset, SZ_1M);
> > > > + pci->cfg.bus = busn;
> > > > + }
> > >
> > > Nice idea, but unfortunately broken: we need config space access from
> > > atomic context, since there are drivers doing that.
> >
> > That aside, I just took a spin_lock so this needs fixing regardless. The
> > only solution I can think of then is to map all of the buses at setup time
> > (making bus_ranges pretty important) and hope that I don't chew through all
> > of vmalloc.
>
> It's possible we have to go further and only map the buses that are
> actually used, rather than the ones that are defined for the bus,
> but just mapping the entire range is a reasonable start I think.
Okey doke.
> > > > + hw = (struct hw_pci) {
> > > > + .nr_controllers = 1,
> > > > + .private_data = (void **)&pci,
> > > > + .setup = gen_pci_setup,
> > > > + .map_irq = of_irq_parse_and_map_pci,
> > > > + .ops = &gen_pci_ops,
> > > > + };
> > > > +
> > > > + pci_common_init_dev(dev, &hw);
> > > > + return 0;
> > > > +}
> > >
> > > A missing part here seems to be a way to propagate errors from
> > > the pci_common_init_dev or gen_pci_setup back to the caller.
> > >
> > > This seems to be a result of the arm pcibios implementation not
> > > being meant for loadable modules, but I suspect it can be fixed.
> > > The nr_controllers>1 logic gets a bit in the way there, but it's
> > > also a model that seems to be getting out of fashion:
> > > kirkwood/dove/orion5x/mv78xx0 use it at the moment, but are
> > > migrating over to the new pci-mvebu code that doesn't as they
> > > get rid of the non-DT probing. pci-rcar-gen2.c uses it, but it
> > > seems they already ran into problems with that and are changing
> > > it. That pretty much leaves iop13xx as the only user, but at
> > > that point we can probably move the loop into iop13xx specific
> > > code.
> >
> > Makes sense once there are no users of the field.
>
> With the patch I just suggested, we can simply keep
> pci_common_init_dev() for older (non-multiplatform) controllers
> and not change them at all but move on to something else for
> the interesting ones, i.e. those we want to share with arm64.
Yes, that looks sensible. An alternative is to create an hw_pci in
pci_host_bridge_register with nr_controllers = 1 then call
pci_common_init_dev. It would remove slightly more code, but obviously ties
the thing to arm.
Will
^ permalink raw reply
* [PATCH] pci: Add support for creating a generic host_bridge from device tree
From: Arnd Bergmann @ 2014-02-13 12:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213115327.GV26684@n2100.arm.linux.org.uk>
On Thursday 13 February 2014 11:53:27 Russell King - ARM Linux wrote:
> On Thu, Feb 13, 2014 at 12:27:05PM +0100, Arnd Bergmann wrote:
> > I would rather get rid of struct hw_pci for architecture independent
> > drivers and add a different registration method on arm32 that is
> > compatible with what we come up with on arm64. The main purpose of
> > hw_pci is to allow multiple PCI controllers to be initialized at
> > once, but we don't actually need that for any of the "modern" platforms
> > where we already have a probe function that gets called once for
> > each controller.
>
> No. The main purpose of hw_pci is as a container to support multiple
> different platform specific PCI implementations in one kernel. It's
> exactly what you need for single zImage.
Well, we definitely need something to manage the assignment of domains,
bus numbers and I/O space windows, but the main issue I see with existing
hw_pci container is that it assumes that you can pass give it all
host bridges for a given domain at once. The problem with this is
that the pci host bridge drivers don't interact with one another, so
a system that needs two different PCI host drivers can't use hw_pci
to register them both, unless we come up with some extra
infrastructure.
Also, the calling conventions for pci_common_init_dev() mean that
it's hard to propagate -EPROBE_DEFER errors back to the driver
probe function, so it seems easier to come up with something new
that deals with all issues at once and that is outside of architecture
specific code.
Arnd
^ permalink raw reply
* [PATCH] pci: Add support for creating a generic host_bridge from device tree
From: Liviu Dudau @ 2014-02-13 12:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <000001cf2899$a6eb75b0$f4c26110$%han@samsung.com>
On Thu, Feb 13, 2014 at 08:57:41AM +0000, Jingoo Han wrote:
>
>
> > -----Original Message-----
> > From: Tanmay Inamdar [mailto:tinamdar at apm.com]
> > Sent: Thursday, February 13, 2014 5:37 PM
> > To: Jingoo Han
> > Cc: Liviu Dudau; Arnd Bergmann; devicetree at vger.kernel.org; linaro-kernel; linux-pci; Will Deacon;
> > LKML; Catalin Marinas; Bjorn Helgaas; LAKML
> > Subject: Re: [PATCH] pci: Add support for creating a generic host_bridge from device tree
> >
> > On Thu, Feb 13, 2014 at 12:10 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Thursday, February 06, 2014 7:18 PM, Liviu Dudau wrote:
> > >> On Wed, Feb 05, 2014 at 10:26:27PM +0000, Tanmay Inamdar wrote:
> > >> > Hello Liviu,
> > >> >
> > >> > I did not get the first email of this particular patch on any of
> > >> > subscribed mailing lists (don't know why), hence replying here.
> > >>
> > >> Strange, it shows in the MARC and GMANE archive for linux-pci, probably
> > >> a hickup on your receiving side?
> > >>
> > >> >
> > >> > +struct pci_host_bridge *
> > >> > +pci_host_bridge_of_init(struct device *parent, int busno, struct pci_ops *ops,
> > >> > + void *host_data, struct list_head *resources)
> > >> > +{
> > >> > + struct pci_bus *root_bus;
> > >> > + struct pci_host_bridge *bridge;
> > >> > +
> > >> > + /* first parse the host bridge bus ranges */
> > >> > + if (pci_host_bridge_of_get_ranges(parent->of_node, resources))
> > >> > + return NULL;
> > >> > +
> > >> > + /* then create the root bus */
> > >> > + root_bus = pci_create_root_bus(parent, busno, ops, host_data, resources);
> > >> > + if (!root_bus)
> > >> > + return NULL;
> > >> > +
> > >> > + bridge = to_pci_host_bridge(root_bus->bridge);
> > >> > +
> > >> > + return bridge;
> > >> > +}
> > >> >
> > >> > You are keeping the domain_nr inside pci_host_bridge structure. In
> > >> > above API, domain_nr is required in 'pci_find_bus' function called
> > >> > from 'pci_create_root_bus'. Since the bridge is allocated after
> > >> > creating root bus, 'pci_find_bus' always gets domain_nr as 0. This
> > >> > will cause problem for scanning multiple domains.
> > >>
> > >> Good catch. I was switching between creating a pci_controller in arch/arm64 and
> > >> adding the needed bits in pci_host_bridge. After internal review I've decided to
> > >> add the domain_nr to pci_host_bridge, but forgot to update the code everywhere.
> > >
> > > Hi Liviu Dudau,
> > >
> > > One more thing,
> > > I am reviewing and compiling your patch.
> > > Would you consider adding 'struct pci_sys_data' and 'struct hw_pci'?
> > >
> > > Currently, 4 PCIe Host drivers (pci-mvebu.c, pci-tegra.c,
> > > pci-rcar-gen2.c, pcie-designware.c) are using 'struct pci_sys_data'
> > > and 'struct hw_pci' in their drivers. Without this, it makes build
> > > errors.
> > >
> > > In arm32, 'struct pci_sys_data' and 'struct hw_pci' is defined
> > > in "arch/arm/include/asm/mach/pci.h".
> > >
> > > Tanmay Inamdar,
> > > Your 'APM X-Gene PCIe' patch also needs 'struct pci_sys_data' and
> > > 'struct hw_pci'. With Liviu Dudau's patch, it will make build
> > > errors. Would you check this?
> >
> > X-Gene PCIe host driver is dependent on arm64 PCI patch. My previous
> > approach was based on 32bit arm PCI support. With Liviu's approach, I
> > will have to make changes in host driver to get rid of hw_pci and
> > pci_sys_data which are no longer required.
>
> I want to use 'drivers/pci/host/pcie-designware.c' for both arm32
> and arm64, without any code changes. However, it looks impossible.
>
> I made 'drivers/pci/host/pcie-designware.c' based on 32bit arm PCI
> support. Then, with Liviu's patch, do I have to make new code for arm64,
> even though the same HW PCIe IP is used?
Hi Jingoo,
Arnd has asked about the transition path for 32bit arm PCI host bridges
and I don't think we came up with a solution yet. My preferred solution
would be to modify the arch/arm API to be more in line with the generic
code and not have to use hw_pci and pci_sys_data anymore. That should
fix your problem of having a single code base for your host controller.
If you look at the pci_sys_data and hw_pci structures, you can see that
there is a bit of duplication between the two and also that some of the
data contained in that structure is generic enough to be contained in
the PCI infrastructure code rather than down in the arch-dependent code.
I confess that I have no in-depth knowledge of the reasons why the arm
code looks like this and if it is still required with the current
infrastructure code. Russell can provide us with entertaining stories here,
I'm sure, on why the code looks like it does.
I have not done any work in this area yet, but if I were to update the
generic arm code I would try to make the code use the pci_host_bridge
structure rather than pci_sys_data. The hw_pci will disappear with the new
APIs.
>
> - For arm32
> drivers/pci/host/pcie-designware.c
>
> - For arm64
> drivers/pci/host/pcie-designware-arm64.c
>
>
> >
> > IMO it should not cause build errors for PCI host drivers dependent on
> > architectures other than arm64. Can you post the error?
> >
>
> I post the build errors.
IMHO it is not worth trying to have the host bridge code cope with two APIs.
It's going to lead to problems no matter what.
Best regards,
Liviu
>
> CC drivers/pci/host/pcie-designware.o
> CHK kernel/config_data.h
> drivers/pci/host/pcie-designware.c:72:52: warning: 'struct pci_sys_data' declared inside parameter list [enabled by default]
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> ^
> drivers/pci/host/pcie-designware.c:72:52: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
> drivers/pci/host/pcie-designware.c: In function 'sys_to_pcie'
> drivers/pci/host/pcie-designware.c:74:12: error: dereferencing pointer to incomplete type
> return sys->private_data;
> ^
> drivers/pci/host/pcie-designware.c: In function 'dw_pcie_msi_map'
> drivers/pci/host/pcie-designware.c:384:2: error: implicit declaration of function 'set_irq_flags' [-Werror=implicit-function-declaration]
> set_irq_flags(irq, IRQF_VALID);
> ^
> drivers/pci/host/pcie-designware.c:384:21: error: 'IRQF_VALID??undeclared (first use in this function)
> set_irq_flags(irq, IRQF_VALID);
> ^
> drivers/pci/host/pcie-designware.c:384:21: note: each undeclared identifier is reported only once for each function it appears in
> drivers/pci/host/pcie-designware.c: In function 'dw_pcie_host_init'
> drivers/pci/host/pcie-designware.c:492:2: error: invalid use of undefined type 'struct hw_pci'
> dw_pci.nr_controllers = 1;
> ^
> drivers/pci/host/pcie-designware.c:493:2: error: invalid use of undefined type 'struct hw_pci'
> dw_pci.private_data = (void **)&pp;
> ^
> drivers/pci/host/pcie-designware.c:495:2: error: implicit declaration of function 'pci_common_init' [-Werror=implicit-function-declaration]
> pci_common_init(&dw_pci);
> ^
> drivers/pci/host/pcie-designware.c:498:2: error: invalid use of undefined type 'struct hw_pci'
> dw_pci.domain++;
> ^
> drivers/pci/host/pcie-designware.c: At top level:
> drivers/pci/host/pcie-designware.c:698:41: warning: 'struct pci_sys_data??declared inside parameter list [enabled by default]
> static int dw_pcie_setup(int nr, struct pci_sys_data *sys)
> ^
> drivers/pci/host/pcie-designware.c: In function 'dw_pcie_setup'
> drivers/pci/host/pcie-designware.c:702:2: warning: passing argument 1 of 'sys_to_pcie' from incompatible pointer type [enabled by default]
> pp = sys_to_pcie(sys);
> ^
> drivers/pci/host/pcie-designware.c:72:33: note: expected 'struct pci_sys_data *' but argument is of type 'struct pci_sys_data *'
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> ^
> drivers/pci/host/pcie-designware.c:708:6: error: dereferencing pointer to incomplete type
> sys->io_offset = global_io_offset - pp->config.io_bus_addr;
> ^
> drivers/pci/host/pcie-designware.c:711:31: error: dereferencing pointer to incomplete type
> pci_add_resource_offset(&sys->resources, &pp->io,
> ^
> drivers/pci/host/pcie-designware.c:712:9: error: dereferencing pointer to incomplete type
> sys->io_offset);
> ^
> drivers/pci/host/pcie-designware.c:715:5: error: dereferencing pointer to incomplete type
> sys->mem_offset = pp->mem.start - pp->config.mem_bus_addr;
> ^
> drivers/pci/host/pcie-designware.c:716:30: error: dereferencing pointer to incomplete type
> pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
> ^
> drivers/pci/host/pcie-designware.c:716:56: error: dereferencing pointer to incomplete type
> pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset);
> ^
> drivers/pci/host/pcie-designware.c: At top level:
> drivers/pci/host/pcie-designware.c:721:56: warning: 'struct pci_sys_data' declared inside parameter list [enabled by default]
> static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> ^
> drivers/pci/host/pcie-designware.c: In function 'dw_pcie_scan_bus'
> drivers/pci/host/pcie-designware.c:724:9: warning: passing argument 1 of 'sys_to_pcie' from incompatible pointer type [enabled by default]
> struct pcie_port *pp = sys_to_pcie(sys);
> ^
> drivers/pci/host/pcie-designware.c:72:33: note: expected 'struct pci_sys_data *' but argument is of type 'sruct pci_sys_data *'
> static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
> ^
> drivers/pci/host/pcie-designware.c:727:24: error: dereferencing pointer to incomplete type
> pp->root_bus_nr = sys->busnr;
> ^
> drivers/pci/host/pcie-designware.c:728:36: error: dereferencing pointer to incomplete type
> bus = pci_scan_root_bus(NULL, sys->busnr, &dw_pcie_ops,
> ^
> drivers/pci/host/pcie-designware.c:729:15: error: dereferencing pointer to incomplete type
> sys, &sys->resources);
> ^
> drivers/pci/host/pcie-designware.c: At top level:
> drivers/pci/host/pcie-designware.c:755:15: error: variable 'dw_pci' has initializer but incomplete type
> static struct hw_pci dw_pci = {
> ^
> drivers/pci/host/pcie-designware.c:756:2: error: unknown field 'setup' specified in initializer
> .setup = dw_pcie_setup,
> ^
> drivers/pci/host/pcie-designware.c:756:2: warning: excess elements in struct initializer [enabled by default]
> drivers/pci/host/pcie-designware.c:756:2: warning: (near initialization for 'dw_pci' [enabled by default]
> drivers/pci/host/pcie-designware.c:757:2: error: unknown field 'scan' specified in initializer
> .scan = dw_pcie_scan_bus,
> ^
> drivers/pci/host/pcie-designware.c:757:2: warning: excess elements in struct initializer [enabled by default]
> drivers/pci/host/pcie-designware.c:757:2: warning: (near initialization for 'dw_pci' [enabled by default]
> drivers/pci/host/pcie-designware.c:758:2: error: unknown field 'map_irq' specified in initializer
> .map_irq = dw_pcie_map_irq,
> ^
> drivers/pci/host/pcie-designware.c:758:2: warning: excess elements in struct initializer [enabled by default]
> drivers/pci/host/pcie-designware.c:758:2: warning: (near initialization for 'dw_pci' [enabled by default]
> drivers/pci/host/pcie-designware.c:759:2: error: unknown field 'add_bus' specified in initializer
> .add_bus = dw_pcie_add_bus,
> ^
> drivers/pci/host/pcie-designware.c:759:2: warning: excess elements in struct initializer [enabled by default]
> drivers/pci/host/pcie-designware.c:759:2: warning: (near initialization for 'dw_pci' [enabled by default]
> cc1: some warnings being treated as errors
> make[3]: *** [drivers/pci/host/pcie-designware.o] Error 1
>
>
> > >
> > >>
> > >> Thanks for reviewing this, will fix in v2.
> > >>
> > >> Do you find porting to the new API straight forward?
> > >>
> > >
>
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v2 3/3] PCI: ARM: add support for generic PCI host controller
From: Arnd Bergmann @ 2014-02-13 12:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213120042.GE13576@mudshark.cambridge.arm.com>
On Thursday 13 February 2014 12:00:42 Will Deacon wrote:
> > With the patch I just suggested, we can simply keep
> > pci_common_init_dev() for older (non-multiplatform) controllers
> > and not change them at all but move on to something else for
> > the interesting ones, i.e. those we want to share with arm64.
>
> Yes, that looks sensible. An alternative is to create an hw_pci in
> pci_host_bridge_register with nr_controllers = 1 then call
> pci_common_init_dev. It would remove slightly more code, but obviously ties
> the thing to arm.
You'd still need to pass all the contents of the hw_pci struct that
get copied into pci_sys_data, so that's not different from passing
hw_pci as we do today.
Arnd
^ permalink raw reply
* [PATCH v2 2/3] ARM: bios32: use pci_enable_resource to enable PCI resources
From: Jingoo Han @ 2014-02-13 12:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392236171-10512-3-git-send-email-will.deacon@arm.com>
On Thursday, February 13, 2014 5:16 AM, Will Deacon wrote:
>
> This patch moves bios32 over to using the generic code for enabling PCI
> resources. Since the core code takes care of bridge resources too, we
> can also drop the explicit IO and MEMORY enabling for them in the arch
> code.
>
> A side-effect of this change is that we no longer explicitly enable
> devices when running in PCI_PROBE_ONLY mode. This stays closer to the
> meaning of the option and prevents us from trying to enable devices
> without any assigned resources (the core code refuses to enable
> resources without parents).
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
I tested this patch with NIC on Exynos platform.
It works properly.
Tested-by: Jingoo Han <jg1.han@samsung.com>
[ 0.406431] PCI: bus1: Fast back to back transfers disabled
[ 0.411912] pci_bus 0000:01: busn_res: [bus 01-ff] end is updated to 01
[ 0.418608] pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 01
[ 0.425351] pci 0000:00:00.0: BAR 8: assigned [mem 0x40100000-0x401fffff]
[ 0.432144] pci 0000:00:00.0: BAR 9: assigned [mem 0x40200000-0x402fffff pref]
[ 0.439426] pci 0000:00:00.0: BAR 7: assigned [io 0x1000-0x1fff]
[ 0.445599] pci 0000:01:00.0: BAR 1: assigned [mem 0x40100000-0x4017ffff]
[ 0.452488] pci 0000:01:00.0: BAR 6: assigned [mem 0x40200000-0x4023ffff pref]
[ 0.459745] pci 0000:01:00.0: BAR 0: assigned [mem 0x40180000-0x4019ffff]
[ 0.466635] pci 0000:01:00.0: BAR 3: assigned [mem 0x401a0000-0x401a3fff]
[ 0.473499] pci 0000:01:00.0: BAR 2: assigned [io 0x1000-0x101f]
[ 0.479656] pci 0000:00:00.0: PCI bridge to [bus 01]
Best regards,
Jingoo Han
> ---
> arch/arm/kernel/bios32.c | 37 +++----------------------------------
> 1 file changed, 3 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 317da88ae65b..91f48804e3bb 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -608,41 +608,10 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> */
> int pcibios_enable_device(struct pci_dev *dev, int mask)
> {
> - u16 cmd, old_cmd;
> - int idx;
> - struct resource *r;
> -
> - pci_read_config_word(dev, PCI_COMMAND, &cmd);
> - old_cmd = cmd;
> - for (idx = 0; idx < 6; idx++) {
> - /* Only set up the requested stuff */
> - if (!(mask & (1 << idx)))
> - continue;
> -
> - r = dev->resource + idx;
> - if (!r->start && r->end) {
> - printk(KERN_ERR "PCI: Device %s not available because"
> - " of resource collisions\n", pci_name(dev));
> - return -EINVAL;
> - }
> - if (r->flags & IORESOURCE_IO)
> - cmd |= PCI_COMMAND_IO;
> - if (r->flags & IORESOURCE_MEM)
> - cmd |= PCI_COMMAND_MEMORY;
> - }
> + if (pci_has_flag(PCI_PROBE_ONLY))
> + return 0;
>
> - /*
> - * Bridges (eg, cardbus bridges) need to be fully enabled
> - */
> - if ((dev->class >> 16) == PCI_BASE_CLASS_BRIDGE)
> - cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
> -
> - if (cmd != old_cmd) {
> - printk("PCI: enabling device %s (%04x -> %04x)\n",
> - pci_name(dev), old_cmd, cmd);
> - pci_write_config_word(dev, PCI_COMMAND, cmd);
> - }
> - return 0;
> + return pci_enable_resources(dev, mask);
> }
>
> int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
> --
> 1.8.2.2
^ permalink raw reply
* [PATCH 1/4] phy: miphy365x: Add Device Tree bindings for the MiPHY365x
From: Mark Rutland @ 2014-02-13 12:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213110314.GH32508@lee--X1>
On Thu, Feb 13, 2014 at 11:03:14AM +0000, Lee Jones wrote:
> > > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > > devices. It has 2 ports which it can use for either; both SATA, both
> > > PCIe or one of each in any configuration.
> > >
> > > Cc: devicetree at vger.kernel.org
> > > Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > .../devicetree/bindings/phy/phy-miphy365x.txt | 43 ++++++++++++++++++++++
> > > 1 file changed, 43 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > > new file mode 100644
> > > index 0000000..fdfa7ca
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
> > > @@ -0,0 +1,43 @@
> > > +STMicroelectronics STi MIPHY365x PHY binding
> > > +============================================
> > > +
> > > +This binding describes a miphy device that is used to control PHY hardware
> > > +for SATA and PCIe.
> > > +
> > > +Required properties:
> > > +- compatible: Should be "st,miphy365x-phy"
> > > +- #phy-cells: Should be 2 (See example)
> >
> > The first example has #phy-cells = <1>.
>
> Right, will fix. Should be 2.
>
> > What do the cells mean? What are the expected values?
>
> http://www.spinics.net/lists/arm-kernel/msg307209.html
Ok. Could that be mentioned in the binding document then?
Cheers,
Mark.
^ permalink raw reply
* [GIT PULL] integrator base patches for v3.15
From: Linus Walleij @ 2014-02-13 12:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi ARM SoC folks,
this is a set of patches I have sent for review and failed to get
ACKs from the proper subsystem maintainers after repeated
pings. However I now need to have this rotated in linux-next
as a base for multiplatform, so please pull it in, it is all ARM
drivers anyway. Russell pointed out some things and these
have been fixed and iterated in this series.
Yours,
Linus Walleij
The following changes since commit b28a960c42fcd9cfc987441fa6d1c1a471f0f9ed:
Linux 3.14-rc2 (2014-02-09 18:15:47 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git
tags/integrator-for-v3.15-1
for you to fetch changes up to 29525484cd2524a31ee0924831ab43b46ea6ebe1:
ARM: integrator: select GPIO block (2014-02-13 11:21:37 +0100)
----------------------------------------------------------------
Integrator/Versatile base patch stack for the v3.15 series:
- Move integrator clock definitions to the device tree, alter
clock and timer drivers accordingly.
- Alter the VIC irqchip driver to support cascaded VICs off
a parent IRQ controller.
- Update the IM-PD1 plugin code to use managed resources.
- Register the VIC on the IM-PD1.
- Select the PL061 GPIO block for the IM-PD1 on the
Integrator/AP.
----------------------------------------------------------------
Linus Walleij (10):
ARM: integrator: define clocks in the device trees
ARM: SP804: make Integrator/CP timer pick clock from DT
ARM: integrator: switch to fetch clocks from device tree
clk: versatile: pass a parent to the ICST clock
clk: versatile: respect parent rate in ICST clock
irqchip: vic: update the base IRQ member correctly
irqchip: support cascaded VICs
ARM: integrator: use managed resources for the IM-PD1
ARM: integrator: register the IM-PD1 VIC
ARM: integrator: select GPIO block
.../devicetree/bindings/clock/arm-integrator.txt | 34 +++++++
arch/arm/boot/dts/integratorap.dts | 35 +++++++
arch/arm/boot/dts/integratorcp.dts | 102 ++++++++++++++++++++-
arch/arm/common/timer-sp.c | 8 +-
arch/arm/mach-integrator/Kconfig | 3 +
arch/arm/mach-integrator/impd1.c | 81 +++++++++-------
arch/arm/mach-integrator/integrator_ap.c | 19 +++-
arch/arm/mach-integrator/integrator_cp.c | 6 --
arch/arm/mach-versatile/core.c | 2 +-
drivers/clk/versatile/clk-icst.c | 21 ++++-
drivers/clk/versatile/clk-icst.h | 1 +
drivers/clk/versatile/clk-impd1.c | 6 +-
drivers/clk/versatile/clk-integrator.c | 83 +++++++----------
drivers/clk/versatile/clk-realview.c | 4 +-
drivers/irqchip/irq-vic.c | 59 ++++++++++--
include/linux/irqchip/arm-vic.h | 6 +-
include/linux/platform_data/clk-integrator.h | 1 -
17 files changed, 353 insertions(+), 118 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/arm-integrator.txt
^ permalink raw reply
* [PATCH v1 0/2] exynos_mct driver: fix irq allocation and cleanup
From: Tomasz Figa @ 2014-02-13 12:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392260923-31659-1-git-send-email-t.dakhran@samsung.com>
Hi Tarek,
On 13.02.2014 04:08, Tarek Dakhran wrote:
> exynos4_local_timer_setup called on the secondary cpu before
> irqs are enabled. request_irq can sleep, which produces next warning:
>
> on boot:
> [ 0.370000] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> [ 0.370000] Setting up static identity map for 0x403b5700 - 0x403b5758
> [ 0.395000] CPU1: Booted secondary processor
> [ 0.395000] ------------[ cut here ]------------
> [ 0.395000] WARNING: CPU: 1 PID: 0 at kernel/locking/lockdep.c:2742 lockdep_trace_alloc+0xe0/0xfc()
> [ 0.395000] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
> [ 0.395000] Modules linked in:
> [ 0.395000] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.14.0-rc2-00004-g0db95f4 #128
> [ 0.395000] [<c0014308>] (unwind_backtrace) from [<c0011690>] (show_stack+0x10/0x14)
> [ 0.395000] [<c0011690>] (show_stack) from [<c03ae7d0>] (dump_stack+0x6c/0xb8)
> [ 0.395000] [<c03ae7d0>] (dump_stack) from [<c001d504>] (warn_slowpath_common+0x68/0x8c)
> [ 0.395000] [<c001d504>] (warn_slowpath_common) from [<c001d5bc>] (warn_slowpath_fmt+0x30/0x40)
> [ 0.395000] [<c001d5bc>] (warn_slowpath_fmt) from [<c0059824>] (lockdep_trace_alloc+0xe0/0xfc)
> [ 0.395000] [<c0059824>] (lockdep_trace_alloc) from [<c00bee24>] (kmem_cache_alloc+0x24/0x160)
> [ 0.395000] [<c00bee24>] (kmem_cache_alloc) from [<c0068174>] (request_threaded_irq+0x64/0x130)
> [ 0.395000] [<c0068174>] (request_threaded_irq) from [<c02efaf8>] (exynos4_local_timer_setup+0xd0/0x124)
> [ 0.395000] [<c02efaf8>] (exynos4_local_timer_setup) from [<c02efc34>] (exynos4_mct_cpu_notify+0x78/0xf0)
> [ 0.395000] [<c02efc34>] (exynos4_mct_cpu_notify) from [<c003d318>] (notifier_call_chain+0x44/0x84)
> [ 0.395000] [<c003d318>] (notifier_call_chain) from [<c001d61c>] (__cpu_notify+0x24/0x40)
> [ 0.395000] [<c001d61c>] (__cpu_notify) from [<c0013314>] (secondary_start_kernel+0xe4/0x134)
> [ 0.395000] [<c0013314>] (secondary_start_kernel) from [<40008624>] (0x40008624)
> [ 0.395000] ---[ end trace 347890460e745f50 ]---
> [ 0.420000] CPU1: update cpu_power 1024
> [ 0.420000] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
>
> on hotplug:
> [ 108.040000] CPU3: Booted secondary processor
> [ 108.040000] BUG: sleeping function called from invalid context at mm/slub.c:965
> [ 108.040000] in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/3
> [ 108.040000] INFO: lockdep is turned off.
> [ 108.040000] irq event stamp: 0
> [ 108.040000] hardirqs last enabled at (0): [< (null)>] (null)
> [ 108.040000] hardirqs last disabled at (0): [<c001b768>] copy_process.part.2+0x2a4/0x12f4
> [ 108.040000] softirqs last enabled at (0): [<c001b768>] copy_process.part.2+0x2a4/0x12f4
> [ 108.040000] softirqs last disabled at (0): [< (null)>] (null)
> [ 108.040000] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G W 3.14.0-rc2-00004-g0db95f4 #128
> [ 108.040000] [<c0014308>] (unwind_backtrace) from [<c0011690>] (show_stack+0x10/0x14)
> [ 108.040000] [<c0011690>] (show_stack) from [<c03ae7d0>] (dump_stack+0x6c/0xb8)
> [ 108.040000] [<c03ae7d0>] (dump_stack) from [<c00beed4>] (kmem_cache_alloc+0xd4/0x160)
> [ 108.040000] [<c00beed4>] (kmem_cache_alloc) from [<c0068174>] (request_threaded_irq+0x64/0x130)
> [ 108.040000] [<c0068174>] (request_threaded_irq) from [<c02efaf8>] (exynos4_local_timer_setup+0xd0/0x124)
> [ 108.040000] [<c02efaf8>] (exynos4_local_timer_setup) from [<c02efc34>] (exynos4_mct_cpu_notify+0x78/0xf0)
> [ 108.040000] [<c02efc34>] (exynos4_mct_cpu_notify) from [<c003d318>] (notifier_call_chain+0x44/0x84)
> [ 108.040000] [<c003d318>] (notifier_call_chain) from [<c001d61c>] (__cpu_notify+0x24/0x40)
> [ 108.040000] [<c001d61c>] (__cpu_notify) from [<c0013314>] (secondary_start_kernel+0xe4/0x134)
> [ 108.040000] [<c0013314>] (secondary_start_kernel) from [<40008624>] (0x40008624)
>
> First patch fixes this problem by removing request_irq from exynos4_local_timer_setup
> Second removes non-dt stuff.
>
> Tested on linux kernel v3.14-rc2.
It would be nice to say on which boards it has been tested. Let me check
this on our boards anyway.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH RFC v3 3/3] Documentation: arm: define DT idle states bindings
From: Lorenzo Pieralisi @ 2014-02-13 12:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213013153.18853.21632@capellas-linux>
Hi Sebastian,
thanks for having a look.
On Thu, Feb 13, 2014 at 01:31:53AM +0000, Sebastian Capella wrote:
> Quoting Lorenzo Pieralisi (2014-02-11 06:17:53)
> > + - cpu-idle-states
> > + Usage: Optional
> > + Value type: <prop-encoded-array>
> > + Definition:
> > + # List of phandles to cpu idle state nodes supported
> > + by this cpu [1].
> > +
>
> Should cpu idle be hyphenated in the definition like:
> "List of phandles to cpu-idle state nodes supported"
No, because it is not meant to be tied to the kernel CPU idle framework.
It is meant to be "List of phandles to idle state nodes supported by this cpu"
and that's what I will do.
On a side note, that's why I was reluctant to call them idle states, for
the records.
> Is anything implied in the ordering of this list?
> Or is this a non-ordered array of phandles?
Non-ordered.
> Would it be a good idea to select a different name for this property vs.
> the node? It seems to get confusing sometimes.
Such as ? "idle-states" ?
> > +According to the Server Base System Architecture document (SBSA, [3]), the
> > +power states an ARM CPU can be put into are identified by the following list:
> > +
> > +1 - Running
> > +2 - Idle_standby
> > +3 - Idle_retention
> > +4 - Sleep
> > +5 - Off
>
> Are these states used in the state->index value?
> Might it be helpful to number these starting with 0?
No, I will remove the numbers.
> > +ARM platforms implement the power states specified in the SBSA through power
> > +management schemes that allow an OS PM implementation to put the processor in
> > +different idle states (which include states 1 to 4 above; "off" state is not
> > +an idle state since it does not have wake-up capabilities, hence it is not
> > +considered in this document).
>
> Might an implementation have both sleep and off states where they have
> different latencies in the case a cpu can wake itself vs. a coprocessor
> waking the cpu?
Yes, but not in SBSA nomenclature. off as in SBSA is not an idle state,
since it does not require IRQ wake up capabilities.
An idle state requires IRQ wake-up capabilities (either through return
from wfi or reset from coprocessor), how it is implemented it does not
matter.
> > +
> > +Idle state parameters (eg entry latency) are platform specific and need to be
> > +characterized with bindings that provide the required information to OSPM
> > +code so that it can build the required tables and use them at runtime.
> > +
> > +The device tree binding definition for ARM idle states is the subject of this
> > +document.
>
> During last connect, we'd discussed that the small set of
> states here could apply to a single node, which can represent a cpu, a
> cluster with cache etc. Then the complexities of the system power state
> would be represented using a heirarchy, with each node in the
> tree having its own state from the list above. This would allow
> a fairly rich system state while having just a few power states defined
> at each level. Is this how you're intending these bindings to go?
Yes, CPUs point at states that can be reached by that CPU (eg a CPU core
gating state is represented by a single node pointed at by all CPUs in the
system - it the same state data, different power domains though).
States are hierarchical, which means that a parent state implies entry on all
substates that's how cluster states are defined.
> > +===========================================
> > +2 - state node
> > +===========================================
>
> should the section numbering be incremented here? Or is this a
> subsection? 2.1?
Yes.
> Also, would it be nice to have a name field for each state?
There is:
"The state node name shall be "stateN", where N = {0, 1, ...} is
the node number; state nodes which are siblings within a single
common parent node must be given a unique and sequential N value,
starting from 0."
I can remove the rule and allow people to call states as they wish
since they already have a compatible property to match them.
Actually, states can be called with any name, provided it is unique.
> > + A state node can contain state child nodes. A state node with
> > + children represents a hierarchical state, which is a superset of
> > + the child states. Hierarchical states require all CPUs on which
> > + they are valid to request the state in order for it to be entered.
>
> Is it possible for a cpu to request a deeper state and unblock other cpus
> from entering this state?
That's not DT bindings business, hierarchical states define states that
require all CPUs on which they are valid to enter that state for it to
be considered "enabled".
It is a hard nut to crack. In x86 this stuff does not exist and it is
managed in HW, basically an idle state is always per-cpu (but it might
end up becoming a package state when all CPUs in a package enter that
state). On ARM, we want to define hierarchical states explicitly to
link resources (ie caches) to them.
CPUs are not prevented from requesting a hierarchical state, but the
state only becomes "enabled" when all CPUs on which it is valid request
it.
I cannot think of any other way of to express this properly but still in
a compact way.
> "all CPUs on which they are valid" is this determined by seeing which
> state's phandle is in each cpu->cpu-idle-states?
Yes, does it make sense ?
> > +
> > + A state node defines the following properties:
> ...
> > + - index
> > + Usage: Required
> > + Value type: <u32>
> > + Definition: It represents an idle state index, starting from 2.
> > + Index 0 represents the processor state "running"
> > + and index 1 represents processor mode
> > + "idle_standby", entered by executing a wfi
> > + instruction (SBSA,[3]); indexes 0 and 1 are
> > + standard ARM states that need not be described.
>
> Do you think maybe something like this might be clearer?
Yes it is.
> Definition: It represents an idle state index.
>
> Index 0 and 1 shall not be specified and are reserved for ARM
> states where index 0 is running, and index 1 is idle_standby
> entered by executing a wfi instruction (SBSA,[3])
>
> What mechanism is used to order the power states WRT power consumption?
I think we should use index for that. The higher the index the lower the
power consumption.
> > + - entry-method
> > + Usage: Required
> > + Value type: <stringlist>
> > + Definition: Describes the method by which a CPU enters the
> > + idle state. This property is required and must be
> > + one of:
> > +
> > + - "arm,psci-cpu-suspend"
> > + ARM PSCI firmware interface, CPU suspend
> > + method[2].
>
> Can psci-cpu-suspend be assumed if entry-method is omitted?
No.
> Can this field be used to combine both psci and non-psci states in any order?
No. I will enforce a unique entry method.
> > + - power-state
> > + Usage: See definition.
> > + Value type: <u32>
> > + Definition: Depends on the entry-method property value.
> > + If entry-method is "arm,psci-cpu-suspend":
> > + # Property is required and represents
> > + psci-power-state parameter. Please refer to
> > + [2] for PSCI bindings definition.
>
> Examples use psci-power-state..
Typo, sorry, it is not C unfortunately...
> If we call this something like entry-method-param rather than power-state,
> would this allow the field to be more flexible? Is flexibility here a goal?
Yes, I can call it like that.
> - power-state
> Usage: See definition.
> Value type: <u32>
> Definition: Parameter to pass to the entry method when
> this state is being entered.
> If entry-method is "arm,psci-cpu-suspend",
> this parameter represents the psci-power-state
> parameter. Please refer to [2] for PSCI bindings
> definition.
>
> > + - power-domains
> > + Usage: Optional
> > + Value type: <prop-encoded-array>
> > + Definition: List of power domain specifiers ([1]) describing
> > + the power domains that are affected by the idle
> > + state entry.
>
> How do you expect this information should be used?
This defines all power domains that are affected by the state entry.
It allows us to understand what caches, devices, whatnots have to be
acted upon state entry.
> I assume psci will be turning off the powerdomains not Linux right?
This is not a PSCI only document, and even if it was, we still need to deal
with devices. Which means we need to know what we have to save/restore (PMU,
arch timer, GIC), and power domains help us detect that.
> If so, is the structure above helpful for psci to associate the cpu
> requesting a state to the specific power domain in the power-domains list?
> Or is this all encoded in the parameter to PSCI suspend? In that case,
> what is the utility?
See above.
> > + - cache-state-retained
> > + Usage: See definition
> > + Value type: <none>
> > + Definition: if present cache memory is retained on power down,
> > + otherwise it is lost.
> > +
> > + - processor-state-retained
> > + Usage: See definition
> > + Value type: <none>
> > + Definition: if present CPU processor logic is retained on
> > + power down, otherwise it is lost.
>
> I don't see a good example of these two retained state properties.
> Isn't this the purpose of idle_retained state? In the explanations, is
> the term 'power down' the same as sleep or off?
Ok, I will remove "power down" and replace it with "state entry".
> > +
> > +cpus {
> > + #size-cells = <0>;
> > + #address-cells = <2>;
> > +
> > + cpu-idle-states {
> > +
> > + STATE0: state0 {
> > + compatible = "arm,cpu-idle-state";
> > + index = <3>;
>
> Are the index fields of nested states independent of each other or
> sequential?
>
> ie:
> - does index=3 here mean pd_cluster is sleep state, and index=2
> below mean the cpu cluster is idle_retention? (both SBSA states)
> - Or does index=3 here mean this state is the next cpu-idle state after
> STATE0_1 below, which has index=2? In this case, are the indexes
> implied to be increasing in order of most power savings?
Forget index as a link to SBSA states indexes above I should have never listed
them as numbers. I understand index is misleading and either I remove it, or I
leave it there to define power savings scale as you mentioned.
> > + entry-method = "arm,psci-cpu-suspend";
> > + psci-power-state = <0x1010000>;
> > + entry-latency = <500>;
> > + exit-latency = <1000>;
> > + min-residency = <2500>;
> > + power-domains = <&pd_clusters 0>;
> > + STATE0_1: state0 {
>
> Should this be STATE0_0?
Well, yes, it is a tag though so it can be whatever we want as long as
it is unique.
> > + compatible = "arm,cpu-idle-state";
> > + index = <2>;
> > + entry-method = "arm,psci-cpu-suspend";
> > + psci-power-state = <0x0010000>;
> > + entry-latency = <200>;
> > + exit-latency = <400>;
> > + min-residency = <300>;
> > + power-domains = <&pd_cores 0>,
> > + <&pd_cores 1>,
> ...
> > + <&pd_cores 7>;
> > + };
> > + };
>
> Would it be possible to add an example illustrating more
> complex cluster/cpu power states? Maybe where both the cpus and
> cluster have multiple states (sleep/retention)?
>
> The current example seems to show just the cpu idle_retention state,
> and the cluster off state.
>
> Maybe an example of how you'd represent something like this with the new
> bindings:
>
> (in increasing order of power saving)
> +-----------------+--------------------+-----------------------------------+
> | cpu | cluster | Notes
> +-----------------+--------------------+-----------------------------------+
> | running | running | not specified running
> | idle_standby | running | not specified WFI
> | idle_retention | running |
> | idle_retention | idle_retention |
> | sleep | idle_retention |
> | sleep | sleep |
> +-----------------|--------------------+-----------------------------------+
>
> The CPU has 4 states: running, idle standby, idle_retention and sleep.
> the first two are not specified per the instructions.
>
> Then the cluster has 3 states: running, retention and sleep.
>
> Maybe a complex case like this would be helpful to understand how the
> bindings should be used.
Ok, I will do that and rework the bindings accordingly.
Thanks,
Lorenzo
^ permalink raw reply
* [PATCH] bus: imx-weim: support weim-cs-gpr for imx6q-weim
From: Shawn Guo @ 2014-02-13 12:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140212164304.GA20251@frolo.macqel>
On Wed, Feb 12, 2014 at 05:43:04PM +0100, Philippe De Muyter wrote:
> Thanks Shawn.
>
> On Tue, Feb 11, 2014 at 10:50:11AM +0800, Shawn Guo wrote:
> > For imx6q-weim type of device, there might a WEIM CS space configuration
> > register in General Purpose Register controller, e.g. IOMUXC_GPR1 on
> > i.MX6Q.
> >
> > Depending on which configuration of the following 4 is chosen for given
> > system, IOMUXC_GPR1[11:0] should be set up as 0x5, 0x1b, 0x4b or 0x249
>
> As the bits are actually grouped by 3, one could write:
> ... as 05, 033, 0113, or 01111
Indeed, it's easier to read. Will change.
> > correspondingly.
> >
> > CS0(128M) CS1(0M) CS2(0M) CS3(0M)
> > CS0(64M) CS1(64M) CS2(0M) CS3(0M)
> > CS0(64M) CS1(32M) CS2(32M) CS3(0M)
> > CS0(32M) CS1(32M) CS2(32M) CS3(32M)
> >
> > The patch creates a table in the driver for above configurations, and
> > detects which one is being used for the booting system by looking at
> > 'ranges' property of WEIM node. Thus the WEIM CS GPR can be set up
> > automatically at boot time.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > Documentation/devicetree/bindings/bus/imx-weim.txt | 6 ++
> > drivers/bus/imx-weim.c | 83 ++++++++++++++++++++
> > 2 files changed, 89 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
> > index 0fd76c4..d114460f 100644
> > --- a/Documentation/devicetree/bindings/bus/imx-weim.txt
> > +++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
> > @@ -19,6 +19,12 @@ Required properties:
> >
> > <cs-number> 0 <physical address of mapping> <size>
> >
> > +Optional properties:
> > +
> > + - fsl,weim-cs-gpr: Should be the phandle to the General Purpose Register
> > + controller that contains WEIM CS GPR register, e.g.
> > + IOMUXC_GPR1 on i.MX6Q.
> > +
>
> Why require that new property ? It makes things harder to use.
I was originally thinking that it may help if some day we get an
imx6q-weim type of device integrated on an new IMX SoC where the WEIM
CS GPR is defined in other block other than IOMUXC. But, yes, it's
unnecessary today, and we can add it when necessary. I will drop it
in v2.
>
> And you could add the body of your commit log here.
>
> > Timing property for child nodes. It is mandatory, not optional.
> >
> > - fsl,weim-cs-timing: The timing array, contains timing values for the
> > diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> > index 3ef58c8..9c8a522 100644
> > --- a/drivers/bus/imx-weim.c
> > +++ b/drivers/bus/imx-weim.c
> > @@ -11,6 +11,9 @@
> > #include <linux/clk.h>
> > #include <linux/io.h>
> > #include <linux/of_device.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> > +#include <linux/regmap.h>
> >
> > struct imx_weim_devtype {
> > unsigned int cs_count;
> > @@ -56,6 +59,83 @@ static const struct of_device_id weim_id_table[] = {
> > };
> > MODULE_DEVICE_TABLE(of, weim_id_table);
> >
> > +struct imx6q_weim_gpr {
> > + u32 cssize[4];
> > + u32 gprval;
> > +};
> > +
> > +static const struct imx6q_weim_gpr imx6q_weim_gpr_data[] __initconst = {
> > + {
> > + /* CS0(128M) CS1(0M) CS2(0M) CS3(0M) */
> > + .cssize = { 128, 0, 0, 0 },
> > + .gprval = 0x5,
> 05
> > + }, {
> > + /* CS0(64M) CS1(64M) CS2(0M) CS3(0M) */
> > + .cssize = { 64, 64, 0, 0 },
> > + .gprval = 0x1b,
> 033
> > + }, {
> > + /* CS0(64M) CS1(32M) CS2(32M) CS3(0M) */
> > + .cssize = { 64, 32, 32, 0 },
> > + .gprval = 0x4b,
> 0113
> > + }, {
> > + /* CS0(64M) CS1(32M) CS2(32M) CS3(0M) */
> /* CS0(32M) CS1(32M) CS2(32M) CS3(32M) */
> > + .cssize = { 32, 32, 32, 32 },
> > + .gprval = 0x249,
> 01111
> > + },
> > +};
> > +
> > +static int __init imx6q_weim_gpr_setup(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + const struct property *prop;
> > + struct regmap *gpr;
> > + u32 cssize[4] = { 0, 0, 0, 0 };
> > + int len;
> > + int ret;
> > + int i;
> > +
> > + gpr = syscon_regmap_lookup_by_phandle(np, "fsl,weim-cs-gpr");
>
> gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>
> > + if (IS_ERR(gpr)) {
> > + dev_dbg(&pdev->dev, "No weim-cs-gpr to set up\n");
> > + return 0;
> > + }
> > +
> > + prop = of_find_property(np, "ranges", &len);
> > + if (prop == NULL)
> > + return -ENOENT;
> > + if (len % (sizeof(u32) * 4))
> > + return -EINVAL;
> > +
> > + for (i = 0; i < len / (sizeof(u32) * 4); i++) {
> > + int cs;
> > + /* read cs index */
> > + ret = of_property_read_u32_index(np, "ranges", i * 4, &cs);
> > + if (ret)
> > + return ret;
> > + /* read cs size */
> > + ret = of_property_read_u32_index(np, "ranges", i * 4 + 3,
> > + &cssize[cs]);
> > + if (ret)
> > + return ret;
> > + /* turn to MB */
> > + cssize[cs] >>= 20;
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(imx6q_weim_gpr_data); i++) {
> > + ret = memcmp(cssize, imx6q_weim_gpr_data[i].cssize,
> > + sizeof(cssize));
> > + if (ret == 0) {
> > + /* Find it. Set up IOMUXC_GPR1[11:0] with the gprval. */
>
> Found it
Right.
>
> > + regmap_update_bits(gpr, IOMUXC_GPR1, 0xfff,
> > + imx6q_weim_gpr_data[i].gprval);
> > + return 0;
> > + }
> > + }
> > +
> > + dev_err(&pdev->dev, "Invalid 'ranges' configuration\n");
> > + return -EINVAL;
> > +}
> > +
> > /* Parse and set the timing for this device. */
> > static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
> > const struct imx_weim_devtype *devtype)
> > @@ -92,6 +172,9 @@ static int __init weim_parse_dt(struct platform_device *pdev,
> > struct device_node *child;
> > int ret;
> >
> > + if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx6q-weim"))
> > + imx6q_weim_gpr_setup(pdev);
> > +
> > for_each_child_of_node(pdev->dev.of_node, child) {
> > if (!child->name)
> > continue;
> > --
> > 1.7.9.5
> >
>
> Now the most important : it works. I have tested it successfully for the
> 4 combinations.
>
> Code could probably be made shorter by using 'of_prop_next_u32' and building
> the gprval incrementally, then checking it against the four possible values,
> though.
Point taken. See it in v2.
Thanks for the review comments.
Shawn
^ permalink raw reply
* [PATCH 1/3] mmc: add support for power-on sequencing through DT
From: Arnd Bergmann @ 2014-02-13 12:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213104248.GU26684@n2100.arm.linux.org.uk>
On Thursday 13 February 2014 10:42:48 Russell King - ARM Linux wrote:
>
> What if we have a platform where things subtly change, like for instance,
> the wiring on the SD slot to fix a problem with UHS-1 cards, which means
> you don't have UHS-1 support for some platforms but do for others.
>
> What if you have a platform which uses a brcm4329 chip for Wifi, but then
> later in the production run switch to using a different Wifi chipset?
As far as I can tell, the power sequencing is normally really
dependent on the device. If someone has an on-board brcm4329
that requires a specific set of clocks, resets, voltages etc
to be routed to the chip and enabled in the correct order to
allow probing, it seems unlikely that changing the wifi chipset
to something else would keep the exact same requirements.
> With this information encoded into DT, the number of DT files quickly
> increases, and then this presents its own problem - how do users get to
> know which DT file should be used for their platform when all they see
> externally is "a product of type A"?
I haven't seen a suggestion that would not encode this in DT. The
difference between Olof's original suggestion and mine is that he
proposed to put the data into the existing node of the host controller,
while my approach would be to add a node for each function with
these requirements and modify the sdio code so that we can deal
with it in the function driver. The data that would get passed
however is almost the same.
Arnd
^ permalink raw reply
* [PATCH v3 01/13] ARM: mvebu: rename armada-370-xp.c to armada-mvebu.c
From: Arnd Bergmann @ 2014-02-13 12:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213125526.3af08c54@skate>
On Thursday 13 February 2014, Thomas Petazzoni wrote:
> I believe it's a matter of taste here. Having a file named
> armada-370-xp.c that handles Armada 375 and Armada 38x looks highly
> confusing to me, and I believe both Gr?gory and Ezequiel were of the
> same opinion.
I certainly agree that it's confusing, but we also do this all the time
in the kernel.
> The number of changes to this file is very limited, so the probability
> of having a large number of complicated patches touching the same file
> being in flight is fairly low.
>
> Maybe we can leave this taste decision to the mach-mvebu maintainers?
Definitely, sorry for being unclear here. I wasn't saying you need to
change it, just trying to state my personal preference.
Arnd
^ permalink raw reply
* [PATCH 3/7] Documentation: devicetree: fix up pl022 clocks
From: Linus Walleij @ 2014-02-13 12:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392118632-11312-4-git-send-email-mark.rutland@arm.com>
On Tue, Feb 11, 2014 at 12:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Currently the pl022 driver expects clocks, and dts provide them, yet the
> binding does not mention clocks at all.
>
> This patch adds a description of the clocks, "apb_pclk" (as required by
> the primecell binding) and "sspclk" for the pl022 itself. The "sspclk"
> name was chosen to match the official documentation, as currently a
> variety of names are used in its place; it is expected that any
> operating system supporting these can continue to do so in the absence
> of an "sspclk" entry.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2] bus: imx-weim: support CS GPR config for imx6q-weim
From: Shawn Guo @ 2014-02-13 12:59 UTC (permalink / raw)
To: linux-arm-kernel
For imx6q-weim type of device, there might a WEIM CS space configuration
register in General Purpose Register controller, e.g. IOMUXC_GPR1 on
i.MX6Q.
Depending on which configuration of the following 4 is chosen for given
system, IOMUXC_GPR1[11:0] should be set up as 05, 033, 0113 or 01111
correspondingly.
CS0(128M) CS1(0M) CS2(0M) CS3(0M)
CS0(64M) CS1(64M) CS2(0M) CS3(0M)
CS0(64M) CS1(32M) CS2(32M) CS3(0M)
CS0(32M) CS1(32M) CS2(32M) CS3(32M)
The patch creates a function for imx6q-weim type of device, which scans
'ranges' property of WEIM node and build the GPR value incrementally.
Thus the WEIM CS GPR can be set up automatically at boot time.
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v1:
- Drop device tree property fsl,weim-cs-gpr
- Use octal value for IOMUXC_GPR1[11:0] bit field
- Use of_property_for_each_u32() to build gprval incrementally
drivers/bus/imx-weim.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 3ef58c8..a120c93 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -11,6 +11,9 @@
#include <linux/clk.h>
#include <linux/io.h>
#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
+#include <linux/regmap.h>
struct imx_weim_devtype {
unsigned int cs_count;
@@ -56,6 +59,59 @@ static const struct of_device_id weim_id_table[] = {
};
MODULE_DEVICE_TABLE(of, weim_id_table);
+static int __init imx6q_weim_gpr_setup(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct property *prop;
+ const __be32 *p;
+ struct regmap *gpr;
+ u32 gprvals[4] = {
+ 05, /* CS0(128M) CS1(0M) CS2(0M) CS3(0M) */
+ 033, /* CS0(64M) CS1(64M) CS2(0M) CS3(0M) */
+ 0113, /* CS0(64M) CS1(32M) CS2(32M) CS3(0M) */
+ 0111, /* CS0(64M) CS1(32M) CS2(32M) CS3(0M) */
+ };
+ u32 gprval = 0;
+ u32 val;
+ int len;
+ int cs = 0;
+ int i = 0;
+
+ gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (IS_ERR(gpr)) {
+ dev_err(&pdev->dev, "failed to find fsl,imx6q-iomuxc-gpr\n");
+ return -ENOENT;
+ }
+
+ prop = of_find_property(np, "ranges", &len);
+ if (prop == NULL)
+ return -ENOENT;
+ if (len % (sizeof(u32) * 4))
+ return -EINVAL;
+
+ of_property_for_each_u32(np, "ranges", prop, p, val) {
+ if (i % 4 == 0) {
+ cs = val;
+ } else if (i % 4 == 3 && val) {
+ val /= SZ_64M;
+ val = (val << 1) | 1;
+ gprval |= val << cs * 3;
+ }
+ i++;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(gprvals); i++) {
+ if (gprval == gprvals[i]) {
+ /* Found it. Set up IOMUXC_GPR1[11:0] with it. */
+ regmap_update_bits(gpr, IOMUXC_GPR1, 0xfff, gprval);
+ return 0;
+ }
+ }
+
+ dev_err(&pdev->dev, "Invalid 'ranges' configuration\n");
+ return -EINVAL;
+}
+
/* Parse and set the timing for this device. */
static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
const struct imx_weim_devtype *devtype)
@@ -92,6 +148,9 @@ static int __init weim_parse_dt(struct platform_device *pdev,
struct device_node *child;
int ret;
+ if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx6q-weim"))
+ imx6q_weim_gpr_setup(pdev);
+
for_each_child_of_node(pdev->dev.of_node, child) {
if (!child->name)
continue;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 02/18] arm64: GICv3 device tree binding documentation
From: Marc Zyngier @ 2014-02-13 12:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <loom.20140207T062553-254@post.gmane.org>
Hi Arnab,
On 07/02/14 05:41, Arnab Basu wrote:
> Hi Marc
>
> Marc Zyngier <marc.zyngier <at> arm.com> writes:
>
>> +
>> +AArch64 SMP cores are often associated with a GICv3, providing private
>> +peripheral interrupts (PPI), shared peripheral interrupts (SPI),
>> +software generated interrupts (SGI), and locality-specific peripheral
>> +Interrupts (LPI).
>> +
>> +Main node required properties:
>> +
>> +- compatible : should at least contain "arm,gic-v3".
>> +- interrupt-controller : Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> + interrupt source. Must be a single cell with a value of at least 3.
>> +
>> + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI
>> + interrupts. Other values are reserved for future use.
>
> These values are defined in
> "include/dt-bindings/interrupt-controller/arm-gic.h" maybe we should start
> mentioning that here and encourage future device treese to use those defines
> to improve readability.
It may improve readability, but it makes the definition rely on
something else. Definition and usage are two different things, and I
want the definition to be completely self-contained and non ambiguous.
In DTS files, people can use whatever macro they decide, and it is their
problem. They will even have out of tree DTS files, for which the
include file is not available.
>
>> +
>> + The 2nd cell contains the interrupt number for the interrupt type.
>> + SPI interrupts are in the range [0-987]. PPI interrupts are in the
>> + range [0-15].
>> +
>> + The 3rd cell is the flags, encoded as follows:
>> + bits[3:0] trigger type and level flags.
>> + 1 = edge triggered
>> + 2 = edge triggered (deprecated, for compatibility with GICv2)
>> + 4 = level triggered
>> + 8 = level triggered (deprecated, for compatibility with GICv2)
>
> Similar to the above comment
> "include/dt-bindings/interrupt-controller/irq.h" defines the trigger type
> and level flags. Although this file currently contains the GICv2 bindings,
> we could update them.
Same as above.
Thanks for the review!
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] thermal: imx: update formula for thermal sensor
From: Shawn Guo @ 2014-02-13 13:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1392199595-1054-1-git-send-email-b20788@freescale.com>
Copy LAKML and pengutronix folks in case they have comments.
On Wed, Feb 12, 2014 at 06:06:35PM +0800, Anson Huang wrote:
> Thermal sensor used to need two calibration points which are
> in fuse map to get a slope for converting thermal sensor's raw
> data to real temperature in degree C. Due to the chip calibration
> limitation, hardware team provides an universal formula to get
> real temperature from internal thermal sensor raw data:
>
> Slope = 0.4297157 - (0.0015976 * 25C fuse);
>
> Update the formula, as there will be no hot point calibration
> data in fuse map from now on.
>
> Signed-off-by: Anson Huang <b20788@freescale.com>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
Shawn
> ---
> drivers/thermal/imx_thermal.c | 39 ++++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 45af765..a99c631 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -62,12 +62,16 @@ enum imx_thermal_trip {
> #define IMX_POLLING_DELAY 2000 /* millisecond */
> #define IMX_PASSIVE_DELAY 1000
>
> +#define FACTOR0 10000000
> +#define FACTOR1 15976
> +#define FACTOR2 4297157
> +
> struct imx_thermal_data {
> struct thermal_zone_device *tz;
> struct thermal_cooling_device *cdev;
> enum thermal_device_mode mode;
> struct regmap *tempmon;
> - int c1, c2; /* See formula in imx_get_sensor_data() */
> + u32 c1, c2; /* See formula in imx_get_sensor_data() */
> unsigned long temp_passive;
> unsigned long temp_critical;
> unsigned long alarm_temp;
> @@ -84,7 +88,7 @@ static void imx_set_alarm_temp(struct imx_thermal_data *data,
> int alarm_value;
>
> data->alarm_temp = alarm_temp;
> - alarm_value = (alarm_temp - data->c2) / data->c1;
> + alarm_value = (data->c2 - alarm_temp) / data->c1;
> regmap_write(map, TEMPSENSE0 + REG_CLR, TEMPSENSE0_ALARM_VALUE_MASK);
> regmap_write(map, TEMPSENSE0 + REG_SET, alarm_value <<
> TEMPSENSE0_ALARM_VALUE_SHIFT);
> @@ -136,7 +140,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, unsigned long *temp)
> n_meas = (val & TEMPSENSE0_TEMP_CNT_MASK) >> TEMPSENSE0_TEMP_CNT_SHIFT;
>
> /* See imx_get_sensor_data() for formula derivation */
> - *temp = data->c2 + data->c1 * n_meas;
> + *temp = data->c2 - n_meas * data->c1;
>
> /* Update alarm value to next higher trip point */
> if (data->alarm_temp == data->temp_passive && *temp >= data->temp_passive)
> @@ -305,6 +309,7 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> int t1, t2, n1, n2;
> int ret;
> u32 val;
> + u64 temp64;
>
> map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> "fsl,tempmon-data");
> @@ -330,6 +335,8 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> * [31:20] - sensor value @ 25C
> * [19:8] - sensor value of hot
> * [7:0] - hot temperature value
> + * Use universal formula now and only need sensor value @ 25C
> + * slope = 0.4297157 - (0.0015976 * 25C fuse)
> */
> n1 = val >> 20;
> n2 = (val & 0xfff00) >> 8;
> @@ -337,20 +344,26 @@ static int imx_get_sensor_data(struct platform_device *pdev)
> t1 = 25; /* t1 always 25C */
>
> /*
> - * Derived from linear interpolation,
> - * Tmeas = T2 + (Nmeas - N2) * (T1 - T2) / (N1 - N2)
> + * Derived from linear interpolation:
> + * slope = 0.4297157 - (0.0015976 * 25C fuse)
> + * slope = (FACTOR2 - FACTOR1 * n1) / FACTOR0
> + * (Nmeas - n1) / (Tmeas - t1) = slope
> * We want to reduce this down to the minimum computation necessary
> * for each temperature read. Also, we want Tmeas in millicelsius
> * and we don't want to lose precision from integer division. So...
> - * milli_Tmeas = 1000 * T2 + 1000 * (Nmeas - N2) * (T1 - T2) / (N1 - N2)
> - * Let constant c1 = 1000 * (T1 - T2) / (N1 - N2)
> - * milli_Tmeas = (1000 * T2) + c1 * (Nmeas - N2)
> - * milli_Tmeas = (1000 * T2) + (c1 * Nmeas) - (c1 * N2)
> - * Let constant c2 = (1000 * T2) - (c1 * N2)
> - * milli_Tmeas = c2 + (c1 * Nmeas)
> + * Tmeas = (Nmeas - n1) / slope + t1
> + * milli_Tmeas = 1000 * (Nmeas - n1) / slope + 1000 * t1
> + * milli_Tmeas = -1000 * (n1 - Nmeas) / slope + 1000 * t1
> + * Let constant c1 = (-1000 / slope)
> + * milli_Tmeas = (n1 - Nmeas) * c1 + 1000 * t1
> + * Let constant c2 = n1 *c1 + 1000 * t1
> + * milli_Tmeas = c2 - Nmeas * c1
> */
> - data->c1 = 1000 * (t1 - t2) / (n1 - n2);
> - data->c2 = 1000 * t2 - data->c1 * n2;
> + temp64 = FACTOR0;
> + temp64 *= 1000;
> + do_div(temp64, FACTOR1 * n1 - FACTOR2);
> + data->c1 = temp64;
> + data->c2 = n1 * data->c1 + 1000 * t1;
>
> /*
> * Set the default passive cooling trip point to 20 ?C below the
> --
> 1.7.9.5
>
>
^ permalink raw reply
* [PATCH v3 01/13] ARM: mvebu: rename armada-370-xp.c to armada-mvebu.c
From: Jason Cooper @ 2014-02-13 13:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140213125526.3af08c54@skate>
On Thu, Feb 13, 2014 at 12:55:26PM +0100, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
>
> On Thu, 13 Feb 2014 12:50:15 +0100, Arnd Bergmann wrote:
> > On Thursday 13 February 2014 12:04:23 Thomas Petazzoni wrote:
> > > In preparation to the introduction of the support of Armada 375 and
> > > Armada 38x, this commit renames arch/arm/mach-mvebu/armada-370-xp.c to
> > > arch/arm/mach-mvebu/armada-mvebu.c. The armada-mvebu.c name was chosen
> > > because:
> > >
> > > * As we are going to merge the support for Kirkwood and Dove into
> > > mach-mvebu, there will be other files with DT_MACHINE_START
> > > structures, so a generic name such as board-dt.c or mvebu.c does
> > > not work.
> > >
> > > * A simple armada.c does not work, because there are Marvell Armada
> > > SOCs that are not part of the MVEBU family. For example, the
> > > Marvell Armada 1500 are part of the mach-berlin family, which is a
> > > completely separate line of SOCs.
> >
> > Your reasoning for the new name makes a lot of sense, but my personal
> > opinion is that I'd rather leave the name as it is and deal with the
> > fact that it's not the best name. Renaming files often causes unexpected
> > problems, in particular if someone else wants to modify the same file.
>
> I believe it's a matter of taste here. Having a file named
> armada-370-xp.c that handles Armada 375 and Armada 38x looks highly
> confusing to me, and I believe both Gr?gory and Ezequiel were of the
> same opinion.
>
> The number of changes to this file is very limited, so the probability
> of having a large number of complicated patches touching the same file
> being in flight is fairly low.
>
> Maybe we can leave this taste decision to the mach-mvebu maintainers?
board-v7.c and then board-v5.c ?
thx,
Jason.
^ 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