| Message ID | 1510219632-79453-1-git-send-email-david.wu@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Philipp Tomsich |
| Headers | show |
| Series | Add gmac support for rk3399-evb rv1108-evb rk3328-evb and rk3229-evb | expand |
> Assuming mac_clk is fed by an external clock, set clk_rmii_src > clock select control register from IO for rgmii interface. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > > Changes in v2: > - New patch > > drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
On Thu, 9 Nov 2017, David Wu wrote: > Assuming mac_clk is fed by an external clock, set clk_rmii_src > clock select control register from IO for rgmii interface. > > Signed-off-by: David Wu <david.wu@rock-chips.com> > Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> See below for requested changes. > --- > > Changes in v2: > - New patch > > drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c > index e87267d..5fd27cd 100644 > --- a/drivers/clk/rockchip/clk_rk322x.c > +++ b/drivers/clk/rockchip/clk_rk322x.c > @@ -239,6 +239,16 @@ static ulong rockchip_mmc_get_clk(struct rk322x_cru *cru, uint clk_general_rate, > return DIV_TO_RATE(src_rate, div) / 2; > } > > +static int rk322x_mac_set_clk(struct rk322x_cru *cru, > + int periph, uint freq) > +{ > + /* Assuming mac_clk is fed by an external clock */ > + rk_clrsetreg(&cru->cru_clksel_con[5], BIT(5), > + BIT(5)); Please use a symbolic constant for BIT(5). If this is the input/output selection for the MAC clk and covered by 'clock_in_out = "input";' in the DTS, then the DTS should be consulted before assuming a specific setting here. > + > + return 0; > +} > + > static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate, > int periph, uint freq) > { > @@ -352,6 +362,9 @@ static ulong rk322x_clk_set_rate(struct clk *clk, ulong rate) > case CLK_DDR: > new_rate = rk322x_ddr_set_clk(priv->cru, rate); > break; > + case SCLK_MAC: > + new_rate = rk322x_mac_set_clk(priv->cru, clk->id, rate); > + break; > default: > return -ENOENT; > } >
Hi Philipp, 在 2017/11/26 22:50, Philipp Tomsich 写道: > > On Thu, 9 Nov 2017, David Wu wrote: > >> Assuming mac_clk is fed by an external clock, set clk_rmii_src >> clock select control register from IO for rgmii interface. >> >> Signed-off-by: David Wu <david.wu@rock-chips.com> >> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > > Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > > See below for requested changes. > >> --- >> >> Changes in v2: >> - New patch >> >> drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/clk/rockchip/clk_rk322x.c >> b/drivers/clk/rockchip/clk_rk322x.c >> index e87267d..5fd27cd 100644 >> --- a/drivers/clk/rockchip/clk_rk322x.c >> +++ b/drivers/clk/rockchip/clk_rk322x.c >> @@ -239,6 +239,16 @@ static ulong rockchip_mmc_get_clk(struct >> rk322x_cru *cru, uint clk_general_rate, >> return DIV_TO_RATE(src_rate, div) / 2; >> } >> >> +static int rk322x_mac_set_clk(struct rk322x_cru *cru, >> + int periph, uint freq) >> +{ >> + /* Assuming mac_clk is fed by an external clock */ >> + rk_clrsetreg(&cru->cru_clksel_con[5], BIT(5), >> + BIT(5)); > > Please use a symbolic constant for BIT(5). > > If this is the input/output selection for the MAC clk and covered by > 'clock_in_out = "input";' in the DTS, then the DTS should be consulted > before assuming a specific setting here. > Yes, I think so, but not do it now; If it is "input", then actually do not need to set clk rate, as long as the direction of setting clk, clk rate rely on external clock input may originate from PHY. If it is "output", then also need to set clk rate. There has been the interface of setting clock rate currently, but not set the direction. Can we in the GMAC driver directly set cru registers of mac_clk direction through the dts property of "clk_in_out", otherwise need clk framework to provide a set of directional interface like setting the parent of mac_clk. >> + >> + return 0; >> +} >> + >> static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint >> clk_general_rate, >> int periph, uint freq) >> { >> @@ -352,6 +362,9 @@ static ulong rk322x_clk_set_rate(struct clk *clk, >> ulong rate) >> case CLK_DDR: >> new_rate = rk322x_ddr_set_clk(priv->cru, rate); >> break; >> + case SCLK_MAC: >> + new_rate = rk322x_mac_set_clk(priv->cru, clk->id, rate); >> + break; >> default: >> return -ENOENT; >> } >> > > >
David, As discussed last week (off-list), I implemented infrastructure-support for the 'assigned-clocks', 'assigned-clock-parents' and 'assigned-clock-rates’ properties in the DTS … and validated on the RK3399. For my work-in-progress tree, see the ‘assigned-clocks-wip’ branch at: https://github.com/ptomsich/u-boot-rockchip/tree/assigned-clocks-wip If this is sufficient to solve our clocking-issue on GMAC let me know. I plan to have some additional time to finish this up for submission around the end of the week. @Jakob: This is the change I mentioned this morning that provides infrastructure to allow setting PLLs via the DTS. These changes are also on our internal GIT in the ‘assigned-clocks-wip’ branch (which I manually mirror onto GitHub, so I can test via Travis-CI). Comments are welcome, Phil. > On 4 Jan 2018, at 14:22, David.Wu <david.wu@rock-chips.com> wrote: > > Hi Philipp, > > Could i just make the clock-driver respond to the requested frequency? I.e. 50MHz will most likely always come from the internal sources > so when 50Mhz for rmii gets requested you just assume that we will use the internal source. And the 125MHz, we assume that we will use the externalsource. > > 在 2017/12/21 19:21, David.Wu 写道: >> Hi Philipp, >> 在 2017/11/26 22:50, Philipp Tomsich 写道: >>> >>> On Thu, 9 Nov 2017, David Wu wrote: >>> >>>> Assuming mac_clk is fed by an external clock, set clk_rmii_src >>>> clock select control register from IO for rgmii interface. >>>> >>>> Signed-off-by: David Wu <david.wu@rock-chips.com> >>>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >>> >>> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >>> >>> See below for requested changes. >>> >>>> --- >>>> >>>> Changes in v2: >>>> - New patch >>>> >>>> drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c >>>> index e87267d..5fd27cd 100644 >>>> --- a/drivers/clk/rockchip/clk_rk322x.c >>>> +++ b/drivers/clk/rockchip/clk_rk322x.c >>>> @@ -239,6 +239,16 @@ static ulong rockchip_mmc_get_clk(struct rk322x_cru *cru, uint clk_general_rate, >>>> return DIV_TO_RATE(src_rate, div) / 2; >>>> } >>>> >>>> +static int rk322x_mac_set_clk(struct rk322x_cru *cru, >>>> + int periph, uint freq) >>>> +{ >>>> + /* Assuming mac_clk is fed by an external clock */ >>>> + rk_clrsetreg(&cru->cru_clksel_con[5], BIT(5), >>>> + BIT(5)); >>> >>> Please use a symbolic constant for BIT(5). >>> >>> If this is the input/output selection for the MAC clk and covered by 'clock_in_out = "input";' in the DTS, then the DTS should be consulted before assuming a specific setting here. >>> >> Yes, I think so, but not do it now; >> If it is "input", then actually do not need to set clk rate, as long as the direction of setting clk, clk rate rely on external clock input may originate from PHY. >> If it is "output", then also need to set clk rate. >> There has been the interface of setting clock rate currently, but not set the direction. Can we in the GMAC driver directly set cru registers of mac_clk direction through the dts property of "clk_in_out", otherwise need clk framework to provide a set of directional interface like setting the parent of mac_clk. >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate, >>>> int periph, uint freq) >>>> { >>>> @@ -352,6 +362,9 @@ static ulong rk322x_clk_set_rate(struct clk *clk, ulong rate) >>>> case CLK_DDR: >>>> new_rate = rk322x_ddr_set_clk(priv->cru, rate); >>>> break; >>>> + case SCLK_MAC: >>>> + new_rate = rk322x_mac_set_clk(priv->cru, clk->id, rate); >>>> + break; >>>> default: >>>> return -ENOENT; >>>> } >>>> >>> >>> >>> >
Hi Philipp,
For the 'assigned-clocks' patches, i tested them well, and have
implemented them at rk3328, rk3229, rk3288 and rk3368. The patches has
been send for your review. Except for one thing, just like you said, we
need to move "clk_set_defaults" further down and do it just before:
if (drv->probe) {
ret = drv->probe(dev);
after ofdata_to_platdata called, or we might have a cru uninitialized
problem.
在 2018/1/8 23:12, Dr. Philipp Tomsich 写道:
> David,
>
> As discussed last week (off-list), I implemented infrastructure-support for the
> 'assigned-clocks', 'assigned-clock-parents' and 'assigned-clock-rates’ properties
> in the DTS … and validated on the RK3399.
>
> For my work-in-progress tree, see the ‘assigned-clocks-wip’ branch at:
> https://github.com/ptomsich/u-boot-rockchip/tree/assigned-clocks-wip
>
> If this is sufficient to solve our clocking-issue on GMAC let me know. I plan to
> have some additional time to finish this up for submission around the end of
> the week.
>
> @Jakob: This is the change I mentioned this morning that provides infrastructure
> to allow setting PLLs via the DTS. These changes are also on our internal GIT
> in the ‘assigned-clocks-wip’ branch (which I manually mirror onto GitHub, so I
> can test via Travis-CI).
>
> Comments are welcome,
> Phil.
>
>> On 4 Jan 2018, at 14:22, David.Wu <david.wu@rock-chips.com> wrote:
>>
>> Hi Philipp,
>>
>> Could i just make the clock-driver respond to the requested frequency? I.e. 50MHz will most likely always come from the internal sources
>> so when 50Mhz for rmii gets requested you just assume that we will use the internal source. And the 125MHz, we assume that we will use the externalsource.
>>
>> 在 2017/12/21 19:21, David.Wu 写道:
>>> Hi Philipp,
>>> 在 2017/11/26 22:50, Philipp Tomsich 写道:
>>>>
>>>> On Thu, 9 Nov 2017, David Wu wrote:
>>>>
>>>>> Assuming mac_clk is fed by an external clock, set clk_rmii_src
>>>>> clock select control register from IO for rgmii interface.
>>>>>
>>>>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>>>>> Acked-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>
>>>> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>>
>>>> See below for requested changes.
>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - New patch
>>>>>
>>>>> drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++
>>>>> 1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c
>>>>> index e87267d..5fd27cd 100644
>>>>> --- a/drivers/clk/rockchip/clk_rk322x.c
>>>>> +++ b/drivers/clk/rockchip/clk_rk322x.c
>>>>> @@ -239,6 +239,16 @@ static ulong rockchip_mmc_get_clk(struct rk322x_cru *cru, uint clk_general_rate,
>>>>> return DIV_TO_RATE(src_rate, div) / 2;
>>>>> }
>>>>>
>>>>> +static int rk322x_mac_set_clk(struct rk322x_cru *cru,
>>>>> + int periph, uint freq)
>>>>> +{
>>>>> + /* Assuming mac_clk is fed by an external clock */
>>>>> + rk_clrsetreg(&cru->cru_clksel_con[5], BIT(5),
>>>>> + BIT(5));
>>>>
>>>> Please use a symbolic constant for BIT(5).
>>>>
>>>> If this is the input/output selection for the MAC clk and covered by 'clock_in_out = "input";' in the DTS, then the DTS should be consulted before assuming a specific setting here.
>>>>
>>> Yes, I think so, but not do it now;
>>> If it is "input", then actually do not need to set clk rate, as long as the direction of setting clk, clk rate rely on external clock input may originate from PHY.
>>> If it is "output", then also need to set clk rate.
>>> There has been the interface of setting clock rate currently, but not set the direction. Can we in the GMAC driver directly set cru registers of mac_clk direction through the dts property of "clk_in_out", otherwise need clk framework to provide a set of directional interface like setting the parent of mac_clk.
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate,
>>>>> int periph, uint freq)
>>>>> {
>>>>> @@ -352,6 +362,9 @@ static ulong rk322x_clk_set_rate(struct clk *clk, ulong rate)
>>>>> case CLK_DDR:
>>>>> new_rate = rk322x_ddr_set_clk(priv->cru, rate);
>>>>> break;
>>>>> + case SCLK_MAC:
>>>>> + new_rate = rk322x_mac_set_clk(priv->cru, clk->id, rate);
>>>>> + break;
>>>>> default:
>>>>> return -ENOENT;
>>>>> }
>>>>>
>>>>
>>>>
>>>>
>>
>
>
>
>
diff --git a/drivers/clk/rockchip/clk_rk322x.c b/drivers/clk/rockchip/clk_rk322x.c index e87267d..5fd27cd 100644 --- a/drivers/clk/rockchip/clk_rk322x.c +++ b/drivers/clk/rockchip/clk_rk322x.c @@ -239,6 +239,16 @@ static ulong rockchip_mmc_get_clk(struct rk322x_cru *cru, uint clk_general_rate, return DIV_TO_RATE(src_rate, div) / 2; } +static int rk322x_mac_set_clk(struct rk322x_cru *cru, + int periph, uint freq) +{ + /* Assuming mac_clk is fed by an external clock */ + rk_clrsetreg(&cru->cru_clksel_con[5], BIT(5), + BIT(5)); + + return 0; +} + static ulong rockchip_mmc_set_clk(struct rk322x_cru *cru, uint clk_general_rate, int periph, uint freq) { @@ -352,6 +362,9 @@ static ulong rk322x_clk_set_rate(struct clk *clk, ulong rate) case CLK_DDR: new_rate = rk322x_ddr_set_clk(priv->cru, rate); break; + case SCLK_MAC: + new_rate = rk322x_mac_set_clk(priv->cru, clk->id, rate); + break; default: return -ENOENT; }
Assuming mac_clk is fed by an external clock, set clk_rmii_src clock select control register from IO for rgmii interface. Signed-off-by: David Wu <david.wu@rock-chips.com> --- Changes in v2: - New patch drivers/clk/rockchip/clk_rk322x.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)