* [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA
From: liviu.dudau at arm.com @ 2016-11-11 10:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8F5F06@lhreml507-mbx>
On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
>
> > -----Original Message-----
> > From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.marinas at arm.com; will.deacon at arm.com;
> > robh+dt at kernel.org; bhelgaas at google.com; mark.rutland at arm.com;
> > olof at lixom.net; arnd at arndb.de; linux-arm-kernel at lists.infradead.org;
> > lorenzo.pieralisi at arm.com; linux-kernel at vger.kernel.org; Linuxarm;
> > devicetree at vger.kernel.org; linux-pci at vger.kernel.org; linux-
> > serial at vger.kernel.org; minyard at acm.org; benh at kernel.crashing.org;
> > zourongrong at gmail.com; John Garry; zhichang.yuan02 at gmail.com;
> > kantyzc at 163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> >
> > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> >
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> >
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> >
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> >
> > >
> > > >
> > > > > + return 0;
> > > > > +
> > > > > + return 1;
> > > > > +}
> > > > >
> > > > > BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > > return false;
> > > > > }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + * For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + * The device addresses are described directly in their regs
> > > > property.
> > > > > + * This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + * devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent: points to the parent dts node;
> > > > > + * @bus: points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr: the address from reg property;
> > > > > + * @na: the address cell counter of @addr;
> > > > > + * @presult: store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > + struct of_bus *bus, __be32 *addr,
> > > > > + int na, u64 *presult)
> > > > > +{
> > > > > + unsigned int flags;
> > > > > + unsigned int rlen;
> > > > > +
> > > > > + /* whether support indirectIO */
> > > > > + if (!indirect_io_enabled())
> > > > > + return 0;
> > > > > +
> > > > > + if (!of_bus_isa_match(parent))
> > > > > + return 0;
> > > > > +
> > > > > + flags = bus->get_flags(addr);
> > > > > + if (!(flags & IORESOURCE_IO))
> > > > > + return 0;
> > > > > +
> > > > > + /* there is ranges property, apply the normal translation
> > > > directly. */
> > > >
> > > > s/there is ranges/if we have a 'ranges'/
> > >
> > > Thanks for spotting this
> > >
> > > >
> > > > > + if (of_get_property(parent, "ranges", &rlen))
> > > > > + return 0;
> > > > > +
> > > > > + *presult = of_read_number(addr + 1, na - 1);
> > > > > + /* this fixup is only valid for specific I/O range. */
> > > > > + return addr_is_indirect_io(*presult);
> > > > > +}
> > > > > +
> > > > > static int of_translate_one(struct device_node *parent, struct
> > > > of_bus *bus,
> > > > > struct of_bus *pbus, __be32 *addr,
> > > > > int na, int ns, int pna, const char *rprop)
> > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > > device_node *dev,
> > > > > result = of_read_number(addr, na);
> > > > > break;
> > > > > }
> > > > > + /*
> > > > > + * For indirectIO device which has no ranges
> > property, get
> > > > > + * the address from reg directly.
> > > > > + */
> > > > > + if (of_get_isa_indirect_io(dev, bus, addr, na,
> > &result)) {
> > > > > + pr_debug("isa indirectIO matched(%s)..addr =
> > > > 0x%llx\n",
> > > > > + of_node_full_name(dev), result);
> > > > > + break;
> > > > > + }
> > > > >
> > > > > /* Get new parent bus and counts */
> > > > > pbus = of_match_bus(parent);
> > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > > device_node *dev,
> > > > > if (taddr == OF_BAD_ADDR)
> > > > > return -EINVAL;
> > > > > memset(r, 0, sizeof(struct resource));
> > > > > - if (flags & IORESOURCE_IO) {
> > > > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > > > unsigned long port;
> > > > > +
> > > > > port = pci_address_to_pio(taddr);
> > > > > if (port == (unsigned long)-1)
> > > > > return -EINVAL;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index ba34907..1a08511 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -3263,7 +3263,7 @@ int __weak
> > pci_register_io_range(phys_addr_t
> > > > addr, resource_size_t size)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > > >
> > > > > /* check if the range hasn't been previously recorded */
> > > > > spin_lock(&io_range_lock);
> > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> > long
> > > > pio)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > > Have you checked that pci_pio_to_address still returns valid values
> > > > after this? I know that
> > > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > > allocating reserving the IO ranges,
> > > > but the values added in the io_range_list are still starting from
> > zero,
> > > > no from PCIBIOS_MIN_IO,
> > >
> > > I think you're wrong here as in pci_address_to_pio we have:
> > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > >
> > > This should be enough to guarantee that the PIOs start at
> > > PCIBIOS_MIN_IO...right?
> >
> > I don't think you can guarantee that the pio value that gets passed
> > into
> > pci_pio_to_address() always comes from a previously returned value by
> > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
>
> Maybe I am missing something...could you make an exampleof a case
> where an IO toke doesn?t come from pci_address_to_pio() ?
Don't know, maybe it is coming from some DT or platform data? I was just
asking for confirmation that no one has seen issues there, I'm not saying
I've seen it happening.
Best regards,
Liviu
>
> Thanks
>
> Gab
>
>
> >
> > if (pio < PCIBIOS_MIN_IO)
> > return address;
> >
> > to avoid adding more checks in the list_for_each_entry() loop.
> >
> > Best regards,
> > Liviu
> >
> > >
> > >
> > > > so the calculation of the address in this function could return
> > > > negative values casted to pci_addr_t.
> > > >
> > > > Maybe you want to adjust the range->start value in
> > > > pci_register_io_range() as well to have it
> > > > offset by PCIBIOS_MIN_IO as well.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > if (pio > IO_SPACE_LIMIT)
> > > > > return address;
> > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > > pci_address_to_pio(phys_addr_t address)
> > > > > {
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *res;
> > > > > - resource_size_t offset = 0;
> > > > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > > > > unsigned long addr = -1;
> > > > >
> > > > > spin_lock(&io_range_lock);
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index 3786473..deec469 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > > > #define for_each_of_pci_range(parser, range) \
> > > > > for (; of_pci_range_parser_one(parser, range);)
> > > > >
> > > > > +
> > > > > +#ifndef indirect_io_enabled
> > > > > +#define indirect_io_enabled indirect_io_enabled
> > > > > +static inline bool indirect_io_enabled(void)
> > > > > +{
> > > > > + return false;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#ifndef addr_is_indirect_io
> > > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > /* Translate a DMA address from device space to CPU space */
> > > > > extern u64 of_translate_dma_address(struct device_node *dev,
> > > > > const __be32 *in_addr);
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 0e49f70..7f6bbb6 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > > pci_bus *bus)
> > > > > /* provide the legacy pci_dma_* API */
> > > > > #include <linux/pci-dma-compat.h>
> > > > >
> > > > > +/*
> > > > > + * define this macro here to refrain from compilation error for
> > some
> > > > > + * platforms. Please keep this macro at the end of this header
> > file.
> > > > > + */
> > > > > +#ifndef PCIBIOS_MIN_IO
> > > > > +#define PCIBIOS_MIN_IO 0
> > > > > +#endif
> > > > > +
> > > > > #endif /* LINUX_PCI_H */
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-
> > pci"
> > > > in
> > > > > the body of a message to majordomo at vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-
> > info.html
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world, |
> > | but they're not |
> > | giving me the |
> > \ source code! /
> > ---------------
> > ?\_(?)_/?
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Anurup M @ 2016-11-11 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d009dcdc-2258-ed6a-53be-2211c7493051@huawei.com>
On Wednesday 09 November 2016 02:36 PM, John Garry wrote:
>
>>>>> I'd suggest requiring #address-cells=<1> and #size-cells=<0> in the
>>>>> master
>>>>> node, and listing the children by reg property. If the address is not
>>>>> easily expressed as a single integer, use a larger #address-cells
>>>>> value.
>>>> We already have something equivalent to reg in "module-id" (see patch
>>>> 02/11), which is the slave device bus address; here's a sample:
>>>> + /* For L3 cache PMU */
>>>> + pmul3c0 {
>>>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>>>> + scl-id = <0x02>;
>>>> + num-events = <0x16>;
>>>> + num-counters = <0x08>;
>>>> + module-id = <0x04>;
>>>> + num-banks = <0x04>;
>>>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>>>> + counter-reg = <0x170>;
>>>> + evctrl-reg = <0x04>;
>>>> + event-en = <0x1000000>;
>>>> + evtype-reg = <0x140>;
>>>> + };
>>>>
>>>> FYI, "module-id" is our own internal hw nomenclature.
>>> Yes, that was my interpretation as well. Please use the standard
>>> "reg" property for this then.
>> Hi Arnd,
>>
>> Firstly my apologies for a mistake in the bindings example in ([PATCH
>> 02/11 ..]).
>> The module-id property is a list as defined in the PMU bindings patch
>> ([PATCH v1 05/11] dt-bindings .. <https://lkml.org/lkml/2016/11/2/323>).
>>
>> + djtag0: djtag at 0 {
>> + compatible = "hisilicon,hip05-cpu-djtag-v1";
>> + pmul3c0 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + scl-id = <0x02>;
>> + num-events = <0x16>;
>> + num-counters = <0x08>;
>> + module-id = <0x04 0x04 0x04 0x04>;
>> + num-banks = <0x04>;
>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>> + counter-reg = <0x170>;
>> + evctrl-reg = <0x04>;
>> + event-en = <0x1000000>;
>> + evtype-reg = <0x140>;
>> + };
>>
>>
>> The L3 cache in hip05/06/07 chips consist of 4 banks (each bank has PMU
>> registers).
>>
>> In hip05/06 all L3 cache banks are identified with same module-id.
>> module-id = <0x04 0x04 0x04 0x04>;
>>
>> But in the case hip07 chip(djtag v2), each L3 cache bank has different
>> module-id
>> module-id = <0x01 0x02 0x03 0x04>;
>>
>> So in this case Please share your opinion on how to model it.
>>
>
> My suggestion is to have a single PMU per module, whether that is 4
> banks or 1 bank per module, as this makes the driver simpler.
>
> I think you mentioned that a separate PMU per bank does not make much
> sense, and you would rather treat all banks as a single bank and
> aggregrate their perf statstics under a single PMU: Can you just use a
> script in userspace which can do this aggregration work if you have
> separate PMUs?
Hi John,
Mark also suggest the same view.
I have some concerns or doubts in having separate PMU for each L3 cache
bank.
We can discuss it in the same thread [[RESEND PATCH v1 07/11]]
<http://www.spinics.net/lists/arm-kernel/msg541938.html>].
Thanks,
Anurup
>
> Maybe perf guys have a view on this also.
>
> John
>
>> Some more detail of L3 cache PMU.
>> ------------------------------------------------
>> The hip05/06/07 chips consists of a multiple Super CPU cluster (16 CPU
>> cores). we call it SCCL.
>> The L3 cache( 4 banks) is shared by all CPU cores in a SCCL.
>> Each L3 cache bank has PMU registers. We always take the sum of the
>> counters to show in perf.
>> Taking individual L3 cache count is not meaningful as there is no
>> mapping of CPU cores to individual
>> L3 cache banks.
>>
>> Please share your suggestion.
>>
>> Thanks,
>> Anurup
>
^ permalink raw reply
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Jan Glauber @ 2016-11-11 10:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108235010.GC17771@arm.com>
Hi Will,
thanks for the review!
On Tue, Nov 08, 2016 at 11:50:10PM +0000, Will Deacon wrote:
> Hi Jan,
>
> Thanks for posting an updated series. I have a few minor comments, which
> we can hopefully address in time for 4.10.
>
> Also, have you run the perf fuzzer with this series applied?
No, that's new to me. Will try it.
> https://github.com/deater/perf_event_tests
>
> (build the tests and look under the fuzzer/ directory for the tool)
>
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > Provide "uncore" facilities for different non-CPU performance
> > counter units.
> >
> > The uncore PMUs can be found under /sys/bus/event_source/devices.
> > All counters are exported via sysfs in the corresponding events
> > files under the PMU directory so the perf tool can list the event names.
> >
> > There are some points that are special in this implementation:
> >
> > 1) The PMU detection relies on PCI device detection. If a
> > matching PCI device is found the PMU is created. The code can deal
> > with multiple units of the same type, e.g. more than one memory
> > controller.
> >
> > 2) Counters are summarized across different units of the same type
> > on one NUMA node but not across NUMA nodes.
> > For instance L2C TAD 0..7 are presented as a single counter
> > (adding the values from TAD 0 to 7). Although losing the ability
> > to read a single value the merged values are easier to use.
> >
> > 3) The counters are not CPU related. A random CPU is picked regardless
> > of the NUMA node. There is a small performance penalty for accessing
> > counters on a remote note but reading a performance counter is a
> > slow operation anyway.
> >
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> > drivers/perf/Kconfig | 13 ++
> > drivers/perf/Makefile | 1 +
> > drivers/perf/uncore/Makefile | 1 +
> > drivers/perf/uncore/uncore_cavium.c | 351 ++++++++++++++++++++++++++++++++++++
> > drivers/perf/uncore/uncore_cavium.h | 71 ++++++++
>
> We already have "uncore" PMUs under drivers/perf, so I'd prefer that we
> renamed this a bit to reflect better what's going on. How about:
>
> drivers/perf/cavium/
>
> and then
>
> drivers/perf/cavium/uncore_thunder.[ch]
>
> ?
OK, agreed.
> > include/linux/cpuhotplug.h | 1 +
> > 6 files changed, 438 insertions(+)
> > create mode 100644 drivers/perf/uncore/Makefile
> > create mode 100644 drivers/perf/uncore/uncore_cavium.c
> > create mode 100644 drivers/perf/uncore/uncore_cavium.h
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 4d5c5f9..3266c87 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -19,4 +19,17 @@ config XGENE_PMU
> > help
> > Say y if you want to use APM X-Gene SoC performance monitors.
> >
> > +config UNCORE_PMU
> > + bool
>
> This isn't needed.
I thought about a symbol for all uncore pmus. But when drivers/perf is
already that it can be removed.
> > +
> > +config UNCORE_PMU_CAVIUM
> > + depends on PERF_EVENTS && NUMA && ARM64
> > + bool "Cavium uncore PMU support"
>
> Please mentioned Thunder somewhere, since that's the SoC being supported.
OK.
> > + select UNCORE_PMU
> > + default y
> > + help
> > + Say y if you want to access performance counters of subsystems
> > + on a Cavium SOC like cache controller, memory controller or
> > + processor interconnect.
> > +
> > endmenu
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> > index b116e98..d6c02c9 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -1,2 +1,3 @@
> > obj-$(CONFIG_ARM_PMU) += arm_pmu.o
> > obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> > +obj-y += uncore/
> > diff --git a/drivers/perf/uncore/Makefile b/drivers/perf/uncore/Makefile
> > new file mode 100644
> > index 0000000..6130e18
> > --- /dev/null
> > +++ b/drivers/perf/uncore/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_UNCORE_PMU_CAVIUM) += uncore_cavium.o
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * Cavium Thunder uncore PMU support.
> > + *
> > + * Copyright (C) 2015,2016 Cavium Inc.
> > + * Author: Jan Glauber <jan.glauber@cavium.com>
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/numa.h>
> > +#include <linux/slab.h>
> > +
> > +#include "uncore_cavium.h"
> > +
> > +/*
> > + * Some notes about the various counters supported by this "uncore" PMU
> > + * and the design:
> > + *
> > + * All counters are 64 bit long.
> > + * There are no overflow interrupts.
> > + * Counters are summarized per node/socket.
> > + * Most devices appear as separate PCI devices per socket with the exception
> > + * of OCX TLK which appears as one PCI device per socket and contains several
> > + * units with counters that are merged.
> > + * Some counters are selected via a control register (L2C TAD) and read by
> > + * a number of counter registers, others (L2C CBC, LMC & OCX TLK) have
> > + * one dedicated counter per event.
> > + * Some counters are not stoppable (L2C CBC & LMC).
> > + * Some counters are read-only (LMC).
> > + * All counters belong to PCI devices, the devices may have additional
> > + * drivers but we assume we are the only user of the counter registers.
> > + * We map the whole PCI BAR so we must be careful to forbid access to
> > + * addresses that contain neither counters nor counter control registers.
> > + */
> > +
> > +void thunder_uncore_read(struct perf_event *event)
> > +{
>
> Rather than have a bunch of global symbols that are called from the
> individual drivers, why don't you pass a struct of function pointers to
> their respective init functions and keep the internals private?
Most of these functions are already in struct pmu. What I can do is
set the shared functions in thunder_uncore_setup as default, and
only override them as needed (like thunder_uncore_read_ocx_tlk)
or the other way around (use default if not set already).
Then I can get rid of them.
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 prev, delta, new = 0;
> > +
> > + node = get_node(hwc->config, uncore);
> > +
> > + /* read counter values from all units on the node */
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > +
> > + prev = local64_read(&hwc->prev_count);
> > + local64_set(&hwc->prev_count, new);
> > + delta = new - prev;
> > + local64_add(delta, &event->count);
> > +}
> > +
> > +int thunder_uncore_add(struct perf_event *event, int flags, u64 config_base,
> > + u64 event_base)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int id;
> > +
> > + node = get_node(hwc->config, uncore);
> > + id = get_id(hwc->config);
> > +
> > + if (!cmpxchg(&node->events[id], NULL, event))
> > + hwc->idx = id;
>
> Does this need to be a full-fat cmpxchg? Who are you racing with?
Just copy'n'paste from the existing drivers. I guess it can be relaxed.
> > +
> > + if (hwc->idx == -1)
> > + return -EBUSY;
>
> This would be much clearer as an else statement after the cmpxchg.
Agreed.
> > +
> > + hwc->config_base = config_base;
> > + hwc->event_base = event_base;
> > + hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > +
> > + if (flags & PERF_EF_START)
> > + uncore->pmu.start(event, PERF_EF_RELOAD);
> > +
> > + return 0;
> > +}
> > +
> > +void thunder_uncore_del(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int i;
> > +
> > + event->pmu->stop(event, PERF_EF_UPDATE);
> > +
> > + /*
> > + * For programmable counters we need to check where we installed it.
> > + * To keep this function generic always test the more complicated
> > + * case (free running counters won't need the loop).
> > + */
> > + node = get_node(hwc->config, uncore);
> > + for (i = 0; i < node->num_counters; i++) {
> > + if (cmpxchg(&node->events[i], event, NULL) == event)
> > + break;
> > + }
> > + hwc->idx = -1;
> > +}
> > +
> > +void thunder_uncore_start(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 new = 0;
> > +
> > + /* read counter values from all units on the node */
> > + node = get_node(hwc->config, uncore);
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > + local64_set(&hwc->prev_count, new);
> > +
> > + hwc->state = 0;
> > + perf_event_update_userpage(event);
> > +}
> > +
> > +void thunder_uncore_stop(struct perf_event *event, int flags)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > +
> > + hwc->state |= PERF_HES_STOPPED;
> > +
> > + if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> > + thunder_uncore_read(event);
> > + hwc->state |= PERF_HES_UPTODATE;
> > + }
> > +}
> > +
> > +int thunder_uncore_event_init(struct perf_event *event)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore *uncore;
> > +
> > + if (event->attr.type != event->pmu->type)
> > + return -ENOENT;
> > +
> > + /* we do not support sampling */
> > + if (is_sampling_event(event))
> > + return -EINVAL;
> > +
> > + /* counters do not have these bits */
> > + if (event->attr.exclude_user ||
> > + event->attr.exclude_kernel ||
> > + event->attr.exclude_host ||
> > + event->attr.exclude_guest ||
> > + event->attr.exclude_hv ||
> > + event->attr.exclude_idle)
> > + return -EINVAL;
> > +
> > + uncore = to_uncore(event->pmu);
> > + if (!uncore)
> > + return -ENODEV;
> > + if (!uncore->event_valid(event->attr.config & UNCORE_EVENT_ID_MASK))
> > + return -EINVAL;
> > +
> > + /* check NUMA node */
> > + node = get_node(event->attr.config, uncore);
> > + if (!node) {
> > + pr_debug("Invalid NUMA node selected\n");
> > + return -EINVAL;
> > + }
> > +
> > + hwc->config = event->attr.config;
> > + hwc->idx = -1;
> > + return 0;
> > +}
> > +
> > +static ssize_t thunder_uncore_attr_show_cpumask(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pmu *pmu = dev_get_drvdata(dev);
> > + struct thunder_uncore *uncore =
> > + container_of(pmu, struct thunder_uncore, pmu);
> > +
> > + return cpumap_print_to_pagebuf(true, buf, &uncore->active_mask);
> > +}
> > +static DEVICE_ATTR(cpumask, S_IRUGO, thunder_uncore_attr_show_cpumask, NULL);
> > +
> > +static struct attribute *thunder_uncore_attrs[] = {
> > + &dev_attr_cpumask.attr,
> > + NULL,
> > +};
> > +
> > +struct attribute_group thunder_uncore_attr_group = {
> > + .attrs = thunder_uncore_attrs,
> > +};
> > +
> > +ssize_t thunder_events_sysfs_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *page)
> > +{
> > + struct perf_pmu_events_attr *pmu_attr =
> > + container_of(attr, struct perf_pmu_events_attr, attr);
> > +
> > + if (pmu_attr->event_str)
> > + return sprintf(page, "%s", pmu_attr->event_str);
> > +
> > + return 0;
> > +}
> > +
> > +/* node attribute depending on number of NUMA nodes */
> > +static ssize_t node_show(struct device *dev, struct device_attribute *attr,
> > + char *page)
> > +{
> > + if (NODES_SHIFT)
> > + return sprintf(page, "config:16-%d\n", 16 + NODES_SHIFT - 1);
>
> If NODES_SHIFT is 1, you'll end up with "config:16-16", which might confuse
> userspace.
So should I use "config:16" in that case? Is it OK to use this also for
NODES_SHIFT=0 ?
> > + else
> > + return sprintf(page, "config:16\n");
> > +}
> > +
> > +struct device_attribute format_attr_node = __ATTR_RO(node);
> > +
> > +/*
> > + * Thunder uncore events are independent from CPUs. Provide a cpumask
> > + * nevertheless to prevent perf from adding the event per-cpu and just
> > + * set the mask to one online CPU. Use the same cpumask for all uncore
> > + * devices.
> > + *
> > + * There is a performance penalty for accessing a device from a CPU on
> > + * another socket, but we do not care (yet).
> > + */
> > +static int thunder_uncore_offline_cpu(unsigned int old_cpu, struct hlist_node *node)
> > +{
> > + struct thunder_uncore *uncore = hlist_entry_safe(node, struct thunder_uncore, node);
>
> Why _safe?
Not required, will remove.
> > + int new_cpu;
> > +
> > + if (!cpumask_test_and_clear_cpu(old_cpu, &uncore->active_mask))
> > + return 0;
> > + new_cpu = cpumask_any_but(cpu_online_mask, old_cpu);
> > + if (new_cpu >= nr_cpu_ids)
> > + return 0;
> > + perf_pmu_migrate_context(&uncore->pmu, old_cpu, new_cpu);
> > + cpumask_set_cpu(new_cpu, &uncore->active_mask);
> > + return 0;
> > +}
> > +
> > +static struct thunder_uncore_node * __init alloc_node(struct thunder_uncore *uncore,
> > + int node_id, int counters)
> > +{
> > + struct thunder_uncore_node *node;
> > +
> > + node = kzalloc(sizeof(*node), GFP_KERNEL);
> > + if (!node)
> > + return NULL;
> > + node->num_counters = counters;
> > + INIT_LIST_HEAD(&node->unit_list);
> > + return node;
> > +}
> > +
> > +int __init thunder_uncore_setup(struct thunder_uncore *uncore, int device_id,
> > + struct pmu *pmu, int counters)
> > +{
> > + unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
> > + struct thunder_uncore_unit *unit, *tmp;
> > + struct thunder_uncore_node *node;
> > + struct pci_dev *pdev = NULL;
> > + int ret, node_id, found = 0;
> > +
> > + /* detect PCI devices */
> > + while ((pdev = pci_get_device(vendor_id, device_id, pdev))) {
>
> Redundant brackets?
OK
> > + if (!pdev)
> > + break;
>
> Redundant check?
OK
> > + node_id = dev_to_node(&pdev->dev);
> > +
> > + /* allocate node if necessary */
> > + if (!uncore->nodes[node_id])
> > + uncore->nodes[node_id] = alloc_node(uncore, node_id, counters);
> > +
> > + node = uncore->nodes[node_id];
> > + if (!node) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit = kzalloc(sizeof(*unit), GFP_KERNEL);
> > + if (!unit) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit->pdev = pdev;
> > + unit->map = ioremap(pci_resource_start(pdev, 0),
> > + pci_resource_len(pdev, 0));
> > + list_add(&unit->entry, &node->unit_list);
> > + node->nr_units++;
> > + found++;
> > + }
> > +
> > + if (!found)
> > + return -ENODEV;
> > +
> > + cpuhp_state_add_instance_nocalls(CPUHP_AP_UNCORE_CAVIUM_ONLINE,
> > + &uncore->node);
> > +
> > + /*
> > + * perf PMU is CPU dependent in difference to our uncore devices.
> > + * Just pick a CPU and migrate away if it goes offline.
> > + */
> > + cpumask_set_cpu(smp_processor_id(), &uncore->active_mask);
> > +
> > + uncore->pmu = *pmu;
> > + ret = perf_pmu_register(&uncore->pmu, uncore->pmu.name, -1);
> > + if (ret)
> > + goto fail;
> > +
> > + return 0;
> > +
> > +fail:
> > + node_id = 0;
> > + while (uncore->nodes[node_id]) {
> > + node = uncore->nodes[node_id];
> > +
> > + list_for_each_entry_safe(unit, tmp, &node->unit_list, entry) {
>
> Why do you need the _safe variant?
OK, not needed
> Will
^ permalink raw reply
* [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Anurup M @ 2016-11-11 10:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4812554.ng09YGQfZ9@wuerfel>
On Thursday 10 November 2016 03:10 AM, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 9:58:38 AM CET Anurup M wrote:
>>> I also see that the compatible strings have the version included in
>>> them, and you can probably drop them by requiring them only in the
>>> fallback:
>>>
>>> compatible = "hisilicon,hip05-cpu-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip05-io-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip06-cpu-djtag", "hisilicon,djtag-v1";
>>> compatible = "hisilicon,hip06-io-djtag", "hisilicon,djtag-v2";
>>> compatible = "hisilicon,hip07-cpu-djtag", "hisilicon,djtag-v2";
>>> compatible = "hisilicon,hip07-io-djtag", "hisilicon,djtag-v2";
>>>
>>> We want to have the first entry be as specific as possible, but
>>> the last (second) entry is the one that can be used by the driver
>>> for matching. When a future hip08/hip09/... chip uses an existing
>>> interface, you then don't have to update the driver.
>> Thanks. I had a similar thought on this. So as I have the version string
>> in the
>> second entry "-v(1/2)".
>> I can use it in driver for matching. So i think I will change it as below.
>> Please correct me if my understanding is wrong.
>>
>> static const struct of_device_id djtag_of_match[] = {
>> - /* for hip05(D02) cpu die */
>> - { .compatible = "hisilicon,hip05-cpu-djtag-v1",
>> + /* for hisi djtag-v1 cpu die */
>> + { .compatible = "hisilicon,hisi-cpu-djtag-v1",
>> .data = djtag_readwrite_v1 },
>> - /* for hip05(D02) io die */
>> - { .compatible = "hisilicon,hip05-io-djtag-v1",
>> + /* for hisi djtag-v1 io die */
>> + { .compatible = "hisilicon,hisi-io-djtag-v1",
> From the code it looks like "hisilicon,hisi-io-djtag-v1" and
> "hisilicon,hisi-cpu-djtag-v1" have the same register-level interface,
> so we just need one compatible string for them to match the driver.
>
> Arnd
The djtag versions in CPU die and IO die can be different in the same chip.
For example in hip06, the CPU die has djtag-v1 whereas IO die has djtag-v2.
So I think it need two different compatible string
for hip06 chip CPU DIE "hisilicon,hisi-cpu-djtag-v1"
for hip06 chip IO DIE "hisilicon,hisi-io-djtag-v2"
Thanks,
Anurup
^ permalink raw reply
* [PULL 0/3] KVM/ARM updates for v4.9-rc4
From: Marc Zyngier @ 2016-11-11 10:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <09984771-dfe4-1811-7d2e-29eaf34d99a9@redhat.com>
On 11/11/16 10:14, Paolo Bonzini wrote:
>
>
> On 04/11/2016 19:36, Marc Zyngier wrote:
>> git://git.kernel.org:/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-v4.9-rc4
>
> What is the extra colon after "org"? :)
That's obviously me screwing up when hacking the .git/config to have
different fetch and push URLs. The odd thing is that git (2.10.1)
doesn't even complain and happily pulls the branch... Did you get an
error? Anyway, I'll fix that in my local config.
> Pulled now, and I will send it in time for rc5.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PULL 0/3] KVM/ARM updates for v4.9-rc4
From: Paolo Bonzini @ 2016-11-11 10:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161104183638.28137-1-marc.zyngier@arm.com>
On 04/11/2016 19:36, Marc Zyngier wrote:
> git://git.kernel.org:/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-v4.9-rc4
What is the extra colon after "org"? :)
Pulled now, and I will send it in time for rc5.
Thanks,
Paolo
^ permalink raw reply
* [linux-sunxi] [PATCH] clk: sunxi-ng: sun8i-h3: Set CLK_SET_RATE_PARENT for audio module clocks
From: Code Kipper @ 2016-11-11 10:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111100558.14629-2-wens@csie.org>
On 11 November 2016 at 11:05, Chen-Yu Tsai <wens@csie.org> wrote:
> The audio module clocks are supposed to be set according to the sample
> rate of the audio stream. The audio PLL provides the clock signal for
> thees module clocks, and only it is freely tunable.
nick! these
CK
>
> Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
> properly tune the clock rate.
>
> Fixes: 0577e4853bfb ("clk: sunxi-ng: Add H3 clocks")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> index 4d70590f05e3..21c427d86f28 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
> @@ -394,16 +394,16 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
> static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
> "pll-audio-2x", "pll-audio" };
> static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
> - 0x0b0, 16, 2, BIT(31), 0);
> + 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
> - 0x0b4, 16, 2, BIT(31), 0);
> + 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_MUX_WITH_GATE(i2s2_clk, "i2s2", i2s_parents,
> - 0x0b8, 16, 2, BIT(31), 0);
> + 0x0b8, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_M_WITH_GATE(spdif_clk, "spdif", "pll-audio",
> - 0x0c0, 0, 4, BIT(31), 0);
> + 0x0c0, 0, 4, BIT(31), CLK_SET_RATE_PARENT);
>
> static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
> 0x0cc, BIT(8), 0);
> @@ -466,7 +466,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
> 0x13c, 16, 3, BIT(31), 0);
>
> static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
> - 0x140, BIT(31), 0);
> + 0x140, BIT(31), CLK_SET_RATE_PARENT);
> static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
> 0x144, BIT(31), 0);
>
> --
> 2.10.2
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: zhichang.yuan @ 2016-11-11 10:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <10334260.ztLXZ2Oynd@wuerfel>
Hi, Arnd,
On 2016/11/11 0:07, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
>>
>> Where should we get the range from? For LPC we know that it is going
>> Work on anything that is not used by PCI I/O space, and this is
>> why we use [0, PCIBIOS_MIN_IO]
>
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
>
>>> Your current version has
>>>
>>> if (arm64_extio_ops->pfout) \
>>> arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>> addr, value, sizeof(type)); \
>>>
>>> Instead, just subtract the start of the range from the logical
>>> port number to transform it back into a bus-local port number:
>>
>> These accessors do not operate on IO tokens:
>>
>> If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
>> addr is not going to be an I/O token; in fact patch 2/3 imposes that
>> the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
>> we have free physical addresses that the accessors can operate on.
>
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
>
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
>
> If we separate the two steps:
>
> a) assign a range of logical I/O port numbers to a bus
> b) register a set of helpers for redirecting logical I/O
> port to a helper function
>
It seems that we need to add a new bus and the corresponding resource management
which can also cover current PCI pio mapping, is it right?
Thanks,
Zhichang
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
>
> Arnd
>
> .
>
^ permalink raw reply
* [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced
From: zhichang.yuan @ 2016-11-11 10:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478806353.7430.137.camel@kernel.crashing.org>
Hi, Ben, Mark,
Thanks for your comments! These are helpful!
On 2016/11/11 3:32, Benjamin Herrenschmidt wrote:
> On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
>> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
>>> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
>>
>> Just to check, do you hook that in your inb/outb/etc?
>
> Yes.
>
>> Generally, it would seem nicer if we could have higher-level
>> isa_{inb,outb,whatever} accessors that we could hook separately from
>> other IO.
>
> Maybe but generally speaking, we don't discriminate accessors per bus,
> ie, readl etc... work on all memory mapped busses, inb... works on all
> busses with an "IO space", at least that's been the idea. It probably
> all comes from the fact that PCI IO and ISA are the same space on
> x86 and most other platforms (not all).
>
>> We don't necessarily have to move all ISA drivers over to that if we had
>> a separate symbol for that interface.
>
> What I do on ppc today is that I have a chunk of virtual address space
> that is reserved for "IO space". The first 64k are "reserved" in that
> they route to "the primary" ISA bus (for legacy crap that uses hard
> coded addresses, though I use that for my LPC bus too). I "allocate"
> space for the PCI IO spaces higher in that space. Was I to support more
> LPC busses I could allocate them up there too.
>
I have similar idea as your PPC MMIO.
We notice the prototype of {in/out()} is something like that:
static inline u8 inb(unsigned long addr)
static inline void outb(u8 value, unsigned long addr)
The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
So, could you divide this 'addr' into several bit segments? The top 8 bits is
defined as bus index. For normal direct IO, the bus index is 0. For those bus
device which need indirectIO or some special I/O accessors, when these devices
are initializing, can request to allocate an unique ID to them, and register
their own accessors to the entry which is corresponding to the ID.
In this way, we can support multiple domains, I think.
But I am not sure whether it is feasible, for example, are there some
architectures/platforms had populated the top 8 bits? Do we need to request IO
region from ioport_resource for those devices? etc...
Thanks,
Zhichang
> The IO resource of a given device thus becomes the actual IO port plus
> the offset of the base of the segment it's in.
>
> For memory mapped IO, inb/outb will just add the virtual address of
> the base of all IO space to that. The hooking mechanism will pickup
> the stuff that isn't memory mapped.
>
> It's a bit messy but then IO space performance has never been a huge
> worry since IO cycles tend to be very slow to begin with.
>
> Note: We also have the ISA memory and ISA FW spaces that we don't have
> good accessors for. They somewhat exist (I think the fbdev layer uses
> some for vga) but it's messy.
>
> Cheers,
> Ben.
>
>
> .
>
^ permalink raw reply
* [PATCH] clk: sunxi-ng: sun8i-h3: Set CLK_SET_RATE_PARENT for audio module clocks
From: Chen-Yu Tsai @ 2016-11-11 10:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111100558.14629-1-wens@csie.org>
The audio module clocks are supposed to be set according to the sample
rate of the audio stream. The audio PLL provides the clock signal for
thees module clocks, and only it is freely tunable.
Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
properly tune the clock rate.
Fixes: 0577e4853bfb ("clk: sunxi-ng: Add H3 clocks")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 4d70590f05e3..21c427d86f28 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -394,16 +394,16 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
"pll-audio-2x", "pll-audio" };
static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
- 0x0b0, 16, 2, BIT(31), 0);
+ 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
- 0x0b4, 16, 2, BIT(31), 0);
+ 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s2_clk, "i2s2", i2s_parents,
- 0x0b8, 16, 2, BIT(31), 0);
+ 0x0b8, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_M_WITH_GATE(spdif_clk, "spdif", "pll-audio",
- 0x0c0, 0, 4, BIT(31), 0);
+ 0x0c0, 0, 4, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
0x0cc, BIT(8), 0);
@@ -466,7 +466,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
0x13c, 16, 3, BIT(31), 0);
static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
- 0x140, BIT(31), 0);
+ 0x140, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
0x144, BIT(31), 0);
--
2.10.2
^ permalink raw reply related
* [PATCH] clk: sunxi-ng: sun8i-a23: Set CLK_SET_RATE_PARENT for audio module clocks
From: Chen-Yu Tsai @ 2016-11-11 10:05 UTC (permalink / raw)
To: linux-arm-kernel
The audio module clocks are supposed to be set according to the sample
rate of the audio stream. The audio PLL provides the clock signal for
thees module clocks, and only it is freely tunable.
Set CLK_SET_RATE_PARENT for the audio module clocks so their users can
properly tune the clock rate.
Fixes: 5690879d93e8 ("clk: sunxi-ng: Add A23 CCU")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/clk/sunxi-ng/ccu-sun8i-a23.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
index 2646d980087b..5c6d37bdf247 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a23.c
@@ -344,10 +344,10 @@ static SUNXI_CCU_MP_WITH_MUX_GATE(spi1_clk, "spi1", mod0_default_parents, 0x0a4,
static const char * const i2s_parents[] = { "pll-audio-8x", "pll-audio-4x",
"pll-audio-2x", "pll-audio" };
static SUNXI_CCU_MUX_WITH_GATE(i2s0_clk, "i2s0", i2s_parents,
- 0x0b0, 16, 2, BIT(31), 0);
+ 0x0b0, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_MUX_WITH_GATE(i2s1_clk, "i2s1", i2s_parents,
- 0x0b4, 16, 2, BIT(31), 0);
+ 0x0b4, 16, 2, BIT(31), CLK_SET_RATE_PARENT);
/* TODO: the parent for most of the USB clocks is not known */
static SUNXI_CCU_GATE(usb_phy0_clk, "usb-phy0", "osc24M",
@@ -415,7 +415,7 @@ static SUNXI_CCU_M_WITH_GATE(ve_clk, "ve", "pll-ve",
0x13c, 16, 3, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(ac_dig_clk, "ac-dig", "pll-audio",
- 0x140, BIT(31), 0);
+ 0x140, BIT(31), CLK_SET_RATE_PARENT);
static SUNXI_CCU_GATE(avs_clk, "avs", "osc24M",
0x144, BIT(31), 0);
--
2.10.2
^ permalink raw reply related
* [PATCH v3 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
The sunxi_pconf_reg helper introduced in the last patch gives us the
chance to rework sunxi_pconf_group_set to have it match the structure
of sunxi_pconf_(group_)get and make it easier to understand.
For each config to set, it:
1. checks if the parameter is supported.
2. checks if the argument is within limits.
3. converts argument to the register value.
4. writes to the register with spinlock held.
As a result the function now blocks unsupported config parameters,
instead of silently ignoring them.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 64 +++++++++++++++++------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index ed71bff39869..fa11a3100346 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -532,23 +532,27 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
{
struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
struct sunxi_pinctrl_group *g = &pctl->groups[group];
- unsigned long flags;
unsigned pin = g->pin - pctl->desc->pin_base;
- u32 val, mask;
- u16 strength;
- u8 dlevel;
int i;
- spin_lock_irqsave(&pctl->lock, flags);
-
for (i = 0; i < num_configs; i++) {
- switch (pinconf_to_config_param(configs[i])) {
+ enum pin_config_param param;
+ unsigned long flags;
+ u32 offset, shift, mask, reg;
+ u16 arg, val;
+ int ret;
+
+ param = pinconf_to_config_param(configs[i]);
+ arg = pinconf_to_config_argument(configs[i]);
+
+ ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+ if (ret < 0)
+ return ret;
+
+ switch (param) {
case PIN_CONFIG_DRIVE_STRENGTH:
- strength = pinconf_to_config_argument(configs[i]);
- if (strength > 40) {
- spin_unlock_irqrestore(&pctl->lock, flags);
+ if (arg < 10 || arg > 40)
return -EINVAL;
- }
/*
* We convert from mA to what the register expects:
* 0: 10mA
@@ -556,37 +560,33 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
* 2: 30mA
* 3: 40mA
*/
- dlevel = strength / 10 - 1;
- val = readl(pctl->membase + sunxi_dlevel_reg(pin));
- mask = DLEVEL_PINS_MASK << sunxi_dlevel_offset(pin);
- writel((val & ~mask)
- | dlevel << sunxi_dlevel_offset(pin),
- pctl->membase + sunxi_dlevel_reg(pin));
+ val = arg / 10 - 1;
break;
case PIN_CONFIG_BIAS_DISABLE:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask),
- pctl->membase + sunxi_pull_reg(pin));
+ val = 0;
break;
case PIN_CONFIG_BIAS_PULL_UP:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask) | 1 << sunxi_pull_offset(pin),
- pctl->membase + sunxi_pull_reg(pin));
+ if (arg == 0)
+ return -EINVAL;
+ val = 1;
break;
case PIN_CONFIG_BIAS_PULL_DOWN:
- val = readl(pctl->membase + sunxi_pull_reg(pin));
- mask = PULL_PINS_MASK << sunxi_pull_offset(pin);
- writel((val & ~mask) | 2 << sunxi_pull_offset(pin),
- pctl->membase + sunxi_pull_reg(pin));
+ if (arg == 0)
+ return -EINVAL;
+ val = 2;
break;
default:
- break;
+ /* sunxi_pconf_reg should catch anything unsupported */
+ WARN_ON(1);
+ return -ENOTSUPP;
}
- } /* for each config */
- spin_unlock_irqrestore(&pctl->lock, flags);
+ spin_lock_irqsave(&pctl->lock, flags);
+ reg = readl(pctl->membase + offset);
+ reg &= ~(mask << shift);
+ writel(reg | val << shift, pctl->membase + offset);
+ spin_unlock_irqrestore(&pctl->lock, flags);
+ } /* for each config */
return 0;
}
--
2.10.2
^ permalink raw reply related
* [PATCH v3 2/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
The sunxi pinctrl driver only caches whatever pinconf setting was last
set on a given pingroup. This is not particularly helpful, nor is it
correct.
Fix this by actually reading the hardware registers and returning
the correct results or error codes. Also filter out unsupported
pinconf settings. Since this driver has a peculiar setup of 1 pin
per group, we can support both pin and pingroup pinconf setting
read back with the same code. The sunxi_pconf_reg helper and code
structure is inspired by pinctrl-msm.
With this done we can also claim to support generic pinconf, by
setting .is_generic = true in pinconf_ops.
Also remove the cached config value. The behavior of this was never
correct, as it only cached 1 setting instead of all of them. Since
we can now read back settings directly from the hardware, it is no
longer required.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 86 +++++++++++++++++++++++++++++++++--
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 1 -
2 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index e04edda8629d..ed71bff39869 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -438,15 +438,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
.get_group_pins = sunxi_pctrl_get_group_pins,
};
+static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
+ u32 *offset, u32 *shift, u32 *mask)
+{
+ switch (param) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ *offset = sunxi_dlevel_reg(pin);
+ *shift = sunxi_dlevel_offset(pin);
+ *mask = DLEVEL_PINS_MASK;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_DISABLE:
+ *offset = sunxi_pull_reg(pin);
+ *shift = sunxi_pull_offset(pin);
+ *mask = PULL_PINS_MASK;
+ break;
+
+ default:
+ return -ENOTSUPP;
+ }
+
+ return 0;
+}
+
+static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
+ unsigned long *config)
+{
+ struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param param = pinconf_to_config_param(*config);
+ u32 offset, shift, mask, val;
+ u16 arg;
+ int ret;
+
+ pin -= pctl->desc->pin_base;
+
+ ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
+ if (ret < 0)
+ return ret;
+
+ val = (readl(pctl->membase + offset) >> shift) & mask;
+
+ switch (pinconf_to_config_param(*config)) {
+ case PIN_CONFIG_DRIVE_STRENGTH:
+ arg = (val + 1) * 10;
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_UP:
+ if (val != SUN4I_PINCTRL_PULL_UP)
+ return -EINVAL;
+ arg = 1; /* hardware is weak pull-up */
+ break;
+
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ if (val != SUN4I_PINCTRL_PULL_DOWN)
+ return -EINVAL;
+ arg = 1; /* hardware is weak pull-down */
+ break;
+
+ case PIN_CONFIG_BIAS_DISABLE:
+ if (val != SUN4I_PINCTRL_NO_PULL)
+ return -EINVAL;
+ arg = 0;
+ break;
+
+ default:
+ /* sunxi_pconf_reg should catch anything unsupported */
+ WARN_ON(1);
+ return -ENOTSUPP;
+ }
+
+ *config = pinconf_to_config_packed(param, arg);
+
+ return 0;
+}
+
static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
unsigned group,
unsigned long *config)
{
struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+ struct sunxi_pinctrl_group *g = &pctl->groups[group];
- *config = pctl->groups[group].config;
-
- return 0;
+ /* We only support 1 pin per group. Chain it to the pin callback */
+ return sunxi_pconf_get(pctldev, g->pin, config);
}
static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
@@ -508,8 +584,6 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
default:
break;
}
- /* cache the config value */
- g->config = configs[i];
} /* for each config */
spin_unlock_irqrestore(&pctl->lock, flags);
@@ -518,6 +592,8 @@ static int sunxi_pconf_group_set(struct pinctrl_dev *pctldev,
}
static const struct pinconf_ops sunxi_pconf_ops = {
+ .is_generic = true,
+ .pin_config_get = sunxi_pconf_get,
.pin_config_group_get = sunxi_pconf_group_get,
.pin_config_group_set = sunxi_pconf_group_set,
};
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index 0afce1ab12d0..a7efb31d6523 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -109,7 +109,6 @@ struct sunxi_pinctrl_function {
struct sunxi_pinctrl_group {
const char *name;
- unsigned long config;
unsigned pin;
};
--
2.10.2
^ permalink raw reply related
* [PATCH v3 1/3] pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN, UP} argument
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111095036.11803-1-wens@csie.org>
According to pinconf-generic.h, the argument for
PIN_CONFIG_BIAS_PULL_{DOWN,UP} is non-zero if the bias is enabled
with a pull up/down resistor, zero if it is directly connected
to VDD or ground.
Since Allwinner hardware uses a weak pull resistor internally,
the argument should be 1.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index e199d95af8c0..e04edda8629d 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -291,12 +291,16 @@ static unsigned long *sunxi_pctrl_build_pin_config(struct device_node *node,
if (sunxi_pctrl_has_bias_prop(node)) {
int pull = sunxi_pctrl_parse_bias_prop(node);
+ int arg = 0;
if (pull < 0) {
ret = pull;
goto err_free;
}
- pinconfig[idx++] = pinconf_to_config_packed(pull, 0);
+ if (pull != PIN_CONFIG_BIAS_DISABLE)
+ arg = 1; /* hardware uses weak pull resistors */
+
+ pinconfig[idx++] = pinconf_to_config_packed(pull, arg);
}
--
2.10.2
^ permalink raw reply related
* [PATCH v3 0/3] pinctrl: sunxi: Support generic pinconf functions
From: Chen-Yu Tsai @ 2016-11-11 9:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi everyone,
This series fixes up generic pinconf support for the sunxi pinctrl driver
library. The driver was doing some bits wrong, like a) storing the pinconf
config value in its struct, and not actually reading the hardware to get
the current config, and b) not using the right arguments for the bias
parameters.
Patch 1 fixes the pin bias parameter arguments.
Patch 2 makes the driver read out pinconf settings from the hardware, and
returns the correct value for unsupported features and disable features.
With this in place it also declares itself as generic pinconf compatible,
which enables us to read the config through the debugfs pinconf interface.
Patch 3 makes the sunxi_pconf_group_set callback use the helper function
introduced in patch 1.
Changes since v2:
- Added Maxime's ack.
- Moved the removal of the cached config value from patch 3 to patch 2
at Maxime's request.
Changes since v1:
- Rebased onto the updated sunxi pinctrl driver with support for the
generic pinconf bindings
- Use separate value for what is written to the register in the pinconf
set function, as Maxime requested.
Regards
ChenYu
Chen-Yu Tsai (3):
pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN,UP} argument
pinctrl: sunxi: Add support for fetching pinconf settings from
hardware
pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
drivers/pinctrl/sunxi/pinctrl-sunxi.c | 156 +++++++++++++++++++++++++---------
drivers/pinctrl/sunxi/pinctrl-sunxi.h | 1 -
2 files changed, 118 insertions(+), 39 deletions(-)
--
2.10.2
^ permalink raw reply
* [PATCH v16 0/5] Mediatek MT8173 CMDQ support
From: Horng-Shyang Liao @ 2016-11-11 9:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABb+yY3mi3M0xqNao=DRYN6Np0LOBfS669iUa_hWo2w1MmM1sw@mail.gmail.com>
On Fri, 2016-11-11 at 11:15 +0530, Jassi Brar wrote:
> On Thu, Nov 10, 2016 at 4:45 PM, Horng-Shyang Liao <hs.liao@mediatek.com> wrote:
> > On Tue, 2016-11-01 at 19:28 +0800, HS Liao wrote:
> >> Hi,
> >>
> >> This is Mediatek MT8173 Command Queue(CMDQ) driver. The CMDQ is used
> >> to help write registers with critical time limitation, such as
> >> updating display configuration during the vblank. It controls Global
> >> Command Engine (GCE) hardware to achieve this requirement.
> >>
> >> These patches have a build dependency on top of v4.9-rc1.
> >>
> >> Changes since v15:
> >> - separate "suspend and resume" patch from "save energy" patch
> >> - don't stop running tasks in cmdq_suspend()
> >> (i.e. leave no running tasks guarantee to clients)
> >>
> >> Best regards,
> >> HS Liao
> >>
> >> HS Liao (5):
> >> dt-bindings: soc: Add documentation for the MediaTek GCE unit
> >> CMDQ: Mediatek CMDQ driver
> >> arm64: dts: mt8173: Add GCE node
> >> CMDQ: suspend and resume
> >> CMDQ: save energy
> >>
> >> .../devicetree/bindings/mailbox/mtk-gce.txt | 43 ++
> >> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 +
> >> drivers/mailbox/Kconfig | 10 +
> >> drivers/mailbox/Makefile | 2 +
> >> drivers/mailbox/mtk-cmdq-mailbox.c | 632 +++++++++++++++++++++
> >> drivers/soc/mediatek/Kconfig | 11 +
> >> drivers/soc/mediatek/Makefile | 1 +
> >> drivers/soc/mediatek/mtk-cmdq-helper.c | 310 ++++++++++
> >> include/linux/mailbox/mtk-cmdq-mailbox.h | 67 +++
> >> include/linux/soc/mediatek/mtk-cmdq.h | 182 ++++++
> >> 10 files changed, 1268 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/mtk-gce.txt
> >> create mode 100644 drivers/mailbox/mtk-cmdq-mailbox.c
> >> create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> >> create mode 100644 include/linux/mailbox/mtk-cmdq-mailbox.h
> >> create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> >>
> >
> >
> > Hi Jassi, Matthias,
> >
> > Sorry to disturb you.
> >
> No, you don't disturb, but the controller driver and protocol driver,
> introduced in the same patch, does :) So does the suspend/resume
> support (patch 4&5) added separately as a patch on top. Please
> reorganise the patchset.
>
> Thanks.
Hi Jassi,
Do you mean
1. split controller driver and protocol driver as two patches,
2. merge patch 4&5 into one patch, and
3. reorganize the patchset as "(1) binding doc (2) controller driver
(3) protocol driver (4) devicetree (5) energy patch" ?
Thanks,
HS
^ permalink raw reply
* [Linaro-acpi] ACPI namespace details for ARM64
From: Lorenzo Pieralisi @ 2016-11-11 9:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a0829be4-6324-758b-2fe6-a274f715a3a9@linaro.org>
On Thu, Nov 10, 2016 at 04:18:54PM -0700, Al Stone wrote:
> On 11/09/2016 03:05 PM, Bjorn Helgaas wrote:
> > Hi all,
> >
> > We've been working through the details of getting ACPI to work on
> > arm64, and there have been lots of questions about what this means for
> > PCI. I've outlined this for several people individually, but I'm
> > going to send this separately, apart from a specific patch series, to
> > make sure we're all on the same page. Please correct my errors and
> > misunderstandings.
> >
> > Bjorn
> >
> >[snip....]
>
> A big +1 to all of this. This also looks like something that should
> be added to either PCI, ACPI or arm64 documentation (or even all three).
And to arm64 platforms FW :)
> What do you think?
I do not think there is anything ARM64 specific in Bjorn's description,
but I do think it is very useful to have it in documentation, these
bits of information are scattered around ACPI specs and PCI FW specs,
having a single source would help and would have prevented asking
Bjorn the same questions 100 times.
> Thank you for putting this together, Bjorn.
+1, Thank you very much for this nice summary Bjorn.
Lorenzo
^ permalink raw reply
* [PATCH 5/5] drm/sun4i: Add support for the overscan profiles
From: Daniel Vetter @ 2016-11-11 9:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110145630.wvzlh6rxusrufv2r@lukather>
On Thu, Nov 10, 2016 at 03:56:30PM +0100, Maxime Ripard wrote:
> Hi Daniel,
>
> On Tue, Nov 08, 2016 at 09:59:27AM +0100, Daniel Vetter wrote:
> > On Tue, Oct 18, 2016 at 10:29:38AM +0200, Maxime Ripard wrote:
> > > Create overscan profiles reducing the displayed zone.
> > >
> > > For each TV standard (PAL and NTSC so far), we create 4 more reduced modes
> > > by steps of 5% that the user will be able to select.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >
> > tbh I think if we agree to do this (and that still seems an open question)
> > I think there should be a generic helper to add these overscan modes with
> > increased porches. Anything that only depends upon the sink (and
> > overscanning is something the sink does) should imo be put into a suitable
> > helper library for everyone to share.
> >
> > Or maybe even stash it into the probe helpers and call it for all TV
> > connectors. Definitely not a driver-private thing.
>
> Last time we discussed it, my recollection was that you didn't want to
> have generic code for it, but I'd be happy to implement it.
>
> I'll come up with something like that.
Well I can flip-flop around with the nonsense I'm sometimes emitting ;-)
Since you called me out, feel free to do whatever you want ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* [PATCH v2 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
From: Chen-Yu Tsai @ 2016-11-11 8:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111083852.yacy33g6pag6ucon@lukather>
On Fri, Nov 11, 2016 at 4:38 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Nov 11, 2016 at 10:44:55AM +0800, Chen-Yu Tsai wrote:
>> The sunxi_pconf_reg helper introduced in the last patch gives us the
>> chance to rework sunxi_pconf_group_set to have it match the structure
>> of sunxi_pconf_(group_)get and make it easier to understand.
>>
>> For each config to set, it:
>>
>> 1. checks if the parameter is supported.
>> 2. checks if the argument is within limits.
>> 3. converts argument to the register value.
>> 4. writes to the register with spinlock held.
>>
>> As a result the function now blocks unsupported config parameters,
>> instead of silently ignoring them.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> But I think the config variable removal should be part of patch 2, as
> discussed there.
OK. I think that makes sense. Re-reading my patches, I can't figure out,
which patch I meant for it to go in. :(
I'll send out a v3.
ChenYu
^ permalink raw reply
* [PATCH v2 2/6] mfd: stm32-adc: Add support for stm32 ADC
From: Lee Jones @ 2016-11-11 8:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201611110537.S7AVN9F6%fengguang.wu@intel.com>
On Fri, 11 Nov 2016, kbuild test robot wrote:
> Hi Fabrice,
>
> [auto build test ERROR on ljones-mfd/for-mfd-next]
> [also build test ERROR on v4.9-rc4 next-20161110]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Fabrice-Gasnier/Add-support-for-STM32-ADC/20161111-011922
> base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
> config: s390-allmodconfig (attached as .config)
> compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=s390
>
> All errors (new ones prefixed by >>):
>
> drivers/mfd/stm32-adc-core: struct of_device_id is 200 bytes. The last of 1 is:
> 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x73 0x74 0x2c 0x73 0x74 0x6d 0x33 0x32 0x66 0x34 0x2d 0x61 0x64 0x63 0x2d 0x63 0x6f 0x72 0x65 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> >> FATAL: drivers/mfd/stm32-adc-core: struct of_device_id is not terminated with a NULL entry!
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
Please fix this and re-submit.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v2 2/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware
From: Chen-Yu Tsai @ 2016-11-11 8:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111083627.yooctcxqs4ow7sn3@lukather>
On Fri, Nov 11, 2016 at 4:36 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Nov 11, 2016 at 10:44:54AM +0800, Chen-Yu Tsai wrote:
>> The sunxi pinctrl driver only caches whatever pinconf setting was last
>> set on a given pingroup. This is not particularly helpful, nor is it
>> correct.
>>
>> Fix this by actually reading the hardware registers and returning
>> the correct results or error codes. Also filter out unsupported
>> pinconf settings. Since this driver has a peculiar setup of 1 pin
>> per group, we can support both pin and pingroup pinconf setting
>> read back with the same code. The sunxi_pconf_reg helper and code
>> structure is inspired by pinctrl-msm.
>>
>> With this done we can also claim to support generic pinconf, by
>> setting .is_generic = true in pinconf_ops.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>> drivers/pinctrl/sunxi/pinctrl-sunxi.c | 84 +++++++++++++++++++++++++++++++++--
>> 1 file changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> index e04edda8629d..3e9f7c675d36 100644
>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>> @@ -438,15 +438,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
>> .get_group_pins = sunxi_pctrl_get_group_pins,
>> };
>>
>> +static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
>> + u32 *offset, u32 *shift, u32 *mask)
>> +{
>> + switch (param) {
>> + case PIN_CONFIG_DRIVE_STRENGTH:
>> + *offset = sunxi_dlevel_reg(pin);
>> + *shift = sunxi_dlevel_offset(pin);
>> + *mask = DLEVEL_PINS_MASK;
>> + break;
>> +
>> + case PIN_CONFIG_BIAS_PULL_UP:
>> + case PIN_CONFIG_BIAS_PULL_DOWN:
>> + case PIN_CONFIG_BIAS_DISABLE:
>> + *offset = sunxi_pull_reg(pin);
>> + *shift = sunxi_pull_offset(pin);
>> + *mask = PULL_PINS_MASK;
>> + break;
>> +
>> + default:
>> + return -ENOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
>> + unsigned long *config)
>> +{
>> + struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> + enum pin_config_param param = pinconf_to_config_param(*config);
>> + u32 offset, shift, mask, val;
>> + u16 arg;
>> + int ret;
>> +
>> + pin -= pctl->desc->pin_base;
>> +
>> + ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
>> + if (ret < 0)
>> + return ret;
>> +
>> + val = (readl(pctl->membase + offset) >> shift) & mask;
>> +
>> + switch (pinconf_to_config_param(*config)) {
>> + case PIN_CONFIG_DRIVE_STRENGTH:
>> + arg = (val + 1) * 10;
>> + break;
>> +
>> + case PIN_CONFIG_BIAS_PULL_UP:
>> + if (val != SUN4I_PINCTRL_PULL_UP)
>> + return -EINVAL;
>> + arg = 1; /* hardware is weak pull-up */
>> + break;
>> +
>> + case PIN_CONFIG_BIAS_PULL_DOWN:
>> + if (val != SUN4I_PINCTRL_PULL_DOWN)
>> + return -EINVAL;
>> + arg = 1; /* hardware is weak pull-down */
>> + break;
>> +
>> + case PIN_CONFIG_BIAS_DISABLE:
>> + if (val != SUN4I_PINCTRL_NO_PULL)
>> + return -EINVAL;
>> + arg = 0;
>> + break;
>> +
>> + default:
>> + /* sunxi_pconf_reg should catch anything unsupported */
>> + WARN_ON(1);
>> + return -ENOTSUPP;
>> + }
>> +
>> + *config = pinconf_to_config_packed(param, arg);
>> +
>> + return 0;
>> +}
>> +
>> static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
>> unsigned group,
>> unsigned long *config)
>> {
>> struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
>> + struct sunxi_pinctrl_group *g = &pctl->groups[group];
>>
>> - *config = pctl->groups[group].config;
>
> Do we still need this variable? Looking at the code, it doesn't look
> that way, and we can remove the caching in the _group_set function and
> the variable itslef in the sunxi_pincttrl_group structure.
It's actually removed in the next patch. :)
ChenYu
^ permalink raw reply
* [PATCH v2 3/3] pinctrl: sunxi: Make sunxi_pconf_group_set use sunxi_pconf_reg helper
From: Maxime Ripard @ 2016-11-11 8:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111024455.16883-4-wens@csie.org>
On Fri, Nov 11, 2016 at 10:44:55AM +0800, Chen-Yu Tsai wrote:
> The sunxi_pconf_reg helper introduced in the last patch gives us the
> chance to rework sunxi_pconf_group_set to have it match the structure
> of sunxi_pconf_(group_)get and make it easier to understand.
>
> For each config to set, it:
>
> 1. checks if the parameter is supported.
> 2. checks if the argument is within limits.
> 3. converts argument to the register value.
> 4. writes to the register with spinlock held.
>
> As a result the function now blocks unsupported config parameters,
> instead of silently ignoring them.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
But I think the config variable removal should be part of patch 2, as
discussed there.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161111/67b851bb/attachment.sig>
^ permalink raw reply
* [PATCH v2 2/3] pinctrl: sunxi: Add support for fetching pinconf settings from hardware
From: Maxime Ripard @ 2016-11-11 8:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111024455.16883-3-wens@csie.org>
On Fri, Nov 11, 2016 at 10:44:54AM +0800, Chen-Yu Tsai wrote:
> The sunxi pinctrl driver only caches whatever pinconf setting was last
> set on a given pingroup. This is not particularly helpful, nor is it
> correct.
>
> Fix this by actually reading the hardware registers and returning
> the correct results or error codes. Also filter out unsupported
> pinconf settings. Since this driver has a peculiar setup of 1 pin
> per group, we can support both pin and pingroup pinconf setting
> read back with the same code. The sunxi_pconf_reg helper and code
> structure is inspired by pinctrl-msm.
>
> With this done we can also claim to support generic pinconf, by
> setting .is_generic = true in pinconf_ops.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> drivers/pinctrl/sunxi/pinctrl-sunxi.c | 84 +++++++++++++++++++++++++++++++++--
> 1 file changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index e04edda8629d..3e9f7c675d36 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -438,15 +438,91 @@ static const struct pinctrl_ops sunxi_pctrl_ops = {
> .get_group_pins = sunxi_pctrl_get_group_pins,
> };
>
> +static int sunxi_pconf_reg(unsigned pin, enum pin_config_param param,
> + u32 *offset, u32 *shift, u32 *mask)
> +{
> + switch (param) {
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + *offset = sunxi_dlevel_reg(pin);
> + *shift = sunxi_dlevel_offset(pin);
> + *mask = DLEVEL_PINS_MASK;
> + break;
> +
> + case PIN_CONFIG_BIAS_PULL_UP:
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + case PIN_CONFIG_BIAS_DISABLE:
> + *offset = sunxi_pull_reg(pin);
> + *shift = sunxi_pull_offset(pin);
> + *mask = PULL_PINS_MASK;
> + break;
> +
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int sunxi_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
> + unsigned long *config)
> +{
> + struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> + enum pin_config_param param = pinconf_to_config_param(*config);
> + u32 offset, shift, mask, val;
> + u16 arg;
> + int ret;
> +
> + pin -= pctl->desc->pin_base;
> +
> + ret = sunxi_pconf_reg(pin, param, &offset, &shift, &mask);
> + if (ret < 0)
> + return ret;
> +
> + val = (readl(pctl->membase + offset) >> shift) & mask;
> +
> + switch (pinconf_to_config_param(*config)) {
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + arg = (val + 1) * 10;
> + break;
> +
> + case PIN_CONFIG_BIAS_PULL_UP:
> + if (val != SUN4I_PINCTRL_PULL_UP)
> + return -EINVAL;
> + arg = 1; /* hardware is weak pull-up */
> + break;
> +
> + case PIN_CONFIG_BIAS_PULL_DOWN:
> + if (val != SUN4I_PINCTRL_PULL_DOWN)
> + return -EINVAL;
> + arg = 1; /* hardware is weak pull-down */
> + break;
> +
> + case PIN_CONFIG_BIAS_DISABLE:
> + if (val != SUN4I_PINCTRL_NO_PULL)
> + return -EINVAL;
> + arg = 0;
> + break;
> +
> + default:
> + /* sunxi_pconf_reg should catch anything unsupported */
> + WARN_ON(1);
> + return -ENOTSUPP;
> + }
> +
> + *config = pinconf_to_config_packed(param, arg);
> +
> + return 0;
> +}
> +
> static int sunxi_pconf_group_get(struct pinctrl_dev *pctldev,
> unsigned group,
> unsigned long *config)
> {
> struct sunxi_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
> + struct sunxi_pinctrl_group *g = &pctl->groups[group];
>
> - *config = pctl->groups[group].config;
Do we still need this variable? Looking at the code, it doesn't look
that way, and we can remove the caching in the _group_set function and
the variable itslef in the sunxi_pincttrl_group structure.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161111/c66f592a/attachment-0001.sig>
^ permalink raw reply
* [PATCH v2 1/3] pinctrl: sunxi: Fix PIN_CONFIG_BIAS_PULL_{DOWN,UP} argument
From: Maxime Ripard @ 2016-11-11 8:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111024455.16883-2-wens@csie.org>
On Fri, Nov 11, 2016 at 10:44:53AM +0800, Chen-Yu Tsai wrote:
> According to pinconf-generic.h, the argument for
> PIN_CONFIG_BIAS_PULL_{DOWN,UP} is non-zero if the bias is enabled
> with a pull up/down resistor, zero if it is directly connected
> to VDD or ground.
>
> Since Allwinner hardware uses a weak pull resistor internally,
> the argument should be 1.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161111/bc2bfaf0/attachment.sig>
^ permalink raw reply
* [PATCH] pinctrl: sunxi: Free configs in pinctrl_map only if it is a config map
From: Maxime Ripard @ 2016-11-11 8:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111023510.14146-1-wens@csie.org>
On Fri, Nov 11, 2016 at 10:35:10AM +0800, Chen-Yu Tsai wrote:
> In the recently refactored sunxi pinctrl library, we are only allocating
> one set of pin configs for each pinmux setting node. When the pinctrl_map
> structure is freed, the pin configs should also be freed. However the
> code assumed the first map would contain the configs, which actually
> never happens, as the mux function map gets added first.
>
> The proper way to do this is to look through all the maps and free the
> first one whose type is actually PIN_MAP_TYPE_CONFIGS_GROUP.
>
> Also slightly expand the comment explaining this.
>
> Fixes: f233dbca6227 ("pinctrl: sunxi: Rework the pin config building code")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161111/e741e672/attachment.sig>
^ 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