[U-Boot] [U-Boot, 5/6] net: gmac_rockchip: Use the proerty of "clock_in_out" to set mac clock

[Philipp Tomsich]

On Thu, 21 Sep 2017, David Wu wrote:

> If the mac clock if from the external IO, set clock rate with 0;
> If the mac clock if from the internal divider pll, set 50M for
> rmii mode and set 125M for rgmii.

If we do this, we really shouldn't treat the external clock any different.
So we should request the speed we expect from the external clk and not 
just pass 0...

More below.

>
> Signed-off-by: David Wu <david.wu at rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> ---
>
> drivers/net/gmac_rockchip.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
> index 5f8f0cd..26f7a96 100644
> --- a/drivers/net/gmac_rockchip.c
> +++ b/drivers/net/gmac_rockchip.c
> @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;
> struct gmac_rockchip_platdata {
> 	struct dw_eth_pdata dw_eth_pdata;
> 	void *grf;
> +	int clk_in;

Please use an enum here to make it clear which value in INPUT and which 
one is OUTPUT.

> 	int tx_delay;
> 	int rx_delay;
> };
> @@ -64,6 +65,8 @@ static int gmac_rockchip_ofdata_to_platdata(struct udevice *dev)
> {
> 	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
>
> +	pdata->clk_in = dev_read_u32_default(dev, "clock_in_out", 1);
> +

The 'u32' seems to be wrong, relative to common usage:
arch/arm/dts/rk3229-evb.dts:    clock_in_out = "input";
arch/arm/dts/rk3288-evb.dtsi:   clock_in_out = "input";
arch/arm/dts/rk3288-fennec.dtsi:        clock_in_out = "input";
arch/arm/dts/rk3288-firefly.dtsi:       clock_in_out = "input";
arch/arm/dts/rk3288-miqi.dtsi:  clock_in_out = "input";
arch/arm/dts/rk3288-phycore-som.dtsi:   clock_in_out = "input";
arch/arm/dts/rk3288-popmetal.dtsi:      clock_in_out = "input";
arch/arm/dts/rk3288-rock2-som.dtsi:     clock_in_out = "input";
arch/arm/dts/rk3288-tinker.dtsi:        clock_in_out = "input";
arch/arm/dts/rk3368-geekbox.dts:        clock_in_out = "input";
arch/arm/dts/rk3368-lion.dts:   clock_in_out = "input";
arch/arm/dts/rk3368-px5-evb.dts:        clock_in_out = "input";
arch/arm/dts/rk3368-sheep.dts:  clock_in_out = "input";
arch/arm/dts/rk3399-evb.dts:    clock_in_out = "input";
arch/arm/dts/rk3399-firefly.dts:        clock_in_out = "input";
arch/arm/dts/rk3399-puma.dtsi:  clock_in_out = "input";

Please remain consistent and test for a string here and for the string 
value.  Also, we are missing documentation on the DTS binding in /doc: 
please add documentation there.

> 	/* Check the new naming-style first... */
> 	pdata->tx_delay = dev_read_u32_default(dev, "tx_delay", -ENOENT);
> 	pdata->rx_delay = dev_read_u32_default(dev, "rx_delay", -ENOENT);
> @@ -294,6 +297,8 @@ static int gmac_rockchip_probe(struct udevice *dev)
> 	struct gmac_rockchip_platdata *pdata = dev_get_platdata(dev);
> 	struct gmac_rockchip_driver_data *data =
> 		(struct gmac_rockchip_driver_data *)dev_get_driver_data(dev);
> +	struct dw_eth_pdata *dw_pdata = dev_get_platdata(dev);
> +	struct eth_pdata *eth_pdata = &dw_pdata->eth_pdata;
> 	const struct rk_gmac_ops *ops = data->ops;
> 	struct clk clk;
> 	int ret;
> @@ -302,10 +307,20 @@ static int gmac_rockchip_probe(struct udevice *dev)
> 	if (ret)
> 		return ret;
>
> -	/* Since mac_clk is fed by an external clock we can use 0 here */
> -	ret = clk_set_rate(&clk, 0);
> -	if (ret)
> -		return ret;
> +	if (pdata->clk_in) {
> +		/*
> +		 * Since mac_clk is fed by an external clock
> +		 * we can use 0 here.
> +		 */
> +		ret = clk_set_rate(&clk, 0);

In hindsight using '0' was a bad decision.
We should always request the expected external clock rate here (so the 
clock driver could do some sanity checking).

> +		if (ret)
> +			return ret;
> +	} else {
> +		if (eth_pdata->phy_interface == PHY_INTERFACE_MODE_RGMII)
> +			clk_set_rate(&clk, 125000000);
> +		else
> +			clk_set_rate(&clk, 50000000);
> +	}
>
> 	pdata->grf = syscon_get_first_range(ROCKCHIP_SYSCON_GRF) +
> 		     data->grf_offset;
>