netdev - Re: [PATCH 4/4] arm/dts: am33xx: Add CPSW and MDIO module nodes for AM33XX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 31 Oct 2012 16:17:27 +0100
From:	Benoit Cousson <b-cousson@...com>
To:	Vaibhav Hiremath <hvaibhav@...com>
CC:	<netdev@...r.kernel.org>, <paul@...an.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-omap@...r.kernel.org>, Mugunthan V N <mugunthanvnm@...com>,
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH 4/4] arm/dts: am33xx: Add CPSW and MDIO module nodes for
 AM33XX

Hi,

On 10/29/2012 09:21 AM, Vaibhav Hiremath wrote:
> From: Mugunthan V N <mugunthanvnm@...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@...com>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@...com>
> Cc: Richard Cochran <richardcochran@...il.com>
> Cc: Benoit Cousson <b-cousson@...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@...00000 {

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@...00208

> +				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@...01000 {
> +				compatible = "ti,davinci_mdio";
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				ti,hwmods = "davinci_mdio";
> +				bus_freq = <1000000>;
> +				reg = <0x4a101000 0x100>;
> +			};
> +

Stray change.

Regards,
Benoit

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html