Re: [PATCH 1/2] dt-bindings: phy: realtek: Add Realtek DHC RTD SoC PCIe PHY

Re: [PATCH 1/2] dt-bindings: phy: realtek: Add Realtek DHC RTD SoC PCIe PHY

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3034 bytes --]

On Fri, Dec 01, 2023 at 06:52:06PM +0800, Tzuyi Chang wrote:
> Add the device tree bindings for the Realtek DHC(Digital Home Center)
> RTD SoCs PCIe PHY.
> 
> Signed-off-by: Tzuyi Chang <tychang@realtek.com>
> ---
>  .../bindings/phy/realtek,rtd-pcie-phy.yaml    | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,rtd-pcie-phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,rtd-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/realtek,rtd-pcie-phy.yaml
> new file mode 100644
> index 000000000000..44ff23f698e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,rtd-pcie-phy.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Realtek Semiconductor Corporation
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,rtd-pcie-phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC PCIe PHY
> +
> +maintainers:
> +  - Tzuyi Chang <tychang@realtek.com>
> +
> +description:
> +  Realtek PCIe PHY supports the DHC(Digital Home Center) RTD series SoCs.
> +  The PCIe PHY driver is designed to support physical layer functionality
> +  of the PCIe controller.
> +
> +properties:
> +  compatible:
> +    enum:

> +      - realtek,rtd1319-pcie0-phy
> +      - realtek,rtd1319-pcie1-phy
> +      - realtek,rtd1319-pcie2-phy
> +      - realtek,rtd1619b-pcie1-phy
> +      - realtek,rtd1619b-pcie2-phy

Please explain why different PHYs on the same SoC need different
compatibles.

> +      - realtek,rtd1319d-pcie1-phy
> +      - realtek,rtd1315e-pcie1-phy

And why bother with the 1 here given there is no 0 or 2?

This looks suspiciously like abuse of the compatible - especially since
most of the ops are the same despite the differing compatibles. The case
where that does not apply, it looks like the issue is down to the
portion of the nvmem cell corresponding to the PHY, which has nothing to
do with the programming model of the PHY itself IMO.

Cheers,
Conor.

> +
> +  "#phy-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    maxItems: 1
> +    description:
> +      Phandle to nvmem cell that contains 'Tx swing trim'
> +      tuning parameter value for PCIe phy.
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: tx_swing_trim
> +
> +  realtek,pcie-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of syscon used to control PCIe MDIO register.
> +
> +required:
> +  - compatible
> +  - realtek,pcie-syscon
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pcie1_phy {
> +        compatible = "realtek,rtd1319d-pcie1-phy";
> +        realtek,pcie-syscon = <&pcie1>;
> +        #phy-cells = <0>;
> +        nvmem-cells = <&otp_pcie_tx_swing_trim>;
> +        nvmem-cell-names = "tx_swing_trim";
> +    };
> -- 
> 2.43.0
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/2] dt-bindings: phy: realtek: Add Realtek DHC RTD SoC PCIe PHY

[Krzysztof Kozlowski]

On 01/12/2023 11:52, Tzuyi Chang wrote:
> +  "#phy-cells":
> +    const: 0
> +
> +  nvmem-cells:
> +    maxItems: 1
> +    description:
> +      Phandle to nvmem cell that contains 'Tx swing trim'
> +      tuning parameter value for PCIe phy.
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: tx_swing_trim
> +
> +  realtek,pcie-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of syscon used to control PCIe MDIO register.

Why this does not have reg property but syscon? This looks hacky.

Where is the DTS of your platform so we can verify the bindings? In the
past Realtek bindings and DTS were sent without testing.
> +
> +required:
> +  - compatible
> +  - realtek,pcie-syscon
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pcie1_phy {

phy {



Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/2] dt-bindings: phy: realtek: Add Realtek DHC RTD SoC PCIe PHY

[Krzysztof Kozlowski]

On 07/12/2023 11:10, TY_Chang[張子逸] wrote:
> Hi Krzysztof,
> 
> Thank you for the review.
> 
>> On 01/12/2023 11:52, Tzuyi Chang wrote:
>>> +  "#phy-cells":
>>> +    const: 0
>>> +
>>> +  nvmem-cells:
>>> +    maxItems: 1
>>> +    description:
>>> +      Phandle to nvmem cell that contains 'Tx swing trim'
>>> +      tuning parameter value for PCIe phy.
>>> +
>>> +  nvmem-cell-names:
>>> +    items:
>>> +      - const: tx_swing_trim
>>> +
>>> +  realtek,pcie-syscon:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: phandle of syscon used to control PCIe MDIO register.
>>
>> Why this does not have reg property but syscon? This looks hacky.
>>
> 
> Our PCIe PHY driver needs to access two registers:
> 1. PCIe MDIO register: Utilized for configuring the PCIe PHY.
> 2. PCIe MAC Link Control and Link Status Register: Use to get the current
>   link speed for calibration purposes.
> 
> Both these registers reside within the PCIe controller registers. The PCIe
> driver has mapped these register address region, so I use regmap to access
> these registers.

Hm, isn't in such case PCIe PHY a child of the PCIe controller? How is
it with resources, like power domains or regulators?

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/2] phy: realtek: pcie: Add PCIe PHY support for Realtek DHC RTD SoCs

[Rob Herring]

On Fri, Dec 1, 2023 at 4:52 AM Tzuyi Chang <tychang@realtek.com> wrote:
>
> Implement the phy driver to support PCIe PHY for Realtek DHC (Digital Home
> Center) RTD SoCs.
>
> Signed-off-by: Tzuyi Chang <tychang@realtek.com>
> ---
>  drivers/phy/realtek/Kconfig        |   8 +
>  drivers/phy/realtek/Makefile       |   1 +
>  drivers/phy/realtek/phy-rtk-pcie.c | 738 +++++++++++++++++++++++++++++
>  3 files changed, 747 insertions(+)
>  create mode 100644 drivers/phy/realtek/phy-rtk-pcie.c
>
> diff --git a/drivers/phy/realtek/Kconfig b/drivers/phy/realtek/Kconfig
> index 75ac7e7c31ae..11c51f3714f1 100644
> --- a/drivers/phy/realtek/Kconfig
> +++ b/drivers/phy/realtek/Kconfig
> @@ -29,4 +29,12 @@ config PHY_RTK_RTD_USB3PHY
>           DWC3 USB IP. This driver will do the PHY initialization
>           of the parameters.
>
> +config PHY_RTD_PCIE
> +       tristate "Realtek RTD PCIe PHY driver"
> +       depends on OF
> +       select GENERIC_PHY
> +       help
> +         Enable this to support the PCIe PHY on Realtek DHC (digital home center)
> +         RTD series SoCs.
> +
>  endif # ARCH_REALTEK || COMPILE_TEST
> diff --git a/drivers/phy/realtek/Makefile b/drivers/phy/realtek/Makefile
> index ed7b47ff8a26..a1f0ad199476 100644
> --- a/drivers/phy/realtek/Makefile
> +++ b/drivers/phy/realtek/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_PHY_RTK_RTD_USB2PHY)      += phy-rtk-usb2.o
>  obj-$(CONFIG_PHY_RTK_RTD_USB3PHY)      += phy-rtk-usb3.o
> +obj-$(CONFIG_PHY_RTD_PCIE)                     += phy-rtk-pcie.o
> diff --git a/drivers/phy/realtek/phy-rtk-pcie.c b/drivers/phy/realtek/phy-rtk-pcie.c
> new file mode 100644
> index 000000000000..8ec845890271
> --- /dev/null
> +++ b/drivers/phy/realtek/phy-rtk-pcie.c
> @@ -0,0 +1,738 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Realtek DHC PCIe PHY driver
> + *
> + * Copyright (c) 2023 Realtek Semiconductor Corp.
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>

You probably don't need this header and the implicit includes it makes
are dropped now in linux-next. Please check what you actually need and
make them explicit.

of_address.h is likely not needed either. Please check.

Rob

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy