diff mbox series

[v9,07/10] arm64: dts: ls1046ardb: Add serdes bindings

Message ID 20221230000139.2846763-8-sean.anderson@seco.com
State New
Headers show
Series phy: Add support for Lynx 10G SerDes | expand

Commit Message

Sean Anderson Dec. 30, 2022, 12:01 a.m. UTC
This adds appropriate bindings for the macs which use the SerDes. The
156.25MHz fixed clock is a crystal. The 100MHz clocks (there are
actually 3) come from a Renesas 6V49205B at address 69 on i2c0. There is
no driver for this device (and as far as I know all you can do with the
100MHz clocks is gate them), so I have chosen to model it as a single
fixed clock.

Note: the SerDes1 lane numbering for the LS1046A is *reversed*.
This means that Lane A (what the driver thinks is lane 0) uses pins
SD1_TX3_P/N.

Because this will break ethernet if the serdes is not enabled, enable
the serdes driver by default on Layerscape.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
This depends on [1].

[1] https://lore.kernel.org/netdev/20220804194705.459670-4-sean.anderson@seco.com/

Changes in v9:
- Fix name of phy mode node
- phy-type -> fsl,phy

Changes in v8:
- Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc.
  This should help remind readers that the numbering corresponds to the
  physical layout of the registers, and not the lane (pin) number.

Changes in v6:
- XGI.9 -> XFI.9

Changes in v4:
- Convert to new bindings

 .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
 drivers/phy/freescale/Kconfig                 |   1 +
 2 files changed, 113 insertions(+)

Comments

Shawn Guo Jan. 25, 2023, 11:43 p.m. UTC | #1
On Thu, Dec 29, 2022 at 07:01:36PM -0500, Sean Anderson wrote:
> This adds appropriate bindings for the macs which use the SerDes. The
> 156.25MHz fixed clock is a crystal. The 100MHz clocks (there are
> actually 3) come from a Renesas 6V49205B at address 69 on i2c0. There is
> no driver for this device (and as far as I know all you can do with the
> 100MHz clocks is gate them), so I have chosen to model it as a single
> fixed clock.
> 
> Note: the SerDes1 lane numbering for the LS1046A is *reversed*.
> This means that Lane A (what the driver thinks is lane 0) uses pins
> SD1_TX3_P/N.
> 
> Because this will break ethernet if the serdes is not enabled, enable
> the serdes driver by default on Layerscape.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> This depends on [1].
> 
> [1] https://lore.kernel.org/netdev/20220804194705.459670-4-sean.anderson@seco.com/
> 
> Changes in v9:
> - Fix name of phy mode node
> - phy-type -> fsl,phy
> 
> Changes in v8:
> - Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc.
>   This should help remind readers that the numbering corresponds to the
>   physical layout of the registers, and not the lane (pin) number.
> 
> Changes in v6:
> - XGI.9 -> XFI.9
> 
> Changes in v4:
> - Convert to new bindings
> 
>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>  drivers/phy/freescale/Kconfig                 |   1 +

The phy driver Kconfig change shouldn't be part of this patch.

>  2 files changed, 113 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> index 7025aad8ae89..534f19855b47 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> @@ -10,6 +10,8 @@
>  
>  /dts-v1/;
>  
> +#include <dt-bindings/phy/phy.h>
> +
>  #include "fsl-ls1046a.dtsi"
>  
>  / {
> @@ -26,8 +28,110 @@ aliases {
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> +
> +	clocks {

Drop this container node.

Shawn

> +		clk_100mhz: clock-100mhz {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <100000000>;
> +		};
> +
> +		clk_156mhz: clock-156mhz {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <156250000>;
> +		};
> +	};
>  };
>  
> +&serdes1 {
> +	clocks = <&clk_100mhz>, <&clk_156mhz>;
> +	clock-names = "ref0", "ref1";
> +	status = "okay";
> +
> +	/*
> +	 * XXX: Lane A uses pins SD1_RX3_P/N! That is, the lane numbers and pin
> +	 * numbers are _reversed_. In addition, the PCCR documentation is
> +	 * _inconsistent_ in its usage of these terms!
> +	 *
> +	 * PCCR "Lane 0" refers to...
> +	 * ==== =====================
> +	 *    0 Lane A
> +	 *    2 Lane A
> +	 *    8 Lane A
> +	 *    9 Lane A
> +	 *    B Lane D!
> +	 */
> +	serdes1_A: phy@0 {
> +		#phy-cells = <0>;
> +		reg = <0>;
> +
> +		/* SGMII.6 */
> +		sgmii-0 {
> +			fsl,pccr = <0x8>;
> +			fsl,index = <0>;
> +			fsl,cfg = <0x1>;
> +			fsl,type = <PHY_TYPE_SGMII>;
> +		};
> +	};
> +
> +	serdes1_B: phy@1 {
> +		#phy-cells = <0>;
> +		reg = <1>;
> +
> +		/* SGMII.5 */
> +		sgmii-1 {
> +			fsl,pccr = <0x8>;
> +			fsl,index = <1>;
> +			fsl,cfg = <0x1>;
> +			fsl,type = <PHY_TYPE_2500BASEX>;
> +		};
> +	};
> +
> +	serdes1_C: phy@2 {
> +		#phy-cells = <0>;
> +		reg = <2>;
> +
> +		/* SGMII.10 */
> +		sgmii-2 {
> +			fsl,pccr = <0x8>;
> +			fsl,index = <2>;
> +			fsl,cfg = <0x1>;
> +			fsl,type = <PHY_TYPE_2500BASEX>;
> +		};
> +
> +		/* XFI.10 */
> +		xfi-0 {
> +			fsl,pccr = <0xb>;
> +			fsl,index = <0>;
> +			fsl,cfg = <0x2>;
> +			fsl,type = <PHY_TYPE_10GBASER>;
> +		};
> +	};
> +
> +	serdes1_D: phy@3 {
> +		#phy-cells = <0>;
> +		reg = <3>;
> +
> +		/* SGMII.9 */
> +		sgmii-3 {
> +			fsl,pccr = <0x8>;
> +			fsl,index = <3>;
> +			fsl,cfg = <0x1>;
> +			fsl,type = <PHY_TYPE_2500BASEX>;
> +		};
> +
> +		/* XFI.9 */
> +		xfi-1 {
> +			fsl,pccr = <0xb>;
> +			fsl,index = <1>;
> +			fsl,cfg = <0x1>;
> +			fsl,type = <PHY_TYPE_10GBASER>;
> +		};
> +	};
> +};
> +
> +
>  &duart0 {
>  	status = "okay";
>  };
> @@ -140,21 +244,29 @@ ethernet@e6000 {
>  	ethernet@e8000 {
>  		phy-handle = <&sgmii_phy1>;
>  		phy-connection-type = "sgmii";
> +		phys = <&serdes1_B>;
> +		phy-names = "serdes";
>  	};
>  
>  	ethernet@ea000 {
>  		phy-handle = <&sgmii_phy2>;
>  		phy-connection-type = "sgmii";
> +		phys = <&serdes1_A>;
> +		phy-names = "serdes";
>  	};
>  
>  	ethernet@f0000 { /* 10GEC1 */
>  		phy-handle = <&aqr106_phy>;
>  		phy-connection-type = "xgmii";
> +		phys = <&serdes1_D>;
> +		phy-names = "serdes";
>  	};
>  
>  	ethernet@f2000 { /* 10GEC2 */
>  		fixed-link = <0 1 1000 0 0>;
>  		phy-connection-type = "xgmii";
> +		phys = <&serdes1_C>;
> +		phy-names = "serdes";
>  	};
>  
>  	mdio@fc000 {
> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
> index 6bebe00f5889..b396162dc859 100644
> --- a/drivers/phy/freescale/Kconfig
> +++ b/drivers/phy/freescale/Kconfig
> @@ -54,6 +54,7 @@ config PHY_FSL_LYNX_10G
>  	depends on ARCH_LAYERSCAPE || PPC || COMPILE_TEST
>  	select GENERIC_PHY
>  	select REGMAP_MMIO
> +	default y if ARCH_LAYERSCAPE
>  	help
>  	  This adds support for the Lynx "SerDes" devices found on various QorIQ
>  	  SoCs. There may be up to four SerDes devices on each SoC, and each
> -- 
> 2.35.1.1320.gc452695387.dirty
>
Sean Anderson Jan. 26, 2023, 4:48 p.m. UTC | #2
On 1/25/23 18:43, Shawn Guo wrote:
> On Thu, Dec 29, 2022 at 07:01:36PM -0500, Sean Anderson wrote:
>> This adds appropriate bindings for the macs which use the SerDes. The
>> 156.25MHz fixed clock is a crystal. The 100MHz clocks (there are
>> actually 3) come from a Renesas 6V49205B at address 69 on i2c0. There is
>> no driver for this device (and as far as I know all you can do with the
>> 100MHz clocks is gate them), so I have chosen to model it as a single
>> fixed clock.
>> 
>> Note: the SerDes1 lane numbering for the LS1046A is *reversed*.
>> This means that Lane A (what the driver thinks is lane 0) uses pins
>> SD1_TX3_P/N.
>> 
>> Because this will break ethernet if the serdes is not enabled, enable
>> the serdes driver by default on Layerscape.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> This depends on [1].
>> 
>> [1] https://lore.kernel.org/netdev/20220804194705.459670-4-sean.anderson@seco.com/
>> 
>> Changes in v9:
>> - Fix name of phy mode node
>> - phy-type -> fsl,phy
>> 
>> Changes in v8:
>> - Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc.
>>   This should help remind readers that the numbering corresponds to the
>>   physical layout of the registers, and not the lane (pin) number.
>> 
>> Changes in v6:
>> - XGI.9 -> XFI.9
>> 
>> Changes in v4:
>> - Convert to new bindings
>> 
>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>  drivers/phy/freescale/Kconfig                 |   1 +
> 
> The phy driver Kconfig change shouldn't be part of this patch.

I put it here for bisectability, since this is the point where we need
to enable it. But I can do this in a separate patch if you want.

>>  2 files changed, 113 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> index 7025aad8ae89..534f19855b47 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> @@ -10,6 +10,8 @@
>>  
>>  /dts-v1/;
>>  
>> +#include <dt-bindings/phy/phy.h>
>> +
>>  #include "fsl-ls1046a.dtsi"
>>  
>>  / {
>> @@ -26,8 +28,110 @@ aliases {
>>  	chosen {
>>  		stdout-path = "serial0:115200n8";
>>  	};
>> +
>> +	clocks {
> 
> Drop this container node.

OK

--Sean
 
>> +		clk_100mhz: clock-100mhz {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-frequency = <100000000>;
>> +		};
>> +
>> +		clk_156mhz: clock-156mhz {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>;
>> +			clock-frequency = <156250000>;
>> +		};
>> +	};
>>  };
>>  
>> +&serdes1 {
>> +	clocks = <&clk_100mhz>, <&clk_156mhz>;
>> +	clock-names = "ref0", "ref1";
>> +	status = "okay";
>> +
>> +	/*
>> +	 * XXX: Lane A uses pins SD1_RX3_P/N! That is, the lane numbers and pin
>> +	 * numbers are _reversed_. In addition, the PCCR documentation is
>> +	 * _inconsistent_ in its usage of these terms!
>> +	 *
>> +	 * PCCR "Lane 0" refers to...
>> +	 * ==== =====================
>> +	 *    0 Lane A
>> +	 *    2 Lane A
>> +	 *    8 Lane A
>> +	 *    9 Lane A
>> +	 *    B Lane D!
>> +	 */
>> +	serdes1_A: phy@0 {
>> +		#phy-cells = <0>;
>> +		reg = <0>;
>> +
>> +		/* SGMII.6 */
>> +		sgmii-0 {
>> +			fsl,pccr = <0x8>;
>> +			fsl,index = <0>;
>> +			fsl,cfg = <0x1>;
>> +			fsl,type = <PHY_TYPE_SGMII>;
>> +		};
>> +	};
>> +
>> +	serdes1_B: phy@1 {
>> +		#phy-cells = <0>;
>> +		reg = <1>;
>> +
>> +		/* SGMII.5 */
>> +		sgmii-1 {
>> +			fsl,pccr = <0x8>;
>> +			fsl,index = <1>;
>> +			fsl,cfg = <0x1>;
>> +			fsl,type = <PHY_TYPE_2500BASEX>;
>> +		};
>> +	};
>> +
>> +	serdes1_C: phy@2 {
>> +		#phy-cells = <0>;
>> +		reg = <2>;
>> +
>> +		/* SGMII.10 */
>> +		sgmii-2 {
>> +			fsl,pccr = <0x8>;
>> +			fsl,index = <2>;
>> +			fsl,cfg = <0x1>;
>> +			fsl,type = <PHY_TYPE_2500BASEX>;
>> +		};
>> +
>> +		/* XFI.10 */
>> +		xfi-0 {
>> +			fsl,pccr = <0xb>;
>> +			fsl,index = <0>;
>> +			fsl,cfg = <0x2>;
>> +			fsl,type = <PHY_TYPE_10GBASER>;
>> +		};
>> +	};
>> +
>> +	serdes1_D: phy@3 {
>> +		#phy-cells = <0>;
>> +		reg = <3>;
>> +
>> +		/* SGMII.9 */
>> +		sgmii-3 {
>> +			fsl,pccr = <0x8>;
>> +			fsl,index = <3>;
>> +			fsl,cfg = <0x1>;
>> +			fsl,type = <PHY_TYPE_2500BASEX>;
>> +		};
>> +
>> +		/* XFI.9 */
>> +		xfi-1 {
>> +			fsl,pccr = <0xb>;
>> +			fsl,index = <1>;
>> +			fsl,cfg = <0x1>;
>> +			fsl,type = <PHY_TYPE_10GBASER>;
>> +		};
>> +	};
>> +};
>> +
>> +
>>  &duart0 {
>>  	status = "okay";
>>  };
>> @@ -140,21 +244,29 @@ ethernet@e6000 {
>>  	ethernet@e8000 {
>>  		phy-handle = <&sgmii_phy1>;
>>  		phy-connection-type = "sgmii";
>> +		phys = <&serdes1_B>;
>> +		phy-names = "serdes";
>>  	};
>>  
>>  	ethernet@ea000 {
>>  		phy-handle = <&sgmii_phy2>;
>>  		phy-connection-type = "sgmii";
>> +		phys = <&serdes1_A>;
>> +		phy-names = "serdes";
>>  	};
>>  
>>  	ethernet@f0000 { /* 10GEC1 */
>>  		phy-handle = <&aqr106_phy>;
>>  		phy-connection-type = "xgmii";
>> +		phys = <&serdes1_D>;
>> +		phy-names = "serdes";
>>  	};
>>  
>>  	ethernet@f2000 { /* 10GEC2 */
>>  		fixed-link = <0 1 1000 0 0>;
>>  		phy-connection-type = "xgmii";
>> +		phys = <&serdes1_C>;
>> +		phy-names = "serdes";
>>  	};
>>  
>>  	mdio@fc000 {
>> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
>> index 6bebe00f5889..b396162dc859 100644
>> --- a/drivers/phy/freescale/Kconfig
>> +++ b/drivers/phy/freescale/Kconfig
>> @@ -54,6 +54,7 @@ config PHY_FSL_LYNX_10G
>>  	depends on ARCH_LAYERSCAPE || PPC || COMPILE_TEST
>>  	select GENERIC_PHY
>>  	select REGMAP_MMIO
>> +	default y if ARCH_LAYERSCAPE
>>  	help
>>  	  This adds support for the Lynx "SerDes" devices found on various QorIQ
>>  	  SoCs. There may be up to four SerDes devices on each SoC, and each
>> -- 
>> 2.35.1.1320.gc452695387.dirty
>>
Shawn Guo Jan. 27, 2023, 7:52 a.m. UTC | #3
On Thu, Jan 26, 2023 at 11:48:53AM -0500, Sean Anderson wrote:
> On 1/25/23 18:43, Shawn Guo wrote:
> > On Thu, Dec 29, 2022 at 07:01:36PM -0500, Sean Anderson wrote:
> >> This adds appropriate bindings for the macs which use the SerDes. The
> >> 156.25MHz fixed clock is a crystal. The 100MHz clocks (there are
> >> actually 3) come from a Renesas 6V49205B at address 69 on i2c0. There is
> >> no driver for this device (and as far as I know all you can do with the
> >> 100MHz clocks is gate them), so I have chosen to model it as a single
> >> fixed clock.
> >> 
> >> Note: the SerDes1 lane numbering for the LS1046A is *reversed*.
> >> This means that Lane A (what the driver thinks is lane 0) uses pins
> >> SD1_TX3_P/N.
> >> 
> >> Because this will break ethernet if the serdes is not enabled, enable
> >> the serdes driver by default on Layerscape.
> >> 
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >> This depends on [1].
> >> 
> >> [1] https://lore.kernel.org/netdev/20220804194705.459670-4-sean.anderson@seco.com/
> >> 
> >> Changes in v9:
> >> - Fix name of phy mode node
> >> - phy-type -> fsl,phy
> >> 
> >> Changes in v8:
> >> - Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc.
> >>   This should help remind readers that the numbering corresponds to the
> >>   physical layout of the registers, and not the lane (pin) number.
> >> 
> >> Changes in v6:
> >> - XGI.9 -> XFI.9
> >> 
> >> Changes in v4:
> >> - Convert to new bindings
> >> 
> >>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
> >>  drivers/phy/freescale/Kconfig                 |   1 +
> > 
> > The phy driver Kconfig change shouldn't be part of this patch.
> 
> I put it here for bisectability, since this is the point where we need
> to enable it. But I can do this in a separate patch if you want.
Sean Anderson Jan. 27, 2023, 4:11 p.m. UTC | #4
On 1/27/23 02:52, Shawn Guo wrote:
> On Thu, Jan 26, 2023 at 11:48:53AM -0500, Sean Anderson wrote:
>> On 1/25/23 18:43, Shawn Guo wrote:
>> > On Thu, Dec 29, 2022 at 07:01:36PM -0500, Sean Anderson wrote:
>> >> This adds appropriate bindings for the macs which use the SerDes. The
>> >> 156.25MHz fixed clock is a crystal. The 100MHz clocks (there are
>> >> actually 3) come from a Renesas 6V49205B at address 69 on i2c0. There is
>> >> no driver for this device (and as far as I know all you can do with the
>> >> 100MHz clocks is gate them), so I have chosen to model it as a single
>> >> fixed clock.
>> >> 
>> >> Note: the SerDes1 lane numbering for the LS1046A is *reversed*.
>> >> This means that Lane A (what the driver thinks is lane 0) uses pins
>> >> SD1_TX3_P/N.
>> >> 
>> >> Because this will break ethernet if the serdes is not enabled, enable
>> >> the serdes driver by default on Layerscape.
>> >> 
>> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> >> ---
>> >> This depends on [1].
>> >> 
>> >> [1] https://lore.kernel.org/netdev/20220804194705.459670-4-sean.anderson@seco.com/
>> >> 
>> >> Changes in v9:
>> >> - Fix name of phy mode node
>> >> - phy-type -> fsl,phy
>> >> 
>> >> Changes in v8:
>> >> - Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc.
>> >>   This should help remind readers that the numbering corresponds to the
>> >>   physical layout of the registers, and not the lane (pin) number.
>> >> 
>> >> Changes in v6:
>> >> - XGI.9 -> XFI.9
>> >> 
>> >> Changes in v4:
>> >> - Convert to new bindings
>> >> 
>> >>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>> >>  drivers/phy/freescale/Kconfig                 |   1 +
>> > 
>> > The phy driver Kconfig change shouldn't be part of this patch.
>> 
>> I put it here for bisectability, since this is the point where we need
>> to enable it. But I can do this in a separate patch if you want.
> 
> From DT ABI perspective, it's already broken anyway if you need to change
> kernel and DT atomically.

AIUI new kernels must work with old device trees, but new device trees need not
work with old kernels. So a change like this is fine, since the kernel won't
touch the serdes if it isn't supplied.

--Sean
Krzysztof Kozlowski Jan. 27, 2023, 4:15 p.m. UTC | #5
On 27/01/2023 17:11, Sean Anderson wrote:
>>>>>
>>>>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>>>>  drivers/phy/freescale/Kconfig                 |   1 +
>>>>
>>>> The phy driver Kconfig change shouldn't be part of this patch.
>>>
>>> I put it here for bisectability, since this is the point where we need
>>> to enable it. But I can do this in a separate patch if you want.
>>
>> From DT ABI perspective, it's already broken anyway if you need to change
>> kernel and DT atomically.
> 
> AIUI new kernels must work with old device trees, but new device trees need not
> work with old kernels. So a change like this is fine, since the kernel won't
> touch the serdes if it isn't supplied.

You used the argument "bisectability". If the patchset is not
bisectable, the ABI is broken.

Best regards,
Krzysztof
Sean Anderson Jan. 27, 2023, 4:22 p.m. UTC | #6
On 1/27/23 11:15, Krzysztof Kozlowski wrote:
> On 27/01/2023 17:11, Sean Anderson wrote:
>>>>>>
>>>>>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>>>>>  drivers/phy/freescale/Kconfig                 |   1 +
>>>>>
>>>>> The phy driver Kconfig change shouldn't be part of this patch.
>>>>
>>>> I put it here for bisectability, since this is the point where we need
>>>> to enable it. But I can do this in a separate patch if you want.
>>>
>>> From DT ABI perspective, it's already broken anyway if you need to change
>>> kernel and DT atomically.
>> 
>> AIUI new kernels must work with old device trees, but new device trees need not
>> work with old kernels. So a change like this is fine, since the kernel won't
>> touch the serdes if it isn't supplied.
> 
> You used the argument "bisectability". If the patchset is not
> bisectable, the ABI is broken.

Well, because Shawn wants it in a separate patch I am just going to enable
the driver by default on Layerscape before adding the device nodes. That way we have

1. Base state, driver not enabled and node is disabled
2. Driver enabled but not used because the node is disabled
3. Driver enabled and bound to node

So there is never a case where the node is bound but the driver isn't enabled
(which would cause the ethernet drivers to fail to probe).

--Sean
Krzysztof Kozlowski Jan. 27, 2023, 4:41 p.m. UTC | #7
On 27/01/2023 17:22, Sean Anderson wrote:
> On 1/27/23 11:15, Krzysztof Kozlowski wrote:
>> On 27/01/2023 17:11, Sean Anderson wrote:
>>>>>>>
>>>>>>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>>>>>>  drivers/phy/freescale/Kconfig                 |   1 +
>>>>>>
>>>>>> The phy driver Kconfig change shouldn't be part of this patch.
>>>>>
>>>>> I put it here for bisectability, since this is the point where we need
>>>>> to enable it. But I can do this in a separate patch if you want.
>>>>
>>>> From DT ABI perspective, it's already broken anyway if you need to change
>>>> kernel and DT atomically.
>>>
>>> AIUI new kernels must work with old device trees, but new device trees need not
>>> work with old kernels. So a change like this is fine, since the kernel won't
>>> touch the serdes if it isn't supplied.
>>
>> You used the argument "bisectability". If the patchset is not
>> bisectable, the ABI is broken.
> 
> Well, because Shawn wants it in a separate patch I am just going to enable
> the driver by default on Layerscape before adding the device nodes. That way we have
> 
> 1. Base state, driver not enabled and node is disabled
> 2. Driver enabled but not used because the node is disabled
> 3. Driver enabled and bound to node
> 
> So there is never a case where the node is bound but the driver isn't enabled
> (which would cause the ethernet drivers to fail to probe).

Then there is no bisectability issues and the Kconfig patch should have
been squashed here... Mentioning bisectability and that squash just
confuses.

Best regards,
Krzysztof
Sean Anderson Jan. 27, 2023, 4:42 p.m. UTC | #8
On 1/27/23 11:41, Krzysztof Kozlowski wrote:
> On 27/01/2023 17:22, Sean Anderson wrote:
>> On 1/27/23 11:15, Krzysztof Kozlowski wrote:
>>> On 27/01/2023 17:11, Sean Anderson wrote:
>>>>>>>>
>>>>>>>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>>>>>>>  drivers/phy/freescale/Kconfig                 |   1 +
>>>>>>>
>>>>>>> The phy driver Kconfig change shouldn't be part of this patch.
>>>>>>
>>>>>> I put it here for bisectability, since this is the point where we need
>>>>>> to enable it. But I can do this in a separate patch if you want.
>>>>>
>>>>> From DT ABI perspective, it's already broken anyway if you need to change
>>>>> kernel and DT atomically.
>>>>
>>>> AIUI new kernels must work with old device trees, but new device trees need not
>>>> work with old kernels. So a change like this is fine, since the kernel won't
>>>> touch the serdes if it isn't supplied.
>>>
>>> You used the argument "bisectability". If the patchset is not
>>> bisectable, the ABI is broken.
>> 
>> Well, because Shawn wants it in a separate patch I am just going to enable
>> the driver by default on Layerscape before adding the device nodes. That way we have
>> 
>> 1. Base state, driver not enabled and node is disabled
>> 2. Driver enabled but not used because the node is disabled
>> 3. Driver enabled and bound to node
>> 
>> So there is never a case where the node is bound but the driver isn't enabled
>> (which would cause the ethernet drivers to fail to probe).
> 
> Then there is no bisectability issues and the Kconfig patch should have
> been squashed here... Mentioning bisectability and that squash just
> confuses.

The Kconfig is currently squashed, but I am going to split it off as requested.

--Sean
Krzysztof Kozlowski Jan. 27, 2023, 4:44 p.m. UTC | #9
On 27/01/2023 17:42, Sean Anderson wrote:
> On 1/27/23 11:41, Krzysztof Kozlowski wrote:
>> On 27/01/2023 17:22, Sean Anderson wrote:
>>> On 1/27/23 11:15, Krzysztof Kozlowski wrote:
>>>> On 27/01/2023 17:11, Sean Anderson wrote:
>>>>>>>>>
>>>>>>>>>  .../boot/dts/freescale/fsl-ls1046a-rdb.dts    | 112 ++++++++++++++++++
>>>>>>>>>  drivers/phy/freescale/Kconfig                 |   1 +
>>>>>>>>
>>>>>>>> The phy driver Kconfig change shouldn't be part of this patch.
>>>>>>>
>>>>>>> I put it here for bisectability, since this is the point where we need
>>>>>>> to enable it. But I can do this in a separate patch if you want.
>>>>>>
>>>>>> From DT ABI perspective, it's already broken anyway if you need to change
>>>>>> kernel and DT atomically.
>>>>>
>>>>> AIUI new kernels must work with old device trees, but new device trees need not
>>>>> work with old kernels. So a change like this is fine, since the kernel won't
>>>>> touch the serdes if it isn't supplied.
>>>>
>>>> You used the argument "bisectability". If the patchset is not
>>>> bisectable, the ABI is broken.
>>>
>>> Well, because Shawn wants it in a separate patch I am just going to enable
>>> the driver by default on Layerscape before adding the device nodes. That way we have
>>>
>>> 1. Base state, driver not enabled and node is disabled
>>> 2. Driver enabled but not used because the node is disabled
>>> 3. Driver enabled and bound to node
>>>
>>> So there is never a case where the node is bound but the driver isn't enabled
>>> (which would cause the ethernet drivers to fail to probe).
>>
>> Then there is no bisectability issues and the Kconfig patch should have

"should have not been squashed", of course...

>> been squashed here... Mentioning bisectability and that squash just
>> confuses.
> 
> The Kconfig is currently squashed, but I am going to split it off as requested.

Yes, thanks.


Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
index 7025aad8ae89..534f19855b47 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
@@ -10,6 +10,8 @@ 
 
 /dts-v1/;
 
+#include <dt-bindings/phy/phy.h>
+
 #include "fsl-ls1046a.dtsi"
 
 / {
@@ -26,8 +28,110 @@  aliases {
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	clocks {
+		clk_100mhz: clock-100mhz {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <100000000>;
+		};
+
+		clk_156mhz: clock-156mhz {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <156250000>;
+		};
+	};
 };
 
+&serdes1 {
+	clocks = <&clk_100mhz>, <&clk_156mhz>;
+	clock-names = "ref0", "ref1";
+	status = "okay";
+
+	/*
+	 * XXX: Lane A uses pins SD1_RX3_P/N! That is, the lane numbers and pin
+	 * numbers are _reversed_. In addition, the PCCR documentation is
+	 * _inconsistent_ in its usage of these terms!
+	 *
+	 * PCCR "Lane 0" refers to...
+	 * ==== =====================
+	 *    0 Lane A
+	 *    2 Lane A
+	 *    8 Lane A
+	 *    9 Lane A
+	 *    B Lane D!
+	 */
+	serdes1_A: phy@0 {
+		#phy-cells = <0>;
+		reg = <0>;
+
+		/* SGMII.6 */
+		sgmii-0 {
+			fsl,pccr = <0x8>;
+			fsl,index = <0>;
+			fsl,cfg = <0x1>;
+			fsl,type = <PHY_TYPE_SGMII>;
+		};
+	};
+
+	serdes1_B: phy@1 {
+		#phy-cells = <0>;
+		reg = <1>;
+
+		/* SGMII.5 */
+		sgmii-1 {
+			fsl,pccr = <0x8>;
+			fsl,index = <1>;
+			fsl,cfg = <0x1>;
+			fsl,type = <PHY_TYPE_2500BASEX>;
+		};
+	};
+
+	serdes1_C: phy@2 {
+		#phy-cells = <0>;
+		reg = <2>;
+
+		/* SGMII.10 */
+		sgmii-2 {
+			fsl,pccr = <0x8>;
+			fsl,index = <2>;
+			fsl,cfg = <0x1>;
+			fsl,type = <PHY_TYPE_2500BASEX>;
+		};
+
+		/* XFI.10 */
+		xfi-0 {
+			fsl,pccr = <0xb>;
+			fsl,index = <0>;
+			fsl,cfg = <0x2>;
+			fsl,type = <PHY_TYPE_10GBASER>;
+		};
+	};
+
+	serdes1_D: phy@3 {
+		#phy-cells = <0>;
+		reg = <3>;
+
+		/* SGMII.9 */
+		sgmii-3 {
+			fsl,pccr = <0x8>;
+			fsl,index = <3>;
+			fsl,cfg = <0x1>;
+			fsl,type = <PHY_TYPE_2500BASEX>;
+		};
+
+		/* XFI.9 */
+		xfi-1 {
+			fsl,pccr = <0xb>;
+			fsl,index = <1>;
+			fsl,cfg = <0x1>;
+			fsl,type = <PHY_TYPE_10GBASER>;
+		};
+	};
+};
+
+
 &duart0 {
 	status = "okay";
 };
@@ -140,21 +244,29 @@  ethernet@e6000 {
 	ethernet@e8000 {
 		phy-handle = <&sgmii_phy1>;
 		phy-connection-type = "sgmii";
+		phys = <&serdes1_B>;
+		phy-names = "serdes";
 	};
 
 	ethernet@ea000 {
 		phy-handle = <&sgmii_phy2>;
 		phy-connection-type = "sgmii";
+		phys = <&serdes1_A>;
+		phy-names = "serdes";
 	};
 
 	ethernet@f0000 { /* 10GEC1 */
 		phy-handle = <&aqr106_phy>;
 		phy-connection-type = "xgmii";
+		phys = <&serdes1_D>;
+		phy-names = "serdes";
 	};
 
 	ethernet@f2000 { /* 10GEC2 */
 		fixed-link = <0 1 1000 0 0>;
 		phy-connection-type = "xgmii";
+		phys = <&serdes1_C>;
+		phy-names = "serdes";
 	};
 
 	mdio@fc000 {
diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 6bebe00f5889..b396162dc859 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -54,6 +54,7 @@  config PHY_FSL_LYNX_10G
 	depends on ARCH_LAYERSCAPE || PPC || COMPILE_TEST
 	select GENERIC_PHY
 	select REGMAP_MMIO
+	default y if ARCH_LAYERSCAPE
 	help
 	  This adds support for the Lynx "SerDes" devices found on various QorIQ
 	  SoCs. There may be up to four SerDes devices on each SoC, and each