[PATCH v2 00/12] J-core J2 cpu and SoC peripherals support

[PATCH v2 00/12] J-core J2 cpu and SoC peripherals support

[Rich Felker]

The following patchset adds support for the J-core J2, an open-source
VHDL reimplementation of the SH-2 ISA, and drivers for the associated
SoC devices (interrupt controller, clocksource, and SPI).

As arch/sh co-maintainer my intent is to have this merged for 4.7, but
I realized my previous post of the patch series omitted device tree
bindings and omitted Cc'ing of subsystem maintainers for the necessary
clocksource, irqchip, and spi drivers.

Rich Felker (12):
  of: add vendor prefix for J-Core
  of: add J-Core cpu bindings
  of: add J-Core interrupt controller bindings
  of: add J-Core timer bindings
  of: add J-Core SPI master bindings
  sh: add support for J-Core J2 processor
  sh: add AT_HWCAP flag for J-Core cas.l instruction
  irqchip: add J-Core AIC driver
  clocksource: add J-Core PIT/RTC driver
  spi: add driver for J-Core SPI controller
  sh: add defconfig for J-Core J2
  sh: add device tree source for J2 FPGA on Mimas v2 board

 .../bindings/interrupt-controller/jcore,aic.txt    |  28 +++
 Documentation/devicetree/bindings/jcore/cpus.txt   |  91 +++++++
 .../devicetree/bindings/spi/jcore,spi.txt          |  23 ++
 .../devicetree/bindings/timer/jcore,pit.txt        |  28 +++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 arch/sh/Kconfig                                    |   8 +
 arch/sh/Makefile                                   |   1 +
 arch/sh/boot/dts/j2_mimas_v2.dts                   |  87 +++++++
 arch/sh/configs/j2_defconfig                       |  38 +++
 arch/sh/include/asm/processor.h                    |   2 +-
 arch/sh/include/uapi/asm/cpu-features.h            |   1 +
 arch/sh/kernel/cpu/init.c                          |   2 +-
 arch/sh/kernel/cpu/proc.c                          |   1 +
 arch/sh/kernel/cpu/sh2/entry.S                     |   5 +
 arch/sh/kernel/cpu/sh2/probe.c                     |  36 ++-
 arch/sh/mm/Makefile                                |   3 +-
 arch/sh/mm/cache-j2.c                              |  58 +++++
 arch/sh/mm/cache.c                                 |   6 +-
 drivers/clocksource/Kconfig                        |   9 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/jcore-pit.c                    | 176 ++++++++++++++
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-jcore-aic.c                    |  95 ++++++++
 drivers/spi/Kconfig                                |   4 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-jcore.c                            | 266 +++++++++++++++++++++
 27 files changed, 973 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt
 create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
 create mode 100755 arch/sh/boot/dts/j2_mimas_v2.dts
 create mode 100644 arch/sh/configs/j2_defconfig
 create mode 100644 arch/sh/mm/cache-j2.c
 create mode 100644 drivers/clocksource/jcore-pit.c
 create mode 100644 drivers/irqchip/irq-jcore-aic.c
 create mode 100644 drivers/spi/spi-jcore.c

-- 
2.8.1

[PATCH v2 02/12] of: add J-Core cpu bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
new file mode 100644
index 0000000..00ef112
--- /dev/null
+++ b/Documentation/devicetree/bindings/jcore/cpus.txt
@@ -0,0 +1,91 @@
+===================
+J-Core cpu bindings
+===================
+
+The J-Core processors are open source CPU cores that can be built as FPGA
+soft cores or ASICs. The device tree is also responsible for describing the
+cache controls and, for SMP configurations, all details of the SMP method,
+as documented below.
+
+
+---------------------
+Top-level "cpus" node
+---------------------
+
+Required properties:
+
+- #address-cells: Must be 1.
+
+- #size-cells: Must be 0.
+
+Optional properties:
+
+- enable-method: Required only for SMP systems. If present, must be
+  "jcore,spin-table".
+
+
+--------------------
+Individual cpu nodes
+--------------------
+
+Required properties:
+
+- device_type: Must be "cpu".
+
+- compatible: Must be "jcore,j2".
+
+- reg: Must be 0 on uniprocessor systems, or the sequential, zero-based
+  hardware cpu id on SMP systems.
+
+Optional properties:
+
+- clock-frequency: Clock frequency of the cpu in Hz.
+
+- cpu-release-addr: Necessary only for secondary processors on SMP systems
+  using the "jcore,spin-table" enable method. If present, must consist of
+  two cells containing physical addresses. The first cell contains an
+  address which, when written, unblocks the secondary cpu. The second cell
+  contains an address from which the cpu will read its initial program
+  counter when unblocked.
+
+
+---------------------
+Cache controller node
+---------------------
+
+Required properties:
+
+- compatible: Must be "jcore,cache".
+
+- reg: A memory range for the cache controller registers.
+
+
+--------
+IPI node
+--------
+
+Device trees for SMP systems must have an IPI node representing the mechanism
+used for inter-processor interrupt generation.
+
+Required properties:
+
+- compatible: Must be "jcore,ipi-controller".
+
+- reg: A memory range used to IPI generation.
+
+- interrupts: An irq on which IPI will be received.
+
+
+----------
+CPUID node
+----------
+
+Device trees for SMP systems must have a CPUID node representing the mechanism
+used to identify the current processor on which execution is taking place.
+
+Required properties:
+
+- compatible: Must be "jcore,cpuid-mmio".
+
+- reg: A memory range containing a single 32-bit mmio register which produces
+  the current cpu id when read.
-- 
2.8.1

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rob Herring]

On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
> 
> diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
> new file mode 100644
> index 0000000..00ef112
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
> @@ -0,0 +1,91 @@
> +===================
> +J-Core cpu bindings
> +===================
> +
> +The J-Core processors are open source CPU cores that can be built as FPGA
> +soft cores or ASICs. The device tree is also responsible for describing the
> +cache controls and, for SMP configurations, all details of the SMP method,
> +as documented below.
> +
> +
> +---------------------
> +Top-level "cpus" node
> +---------------------
> +
> +Required properties:
> +
> +- #address-cells: Must be 1.
> +
> +- #size-cells: Must be 0.
> +
> +Optional properties:
> +
> +- enable-method: Required only for SMP systems. If present, must be
> +  "jcore,spin-table".
> +
> +
> +--------------------
> +Individual cpu nodes
> +--------------------
> +
> +Required properties:
> +
> +- device_type: Must be "cpu".
> +
> +- compatible: Must be "jcore,j2".

Okay to have this, but you should have compatible strings for specific 
core implementations. AIUI, J2 is just the ISA.

> +
> +- reg: Must be 0 on uniprocessor systems, or the sequential, zero-based
> +  hardware cpu id on SMP systems.
> +
> +Optional properties:
> +
> +- clock-frequency: Clock frequency of the cpu in Hz.
> +
> +- cpu-release-addr: Necessary only for secondary processors on SMP systems
> +  using the "jcore,spin-table" enable method. If present, must consist of
> +  two cells containing physical addresses. The first cell contains an
> +  address which, when written, unblocks the secondary cpu. The second cell
> +  contains an address from which the cpu will read its initial program
> +  counter when unblocked.
> +
> +
> +---------------------
> +Cache controller node
> +---------------------
> +
> +Required properties:
> +
> +- compatible: Must be "jcore,cache".

That's pretty generic...

> +
> +- reg: A memory range for the cache controller registers.

And standard cache properties? Are size, sets, ways, line size, etc. 
discoverable?

> +
> +
> +--------
> +IPI node
> +--------
> +
> +Device trees for SMP systems must have an IPI node representing the mechanism
> +used for inter-processor interrupt generation.
> +
> +Required properties:
> +
> +- compatible: Must be "jcore,ipi-controller".

Again, seems pretty generic.

> +
> +- reg: A memory range used to IPI generation.
> +
> +- interrupts: An irq on which IPI will be received.
> +
> +
> +----------
> +CPUID node
> +----------
> +
> +Device trees for SMP systems must have a CPUID node representing the mechanism
> +used to identify the current processor on which execution is taking place.
> +
> +Required properties:
> +
> +- compatible: Must be "jcore,cpuid-mmio".
> +
> +- reg: A memory range containing a single 32-bit mmio register which produces
> +  the current cpu id when read.

This id matches the reg value in cpu node, right? If not, it should.

Rob

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rich Felker]

On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
> > new file mode 100644
> > index 0000000..00ef112
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
> > @@ -0,0 +1,91 @@
> > +===================
> > +J-Core cpu bindings
> > +===================
> > +
> > +The J-Core processors are open source CPU cores that can be built as FPGA
> > +soft cores or ASICs. The device tree is also responsible for describing the
> > +cache controls and, for SMP configurations, all details of the SMP method,
> > +as documented below.
> > +
> > +
> > +---------------------
> > +Top-level "cpus" node
> > +---------------------
> > +
> > +Required properties:
> > +
> > +- #address-cells: Must be 1.
> > +
> > +- #size-cells: Must be 0.
> > +
> > +Optional properties:
> > +
> > +- enable-method: Required only for SMP systems. If present, must be
> > +  "jcore,spin-table".
> > +
> > +
> > +--------------------
> > +Individual cpu nodes
> > +--------------------
> > +
> > +Required properties:
> > +
> > +- device_type: Must be "cpu".
> > +
> > +- compatible: Must be "jcore,j2".
> 
> Okay to have this, but you should have compatible strings for specific 
> core implementations. AIUI, J2 is just the ISA.

There was some past discussion you probably missed on the linux-sh
list, starting here:

http://www.spinics.net/lists/linux-sh/msg50028.html

Basically it's really hard to identify what "the specific core
implementation" even means with a soft core. If you have some ideas
I'd be happy to hear them, but I think there should always be a
"jcore,j2" fallback compatible tag in any case.

FYI the way we're trying to use DT is to avoid hard-coding any
properties about the SoC (like mmio register addresses, cache
properties, etc.) in the cpu compatible tag, and instead breaking
things down into as many device nodes as possible, to allow variations
to be represented without having to encode them in the kernel/driver
sources.

> > +- reg: Must be 0 on uniprocessor systems, or the sequential, zero-based
> > +  hardware cpu id on SMP systems.
> > +
> > +Optional properties:
> > +
> > +- clock-frequency: Clock frequency of the cpu in Hz.
> > +
> > +- cpu-release-addr: Necessary only for secondary processors on SMP systems
> > +  using the "jcore,spin-table" enable method. If present, must consist of
> > +  two cells containing physical addresses. The first cell contains an
> > +  address which, when written, unblocks the secondary cpu. The second cell
> > +  contains an address from which the cpu will read its initial program
> > +  counter when unblocked.
> > +
> > +
> > +---------------------
> > +Cache controller node
> > +---------------------
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "jcore,cache".
> 
> That's pretty generic...

Most of the DT compatible tags were derived from the component
directory/source-file names in the J-Core source. In the case of
cache, it's actually called "icache" in the source for historical
reasons despite also including dcache, so I just used "cache" in the
DT.

> > +
> > +- reg: A memory range for the cache controller registers.
> 
> And standard cache properties? Are size, sets, ways, line size, etc. 
> discoverable?

I want to do the cache properties in a way that's generic for all of
arch/sh rather than specific to J2, but that's going to be part of the
DT conversion project. That's why I just have properties hard-coded
for now in the kernel source. They're not really used anyway except
for presenting them in /proc/cpuinfo.

> > +--------
> > +IPI node
> > +--------
> > +
> > +Device trees for SMP systems must have an IPI node representing the mechanism
> > +used for inter-processor interrupt generation.
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "jcore,ipi-controller".
> 
> Again, seems pretty generic.

I'm open to different ideas for naming schemes, but in the big scheme
of things it doesn't really matter. When there's a new incompatible
one, it can have a new compatible tag.

> > +----------
> > +CPUID node
> > +----------
> > +
> > +Device trees for SMP systems must have a CPUID node representing the mechanism
> > +used to identify the current processor on which execution is taking place.
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "jcore,cpuid-mmio".
> > +
> > +- reg: A memory range containing a single 32-bit mmio register which produces
> > +  the current cpu id when read.
> 
> This id matches the reg value in cpu node, right? If not, it should.

Yes. Should I document that here?

Rich

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rob Herring]

On Mon, May 23, 2016 at 4:03 PM, Rich Felker <dalias@libc.org> wrote:
> On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
>> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
>> > Signed-off-by: Rich Felker <dalias@libc.org>
>> > ---
>> >  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
>> >  1 file changed, 91 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
>> > new file mode 100644
>> > index 0000000..00ef112
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
>> > @@ -0,0 +1,91 @@
>> > +===================
>> > +J-Core cpu bindings
>> > +===================
>> > +
>> > +The J-Core processors are open source CPU cores that can be built as FPGA
>> > +soft cores or ASICs. The device tree is also responsible for describing the
>> > +cache controls and, for SMP configurations, all details of the SMP method,
>> > +as documented below.
>> > +
>> > +
>> > +---------------------
>> > +Top-level "cpus" node
>> > +---------------------
>> > +
>> > +Required properties:
>> > +
>> > +- #address-cells: Must be 1.
>> > +
>> > +- #size-cells: Must be 0.
>> > +
>> > +Optional properties:
>> > +
>> > +- enable-method: Required only for SMP systems. If present, must be
>> > +  "jcore,spin-table".
>> > +
>> > +
>> > +--------------------
>> > +Individual cpu nodes
>> > +--------------------
>> > +
>> > +Required properties:
>> > +
>> > +- device_type: Must be "cpu".
>> > +
>> > +- compatible: Must be "jcore,j2".
>>
>> Okay to have this, but you should have compatible strings for specific
>> core implementations. AIUI, J2 is just the ISA.
>
> There was some past discussion you probably missed on the linux-sh
> list, starting here:
>
> http://www.spinics.net/lists/linux-sh/msg50028.html
>
> Basically it's really hard to identify what "the specific core
> implementation" even means with a soft core. If you have some ideas
> I'd be happy to hear them, but I think there should always be a
> "jcore,j2" fallback compatible tag in any case.

Presumably you do some sort of versioning on the VHDL source that you
can correlate to.

If you have sufficient s/w accessible version registers that are
always going to be updated on IP changes then, you don't really need
more specific compatible strings.

> FYI the way we're trying to use DT is to avoid hard-coding any
> properties about the SoC (like mmio register addresses, cache
> properties, etc.) in the cpu compatible tag, and instead breaking
> things down into as many device nodes as possible, to allow variations
> to be represented without having to encode them in the kernel/driver
> sources.

Yes, that is the purpose of DT.

>> > +- reg: Must be 0 on uniprocessor systems, or the sequential, zero-based
>> > +  hardware cpu id on SMP systems.
>> > +
>> > +Optional properties:
>> > +
>> > +- clock-frequency: Clock frequency of the cpu in Hz.
>> > +
>> > +- cpu-release-addr: Necessary only for secondary processors on SMP systems
>> > +  using the "jcore,spin-table" enable method. If present, must consist of
>> > +  two cells containing physical addresses. The first cell contains an
>> > +  address which, when written, unblocks the secondary cpu. The second cell
>> > +  contains an address from which the cpu will read its initial program
>> > +  counter when unblocked.
>> > +
>> > +
>> > +---------------------
>> > +Cache controller node
>> > +---------------------
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: Must be "jcore,cache".
>>
>> That's pretty generic...
>
> Most of the DT compatible tags were derived from the component
> directory/source-file names in the J-Core source. In the case of
> cache, it's actually called "icache" in the source for historical
> reasons despite also including dcache, so I just used "cache" in the
> DT.
>
>> > +
>> > +- reg: A memory range for the cache controller registers.
>>
>> And standard cache properties? Are size, sets, ways, line size, etc.
>> discoverable?
>
> I want to do the cache properties in a way that's generic for all of
> arch/sh rather than specific to J2, but that's going to be part of the
> DT conversion project. That's why I just have properties hard-coded
> for now in the kernel source. They're not really used anyway except
> for presenting them in /proc/cpuinfo.

The DT spec (devicetree.org, formerly ePAPR) already defines arch
independent cache properties.

>> > +--------
>> > +IPI node
>> > +--------
>> > +
>> > +Device trees for SMP systems must have an IPI node representing the mechanism
>> > +used for inter-processor interrupt generation.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: Must be "jcore,ipi-controller".
>>
>> Again, seems pretty generic.
>
> I'm open to different ideas for naming schemes, but in the big scheme
> of things it doesn't really matter. When there's a new incompatible
> one, it can have a new compatible tag.

It should make sense according to how you version the VHDL sources.
The Xilinx folks have X.Y version numbers for example. We generally
don't accept those, but for soft IP that is an exception.

Better yet, since you can change "the hardware", make it more
discoverable with registers for version numbering and feature bits.
The failure here is having a process where that can be forgotten...

>> > +----------
>> > +CPUID node
>> > +----------
>> > +
>> > +Device trees for SMP systems must have a CPUID node representing the mechanism
>> > +used to identify the current processor on which execution is taking place.
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: Must be "jcore,cpuid-mmio".
>> > +
>> > +- reg: A memory range containing a single 32-bit mmio register which produces
>> > +  the current cpu id when read.
>>
>> This id matches the reg value in cpu node, right? If not, it should.
>
> Yes. Should I document that here?

Yes.

Rob

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rich Felker]

On Mon, May 23, 2016 at 06:29:48PM -0500, Rob Herring wrote:
> >> > +- compatible: Must be "jcore,j2".
> >>
> >> Okay to have this, but you should have compatible strings for specific
> >> core implementations. AIUI, J2 is just the ISA.
> >
> > There was some past discussion you probably missed on the linux-sh
> > list, starting here:
> >
> > http://www.spinics.net/lists/linux-sh/msg50028.html
> >
> > Basically it's really hard to identify what "the specific core
> > implementation" even means with a soft core. If you have some ideas
> > I'd be happy to hear them, but I think there should always be a
> > "jcore,j2" fallback compatible tag in any case.
> 
> Presumably you do some sort of versioning on the VHDL source that you
> can correlate to.

Yes, but I'm not convinced that helps with anything. Presumably the
point of having a fine-grained version in the compatible tag would be
to work around bugs. But if those bugs could come from build options,
specific version of the FPGA tools used, specific board targets, etc.
then a version really isn't sufficient to help you work around bugs.
On the other hand, as long as it's soft core, if there is a bug you
just fix the bitstream rather than putting workaround hacks in the
kernel or other software.

It does make sense to have some identifier for particular builds and
production runs once we have ASICs, of course, but that's a topic to
discuss when we get there.

I think most of this was already discussed in the thread I linked
above.

> >> > +- reg: A memory range for the cache controller registers.
> >>
> >> And standard cache properties? Are size, sets, ways, line size, etc.
> >> discoverable?
> >
> > I want to do the cache properties in a way that's generic for all of
> > arch/sh rather than specific to J2, but that's going to be part of the
> > DT conversion project. That's why I just have properties hard-coded
> > for now in the kernel source. They're not really used anyway except
> > for presenting them in /proc/cpuinfo.
> 
> The DT spec (devicetree.org, formerly ePAPR) already defines arch
> independent cache properties.

Indeed, but they don't seem to be comprehensive. In particular they
don't seem to represent the properties needed to compute aliasing for
VIPT cache architectures, or to represent whether the cache is
virtually or physically indexed or tagged.

> >> > +--------
> >> > +IPI node
> >> > +--------
> >> > +
> >> > +Device trees for SMP systems must have an IPI node representing the mechanism
> >> > +used for inter-processor interrupt generation.
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible: Must be "jcore,ipi-controller".
> >>
> >> Again, seems pretty generic.
> >
> > I'm open to different ideas for naming schemes, but in the big scheme
> > of things it doesn't really matter. When there's a new incompatible
> > one, it can have a new compatible tag.
> 
> It should make sense according to how you version the VHDL sources.
> The Xilinx folks have X.Y version numbers for example. We generally
> don't accept those, but for soft IP that is an exception.
> 
> Better yet, since you can change "the hardware", make it more
> discoverable with registers for version numbering and feature bits.
> The failure here is having a process where that can be forgotten...

I suspect that takes more space on the FPGA/ASIC than adding a few
bytes to the DTB in the boot rom would, so it's probably a worse
solution.

We could potentially allow compatible tags of the form "foo-v*" for
each "foo" documented now, where the version is from the jcore source
repo, and recommend (for example):

compatible = "jcore,foo-v1.23", "jcore,foo";

This would be separate from the numbers that are already present in
things like "spi2" and "spi3" (functionally distinct spi master
devices) or "aic1" and "aic2" (functionally distinct interrupt
controllers).

How would you want this to be documented, if we go that route?
Certainly all possible values cannot be specified in the binding docs
then.

> >> > +----------
> >> > +CPUID node
> >> > +----------
> >> > +
> >> > +Device trees for SMP systems must have a CPUID node representing the mechanism
> >> > +used to identify the current processor on which execution is taking place.
> >> > +
> >> > +Required properties:
> >> > +
> >> > +- compatible: Must be "jcore,cpuid-mmio".
> >> > +
> >> > +- reg: A memory range containing a single 32-bit mmio register which produces
> >> > +  the current cpu id when read.
> >>
> >> This id matches the reg value in cpu node, right? If not, it should.
> >
> > Yes. Should I document that here?
> 
> Yes.

OK, adding it to read:

- reg: A memory range containing a single 32-bit mmio register which produces
  the current cpu id (matching the "reg" property of the cpu performing the
  read) when read.

Rich

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rob Landley]

On 05/23/2016 06:29 PM, Rob Herring wrote:
> On Mon, May 23, 2016 at 4:03 PM, Rich Felker <dalias@libc.org> wrote:
>> On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
>>> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
>>>> Signed-off-by: Rich Felker <dalias@libc.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
>>>>  1 file changed, 91 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
>>>> new file mode 100644
>>>> index 0000000..00ef112
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
>>>> @@ -0,0 +1,91 @@
>>>> +===================
>>>> +J-Core cpu bindings
>>>> +===================
>>>> +
>>>> +The J-Core processors are open source CPU cores that can be built as FPGA
>>>> +soft cores or ASICs. The device tree is also responsible for describing the
>>>> +cache controls and, for SMP configurations, all details of the SMP method,
>>>> +as documented below.
>>>> +
>>>> +
>>>> +---------------------
>>>> +Top-level "cpus" node
>>>> +---------------------
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- #address-cells: Must be 1.
>>>> +
>>>> +- #size-cells: Must be 0.
>>>> +
>>>> +Optional properties:
>>>> +
>>>> +- enable-method: Required only for SMP systems. If present, must be
>>>> +  "jcore,spin-table".
>>>> +
>>>> +
>>>> +--------------------
>>>> +Individual cpu nodes
>>>> +--------------------
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- device_type: Must be "cpu".
>>>> +
>>>> +- compatible: Must be "jcore,j2".
>>>
>>> Okay to have this, but you should have compatible strings for specific
>>> core implementations. AIUI, J2 is just the ISA.
>>
>> There was some past discussion you probably missed on the linux-sh
>> list, starting here:
>>
>> http://www.spinics.net/lists/linux-sh/msg50028.html
>>
>> Basically it's really hard to identify what "the specific core
>> implementation" even means with a soft core. If you have some ideas
>> I'd be happy to hear them, but I think there should always be a
>> "jcore,j2" fallback compatible tag in any case.
> 
> Presumably you do some sort of versioning on the VHDL source that you
> can correlate to.
> 
> If you have sufficient s/w accessible version registers that are
> always going to be updated on IP changes then, you don't really need
> more specific compatible strings.

There are no version registers: the boot ROM can be output as part of
the build, and the dtb can be provided by the boot ROM. So you don't
need boot registers, you literally put any version info you need in the
dtb in the boot rom.

> Better yet, since you can change "the hardware", make it more
> discoverable with registers for version numbering and feature bits.
> The failure here is having a process where that can be forgotten...

Why would you add hardware version registers when the hardware's
attached boot rom is providing a dtb?

What's the point?

Rob

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rob Herring]

On Tue, May 24, 2016 at 4:30 PM, Rob Landley <rob@landley.net> wrote:
>
>
> On 05/23/2016 06:29 PM, Rob Herring wrote:
>> On Mon, May 23, 2016 at 4:03 PM, Rich Felker <dalias@libc.org> wrote:
>>> On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
>>>> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
>>>>> Signed-off-by: Rich Felker <dalias@libc.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
>>>>>  1 file changed, 91 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
>>>>> new file mode 100644
>>>>> index 0000000..00ef112
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
>>>>> @@ -0,0 +1,91 @@
>>>>> +===================
>>>>> +J-Core cpu bindings
>>>>> +===================
>>>>> +
>>>>> +The J-Core processors are open source CPU cores that can be built as FPGA
>>>>> +soft cores or ASICs. The device tree is also responsible for describing the
>>>>> +cache controls and, for SMP configurations, all details of the SMP method,
>>>>> +as documented below.
>>>>> +
>>>>> +
>>>>> +---------------------
>>>>> +Top-level "cpus" node
>>>>> +---------------------
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +- #address-cells: Must be 1.
>>>>> +
>>>>> +- #size-cells: Must be 0.
>>>>> +
>>>>> +Optional properties:
>>>>> +
>>>>> +- enable-method: Required only for SMP systems. If present, must be
>>>>> +  "jcore,spin-table".
>>>>> +
>>>>> +
>>>>> +--------------------
>>>>> +Individual cpu nodes
>>>>> +--------------------
>>>>> +
>>>>> +Required properties:
>>>>> +
>>>>> +- device_type: Must be "cpu".
>>>>> +
>>>>> +- compatible: Must be "jcore,j2".
>>>>
>>>> Okay to have this, but you should have compatible strings for specific
>>>> core implementations. AIUI, J2 is just the ISA.
>>>
>>> There was some past discussion you probably missed on the linux-sh
>>> list, starting here:
>>>
>>> http://www.spinics.net/lists/linux-sh/msg50028.html
>>>
>>> Basically it's really hard to identify what "the specific core
>>> implementation" even means with a soft core. If you have some ideas
>>> I'd be happy to hear them, but I think there should always be a
>>> "jcore,j2" fallback compatible tag in any case.
>>
>> Presumably you do some sort of versioning on the VHDL source that you
>> can correlate to.
>>
>> If you have sufficient s/w accessible version registers that are
>> always going to be updated on IP changes then, you don't really need
>> more specific compatible strings.
>
> There are no version registers: the boot ROM can be output as part of
> the build, and the dtb can be provided by the boot ROM. So you don't
> need boot registers, you literally put any version info you need in the
> dtb in the boot rom.

You can, but you are not doing that from the looks of it. Maybe you're
not to that point to need versioning and that's fine, but it doesn't
sound like you all have thought about it.

>> Better yet, since you can change "the hardware", make it more
>> discoverable with registers for version numbering and feature bits.
>> The failure here is having a process where that can be forgotten...
>
> Why would you add hardware version registers when the hardware's
> attached boot rom is providing a dtb?
>
> What's the point?

You are missing who is reading and caring about what the version is.
It's all the software that cares what's in either version registers or
dtb to know what are the specific features of the h/w. At some point
you will have a single driver that needs to support multiple versions
and/or configurations of hardware/IP.

Rob

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rich Felker]

On Tue, May 24, 2016 at 08:13:14PM -0500, Rob Herring wrote:
> On Tue, May 24, 2016 at 4:30 PM, Rob Landley <rob@landley.net> wrote:
> >
> >
> > On 05/23/2016 06:29 PM, Rob Herring wrote:
> >> On Mon, May 23, 2016 at 4:03 PM, Rich Felker <dalias@libc.org> wrote:
> >>> On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
> >>>> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
> >>>>> Signed-off-by: Rich Felker <dalias@libc.org>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
> >>>>>  1 file changed, 91 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..00ef112
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
> >>>>> @@ -0,0 +1,91 @@
> >>>>> +===================
> >>>>> +J-Core cpu bindings
> >>>>> +===================
> >>>>> +
> >>>>> +The J-Core processors are open source CPU cores that can be built as FPGA
> >>>>> +soft cores or ASICs. The device tree is also responsible for describing the
> >>>>> +cache controls and, for SMP configurations, all details of the SMP method,
> >>>>> +as documented below.
> >>>>> +
> >>>>> +
> >>>>> +---------------------
> >>>>> +Top-level "cpus" node
> >>>>> +---------------------
> >>>>> +
> >>>>> +Required properties:
> >>>>> +
> >>>>> +- #address-cells: Must be 1.
> >>>>> +
> >>>>> +- #size-cells: Must be 0.
> >>>>> +
> >>>>> +Optional properties:
> >>>>> +
> >>>>> +- enable-method: Required only for SMP systems. If present, must be
> >>>>> +  "jcore,spin-table".
> >>>>> +
> >>>>> +
> >>>>> +--------------------
> >>>>> +Individual cpu nodes
> >>>>> +--------------------
> >>>>> +
> >>>>> +Required properties:
> >>>>> +
> >>>>> +- device_type: Must be "cpu".
> >>>>> +
> >>>>> +- compatible: Must be "jcore,j2".
> >>>>
> >>>> Okay to have this, but you should have compatible strings for specific
> >>>> core implementations. AIUI, J2 is just the ISA.
> >>>
> >>> There was some past discussion you probably missed on the linux-sh
> >>> list, starting here:
> >>>
> >>> http://www.spinics.net/lists/linux-sh/msg50028.html
> >>>
> >>> Basically it's really hard to identify what "the specific core
> >>> implementation" even means with a soft core. If you have some ideas
> >>> I'd be happy to hear them, but I think there should always be a
> >>> "jcore,j2" fallback compatible tag in any case.
> >>
> >> Presumably you do some sort of versioning on the VHDL source that you
> >> can correlate to.
> >>
> >> If you have sufficient s/w accessible version registers that are
> >> always going to be updated on IP changes then, you don't really need
> >> more specific compatible strings.
> >
> > There are no version registers: the boot ROM can be output as part of
> > the build, and the dtb can be provided by the boot ROM. So you don't
> > need boot registers, you literally put any version info you need in the
> > dtb in the boot rom.
> 
> You can, but you are not doing that from the looks of it. Maybe you're
> not to that point to need versioning and that's fine, but it doesn't
> sound like you all have thought about it.

It's been thought about and discussed both on the linux-sh list and
internally in the J-Core development process, but it's certainly a
topic that could use more discussion. I don't think it should be a
blocking issue for registering current bindings, though.

> >> Better yet, since you can change "the hardware", make it more
> >> discoverable with registers for version numbering and feature bits.
> >> The failure here is having a process where that can be forgotten...
> >
> > Why would you add hardware version registers when the hardware's
> > attached boot rom is providing a dtb?
> >
> > What's the point?
> 
> You are missing who is reading and caring about what the version is.
> It's all the software that cares what's in either version registers or
> dtb to know what are the specific features of the h/w. At some point
> you will have a single driver that needs to support multiple versions
> and/or configurations of hardware/IP.

This is why we have both "jcore,aic1" and "jcore,aic2" now, and why
we'll soon have a "jcore,spi3" binding for the new SPI master with DMA
support. The intent is that stable hardware interfaces are maintained
at the hardware source level, and the binding names correspond to
component names in the hardware source.

If there are good reasons for more fine-grained version information,
we can add binding specifications for such, but the only reason I've
seen so far is bug workarounds, and it really doesn't make sense to be
putting bug workarounds in the kernel rather than just fixing the
source and flashing the FPGA configuration. Once we get to ASICs of
course it might make sense.

Do you have other compelling reasons for fine-grained versioning?

Rich

Re: [PATCH v2 02/12] of: add J-Core cpu bindings

[Rob Herring]

On Tue, May 24, 2016 at 9:33 PM, Rich Felker <dalias@libc.org> wrote:
> On Tue, May 24, 2016 at 08:13:14PM -0500, Rob Herring wrote:
>> On Tue, May 24, 2016 at 4:30 PM, Rob Landley <rob@landley.net> wrote:
>> >
>> >
>> > On 05/23/2016 06:29 PM, Rob Herring wrote:
>> >> On Mon, May 23, 2016 at 4:03 PM, Rich Felker <dalias@libc.org> wrote:
>> >>> On Mon, May 23, 2016 at 03:48:46PM -0500, Rob Herring wrote:
>> >>>> On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
>> >>>>> Signed-off-by: Rich Felker <dalias@libc.org>
>> >>>>> ---
>> >>>>>  Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
>> >>>>>  1 file changed, 91 insertions(+)
>> >>>>>  create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt
>> >>>>>
>> >>>>> diff --git a/Documentation/devicetree/bindings/jcore/cpus.txt b/Documentation/devicetree/bindings/jcore/cpus.txt
>> >>>>> new file mode 100644
>> >>>>> index 0000000..00ef112
>> >>>>> --- /dev/null
>> >>>>> +++ b/Documentation/devicetree/bindings/jcore/cpus.txt
>> >>>>> @@ -0,0 +1,91 @@
>> >>>>> +===================
>> >>>>> +J-Core cpu bindings
>> >>>>> +===================
>> >>>>> +
>> >>>>> +The J-Core processors are open source CPU cores that can be built as FPGA
>> >>>>> +soft cores or ASICs. The device tree is also responsible for describing the
>> >>>>> +cache controls and, for SMP configurations, all details of the SMP method,
>> >>>>> +as documented below.
>> >>>>> +
>> >>>>> +
>> >>>>> +---------------------
>> >>>>> +Top-level "cpus" node
>> >>>>> +---------------------
>> >>>>> +
>> >>>>> +Required properties:
>> >>>>> +
>> >>>>> +- #address-cells: Must be 1.
>> >>>>> +
>> >>>>> +- #size-cells: Must be 0.
>> >>>>> +
>> >>>>> +Optional properties:
>> >>>>> +
>> >>>>> +- enable-method: Required only for SMP systems. If present, must be
>> >>>>> +  "jcore,spin-table".
>> >>>>> +
>> >>>>> +
>> >>>>> +--------------------
>> >>>>> +Individual cpu nodes
>> >>>>> +--------------------
>> >>>>> +
>> >>>>> +Required properties:
>> >>>>> +
>> >>>>> +- device_type: Must be "cpu".
>> >>>>> +
>> >>>>> +- compatible: Must be "jcore,j2".
>> >>>>
>> >>>> Okay to have this, but you should have compatible strings for specific
>> >>>> core implementations. AIUI, J2 is just the ISA.
>> >>>
>> >>> There was some past discussion you probably missed on the linux-sh
>> >>> list, starting here:
>> >>>
>> >>> http://www.spinics.net/lists/linux-sh/msg50028.html
>> >>>
>> >>> Basically it's really hard to identify what "the specific core
>> >>> implementation" even means with a soft core. If you have some ideas
>> >>> I'd be happy to hear them, but I think there should always be a
>> >>> "jcore,j2" fallback compatible tag in any case.
>> >>
>> >> Presumably you do some sort of versioning on the VHDL source that you
>> >> can correlate to.
>> >>
>> >> If you have sufficient s/w accessible version registers that are
>> >> always going to be updated on IP changes then, you don't really need
>> >> more specific compatible strings.
>> >
>> > There are no version registers: the boot ROM can be output as part of
>> > the build, and the dtb can be provided by the boot ROM. So you don't
>> > need boot registers, you literally put any version info you need in the
>> > dtb in the boot rom.
>>
>> You can, but you are not doing that from the looks of it. Maybe you're
>> not to that point to need versioning and that's fine, but it doesn't
>> sound like you all have thought about it.
>
> It's been thought about and discussed both on the linux-sh list and
> internally in the J-Core development process, but it's certainly a
> topic that could use more discussion. I don't think it should be a
> blocking issue for registering current bindings, though.

It's not, I just want to understand the direction and that the need
here is understood.

>> >> Better yet, since you can change "the hardware", make it more
>> >> discoverable with registers for version numbering and feature bits.
>> >> The failure here is having a process where that can be forgotten...
>> >
>> > Why would you add hardware version registers when the hardware's
>> > attached boot rom is providing a dtb?
>> >
>> > What's the point?
>>
>> You are missing who is reading and caring about what the version is.
>> It's all the software that cares what's in either version registers or
>> dtb to know what are the specific features of the h/w. At some point
>> you will have a single driver that needs to support multiple versions
>> and/or configurations of hardware/IP.
>
> This is why we have both "jcore,aic1" and "jcore,aic2" now, and why
> we'll soon have a "jcore,spi3" binding for the new SPI master with DMA
> support. The intent is that stable hardware interfaces are maintained
> at the hardware source level, and the binding names correspond to
> component names in the hardware source.
>
> If there are good reasons for more fine-grained version information,
> we can add binding specifications for such, but the only reason I've
> seen so far is bug workarounds, and it really doesn't make sense to be
> putting bug workarounds in the kernel rather than just fixing the
> source and flashing the FPGA configuration. Once we get to ASICs of
> course it might make sense.
>
> Do you have other compelling reasons for fine-grained versioning?

Differences in features implied by compatible strings is the other
main reason. These would be features that are not configurable as
those you will probably want separate properties for. But if your
going to respin the VHDL to fix any issues, then what you have is
sufficient.

ASIC implementations should have compatible strings tied to the particular ASIC.

Rob

[PATCH v2 01/12] of: add vendor prefix for J-Core

[Rich Felker]

The J-Core project (j-core.org) produces open source cpu and SoC
peripheral cores synthesizable as FPGA bitstreams or ASICs.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 86740d4..9c070b8 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -126,6 +126,7 @@ invensense	InvenSense Inc.
 isee	ISEE 2007 S.L.
 isil	Intersil
 issi	Integrated Silicon Solutions Inc.
+jcore	J-Core.org
 jedec	JEDEC Solid State Technology Association
 karo	Ka-Ro electronics GmbH
 keymile	Keymile GmbH
-- 
2.8.1

Re: [PATCH v2 01/12] of: add vendor prefix for J-Core

[Rob Herring]

On Fri, May 20, 2016 at 02:53:03AM +0000, Rich Felker wrote:
> The J-Core project (j-core.org) produces open source cpu and SoC
> peripheral cores synthesizable as FPGA bitstreams or ASICs.
> 
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 08/12] irqchip: add J-Core AIC driver

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
My previous post of the patch series accidentally omitted omitted
Cc'ing of subsystem maintainers for the necessary clocksource,
irqchip, and spi drivers. Please ack if this looks ok because I want
to get it merged as part of the arch/sh pull request for 4.7.

 drivers/irqchip/Kconfig         |  6 +++
 drivers/irqchip/Makefile        |  1 +
 drivers/irqchip/irq-jcore-aic.c | 95 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 drivers/irqchip/irq-jcore-aic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e12479..3cb37d6 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -149,6 +149,12 @@ config PIC32_EVIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config JCORE_AIC
+	bool "J-Core integrated AIC"
+	select IRQ_DOMAIN
+	help
+	  Support for the J-Core integrated AIC.
+
 config RENESAS_INTC_IRQPIN
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b03cfcb..5a1f1bf 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_I8259)			+= irq-i8259.o
 obj-$(CONFIG_IMGPDC_IRQ)		+= irq-imgpdc.o
 obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
 obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
+obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
new file mode 100644
index 0000000..68178fb
--- /dev/null
+++ b/drivers/irqchip/irq-jcore-aic.c
@@ -0,0 +1,95 @@
+/*
+ * J-Core SoC AIC driver
+ *
+ * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/cpu.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define AIC1_INTPRI 8
+
+struct aic_data {
+	unsigned char __iomem *base;
+	u32 cpu_offset;
+	struct irq_chip chip;
+	struct irq_domain *domain;
+	struct notifier_block nb;
+} aic_data;
+
+static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
+{
+	struct aic_data *aic = d->host_data;
+
+	irq_set_chip_data(irq, aic);
+	irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aic_irqdomain_ops = {
+	.map = aic_irqdomain_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static void noop(struct irq_data *data)
+{
+}
+
+static void aic1_localenable(struct aic_data *aic)
+{
+	unsigned cpu = smp_processor_id();
+	pr_info("Local AIC enable on cpu %u\n", cpu);
+	writel(0xffffffff, aic->base + cpu * aic->cpu_offset + AIC1_INTPRI);
+}
+
+static int aic1_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
+{
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		aic1_localenable(container_of(self, struct aic_data, nb));
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
+{
+	struct aic_data *aic = &aic_data;
+
+	aic->base = of_iomap(node, 0);
+	of_property_read_u32(node, "cpu-offset", &aic->cpu_offset);
+
+	pr_info("Initializing J-Core AIC at %p\n", aic->base);
+
+	if (of_device_is_compatible(node, "jcore,aic1")) {
+		/* For aic1, need to enabled zero-priority-by-default irqs */
+		aic->nb.notifier_call = aic1_cpu_notify;
+		register_cpu_notifier(&aic->nb);
+		aic1_localenable(aic);
+	}
+
+	aic->chip.name = node->name;
+	aic->chip.irq_mask = noop;
+	aic->chip.irq_unmask = noop;
+
+	aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic);
+	irq_create_strict_mappings(aic->domain, 16, 16, 112);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init);
+IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
-- 
2.8.1

Re: [PATCH v2 08/12] irqchip: add J-Core AIC driver

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> --- /dev/null
> +++ b/drivers/irqchip/irq-jcore-aic.c
> @@ -0,0 +1,95 @@

> +struct aic_data {

static?

> +       unsigned char __iomem *base;
> +       u32 cpu_offset;
> +       struct irq_chip chip;
> +       struct irq_domain *domain;
> +       struct notifier_block nb;
> +} aic_data;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 08/12] irqchip: add J-Core AIC driver

[Marc Zyngier]

On 20/05/16 03:53, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.

For a start, a decent commit message wouldn't hurt.

> 
>  drivers/irqchip/Kconfig         |  6 +++
>  drivers/irqchip/Makefile        |  1 +
>  drivers/irqchip/irq-jcore-aic.c | 95 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/irqchip/irq-jcore-aic.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 3e12479..3cb37d6 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -149,6 +149,12 @@ config PIC32_EVIC
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN
>  
> +config JCORE_AIC
> +	bool "J-Core integrated AIC"
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the J-Core integrated AIC.
> +
>  config RENESAS_INTC_IRQPIN
>  	bool
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b03cfcb..5a1f1bf 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_I8259)			+= irq-i8259.o
>  obj-$(CONFIG_IMGPDC_IRQ)		+= irq-imgpdc.o
>  obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
>  obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
> +obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
> new file mode 100644
> index 0000000..68178fb
> --- /dev/null
> +++ b/drivers/irqchip/irq-jcore-aic.c
> @@ -0,0 +1,95 @@
> +/*
> + * J-Core SoC AIC driver
> + *
> + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/cpu.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define AIC1_INTPRI 8
> +
> +struct aic_data {
> +	unsigned char __iomem *base;
> +	u32 cpu_offset;
> +	struct irq_chip chip;
> +	struct irq_domain *domain;
> +	struct notifier_block nb;
> +} aic_data;
> +
> +static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	struct aic_data *aic = d->host_data;
> +
> +	irq_set_chip_data(irq, aic);
> +	irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq);
> +	irq_set_probe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops aic_irqdomain_ops = {
> +	.map = aic_irqdomain_map,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static void noop(struct irq_data *data)
> +{
> +}
> +
> +static void aic1_localenable(struct aic_data *aic)
> +{
> +	unsigned cpu = smp_processor_id();
> +	pr_info("Local AIC enable on cpu %u\n", cpu);
> +	writel(0xffffffff, aic->base + cpu * aic->cpu_offset + AIC1_INTPRI);
> +}
> +
> +static int aic1_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> +{
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_STARTING:
> +		aic1_localenable(container_of(self, struct aic_data, nb));
> +		break;
> +	}

And nothing happens when the CPU goes down?

> +	return NOTIFY_OK;
> +}
> +
> +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct aic_data *aic = &aic_data;
> +
> +	aic->base = of_iomap(node, 0);
> +	of_property_read_u32(node, "cpu-offset", &aic->cpu_offset);
> +
> +	pr_info("Initializing J-Core AIC at %p\n", aic->base);
> +
> +	if (of_device_is_compatible(node, "jcore,aic1")) {
> +		/* For aic1, need to enabled zero-priority-by-default irqs */
> +		aic->nb.notifier_call = aic1_cpu_notify;
> +		register_cpu_notifier(&aic->nb);
> +		aic1_localenable(aic);
> +	}
> +
> +	aic->chip.name = node->name;
> +	aic->chip.irq_mask = noop;
> +	aic->chip.irq_unmask = noop;

So this driver is doing exactly nothing. Not even an EOI. How does it
work? How is this driver involved in the interrupt flow?

> +
> +	aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic);

The DT binding says that aic1 has 8 interrupts, and aic2 has 64. Why are
you allocating 128 of them?

> +	irq_create_strict_mappings(aic->domain, 16, 16, 112);

What are the first 16 interrupts for? By the look of it, this is a
legacy domain in disguise.

> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init);
> +IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
> 

To be honest, this doesn't look like an irqchip driver. More like a
glorified probe function. Maybe this is a property of the architecture,
but I'd really like at least a comment explaining this.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 08/12] irqchip: add J-Core AIC driver

[Rich Felker]

On Fri, May 20, 2016 at 09:15:56AM +0100, Marc Zyngier wrote:
> On 20/05/16 03:53, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> > My previous post of the patch series accidentally omitted omitted
> > Cc'ing of subsystem maintainers for the necessary clocksource,
> > irqchip, and spi drivers. Please ack if this looks ok because I want
> > to get it merged as part of the arch/sh pull request for 4.7.
> 
> For a start, a decent commit message wouldn't hurt.

Adding it.

> >  drivers/irqchip/Kconfig         |  6 +++
> >  drivers/irqchip/Makefile        |  1 +
> >  drivers/irqchip/irq-jcore-aic.c | 95 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 102 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-jcore-aic.c
> > 
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 3e12479..3cb37d6 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -149,6 +149,12 @@ config PIC32_EVIC
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> >  
> > +config JCORE_AIC
> > +	bool "J-Core integrated AIC"
> > +	select IRQ_DOMAIN
> > +	help
> > +	  Support for the J-Core integrated AIC.
> > +
> >  config RENESAS_INTC_IRQPIN
> >  	bool
> >  	select IRQ_DOMAIN
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index b03cfcb..5a1f1bf 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -37,6 +37,7 @@ obj-$(CONFIG_I8259)			+= irq-i8259.o
> >  obj-$(CONFIG_IMGPDC_IRQ)		+= irq-imgpdc.o
> >  obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
> >  obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
> > +obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
> >  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
> >  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
> >  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
> > diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
> > new file mode 100644
> > index 0000000..68178fb
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-jcore-aic.c
> > @@ -0,0 +1,95 @@
> > +/*
> > + * J-Core SoC AIC driver
> > + *
> > + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/io.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/module.h>
> > +#include <linux/cpu.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +
> > +#define AIC1_INTPRI 8
> > +
> > +struct aic_data {
> > +	unsigned char __iomem *base;
> > +	u32 cpu_offset;
> > +	struct irq_chip chip;
> > +	struct irq_domain *domain;
> > +	struct notifier_block nb;
> > +} aic_data;
> > +
> > +static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
> > +{
> > +	struct aic_data *aic = d->host_data;
> > +
> > +	irq_set_chip_data(irq, aic);
> > +	irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq);
> > +	irq_set_probe(irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops aic_irqdomain_ops = {
> > +	.map = aic_irqdomain_map,
> > +	.xlate = irq_domain_xlate_onecell,
> > +};
> > +
> > +static void noop(struct irq_data *data)
> > +{
> > +}
> > +
> > +static void aic1_localenable(struct aic_data *aic)
> > +{
> > +	unsigned cpu = smp_processor_id();
> > +	pr_info("Local AIC enable on cpu %u\n", cpu);
> > +	writel(0xffffffff, aic->base + cpu * aic->cpu_offset + AIC1_INTPRI);
> > +}
> > +
> > +static int aic1_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> > +{
> > +	switch (action & ~CPU_TASKS_FROZEN) {
> > +	case CPU_STARTING:
> > +		aic1_localenable(container_of(self, struct aic_data, nb));
> > +		break;
> > +	}
> 
> And nothing happens when the CPU goes down?

There is no support for bringing down CPUs (SMP support isn't even
going upstream yet in this patch series, but I didn't want to
gratuitously rip the SMP support out of the drivers only to add it
back later). If/when that's added it will have to be determined at
that point whether any action is required.

> > +	return NOTIFY_OK;
> > +}
> > +
> > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	struct aic_data *aic = &aic_data;
> > +
> > +	aic->base = of_iomap(node, 0);
> > +	of_property_read_u32(node, "cpu-offset", &aic->cpu_offset);
> > +
> > +	pr_info("Initializing J-Core AIC at %p\n", aic->base);
> > +
> > +	if (of_device_is_compatible(node, "jcore,aic1")) {
> > +		/* For aic1, need to enabled zero-priority-by-default irqs */
> > +		aic->nb.notifier_call = aic1_cpu_notify;
> > +		register_cpu_notifier(&aic->nb);
> > +		aic1_localenable(aic);
> > +	}
> > +
> > +	aic->chip.name = node->name;
> > +	aic->chip.irq_mask = noop;
> > +	aic->chip.irq_unmask = noop;
> 
> So this driver is doing exactly nothing. Not even an EOI. How does it
> work? How is this driver involved in the interrupt flow?

For aic1, the driver is setting non-zero priorities, without which no
interrupts will ever arrive. For aic2, this is not needed. Aside from
that, the driver is providing an irq domain, without which nothing in
the DT could get an irq.

Right now the whole arch/sh irq framework lacks a good abstraction for
arbitrary interrupt controllers. By default the cpu trap number is
taken as the interrupt number and passed into generic_handle_irq;
legacy board files can hook this, but there's no way yet to do that
for systems described by DT.

When that's improved (a long project because it involves getting
legacy hardware, testing on it, and converting over support for legacy
hardware to use new frameworks) the AIC driver will do something to
register a top-level interrupt handling function, and hopefully by
then the conventions for such registration will no longer be
arch-specific.

> > +
> > +	aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic);
> 
> The DT binding says that aic1 has 8 interrupts, and aic2 has 64. Why are
> you allocating 128 of them?

Because the 64 are 64-127, while the 8 are 17-24, which are really 8+1
because the timer interrupt can be programmed to any cpu trap number,
not necessarily in any restricted range. The irq domain has to map
these numbers which will appear in the DT. I mapped 16-127 because
trap numbers lower than 16 are reserved for exceptions. Some of that
range is also reserved for syscalls and not usable; I can omit those
if desirable.

> > +	irq_create_strict_mappings(aic->domain, 16, 16, 112);
> 
> What are the first 16 interrupts for? By the look of it, this is a
> legacy domain in disguise.

The first 16 trap numbers (and others) are reserved for non-interrupt
use. As far as I know there's no compelling reason there needs to be a
one-to-one mapping between these trap numbers and virtual irq numbers,
and I can probably change that if you really like, but it doesn't seem
to hurt.

> > +
> > +	return 0;
> > +}
> > +
> > +IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init);
> > +IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
> 
> To be honest, this doesn't look like an irqchip driver. More like a
> glorified probe function. Maybe this is a property of the architecture,
> but I'd really like at least a comment explaining this.

Yes, I think that's a correct assessment. I have a v3 series I'm
posting now but I can add comments following that.

Rich

[PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
new file mode 100644
index 0000000..dc9fde8
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
@@ -0,0 +1,28 @@
+J-Core Advanced Interrupt Controller
+
+Required properties:
+
+- compatible : Should be "jcore,aic1" for the (obsolete) first-generation aic
+  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
+  the "aic2" core with 64 interrupts.
+
+- interrupt-controller : Identifies the node as an interrupt controller
+
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+
+Additional properties required for aic1:
+
+- reg : Memory region for configuration.
+
+- cpu-offset : For SMP, the offset to the per-cpu memory region for
+  configuration, to be scaled by the cpu number.
+
+
+Example:
+
+aic: interrupt-controller {
+	compatible = "jcore,aic2";
+	interrupt-controller;
+	#interrupt-cells = <1>;
+};
-- 
2.8.1

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> +Additional properties required for aic1:
> +
> +- reg : Memory region for configuration.
> +
> +- cpu-offset : For SMP, the offset to the per-cpu memory region for
> +  configuration, to be scaled by the cpu number.

Does cpu-offset apply to aic1 only?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rich Felker]

On Fri, May 20, 2016 at 10:04:26AM +0200, Geert Uytterhoeven wrote:
> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> > +Additional properties required for aic1:
> > +
> > +- reg : Memory region for configuration.
> > +
> > +- cpu-offset : For SMP, the offset to the per-cpu memory region for
> > +  configuration, to be scaled by the cpu number.
> 
> Does cpu-offset apply to aic1 only?

The current kernel driver doesn't have any reason to _need_ cpu-offset
for aic2, but since there are registers there that a driver (even a
non-Linux one) may want to use, I think it makes sense that it should
be present in the bindings.

Rich

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Geert Uytterhoeven]

On Sat, May 21, 2016 at 12:34 AM, Rich Felker <dalias@libc.org> wrote:
> On Fri, May 20, 2016 at 10:04:26AM +0200, Geert Uytterhoeven wrote:
>> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
>> > +Additional properties required for aic1:
>> > +
>> > +- reg : Memory region for configuration.
>> > +
>> > +- cpu-offset : For SMP, the offset to the per-cpu memory region for
>> > +  configuration, to be scaled by the cpu number.
>>
>> Does cpu-offset apply to aic1 only?
>
> The current kernel driver doesn't have any reason to _need_ cpu-offset
> for aic2, but since there are registers there that a driver (even a
> non-Linux one) may want to use, I think it makes sense that it should
> be present in the bindings.

Hence the "for aic1" should be dropped?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rich Felker]

On Sat, May 21, 2016 at 08:07:54PM +0200, Geert Uytterhoeven wrote:
> On Sat, May 21, 2016 at 12:34 AM, Rich Felker <dalias@libc.org> wrote:
> > On Fri, May 20, 2016 at 10:04:26AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> >> > +Additional properties required for aic1:
> >> > +
> >> > +- reg : Memory region for configuration.
> >> > +
> >> > +- cpu-offset : For SMP, the offset to the per-cpu memory region for
> >> > +  configuration, to be scaled by the cpu number.
> >>
> >> Does cpu-offset apply to aic1 only?
> >
> > The current kernel driver doesn't have any reason to _need_ cpu-offset
> > for aic2, but since there are registers there that a driver (even a
> > non-Linux one) may want to use, I think it makes sense that it should
> > be present in the bindings.
> 
> Hence the "for aic1" should be dropped?

Yes, I've fixed that locally. Moved reg to required and cpu-offset
to optional but needed for smp.

Rich

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rob Herring]

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> new file mode 100644
> index 0000000..dc9fde8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> @@ -0,0 +1,28 @@
> +J-Core Advanced Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : Should be "jcore,aic1" for the (obsolete) first-generation aic
> +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
> +  the "aic2" core with 64 interrupts.
> +
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.

No level/edge support? Need 2 cells if so.

> +
> +Additional properties required for aic1:
> +
> +- reg : Memory region for configuration.
> +
> +- cpu-offset : For SMP, the offset to the per-cpu memory region for
> +  configuration, to be scaled by the cpu number.
> +
> +
> +Example:
> +
> +aic: interrupt-controller {
> +	compatible = "jcore,aic2";
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +};
> -- 
> 2.8.1
> 
> 

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rich Felker]

On Mon, May 23, 2016 at 03:53:20PM -0500, Rob Herring wrote:
> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > new file mode 100644
> > index 0000000..dc9fde8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > @@ -0,0 +1,28 @@
> > +J-Core Advanced Interrupt Controller
> > +
> > +Required properties:
> > +
> > +- compatible : Should be "jcore,aic1" for the (obsolete) first-generation aic
> > +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
> > +  the "aic2" core with 64 interrupts.
> > +
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +
> > +- #interrupt-cells : Specifies the number of cells needed to encode an
> > +  interrupt source. The value shall be 1.
> 
> No level/edge support? Need 2 cells if so.

No, all the logic is in hardware. From the software side you just need
handle_simple_irq or equivalent.

Rich

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Marc Zyngier]

On 23/05/16 22:13, Rich Felker wrote:
> On Mon, May 23, 2016 at 03:53:20PM -0500, Rob Herring wrote:
>> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
>>> Signed-off-by: Rich Felker <dalias@libc.org>
>>> ---
>>>  .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
>>> new file mode 100644
>>> index 0000000..dc9fde8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
>>> @@ -0,0 +1,28 @@
>>> +J-Core Advanced Interrupt Controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible : Should be "jcore,aic1" for the (obsolete) first-generation aic
>>> +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
>>> +  the "aic2" core with 64 interrupts.
>>> +
>>> +- interrupt-controller : Identifies the node as an interrupt controller
>>> +
>>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>>> +  interrupt source. The value shall be 1.
>>
>> No level/edge support? Need 2 cells if so.
> 
> No, all the logic is in hardware. From the software side you just need
> handle_simple_irq or equivalent.

Not even an EOI?

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 03/12] of: add J-Core interrupt controller bindings

[Rich Felker]

On Tue, May 24, 2016 at 09:09:41AM +0100, Marc Zyngier wrote:
> On 23/05/16 22:13, Rich Felker wrote:
> > On Mon, May 23, 2016 at 03:53:20PM -0500, Rob Herring wrote:
> >> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> >>> Signed-off-by: Rich Felker <dalias@libc.org>
> >>> ---
> >>>  .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
> >>>  1 file changed, 28 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> >>> new file mode 100644
> >>> index 0000000..dc9fde8
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> >>> @@ -0,0 +1,28 @@
> >>> +J-Core Advanced Interrupt Controller
> >>> +
> >>> +Required properties:
> >>> +
> >>> +- compatible : Should be "jcore,aic1" for the (obsolete) first-generation aic
> >>> +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
> >>> +  the "aic2" core with 64 interrupts.
> >>> +
> >>> +- interrupt-controller : Identifies the node as an interrupt controller
> >>> +
> >>> +- #interrupt-cells : Specifies the number of cells needed to encode an
> >>> +  interrupt source. The value shall be 1.
> >>
> >> No level/edge support? Need 2 cells if so.
> > 
> > No, all the logic is in hardware. From the software side you just need
> > handle_simple_irq or equivalent.
> 
> Not even an EOI?

What I mean is that there is no ack/eoi interface. While I haven't
worked directly on the relevant vhdl, my understanding is that the aic
clears the pending status of an interrupt atomically with acceptance
of the interrupt by the cpu.

Do you have any more specific questions I can try to answer?

Rich

[PATCH v2 06/12] sh: add support for J-Core J2 processor

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
This revision of the patch is less invasive than the previous one,
moving J2-specific cache init from  arch/sh/kernel/cpu/init.c to
arch/sh/kernel/cpu/sh2/probe.c. Unnecessary variable cpu-offset for
CCR register has also been removed.

 arch/sh/Kconfig                 |  8 ++++++
 arch/sh/Makefile                |  1 +
 arch/sh/include/asm/processor.h |  2 +-
 arch/sh/kernel/cpu/init.c       |  2 +-
 arch/sh/kernel/cpu/proc.c       |  1 +
 arch/sh/kernel/cpu/sh2/entry.S  |  5 ++++
 arch/sh/kernel/cpu/sh2/probe.c  | 34 +++++++++++++++++++++++-
 arch/sh/mm/Makefile             |  3 ++-
 arch/sh/mm/cache-j2.c           | 58 +++++++++++++++++++++++++++++++++++++++++
 arch/sh/mm/cache.c              |  6 ++++-
 10 files changed, 115 insertions(+), 5 deletions(-)
 create mode 100644 arch/sh/mm/cache-j2.c

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 4fa5894..efb0af4 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -182,6 +182,10 @@ config CPU_SH2A
 	select CPU_SH2
 	select UNCACHED_MAPPING
 
+config CPU_J2
+	bool
+	select CPU_SH2
+
 config CPU_SH3
 	bool
 	select CPU_HAS_INTEVT
@@ -248,6 +252,10 @@ config CPU_SUBTYPE_SH7619
 	select CPU_SH2
 	select SYS_SUPPORTS_SH_CMT
 
+config CPU_SUBTYPE_J2
+	bool "Support J2 processor"
+	select CPU_J2
+
 # SH-2A Processor Support
 
 config CPU_SUBTYPE_SH7201
diff --git a/arch/sh/Makefile b/arch/sh/Makefile
index 3b2c8b4..0047666 100644
--- a/arch/sh/Makefile
+++ b/arch/sh/Makefile
@@ -31,6 +31,7 @@ isa-y					:= $(isa-y)-up
 endif
 
 cflags-$(CONFIG_CPU_SH2)		:= $(call cc-option,-m2,)
+cflags-$(CONFIG_CPU_J2)			:= $(call cc-option,-mj2,)
 cflags-$(CONFIG_CPU_SH2A)		+= $(call cc-option,-m2a,) \
 					   $(call cc-option,-m2a-nofpu,) \
 					   $(call cc-option,-m4-nofpu,)
diff --git a/arch/sh/include/asm/processor.h b/arch/sh/include/asm/processor.h
index 1506897..f9a0994 100644
--- a/arch/sh/include/asm/processor.h
+++ b/arch/sh/include/asm/processor.h
@@ -15,7 +15,7 @@
  */
 enum cpu_type {
 	/* SH-2 types */
-	CPU_SH7619,
+	CPU_SH7619, CPU_J2,
 
 	/* SH-2A types */
 	CPU_SH7201, CPU_SH7203, CPU_SH7206, CPU_SH7263, CPU_SH7264, CPU_SH7269,
diff --git a/arch/sh/kernel/cpu/init.c b/arch/sh/kernel/cpu/init.c
index bfd9e27..c8b3be1 100644
--- a/arch/sh/kernel/cpu/init.c
+++ b/arch/sh/kernel/cpu/init.c
@@ -106,7 +106,7 @@ void __attribute__ ((weak)) l2_cache_init(void)
 /*
  * Generic first-level cache init
  */
-#ifdef CONFIG_SUPERH32
+#if defined(CONFIG_SUPERH32) && !defined(CONFIG_CPU_J2)
 static void cache_init(void)
 {
 	unsigned long ccr, flags;
diff --git a/arch/sh/kernel/cpu/proc.c b/arch/sh/kernel/cpu/proc.c
index 9e6624c..4df4b28 100644
--- a/arch/sh/kernel/cpu/proc.c
+++ b/arch/sh/kernel/cpu/proc.c
@@ -27,6 +27,7 @@ static const char *cpu_name[] = {
 	[CPU_MXG]	= "MX-G",	[CPU_SH7723]	= "SH7723",
 	[CPU_SH7366]	= "SH7366",	[CPU_SH7724]	= "SH7724",
 	[CPU_SH7372]	= "SH7372",	[CPU_SH7734]	= "SH7734",
+	[CPU_J2]	= "J2",
 	[CPU_SH_NONE]	= "Unknown"
 };
 
diff --git a/arch/sh/kernel/cpu/sh2/entry.S b/arch/sh/kernel/cpu/sh2/entry.S
index a150595..354f344 100644
--- a/arch/sh/kernel/cpu/sh2/entry.S
+++ b/arch/sh/kernel/cpu/sh2/entry.S
@@ -147,6 +147,11 @@ ENTRY(exception_handler)
 	mov	#31,r8
 	cmp/hs	r8,r9
 	bt	trap_entry	! 64 > vec >= 31  is trap
+	mov	#16,r8
+#ifdef CONFIG_CPU_J2
+	cmp/hs	r8,r9
+	bt	interrupt_entry	! 31 > vec >= 16 is interrupt
+#endif
 
 	mov.l	4f,r8
 	mov	r9,r4
diff --git a/arch/sh/kernel/cpu/sh2/probe.c b/arch/sh/kernel/cpu/sh2/probe.c
index 6c687ae..3dd8187 100644
--- a/arch/sh/kernel/cpu/sh2/probe.c
+++ b/arch/sh/kernel/cpu/sh2/probe.c
@@ -10,10 +10,27 @@
  * for more details.
  */
 #include <linux/init.h>
+#include <linux/of_fdt.h>
+#include <linux/smp.h>
+#include <linux/io.h>
 #include <asm/processor.h>
 #include <asm/cache.h>
 
-void cpu_probe(void)
+#if defined(CONFIG_CPU_J2)
+extern u32 j2_ccr_base;
+static int __init scan_cache(unsigned long node, const char *uname,
+			     int depth, void *data)
+{
+	if (!of_flat_dt_is_compatible(node, "jcore,cache"))
+		return 0;
+
+	j2_ccr_base = of_flat_dt_translate_address(node);
+
+	return 1;
+}
+#endif
+
+void __ref cpu_probe(void)
 {
 #if defined(CONFIG_CPU_SUBTYPE_SH7619)
 	boot_cpu_data.type			= CPU_SH7619;
@@ -24,10 +41,25 @@ void cpu_probe(void)
 	boot_cpu_data.dcache.linesz		= L1_CACHE_BYTES;
 	boot_cpu_data.dcache.flags		= 0;
 #endif
+
+#if defined(CONFIG_CPU_J2)
+	unsigned cpu = hard_smp_processor_id();
+	if (cpu == 0) of_scan_flat_dt(scan_cache, NULL);
+	if (j2_ccr_base) __raw_writel(0x80000303, j2_ccr_base + 4*cpu);
+	if (cpu != 0) return;
+	boot_cpu_data.type			= CPU_J2;
+	/* FIXME: cache properties should come from device tree. */
+	boot_cpu_data.dcache.ways		= 1;
+	boot_cpu_data.dcache.sets		= 256;
+	boot_cpu_data.dcache.entry_shift	= 5;
+	boot_cpu_data.dcache.linesz		= 32;
+	boot_cpu_data.dcache.flags		= 0;
+#else
 	/*
 	 * SH-2 doesn't have separate caches
 	 */
 	boot_cpu_data.dcache.flags |= SH_CACHE_COMBINED;
+#endif
 	boot_cpu_data.icache = boot_cpu_data.dcache;
 	boot_cpu_data.family = CPU_FAMILY_SH2;
 }
diff --git a/arch/sh/mm/Makefile b/arch/sh/mm/Makefile
index cee6b99..92c3bd9 100644
--- a/arch/sh/mm/Makefile
+++ b/arch/sh/mm/Makefile
@@ -4,7 +4,8 @@
 
 obj-y			:= alignment.o cache.o init.o consistent.o mmap.o
 
-cacheops-$(CONFIG_CPU_SH2)		:= cache-sh2.o
+cacheops-$(CONFIG_CPU_J2)		:= cache-j2.o
+cacheops-$(CONFIG_CPU_SUBTYPE_SH7619)	:= cache-sh2.o
 cacheops-$(CONFIG_CPU_SH2A)		:= cache-sh2a.o
 cacheops-$(CONFIG_CPU_SH3)		:= cache-sh3.o
 cacheops-$(CONFIG_CPU_SH4)		:= cache-sh4.o flush-sh4.o
diff --git a/arch/sh/mm/cache-j2.c b/arch/sh/mm/cache-j2.c
new file mode 100644
index 0000000..baf36d3
--- /dev/null
+++ b/arch/sh/mm/cache-j2.c
@@ -0,0 +1,58 @@
+/*
+ * arch/sh/mm/cache-j2.c
+ *
+ * Copyright (C) 2015 SEI
+ *
+ * Released under the terms of the GNU GPL v2.0.
+ */
+
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/cpumask.h>
+
+#include <asm/cache.h>
+#include <asm/addrspace.h>
+#include <asm/processor.h>
+#include <asm/cacheflush.h>
+#include <asm/io.h>
+
+u32 j2_ccr_base;
+
+static void j2_flush_icache(void *args)
+{
+	unsigned cpu;
+	for_each_possible_cpu(cpu)
+		__raw_writel(0x80000103, j2_ccr_base + 4*cpu);
+}
+
+static void j2_flush_dcache(void *args)
+{
+	unsigned cpu;
+	for_each_possible_cpu(cpu)
+		__raw_writel(0x80000203, j2_ccr_base + 4*cpu);
+}
+
+static void j2_flush_both(void *args)
+{
+	unsigned cpu;
+	for_each_possible_cpu(cpu)
+		__raw_writel(0x80000303, j2_ccr_base + 4*cpu);
+}
+
+void __init j2_cache_init(void)
+{
+	if (!j2_ccr_base)
+		return;
+
+	local_flush_cache_all = j2_flush_both;
+	local_flush_cache_mm = j2_flush_both;
+	local_flush_cache_dup_mm = j2_flush_both;
+	local_flush_cache_page = j2_flush_both;
+	local_flush_cache_range = j2_flush_both;
+	local_flush_dcache_page = j2_flush_dcache;
+	local_flush_icache_range = j2_flush_icache;
+	local_flush_icache_page = j2_flush_icache;
+	local_flush_cache_sigtramp = j2_flush_icache;
+
+	pr_info("Initial J2 CCR is %.8x\n", __raw_readl(j2_ccr_base));
+}
diff --git a/arch/sh/mm/cache.c b/arch/sh/mm/cache.c
index 776d664..70cc52f 100644
--- a/arch/sh/mm/cache.c
+++ b/arch/sh/mm/cache.c
@@ -309,7 +309,11 @@ void __init cpu_cache_init(void)
 	if (unlikely(cache_disabled))
 		goto skip;
 
-	if (boot_cpu_data.family == CPU_FAMILY_SH2) {
+	if (boot_cpu_data.type == CPU_J2) {
+		extern void __weak j2_cache_init(void);
+
+		j2_cache_init();
+	} else if (boot_cpu_data.family == CPU_FAMILY_SH2) {
 		extern void __weak sh2_cache_init(void);
 
 		sh2_cache_init();
-- 
2.8.1

[PATCH v2 11/12] sh: add defconfig for J-Core J2

[Rich Felker]

This defconfig is intended not to be specific to a particular board;
it enables drivers for all currently-supported hardware, and should be
updated to include additional drivers as they are added.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 arch/sh/configs/j2_defconfig | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 arch/sh/configs/j2_defconfig

diff --git a/arch/sh/configs/j2_defconfig b/arch/sh/configs/j2_defconfig
new file mode 100644
index 0000000..fca167f
--- /dev/null
+++ b/arch/sh/configs/j2_defconfig
@@ -0,0 +1,38 @@
+CONFIG_SYSVIPC=y
+CONFIG_POSIX_MQUEUE=y
+CONFIG_NO_HZ=y
+CONFIG_HIGH_RES_TIMERS=y
+CONFIG_CPU_SUBTYPE_J2=y
+CONFIG_MEMORY_START=0x10000000
+CONFIG_MEMORY_SIZE=0x04000000
+CONFIG_CPU_BIG_ENDIAN=y
+CONFIG_SH_DEVICE_TREE=y
+CONFIG_HZ_100=y
+CONFIG_CMDLINE_OVERWRITE=y
+CONFIG_CMDLINE="console=ttyUL0 earlycon"
+CONFIG_BINFMT_ELF_FDPIC=y
+CONFIG_BINFMT_FLAT=y
+CONFIG_NET=y
+CONFIG_UNIX=y
+CONFIG_INET=y
+CONFIG_DEVTMPFS=y
+CONFIG_DEVTMPFS_MOUNT=y
+CONFIG_NETDEVICES=y
+CONFIG_SERIAL_UARTLITE=y
+CONFIG_SERIAL_UARTLITE_CONSOLE=y
+CONFIG_I2C=y
+CONFIG_SPI=y
+CONFIG_SPI_JCORE=y
+CONFIG_WATCHDOG=y
+CONFIG_MMC=y
+CONFIG_MMC_SPI=y
+CONFIG_CLKSRC_JCORE_PIT=y
+CONFIG_JCORE_AIC=y
+CONFIG_EXT4_FS=y
+CONFIG_VFAT_FS=y
+CONFIG_FAT_DEFAULT_IOCHARSET="ascii"
+CONFIG_FAT_DEFAULT_UTF8=y
+CONFIG_NLS_DEFAULT="utf8"
+CONFIG_NLS_CODEPAGE_437=y
+CONFIG_NLS_ASCII=y
+CONFIG_NLS_UTF8=y
-- 
2.8.1

[PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
My previous post of the patch series accidentally omitted omitted
Cc'ing of subsystem maintainers for the necessary clocksource,
irqchip, and spi drivers. Please ack if this looks ok because I want
to get it merged as part of the arch/sh pull request for 4.7.

 drivers/clocksource/Kconfig     |   9 ++
 drivers/clocksource/Makefile    |   1 +
 drivers/clocksource/jcore-pit.c | 176 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)
 create mode 100644 drivers/clocksource/jcore-pit.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index c346be6..a29fd31 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -297,6 +297,15 @@ config SYS_SUPPORTS_SH_TMU
 config SYS_SUPPORTS_EM_STI
         bool
 
+config CLKSRC_JCORE_PIT
+	bool "J-Core integrated PIT/RTC"
+	depends on GENERIC_CLOCKEVENTS
+	depends on HAS_IOMEM
+	help
+	  This enables build of clocksource and clockevent driver for
+	  the integrated PIT/RTC in the J-Core synthesizable, open source
+	  SoC.
+
 config SH_TIMER_CMT
 	bool "Renesas CMT timer driver" if COMPILE_TEST
 	depends on GENERIC_CLOCKEVENTS
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index dc2b899..d825fcf 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
 obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
 obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
 obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
+obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
 obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
 obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
 obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
diff --git a/drivers/clocksource/jcore-pit.c b/drivers/clocksource/jcore-pit.c
new file mode 100644
index 0000000..a4fde51
--- /dev/null
+++ b/drivers/clocksource/jcore-pit.c
@@ -0,0 +1,176 @@
+/*
+ * J-Core SoC PIT/RTC driver
+ *
+ * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/sched.h>
+#include <linux/kernel.h>
+#include <linux/param.h>
+#include <linux/interrupt.h>
+#include <linux/profile.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/cpu.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/irqchip.h>
+#include <asm/io.h>
+#include <asm/irq.h>
+#include <asm/rtc.h>
+
+static unsigned char __iomem *pit_base;
+static int pit_irq;
+static u32 percpu_offset;
+static u32 enable_val;
+
+static struct clock_event_device __percpu *pit_percpu;
+
+#define REG_PITEN 0x00
+#define REG_THROT 0x10
+#define REG_COUNT 0x14
+#define REG_BUSPD 0x18
+#define REG_SECHI 0x20
+#define REG_SECLO 0x24
+#define REG_NSEC  0x28
+
+static cycle_t rtc_read(struct clocksource *cs)
+{
+	u32 sechi, seclo, nsec, sechi0, seclo0;
+
+	sechi = __raw_readl(pit_base + REG_SECHI);
+	seclo = __raw_readl(pit_base + REG_SECLO);
+	do {
+		sechi0 = sechi;
+		seclo0 = seclo;
+		nsec  = __raw_readl(pit_base + REG_NSEC);
+		sechi = __raw_readl(pit_base + REG_SECHI);
+		seclo = __raw_readl(pit_base + REG_SECLO);
+	} while (sechi0 != sechi || seclo0 != seclo);
+
+	return ((u64)sechi << 32 | seclo) * 1000000000 + nsec;
+}
+
+struct clocksource rtc_csd = {
+	.name = "rtc",
+	.rating = 400,
+	.read = rtc_read,
+	.mult = 1,
+	.shift = 0,
+	.mask = CLOCKSOURCE_MASK(64),
+	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static int pit_disable(struct clock_event_device *ced)
+{
+	unsigned cpu = smp_processor_id();
+	writel(0, pit_base + percpu_offset*cpu + REG_PITEN);
+	return 0;
+}
+
+static int pit_set(unsigned long delta, struct clock_event_device *ced)
+{
+	unsigned cpu = smp_processor_id();
+
+	pit_disable(ced);
+
+	writel(delta, pit_base + percpu_offset*cpu + REG_THROT);
+	writel(enable_val, pit_base + percpu_offset*cpu + REG_PITEN);
+
+	return 0;
+}
+
+static int pit_set_periodic(struct clock_event_device *ced)
+{
+	unsigned cpu = smp_processor_id();
+	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
+
+	return pit_set(DIV_ROUND_CLOSEST(1000000000, HZ*per), ced);
+}
+
+static int pit_local_init(struct clock_event_device *ced)
+{
+	unsigned cpu = smp_processor_id();
+	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
+
+	pr_info("Local PIT init on cpu %u\n", cpu);
+
+	ced->name = "pit";
+	ced->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT
+		| CLOCK_EVT_FEAT_PERCPU;
+	ced->cpumask = cpumask_of(cpu);
+	ced->rating = 400;
+	ced->irq = pit_irq;
+	ced->set_state_shutdown = pit_disable;
+	ced->set_state_periodic = pit_set_periodic;
+	ced->set_state_oneshot = pit_disable;
+	ced->set_next_event = pit_set;
+
+	clockevents_config_and_register(ced, DIV_ROUND_CLOSEST(1000000000, per),
+	                                1, 0xffffffff);
+
+	pit_set_periodic(ced);
+
+	return 0;
+}
+
+static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
+{
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		pit_local_init(this_cpu_ptr(pit_percpu));
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block pit_cpu_nb = {
+	.notifier_call = pit_cpu_notify,
+};
+
+static irqreturn_t timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *ced = this_cpu_ptr(dev_id);
+
+	if (clockevent_state_oneshot(ced)) pit_disable(ced);
+
+	ced->event_handler(ced);
+
+	return IRQ_HANDLED;
+}
+
+static void __init pit_init(struct device_node *node)
+{
+	unsigned long hwirq;
+	int err;
+
+	pit_base = of_iomap(node, 0);
+	pit_irq = irq_of_parse_and_map(node, 0);
+	of_property_read_u32(node, "cpu-offset", &percpu_offset);
+
+	pr_info("Initializing J-Core PIT at %p IRQ %d\n", pit_base, pit_irq);
+
+	clocksource_register_hz(&rtc_csd, 1000000000);
+
+	pit_percpu = alloc_percpu(struct clock_event_device);
+	register_cpu_notifier(&pit_cpu_nb);
+
+	err = request_irq(pit_irq, timer_interrupt,
+		IRQF_TIMER | IRQF_PERCPU, "pit", pit_percpu);
+	if (err) pr_err("pit irq request failed: %d\n", err);
+
+	hwirq = irq_get_irq_data(pit_irq)->hwirq;
+	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);
+
+	pit_local_init(this_cpu_ptr(pit_percpu));
+}
+
+CLOCKSOURCE_OF_DECLARE(jcore_pit, "jcore,pit", pit_init);
-- 
2.8.1

Re: [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Daniel Lezcano]

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---

Hi Rich,

please add a nice changelog describing how works the timer.

Having openhardware is really awesome and that deserves a nice
documentation. I noticed the changelog of this patchset it very light and 
that's a pity regarding the potential of the J-core project.

I'm very much looking forward to see the J-core evolving and, IIUC, bringing 
kernel developer to review [and participate to] the CPU design is one of 
your objective and that's really cool. If you can beef up the changelog with 
detailed description and pointers to documentation to refer to, that will 
help a lot, especially when new drivers are added, to fill the gap between 
HW/SW.

> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.

In future send the patches sooner so they can be reviewed in a relaxed way 
and picked up in the right tree. I doubt that will make it for 4.7.

>  drivers/clocksource/Kconfig     |   9 ++
>  drivers/clocksource/Makefile    |   1 +
>  drivers/clocksource/jcore-pit.c | 176 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
>  create mode 100644 drivers/clocksource/jcore-pit.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index c346be6..a29fd31 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -297,6 +297,15 @@ config SYS_SUPPORTS_SH_TMU
>  config SYS_SUPPORTS_EM_STI
>          bool
>  
> +config CLKSRC_JCORE_PIT
> +	bool "J-Core integrated PIT/RTC"

Replace by:

bool "J-Core integrated PIT/RTC" if COMPILE_TEST

Let the platform's Kconfig to select the timer. Beside, check the timer 
compiles correctly on other platform (eg. x86) for compilation test 
coverage.

Below a comment regarding RTC/PIT name.

> +	depends on GENERIC_CLOCKEVENTS
> +	depends on HAS_IOMEM
> +	help
> +	  This enables build of clocksource and clockevent driver for
> +	  the integrated PIT/RTC in the J-Core synthesizable, open source
> +	  SoC.
>
>
>  config SH_TIMER_CMT
>  	bool "Renesas CMT timer driver" if COMPILE_TEST
>  	depends on GENERIC_CLOCKEVENTS
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index dc2b899..d825fcf 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
>  obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
>  obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
>  obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
> +obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
>  obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
>  obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
>  obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
> diff --git a/drivers/clocksource/jcore-pit.c b/drivers/clocksource/jcore-pit.c
> new file mode 100644
> index 0000000..a4fde51
> --- /dev/null
> +++ b/drivers/clocksource/jcore-pit.c
> @@ -0,0 +1,176 @@
> +/*
> + * J-Core SoC PIT/RTC driver

Is it really RTC ? :)

Please fix the names to have something more accurate (eg. jcore_clockevent, 
jcore_clocksource) regarding how the other drivers are named.

> + *
> + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/kernel.h>
> +#include <linux/param.h>
> +#include <linux/interrupt.h>
> +#include <linux/profile.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/cpu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqchip.h>
> +#include <asm/io.h>
> +#include <asm/irq.h>
> +#include <asm/rtc.h>

Are all these headers really needed ?

> +static unsigned char __iomem *pit_base;

s/unsigned char/void/

> +static int pit_irq;
> +static u32 percpu_offset;
> +static u32 enable_val;
> +static struct clock_event_device __percpu *pit_percpu;

Are the clockevents per cpu on the J2 ? Is there any documentation 
describing this hardware I can refer to ?

It would make sense to group all these values into a structure and use 
container_of to access them and precalculate the percpu_offset, so no need 
to compute the offset when entering the callbacks again and again.

struct jcore_percpu_clkevt {
	__iomem void *addr;
	struct clock_event_device clkevt;
}

> +#define REG_PITEN 0x00
> +#define REG_THROT 0x10
> +#define REG_COUNT 0x14
> +#define REG_BUSPD 0x18
> +#define REG_SECHI 0x20
> +#define REG_SECLO 0x24
> +#define REG_NSEC  0x28
> +
> +static cycle_t rtc_read(struct clocksource *cs)
> +{
> +	u32 sechi, seclo, nsec, sechi0, seclo0;
> +
> +	sechi = __raw_readl(pit_base + REG_SECHI);
> +	seclo = __raw_readl(pit_base + REG_SECLO);
> +	do {
> +		sechi0 = sechi;
> +		seclo0 = seclo;
> +		nsec  = __raw_readl(pit_base + REG_NSEC);
> +		sechi = __raw_readl(pit_base + REG_SECHI);
> +		seclo = __raw_readl(pit_base + REG_SECLO);
> +	} while (sechi0 != sechi || seclo0 != seclo);
> +
> +	return ((u64)sechi << 32 | seclo) * 1000000000 + nsec;

s/1000000000/NSEC_PER_SEC/

> +}

Do you really, really, want to use the 64bits ? There is no real benefit and 
it has a significant overhead. The impact on a j-core could be really not 
negligible IMHO.

> +
> +struct clocksource rtc_csd = {
> +	.name = "rtc",
> +	.rating = 400,
> +	.read = rtc_read,
> +	.mult = 1,
> +	.shift = 0,
> +	.mask = CLOCKSOURCE_MASK(64),
> +	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static int pit_disable(struct clock_event_device *ced)
> +{
> +	unsigned cpu = smp_processor_id();
> +	writel(0, pit_base + percpu_offset*cpu + REG_PITEN);
> +	return 0;
> +}
> +
> +static int pit_set(unsigned long delta, struct clock_event_device *ced)
> +{
> +	unsigned cpu = smp_processor_id();
> +
> +	pit_disable(ced);

Move out the function pit_disabled().

> +	writel(delta, pit_base + percpu_offset*cpu + REG_THROT);
> +	writel(enable_val, pit_base + percpu_offset*cpu + REG_PITEN);

Write an enable function and move it out.

> +
> +	return 0;
> +}
> +
> +static int pit_set_periodic(struct clock_event_device *ced)
> +{
> +	unsigned cpu = smp_processor_id();
> +	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
> +
> +	return pit_set(DIV_ROUND_CLOSEST(1000000000, HZ*per), ced);
> +}
> +
> +static int pit_local_init(struct clock_event_device *ced)
> +{
> +	unsigned cpu = smp_processor_id();
> +	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
> +
> +	pr_info("Local PIT init on cpu %u\n", cpu);
> +
> +	ced->name = "pit";
> +	ced->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT
> +		| CLOCK_EVT_FEAT_PERCPU;
> +	ced->cpumask = cpumask_of(cpu);
> +	ced->rating = 400;
> +	ced->irq = pit_irq;
> +	ced->set_state_shutdown = pit_disable;
> +	ced->set_state_periodic = pit_set_periodic;
> +	ced->set_state_oneshot = pit_disable;
> +	ced->set_next_event = pit_set;

Don't mix helper functions and callbacks:

static void pit_set_periodic(struct clock_event_device *ced)
{
	__iomem void *addr = to_jcore_clkevt(ced)->addr;
	unsigned long period = readl(addr + REG_BUSPD);

	pit_disable(addr);
	pit_set(period, addr);
	pit_enabled(addr);
}

static void pit_set_next_event(unsigned long delta, struct 
clock_event_device *ced)
{
	__iomem void *addr = to_jcore_clkevt(ced)->addr;

	pit_disable(addr);
	pit_set(delta, addr);
	pit_enable(addr);
}

(jcore_set_next_event, jcore_set_periodic)

> +
> +	clockevents_config_and_register(ced, DIV_ROUND_CLOSEST(1000000000, per),
> +	                                1, 0xffffffff);
> +
> +	pit_set_periodic(ced);

It is up to the time framework to set this.

I don't see enable_percpu_irq / disable_percpu_irq in this driver.

> +	return 0;
> +}
> +
> +static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> +{
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_STARTING:
> +		pit_local_init(this_cpu_ptr(pit_percpu));
> +		break;
> +	}

DYING is missing.

> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block pit_cpu_nb = {
> +	.notifier_call = pit_cpu_notify,
> +};
> +
> +static irqreturn_t timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *ced = this_cpu_ptr(dev_id);

this_cpu_ptr shouldn't be used here, see the comment related to percpu in 
the init function.

> +	if (clockevent_state_oneshot(ced)) pit_disable(ced);

CR missing before pit_disable().

> +	ced->event_handler(ced);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void __init pit_init(struct device_node *node)
> +{
> +	unsigned long hwirq;
> +	int err;

Test return code for every function below.

> +
> +	pit_base = of_iomap(node, 0);
> +	pit_irq = irq_of_parse_and_map(node, 0);
> +	of_property_read_u32(node, "cpu-offset", &percpu_offset);
> +
> +	pr_info("Initializing J-Core PIT at %p IRQ %d\n", pit_base, pit_irq);
> +
> +	clocksource_register_hz(&rtc_csd, 1000000000);

I suggest the rate to be passed through the DT.

> +	pit_percpu = alloc_percpu(struct clock_event_device);
> +	register_cpu_notifier(&pit_cpu_nb);
> +
> +	err = request_irq(pit_irq, timer_interrupt,
> +		IRQF_TIMER | IRQF_PERCPU, "pit", pit_percpu);

So, we have per CPU private IRQ with the same number, right?

You should use the 'percpu' API.

 - request_percpu_irq
 - enable_percpu_irq
 - disable_percpu_irq

The interrupt callback will have the right percpu dev_id parameter pointing 
to the right percpu structure. So you should not have to play with 
this_cpu_ptr anywhere.

> +	if (err) pr_err("pit irq request failed: %d\n", err);

CR before pr_err.

> +	hwirq = irq_get_irq_data(pit_irq)->hwirq;
> +	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);

Can you explain? (and replace litterals by macros).

> +	pit_local_init(this_cpu_ptr(pit_percpu));
> +}

I am wondering if the j2 is subject to change. It is now designable through 
FPGA and I imagine the design can go back and forth, no? If yes (and that's 
good), wouldn't make sense to make this driver (and others) highly 
configurable via the DT, much more than what there is now in order to 
prevent errata and kernel changes?

Thanks
  -- Daniel

Re: [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Rich Felker]

On Fri, May 20, 2016 at 04:01:56PM +0200, Daniel Lezcano wrote:
> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> 
> Hi Rich,
> 
> please add a nice changelog describing how works the timer.

OK. Do you prefer this in changelog, comments in the source, or both?

> Having openhardware is really awesome and that deserves a nice
> documentation. I noticed the changelog of this patchset it very light and 
> that's a pity regarding the potential of the J-core project.
> 
> I'm very much looking forward to see the J-core evolving and, IIUC, bringing 
> kernel developer to review [and participate to] the CPU design is one of 
> your objective and that's really cool. If you can beef up the changelog with 
> detailed description and pointers to documentation to refer to, that will 
> help a lot, especially when new drivers are added, to fill the gap between 
> HW/SW.

*Nod*

> > My previous post of the patch series accidentally omitted omitted
> > Cc'ing of subsystem maintainers for the necessary clocksource,
> > irqchip, and spi drivers. Please ack if this looks ok because I want
> > to get it merged as part of the arch/sh pull request for 4.7.
> 
> In future send the patches sooner so they can be reviewed in a relaxed way 
> and picked up in the right tree. I doubt that will make it for 4.7.
> 
> >  drivers/clocksource/Kconfig     |   9 ++
> >  drivers/clocksource/Makefile    |   1 +
> >  drivers/clocksource/jcore-pit.c | 176 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 186 insertions(+)
> >  create mode 100644 drivers/clocksource/jcore-pit.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index c346be6..a29fd31 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -297,6 +297,15 @@ config SYS_SUPPORTS_SH_TMU
> >  config SYS_SUPPORTS_EM_STI
> >          bool
> >  
> > +config CLKSRC_JCORE_PIT
> > +	bool "J-Core integrated PIT/RTC"
> 
> Replace by:
> 
> bool "J-Core integrated PIT/RTC" if COMPILE_TEST
> 
> Let the platform's Kconfig to select the timer. Beside, check the timer 
> compiles correctly on other platform (eg. x86) for compilation test 
> coverage.

So the prompt would not even appear unless COMPILE_TEST is selected? I
don't mind doing it that way if this is the established best practice.
For clocksource/clockevent, the system isn't even usable without it,
so there's little harm in having CONFIG_CPU_J2 select CLKSRC_JCORE_PIT
for the user. On the other hand, some of the drivers like the SPI
master, (future) EMAC, etc. are things you might want to be able to
turn off to build a size-optimized kernel for a system that doesn't
need (or doesn't even have) them, so this approach does not seem to
make sense for other such drivers.

My main theoretical concern here is what happens if someone uses the
J2 cpu core with completely different SoC peripherals hooked up to it,
and doesn't want to be forced to build the jcore-pit driver. Is this
type of thing an issue people have thought about and reached a
canonical solution to?

> Below a comment regarding RTC/PIT name.
> 
> > +	depends on GENERIC_CLOCKEVENTS
> > +	depends on HAS_IOMEM
> > +	help
> > +	  This enables build of clocksource and clockevent driver for
> > +	  the integrated PIT/RTC in the J-Core synthesizable, open source
> > +	  SoC.
> >
> >
> >  config SH_TIMER_CMT
> >  	bool "Renesas CMT timer driver" if COMPILE_TEST
> >  	depends on GENERIC_CLOCKEVENTS
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index dc2b899..d825fcf 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
> >  obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
> >  obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
> >  obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o
> > +obj-$(CONFIG_CLKSRC_JCORE_PIT)		+= jcore-pit.o
> >  obj-$(CONFIG_SH_TIMER_CMT)	+= sh_cmt.o
> >  obj-$(CONFIG_SH_TIMER_MTU2)	+= sh_mtu2.o
> >  obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
> > diff --git a/drivers/clocksource/jcore-pit.c b/drivers/clocksource/jcore-pit.c
> > new file mode 100644
> > index 0000000..a4fde51
> > --- /dev/null
> > +++ b/drivers/clocksource/jcore-pit.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * J-Core SoC PIT/RTC driver
> 
> Is it really RTC ? :)

That's the name used in the hardware. It's a settable clock counting
seconds/nanoseconds and it's the most appropriate thing the hardware
has for a clocksource device. The old kernel patches that existed when
I took over were not using it and instead used jiffies and a PIT
register that returned ns since the last timer interrupt, which is now
unused (that approach precluded dyntick).

> Please fix the names to have something more accurate (eg. jcore_clockevent, 
> jcore_clocksource) regarding how the other drivers are named.

Filename/Kconfig/etc., or names in the source? I think you mean the
latter but I just want to check.

> > + *
> > + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + */
> > +
> > +#include <linux/sched.h>
> > +#include <linux/kernel.h>
> > +#include <linux/param.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/profile.h>
> > +#include <linux/init.h>
> > +#include <linux/irq.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/cpu.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/irqchip.h>
> > +#include <asm/io.h>
> > +#include <asm/irq.h>
> > +#include <asm/rtc.h>
> 
> Are all these headers really needed ?

I'll re-check. Sorry I didn't notice the list had gotten so long. This
code evolved from a pre-DT board file that also had interrupt
controller and experimental SMP stuff in it and I never went back to
check what was really needed.

> > +static unsigned char __iomem *pit_base;
> 
> s/unsigned char/void/

OK. I cringe at doing arithmetic on void*, but that seems to be the
convention in the kernel.

> > +static int pit_irq;
> > +static u32 percpu_offset;
> > +static u32 enable_val;
> > +static struct clock_event_device __percpu *pit_percpu;
> 
> Are the clockevents per cpu on the J2 ?

Yes. There's an instance of the PIT (which is actually attached to an
interrupt controller in the hardware) for each cpu. The smp support is
not in this patch series since it's still new and there's no public
board-target suporting smp, but there will be soon.

> Is there any documentation 
> describing this hardware I can refer to ?

There's the vhdl source. I'm not sure what else is publicly available
right now; I'll check on it for you.

> It would make sense to group all these values into a structure and use 
> container_of to access them and precalculate the percpu_offset, so no need 
> to compute the offset when entering the callbacks again and again.
> 
> struct jcore_percpu_clkevt {
> 	__iomem void *addr;
> 	struct clock_event_device clkevt;
> }
> 
> > +#define REG_PITEN 0x00
> > +#define REG_THROT 0x10
> > +#define REG_COUNT 0x14
> > +#define REG_BUSPD 0x18
> > +#define REG_SECHI 0x20
> > +#define REG_SECLO 0x24
> > +#define REG_NSEC  0x28
> > +
> > +static cycle_t rtc_read(struct clocksource *cs)
> > +{
> > +	u32 sechi, seclo, nsec, sechi0, seclo0;
> > +
> > +	sechi = __raw_readl(pit_base + REG_SECHI);
> > +	seclo = __raw_readl(pit_base + REG_SECLO);
> > +	do {
> > +		sechi0 = sechi;
> > +		seclo0 = seclo;
> > +		nsec  = __raw_readl(pit_base + REG_NSEC);
> > +		sechi = __raw_readl(pit_base + REG_SECHI);
> > +		seclo = __raw_readl(pit_base + REG_SECLO);
> > +	} while (sechi0 != sechi || seclo0 != seclo);
> > +
> > +	return ((u64)sechi << 32 | seclo) * 1000000000 + nsec;
> 
> s/1000000000/NSEC_PER_SEC/

OK.

> > +}
> 
> Do you really, really, want to use the 64bits ? There is no real benefit and 
> it has a significant overhead. The impact on a j-core could be really not 
> negligible IMHO.

With just 32-bit, there's no way the cpu could sleep more than 4
seconds without time desynchronization. Right now it doesn't seem to
do that anyway (max seems like abou 1-1.5 sec) but it feels like a
shame to preclude it. I agree there's some serious cost to the 64-bit
arithmetic though to weigh in. One option would be trying to avoid
using the high part of seconds, but I think this hits a (admittedly
largely theoretical) discontinuity in the resulting nanosecond count
when seclo overflows.

> > +struct clocksource rtc_csd = {
> > +	.name = "rtc",
> > +	.rating = 400,
> > +	.read = rtc_read,
> > +	.mult = 1,
> > +	.shift = 0,
> > +	.mask = CLOCKSOURCE_MASK(64),
> > +	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> > +static int pit_disable(struct clock_event_device *ced)
> > +{
> > +	unsigned cpu = smp_processor_id();
> > +	writel(0, pit_base + percpu_offset*cpu + REG_PITEN);
> > +	return 0;
> > +}
> > +
> > +static int pit_set(unsigned long delta, struct clock_event_device *ced)
> > +{
> > +	unsigned cpu = smp_processor_id();
> > +
> > +	pit_disable(ced);
> 
> Move out the function pit_disabled().

I don't understand what you're asking for here. The "throttle"
register is only programmable when the enable bit is clear, so disable
has to be called before setting the timer. Are you saying there should
be a separate disable "helper function" from the function whose
address is stored in the clockevent device set_state_shutdown pointer?

> > +	writel(delta, pit_base + percpu_offset*cpu + REG_THROT);
> > +	writel(enable_val, pit_base + percpu_offset*cpu + REG_PITEN);
> 
> Write an enable function and move it out.

So the set function should call pit_disable, then write the throttle
register, then call pit_enable?

> > +
> > +	return 0;
> > +}
> > +
> > +static int pit_set_periodic(struct clock_event_device *ced)
> > +{
> > +	unsigned cpu = smp_processor_id();
> > +	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
> > +
> > +	return pit_set(DIV_ROUND_CLOSEST(1000000000, HZ*per), ced);
> > +}
> > +
> > +static int pit_local_init(struct clock_event_device *ced)
> > +{
> > +	unsigned cpu = smp_processor_id();
> > +	unsigned long per = readl(pit_base + percpu_offset*cpu + REG_BUSPD);
> > +
> > +	pr_info("Local PIT init on cpu %u\n", cpu);
> > +
> > +	ced->name = "pit";
> > +	ced->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT
> > +		| CLOCK_EVT_FEAT_PERCPU;
> > +	ced->cpumask = cpumask_of(cpu);
> > +	ced->rating = 400;
> > +	ced->irq = pit_irq;
> > +	ced->set_state_shutdown = pit_disable;
> > +	ced->set_state_periodic = pit_set_periodic;
> > +	ced->set_state_oneshot = pit_disable;
> > +	ced->set_next_event = pit_set;
> 
> Don't mix helper functions and callbacks:

So have dedicated shutdown & oneshot functions that just call disable,
a dedicated set_next_event function that just calls pit_set, and a
dedicated set_periodic function that calls pit_set with the right
computed time?

> static void pit_set_periodic(struct clock_event_device *ced)
> {
> 	__iomem void *addr = to_jcore_clkevt(ced)->addr;
> 	unsigned long period = readl(addr + REG_BUSPD);
> 
> 	pit_disable(addr);
> 	pit_set(period, addr);
> 	pit_enabled(addr);
> }
> 
> static void pit_set_next_event(unsigned long delta, struct 
> clock_event_device *ced)
> {
> 	__iomem void *addr = to_jcore_clkevt(ced)->addr;
> 
> 	pit_disable(addr);
> 	pit_set(delta, addr);
> 	pit_enable(addr);
> }
> 
> (jcore_set_next_event, jcore_set_periodic)

OK.

> > +	clockevents_config_and_register(ced, DIV_ROUND_CLOSEST(1000000000, per),
> > +	                                1, 0xffffffff);
> > +
> > +	pit_set_periodic(ced);
> 
> It is up to the time framework to set this.

To call the set_periodic function? OK.

> I don't see enable_percpu_irq / disable_percpu_irq in this driver.

I was unable to get the percpu irq framework to work correctly with my
driver for the interrupt controller. Looking at some other irqchip
drivers now, it seems they have to call irq_set_percpu_devid from the
domain mapping function (or somewhere) exactly once, but I don't see
any good way to know whether to do this. In principle at the hw level
all irqs are percpu, but most peripherals' irq lines are only wired to
cpu0.

> > +	return 0;
> > +}
> > +
> > +static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> > +{
> > +	switch (action & ~CPU_TASKS_FROZEN) {
> > +	case CPU_STARTING:
> > +		pit_local_init(this_cpu_ptr(pit_percpu));
> > +		break;
> > +	}
> 
> DYING is missing.

Does it need to unregister the device?

> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block pit_cpu_nb = {
> > +	.notifier_call = pit_cpu_notify,
> > +};
> > +
> > +static irqreturn_t timer_interrupt(int irq, void *dev_id)
> > +{
> > +	struct clock_event_device *ced = this_cpu_ptr(dev_id);
> 
> this_cpu_ptr shouldn't be used here, see the comment related to percpu in 
> the init function.

Changing this depends on the above.

> > +	if (clockevent_state_oneshot(ced)) pit_disable(ced);
> 
> CR missing before pit_disable().

OK.

> > +	ced->event_handler(ced);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void __init pit_init(struct device_node *node)
> > +{
> > +	unsigned long hwirq;
> > +	int err;
> 
> Test return code for every function below.

OK.

> 
> > +
> > +	pit_base = of_iomap(node, 0);
> > +	pit_irq = irq_of_parse_and_map(node, 0);
> > +	of_property_read_u32(node, "cpu-offset", &percpu_offset);
> > +
> > +	pr_info("Initializing J-Core PIT at %p IRQ %d\n", pit_base, pit_irq);
> > +
> > +	clocksource_register_hz(&rtc_csd, 1000000000);
> 
> I suggest the rate to be passed through the DT.

Normally I would agree, the units of the hw registers are seconds and
nanoseconds. I don't see any circumstance under which it would make
sense to change the value here without a different hw register
interface.

> > +	pit_percpu = alloc_percpu(struct clock_event_device);
> > +	register_cpu_notifier(&pit_cpu_nb);
> > +
> > +	err = request_irq(pit_irq, timer_interrupt,
> > +		IRQF_TIMER | IRQF_PERCPU, "pit", pit_percpu);
> 
> So, we have per CPU private IRQ with the same number, right?

Yes.

> You should use the 'percpu' API.
> 
>  - request_percpu_irq
>  - enable_percpu_irq
>  - disable_percpu_irq

Still trying to figure out how to make that work.

> The interrupt callback will have the right percpu dev_id parameter pointing 
> to the right percpu structure. So you should not have to play with 
> this_cpu_ptr anywhere.
> 
> > +	if (err) pr_err("pit irq request failed: %d\n", err);
> 
> CR before pr_err.

OK.

> > +	hwirq = irq_get_irq_data(pit_irq)->hwirq;
> > +	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);
> 
> Can you explain? (and replace litterals by macros).

Since the PIT is actually integrated with the interrupt controller in
the hardware, it's not tied to a fixed IRQ; it's programmable. However
I don't know any way to represent "driver can use any irq it wants but
should avoid sharing" in the DT, so I opted to have the DT just assign
one. The above code programs it. Bit 26 is the actual enable bit, and
the actual hw irq number (which conceptually need not match the virq
number, although it does) gets programmed in a way thats compatible
with the programmable interrupt priority stuff aic1 did (generating an
appropriate priority matching what you'd get with aic2). I have
unpolished specs from one of our hw engineers that I could review and
see if this could be simplified/clarified.

> > +	pit_local_init(this_cpu_ptr(pit_percpu));
> > +}
> 
> I am wondering if the j2 is subject to change. It is now designable through 
> FPGA and I imagine the design can go back and forth, no? If yes (and that's 
> good), wouldn't make sense to make this driver (and others) highly 
> configurable via the DT, much more than what there is now in order to 
> prevent errata and kernel changes?

It's hard to predict exactly what might change or how it might change,
which is why I went to the effort to redo all our old drivers in a DT
framework. The current "jcore,pit" binding is intended only for things
sufficiently compatible with the current programming interface work
with drivers (or bare-metal software) written for the current
programming interface. It's expected that we'll add new compatible
tags if/when changes have to be made, and then the updated driver can
use whatever new generality is needed with the new compatible tag, or
keep working the same as it does now (possibly via more general code
with appropriate parameter values) when the old compatible tag is
used.

Rich

Re: [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Rob Landley]

On 05/20/2016 10:15 PM, Rich Felker wrote:
> On Fri, May 20, 2016 at 04:01:56PM +0200, Daniel Lezcano wrote:
>>> +	return ((u64)sechi << 32 | seclo) * 1000000000 + nsec;
>>
>> s/1000000000/NSEC_PER_SEC/
...
>>> +	hwirq = irq_get_irq_data(pit_irq)->hwirq;
>>> +	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);
>>
>> Can you explain? (and replace litterals by macros).

  "Hiding numbers that are used just once into defines to put them
   out of sight does not really help readability." - Pavel Machek

  http://lkml.iu.edu/hypermail/linux/kernel/1407.1/00299.html

Rob

Re: [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Daniel Lezcano]

On Fri, May 20, 2016 at 11:15:16PM -0400, Rich Felker wrote:
> On Fri, May 20, 2016 at 04:01:56PM +0200, Daniel Lezcano wrote:
> > On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > ---
> > 
> > Hi Rich,
> > 
> > please add a nice changelog describing how works the timer.
> 
> OK. Do you prefer this in changelog, comments in the source, or both?

I prefer a [detailed] description of the timer in the changelog or the file 
header. As the timer is trivial for now, I don't see the benefit of adding 
too much comments in the code.

> > Having openhardware is really awesome and that deserves a nice
> > documentation. I noticed the changelog of this patchset it very light and 
> > that's a pity regarding the potential of the J-core project.
> > 
> > I'm very much looking forward to see the J-core evolving and, IIUC, bringing 
> > kernel developer to review [and participate to] the CPU design is one of 
> > your objective and that's really cool. If you can beef up the changelog with 
> > detailed description and pointers to documentation to refer to, that will 
> > help a lot, especially when new drivers are added, to fill the gap between 
> > HW/SW.
> 
> *Nod*
> 
> > > My previous post of the patch series accidentally omitted omitted
> > > Cc'ing of subsystem maintainers for the necessary clocksource,
> > > irqchip, and spi drivers. Please ack if this looks ok because I want
> > > to get it merged as part of the arch/sh pull request for 4.7.
> > 
> > In future send the patches sooner so they can be reviewed in a relaxed way 
> > and picked up in the right tree. I doubt that will make it for 4.7.
> > 
> > >  drivers/clocksource/Kconfig     |   9 ++
> > >  drivers/clocksource/Makefile    |   1 +
> > >  drivers/clocksource/jcore-pit.c | 176 ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 186 insertions(+)
> > >  create mode 100644 drivers/clocksource/jcore-pit.c
> > > 
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index c346be6..a29fd31 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -297,6 +297,15 @@ config SYS_SUPPORTS_SH_TMU
> > >  config SYS_SUPPORTS_EM_STI
> > >          bool
> > >  
> > > +config CLKSRC_JCORE_PIT
> > > +	bool "J-Core integrated PIT/RTC"
> > 
> > Replace by:
> > 
> > bool "J-Core integrated PIT/RTC" if COMPILE_TEST
> > 
> > Let the platform's Kconfig to select the timer. Beside, check the timer 
> > compiles correctly on other platform (eg. x86) for compilation test 
> > coverage.
> 
> So the prompt would not even appear unless COMPILE_TEST is selected? 

Correct.

> I don't mind doing it that way if this is the established best practice.
> For clocksource/clockevent, the system isn't even usable without it,
> so there's little harm in having CONFIG_CPU_J2 select CLKSRC_JCORE_PIT
> for the user. On the other hand, some of the drivers like the SPI
> master, (future) EMAC, etc. are things you might want to be able to
> turn off to build a size-optimized kernel for a system that doesn't
> need (or doesn't even have) them, so this approach does not seem to
> make sense for other such drivers.
> 
> My main theoretical concern here is what happens if someone uses the
> J2 cpu core with completely different SoC peripherals hooked up to it,
> and doesn't want to be forced to build the jcore-pit driver. Is this
> type of thing an issue people have thought about and reached a
> canonical solution to?

In this case it is the SoC's Kconfig which is in charge of setting the right 
parameter not the CPU's Kconfig option. This is what we find in the ARM 
configs file where the ARMv7 is the "CPU" but the SoC is different.

But if these timers are local to the CPU, there is a good chance nobody will 
change that as they are the most efficient and certainly well fitted for 
power management. I believe the timer supposed to work as the broadcast 
timer is more subject to change.

IMO, some clarifications about the CPU design and these timers may be 
needed. If the local timers are specified as part of the CPU, then 
CONFIG_CPU_J2 => CONFIG_JCORE_PIT, otherwise CONFIG_SOC_J2_BASED => 
CONFIG_CPU_J2 [+ CONFIG_JCORE_PIT].

[ ... ]

> > > +/*
> > > + * J-Core SoC PIT/RTC driver
> > 
> > Is it really RTC ? :)
> 
> That's the name used in the hardware. It's a settable clock counting
> seconds/nanoseconds and it's the most appropriate thing the hardware
> has for a clocksource device. The old kernel patches that existed when
> I took over were not using it and instead used jiffies and a PIT
> register that returned ns since the last timer interrupt, which is now
> unused (that approach precluded dyntick).

Ok, just for clarification. From Linux, a RTC is different from a 
clocksource. The former is backed up with a battery, relies almost always on 
a quartz and populates /dev/rtc, the latter is based on a clock (which can 
vary) and is volatile. Naming it RTC is confusing as it is a clocksource 
(and RTC falls under drivers/rtc).

> > Please fix the names to have something more accurate (eg. 
> > jcore_clockevent, jcore_clocksource) regarding how the other drivers are 
> > named.
> 
> Filename/Kconfig/etc., or names in the source? I think you mean the
> latter but I just want to check.

Yes, it is the latter.

[ ... ]

> > > +#include <asm/io.h>
> > > +#include <asm/irq.h>
> > > +#include <asm/rtc.h>
> > 
> > Are all these headers really needed ?
> 
> I'll re-check. Sorry I didn't notice the list had gotten so long. This
> code evolved from a pre-DT board file that also had interrupt
> controller and experimental SMP stuff in it and I never went back to
> check what was really needed.
> 
> > > +static unsigned char __iomem *pit_base;
> > 
> > s/unsigned char/void/
> 
> OK. I cringe at doing arithmetic on void*, but that seems to be the
> convention in the kernel.
> 
> > > +static int pit_irq;
> > > +static u32 percpu_offset;
> > > +static u32 enable_val;
> > > +static struct clock_event_device __percpu *pit_percpu;
> > 
> > Are the clockevents per cpu on the J2 ?
> 
> Yes. There's an instance of the PIT (which is actually attached to an
> interrupt controller in the hardware) for each cpu. The smp support is
> not in this patch series since it's still new and there's no public
> board-target suporting smp, but there will be soon.
> 
> > Is there any documentation 
> > describing this hardware I can refer to ?
> 
> There's the vhdl source. I'm not sure what else is publicly available
> right now; I'll check on it for you.

Great, thank you.

[ ... ]

> > s/1000000000/NSEC_PER_SEC/
> 
> OK.
> 
> > > +}
> > 
> > Do you really, really, want to use the 64bits ? There is no real benefit and 
> > it has a significant overhead. The impact on a j-core could be really not 
> > negligible IMHO.
> 
> With just 32-bit, there's no way the cpu could sleep more than 4
> seconds without time desynchronization. 

What do you mean by time 'desynchronization' ?

> Right now it doesn't seem to
> do that anyway (max seems like abou 1-1.5 sec) but it feels like a
> shame to preclude it. I agree there's some serious cost to the 64-bit
> arithmetic though to weigh in. One option would be trying to avoid
> using the high part of seconds, but I think this hits a (admittedly
> largely theoretical) discontinuity in the resulting nanosecond count
> when seclo overflows.

IIUC your comment, the timekeeping layer is supposed to compensate, that's 
why the .mask below is for. It could be a good idea to benchmark 32/64b and 
then take a decision from there.

[ ... ]

> > > +static int pit_set(unsigned long delta, struct clock_event_device 
> > > *ced)
> > > +{
> > > +	unsigned cpu = smp_processor_id();
> > > +
> > > +	pit_disable(ced);
> > 
> > Move out the function pit_disabled().
> 
> I don't understand what you're asking for here. The "throttle"
> register is only programmable when the enable bit is clear, so disable
> has to be called before setting the timer. Are you saying there should
> be a separate disable "helper function" from the function whose
> address is stored in the clockevent device set_state_shutdown pointer?
> 
> > > +	writel(delta, pit_base + percpu_offset*cpu + REG_THROT);
> > > +	writel(enable_val, pit_base + percpu_offset*cpu + REG_PITEN);
> > 
> > Write an enable function and move it out.
> 
> So the set function should call pit_disable, then write the throttle
> register, then call pit_enable?

What I meant is to create simple and trivial functions, like foo_set, 
foo_enable, foo_disable, ... so we can call them from set_periodic, 
set_next_event, ...

[ ... ]

> > static void pit_set_periodic(struct clock_event_device *ced)
> > {
> > 	__iomem void *addr = to_jcore_clkevt(ced)->addr;
> > 	unsigned long period = readl(addr + REG_BUSPD);
> > 
> > 	pit_disable(addr);
> > 	pit_set(period, addr);
> > 	pit_enabled(addr);
> > }
> > 
> > static void pit_set_next_event(unsigned long delta, struct 
> > clock_event_device *ced)
> > {
> > 	__iomem void *addr = to_jcore_clkevt(ced)->addr;
> > 
> > 	pit_disable(addr);
> > 	pit_set(delta, addr);
> > 	pit_enable(addr);
> > }
> > 
> > (jcore_set_next_event, jcore_set_periodic)
> 
> OK.
> 
> > > +	clockevents_config_and_register(ced, DIV_ROUND_CLOSEST(1000000000, per),
> > > +	                                1, 0xffffffff);
> > > +
> > > +	pit_set_periodic(ced);
> > 
> > It is up to the time framework to set this.
> 
> To call the set_periodic function? OK.
> 
> > I don't see enable_percpu_irq / disable_percpu_irq in this driver.
> 
> I was unable to get the percpu irq framework to work correctly with my
> driver for the interrupt controller. Looking at some other irqchip
> drivers now, it seems they have to call irq_set_percpu_devid from the
> domain mapping function (or somewhere) exactly once, but I don't see
> any good way to know whether to do this. In principle at the hw level
> all irqs are percpu, but most peripherals' irq lines are only wired to
> cpu0.

Mmh, that may need further investigation. Does the j2core-stable kernel work 
on the numato ?

> > > +	return 0;
> > > +}
> > > +
> > > +static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> > > +{
> > > +	switch (action & ~CPU_TASKS_FROZEN) {
> > > +	case CPU_STARTING:
> > > +		pit_local_init(this_cpu_ptr(pit_percpu));
> > > +		break;
> > > +	}
> > 
> > DYING is missing.
> 
> Does it need to unregister the device?

No. In case of SMP (which I understood it is not yet ready), we should 
enable/disable percpu irq. And thinking beyond for power management, if the 
timer does not belong to cpu power domain, the local timer should be powered 
down (like the exynos_mct). May be it goes too far, but it is for the 
record.

At least, disable percpu irq should be added but the percpu API seems to not 
work and SMP does not exists yet, so let's forget this for the moment.
 
[ ... ]

> > > +	clocksource_register_hz(&rtc_csd, 1000000000);
> > 
> > I suggest the rate to be passed through the DT.
> 
> Normally I would agree, the units of the hw registers are seconds and
> nanoseconds. I don't see any circumstance under which it would make
> sense to change the value here without a different hw register
> interface.

Isn't it possible the clock provider could be different for the timer on 
different SoC ?

> > > +	pit_percpu = alloc_percpu(struct clock_event_device);
> > > +	register_cpu_notifier(&pit_cpu_nb);
> > > +
> > > +	err = request_irq(pit_irq, timer_interrupt,
> > > +		IRQF_TIMER | IRQF_PERCPU, "pit", pit_percpu);
> > 
> > So, we have per CPU private IRQ with the same number, right?
> 
> Yes.
> 
> > You should use the 'percpu' API.
> > 
> >  - request_percpu_irq
> >  - enable_percpu_irq
> >  - disable_percpu_irq
> 
> Still trying to figure out how to make that work.
> 
> > The interrupt callback will have the right percpu dev_id parameter pointing 
> > to the right percpu structure. So you should not have to play with 
> > this_cpu_ptr anywhere.
> > 
> > > +	if (err) pr_err("pit irq request failed: %d\n", err);
> > 
> > CR before pr_err.
> 
> OK.
> 
> > > +	hwirq = irq_get_irq_data(pit_irq)->hwirq;
> > > +	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);
> > 
> > Can you explain? (and replace litterals by macros).
> 
> Since the PIT is actually integrated with the interrupt controller in
> the hardware, it's not tied to a fixed IRQ; it's programmable. However
> I don't know any way to represent "driver can use any irq it wants but
> should avoid sharing" in the DT, so I opted to have the DT just assign
> one. 

Sorry, I don't get the point. IIUC, the timers have private IRQ lines,
why can't they describe their own IRQs even they are the same number ?

> The above code programs it. Bit 26 is the actual enable bit, and
> the actual hw irq number (which conceptually need not match the virq
> number, although it does) gets programmed in a way thats compatible
> with the programmable interrupt priority stuff aic1 did (generating an
> appropriate priority matching what you'd get with aic2). I have
> unpolished specs from one of our hw engineers that I could review and
> see if this could be simplified/clarified.

Ok. I am not 100% sure but I think there is a glitch with the code above. I 
suspect something is missing in the irq driver.

> > > +	pit_local_init(this_cpu_ptr(pit_percpu));
> > > +}
> > 
> > I am wondering if the j2 is subject to change. It is now designable through 
> > FPGA and I imagine the design can go back and forth, no? If yes (and that's 
> > good), wouldn't make sense to make this driver (and others) highly 
> > configurable via the DT, much more than what there is now in order to 
> > prevent errata and kernel changes?
> 
> It's hard to predict exactly what might change or how it might change,
> which is why I went to the effort to redo all our old drivers in a DT
> framework. The current "jcore,pit" binding is intended only for things
> sufficiently compatible with the current programming interface work
> with drivers (or bare-metal software) written for the current
> programming interface. It's expected that we'll add new compatible
> tags if/when changes have to be made, and then the updated driver can
> use whatever new generality is needed with the new compatible tag, or
> keep working the same as it does now (possibly via more general code
> with appropriate parameter values) when the old compatible tag is
> used.

Ok, thanks.

  -- Daniel

Re: [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver

[Rich Felker]

On Mon, May 23, 2016 at 10:32:35PM +0200, Daniel Lezcano wrote:
> On Fri, May 20, 2016 at 11:15:16PM -0400, Rich Felker wrote:
> > On Fri, May 20, 2016 at 04:01:56PM +0200, Daniel Lezcano wrote:
> > > On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > > > Signed-off-by: Rich Felker <dalias@libc.org>
> > > > ---
> > > 
> > > Hi Rich,
> > > 
> > > please add a nice changelog describing how works the timer.
> > 
> > OK. Do you prefer this in changelog, comments in the source, or both?
> 
> I prefer a [detailed] description of the timer in the changelog or the file 
> header. As the timer is trivial for now, I don't see the benefit of adding 
> too much comments in the code.

OK, my question was just changelog (commit message) vs block comment
in file header. I tend to prefer commit messages because they're
linked to a point in time and don't run the risk of getting out of
sync with the file contents as changes are made, but I can do
whichever you prefer.

BTW if we get this in a form you're ok with, can you ack it to be
pulled as part of my tree (sh arch) or do you want it to go through
your channel to upstream?

> > > Let the platform's Kconfig to select the timer. Beside, check the timer 
> > > compiles correctly on other platform (eg. x86) for compilation test 
> > > coverage.
> > 
> > So the prompt would not even appear unless COMPILE_TEST is selected? 
> 
> Correct.
> 
> > I don't mind doing it that way if this is the established best practice.
> > For clocksource/clockevent, the system isn't even usable without it,
> > so there's little harm in having CONFIG_CPU_J2 select CLKSRC_JCORE_PIT
> > for the user. On the other hand, some of the drivers like the SPI
> > master, (future) EMAC, etc. are things you might want to be able to
> > turn off to build a size-optimized kernel for a system that doesn't
> > need (or doesn't even have) them, so this approach does not seem to
> > make sense for other such drivers.
> > 
> > My main theoretical concern here is what happens if someone uses the
> > J2 cpu core with completely different SoC peripherals hooked up to it,
> > and doesn't want to be forced to build the jcore-pit driver. Is this
> > type of thing an issue people have thought about and reached a
> > canonical solution to?
> 
> In this case it is the SoC's Kconfig which is in charge of setting the right 
> parameter not the CPU's Kconfig option. This is what we find in the ARM 
> configs file where the ARMv7 is the "CPU" but the SoC is different.

My intent in overhauling arch/sh to device tree is not to have
SoC-specific kernels, but to be able to make a single kernel that
works on any SoC with matching cpu isa. Not being able to select the
drivers needed without they getting implicitly selected via a
SoC-specific Kconfig option rather undermines that.

It's certainly plausible to have a user-configurable arch/sh Kconfig
option "Include J-Core SoC drivers" that selects all of the SoC
peripheral drivers, but that doesn't seem like the proper modern way
of doing things...

> But if these timers are local to the CPU, there is a good chance nobody will 
> change that as they are the most efficient and certainly well fitted for 
> power management. I believe the timer supposed to work as the broadcast 
> timer is more subject to change.
> 
> IMO, some clarifications about the CPU design and these timers may be 
> needed. If the local timers are specified as part of the CPU, then 
> CONFIG_CPU_J2 => CONFIG_JCORE_PIT, otherwise CONFIG_SOC_J2_BASED => 
> CONFIG_CPU_J2 [+ CONFIG_JCORE_PIT].

In our hw abstraction, almost nothing is considered "part of the cpu".
Even things like the "L1" cache controller are modelled as DT nodes.
This makes it possible to do custom stripped-down SoCs etc. or to
experiment with completely different designs for one or more
components while reusing everything else.

> > > > +/*
> > > > + * J-Core SoC PIT/RTC driver
> > > 
> > > Is it really RTC ? :)
> > 
> > That's the name used in the hardware. It's a settable clock counting
> > seconds/nanoseconds and it's the most appropriate thing the hardware
> > has for a clocksource device. The old kernel patches that existed when
> > I took over were not using it and instead used jiffies and a PIT
> > register that returned ns since the last timer interrupt, which is now
> > unused (that approach precluded dyntick).
> 
> Ok, just for clarification. From Linux, a RTC is different from a 
> clocksource. The former is backed up with a battery, relies almost always on 
> a quartz and populates /dev/rtc, the latter is based on a clock (which can 
> vary) and is volatile. Naming it RTC is confusing as it is a clocksource 
> (and RTC falls under drivers/rtc).

Yes, I'll try to come up with a name that's less confusing in a Linux
context but still makes sense with the hardware source.

> > > Is there any documentation 
> > > describing this hardware I can refer to ?
> > 
> > There's the vhdl source. I'm not sure what else is publicly available
> > right now; I'll check on it for you.
> 
> Great, thank you.
> 
> [ ... ]
> 
> > > s/1000000000/NSEC_PER_SEC/
> > 
> > OK.
> > 
> > > > +}
> > > 
> > > Do you really, really, want to use the 64bits ? There is no real benefit and 
> > > it has a significant overhead. The impact on a j-core could be really not 
> > > negligible IMHO.
> > 
> > With just 32-bit, there's no way the cpu could sleep more than 4
> > seconds without time desynchronization. 
> 
> What do you mean by time 'desynchronization' ?

What I meant was that, if you sleep (no timer interrupts) for 10s, and
the clocksource count is 32-bit, then after waking up you can't
distinguish between the following 3 cases:

- 10s elapsed
- 5.705032704s elapsed
- 1.410065408s elapsed

Note that you can't just cheat by knowing how long you programmed the
clock event device to sleep for, because another interrupt can be what
wakes the cpu up from sleeping. So it seems like 32-bit clocksource
precludes long sleeps.

Like I said, though, I don't think the kernel even tries to do them
now, so it's probably a non-issue at least in the short term.

> What I meant is to create simple and trivial functions, like foo_set, 
> foo_enable, foo_disable, ... so we can call them from set_periodic, 
> set_next_event, ...

OK.

> > > I don't see enable_percpu_irq / disable_percpu_irq in this driver.
> > 
> > I was unable to get the percpu irq framework to work correctly with my
> > driver for the interrupt controller. Looking at some other irqchip
> > drivers now, it seems they have to call irq_set_percpu_devid from the
> > domain mapping function (or somewhere) exactly once, but I don't see
> > any good way to know whether to do this. In principle at the hw level
> > all irqs are percpu, but most peripherals' irq lines are only wired to
> > cpu0.
> 
> Mmh, that may need further investigation.

Yeah. It's really confusing to me. The kernel's percpu irq stuff seems
like it has a strong assumption that an irq being percpu or not is a
property of the interrupt controller hardware rather than how it's
configured for the device attached to the irq. But it works fine
without using that layer in practice, just using normal percpu storage
allocation and the percpu irq flag and/or flags to request a hard irq
handler rather than tasklet or whatever (so that the handler
necessarily runs on the cpu the irq was delivered to).

> Does the j2core-stable kernel work 
> on the numato ?

Which kernel are you calling j2core-stable? With the patch series
under discussion right now, it works on the numato. You need at least
the config options from j2_defconfig and the SD card you boot from
needs both the vmlinux and a dt.dtb built from j2_mimas_v2.dts (also
provided in this patch series). Future versions of the FPGA bitstream
(once the DT bindings are approved) will include the DTB in the boot
rom and won't need a dt.dtb on the SD card.

> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)
> > > > +{
> > > > +	switch (action & ~CPU_TASKS_FROZEN) {
> > > > +	case CPU_STARTING:
> > > > +		pit_local_init(this_cpu_ptr(pit_percpu));
> > > > +		break;
> > > > +	}
> > > 
> > > DYING is missing.
> > 
> > Does it need to unregister the device?
> 
> No. In case of SMP (which I understood it is not yet ready), we should 
> enable/disable percpu irq. And thinking beyond for power management, if the 
> timer does not belong to cpu power domain, the local timer should be powered 
> down (like the exynos_mct). May be it goes too far, but it is for the 
> record.
> 
> At least, disable percpu irq should be added but the percpu API seems to not 
> work and SMP does not exists yet, so let's forget this for the moment.

SMP has been working fine for several months on SEI's commercial
hardware using J2, and on the prototype Turtle boards we just got. The
main reasons I've held off from including SMP in this patch series are
that it's a bit more invasive in otherwise-unrelated arch/sh files,
and that it doesn't really make sense to put something in the upstream
kernel when there's no publicly available hardware that you can use it
on.

> > > > +	clocksource_register_hz(&rtc_csd, 1000000000);
> > > 
> > > I suggest the rate to be passed through the DT.
> > 
> > Normally I would agree, the units of the hw registers are seconds and
> > nanoseconds. I don't see any circumstance under which it would make
> > sense to change the value here without a different hw register
> > interface.
> 
> Isn't it possible the clock provider could be different for the timer on 
> different SoC ?

What I'm saying is that the 3 registers being read to produce the
counter are nominally seconds-hi, seconds-lo, and nanoseconds. While I
see how clocksource base could be considered a parameter that can vary
if there were just one hw register that was the counter, I don't see
how anything but NSEC_PER_SEC makes sense as a base with this register
interface.

> > > > +	hwirq = irq_get_irq_data(pit_irq)->hwirq;
> > > > +	enable_val = (1<<26) | ((hwirq&0x3c)<<18) | (hwirq<<12);
> > > 
> > > Can you explain? (and replace litterals by macros).
> > 
> > Since the PIT is actually integrated with the interrupt controller in
> > the hardware, it's not tied to a fixed IRQ; it's programmable. However
> > I don't know any way to represent "driver can use any irq it wants but
> > should avoid sharing" in the DT, so I opted to have the DT just assign
> > one. 
> 
> Sorry, I don't get the point. IIUC, the timers have private IRQ lines,
> why can't they describe their own IRQs even they are the same number ?
> 
> > The above code programs it. Bit 26 is the actual enable bit, and
> > the actual hw irq number (which conceptually need not match the virq
> > number, although it does) gets programmed in a way thats compatible
> > with the programmable interrupt priority stuff aic1 did (generating an
> > appropriate priority matching what you'd get with aic2). I have
> > unpolished specs from one of our hw engineers that I could review and
> > see if this could be simplified/clarified.
> 
> Ok. I am not 100% sure but I think there is a glitch with the code above. I 
> suspect something is missing in the irq driver.

I don't follow. The code is working as-is. All it does is program the
PIT to generate the interrupt that the DT says it generates, and to
generate it on a nonzero priority. I'll break down the computation
into a form that makes sense, though.

At this point I'm about done with the changes you requested short of
using the percpu irq stuff (that doesn't seem to work), as well as
changes requested by reviewers of the other patches in the series.
I'll post a v3 patchset after reviewing it all tomorrow.

Rich

[PATCH v2 04/12] of: add J-Core timer bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 .../devicetree/bindings/timer/jcore,pit.txt        | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt

diff --git a/Documentation/devicetree/bindings/timer/jcore,pit.txt b/Documentation/devicetree/bindings/timer/jcore,pit.txt
new file mode 100644
index 0000000..d53759a
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
@@ -0,0 +1,28 @@
+J-Core Programmable Interval Timer and Realtime Clock
+
+Required properties:
+
+- compatible: Must be "jcore,pit".
+
+- reg: Memory region for timer/rtc registers.
+
+- interrupts: An interrupt to assign for the timer. The actual pit
+  core is integrated with the aic and allows the timer interrupt
+  assignment to be programmed by software, but this property is
+  required in order to reserve an interrupt number that doesn't
+  conflict with other devices.
+
+Optional properties:
+
+- cpu-offset: For SMP, the per-cpu offset to the local timer
+  programming memory range.
+
+
+Example:
+
+timer {
+	compatible = "jcore,pit";
+	reg = < 0x200 0x30 >;
+	cpu-offset = < 0x300 >;
+	interrupts = < 0x48 >;
+};
-- 
2.8.1

Re: [PATCH v2 04/12] of: add J-Core timer bindings

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> +++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
> @@ -0,0 +1,28 @@

> +Example:
> +
> +timer {

timer@200

> +       compatible = "jcore,pit";
> +       reg = < 0x200 0x30 >;
> +       cpu-offset = < 0x300 >;
> +       interrupts = < 0x48 >;
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 12/12] sh: add device tree source for J2 FPGA on Mimas v2 board

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 arch/sh/boot/dts/j2_mimas_v2.dts | 87 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100755 arch/sh/boot/dts/j2_mimas_v2.dts

diff --git a/arch/sh/boot/dts/j2_mimas_v2.dts b/arch/sh/boot/dts/j2_mimas_v2.dts
new file mode 100755
index 0000000..f47fb2f
--- /dev/null
+++ b/arch/sh/boot/dts/j2_mimas_v2.dts
@@ -0,0 +1,87 @@
+/dts-v1/;
+
+/ {
+	compatible = "jcore,j2-soc";
+	model = "J2 FPGA SoC on Mimas v2 board";
+
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	interrupt-parent = <&aic>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "jcore,j2";
+			reg = < 0 >;
+			clock-frequency = < 50000000 >;
+		};
+	};
+
+	memory@10000000 {
+		device_type = "memory";
+		reg = < 0x10000000 0x4000000 >;
+	};
+
+	chosen {
+		stdout-path = "/soc@abcd0000/serial@100";
+	};
+
+	soc@abcd0000 {
+		compatible = "simple-bus";
+		ranges = <0 0xabcd0000 0x100000>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		aic: interrupt-controller {
+			compatible = "jcore,aic1";
+			reg = < 0x200 0x10 >;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		cache-controller {
+			compatible = "jcore,cache";
+			reg = < 0xc0 4 >;
+		};
+
+		timer {
+			compatible = "jcore,pit";
+			reg = < 0x200 0x30 >;
+			interrupts = < 0x48 >;
+		};
+
+		spi {
+			compatible = "jcore,spi2";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			spi-max-frequency = <12500000>;
+
+			reg = < 0x40 0x8 >;
+
+			sdcard@1 {
+				compatible = "mmc-spi-slot";
+				reg = <0>;
+				spi-max-frequency = <12500000>;
+				voltage-ranges = <3200 3400>;
+				mode = <0>;
+			};
+		};
+
+		serial@100 {
+			clock-frequency = <125000000>;
+			compatible = "xlnx,xps-uartlite-1.00.a";
+			current-speed = <19200>;
+			device_type = "serial";
+			interrupts = < 0x12 >;
+			port-number = <0>;
+			reg = < 0x100 0x10 >;
+		};
+	};
+};
-- 
2.8.1

Re: [PATCH v2 12/12] sh: add device tree source for J2 FPGA on Mimas v2 board

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> --- /dev/null
> +++ b/arch/sh/boot/dts/j2_mimas_v2.dts
> @@ -0,0 +1,87 @@
> +/dts-v1/;
> +
> +/ {
> +       compatible = "jcore,j2-soc";
> +       model = "J2 FPGA SoC on Mimas v2 board";
> +
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       interrupt-parent = <&aic>;
> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       device_type = "cpu";
> +                       compatible = "jcore,j2";
> +                       reg = < 0 >;
> +                       clock-frequency = < 50000000 >;
> +               };
> +       };
> +
> +       memory@10000000 {
> +               device_type = "memory";
> +               reg = < 0x10000000 0x4000000 >;
> +       };
> +
> +       chosen {
> +               stdout-path = "/soc@abcd0000/serial@100";
> +       };
> +
> +       soc@abcd0000 {
> +               compatible = "simple-bus";
> +               ranges = <0 0xabcd0000 0x100000>;
> +
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               aic: interrupt-controller {

aic: interrupt-controller@200 {

> +                       compatible = "jcore,aic1";
> +                       reg = < 0x200 0x10 >;
> +                       interrupt-controller;
> +                       #interrupt-cells = <1>;
> +               };
> +
> +               cache-controller {

@c0

> +                       compatible = "jcore,cache";
> +                       reg = < 0xc0 4 >;
> +               };
> +
> +               timer {

@200

> +                       compatible = "jcore,pit";
> +                       reg = < 0x200 0x30 >;
> +                       interrupts = < 0x48 >;
> +               };
> +
> +               spi {

@40

> +                       compatible = "jcore,spi2";
> +
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       spi-max-frequency = <12500000>;
> +
> +                       reg = < 0x40 0x8 >;
> +
> +                       sdcard@1 {

@0, to match reg below?

> +                               compatible = "mmc-spi-slot";
> +                               reg = <0>;
> +                               spi-max-frequency = <12500000>;
> +                               voltage-ranges = <3200 3400>;
> +                               mode = <0>;
> +                       };
> +               };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 12/12] sh: add device tree source for J2 FPGA on Mimas v2 board

[Rich Felker]

On Fri, May 20, 2016 at 10:17:34AM +0200, Geert Uytterhoeven wrote:
> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> > --- /dev/null
> > +++ b/arch/sh/boot/dts/j2_mimas_v2.dts
> > @@ -0,0 +1,87 @@
> > +/dts-v1/;
> > +
> > +/ {
> > +       compatible = "jcore,j2-soc";
> > +       model = "J2 FPGA SoC on Mimas v2 board";
> > +
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       interrupt-parent = <&aic>;
> > +
> > +       cpus {
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               cpu@0 {
> > +                       device_type = "cpu";
> > +                       compatible = "jcore,j2";
> > +                       reg = < 0 >;
> > +                       clock-frequency = < 50000000 >;
> > +               };
> > +       };
> > +
> > +       memory@10000000 {
> > +               device_type = "memory";
> > +               reg = < 0x10000000 0x4000000 >;
> > +       };
> > +
> > +       chosen {
> > +               stdout-path = "/soc@abcd0000/serial@100";
> > +       };
> > +
> > +       soc@abcd0000 {
> > +               compatible = "simple-bus";
> > +               ranges = <0 0xabcd0000 0x100000>;
> > +
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +
> > +               aic: interrupt-controller {
> 
> aic: interrupt-controller@200 {
> 
> > +                       compatible = "jcore,aic1";
> > +                       reg = < 0x200 0x10 >;
> > +                       interrupt-controller;
> > +                       #interrupt-cells = <1>;
> > +               };
> > +
> > +               cache-controller {
> 
> @c0
> 
> > +                       compatible = "jcore,cache";
> > +                       reg = < 0xc0 4 >;
> > +               };
> > +
> > +               timer {
> 
> @200
> 
> > +                       compatible = "jcore,pit";
> > +                       reg = < 0x200 0x30 >;
> > +                       interrupts = < 0x48 >;
> > +               };
> > +
> > +               spi {
> 
> @40
> 
> > +                       compatible = "jcore,spi2";
> > +
> > +                       #address-cells = <1>;
> > +                       #size-cells = <0>;
> > +
> > +                       spi-max-frequency = <12500000>;
> > +
> > +                       reg = < 0x40 0x8 >;
> > +
> > +                       sdcard@1 {
> 
> @0, to match reg below?

Yes; thanks for catching that. The chipselect logic was wrong a long
time ago and the wrong setting to compensate for that was fixed in the
actual reg cell but not in the name when the driver was fixed. I'm
applying all these changes.

Rich

[PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
My previous post of the patch series accidentally omitted omitted
Cc'ing of subsystem maintainers for the necessary clocksource,
irqchip, and spi drivers. Please ack if this looks ok because I want
to get it merged as part of the arch/sh pull request for 4.7.

 drivers/spi/Kconfig     |   4 +
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-jcore.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 drivers/spi/spi-jcore.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9d8c84b..5bd2ccf 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -285,6 +285,10 @@ config SPI_IMX
 	  This enables using the Freescale i.MX SPI controllers in master
 	  mode.
 
+config SPI_JCORE
+	tristate "J-Core SPI Master"
+	depends on OF
+
 config SPI_LM70_LLP
 	tristate "Parallel port adapter for LM70 eval board (DEVELOPMENT)"
 	depends on PARPORT
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index fbb255c..6a34124 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_FSL_SPI)		+= spi-fsl-spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi-gpio.o
 obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
+obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_LP8841_RTC)		+= spi-lp8841-rtc.o
 obj-$(CONFIG_SPI_MESON_SPIFC)		+= spi-meson-spifc.o
diff --git a/drivers/spi/spi-jcore.c b/drivers/spi/spi-jcore.c
new file mode 100644
index 0000000..552304c
--- /dev/null
+++ b/drivers/spi/spi-jcore.c
@@ -0,0 +1,266 @@
+/*
+ * J-Core SPI controller driver
+ *
+ * Copyright (C) 2012-2016 SEI Inc.
+ *
+ * Current version by Rich Felker
+ * Based loosely on initial version by Oleksandr G Zhadan
+ *
+ */
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/delay.h>
+
+#define USE_MESSAGE_MODE 1
+
+#define DRV_NAME "jcore_spi"
+
+#define MAX_SPI_SPEED			12500000	/* 12.5 MHz */
+
+#define CTRL_REG		0x0
+#define DATA_REG		0x4
+
+#define SPI_NOCHIP_CS	0
+#define SPI_FLASH_CS	1
+#define SPI_CONF_CS	2
+#define SPI_SD_CS	2
+#define SPI_CODEC_CS	3
+
+#define JCORE_SPI_CTRL_ACS		0x01
+#define JCORE_SPI_CTRL_XMIT		0x02
+#define JCORE_SPI_STAT_BUSY		0x02
+#define JCORE_SPI_CTRL_CCS		0x04
+#define JCORE_SPI_CTRL_LOOP		0x08
+#define JCORE_SPI_CTRL_DCS		0x10
+
+#define JCORE_SPI_WAIT_RDY_MAX_LOOP	2000000	/* in usec */
+
+struct jcore_spi {
+	struct spi_master *master;
+	void __iomem *base;
+	volatile unsigned int ctrlReg;
+	unsigned int csReg;
+	unsigned int speedReg;
+	unsigned int speed_hz;
+};
+
+static void jcore_spi_wait_till_ready(struct jcore_spi *hw, int timeout)
+{
+	while (timeout--) {
+		hw->ctrlReg = readl(hw->base + CTRL_REG);
+		if (!(hw->ctrlReg & JCORE_SPI_STAT_BUSY))
+			return;
+		cpu_relax();
+	}
+	pr_err("%s: Timeout..\n", __func__);
+}
+
+static void jcore_spi_program(struct jcore_spi *hw)
+{
+	jcore_spi_wait_till_ready(hw, JCORE_SPI_WAIT_RDY_MAX_LOOP);
+	writel(hw->csReg | hw->speedReg, hw->base + CTRL_REG);	
+}
+
+static void jcore_spi_chipsel(struct spi_device *spi, bool value)
+{
+	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
+
+	pr_debug("%s: CS=%d\n", __func__, value);
+
+	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
+		^ (!value << 2*spi->chip_select);
+
+	jcore_spi_program(hw);
+}
+
+static void jcore_spi_baudrate(struct jcore_spi *hw, int speed)
+{
+	if (speed == hw->speed_hz) return;
+	hw->speed_hz = speed;
+	hw->speedReg = ((MAX_SPI_SPEED / speed) - 1) << 27;
+	jcore_spi_program(hw);
+	pr_debug("%s: speed=%d pre=0x%x\n", __func__, speed, hw->speedReg);
+}
+
+static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
+{
+	struct jcore_spi *hw = spi_master_get_devdata(master);
+
+	void *ctrl_reg = hw->base + CTRL_REG;
+	void *data_reg = hw->base + DATA_REG;
+	int timeout;
+	int xmit;
+	int status;
+
+	/* data buffers */
+	const unsigned char *tx;
+	unsigned char *rx;
+	int len;
+	int count;
+
+	jcore_spi_baudrate(hw, t->speed_hz);
+
+	xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
+	tx = t->tx_buf;
+	rx = t->rx_buf;
+	len = t->len;
+
+	for (count = 0; count < len; count++) {
+		timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
+		do status = readl(ctrl_reg);
+		while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
+		if (!timeout) break;
+
+		writel(tx ? *tx++ : 0, data_reg);
+		writel(xmit, ctrl_reg);
+
+		timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
+		do status = readl(ctrl_reg);
+		while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
+		if (!timeout) break;
+
+		if (rx) *rx++ = readl(data_reg);
+	}
+
+#if !USE_MESSAGE_MODE
+	spi_finalize_current_transfer(master);
+#endif
+
+	return count<len ? -EREMOTEIO : 0;
+}
+
+#if USE_MESSAGE_MODE
+static int jcore_spi_transfer_one_message(struct spi_master *master,
+					struct spi_message *msg)
+{
+	struct spi_device *spi = msg->spi;
+	struct spi_transfer *xfer;
+	bool keep_cs = false;
+	int ret = 0;
+
+	jcore_spi_chipsel(spi, false);
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		ret = jcore_spi_txrx(master, spi, xfer);
+		if (ret) break;
+		if (xfer->delay_usecs)
+			udelay(xfer->delay_usecs);
+		if (xfer->cs_change) {
+			if (list_is_last(&xfer->transfer_list,
+					 &msg->transfers)) {
+				keep_cs = true;
+			} else {
+				jcore_spi_chipsel(spi, true);
+				udelay(10);
+				jcore_spi_chipsel(spi, false);
+			}
+		}
+		msg->actual_length += xfer->len;
+	}
+
+	if (!keep_cs)
+		jcore_spi_chipsel(spi, true);
+
+	msg->status = ret;
+
+	spi_finalize_current_message(master);
+
+	return ret;
+}
+#endif
+
+static int jcore_spi_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct jcore_spi *hw;
+	struct spi_master *master;
+	struct resource *res;
+	int err = -ENODEV;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(struct jcore_spi));
+	if (!master)
+		return err;
+
+	/* setup the master state. */
+	master->num_chipselect = 3;
+	master->mode_bits = SPI_MODE_3;
+#if USE_MESSAGE_MODE
+	master->transfer_one_message = jcore_spi_transfer_one_message;
+#else
+	master->transfer_one = jcore_spi_txrx;
+#endif
+	master->set_cs = jcore_spi_chipsel;
+	master->dev.of_node = node;
+
+	hw = spi_master_get_devdata(master);
+	hw->master = master;
+	platform_set_drvdata(pdev, hw);
+
+	/* find and map our resources */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		goto exit_busy;
+	if (!devm_request_mem_region
+	    (&pdev->dev, res->start, resource_size(res), pdev->name))
+		goto exit_busy;
+	hw->base =
+	    devm_ioremap_nocache(&pdev->dev, res->start, resource_size(res));
+	if (!hw->base)
+		goto exit_busy;
+
+	jcore_spi_baudrate(hw, 400000);
+
+	pdev->dev.dma_mask = 0;
+	/* register our spi controller */
+	err = spi_register_master(master);
+	if (err)
+		goto exit;
+	dev_info(&pdev->dev, "base %p, noirq\n", hw->base);
+
+	return 0;
+
+exit_busy:
+	err = -EBUSY;
+exit:
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(master);
+	return err;
+}
+
+static int jcore_spi_remove(struct platform_device *dev)
+{
+	struct jcore_spi *hw = platform_get_drvdata(dev);
+	struct spi_master *master = hw->master;
+
+	platform_set_drvdata(dev, NULL);
+	spi_master_put(master);
+	return 0;
+}
+
+static const struct of_device_id jcore_spi_of_match[] = {
+	{ .compatible = "jcore,spi2" },
+	{},
+};
+
+static struct platform_driver jcore_spi_driver = {
+	.probe = jcore_spi_probe,
+	.remove = jcore_spi_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.pm = NULL,
+		.of_match_table = jcore_spi_of_match,
+	},
+};
+
+module_platform_driver(jcore_spi_driver);
+
+MODULE_DESCRIPTION("J-Core SPI driver");
+MODULE_AUTHOR("Rich Felker <dalias@libc.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.8.1

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> --- /dev/null
> +++ b/drivers/spi/spi-jcore.c

> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> +{
> +       struct jcore_spi *hw = spi_master_get_devdata(master);
> +
> +       void *ctrl_reg = hw->base + CTRL_REG;
> +       void *data_reg = hw->base + DATA_REG;
> +       int timeout;

unsigned int

> +       int xmit;

u32

> +       int status;

u32

> +
> +       /* data buffers */
> +       const unsigned char *tx;
> +       unsigned char *rx;
> +       int len;

unsigned int

> +       int count;

unsigned int

> +
> +       jcore_spi_baudrate(hw, t->speed_hz);
> +
> +       xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
> +       tx = t->tx_buf;
> +       rx = t->rx_buf;
> +       len = t->len;
> +
> +       for (count = 0; count < len; count++) {
> +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> +               do status = readl(ctrl_reg);
> +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);

do {
        ...
} while (...)

> +               if (!timeout) break;

if (...)
        ...

> +
> +               writel(tx ? *tx++ : 0, data_reg);

You can remove the check for tx if you set the SPI_MASTER_MUST_TX
flag in spi_master.flags.

> +               writel(xmit, ctrl_reg);
> +
> +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> +               do status = readl(ctrl_reg);
> +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);

do {
        ...
} while (...)

> +               if (!timeout) break;


if (...)
        ...

> +
> +               if (rx) *rx++ = readl(data_reg);


if (...)
        ...

> +       }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Fri, May 20, 2016 at 10:15:08AM +0200, Geert Uytterhoeven wrote:
> On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> > --- /dev/null
> > +++ b/drivers/spi/spi-jcore.c
> 
> > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> > +{
> > +       struct jcore_spi *hw = spi_master_get_devdata(master);
> > +
> > +       void *ctrl_reg = hw->base + CTRL_REG;
> > +       void *data_reg = hw->base + DATA_REG;
> > +       int timeout;
> 
> unsigned int

All of these have value ranges where the type is irrelevant, but I'm
happy to change it to whatever types you prefer.

> > +       jcore_spi_baudrate(hw, t->speed_hz);
> > +
> > +       xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT;
> > +       tx = t->tx_buf;
> > +       rx = t->rx_buf;
> > +       len = t->len;
> > +
> > +       for (count = 0; count < len; count++) {
> > +               timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP;
> > +               do status = readl(ctrl_reg);
> > +               while ((status & JCORE_SPI_STAT_BUSY) && --timeout);
> 
> do {
>         ...
> } while (...)
> 
> > +               if (!timeout) break;
> 
> if (...)
>         ...

OK. (for this and other instances)

> > +
> > +               writel(tx ? *tx++ : 0, data_reg);
> 
> You can remove the check for tx if you set the SPI_MASTER_MUST_TX
> flag in spi_master.flags.

I don't want to do that, because the new version of the hardware
that's going to support DMA can only do unidirectional DMA, and thus
we need to be able to see if either tx or rx is null.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.

This is *extremely* late for a first posting of a driver for v4.7 (you
missed the list as well as the maintainers).

> +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> +{
> +	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> +
> +	pr_debug("%s: CS=%d\n", __func__, value);

dev_dbg()

> +
> +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> +		^ (!value << 2*spi->chip_select);

Why the xor here and not an or?  The coding style is also weird, a mix
of extra spaces around the () and missing ones around *.  I'm finding
the intent of the code confusing here.

> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)

Coding style, please keep lines under 80 columns unless there's a good
reason.

> +#if !USE_MESSAGE_MODE
> +	spi_finalize_current_transfer(master);
> +#endif

I'm not sure what the if is about but it doesn't belong upstream, you
shouldn't be open coding bits of the framework.

> +	/* register our spi controller */
> +	err = spi_register_master(master);

devm_

> +static int jcore_spi_remove(struct platform_device *dev)
> +{
> +	struct jcore_spi *hw = platform_get_drvdata(dev);
> +	struct spi_master *master = hw->master;
> +
> +	platform_set_drvdata(dev, NULL);
> +	spi_master_put(master);
> +	return 0;
> +}

This can be removed entirely.

> +static const struct of_device_id jcore_spi_of_match[] = {
> +	{ .compatible = "jcore,spi2" },
> +	{},
> +};

This is adding a DT binding with no binding document.  All new DT
bindings need to be documented.

> +		.owner = THIS_MODULE,
> +		.pm = NULL,

No need to set either of these.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:
> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> > My previous post of the patch series accidentally omitted omitted
> > Cc'ing of subsystem maintainers for the necessary clocksource,
> > irqchip, and spi drivers. Please ack if this looks ok because I want
> > to get it merged as part of the arch/sh pull request for 4.7.
> 
> This is *extremely* late for a first posting of a driver for v4.7 (you
> missed the list as well as the maintainers).

I'm sorry for the mix-up. The kernel workflow is still fairly new to
me and I've been fighting with git format-patch/send-email and
get_maintainer.pl to get big patch series prepared the way I want and
sent to the right people/lists. I think I've got it right now though.

> > +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> > +{
> > +	struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> > +
> > +	pr_debug("%s: CS=%d\n", __func__, value);
> 
> dev_dbg()

OK.

> > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > +		^ (!value << 2*spi->chip_select);
> 
> Why the xor here and not an or?  The coding style is also weird, a mix
> of extra spaces around the () and missing ones around *.  I'm finding
> the intent of the code confusing here.

The intent is to set all chipselect bits (the 3 macro values) high
except possibly spi->chip_select. The xor is to turn off a bit, not
turn it on. &~ would also have worked; would that be more idiomatic?

Since the individual CS bits and their names in the hardware aren't
actually relevant to the driver, I'm replacing them with a single:

#define JCORE_SPI_CTRL_CS_BITS          0x15

so I can just write:

hw->csReg = JCORE_SPI_CTRL_CS_BITS ^ (!value << 2*spi->chip_select);

Does that look better, or should I opt for &~?

> > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
> 
> Coding style, please keep lines under 80 columns unless there's a good
> reason.

OK.

> > +#if !USE_MESSAGE_MODE
> > +	spi_finalize_current_transfer(master);
> > +#endif
> 
> I'm not sure what the if is about but it doesn't belong upstream, you
> shouldn't be open coding bits of the framework.

I can explain the motivation and see what you think is best to do. I'd
like to be able to provide just the transfer_one operation, and use
the generic transfer_one_message. However, at 50 MHz cpu clock, the
stats collection and other overhead in spi.c's generic
spi_transfer_one_message takes up so much time between the end of one
SD card transfer and the beginning of the next that the overall
transfer rate is something like 15-20% higher with my version.

Another consideration is that DMA support is being added to the
hardware right now, and I think we're going to want to be able to
queue up whole messages for DMA rather than just individual transfers;
in that case, spi_transfer_one_message is probably not suitable
anyway. So we'll probably have to end up having our own
transfer_one_message function anyway.

If that's not actually needed, a possible alternative for fixing the
performance problem might be adding a Kconfig option to turn off all
the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I
could work on that instead (or in addition), and I wouldn't consider
it critical to get in for this merge window.

> > +	/* register our spi controller */
> > +	err = spi_register_master(master);
> 
> devm_
> 
> > +static int jcore_spi_remove(struct platform_device *dev)
> > +{
> > +	struct jcore_spi *hw = platform_get_drvdata(dev);
> > +	struct spi_master *master = hw->master;
> > +
> > +	platform_set_drvdata(dev, NULL);
> > +	spi_master_put(master);
> > +	return 0;
> > +}
> 
> This can be removed entirely.

OK. Does using the devm register function cause removal to be handled
generically, or is there another reason it's not needed?

> > +static const struct of_device_id jcore_spi_of_match[] = {
> > +	{ .compatible = "jcore,spi2" },
> > +	{},
> > +};
> 
> This is adding a DT binding with no binding document.  All new DT
> bindings need to be documented.

The DT binding was in patch 05/12. Should linux-spi have been added to
the Cc list for it? get_maintainer.pl didn't include it.

> > +		.owner = THIS_MODULE,
> > +		.pm = NULL,
> 
> No need to set either of these.

OK.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]

On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote:
> On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:

> > This is *extremely* late for a first posting of a driver for v4.7 (you
> > missed the list as well as the maintainers).

> I'm sorry for the mix-up. The kernel workflow is still fairly new to
> me and I've been fighting with git format-patch/send-email and
> get_maintainer.pl to get big patch series prepared the way I want and
> sent to the right people/lists. I think I've got it right now though.

One question here is why this is even part of a series - it's adding a
new controller driver which wouldn't normally have any sort of direct
build or other dependency on anything else or have other things
depending on it.  Unless there are dependencies it is generally best to
send separate changes separately as far as possible so that there is no
need to worry about issues with one part of the series slowing down
other parts of the series.

If it does make sense to send a series you need to communicate what's
going on with dependencies with all the maintainers, normally at least
the cover letter if not the entire series should go to everyone.

You should never rely on get_maintainers, you need to think about what
it's telling you.  It both misses people and generates false positives
(especially when it looks at git history).  It's useful to look at since
it normally gets you most of the way there but 

> > > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > > +		^ (!value << 2*spi->chip_select);

> > Why the xor here and not an or?  The coding style is also weird, a mix
> > of extra spaces around the () and missing ones around *.  I'm finding
> > the intent of the code confusing here.

> The intent is to set all chipselect bits (the 3 macro values) high
> except possibly spi->chip_select. The xor is to turn off a bit, not
> turn it on. &~ would also have worked; would that be more idiomatic?

No, the reader has to be able to tell what the code is doing.  If this
made sense the thing to do would be to write out the logic operations
explicitly to make it clear that every step is deliberate.  However in
this case it sounds like the code is just plain buggy, though - the
driver is being asked to set a specific chip select to a specific value
but instead of just doing that it is also trying to also change some
other settings.  Don't do that, just have the function do what it was
asked.  If the calling code has problems it'll need fixing anyway and if
some feature you hadn't anticipated ends up meaning the combination of
operations makes sense then things will just work.

> > > +#if !USE_MESSAGE_MODE
> > > +	spi_finalize_current_transfer(master);
> > > +#endif

> > I'm not sure what the if is about but it doesn't belong upstream, you
> > shouldn't be open coding bits of the framework.

> I can explain the motivation and see what you think is best to do. I'd
> like to be able to provide just the transfer_one operation, and use
> the generic transfer_one_message. However, at 50 MHz cpu clock, the
> stats collection and other overhead in spi.c's generic
> spi_transfer_one_message takes up so much time between the end of one
> SD card transfer and the beginning of the next that the overall
> transfer rate is something like 15-20% higher with my version.

No, this is just not a good approach.  If the generic code isn't working
for you improve the generic code, don't just copy it and open code the
bits you like.  The reason Linux has all these great framework is that
people collaborate to make the generic code as good and fully featured
as they can rather than open coding everything in drivers.  Doing things
in drivers results in lots of code duplication which increases the cost
of maintaining things and requires that everyone waste time copying code
in order to keep the feature set consistent between drivers.

Drivers should do things specific to a given piece of hardware, anything
in the generic code should be dealt with in the generic code.

> Another consideration is that DMA support is being added to the
> hardware right now, and I think we're going to want to be able to
> queue up whole messages for DMA rather than just individual transfers;
> in that case, spi_transfer_one_message is probably not suitable
> anyway. So we'll probably have to end up having our own
> transfer_one_message function anyway.

This is again a simple generic optimisation which would be best
implemented in generic code - it's not just this device which would
benefit from the ability to coalesce compatible transfers.  There is no
reason for individual drivers to even think about such an optimisation,
the core should just be handling them a transfer with a coalesced DMA
transfer in it.  We're not doing it at present because someone needs to
take the time to write the code that figures out if adjacent transfers
are compatible and merges them if they are.

> If that's not actually needed, a possible alternative for fixing the
> performance problem might be adding a Kconfig option to turn off all
> the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I
> could work on that instead (or in addition), and I wouldn't consider
> it critical to get in for this merge window.

This driver is *not* going in during this merge window, sorry.  You need
to get code into maintainer trees before the merge window opens but this
was only sent to maintainers after the merge window was already open.
This isn't the end of the world, there will be another kernel release in
a few months.

> > > +	platform_set_drvdata(dev, NULL);
> > > +	spi_master_put(master);
> > > +	return 0;
> > > +}

> > This can be removed entirely.

> OK. Does using the devm register function cause removal to be handled
> generically, or is there another reason it's not needed?

Yes.

> > > +static const struct of_device_id jcore_spi_of_match[] = {
> > > +	{ .compatible = "jcore,spi2" },
> > > +	{},
> > > +};

> > This is adding a DT binding with no binding document.  All new DT
> > bindings need to be documented.

> The DT binding was in patch 05/12. Should linux-spi have been added to
> the Cc list for it? get_maintainer.pl didn't include it.

Yes, the documentation and the code need to be reviewed together.  It's
hard to tell if the code implements the bindings without seeing both.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Rich Felker]

On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote:
> On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote:
> > On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote:
> 
> > > This is *extremely* late for a first posting of a driver for v4.7 (you
> > > missed the list as well as the maintainers).
> 
> > I'm sorry for the mix-up. The kernel workflow is still fairly new to
> > me and I've been fighting with git format-patch/send-email and
> > get_maintainer.pl to get big patch series prepared the way I want and
> > sent to the right people/lists. I think I've got it right now though.
> 
> One question here is why this is even part of a series - it's adding a
> new controller driver which wouldn't normally have any sort of direct
> build or other dependency on anything else or have other things
> depending on it.  Unless there are dependencies it is generally best to
> send separate changes separately as far as possible so that there is no
> need to worry about issues with one part of the series slowing down
> other parts of the series.

I grouped the patches as a series because they make up support for a
complete SoC. While some of the peripheral drivers may well be useful
for non-J2 systems in the future (the cores are all open source, BSD
licensed), there's no short-term benefit to having these drivers
without the main J2 support.

In terms of hard dependencies, the mimas_v2 dts file depends on the
jcore,spi binding, but nothing depends on the driver. Still it's
important to have for the sake of actually making use of it all.

> > > > +	hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> > > > +		^ (!value << 2*spi->chip_select);
> 
> > > Why the xor here and not an or?  The coding style is also weird, a mix
> > > of extra spaces around the () and missing ones around *.  I'm finding
> > > the intent of the code confusing here.
> 
> > The intent is to set all chipselect bits (the 3 macro values) high
> > except possibly spi->chip_select. The xor is to turn off a bit, not
> > turn it on. &~ would also have worked; would that be more idiomatic?
> 
> No, the reader has to be able to tell what the code is doing.  If this
> made sense the thing to do would be to write out the logic operations
> explicitly to make it clear that every step is deliberate.  However in
> this case it sounds like the code is just plain buggy, though - the
> driver is being asked to set a specific chip select to a specific value
> but instead of just doing that it is also trying to also change some
> other settings.

It may be redundant, if the general SPI framework handles mutual
exclusion of chipselects, but it's not buggy. Only a single chipselect
can be active (low) at once; with multiple chips selected very bad
things will happen. I don't see any documentation of the high-level
SPI framework in Linux and what (if anything) it does to ensure that
all other chipselects are disabled before enabling one, which I why I
wrote the code so that the other chipselects are explicitly disabled.

> Don't do that, just have the function do what it was
> asked.  If the calling code has problems it'll need fixing anyway and if
> some feature you hadn't anticipated ends up meaning the combination of
> operations makes sense then things will just work.

Setting just a single bit seems more complicated and error-prone to
me; explicit effort has to be made to ensure that the hardware state
remains in sync with the driver's view of it. As written now, the
whole register (with all the cs/speed/etc. bits) is just written every
time.

> > > > +#if !USE_MESSAGE_MODE
> > > > +	spi_finalize_current_transfer(master);
> > > > +#endif
> 
> > > I'm not sure what the if is about but it doesn't belong upstream, you
> > > shouldn't be open coding bits of the framework.
> 
> > I can explain the motivation and see what you think is best to do. I'd
> > like to be able to provide just the transfer_one operation, and use
> > the generic transfer_one_message. However, at 50 MHz cpu clock, the
> > stats collection and other overhead in spi.c's generic
> > spi_transfer_one_message takes up so much time between the end of one
> > SD card transfer and the beginning of the next that the overall
> > transfer rate is something like 15-20% higher with my version.
> 
> No, this is just not a good approach.  If the generic code isn't working
> for you improve the generic code, don't just copy it and open code the
> bits you like.  The reason Linux has all these great framework is that
> people collaborate to make the generic code as good and fully featured
> as they can rather than open coding everything in drivers.  Doing things
> in drivers results in lots of code duplication which increases the cost
> of maintaining things and requires that everyone waste time copying code
> in order to keep the feature set consistent between drivers.

I totally agree with this principle, and I'm fine with dropping the
offending code. I may maintain it out-of-tree as a performance patch
for a while, but DMA and other improvements will hopefully make it
unnecessary.

> > Another consideration is that DMA support is being added to the
> > hardware right now, and I think we're going to want to be able to
> > queue up whole messages for DMA rather than just individual transfers;
> > in that case, spi_transfer_one_message is probably not suitable
> > anyway. So we'll probably have to end up having our own
> > transfer_one_message function anyway.
> 
> This is again a simple generic optimisation which would be best
> implemented in generic code - it's not just this device which would
> benefit from the ability to coalesce compatible transfers.  There is no
> reason for individual drivers to even think about such an optimisation,
> the core should just be handling them a transfer with a coalesced DMA
> transfer in it.  We're not doing it at present because someone needs to
> take the time to write the code that figures out if adjacent transfers
> are compatible and merges them if they are.

OK, then let's go with that approach. As long as there's a viable way
forward for doing it in a way that benefits all devices/drivers, I
prefer it anyway. I just didn't want to hit a situation where I later
propose that and a mainainer (you or someone else) tells me it's not
possible or not necessary.

Rich

Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

On Mon, May 23, 2016 at 04:29:38PM -0400, Rich Felker wrote:
> On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote:

> > One question here is why this is even part of a series - it's adding a
> > new controller driver which wouldn't normally have any sort of direct
> > build or other dependency on anything else or have other things
> > depending on it.  Unless there are dependencies it is generally best to
> > send separate changes separately as far as possible so that there is no
> > need to worry about issues with one part of the series slowing down
> > other parts of the series.

> I grouped the patches as a series because they make up support for a
> complete SoC. While some of the peripheral drivers may well be useful
> for non-J2 systems in the future (the cores are all open source, BSD
> licensed), there's no short-term benefit to having these drivers
> without the main J2 support.

That's what -next is for, merging everyone's trees together to give
something to test.  Practically speaking most maintainence is done at
the build level, it's how we do all the other SoCs so that people can
see what's going on at the subsystem level.

> > No, the reader has to be able to tell what the code is doing.  If this
> > made sense the thing to do would be to write out the logic operations
> > explicitly to make it clear that every step is deliberate.  However in
> > this case it sounds like the code is just plain buggy, though - the
> > driver is being asked to set a specific chip select to a specific value
> > but instead of just doing that it is also trying to also change some
> > other settings.

> It may be redundant, if the general SPI framework handles mutual
> exclusion of chipselects, but it's not buggy. Only a single chipselect
> can be active (low) at once; with multiple chips selected very bad
> things will happen. I don't see any documentation of the high-level
> SPI framework in Linux and what (if anything) it does to ensure that
> all other chipselects are disabled before enabling one, which I why I
> wrote the code so that the other chipselects are explicitly disabled.

There is no such guarantee because there are applications where it
makes sense to write to multiple chips simultaneously - this is one
common way of doing simultaneous updates over multiple audio audio
CODECs to ensure synchronization for example.  It's also just not
something that it makes any sense to worry about at the indivudal driver
level.

The generic code is responsible for ensuring that things work well,
writing bodges that silently try to work around the generic code is
always a recipie for fragility, especially if that code is hard to
understand.  Either the driver was making unwarranted assumptions that
break use cases it didn't think of or we end up having to find and fix
issues multiple times due to duplication.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 07/12] sh: add AT_HWCAP flag for J-Core cas.l instruction

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 arch/sh/include/uapi/asm/cpu-features.h | 1 +
 arch/sh/kernel/cpu/sh2/probe.c          | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/sh/include/uapi/asm/cpu-features.h b/arch/sh/include/uapi/asm/cpu-features.h
index 694abe4..2f1bc85 100644
--- a/arch/sh/include/uapi/asm/cpu-features.h
+++ b/arch/sh/include/uapi/asm/cpu-features.h
@@ -22,5 +22,6 @@
 #define CPU_HAS_L2_CACHE	0x0080	/* Secondary cache / URAM */
 #define CPU_HAS_OP32		0x0100	/* 32-bit instruction support */
 #define CPU_HAS_PTEAEX		0x0200	/* PTE ASID Extension support */
+#define CPU_HAS_CAS_L		0x0400	/* cas.l atomic compare-and-swap */
 
 #endif /* __ASM_SH_CPU_FEATURES_H */
diff --git a/arch/sh/kernel/cpu/sh2/probe.c b/arch/sh/kernel/cpu/sh2/probe.c
index 3dd8187..665fcfb 100644
--- a/arch/sh/kernel/cpu/sh2/probe.c
+++ b/arch/sh/kernel/cpu/sh2/probe.c
@@ -54,6 +54,8 @@ void __ref cpu_probe(void)
 	boot_cpu_data.dcache.entry_shift	= 5;
 	boot_cpu_data.dcache.linesz		= 32;
 	boot_cpu_data.dcache.flags		= 0;
+
+	boot_cpu_data.flags |= CPU_HAS_CAS_L;
 #else
 	/*
 	 * SH-2 doesn't have separate caches
-- 
2.8.1

[PATCH v2 05/12] of: add J-Core SPI master bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.txt b/Documentation/devicetree/bindings/spi/jcore,spi.txt
new file mode 100644
index 0000000..9055e7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/jcore,spi.txt
@@ -0,0 +1,23 @@
+J-Core SPI master
+
+Required properties:
+
+- compatible: Must be "jcore,spi2".
+
+- reg: Memory region for registers.
+
+- #address-cells: Must be 1.
+
+- #size-cells: Must be 0.
+
+See spi-bus.txt for additional properties not specific to this device.
+
+Example:
+
+spi {
+	compatible = "jcore,spi2";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = < 0x40 0x8 >;
+	spi-max-frequency = <12500000>;
+}
-- 
2.8.1

Re: [PATCH v2 05/12] of: add J-Core SPI master bindings

[Geert Uytterhoeven]

On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/jcore,spi.txt
> @@ -0,0 +1,23 @@
> +Example:
> +
> +spi {

spi@40

> +       compatible = "jcore,spi2";
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       reg = < 0x40 0x8 >;
> +       spi-max-frequency = <12500000>;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 05/12] of: add J-Core SPI master bindings

[Rob Herring]

On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.txt b/Documentation/devicetree/bindings/spi/jcore,spi.txt
> new file mode 100644
> index 0000000..9055e7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/jcore,spi.txt
> @@ -0,0 +1,23 @@
> +J-Core SPI master
> +
> +Required properties:
> +
> +- compatible: Must be "jcore,spi2".

In general, these all seem a bit generic. You should be able to 
correlate IP versions to compatible strings.

Rob

Re: [PATCH v2 05/12] of: add J-Core SPI master bindings

[Rich Felker]

On Mon, May 23, 2016 at 04:00:20PM -0500, Rob Herring wrote:
> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.txt b/Documentation/devicetree/bindings/spi/jcore,spi.txt
> > new file mode 100644
> > index 0000000..9055e7d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/jcore,spi.txt
> > @@ -0,0 +1,23 @@
> > +J-Core SPI master
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "jcore,spi2".
> 
> In general, these all seem a bit generic. You should be able to 
> correlate IP versions to compatible strings.

"spi2" is the name of the "IP core" in the J-Core source tree. The WIP
that adds DMA support and makes some changes is called "spi3" there;
I'll submit a patch adding "jcore,spi3" to the binding file once the
vhdl for it has been released.

Rich

Re: [PATCH v2 05/12] of: add J-Core SPI master bindings

[Rob Herring]

On Mon, May 23, 2016 at 4:06 PM, Rich Felker <dalias@libc.org> wrote:
> On Mon, May 23, 2016 at 04:00:20PM -0500, Rob Herring wrote:
>> On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
>> > Signed-off-by: Rich Felker <dalias@libc.org>
>> > ---
>> >  .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
>> >  1 file changed, 23 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/spi/jcore,spi.txt b/Documentation/devicetree/bindings/spi/jcore,spi.txt
>> > new file mode 100644
>> > index 0000000..9055e7d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/spi/jcore,spi.txt
>> > @@ -0,0 +1,23 @@
>> > +J-Core SPI master
>> > +
>> > +Required properties:
>> > +
>> > +- compatible: Must be "jcore,spi2".
>>
>> In general, these all seem a bit generic. You should be able to
>> correlate IP versions to compatible strings.
>
> "spi2" is the name of the "IP core" in the J-Core source tree. The WIP
> that adds DMA support and makes some changes is called "spi3" there;
> I'll submit a patch adding "jcore,spi3" to the binding file once the
> vhdl for it has been released.

Okay.

Rob