[PATCH v7 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver

[Loc Ho]

Hi,

> [...]
>
>> + * The APM X-Gene PHY consists of two PLL clock macro's (CMU) and lanes.
>> + * The first PLL clock macro is used for internal reference clock. The second
>> + * PLL clock macro is used to generate the clock for the PHY. This driver
>> + * configures the first PLL CMU, the second PLL CMU, and programs the PHY to
>> + * operate according to the mode of operation. The first PLL CMU is only
>> + * required if internal clock is enabled.
>> + *
>> + * Logical Layer Out Of HW module units:
>> + *
>> + * -----------------
>> + * | Internal      |    |------|
>> + * | Ref PLL CMU   |----|      |     -------------    ---------
>> + * ------------ ----    | MUX  |-----|PHY PLL CMU|----| Serdes|
>> + *                      |      |     |           |    ---------
>> + * External Clock ------|      |     -------------
>> + *                      |------|
>> + *
>> + * The Ref PLL CMU CSR (Configureation System Registers) is accessed
>> + * indirectly from the SDS offset at 0x2000. It is only required for
>> + * internal reference clock.
>> + * The PHY PLL CMU CSR is accessed indirectly from the SDS offset at 0x0000.
>> + * The Serdes CSR is accessed indirectly from the SDS offset at 0x0400.
>> + *
>> + * The Ref PLL CMU can be located within the same PHY IP or outside the PHY IP
>> + * due to shared Ref PLL CMU. For PHY with Ref PLL CMU shared with another IP,
>> + * it is located outside the PHY IP. This is the case for the PHY located
>> + * at 0x1f23a000 (SATA Port 4/5). For such PHY, another resource is required
>> + * to located the SDS/Ref PLL CMU module and its clock for that IP enabled.
>> + *
>> + * Currently, this driver only supports SATA mode with external clock.
>> + */
>
> Having looked at this and the binding, I'm confused as to why we need an
> additional reg entry when we're using the external clock.
>
> Surely the second resource (the external CMU base) is a property of the
> shared ref PLL clock, rather than the PHY itself. How do you handle two
> units trying to use the shared PLL?

You have an good point here. For the time being, let me rip out the
external CMU support as we don't support internal clock just yet. It
will be deal with later on.

>
> I think makes sense to model the ref PLL CMU as a separate clock, and
> use clock-names to differentiate between the two inputs to the mux:
>
> external: external_clock {
>         compatible = "fixed-clock";
>         clock-frequency = < ... >;
>         #clock-cells = <0>;
> };
>
> ref_pll: reference_clock {
>         compatible = "apm,xgene-ref-pll";
>         reg = < .... >;
>         #clock-cells = <0>;
> };
>
> phy {
>         compatible = "apm,xgene-phy";
>         reg = < ... >;
>         clocks = <&ref_pll>, <&external>;
>         clock-names = "ref-pll", "external";
>         ...
> };
>
>
> That also means the phy only needs a single compatible string -- you can
> figure out what to do by probing the set of clocks.
>
> Does that make sense? Is there something I'm missing?

Okay... As mentioned, let me rip the ref CMU out and address this at a
later time - going with your suggested approach in the future.

-Loc