J-core J2 cpu and SoC peripherals support

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 <[email protected]>

---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +25 -0

new file mode 100644
index 0000000..00ef112
@@ -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".
+

Documentation/devicetree/bindings/jcore/cpus.txt new file +13 -0

+
+- 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

Documentation/devicetree/bindings/jcore/cpus.txt new file +9 -0

+
+- reg: A memory range used to IPI generation.
+
+- interrupts: An irq on which IPI will be received.
+
+
+----------
+CPUID node
+----------

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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.

Quoted 16 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +16 -0

+- 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.
+
+
+---------------------

Documentation/devicetree/bindings/jcore/cpus.txt line 58

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+Cache controller node
+---------------------
+
+Required properties:
+
+- compatible: Must be "jcore,cache".

That's pretty generic...

Documentation/devicetree/bindings/jcore/cpus.txt line 59

Quoted 2 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +2 -0

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

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

Quoted 4 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +4 -0

+--------
+IPI node
+--------
+

Documentation/devicetree/bindings/jcore/cpus.txt line 72

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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.

Quoted 7 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +7 -0

+----------
+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.
+

Documentation/devicetree/bindings/jcore/cpus.txt line 91

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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.

Quoted 43 lines · Rob Herring

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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.

Quoted 16 lines

Quoted 16 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +16 -0

+- 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.
+
+
+---------------------

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 58

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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.

Quoted 3 lines

Quoted 2 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +2 -0

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +4 -0

+--------
+IPI node
+--------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 72

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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.

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +7 -0

+----------
+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.
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 91

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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?

Quoted 10 lines

Quoted 4 lines

Quoted 1 line

+- 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

Quoted 4 lines

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.

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

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

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

Quoted 5 lines

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.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

+--------

Documentation/devicetree/bindings/jcore/cpus.txt new file +3 -0

+IPI node
+--------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 72

Quoted 6 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +6 -0

+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.

Quoted 3 lines

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...

Quoted 16 lines

Quoted 15 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 91

Quoted 13 lines

+----------

Documentation/devicetree/bindings/jcore/cpus.txt new file +12 -0

+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.

Quoted 1 line

Yes. Should I document that here?

Yes.

Quoted 56 lines · Rich Felker

Quoted 43 lines · Rob Herring

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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:

Quoted 6 lines

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.

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...

Quoted 67 lines · Rob Herring

Quoted 56 lines · Rich Felker

Quoted 43 lines · Rob Herring

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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:

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 74 lines · Rob Landley

Quoted 70 lines · Rob Herring

Quoted 56 lines · Rich Felker

Quoted 43 lines · Rob Herring

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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:

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

Quoted 3 lines

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?

Quoted 1 line

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.

Quoted 80 lines · Rob Herring

Quoted 78 lines · Rob Landley

Quoted 64 lines · Rob Herring

Quoted 56 lines · Rich Felker

Quoted 43 lines · Rob Herring

Quoted 42 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 Documentation/devicetree/bindings/jcore/cpus.txt | 91 ++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/jcore/cpus.txt

Documentation/devicetree/bindings/jcore/cpus.txt new file +30 -0

new file mode 100644
index 0000000..00ef112
@@ -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
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/jcore/cpus.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/jcore/cpus.txt new file +5 -0

+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:

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 1 line

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?

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 19

Quoted 6 lines · Rich Felker

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

+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?

Quoted 9 lines · Geert Uytterhoeven

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 19

Quoted 6 lines · Rich Felker

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

+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.

Quoted 11 lines · Rich Felker

Quoted 9 lines · Geert Uytterhoeven

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 19

Quoted 6 lines · Rich Felker

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

+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?

Quoted 4 lines

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?

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +2 -0

new file mode 100644
index 0000000..dc9fde8
@@ -0,0 +1,28 @@
+J-Core Advanced Interrupt Controller
+

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +16 -0

+
+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>;
+};

Signature · 1 line

2.8.1

Quoted 18 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

new file mode 100644
index 0000000..dc9fde8
@@ -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

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 12

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

+  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.

Quoted 19 lines · Rob Herring

Quoted 18 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

new file mode 100644
index 0000000..dc9fde8
@@ -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

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 12

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

+  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.

Quoted 29 lines · Rich Felker

Quoted 27 lines · Rob Herring

Quoted 14 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +2 -0

new file mode 100644
index 0000000..dc9fde8
@@ -0,0 +1,28 @@
+J-Core Advanced Interrupt Controller
+

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 12

Quoted 10 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +10 -0

+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.

Quoted 2 lines

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

Not even an EOI?

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Documentation/devicetree/bindings/spi/jcore,spi.txt new file +0 -0

new file mode 100644
index 0000000..9055e7d

Quoted 6 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Documentation/devicetree/bindings/spi/jcore,spi.txt line 5

Quoted 11 lines

Documentation/devicetree/bindings/spi/jcore,spi.txt new file +5 -0

new file mode 100644
index 0000000..9055e7d
@@ -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.

Quoted 7 lines · Rob Herring

Quoted 6 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Quoted 14 lines

Documentation/devicetree/bindings/spi/jcore,spi.txt line 5

Quoted 11 lines

Documentation/devicetree/bindings/spi/jcore,spi.txt new file +5 -0

new file mode 100644
index 0000000..9055e7d
@@ -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.

Signed-off-by Rich Felker <[email protected]>

---
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

drivers/irqchip/Kconfig +6 -0

index 3e12479..3cb37d6 100644
@@ -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

drivers/irqchip/Makefile +1 -0

index b03cfcb..5a1f1bf 100644
@@ -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

drivers/irqchip/irq-jcore-aic.c new file +54 -0

new file mode 100644
index 0000000..68178fb
@@ -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);

drivers/irqchip/irq-jcore-aic.c new file +12 -0

+	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);
+

Quoted 6 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
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.

Quoted 99 lines

 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

drivers/irqchip/Kconfig +6 -0

index 3e12479..3cb37d6 100644
@@ -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

drivers/irqchip/Makefile +1 -0

index b03cfcb..5a1f1bf 100644
@@ -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

drivers/irqchip/irq-jcore-aic.c new file +58 -0

new file mode 100644
index 0000000..68178fb
@@ -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)

drivers/irqchip/irq-jcore-aic.c line 64

Quoted 6 lines

drivers/irqchip/irq-jcore-aic.c new file +6 -0

+{
+	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?

Quoted 17 lines

drivers/irqchip/irq-jcore-aic.c new file +17 -0

+	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);

drivers/irqchip/irq-jcore-aic.c line 86

Quoted 5 lines

drivers/irqchip/irq-jcore-aic.c new file +5 -0

+	}
+
+	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?

drivers/irqchip/irq-jcore-aic.c line 88

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

+
+	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?

drivers/irqchip/irq-jcore-aic.c line 89

Quoted 1 line

drivers/irqchip/irq-jcore-aic.c new file +1 -0

+	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.

drivers/irqchip/irq-jcore-aic.c line 95

Quoted 6 lines

drivers/irqchip/irq-jcore-aic.c new file +6 -0

+
+	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.

Signed-off-by Rich Felker <[email protected]>

---

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

drivers/clocksource/Kconfig +0 -0

index c346be6..a29fd31 100644

drivers/clocksource/Kconfig +6 -0

+	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

drivers/clocksource/Makefile +1 -0

index dc2b899..d825fcf 100644
@@ -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

drivers/clocksource/jcore-pit.c new file +0 -0

new file mode 100644
index 0000000..a4fde51

drivers/clocksource/jcore-pit.c new file +16 -0

+ *
+ * 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>

drivers/clocksource/jcore-pit.c new file +13 -0

+#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);

drivers/clocksource/jcore-pit.c new file +13 -0

+
+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)
+{

drivers/clocksource/jcore-pit.c new file +19 -0

+
+	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);
+

Quoted 2 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---

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.

Quoted 4 lines

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.

Quoted 11 lines

 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

drivers/clocksource/Kconfig +0 -0

index c346be6..a29fd31 100644

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 301

Quoted 5 lines

drivers/clocksource/Kconfig +2 -0

@@ -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.

Quoted 25 lines

drivers/clocksource/Kconfig +6 -0

+	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

drivers/clocksource/Makefile +1 -0

index dc2b899..d825fcf 100644
@@ -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

drivers/clocksource/jcore-pit.c new file +0 -0

new file mode 100644

drivers/clocksource/jcore-pit.c line 2

Quoted 6 lines

index 0000000..a4fde51

drivers/clocksource/jcore-pit.c new file +2 -0

@@ -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.

Quoted 20 lines

drivers/clocksource/jcore-pit.c new file +20 -0

+ *
+ * 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>

drivers/clocksource/jcore-pit.c line 28

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -0

+#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 ?

drivers/clocksource/jcore-pit.c line 30

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

+static unsigned char __iomem *pit_base;

s/unsigned char/void/

drivers/clocksource/jcore-pit.c line 35

Quoted 4 lines

drivers/clocksource/jcore-pit.c new file +4 -0

+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;
}

Quoted 17 lines

drivers/clocksource/jcore-pit.c new file +17 -0

+#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;

drivers/clocksource/jcore-pit.c line 59

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -0

+		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/

drivers/clocksource/jcore-pit.c line 57

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

+}

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.

Quoted 16 lines

drivers/clocksource/jcore-pit.c new file +16 -0

+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;
+}

drivers/clocksource/jcore-pit.c line 83

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -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().

drivers/clocksource/jcore-pit.c line 86

Quoted 2 lines

drivers/clocksource/jcore-pit.c new file +2 -0

+	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.

Quoted 23 lines

drivers/clocksource/jcore-pit.c new file +23 -0

+
+	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);

drivers/clocksource/jcore-pit.c line 115

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -0

+	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)

drivers/clocksource/jcore-pit.c line 120

Quoted 4 lines

drivers/clocksource/jcore-pit.c new file +4 -0

+	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.

Quoted 4 lines

drivers/clocksource/jcore-pit.c new file +4 -0

+	return 0;
+}
+
+static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)

drivers/clocksource/jcore-pit.c line 131

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -0

+{
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		pit_local_init(this_cpu_ptr(pit_percpu));
+		break;
+	}

DYING is missing.

Quoted 5 lines

drivers/clocksource/jcore-pit.c new file +5 -0

+	return NOTIFY_OK;
+}
+
+static struct notifier_block pit_cpu_nb = {
+	.notifier_call = pit_cpu_notify,

drivers/clocksource/jcore-pit.c line 141

Quoted 5 lines

drivers/clocksource/jcore-pit.c new file +5 -0

+};
+
+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.

drivers/clocksource/jcore-pit.c line 143

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

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

CR missing before pit_disable().

drivers/clocksource/jcore-pit.c line 153

Quoted 9 lines

drivers/clocksource/jcore-pit.c new file +9 -0

+	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.

drivers/clocksource/jcore-pit.c line 161

Quoted 9 lines

drivers/clocksource/jcore-pit.c new file +8 -0

+
+	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.

drivers/clocksource/jcore-pit.c line 167

Quoted 5 lines

drivers/clocksource/jcore-pit.c new file +5 -0

+	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.

drivers/clocksource/jcore-pit.c line 168

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

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

CR before pr_err.

drivers/clocksource/jcore-pit.c line 171

Quoted 2 lines

drivers/clocksource/jcore-pit.c new file +2 -0

+	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).

drivers/clocksource/jcore-pit.c line 162

Quoted 2 lines

drivers/clocksource/jcore-pit.c new file +2 -0

+	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?

Quoted 3 lines · Daniel Lezcano

drivers/clocksource/jcore-pit.c line 59

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

+	return ((u64)sechi << 32 | seclo) * 1000000000 + nsec;

s/1000000000/NSEC_PER_SEC/

Quoted 4 lines

drivers/clocksource/jcore-pit.c line 171

Quoted 2 lines

drivers/clocksource/jcore-pit.c new file +2 -0

+	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).

Quoted 7 lines · Daniel Lezcano

Quoted 2 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---

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?

Quoted 10 lines

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*

Quoted 27 lines

Quoted 4 lines

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.

Quoted 11 lines

 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

drivers/clocksource/Kconfig +0 -0

index c346be6..a29fd31 100644

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 301

Quoted 5 lines

drivers/clocksource/Kconfig +2 -0

@@ -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:

Quoted 5 lines

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?

Quoted 4 lines

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 303

Quoted 2 lines

drivers/clocksource/Kconfig +2 -0

+/*
+ * 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).

Quoted 3 lines

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.

Quoted 5 lines

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 306

Quoted 3 lines

drivers/clocksource/Kconfig +3 -0

+#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.

Quoted 3 lines

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 307

Quoted 1 line

drivers/clocksource/Kconfig +1 -0

+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.

Quoted 6 lines

drivers/clocksource/Kconfig config SYS_SUPPORTS_SH_TMU · line 311

Quoted 4 lines

drivers/clocksource/Kconfig +4 -0

+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.

Quoted 2 lines

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.

Quoted 1 line

s/1000000000/NSEC_PER_SEC/

OK.

Quoted 5 lines

Quoted 1 line

+}

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.

Quoted 8 lines

drivers/clocksource/Kconfig

Quoted 6 lines

+static int pit_set(unsigned long delta, struct clock_event_device *ced)

drivers/clocksource/Kconfig +4 -0

+{
+	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?

Quoted 4 lines

drivers/clocksource/Kconfig

Quoted 2 lines

drivers/clocksource/Kconfig +2 -0

+	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?

Quoted 20 lines

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);
}

Quoted 1 line

(jcore_set_next_event, jcore_set_periodic)

OK.

Quoted 6 lines

drivers/clocksource/Kconfig

Quoted 4 lines

drivers/clocksource/Kconfig +4 -0

+	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.

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

drivers/clocksource/Kconfig +4 -0

+	return 0;
+}
+
+static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)

Quoted 8 lines

drivers/clocksource/Kconfig

Quoted 6 lines

drivers/clocksource/Kconfig +6 -0

+{
+	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?

Quoted 3 lines

drivers/clocksource/Kconfig

Quoted 1 line

drivers/clocksource/Kconfig +1 -0

+	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.

Quoted 7 lines

drivers/clocksource/Kconfig

Quoted 5 lines

drivers/clocksource/Kconfig +5 -0

+	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.

Quoted 5 lines

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.

Quoted 7 lines

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.

drivers/clocksource/Kconfig

Quoted 1 line

drivers/clocksource/Kconfig +1 -0

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

CR before pr_err.

OK.

Quoted 4 lines

drivers/clocksource/Kconfig

Quoted 2 lines

drivers/clocksource/Kconfig +2 -0

+	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.

Quoted 8 lines

drivers/clocksource/Kconfig

Quoted 2 lines

drivers/clocksource/Kconfig +2 -0

+	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.

Quoted 7 lines · Rich Felker

Quoted 6 lines · Daniel Lezcano

Quoted 2 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---

Hi Rich,

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 9 lines

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.

Quoted 5 lines

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].

Quoted 11 lines

Quoted 4 lines

Quoted 2 lines

+/*
+ * 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).

Quoted 5 lines

Quoted 2 lines

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.

[ ... ]

Quoted 6 lines

Quoted 1 line

s/1000000000/NSEC_PER_SEC/

OK.

Quoted 2 lines

Quoted 1 line

+}

Quoted 6 lines

Quoted 3 lines

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 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, ...

Quoted 9 lines

Quoted 1 line

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 ?

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

drivers/clocksource/jcore-pit.c new file +4 -0

+	return 0;
+}
+
+static int pit_cpu_notify(struct notifier_block *self, unsigned long action, void *hcpu)

Quoted 8 lines

drivers/clocksource/jcore-pit.c line 131

Quoted 6 lines

drivers/clocksource/jcore-pit.c new file +6 -0

+{
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_STARTING:
+		pit_local_init(this_cpu_ptr(pit_percpu));
+		break;
+	}

DYING is missing.

Quoted 1 line

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.

Quoted 8 lines

Quoted 3 lines

drivers/clocksource/jcore-pit.c line 161

Quoted 1 line

drivers/clocksource/jcore-pit.c new file +1 -0

+	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 ?

Quoted 10 lines

Quoted 4 lines

drivers/clocksource/jcore-pit.c line 171

Quoted 2 lines

drivers/clocksource/jcore-pit.c new file +2 -0

+	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 ?

Quoted 7 lines

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.

+ int xmit;

+ int status;

+ int count;

+ if (!timeout) break;

+ if (!timeout) break;

+ }

Quoted 2 lines · Rich Felker

drivers/spi/spi-jcore.c new file +0 -0

drivers/spi/spi-jcore.c line 97

Quoted 7 lines

drivers/spi/spi-jcore.c new file +7 -0

+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

drivers/spi/spi-jcore.c line 117

Quoted 11 lines

drivers/spi/spi-jcore.c new file +11 -0

+       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 (...)

Quoted 1 line

+ if (!timeout) break;

if (...)

        ...

drivers/spi/spi-jcore.c

Quoted 2 lines

drivers/spi/spi-jcore.c new file +2 -0

+
+               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.

Signed-off-by Rich Felker <[email protected]>

---
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.