| Message ID | 1388507534-10570-7-git-send-email-jjhiblot@traphandler.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 31 December 2013 17:32:14 jjhiblot@traphandler.com wrote: > > + ethernet@30000000 { > + compatible = "davicom,dm9000"; > + reg = <0x30000000 0x2 0x30000004 0x2>; > + interrupt-parent = <&pioC>; > + interrupts = <11 IRQ_TYPE_EDGE_BOTH>; > + local-mac-address = [00 00 de ad be ef]; > + davicom,no-eeprom; > + > + smc,cs = <2>; > + smc,ncs_read_setup = <0>; > + smc,nrd_setup = <2>; > + smc,ncs_write_setup = <0>; > + smc,nwe_setup = <2>; > + smc,ncs_read_pulse = <8>; > + smc,nrd_pulse = <4>; > + smc,ncs_write_pulse = <8>; > + smc,nwe_pulse = <4>; > + smc,read_cycle = <16>; > + smc,write_cycle = <16>; > + smc,tdf_cycles = <1>; > + smc,tdf_optimized = <0>; > + smc,page_size = <0>; > + smc,byte_access_type = <1>; > + smc,bus_width = <1>; > + smc,nwait_mode = <0>; > + smc,read_mode = <1>; > + smc,write_mode = <1>; > + }; I don't like how this forces you to mix the properties of the ethernet device with the properties of the smc port. I would also like to see the address translation reflected in the DT nodes. This should probably just work out if you restructure the DT representation, and won't need changes in the driver. How about this: smc: smc@ffffec00 { #address-cells = <2>; /* cs, address */ #size-cells = <1>; ranges = <2 0 0x30000000 0x10000000> <3 0 0x40000000 0x10000000>; extbus@2.0 { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; ranges = <0 2 0 0x10000000>; smc,ncs_read_setup = <0>; smc,nrd_setup = <2>; smc,... ethernet@0 { compatible = "davicom,dm9000"; reg = <0x0 0x2> <0x4 0x2>; }; }; }; If each bus interface has a fixed range of registers, you can actually collapse the upper two nodes again and just have one device per chipselect. Arnd
2013/12/31 Arnd Bergmann <arnd@arndb.de>: > On Tuesday 31 December 2013 17:32:14 jjhiblot@traphandler.com wrote: >> >> + ethernet@30000000 { >> + compatible = "davicom,dm9000"; >> + reg = <0x30000000 0x2 0x30000004 0x2>; >> + interrupt-parent = <&pioC>; >> + interrupts = <11 IRQ_TYPE_EDGE_BOTH>; >> + local-mac-address = [00 00 de ad be ef]; >> + davicom,no-eeprom; >> + >> + smc,cs = <2>; >> + smc,ncs_read_setup = <0>; >> + smc,nrd_setup = <2>; >> + smc,ncs_write_setup = <0>; >> + smc,nwe_setup = <2>; >> + smc,ncs_read_pulse = <8>; >> + smc,nrd_pulse = <4>; >> + smc,ncs_write_pulse = <8>; >> + smc,nwe_pulse = <4>; >> + smc,read_cycle = <16>; >> + smc,write_cycle = <16>; >> + smc,tdf_cycles = <1>; >> + smc,tdf_optimized = <0>; >> + smc,page_size = <0>; >> + smc,byte_access_type = <1>; >> + smc,bus_width = <1>; >> + smc,nwait_mode = <0>; >> + smc,read_mode = <1>; >> + smc,write_mode = <1>; >> + }; > > I don't like how this forces you to mix the properties > of the ethernet device with the properties of the smc > port. I would also like to see the address translation > reflected in the DT nodes. This should probably just work > out if you restructure the DT representation, and won't need > changes in the driver. > > How about this: > > smc: smc@ffffec00 { > #address-cells = <2>; /* cs, address */ > #size-cells = <1>; > ranges = <2 0 0x30000000 0x10000000> > <3 0 0x40000000 0x10000000>; > > extbus@2.0 { > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0 2 0 0x10000000>; > smc,ncs_read_setup = <0>; > smc,nrd_setup = <2>; > smc,... > > ethernet@0 { > compatible = "davicom,dm9000"; > reg = <0x0 0x2> <0x4 0x2>; > }; > }; > }; I'm okay with this. I'll do something along those lines in the next round of the patch set. > > If each bus interface has a fixed range of registers, you can actually > collapse the upper two nodes again and just have one device per > chipselect. This is indeed the case, but I'd rather keep the top node to highlight the fact that all the devices are physically tied to the same bus. Jean-Jacques > > Arnd > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thursday 02 January 2014 20:31:00 Jean-Jacques Hiblot wrote: > > If each bus interface has a fixed range of registers, you can actually > > collapse the upper two nodes again and just have one device per > > chipselect. > This is indeed the case, but I'd rather keep the top node to highlight > the fact that all the devices are physically tied to the same bus. How about using a top-level "simple-bus" node with the complex ranges property but making the driver match the sub-devices with the smaller register range then? Arnd
Hi Arnd, sorry for the delay. I'll rework the driver to include some ideas proposed by you and Boris. Boris proposed to have several timing decoders (a generic one, one for NAND and so on). I plan to use the compatibility name to select the decoder.and use the DT representation you proposed last : one top "simple-bus" and one sub-node "atmel,smc" per chip-select with one (or more) sub-device per chip-select. Jean-Jacques 2014/1/2 Arnd Bergmann <arnd@arndb.de>: > On Thursday 02 January 2014 20:31:00 Jean-Jacques Hiblot wrote: >> > If each bus interface has a fixed range of registers, you can actually >> > collapse the upper two nodes again and just have one device per >> > chipselect. >> This is indeed the case, but I'd rather keep the top node to highlight >> the fact that all the devices are physically tied to the same bus. > > How about using a top-level "simple-bus" node with the complex ranges > property but making the driver match the sub-devices with the smaller > register range then? > > Arnd > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm/boot/dts/at91sam9261ek.dts b/arch/arm/boot/dts/at91sam9261ek.dts index 11cf619..7a7997f 100644 --- a/arch/arm/boot/dts/at91sam9261ek.dts +++ b/arch/arm/boot/dts/at91sam9261ek.dts @@ -76,6 +76,35 @@ smc: smc@ffffec00 { status = "okay"; + ethernet@30000000 { + compatible = "davicom,dm9000"; + reg = <0x30000000 0x2 0x30000004 0x2>; + interrupt-parent = <&pioC>; + interrupts = <11 IRQ_TYPE_EDGE_BOTH>; + local-mac-address = [00 00 de ad be ef]; + davicom,no-eeprom; + + smc,cs = <2>; + smc,ncs_read_setup = <0>; + smc,nrd_setup = <2>; + smc,ncs_write_setup = <0>; + smc,nwe_setup = <2>; + smc,ncs_read_pulse = <8>; + smc,nrd_pulse = <4>; + smc,ncs_write_pulse = <8>; + smc,nwe_pulse = <4>; + smc,read_cycle = <16>; + smc,write_cycle = <16>; + smc,tdf_cycles = <1>; + smc,tdf_optimized = <0>; + smc,page_size = <0>; + smc,byte_access_type = <1>; + smc,bus_width = <1>; + smc,nwait_mode = <0>; + smc,read_mode = <1>; + smc,write_mode = <1>; + }; + nand0: nand@40000000 { nand-bus-width = <8>; nand-ecc-mode = "soft";