On 9/12/2023 4:11 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-08 04:10:35)
>>
>>
>> Yes, the uniphy implements the clock provider that supports changing
>> rate, which will be upstream later, and nss_cc_mac5_rx_clk_src is the
>> special case, which is only used in the switch device qca8386.
>
> Ok great.
>
>>
>> For the phy device qca8084(uniphy has only 312.5M fix clock which is
>> registered by device tree), this clock nss_cc_mac5_rx_clk_src is not used.
>>
>> The issue for the switch device(qca8386) here is the clock rate of
>> parent uniphy can't be changed because of the clock rate requirement of
>> branch clock, since the uniphy clock rate is decided by the current
>> working interface mode(PHY_INTERFACE_MODE_2500BASEX with 312.5M or
>> PHY_INTERFACE_MODE_SGMII with 125M).
>
> Got it.
>
>>
>> For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
>> the parent uniphy clock rate is 312.5M, which is decided by hardware and
>> can't be changed. when a branch clock requires a 25M clock, the parent
>> uniphy clock maybe updated to 125M by clock framework if the flag
>> CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
>> uniphy is still 315.5M since the uniphy still works in the interface
>> mode PHY_INTERFACE_MODE_2500BASEX.
>>
>
> If the parent rate can't change because CLK_SET_RATE_PARENT is missing
> and the hardware doesn't allow it, then perhaps instead of having a
> frequency table we should have rcg clk ops for determine_rate that
> simply looks at the parent rates and finds the rate closest to what is
> desired. And for the set_rate clk_op we can have it be simple and just
> program a fixed divider. The benefit is less frequency tables that don't
> do anything and less hard-coding of the frequency. I thought we already
> had those rcg clk_ops but I couldn't find them with a quick glance.
Thanks Stephen for the suggestion.
looks you are saying the clk ops clk_dp_ops for the fix parent rate?
which seems not meet the clock requirement of this clock.
For the device qca8k, it is also possible to switch the interface modes
between PHY_INTERFACE_MODE_2500BASEX(312.5M) and
PHY_INTERFACE_MODE_SGMII(125M) during the running time, and there are
multiple parent clock source(P_UNIPHY0_RX or P_UNIPHY0_TX) for the RCG
clocks to select according to the current work mode. so the parent_map
and freq_tbl are necessary to this clock.
such as the following clock table, same parent clock rate has the
different parent source.
+static const struct parent_map nss_cc_uniphy0_rx_tx_map[] = {
+ { P_XO, 0 },
+ { P_UNIPHY0_RX, 1 },
+ { P_UNIPHY0_TX, 2 },
+};
+
+static const struct freq_tbl ftbl_nss_cc_mac5_rx_clk_src[] = {
+ F(50000000, P_XO, 1, 0, 0),
+ F(125000000, P_UNIPHY0_RX, 1, 0, 0),
+ F(125000000, P_UNIPHY0_TX, 1, 0, 0),
+ F(312500000, P_UNIPHY0_RX, 1, 0, 0),
+ F(312500000, P_UNIPHY0_TX, 1, 0, 0),
+ { }
+};
Quoting Jie Luo (2023-09-12 05:07:02)
>
>
> On 9/12/2023 4:11 AM, Stephen Boyd wrote:
> > Quoting Jie Luo (2023-09-08 04:10:35)
> >>
> >> For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
> >> the parent uniphy clock rate is 312.5M, which is decided by hardware and
> >> can't be changed. when a branch clock requires a 25M clock, the parent
> >> uniphy clock maybe updated to 125M by clock framework if the flag
> >> CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
> >> uniphy is still 315.5M since the uniphy still works in the interface
> >> mode PHY_INTERFACE_MODE_2500BASEX.
> >>
> >
> > If the parent rate can't change because CLK_SET_RATE_PARENT is missing
> > and the hardware doesn't allow it, then perhaps instead of having a
> > frequency table we should have rcg clk ops for determine_rate that
> > simply looks at the parent rates and finds the rate closest to what is
> > desired. And for the set_rate clk_op we can have it be simple and just
> > program a fixed divider. The benefit is less frequency tables that don't
> > do anything and less hard-coding of the frequency. I thought we already
> > had those rcg clk_ops but I couldn't find them with a quick glance.
>
> Thanks Stephen for the suggestion.
> looks you are saying the clk ops clk_dp_ops for the fix parent rate?
> which seems not meet the clock requirement of this clock.
Yeah that is close, but the determine_rate clk_op needs to look at all
possible parents. With the dp clk_ops we assume that only one parent is
possible.
>
> For the device qca8k, it is also possible to switch the interface modes
> between PHY_INTERFACE_MODE_2500BASEX(312.5M) and
> PHY_INTERFACE_MODE_SGMII(125M) during the running time, and there are
> multiple parent clock source(P_UNIPHY0_RX or P_UNIPHY0_TX) for the RCG
> clocks to select according to the current work mode. so the parent_map
> and freq_tbl are necessary to this clock.
I still don't see why the freq_tbl is necessary.
On 9/13/2023 1:18 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-12 05:07:02)
>>
>>
>> On 9/12/2023 4:11 AM, Stephen Boyd wrote:
>>> Quoting Jie Luo (2023-09-08 04:10:35)
>>>>
>>>> For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
>>>> the parent uniphy clock rate is 312.5M, which is decided by hardware and
>>>> can't be changed. when a branch clock requires a 25M clock, the parent
>>>> uniphy clock maybe updated to 125M by clock framework if the flag
>>>> CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
>>>> uniphy is still 315.5M since the uniphy still works in the interface
>>>> mode PHY_INTERFACE_MODE_2500BASEX.
>>>>
>>>
>>> If the parent rate can't change because CLK_SET_RATE_PARENT is missing
>>> and the hardware doesn't allow it, then perhaps instead of having a
>>> frequency table we should have rcg clk ops for determine_rate that
>>> simply looks at the parent rates and finds the rate closest to what is
>>> desired. And for the set_rate clk_op we can have it be simple and just
>>> program a fixed divider. The benefit is less frequency tables that don't
>>> do anything and less hard-coding of the frequency. I thought we already
>>> had those rcg clk_ops but I couldn't find them with a quick glance.
>>
>> Thanks Stephen for the suggestion.
>> looks you are saying the clk ops clk_dp_ops for the fix parent rate?
>> which seems not meet the clock requirement of this clock.
>
> Yeah that is close, but the determine_rate clk_op needs to look at all
> possible parents. With the dp clk_ops we assume that only one parent is
> possible.
>
>>
>> For the device qca8k, it is also possible to switch the interface modes
>> between PHY_INTERFACE_MODE_2500BASEX(312.5M) and
>> PHY_INTERFACE_MODE_SGMII(125M) during the running time, and there are
>> multiple parent clock source(P_UNIPHY0_RX or P_UNIPHY0_TX) for the RCG
>> clocks to select according to the current work mode. so the parent_map
>> and freq_tbl are necessary to this clock.
>
> I still don't see why the freq_tbl is necessary.
Hi Stephen,
For clk_rcg2_ops, freq_tbl is used to find the closest rate to decided
the parent clock, the configuration of clock source and clock divider
are saved in the freq_tbl to configure the RCG hardware register, the
mapping of parent clock and hardware register value is decided by the
freq_tbl for the RCG clock.
Quoting Jie Luo (2023-09-12 20:27:25)
>
>
> On 9/13/2023 1:18 AM, Stephen Boyd wrote:
> > Quoting Jie Luo (2023-09-12 05:07:02)
> >>
> >> and freq_tbl are necessary to this clock.
> >
> > I still don't see why the freq_tbl is necessary.
>
> Hi Stephen,
> For clk_rcg2_ops, freq_tbl is used to find the closest rate to decided
> the parent clock, the configuration of clock source and clock divider
> are saved in the freq_tbl to configure the RCG hardware register, the
> mapping of parent clock and hardware register value is decided by the
> freq_tbl for the RCG clock.
The divider is always 1. The frequency is the frequency of the parent.
The two pieces of information are already known without the frequency
table. Why is it needed?
On 9/15/2023 12:30 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-12 20:27:25)
>>
>>
>> On 9/13/2023 1:18 AM, Stephen Boyd wrote:
>>> Quoting Jie Luo (2023-09-12 05:07:02)
>>>>
>>>> and freq_tbl are necessary to this clock.
>>>
>>> I still don't see why the freq_tbl is necessary.
>>
>> Hi Stephen,
>> For clk_rcg2_ops, freq_tbl is used to find the closest rate to decided
>> the parent clock, the configuration of clock source and clock divider
>> are saved in the freq_tbl to configure the RCG hardware register, the
>> mapping of parent clock and hardware register value is decided by the
>> freq_tbl for the RCG clock.
>
> The divider is always 1. The frequency is the frequency of the parent.
> The two pieces of information are already known without the frequency
> table. Why is it needed?
Hi Stephen,
For mac0 and mac5 RCG clock, it is true with divider 1, since these two
MACs are connected with CPU port, which is always the fix link speed,
the clock rate is always 312.5M or 125M, in this case with multiple
parent clocks and divider 1, it seems there is no special RCG clock ops
for it currently, so we leverage the clock ops clk_rcg2_ops.
For other MACs(1-4), which are connected with physical port, the link
speed is dynamically changed, and the divider is different for the
different link speed, such as the mac1 clock freq table as below.
static const struct freq_tbl ftbl_nss_cc_mac1_tx_clk_src[] = {
F(25000000, P_UNIPHY1_TX312P5M, 12.5, 0, 0),
F(25000000, P_UNIPHY1_RX312P5M, 12.5, 0, 0),
F(50000000, P_XO, 1, 0, 0),
F(125000000, P_UNIPHY1_TX312P5M, 2.5, 0, 0),
F(125000000, P_UNIPHY1_RX312P5M, 2.5, 0, 0),
F(312500000, P_UNIPHY1_TX312P5M, 1, 0, 0),
F(312500000, P_UNIPHY1_RX312P5M, 1, 0, 0),
{ }
};
Thanks,
Jie.
On 9/15/2023 12:30 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-12 20:27:25)
>>
>>
>> On 9/13/2023 1:18 AM, Stephen Boyd wrote:
>>> Quoting Jie Luo (2023-09-12 05:07:02)
>>>>
>>>> and freq_tbl are necessary to this clock.
>>>
>>> I still don't see why the freq_tbl is necessary.
>>
>> Hi Stephen,
>> For clk_rcg2_ops, freq_tbl is used to find the closest rate to decided
>> the parent clock, the configuration of clock source and clock divider
>> are saved in the freq_tbl to configure the RCG hardware register, the
>> mapping of parent clock and hardware register value is decided by the
>> freq_tbl for the RCG clock.
>
> The divider is always 1. The frequency is the frequency of the parent.
> The two pieces of information are already known without the frequency
> table. Why is it needed?
Hi Stephen,
i uploaded the new patchset V9 to remove these redundant freq_tbl by
using the clk_ops clk_ops clk_rcg2_mux_closest_ops, thanks for this
suggestion for the code improvement.
Best Regards,
Jie