Re: [PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Masahiro Yamada 
Hi Arnd,

2015-12-09 1:59 GMT+09:00 Arnd Bergmann :
>
> Just a little thought about one thing I found odd:
>
>> +static int uniphier_system_bus_check_overlap(
>> + struct uniphier_system_bus_priv tmp)
>> +{
>
> Did you intentionally pass this by value?

Yes, I did not want to disturb the original one.

It is copied to a temporary storage, and sorted by the base address,
and discarded after the overlap checking.

But, you are right.   It is possible (and simpler)
to check the region overlap without sorting.


> Maybe do it explicitly using a pointer
> and memcpy to a local variable, which has a similar effect. Alternatively
> just check each newly probed child node for conflicts with any of the
> previous children. That is slightly more expensive at O(n^2) instead of O(n)
> but n is always small here and you can avoid sorting first.

Good idea.  I simplified this function in v4.



-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Arnd Bergmann 
On Wednesday 09 December 2015 01:21:58 Masahiro Yamada wrote:
> The UniPhier System Bus is an external that connects on-board devices
> to the UniPhier SoC.  Each bank (chip select) is dynamically mapped
> to the CPU-viewed address base via the bus controller.  The bus
> controller must be configured before any access to the bus.
> 
> This driver parses the "ranges" property of the System Bus node and
> initialized the bus controller.  After the bus becomes ready, devices
> below it are populated.
> 
> Note:
> Each bank can be mapped anywhere in the supported address space;
> there is nothing preventing us from assigning bank 0 on 0x4200,
> 0x4300, or anywhere as long as such region is not used by others.
> So, the "ranges" is just one possible software configuration, which
> does not seem to fit in device tree because device tree is a hardware
> description language.  However, of_translate_address() requires
> "ranges" in every bus node between CPUs and device mapped on the CPU
> address space.  In other words, "ranges" properties must be statically
> defined in device tree.  After some discussion, I decided the dynamic
> address reassignment by the driver is too bothersome.  Instead, the
> device tree should provide a reasonable translation setup that the OS
> can rely on.
> 
> Signed-off-by: Masahiro Yamada 
> Acked-by: Rob Herring 

Looks very nice.

Acked-by: Arnd Bergmann 

Just a little thought about one thing I found odd:

> +static int uniphier_system_bus_check_overlap(
> + struct uniphier_system_bus_priv tmp)
> +{

Did you intentionally pass this by value? Maybe do it explicitly using a pointer
and memcpy to a local variable, which has a similar effect. Alternatively
just check each newly probed child node for conflicts with any of the
previous children. That is slightly more expensive at O(n^2) instead of O(n)
but n is always small here and you can avoid sorting first.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Masahiro Yamada 
The UniPhier System Bus is an external that connects on-board devices
to the UniPhier SoC.  Each bank (chip select) is dynamically mapped
to the CPU-viewed address base via the bus controller.  The bus
controller must be configured before any access to the bus.

This driver parses the "ranges" property of the System Bus node and
initialized the bus controller.  After the bus becomes ready, devices
below it are populated.

Note:
Each bank can be mapped anywhere in the supported address space;
there is nothing preventing us from assigning bank 0 on 0x4200,
0x4300, or anywhere as long as such region is not used by others.
So, the "ranges" is just one possible software configuration, which
does not seem to fit in device tree because device tree is a hardware
description language.  However, of_translate_address() requires
"ranges" in every bus node between CPUs and device mapped on the CPU
address space.  In other words, "ranges" properties must be statically
defined in device tree.  After some discussion, I decided the dynamic
address reassignment by the driver is too bothersome.  Instead, the
device tree should provide a reasonable translation setup that the OS
can rely on.

Signed-off-by: Masahiro Yamada 
Acked-by: Rob Herring 
---

Changes in v3:
  - Minor fix of git-log
  - Add Rob's Acked-by

Changes in v2:
  - Re-design the binding, driver implementation
Switch to a single node, populate children after the bus is setup

 .../bindings/bus/uniphier-system-bus.txt   |  65 +
 MAINTAINERS|   1 +
 drivers/bus/Kconfig|   8 +
 drivers/bus/Makefile   |   1 +
 drivers/bus/uniphier-system-bus.c  | 283 +
 5 files changed, 358 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
 create mode 100644 drivers/bus/uniphier-system-bus.c

diff --git a/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt 
b/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
new file mode 100644
index 000..5ec47c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
@@ -0,0 +1,65 @@
+UniPhier System Bus
+
+The UniPhier System Bus is an external bus that connects on-board devices to
+the UniPhier SoC.  It is a simple (semi-)parallel bus with address, data, and
+some control signals.  It supports up to 8 banks (chip selects).
+
+Before any access to the bus, the bus controller must be configured; the bus
+controller registers provide the control for the address translation from the
+the System Bus to the parent bus.  The needed setup includes the base address,
+the size, and some timing parameters of each bank.
+
+Required properties:
+- compatible: should be "socionext,uniphier-system-bus".
+- reg: offset and length of the register set for the bus controller device.
+- #address-cells: should be 2.  The first cell is the bank number (chip 
select).
+  The second cell is the address offset within the bank.
+- #size-cells: should be 1.
+- ranges: should provide a proper address translation from the System Bus to
+  the parent bus.
+
+Note:
+The address region(s) that can be assigned for the System Bus is implementation
+defined.  Some SoCs can use 0x-0x0fff and 0x4000-0x4fff,
+while other SoCs can only use 0x4000-0x4fff.  There might be additional
+limitations depending on SoCs and the boot mode.  The address translation is
+arbitrary as long as the banks are assigned in the supported address space with
+the required alignment and they do not overlap one another.
+For example, it is possible to map:
+  bank 0 to 0x4200-0x43ff, bank 5 to 0x4600-0x46ff
+It is also possible to map:
+  bank 0 to 0x4800-0x49ff, bank 5 to 0x4400-0x44ff
+There is no reason to stick to a particular translation mapping, but the
+"ranges" property should provide a "reasonable" default that is known to work.
+The software should initialize the bus controller according to it.
+
+Example:
+
+   system-bus {
+   compatible = "socionext,uniphier-system-bus";
+   reg = <0x58c0 0x400>;
+   #address-cells = <2>;
+   #size-cells = <1>;
+   ranges = <1 0x 0x4200 0x0200
+ 5 0x 0x4600 0x0100>;
+
+   ethernet@1,01f0 {
+   compatible = "smsc,lan9115";
+   reg = <1 0x01f0 0x1000>;
+   interrupts = <0 48 4>
+   phy-mode = "mii";
+   };
+
+   uart@5,0020 {
+   compatible = "ns16550a";
+   reg = <5 0x0020 0x20>;
+   interrupts = <0 49 4>
+   clock-frequency = <12288000>;
+   };
+   };
+
+In this example,
+ - the Ethernet device is

Re: [PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Arnd Bergmann 
On Wednesday 09 December 2015 01:21:58 Masahiro Yamada wrote:
> The UniPhier System Bus is an external that connects on-board devices
> to the UniPhier SoC.  Each bank (chip select) is dynamically mapped
> to the CPU-viewed address base via the bus controller.  The bus
> controller must be configured before any access to the bus.
> 
> This driver parses the "ranges" property of the System Bus node and
> initialized the bus controller.  After the bus becomes ready, devices
> below it are populated.
> 
> Note:
> Each bank can be mapped anywhere in the supported address space;
> there is nothing preventing us from assigning bank 0 on 0x4200,
> 0x4300, or anywhere as long as such region is not used by others.
> So, the "ranges" is just one possible software configuration, which
> does not seem to fit in device tree because device tree is a hardware
> description language.  However, of_translate_address() requires
> "ranges" in every bus node between CPUs and device mapped on the CPU
> address space.  In other words, "ranges" properties must be statically
> defined in device tree.  After some discussion, I decided the dynamic
> address reassignment by the driver is too bothersome.  Instead, the
> device tree should provide a reasonable translation setup that the OS
> can rely on.
> 
> Signed-off-by: Masahiro Yamada 
> Acked-by: Rob Herring 

Looks very nice.

Acked-by: Arnd Bergmann 

Just a little thought about one thing I found odd:

> +static int uniphier_system_bus_check_overlap(
> + struct uniphier_system_bus_priv tmp)
> +{

Did you intentionally pass this by value? Maybe do it explicitly using a pointer
and memcpy to a local variable, which has a similar effect. Alternatively
just check each newly probed child node for conflicts with any of the
previous children. That is slightly more expensive at O(n^2) instead of O(n)
but n is always small here and you can avoid sorting first.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Masahiro Yamada 
The UniPhier System Bus is an external that connects on-board devices
to the UniPhier SoC.  Each bank (chip select) is dynamically mapped
to the CPU-viewed address base via the bus controller.  The bus
controller must be configured before any access to the bus.

This driver parses the "ranges" property of the System Bus node and
initialized the bus controller.  After the bus becomes ready, devices
below it are populated.

Note:
Each bank can be mapped anywhere in the supported address space;
there is nothing preventing us from assigning bank 0 on 0x4200,
0x4300, or anywhere as long as such region is not used by others.
So, the "ranges" is just one possible software configuration, which
does not seem to fit in device tree because device tree is a hardware
description language.  However, of_translate_address() requires
"ranges" in every bus node between CPUs and device mapped on the CPU
address space.  In other words, "ranges" properties must be statically
defined in device tree.  After some discussion, I decided the dynamic
address reassignment by the driver is too bothersome.  Instead, the
device tree should provide a reasonable translation setup that the OS
can rely on.

Signed-off-by: Masahiro Yamada 
Acked-by: Rob Herring 
---

Changes in v3:
  - Minor fix of git-log
  - Add Rob's Acked-by

Changes in v2:
  - Re-design the binding, driver implementation
Switch to a single node, populate children after the bus is setup

 .../bindings/bus/uniphier-system-bus.txt   |  65 +
 MAINTAINERS|   1 +
 drivers/bus/Kconfig|   8 +
 drivers/bus/Makefile   |   1 +
 drivers/bus/uniphier-system-bus.c  | 283 +
 5 files changed, 358 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
 create mode 100644 drivers/bus/uniphier-system-bus.c

diff --git a/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt 
b/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
new file mode 100644
index 000..5ec47c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/uniphier-system-bus.txt
@@ -0,0 +1,65 @@
+UniPhier System Bus
+
+The UniPhier System Bus is an external bus that connects on-board devices to
+the UniPhier SoC.  It is a simple (semi-)parallel bus with address, data, and
+some control signals.  It supports up to 8 banks (chip selects).
+
+Before any access to the bus, the bus controller must be configured; the bus
+controller registers provide the control for the address translation from the
+the System Bus to the parent bus.  The needed setup includes the base address,
+the size, and some timing parameters of each bank.
+
+Required properties:
+- compatible: should be "socionext,uniphier-system-bus".
+- reg: offset and length of the register set for the bus controller device.
+- #address-cells: should be 2.  The first cell is the bank number (chip 
select).
+  The second cell is the address offset within the bank.
+- #size-cells: should be 1.
+- ranges: should provide a proper address translation from the System Bus to
+  the parent bus.
+
+Note:
+The address region(s) that can be assigned for the System Bus is implementation
+defined.  Some SoCs can use 0x-0x0fff and 0x4000-0x4fff,
+while other SoCs can only use 0x4000-0x4fff.  There might be additional
+limitations depending on SoCs and the boot mode.  The address translation is
+arbitrary as long as the banks are assigned in the supported address space with
+the required alignment and they do not overlap one another.
+For example, it is possible to map:
+  bank 0 to 0x4200-0x43ff, bank 5 to 0x4600-0x46ff
+It is also possible to map:
+  bank 0 to 0x4800-0x49ff, bank 5 to 0x4400-0x44ff
+There is no reason to stick to a particular translation mapping, but the
+"ranges" property should provide a "reasonable" default that is known to work.
+The software should initialize the bus controller according to it.
+
+Example:
+
+   system-bus {
+   compatible = "socionext,uniphier-system-bus";
+   reg = <0x58c0 0x400>;
+   #address-cells = <2>;
+   #size-cells = <1>;
+   ranges = <1 0x 0x4200 0x0200
+ 5 0x 0x4600 0x0100>;
+
+   ethernet@1,01f0 {
+   compatible = "smsc,lan9115";
+   reg = <1 0x01f0 0x1000>;
+   interrupts = <0 48 4>
+   phy-mode = "mii";
+   };
+
+   uart@5,0020 {
+   compatible = "ns16550a";
+   reg = <5 0x0020 0x20>;
+   interrupts = <0 49 4>
+   clock-frequency = <12288000>;
+   };
+   };

Re: [PATCH v3] bus: uniphier-system-bus: add UniPhier System Bus driver

2015-12-08 Thread Masahiro Yamada 
Hi Arnd,

2015-12-09 1:59 GMT+09:00 Arnd Bergmann :
>
> Just a little thought about one thing I found odd:
>
>> +static int uniphier_system_bus_check_overlap(
>> + struct uniphier_system_bus_priv tmp)
>> +{
>
> Did you intentionally pass this by value?

Yes, I did not want to disturb the original one.

It is copied to a temporary storage, and sorted by the base address,
and discarded after the overlap checking.

But, you are right.   It is possible (and simpler)
to check the region overlap without sorting.


> Maybe do it explicitly using a pointer
> and memcpy to a local variable, which has a similar effect. Alternatively
> just check each newly probed child node for conflicts with any of the
> previous children. That is slightly more expensive at O(n^2) instead of O(n)
> but n is always small here and you can avoid sorting first.

Good idea.  I simplified this function in v4.



-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/