| Message ID | 1351498881-32482-6-git-send-email-hvaibhav@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>>>> "Vaibhav" == Vaibhav Hiremath <hvaibhav@ti.com> writes: Vaibhav> From: Mugunthan V N <mugunthanvnm@ti.com> Vaibhav> Add CPSW and MDIO related device tree data for AM33XX. Vaibhav> Also enable them into board/evm dts files by providing Vaibhav> respective phy-id. Vaibhav> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> Vaibhav> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com> Vaibhav> Cc: Richard Cochran <richardcochran@gmail.com> Vaibhav> Cc: Benoit Cousson <b-cousson@ti.com> Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Hi, On 10/29/2012 09:21 AM, Vaibhav Hiremath wrote: > From: Mugunthan V N <mugunthanvnm@ti.com> > > Add CPSW and MDIO related device tree data for AM33XX. > Also enable them into board/evm dts files by providing > respective phy-id. Is there any bindings documentation for that device? > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com> > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Benoit Cousson <b-cousson@ti.com> > --- > arch/arm/boot/dts/am335x-bone.dts | 8 ++++++ > arch/arm/boot/dts/am335x-evm.dts | 8 ++++++ > arch/arm/boot/dts/am33xx.dtsi | 50 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 66 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts > index c634f87..e233cfa 100644 > --- a/arch/arm/boot/dts/am335x-bone.dts > +++ b/arch/arm/boot/dts/am335x-bone.dts > @@ -78,3 +78,11 @@ > }; > }; > }; > + > +&cpsw_emac0 { > + phy_id = "4a101000.mdio:00"; Why are you using that kind of interface? You seem to want a reference to a device. Cannot you have something less hard coded like: phy_id = <&davinci_mdio>, <0>; > +}; > + > +&cpsw_emac1 { > + phy_id = "4a101000.mdio:01"; > +}; > diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts > index 185d632..415c3b3 100644 > --- a/arch/arm/boot/dts/am335x-evm.dts > +++ b/arch/arm/boot/dts/am335x-evm.dts > @@ -118,3 +118,11 @@ > }; > }; > }; > + > +&cpsw_emac0 { > + phy_id = "4a101000.mdio:00"; > +}; > + > +&cpsw_emac1 { > + phy_id = "4a101000.mdio:01"; > +}; > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index bb31bff..f6bea04 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -210,5 +210,55 @@ > interrupt-parent = <&intc>; > interrupts = <91>; > }; > + > + mac: ethernet@4A100000 { hexa data should be in lower case. > + compatible = "ti,cpsw"; > + ti,hwmods = "cpgmac0"; > + cpdma_channels = <8>; > + host_port_no = <0>; > + cpdma_reg_ofs = <0x800>; > + cpdma_sram_ofs = <0xa00>; > + ale_reg_ofs = <0xd00>; > + ale_entries = <1024>; > + host_port_reg_ofs = <0x108>; > + hw_stats_reg_ofs = <0x900>; > + bd_ram_ofs = <0x2000>; > + bd_ram_size = <0x2000>; > + no_bd_ram = <0>; > + rx_descs = <64>; > + mac_control = <0x20>; Do you have to store all these data in the DTS? Cannot it be in the driver? Do you expect to have several instance of the same IP with different parameters here? > + slaves = <2>; > + reg = <0x4a100000 0x800 > + 0x4a101200 0x100 > + 0x4a101000 0x100>; Please align the address. > + #address-cells = <1>; > + #size-cells = <1>; > + interrupt-parent = <&intc>; > + /* c0_rx_thresh_pend c0_rx_pend c0_tx_pend c0_misc_pend*/ Please use a standard multi-line comment instead of trying to put everything in one line. > + interrupts = <40 41 42 43>; > + ranges; You should add blank line here for readability. > + cpsw_emac0: slave@0 { Mmm, you are using some address later and here some relative number, that does not looks very consistent. > + slave_reg_ofs = <0x208>; Is it an offset from 4a100000? Cannot you use the address for the slave name? Something like that: cpsw_emac0: slave@4a100208 > + sliver_reg_ofs = <0xd80>; > + /* Filled in by U-Boot */ > + mac-address = [ 00 00 00 00 00 00 ]; > + }; You should add blank line here for readability. > + cpsw_emac1: slave@1 { > + slave_reg_ofs = <0x308>; > + sliver_reg_ofs = <0xdc0>; > + /* Filled in by U-Boot */ > + mac-address = [ 00 00 00 00 00 00 ]; > + }; > + > + davinci_mdio: mdio@4a101000 { > + compatible = "ti,davinci_mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + ti,hwmods = "davinci_mdio"; > + bus_freq = <1000000>; > + reg = <0x4a101000 0x100>; > + }; > + Stray change. Regards, Benoit
On Wed, Oct 31, 2012 at 20:47:27, Cousson, Benoit wrote: > Hi, > > On 10/29/2012 09:21 AM, Vaibhav Hiremath wrote: > > From: Mugunthan V N <mugunthanvnm@ti.com> > > > > Add CPSW and MDIO related device tree data for AM33XX. > > Also enable them into board/evm dts files by providing > > respective phy-id. > > Is there any bindings documentation for that device? > Yes, the base DT binding documentation is present in file Documentation/devicetree/bindings/net/cpsw.txt > > Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com> > > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com> > > Cc: Richard Cochran <richardcochran@gmail.com> > > Cc: Benoit Cousson <b-cousson@ti.com> > > --- > > arch/arm/boot/dts/am335x-bone.dts | 8 ++++++ > > arch/arm/boot/dts/am335x-evm.dts | 8 ++++++ > > arch/arm/boot/dts/am33xx.dtsi | 50 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 66 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts > > index c634f87..e233cfa 100644 > > --- a/arch/arm/boot/dts/am335x-bone.dts > > +++ b/arch/arm/boot/dts/am335x-bone.dts > > @@ -78,3 +78,11 @@ > > }; > > }; > > }; > > + > > +&cpsw_emac0 { > > + phy_id = "4a101000.mdio:00"; > > Why are you using that kind of interface? You seem to want a reference > to a device. > > Cannot you have something less hard coded like: > phy_id = <&davinci_mdio>, <0>; > > > > +}; > > + > > +&cpsw_emac1 { > > + phy_id = "4a101000.mdio:01"; > > +}; > > diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts > > index 185d632..415c3b3 100644 > > --- a/arch/arm/boot/dts/am335x-evm.dts > > +++ b/arch/arm/boot/dts/am335x-evm.dts > > @@ -118,3 +118,11 @@ > > }; > > }; > > }; > > + > > +&cpsw_emac0 { > > + phy_id = "4a101000.mdio:00"; > > +}; > > + > > +&cpsw_emac1 { > > + phy_id = "4a101000.mdio:01"; > > +}; > > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > > index bb31bff..f6bea04 100644 > > --- a/arch/arm/boot/dts/am33xx.dtsi > > +++ b/arch/arm/boot/dts/am33xx.dtsi > > @@ -210,5 +210,55 @@ > > interrupt-parent = <&intc>; > > interrupts = <91>; > > }; > > + > > + mac: ethernet@4A100000 { > > hexa data should be in lower case. > Oops. Will correct it. > > + compatible = "ti,cpsw"; > > + ti,hwmods = "cpgmac0"; > > + cpdma_channels = <8>; > > + host_port_no = <0>; > > + cpdma_reg_ofs = <0x800>; > > + cpdma_sram_ofs = <0xa00>; > > + ale_reg_ofs = <0xd00>; > > + ale_entries = <1024>; > > + host_port_reg_ofs = <0x108>; > > + hw_stats_reg_ofs = <0x900>; > > + bd_ram_ofs = <0x2000>; > > + bd_ram_size = <0x2000>; > > + no_bd_ram = <0>; > > + rx_descs = <64>; > > + mac_control = <0x20>; > > Do you have to store all these data in the DTS? Cannot it be in the driver? > > Do you expect to have several instance of the same IP with different > parameters here? > I will let Mugunthan respond to this. > > + slaves = <2>; > > + reg = <0x4a100000 0x800 > > + 0x4a101200 0x100 > > + 0x4a101000 0x100>; > > Please align the address. Ok. > > > + #address-cells = <1>; > > + #size-cells = <1>; > > + interrupt-parent = <&intc>; > > + /* c0_rx_thresh_pend c0_rx_pend c0_tx_pend c0_misc_pend*/ > > Please use a standard multi-line comment instead of trying to put > everything in one line. > Oops. Will correct it. > > + interrupts = <40 41 42 43>; > > + ranges; > > You should add blank line here for readability. > OK. > > + cpsw_emac0: slave@0 { > > Mmm, you are using some address later and here some relative number, > that does not looks very consistent. > > > + slave_reg_ofs = <0x208>; > > Is it an offset from 4a100000? Cannot you use the address for the slave > name? > > Something like that: cpsw_emac0: slave@4a100208 > > > + sliver_reg_ofs = <0xd80>; > > + /* Filled in by U-Boot */ > > + mac-address = [ 00 00 00 00 00 00 ]; > > + }; > > You should add blank line here for readability. > Ok Thanks, Vaibhav
On Wed, Oct 31, 2012 at 04:17:27PM +0100, Benoit Cousson wrote: > > + compatible = "ti,cpsw"; > > + ti,hwmods = "cpgmac0"; > > + cpdma_channels = <8>; > > + host_port_no = <0>; > > + cpdma_reg_ofs = <0x800>; > > + cpdma_sram_ofs = <0xa00>; > > + ale_reg_ofs = <0xd00>; > > + ale_entries = <1024>; > > + host_port_reg_ofs = <0x108>; > > + hw_stats_reg_ofs = <0x900>; > > + bd_ram_ofs = <0x2000>; > > + bd_ram_size = <0x2000>; > > + no_bd_ram = <0>; > > + rx_descs = <64>; > > + mac_control = <0x20>; > > Do you have to store all these data in the DTS? Cannot it be in the driver? > > Do you expect to have several instance of the same IP with different > parameters here? As I understand it, there are only two different layouts for the CPSW, the one in the dm814x and the one in the am335x. So I think it would work to put only the version register offet in the DT, and the let the driver figure out the rest from there. But if TI is planning on reordering the registers with each new silicon revision, again and again, then it might make sense to keep the offsets in the DT. [ I really wonder why the hardware people think that reshuffling the register layout constitutes an improvement. ] Thanks, Richard
On 11/1/2012 8:45 AM, Richard Cochran wrote: > On Wed, Oct 31, 2012 at 04:17:27PM +0100, Benoit Cousson wrote: >>> + compatible = "ti,cpsw"; >>> + ti,hwmods = "cpgmac0"; >>> + cpdma_channels = <8>; >>> + host_port_no = <0>; >>> + cpdma_reg_ofs = <0x800>; >>> + cpdma_sram_ofs = <0xa00>; >>> + ale_reg_ofs = <0xd00>; >>> + ale_entries = <1024>; >>> + host_port_reg_ofs = <0x108>; >>> + hw_stats_reg_ofs = <0x900>; >>> + bd_ram_ofs = <0x2000>; >>> + bd_ram_size = <0x2000>; >>> + no_bd_ram = <0>; >>> + rx_descs = <64>; >>> + mac_control = <0x20>; >> >> Do you have to store all these data in the DTS? Cannot it be in the driver? >> >> Do you expect to have several instance of the same IP with different >> parameters here? > > As I understand it, there are only two different layouts for the CPSW, > the one in the dm814x and the one in the am335x. So I think it would > work to put only the version register offet in the DT, and the let the > driver figure out the rest from there. Yes, that's indeed better. We did that for other IPs already (GPIO, I2C...) > But if TI is planning on reordering the registers with each new > silicon revision, again and again, then it might make sense to keep > the offsets in the DT. Yeah, let's assume they will do a better job in the future. All these offset registers information does belong to the driver, and even if the HW change a lot, I still rather hide that in the driver. It will always be cleaner, most efficient, and will reduce the size if the blob. > [ I really wonder why the hardware people think that reshuffling the > register layout constitutes an improvement. ] I've been wondering that for ten years :-( I'm always hoping it will be better some day. Regards, Benoit
> -----Original Message----- > From: Cousson, Benoit > Sent: Wednesday, October 31, 2012 8:47 PM > To: Hiremath, Vaibhav > Cc: netdev@vger.kernel.org; paul@pwsan.com; linux-arm- > kernel@lists.infradead.org; linux-omap@vger.kernel.org; N, Mugunthan V; > Richard Cochran > Subject: Re: [PATCH 4/4] arm/dts: am33xx: Add CPSW and MDIO module > nodes for AM33XX > > + compatible = "ti,cpsw"; > > + ti,hwmods = "cpgmac0"; > > + cpdma_channels = <8>; > > + host_port_no = <0>; > > + cpdma_reg_ofs = <0x800>; > > + cpdma_sram_ofs = <0xa00>; > > + ale_reg_ofs = <0xd00>; > > + ale_entries = <1024>; > > + host_port_reg_ofs = <0x108>; > > + hw_stats_reg_ofs = <0x900>; > > + bd_ram_ofs = <0x2000>; > > + bd_ram_size = <0x2000>; > > + no_bd_ram = <0>; > > + rx_descs = <64>; > > + mac_control = <0x20>; > > Do you have to store all these data in the DTS? Cannot it be in the > driver? > > Do you expect to have several instance of the same IP with different > parameters here? Though CPSW is a single IP in AM335X, CPSW has sub modules like CPDMA, ALE, SLIVER, CPTS and SLAVES where is IP integrator can locate it at different offsets. For example comparing the CPSW ip in TI814X and AM335X all the above offsets are changed. So I have kept all these offsets in DT as driver should not hold any SoC related informations > > + cpsw_emac0: slave@0 { > > Mmm, you are using some address later and here some relative number, > that does not looks very consistent. > > > + slave_reg_ofs = <0x208>; > > Is it an offset from 4a100000? Cannot you use the address for the slave > name? > > Something like that: cpsw_emac0: slave@4a100208 > I have followed the naming convention as per the TRM which is mentioned as Slave 0 and Slave 1 and its offset is from 0x4a100000. I have taken the reference from arch/arm/boot/dts/picoxcell-pc3x2.dtsi+167 and followed the same naming convention from TRM. There is one more method to implement in reference from arch/arm/boot/dts/imx6q.dtsi+408 as below + cpsw_emac0: slave@208 { + slave_reg_ofs = <0x208>; + sliver_reg_ofs = <0xd80>; + /* Filled in by U-Boot */ + mac-address = [ 00 00 00 00 00 00 ]; + }; Regards Mugunthan V N
On Fri, Nov 02, 2012 at 08:46:46AM +0000, N, Mugunthan V wrote: > > > > Do you expect to have several instance of the same IP with different > > parameters here? > > Though CPSW is a single IP in AM335X, CPSW has sub modules like CPDMA, ALE, > SLIVER, CPTS and SLAVES where is IP integrator can locate it at different > offsets. For example comparing the CPSW ip in TI814X and AM335X all the > above offsets are changed. So I have kept all these offsets in DT as driver > should not hold any SoC related informations Did you see the two messages on this point from yesterday? Thanks, Richard
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Friday, November 02, 2012 2:26 PM > To: N, Mugunthan V > Cc: Cousson, Benoit; Hiremath, Vaibhav; netdev@vger.kernel.org; > paul@pwsan.com; linux-arm-kernel@lists.infradead.org; linux- > omap@vger.kernel.org > Subject: Re: [PATCH 4/4] arm/dts: am33xx: Add CPSW and MDIO module > nodes for AM33XX > > On Fri, Nov 02, 2012 at 08:46:46AM +0000, N, Mugunthan V wrote: > > > > > > Do you expect to have several instance of the same IP with > different > > > parameters here? > > > > Though CPSW is a single IP in AM335X, CPSW has sub modules like > CPDMA, ALE, > > SLIVER, CPTS and SLAVES where is IP integrator can locate it at > different > > offsets. For example comparing the CPSW ip in TI814X and AM335X all > the > > above offsets are changed. So I have kept all these offsets in DT as > driver > > should not hold any SoC related informations > > Did you see the two messages on this point from yesterday? I saw those posts. The CPSW ip version changes tracks the internal IP changes and there is no possible way to track the offset changes. For example CPTS sub module offsets in DM814x and AM335x are different though the CPTS version is same in both IP versions. So keeping these offset in DT will make the same driver works directly with DT changes for future SoC. Regards Mugunthan V N
On Fri, Nov 02, 2012 at 10:42:04AM +0000, N, Mugunthan V wrote: > > I saw those posts. The CPSW ip version changes tracks the internal IP > changes and there is no possible way to track the offset changes. For > example CPTS sub module offsets in DM814x and AM335x are different > though the CPTS version is same in both IP versions. So keeping these > offset in DT will make the same driver works directly with DT changes > for future SoC. But the CPSW versions are different, and the offsets could be determined that way, couldn't they? The TRM for the DM814x does not even make the distinction among CPSW_SS, CPSW_PORT, CPSW_CPDMA, and so on. Instead, it places all of the registers into one space called CPSW_3G. So I agree with Benoit. Placing all of the offsets into DT seems like over-engineering to me, unless you know of TI's plans to release a new SoC with the same CPSW version but different register offsets. Thanks, Richard
This patch is a follow up to show how to remove all the various register offsets from the DT for the CPSW driver. This applies on top of the bug fix I posted earlier for IO mapping leak. Since I am currently awaiting a replacement for my defective BeagleBone, this patch is compile tested only. Thanks, Richard Richard Cochran (1): cpsw: simplify the setup of the register pointers Documentation/devicetree/bindings/net/cpsw.txt | 34 ---- drivers/net/ethernet/ti/cpsw.c | 209 +++++++++-------------- include/linux/platform_data/cpsw.h | 19 -- 3 files changed, 82 insertions(+), 180 deletions(-)
On 10/31/2012 8:47 PM, Benoit Cousson wrote: > diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts > index c634f87..e233cfa 100644 > --- a/arch/arm/boot/dts/am335x-bone.dts > +++ b/arch/arm/boot/dts/am335x-bone.dts > @@ -78,3 +78,11 @@ > }; > }; > }; > + > +&cpsw_emac0 { > + phy_id = "4a101000.mdio:00"; > Why are you using that kind of interface? You seem to want a reference > to a device. > > Cannot you have something less hard coded like: > phy_id = <&davinci_mdio>, <0>; Benoit I am having a issue in converting the above phy_id conversion from "<&davinci_mdio>, <0>" to "4a101000.mdio:00". Since davinci_mdio is a child node to cpsw, the platform device for mdio is not created to get the mdio device name "4a101000.mdio". Can you point me a reference to get the right implementation. Regards Mugunthan V N
diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts index c634f87..e233cfa 100644 --- a/arch/arm/boot/dts/am335x-bone.dts +++ b/arch/arm/boot/dts/am335x-bone.dts @@ -78,3 +78,11 @@ }; }; }; + +&cpsw_emac0 { + phy_id = "4a101000.mdio:00"; +}; + +&cpsw_emac1 { + phy_id = "4a101000.mdio:01"; +}; diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index 185d632..415c3b3 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -118,3 +118,11 @@ }; }; }; + +&cpsw_emac0 { + phy_id = "4a101000.mdio:00"; +}; + +&cpsw_emac1 { + phy_id = "4a101000.mdio:01"; +}; diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index bb31bff..f6bea04 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -210,5 +210,55 @@ interrupt-parent = <&intc>; interrupts = <91>; }; + + mac: ethernet@4A100000 { + compatible = "ti,cpsw"; + ti,hwmods = "cpgmac0"; + cpdma_channels = <8>; + host_port_no = <0>; + cpdma_reg_ofs = <0x800>; + cpdma_sram_ofs = <0xa00>; + ale_reg_ofs = <0xd00>; + ale_entries = <1024>; + host_port_reg_ofs = <0x108>; + hw_stats_reg_ofs = <0x900>; + bd_ram_ofs = <0x2000>; + bd_ram_size = <0x2000>; + no_bd_ram = <0>; + rx_descs = <64>; + mac_control = <0x20>; + slaves = <2>; + reg = <0x4a100000 0x800 + 0x4a101200 0x100 + 0x4a101000 0x100>; + #address-cells = <1>; + #size-cells = <1>; + interrupt-parent = <&intc>; + /* c0_rx_thresh_pend c0_rx_pend c0_tx_pend c0_misc_pend*/ + interrupts = <40 41 42 43>; + ranges; + cpsw_emac0: slave@0 { + slave_reg_ofs = <0x208>; + sliver_reg_ofs = <0xd80>; + /* Filled in by U-Boot */ + mac-address = [ 00 00 00 00 00 00 ]; + }; + cpsw_emac1: slave@1 { + slave_reg_ofs = <0x308>; + sliver_reg_ofs = <0xdc0>; + /* Filled in by U-Boot */ + mac-address = [ 00 00 00 00 00 00 ]; + }; + + davinci_mdio: mdio@4a101000 { + compatible = "ti,davinci_mdio"; + #address-cells = <1>; + #size-cells = <0>; + ti,hwmods = "davinci_mdio"; + bus_freq = <1000000>; + reg = <0x4a101000 0x100>; + }; + + }; }; };