[OpenWrt-Devel] [PATCH v2 2/3] ramips: Get ethernet ports to be disabled from the device tree.

[John Crispin]

Hi,

comments inline

On 12/12/2015 02:10, Vittorio G (VittGam) wrote:
> Line 461 is actually enabling all switch ports, so line 508 is always getting
> zero ports to be disabled (except for port 5 in SoCs where this is not implemented,
> as it will be sticky disabled in register POC0).
> 
> Also, the bootloader on some routers sets all ports to disabled (which is
> the case for at least one router based on RT5350).
> 
> So, this patch allows configuring ports to be disabled in the device tree.
> 
> Power can be saved too this way, since disabling ports here actually disables
> power to ethernet PHYs.
> 
> Signed-off-by: Vittorio Gambaletta <openwrt at vittgam.net>
> 
> ---
> 
> Please apply to both trunk and CC.
> 
> --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c
> +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c
> @@ -233,6 +233,7 @@ struct rt305x_esw {
>  	spinlock_t		reg_rw_lock;
>  
>  	unsigned char		port_map;
> +	unsigned char		port_disable;
>  	unsigned int		reg_initval_fct2;
>  	unsigned int		reg_initval_fpa2;
>  	unsigned int		reg_led_polarity;
> @@ -457,7 +458,7 @@ static void esw_hw_init(struct rt305x_esw *esw)
>  		      (RT305X_ESW_PORTS_ALL << RT305X_ESW_PFC1_EN_VLAN_S),
>  		      RT305X_ESW_REG_PFC1);
>  
> -	/* Enable Back Pressure, and Flow Control */
> +	/* Enable all ports; enable Back Pressure and Flow Control */

why a semicolon ? i believe a "," needs to be added instead please also
remove the superfluous "," while you are at it to make the comment
orthographically correct. it seems to be have been copied from SDK code.

>  	esw_w32(esw,
>  		      ((RT305X_ESW_PORTS_ALL << RT305X_ESW_POC0_EN_BP_S) |
>  		       (RT305X_ESW_PORTS_ALL << RT305X_ESW_POC0_EN_FC_S)),
> @@ -504,8 +505,9 @@ static void esw_hw_init(struct rt305x_esw *esw)
>  	esw_w32(esw, 0x00000005, RT305X_ESW_REG_P3LED);
>  	esw_w32(esw, 0x00000005, RT305X_ESW_REG_P4LED);
>  
> -	/* Copy disabled port configuration from bootloader setup */
> -	port_disable = esw_get_port_disable(esw);
> +	/* Copy disabled port configuration from device tree setup */
> +	if (esw->port_disable)
> +		port_disable = esw->port_disable;

this changes the behavior of the code and will most likely break several
board.

>  	for (i = 0; i < 6; i++)
>  		esw->ports[i].disable = (port_disable & (1 << i)) != 0;
>  
> @@ -1419,6 +1421,10 @@ static int esw_probe(struct platform_device *pdev)
>  	port_map = of_get_property(np, "ralink,portmap", NULL);
>  	if (port_map)
>  		esw->port_map = be32_to_cpu(*port_map);
> +
> +	port_map = of_get_property(np, "ralink,portdisable", NULL);
> +	if (port_map)
> +		esw->port_disable = be32_to_cpu(*port_map);
>  

port_map is named as such as it is a port_map and not a port_disable. we
already concluded yesterday that this is a bad code style.

	John

>  	reg_init = of_get_property(np, "ralink,fct2", NULL);
>  	if (reg_init)
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel