Hi Marek,

On 4/16/21 5:31 PM, Marek Vasut wrote:

On 4/16/21 5:23 PM, Alexandre TORGUE wrote:

Hello Alexandre,

quoted

On 4/16/21 3:47 PM, Marek Vasut wrote:

quoted

On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:

quoted

Hi Marek

Hello Gabriel,

quoted

On 4/14/21 4:04 PM, Marek Vasut wrote:

quoted

On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:

quoted

Hi Marek,

Hello Gabriel,

quoted

Thanks for the patchset

On 4/8/21 8:57 PM, Marek Vasut wrote:

quoted

The ETHCK_K are modeled as composite clock of MUX and GATE, 
however per
STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
Peripheral
clock distribution for Ethernet, ETHPTPDIV divider is attached 
past the
ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
gate.
Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP 
clock are
in use, ETHCKEN gate can be turned off. Current driver does not 
permit
that, fix it.

I don"t understand, it's already the case.

ETHCK_K it's a composite with a MUX and a GATE.

But ETHCK_K is _not_ a composite clock, look at the Figure 83 in 
the datasheet again and schematic below.

quoted

ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
(no gate)

But ETHPTP_K shouldn't control any mux, it is only a divider.

quoted

If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.

Look, this is what you have today:

            .------------ ETHCK_K -----------.
            |_______               _______   |
pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
            |               |
            |               '--(ETHCKSELR[7:4] divider)--[x] 
clk_ptp_ref
            |                                          |
            '------------ ETHPTP_K --------------------'

And this is what you should have, to avoid having two composite 
clock which control the same mux using the same register bit, i.e. 
what this patch implements:

            .- ck_ker_eth -.  .--- ETHCK_K --.
            |_______       |  |    _______   |
pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
                            |
                            '--(ETHCKSELR[7:4] divider)--[x] 
clk_ptp_ref
                             |                         |
                             '---- ETHPTP_K -----------'

These 2 solutions are valid. I made the choice to implement the 
first one to be able to change parent with the kernel clock of the 
IP (no need to add an intermediate binding).

Which IP are you talking about in here ?

quoted

It's the same principle for all kernel of this soc.

The first option is wrong, because in that model, you have two 
composite clock which control the same one mux bit in the same 
register. Basically you register two distinct clock which operate the 
same hardware knob.

quoted

I can ask to Alexandre to comeback of this principle, but i 'm not 
favorable.

The only discussing thing is how the clock is shown. I mean either two 
composites or one mux plus two gates. Gabriel made a choice to 
abstract the mux in two composite clocks. But it seems that at the end 
we have the same behaviour, isn't ?

Not really. Since the two composite clock control the same mux bit, 
consider what would happen if you were to select pll4_p_ck as parent for 
one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
ETHPTP_K), what would be the result ? I guess the result would depend on 
when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
don't think that's how it should work. With a single mux controlling 
that one single bit, such situation wouldn't happen.

The reparenting is managed. This mux has specific ops.
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll4_p
pll4_p
root@stm32mp1-disco-oss:~# echo pll3_q > 
/sys/kernel/debug/clk/ethptp_k/clk_set_parent
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll3_q
pll3_q

quoted

Adding "ck_ker_eth" would impose a new clock to take in DT ?

Nope, the ck_ker_eth is without ID and internal to the driver. They 
exist only to describe the clock tree correctly.

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel