On 14.11.20 23:57, Marek Behún wrote:
> On Sat, 14 Nov 2020 22:36:09 +0100
> Andreas Färber <afaerber@xxxxxxx> wrote:
>
>> Hi Marek,
>>
>> On 14.11.20 19:32, Marek Behún wrote:
>>> Turris Omnia has a SFP cage that, together with WAN PHY is connected to
>>
>> "an SFP"
>> Comma missing after PHY (or drop before together).
>>
>>> eth2 SerDes via a SerDes multiplexor. Describe the SFP cage, but leave
>>> it disabled. Until phylink has support for such multiplexor we will
>>> leave it to U-Boot to enable SFP and disable WAN PHY at boot time
>>> depending on whether a SFP module is present.
>>
>> multiplexor vs. multiplexer may be a British thing? Thunderbird
>> underlines it fwiw.
>>
>>>
>>> Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>
>>> Fixes: 26ca8b52d6e1 ("ARM: dts: add support for Turris Omnia")
>>> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>>> Cc: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx>
>>> Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
>>> Cc: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx>
>>> Cc: Andreas Färber <afaerber@xxxxxxx>
>>> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>>> Cc: devicetree@xxxxxxxxxxxxxxx
>>> ---
>>> arch/arm/boot/dts/armada-385-turris-omnia.dts | 19 ++++++++++++++++++-
>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/armada-385-turris-omnia.dts b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> index 7ccebf7d1757..14c21cddef72 100644
>>> --- a/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> +++ b/arch/arm/boot/dts/armada-385-turris-omnia.dts
>>> @@ -82,6 +82,22 @@ pcie@3,0 {
>>> };
>>> };
>>> };
>>> +
>>> + sfp: sfp {
>>> + compatible = "sff,sfp";
>>> + i2c-bus = <&sfp_i2c>;
>>> + tx-fault-gpios = <&pcawan 0 GPIO_ACTIVE_HIGH>;
>>> + tx-disable-gpios = <&pcawan 1 GPIO_ACTIVE_HIGH>;
>>> + rate-select0-gpios = <&pcawan 2 GPIO_ACTIVE_HIGH>;
>>> + los-gpios = <&pcawan 3 GPIO_ACTIVE_HIGH>;
>>> + mod-def0-gpios = <&pcawan 4 GPIO_ACTIVE_LOW>;
>>> +
>>> + /*
>>> + * For now this has to be enabled at boot time by U-Boot when
>>> + * a SFP module is present.
>>> + */
>>> + status = "disabled";
>>> + };
>>> };
>>>
>>> &bm {
>>> @@ -130,6 +146,7 @@ ð2 {
>>> phy-mode = "sgmii";
>>> phy = <&phy1>;
>>> phys = <&comphy5 2>;
>>> + sfp = <&sfp>;
>>> buffer-manager = <&bm>;
>>> bm,pool-long = <2>;
>>> bm,pool-short = <3>;
>>> @@ -195,7 +212,7 @@ i2c@3 {
>>> /* routed to PCIe2 connector (CN62A) */
>>> };
>>>
>>> - i2c@4 {
>>> + sfp_i2c: i2c@4 {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> reg = <4>;
>>
>> Matches what I've come up with,
>>
>> Reviewed-by: Andreas Färber <afaerber@xxxxxxx>
>>
>> However, I also needed to set managed = "in-band-status" when enabling
>> SFP node and removing phy property. Shall we prepare it with its default
>> value of "auto" and add a comment? (unlike disabled -> okay,
>> in-band-status is longer than auto, so not sure whether it helps U-Boot,
>> but it may help humans.
>
> The idea is that for now when U-Boot detects that SFP is present, it
> shall change the device tree accordingly:
> remove phy-handle
> add managed = in-band-status
> enable sfp node
>
> This is the only way to support this reasobaly until the multiplexer is
> somehow supported by kernel.
I do understand the idea. My point was that you added a 4-line comment
about status property further above, but no comments about phy-handle
nor managed properties down here.
It might also be a good idea to explain in a comment why they are
mutually exclusive (mod-def0, multiplexer).
Have you done any debugging as to why we can't just leave the sfp node
enabled? Does it toggle mod-def0-gpios on probe even if no SFP is
physically present on i2c? Maybe it can be simplified over in sfp code?
Regards,
Andreas
--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)
