Exynos SROMc configuration and Ethernet support for SMDK5410
v3 cover Exynos SROMc configuration and Ethernet support for SMDK5410
This patch extends Exynos SROM controller driver with ability to configure controller outputs and enables SMSC9115 Ethernet chip on SMDK5410 board, which is connected via SROMc bank #3.
With this patchset, support for the whole existing SMDK range can be added. Actually, only bank number is different.
This patchset also depends on Exynos 5410 pinctrl support, introduced by patches 0003 and 0004 from this set:
[PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
Pinctrl support is necessary in order to correctly configure multifunctional pins of the Exynos chip.
v2 => v3:
- Fixed up SROMc region size in the device tree
- Reordered patches, documentation goes first now
v1 => v2:
- Fixed some typos and bad labels in device tree
- Improved documentation
Pavel Fedin (4):
Documentation: dt-bindings: Describe SROMc configuration
ARM: dts: Add SROMc to Exynos 5410
drivers: exynos-srom: Add support for bank configuration
ARM: dts: Add Ethernet chip to SMDK5410
.../bindings/arm/samsung/exynos-srom.txt | 24 ++++++++++++- arch/arm/boot/dts/exynos5410-smdk5410.dts | 42 ++++++++++++++++++++++ arch/arm/boot/dts/exynos5410.dtsi | 5 +++ arch/arm/mach-exynos/Kconfig | 2 +- drivers/soc/samsung/Kconfig | 2 +- drivers/soc/samsung/exynos-srom.c | 42 +++++++++++++++++++++- 6 files changed, 113 insertions(+), 4 deletions(-)
Signature · 1 line
2.4.4
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
Add documentation for new properties, allowing bank configuration.
--- .../bindings/arm/samsung/exynos-srom.txt | 24 +++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +23 -1
@@ -5,8 +5,30 @@ Required properties: - reg: offset and length of the register set -Example: +Bank configurations can be defined in optional subnodes. Each subnode +describes one bank and contains the following: + +Required properties: +- bank : bank number (0 - 3) + +- srom-timing : array of 7 integers: Tacp, Tcah, Tcoh, Tacc, Tcos, Tacs + +Optional properties: +- width : data width in bytes (1 or 2). If omitted, default of 1 is used. + +Example: basic definition, no banks are configured + sromc at 12570000 { + compatible = "samsung,exynos-srom"; + reg = <0x12570000 0x10>; + }; + +Example: SROMc with bank3 configuration sromc at 12570000 { compatible = "samsung,exynos-srom"; reg = <0x12570000 0x10>; + bank at 3 { + bank = <3>; + width = <2>; + srom-timing = <1 9 12 1 9 1 1>; + }; };
Signature · 1 line
2.4.4
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
Quoted 6 lines · Pavel Fedin
Add documentation for new properties, allowing bank configuration.
Signed-off-by Pavel Fedin <[email protected]>--- .../bindings/arm/samsung/exynos-srom.txt | 24 +++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)
You missed here a lot of potential Cc. Please use get_maintainer.pl script. It *must* be sent to devicetree list.
Please send it also to DeviceTree maintainers because you are adding quite generic names for bindings so they may have interesting thoughts on this.
LKML list is also missing.
Quoted 6 lines
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +0
-0
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt Required properties: · line 12
Quoted 9 lines
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +5
-1@@ -5,8 +5,30 @@ Required properties: - reg: offset and length of the register set -Example: +Bank configurations can be defined in optional subnodes. Each subnode +describes one bank and contains the following: + +Required properties: +- bank : bank number (0 - 3)
I wonder whether this should be maybe "reg"?
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt Required properties: · line 14
Quoted 2 lines
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +2
-0+ +- srom-timing : array of 7 integers: Tacp, Tcah, Tcoh, Tacc, Tcos, Tacs
Missing "pmc"? Where is the seventh?
This looks vendor specific, so prefix with "samsung,exynos-".
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt Required properties: · line 17
Quoted 3 lines
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +3
-0+ +Optional properties: +- width : data width in bytes (1 or 2). If omitted, default of 1 is used.
This is not bank-width? So data-width? Any vendor prefix here? How generic is this?
These are first things which came to my mind. Maybe DT maintainers will come with something more...
Best regards,
Krzysztof
Quoted 18 lines
Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt +13
-0+ +Example: basic definition, no banks are configured + sromc at 12570000 { + compatible = "samsung,exynos-srom"; + reg = <0x12570000 0x10>; + }; + +Example: SROMc with bank3 configuration sromc at 12570000 { compatible = "samsung,exynos-srom"; reg = <0x12570000 0x10>; + bank at 3 { + bank = <3>; + width = <2>; + srom-timing = <1 9 12 1 9 1 1>; + }; };
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
Hello!
Quoted 8 lines
You missed here a lot of potential Cc. Please use get_maintainer.pl script. It *must* be sent to devicetree list.
Please send it also to DeviceTree maintainers because you are adding quite generic names for bindings so they may have interesting thoughts on this.
LKML list is also missing.
Ok. Next version will go there too.
Quoted 1 line
Any vendor prefix here? How generic is this?
I just don't know... Does *everything* really need a vendor prefix? How readable would that be? "compatible" property already says
that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-specific device are automatically vendor-specific.
Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree ML.
P.S. I intentionally omit the rest of the text in order to avoid overquoting, since we are already focused on some specific things.
If someone wants to get the whole picture, he/she could just browse back to the start of the thread. Or is it considered wrong approach in this particular ML?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
2015-10-29 16:45 GMT+09:00 Pavel Fedin <[email protected]>:
Quoted 11 lines
Hello!
Quoted 8 lines
You missed here a lot of potential Cc. Please use get_maintainer.pl script. It *must* be sent to devicetree list.
Please send it also to DeviceTree maintainers because you are adding quite generic names for bindings so they may have interesting thoughts on this.
LKML list is also missing.
Quoted 8 lines
Ok. Next version will go there too.
Quoted 1 line
Any vendor prefix here? How generic is this?
I just don't know... Does *everything* really need a vendor prefix? How readable would that be? "compatible" property already says
that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-specific device are automatically vendor-specific.
Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetree ML.
Which my reply you are referring to? You stripped part of some sentence and put it without *any context*. Just random sentence. I asked for vendor prefix in few places... srom-timing? width? And I do not remember where I used exactly these words. This is why *context* is needed.
Quoted 4 lines
P.S. I intentionally omit the rest of the text in order to avoid overquoting, since we are already focused on some specific things.
If someone wants to get the whole picture, he/she could just browse back to the start of the thread. Or is it considered wrong approach in this particular ML?
Yep, selecting some sentences and replying only to them, leaving the context away, is highly confusing. I reply in different places, sometimes asking for similar things. Removing context messes this up. In the same time all of my other questions are being removed without any acknowledgment so I do not know if they were ignored or just silently acked.
I show you example:
Quoted 1 line
I just don't know...
I don't know neither.
Quoted 1 line
If someone wants to get the whole picture, he/she could just browse back to the start of the thread.
Yes, I would just scroll up and see the context. No, wait. There is no context. I have to find mine original email and compare which part was ignored, which was applied and which quoted sentence you are referring to.
Best regards,
Krzysztof
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
Hello!
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Any vendor prefix here? How generic is this?
I just don't know... Does *everything* really need a vendor prefix? How readable would that
be? "compatible" property already says
Quoted 1 line
that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-
specific device are automatically
Quoted 8 lines
Quoted 2 lines
vendor-specific.
Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetreeML.
Which my reply you are referring to? You stripped part of some sentence and put it without *any context*. Just random sentence. I asked for vendor prefix in few places... srom-timing? width? And I do not remember where I used exactly these words.
Ok, sorry, i promise to improve. :)
Anyway, i have figured out how to add sub-devices, and heavily modified the whole thing. And indeed, vendor prefix is now very useful, so i added it to all three properties. Making v4...
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
PATCH v3 1/4 Documentation: dt-bindings: Describe SROMc configuration
Quoted 12 lines · Pavel Fedin
Hello!
Quoted 10 lines
Quoted 3 lines
Quoted 1 line
Any vendor prefix here? How generic is this?
I just don't know... Does *everything* really need a vendor prefix? How readable would that
be? "compatible" property already says
Quoted 1 line
that it's samsung-exynos-specific. And IMHO it's quite obvious that properties of vendor-
specific device are automatically
Quoted 2 lines
vendor-specific.
Ok, i am currently fixing up the rest and will post v4 soon, and will Cc: it to devicetreeML.
Quoted 7 lines
Quoted 4 lines
Which my reply you are referring to? You stripped part of some sentence and put it without *any context*. Just random sentence. I asked for vendor prefix in few places... srom-timing? width? And I do not remember where I used exactly these words.
Ok, sorry, i promise to improve. :)
Anyway, i have figured out how to add sub-devices, and heavily modified the whole thing. And indeed, vendor prefix is now very useful, so i added it to all three properties. Making v4...
Actually now I found:Documentation/devicetree/bindings/net/gpmc-eth.txt
Aren't you duplicating this work? This looks very, very similar.
Best regards,
Krzysztof
PATCH v3 2/4 ARM: dts: Add SROMc to Exynos 5410
In reply to Pavel FedinThis machine uses own SoC device tree file, add missing part.
--- arch/arm/boot/dts/exynos5410.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm/boot/dts/exynos5410.dtsi +5 -0
@@ -101,6 +101,11 @@ reg = <0x10000000 0x100>; }; + sromc: sromc at 12250000 { + compatible = "samsung,exynos-srom"; + reg = <0x12250000 0x14>; + }; + pmu_system_controller: system-controller at 10040000 { compatible = "samsung,exynos5410-pmu", "syscon"; reg = <0x10040000 0x5000>;
Signature · 1 line
2.4.4
PATCH v3 3/4 drivers: exynos-srom: Add support for bank configuration
In reply to Pavel FedinBindings are based on u-boot implementation, however they are stored in subnodes, providing support for more than one bank.
Since the driver now does more than suspend-resume support, dependency on CONFIG_PM is removed.
--- arch/arm/mach-exynos/Kconfig | 2 +- drivers/soc/samsung/Kconfig | 2 +- drivers/soc/samsung/exynos-srom.c | 42 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-)
arch/arm/mach-exynos/Kconfig +1 -1
@@ -16,7 +16,7 @@ menuconfig ARCH_EXYNOS select ARM_GIC select COMMON_CLK_SAMSUNG select EXYNOS_THERMAL - select EXYNOS_SROM if PM + select EXYNOS_SROM select HAVE_ARM_SCU if SMP select HAVE_S3C2410_I2C if I2C select HAVE_S3C2410_WATCHDOG if WATCHDOG
drivers/soc/samsung/Kconfig +1 -1
@@ -8,6 +8,6 @@ config SOC_SAMSUNG config EXYNOS_SROM bool - depends on ARM && ARCH_EXYNOS && PM + depends on ARM && ARCH_EXYNOS endmenu
drivers/soc/samsung/exynos-srom.c +41 -1
56 lines not on this page · raw message
Signature · 1 line
2.4.4
PATCH v3 3/4 drivers: exynos-srom: Add support for bank configuration
Quoted 44 lines · Pavel Fedin
Bindings are based on u-boot implementation, however they are stored in subnodes, providing support for more than one bank.
Since the driver now does more than suspend-resume support, dependency on
CONFIG_PMis removed.Signed-off-by Pavel Fedin <[email protected]>--- arch/arm/mach-exynos/Kconfig | 2 +- drivers/soc/samsung/Kconfig | 2 +- drivers/soc/samsung/exynos-srom.c | 42 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 3 deletions(-)arch/arm/mach-exynos/Kconfig +1
-1@@ -16,7 +16,7 @@ menuconfig ARCH_EXYNOS select ARM_GIC select COMMON_CLK_SAMSUNG select EXYNOS_THERMAL - select EXYNOS_SROM if PM + select EXYNOS_SROM select HAVE_ARM_SCU if SMP select HAVE_S3C2410_I2C if I2C select HAVE_S3C2410_WATCHDOG if WATCHDOGdrivers/soc/samsung/Kconfig +1
-1@@ -8,6 +8,6 @@ config SOC_SAMSUNG config EXYNOS_SROM bool - depends on ARM && ARCH_EXYNOS && PM + depends on ARM && ARCH_EXYNOS endmenudrivers/soc/samsung/exynos-srom.c +0
-0@@ -67,9 +67,46 @@ static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump(return rd;
drivers/soc/samsung/exynos-srom.c static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump( · line 77
Quoted 10 lines
drivers/soc/samsung/exynos-srom.c +8
-0} +static int decode_sromc (struct exynos_srom *srom,struct device_node *np) +{ +u32 bank, width; +u32 timing[7 ]; +u32 bw; + +if (of_property_read_u32 (np,"bank" , &bank)) +return -ENXIO ;
This is an invalid value in DTB, not missing device or address, so -EINVAL.
drivers/soc/samsung/exynos-srom.c static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump( · line 80
Quoted 3 lines
drivers/soc/samsung/exynos-srom.c +3
-0+if (of_property_read_u32 (np,"width" , &width)) + width =1 ; +if (of_property_read_u32_array (np,"srom-timing" , timing,7 )) {
s/7/ARRAY_SIZE/
drivers/soc/samsung/exynos-srom.c static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump( · line 81
Quoted 1 line
drivers/soc/samsung/exynos-srom.c +1
-0+pr_err ("Could not decode SROMC configuration\n" );
dev_err()
drivers/soc/samsung/exynos-srom.c static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump( · line 82
Quoted 1 line
drivers/soc/samsung/exynos-srom.c +1
-0+return -ENXIO ;
Here also I would prefer -EINVAL
drivers/soc/samsung/exynos-srom.c static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump( · line 92
Quoted 10 lines
drivers/soc/samsung/exynos-srom.c +10
-0+ } + + bank *=4 ;/* Convert bank into shift/offset */ + + bw =1 <<EXYNOS_SROM_BW__BYTEENABLE__SHIFT ; +if (width ==2 ) + bw |=1 <<EXYNOS_SROM_BW__DATAWIDTH__SHIFT ; + bw <<= bank; + bw |=__raw_readl (srom->reg_base +EXYNOS_SROM_BW ) & + ~(EXYNOS_SROM_BW__CS_MASK << bank);
This is reversed pattern. First read, then set or clear bits and finally write. It makes easier to understand what is the intention.
Quoted 22 lines
drivers/soc/samsung/exynos-srom.c +15
-1+__raw_writel (bw, srom->reg_base +EXYNOS_SROM_BW ); + +__raw_writel ((timing[0 ] <<EXYNOS_SROM_BCX__PMC__SHIFT ) | + (timing[1 ] <<EXYNOS_SROM_BCX__TACP__SHIFT ) | + (timing[2 ] <<EXYNOS_SROM_BCX__TCAH__SHIFT ) | + (timing[3 ] <<EXYNOS_SROM_BCX__TCOH__SHIFT ) | + (timing[4 ] <<EXYNOS_SROM_BCX__TACC__SHIFT ) | + (timing[5 ] <<EXYNOS_SROM_BCX__TCOS__SHIFT ) | + (timing[6 ] <<EXYNOS_SROM_BCX__TACS__SHIFT ), + srom->reg_base +EXYNOS_SROM_BC0 + bank); + +return 0 ; +} +static int exynos_srom_probe (struct platform_device *pdev) { -struct device_node *np; +struct device_node *np, *child;struct exynos_srom *srom;struct device *dev = &pdev->dev;
drivers/soc/samsung/exynos-srom.c static int exynos_srom_probe(struct platform_device *pdev) · line 141
Quoted 5 lines
drivers/soc/samsung/exynos-srom.c +2
-0@@ -100,6 +137,9 @@ static int exynos_srom_probe(struct platform_device *pdev)return -ENOMEM ; } +for_each_child_of_node (np, child) +decode_sromc (srom, child);
You ignore the return value here so bank may be not configured but device probe will return 0.
Maybe clean up and fail the probe?
Best regards,
Krzysztof
Quoted 5 lines
drivers/soc/samsung/exynos-srom.c +1
-0+return 0 ; }
PATCH v3 3/4 drivers: exynos-srom: Add support for bank configuration
Hello!
Quoted 5 lines
drivers/soc/samsung/exynos-srom.c static int exynos_srom_probe(struct platform_device *pdev) · line 141
Quoted 2 lines
drivers/soc/samsung/exynos-srom.c +2
-0+for_each_child_of_node (np, child) +decode_sromc (srom, child);You ignore the return value here so bank may be not configured but device probe will return 0.
Yes, so that banks which are described correctly, will still be configured.
Quoted 1 line
Maybe clean up and fail the probe?
I think it's not fatal, so cleanup is not necessary. May be dev_warn() in this case?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
PATCH v3 3/4 drivers: exynos-srom: Add support for bank configuration
Quoted 2 lines · Pavel Fedin
Hello!
Where is the contents of email and patch?
Quoted 7 lines
Quoted 5 lines
drivers/soc/samsung/exynos-srom.c static int exynos_srom_probe(struct platform_device *pdev) · line 141
Quoted 2 lines
drivers/soc/samsung/exynos-srom.c +2
-0+for_each_child_of_node (np, child) +decode_sromc (srom, child);You ignore the return value here so bank may be not configured but device probe will return 0.
Yes, so that banks which are described correctly, will still be configured.
But the system won't get any information about the failure. Device using these banks should not probe in such case.
Quoted 4 lines
Quoted 2 lines
Maybe clean up and fail the probe?
I think it's not fatal, so cleanup is not necessary. May be
dev_warn()in this case?
Now this is just silently ignoring return value (which is not needed then) and not printing any errors. But that actually depends on previous comment - whether the driver will fail the probe.
Best regards,
Krzysztof
PATCH v3 3/4 drivers: exynos-srom: Add support for bank configuration
Hello!
Quoted 2 lines
But the system won't get any information about the failure. Device using these banks should not probe in such case.
In order to achieve this effect, i believe, it's not enough to fail SROMc probe. Peripherials (such as smsc) should then be
declared as subnodes.
There's one problem with this, however. I could not manage to do it. If i place device's node inside of SROMc node, it does not get
probed at all, and i don't really understand why. I tried to read the code and i suggest that only specific devices (like compatible="simple-bus") are searched for sub-devices.
OTOH, this patch (http://www.spinics.net/lists/arm-kernel/msg449703.html) perfectly worked, and generic timer node placed inside of
MCT node was perfectly probed by the kernel. So i still don't understand why i could not place smsc description inside of SROMc.
May be you can help me with this?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
PATCH v3 4/4 ARM: dts: Add Ethernet chip to SMDK5410
In reply to Pavel FedinThe chip is smsc9115, connected via SROMc bank 3. Additionally, some GPIO initialization is required.
--- arch/arm/boot/dts/exynos5410-smdk5410.dts | 42 +++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
arch/arm/boot/dts/exynos5410-smdk5410.dts +42 -0
@@ -35,6 +35,17 @@ reg = <0x02037000 0x1000>; }; + ethernet at 07000000 { + compatible = "smsc,lan9115"; + reg = <0x07000000 0x10000>; + phy-mode = "mii"; + interrupt-parent = <&gpx0>; + interrupts = <5 8>; + reg-io-width = <2>; + smsc,irq-push-pull; + smsc,force-internal-phy; + }; + }; &mmc_0 {
@@ -61,6 +72,27 @@ disable-wp; }; +&pinctrl_0 { + srom_ctl: srom-ctl { + samsung,pins = "gpy0-3", "gpy0-4", "gpy0-5", + "gpy1-0", "gpy1-1", "gpy1-2", "gpy1-3"; + samsung,pin-function = <2>; + samsung,pin-drv = <0>; + }; + + srom_ebi: srom-ebi { + samsung,pins = "gpy3-0", "gpy3-1", "gpy3-2", "gpy3-3", + "gpy3-4", "gpy3-5", "gpy3-6", "gpy3-7", + "gpy5-0", "gpy5-1", "gpy5-2", "gpy5-3", + "gpy5-4", "gpy5-5", "gpy5-6", "gpy5-7", + "gpy6-0", "gpy6-1", "gpy6-2", "gpy6-3", + "gpy6-4", "gpy6-5", "gpy6-6", "gpy6-7"; + samsung,pin-function = <2>; + samsung,pin-pud = <3>; + samsung,pin-drv = <0>; + }; +}; + &uart0 { status = "okay"; };
@@ -72,3 +104,13 @@ &uart2 { status = "okay"; }; + +&sromc { + pinctrl-names = "default"; + pinctrl-0 = <&srom_ctl>, <&srom_ebi>; + bank at 3 { + bank = <3>; + width = <2>; + srom-timing = <1 9 12 1 9 1 1>; + }; +};
Signature · 1 line
2.4.4