Initial Device Tree support for S3C64xx

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa <[email protected]>

Quoted 10 lines

+++ b/Documentation/devicetree/bindings/arm/vic.txt

(quoted diff) +1 -0

@@ -18,6 +18,9 @@ Required properties:
 Optional properties:
 
 - interrupts : Interrupt source for parent controllers if the VIC is
 nested.> 
+- interrupt-mask : Bit mask of valid interrupt sources (defaults to

all valid) +- wakeup-mask : Bit mask of interrupt sources that can wake up the system + (defaults to all allowed)

The "interrupt-mask" is called "valid-mask" on the FPGA IRQ controller See Documentation/devicetree/bindings/arm/versatile-fpga-irq.txt

Example:

                reg = <0x10000040 0x100>;
                clear-mask = <0xffffffff>;
                valid-mask = <0x00000007>;

Please use the same name.

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Signed-off-by Tomasz Figa <[email protected]>

---
 Documentation/devicetree/bindings/arm/vic.txt | 6 ++++++
 drivers/irqchip/irq-vic.c                     | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

Changes since v7:
- Renamed interrupt-mask property to valid-mask, for consistency with bindings of other interrupt controllers.

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

+- wakeup-mask : Bit mask of interrupt sources that can wake up the system
+  (defaults to all allowed)

Quoted 5 lines · Tomasz Figa

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

Quoted 5 lines

Documentation/devicetree/bindings/arm/vic.txt +1 -0

 
+- valid-mask : Bit mask of valid interrupt sources (defaults to all

valid)

Until a device requests an interrupt, it can be left disabled. Once something does request it, it gets enabled. If nothing requests it, it never gets enabled. Doesn't the result of that logic end up being the same thing as valid-mask represents? The only difference would be if some device requests an invalid interrupt source, but then why not just fix the interrupt client instead of adding this property to reject the request?

Quoted 2 lines

+- wakeup-mask : Bit mask of interrupt sources that can wake up the
system +  (defaults to all allowed)

Shouldn't drivers for end-devices request interrupts, and then set wake enable on those interrupts, which would then trickle up the IRQ chain to tell the VIC which interrupts to enable wakeup on?

Quoted 7 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 5 lines · Tomasz Figa

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Quoted 2 lines

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

Quoted 8 lines · Tomasz Figa

Quoted 7 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 5 lines · Tomasz Figa

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Quoted 6 lines

Quoted 2 lines

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

Ah, OK. It may be worth mentioning the intent of the properties.

I
suppose this is purely a representation of HW then, so it's reasonable to include it in the binding. I'm not sure /quite/ how useful it is; after all the error-checking that it enables will never trigger assuming the rest of the DT is written to "request" the correct interrupts. However, I guess there is little harm in allowing these properties.

Bikeshedding a bit, but perhaps rename wakeup-mask to valid-wakeup-mask (and perhaps valid-mask to valid-source-mask for consistency, although I see you already renamed that to be consistent with other bindings...)? To me, wakeup-mask sounds like a configuration of which sources should be configured to wakeup the system; something to do with configuration/policy rather than HW capabilities.

Either way, the binding,

Acked-by Stephen Warren <[email protected]>

(I assume those 2 new properties always have 1 cell since the controller can only support up to 32 IRQ sources? If not, then perhaps add wording to describe how long the properties should be).

Quoted 9 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 5 lines · Tomasz Figa

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

To clarify, this is a mask of valid interrupt sources of the VIC, where set bit indicates that given signal is wired and clear bit that it is not.

On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <[email protected]>

Quoted 15 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 9 lines · Tomasz Figa

Quoted 5 lines

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

Quoted 4 lines

To clarify, this is a mask of valid interrupt sources of the VIC, where
set bit indicates that given signal is wired and clear bit that it is not.

I agree with Stephen here. The valid interrupts are the ones in the DT. The reserved ones are the ones not present. If it is not needed for the operation of the VIC, then remove it. The argument of sanity checking could apply to all interrupt controllers.

Quoted 1 line · On Saturday 24 of August 2013 10:25:26 Rob Herring wrote:

On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <[email protected]>

Quoted 15 lines · wrote:

Quoted 15 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 9 lines · Tomasz Figa

Quoted 5 lines

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

Quoted 8 lines

Quoted 4 lines

To clarify, this is a mask of valid interrupt sources of the VIC, where
set bit indicates that given signal is wired and clear bit that it is not.

I agree with Stephen here. The valid interrupts are the ones in the DT. The reserved ones are the ones not present. If it is not needed for the operation of the VIC, then remove it. The argument of sanity checking could apply to all interrupt controllers.

Sorry, but I don't get what's wrong in having a more detailed description than required just for operation of the hardware.

The feature of sanity checks based on interrupt_mask (here now called valid-mask) has been present in the VIC driver since a long time already (if not from the beginning of existence of this driver) and before we started using DT, the mask was being passed from platform code as VIC init function argument.

I'd prefer this feature to be available when using DT as well, unless we really want to move things backwards, just because we want to use DT...

Quoted 34 lines · Tomasz Figa

Quoted 1 line · On Saturday 24 of August 2013 10:25:26 Rob Herring wrote:

On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <[email protected]>

Quoted 22 lines · wrote:

Quoted 16 lines · On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

Quoted 10 lines · Tomasz Figa

Quoted 5 lines

This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree.

If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch.

Can you explain further why the VIC needs this information up-front?
Presumably it can accumulate it as devices request interrupts.

It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested.

Quoted 5 lines

To clarify, this is a mask of valid interrupt sources of the VIC, where
set bit indicates that given signal is wired and clear bit that it is
not.

Quoted 4 lines

I agree with Stephen here. The valid interrupts are the ones in the DT. The reserved ones are the ones not present. If it is not needed for the operation of the VIC, then remove it. The argument of sanity checking could apply to all interrupt controllers.

Sorry, but I don't get what's wrong in having a more detailed description than required just for operation of the hardware.

Quoted 5 lines

The feature of sanity checks based on interrupt_mask (here now called valid-mask) has been present in the VIC driver since a long time already (if not from the beginning of existence of this driver) and before we started using DT, the mask was being passed from platform code as VIC init function argument.

So we should base the binding on the Linux software design?

Quoted 3 lines

I'd prefer this feature to be available when using DT as well, unless we really want to move things backwards, just because we want to use DT...

As I mentioned all these arguments apply to ALL interrupt controllers except ones which a mask does not work. So IF this makes sense, then this should be a generic property and generic code to support.

You simply have the same information twice. One is distributed

and one
is centralized. While it adds a way to validate things it also adds a way to introduce errors. Suppose someone writes a dts such that valid-mask matches the irq lines present in that dts (simply because they were lazy or don't have documentation of all interrupt lines).

Then
you go add a node with a new interrupt (because the initial dts was not complete). Updating the valid-mask could very easily be forgotten. Yes, this should all be found by testing, but people don't always have access to all the h/w.

Since this driver does not handle GPIO on device tree enabled platforms any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa <[email protected]>

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa <[email protected]>

Quoted 6 lines

Since this driver does not handle GPIO on device tree enabled platforms
any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa <[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

Quoted 2 lines · On Thursday 22 of August 2013 01:21:25 Linus Walleij wrote:

This seems like something I should apply to the GPIO tree right now, shall I?

Would be nice, but since this patch is a dependency for rest of this series and there are no other dependencies on GPIO tree, it might be easier to just let Kukjin take all the patches with required Acks.

Quoted 1 line · On Thursday 22 of August 2013 01:21:25 Linus Walleij wrote:

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Since this driver does not handle GPIO on device tree enabled platforms
any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Quoted 4 lines

Quoted 1 line

Signed-off-by Tomasz Figa <[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

Would be nice, but since this patch is a dependency for rest of this series and there are no other dependencies on GPIO tree, it might be easier to just let Kukjin take all the patches with required Acks.

Quoted 16 lines · Tomasz Figa

Quoted 2 lines · On Thursday 22 of August 2013 01:21:25 Linus Walleij wrote:

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa
<[email protected]>

Quoted 10 lines · wrote:

Quoted 7 lines

Since this driver does not handle GPIO on device tree enabled platforms
any more, it should be bypassed whenever device tree is available, to
not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa <[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

Quoted 3 lines

Would be nice, but since this patch is a dependency for rest of this series and there are no other dependencies on GPIO tree, it might be easier to just let Kukjin take all the patches with required Acks.

Oh, I didn't see any dependencies...but if so, Linus do you agree with Tomasz' opinion?

Please let me know so that I could take this with others into the samsung tree. And please kindly ignore my previous reply on this ;-)

Quoted 13 lines · Tomasz Figa

Quoted 1 line · On Thursday 22 of August 2013 01:21:25 Linus Walleij wrote:

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa <[email protected]>

Quoted 9 lines · wrote:

Quoted 6 lines

Since this driver does not handle GPIO on device tree enabled platforms
any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa <[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

Quoted 4 lines

Would be nice, but since this patch is a dependency for rest of this series and there are no other dependencies on GPIO tree, it might be easier to just let Kukjin take all the patches with required Acks.

Oh, I didn't see any dependencies...but if so, Linus do you agree with Tomasz' opinion?

Quoted 18 lines · Kukjin Kim

Quoted 17 lines · Tomasz Figa

Quoted 1 line · On Thursday 22 of August 2013 01:21:25 Linus Walleij wrote:

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa<[email protected]>

Quoted 9 lines · wrote:

Quoted 6 lines

Since this driver does not handle GPIO on device tree enabled platforms
any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa<[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

Would be nice, but since this patch is a dependency for rest of this series and there are no other dependencies on GPIO tree, it might be easier to just let Kukjin take all the patches with required Acks.

Quoted 2 lines

Oh, I didn't see any dependencies...but if so, Linus do you agree with Tomasz' opinion?

Yes please merge it through the Samsung tree.

Acked-by Linus Walleij<[email protected]>

On Wed, Aug 21, 2013 at 11:21 PM, Tomasz Figa <[email protected]> wrote:

Quoted 5 lines

Since this driver does not handle GPIO on device tree enabled platforms any more, it should be bypassed whenever device tree is available, to not conflict with the new pinctrl-samsung driver.

Signed-off-by Tomasz Figa <[email protected]>

This seems like something I should apply to the GPIO tree right now, shall I?

This series adds Device Tree support for Samsung S3C64xx SoC series.

It fixes several problems preventing from booting an S3C64xx-based system using Device Tree, adds all the infrastructure for Device Tree-based board support, including mach-s3c64xx-dt and dts include files for S3C64xx SoCs, and basic device tree source for FriendlyARM Mini6410 and SAMSUNG SMDK6410 boards.

Current support is somewhat limite, but should be fine as a start and will be extended in future, hopefully with help of S3C64xx board maintainers.

Depends on:
- [PATCH v2 0/8] Common Clock Framework support for Samsung S3C64xx

    http://thread.gmane.org/gmane.linux.usb.general/90493

On Samsung S3C6410-based Mini6410 and SMDK6410 boards:

Tested-by Tomasz Figa<[email protected]>

Changes since v6:
- Rebased onto current for-next branch of Kgene's tree,
- Dropped the idea of .init_platform() machine callback,
- Added support for i2c controllers,
- Replaced separate apb and ahb nodes with a single common soc node,
- Updated cpus node according to CPU bindings,
- Updated fixed rate clock nodes to use generic bindings,
- Added support for PWM buzzer and I2C EEPROM on Mini6410 board,
- Simplified the condition of skipping legacy GPIO initialization.

Changes since v5:
- Rebased onto current for-next branch of Kgene's tree,
- Added support for new Common Clock Framework based clock driver,
- Introduced hierarchical structure of device nodes (representing physical AHB and APB buses),
- Bypassed most of legacy initialization when booting with DT,
- Used preprocessor macros in dts files to improve readabilty,
- Added support for SMDK6410 board.

Changes since v4:
- Rebased onto current for-next branch of Kgene's tree
- Added support for pinctrl (driver merged through pinctrl tree)
- Added labels to all nodes that can be overridden from board dts
- Modified board dts to use the&label syntax to reference device nodes

Changes since v3:
- Rebased onto current for-next branch of Kgene's tree
- Updated timer node for next version of samsung-time patches
- Replaced local s3c64xx_dt_init_irq with irqchip_init

Changes since v2:
- Updated timer node for next version of samsung-time patches

Changes since v1:
- Dropped two VIC-related patches that are no longer necessary
- Added Device Tree-based instantation of system timers
- Corrected VIC interrupt specifiers
- Adjusted CPU compatible values and dropped @id

Tomasz Figa (7):
ARM: common: vic: Parse interrupt and resume masks from device tree
ARM: s3c64xx: Bypass legacy initialization when booting with DT
gpio: samsung: Skip initialization if device tree is present
ARM: s3c64xx: Add board file for boot using Device Tree
ARM: dts: Add basic dts include files for Samsung S3C64xx SoCs
ARM: dts: Add dts file for S3C6410-based Mini6410 board
ARM: dts: Add dts file for S3C6410-based SMDK6410 board

  Documentation/devicetree/bindings/arm/vic.txt |   6 +
  arch/arm/boot/dts/Makefile                    |   2 +
  arch/arm/boot/dts/s3c6400.dtsi                |  41 ++
  arch/arm/boot/dts/s3c6410-mini6410.dts        | 228 +++++++++
  arch/arm/boot/dts/s3c6410-smdk6410.dts        | 103 ++++
  arch/arm/boot/dts/s3c6410.dtsi                |  57 +++
  arch/arm/boot/dts/s3c64xx-pinctrl.dtsi        | 687 ++++++++++++++++++++++++++
  arch/arm/boot/dts/s3c64xx.dtsi                | 199 ++++++++
  arch/arm/mach-s3c64xx/Kconfig                 |  16 +
  arch/arm/mach-s3c64xx/Makefile                |   1 +
  arch/arm/mach-s3c64xx/common.c                |  12 +
  arch/arm/mach-s3c64xx/dma.c                   |   9 +
  arch/arm/mach-s3c64xx/irq-pm.c                |   9 +
  arch/arm/mach-s3c64xx/mach-s3c64xx-dt.c       |  85 ++++
  arch/arm/mach-s3c64xx/s3c6400.c               |   9 +
  arch/arm/mach-s3c64xx/s3c6410.c               |   9 +
  arch/arm/plat-samsung/init.c                  |  12 +-
  drivers/gpio/gpio-samsung.c                   |  34 +-
  drivers/irqchip/irq-vic.c                     |   7 +-
  19 files changed, 1497 insertions(+), 29 deletions(-)

create mode 100644 arch/arm/boot/dts/s3c6400.dtsi create mode 100644 arch/arm/boot/dts/s3c6410-mini6410.dts create mode 100644 arch/arm/boot/dts/s3c6410-smdk6410.dts create mode 100644 arch/arm/boot/dts/s3c6410.dtsi create mode 100644 arch/arm/boot/dts/s3c64xx-pinctrl.dtsi create mode 100644 arch/arm/boot/dts/s3c64xx.dtsi create mode 100644 arch/arm/mach-s3c64xx/mach-s3c64xx-dt.c