LinuxLists.cc - [PATCH v2 0/5] support ipq5332 platform

[permalink] [raw]

Subject: [PATCH v2 4/5] net: mdio: ipq4019: support MDIO clock frequency divider

The MDIO clock frequency can be divided according to the
MDIO control register value.

The MDIO system clock is fixed to 100MHZ, the working
frequency is 100MHZ/(divider + 1), the divider value
is from the bit[7:0] of control register 0x40.

Signed-off-by: Luo Jie <[email protected]>
---
drivers/net/mdio/mdio-ipq4019.c | 45 +++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 8d3c6bae379f..2c724d3cd513 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -29,6 +29,9 @@
/* 0 = Clause 22, 1 = Clause 45 */
#define MDIO_MODE_C45 BIT(8)

+/* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is 100MHz */
+#define MDIO_CLK_DIV_MASK GENMASK(7, 0)
+
#define IPQ4019_MDIO_TIMEOUT 10000
#define IPQ4019_MDIO_SLEEP 10

@@ -77,6 +80,7 @@ struct ipq4019_mdio_data {
void __iomem *cmn_membase;
void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
struct clk *clk[MDIO_CLK_CNT];
+ int clk_div;
};

static const char *const mdio_clk_name[] = {
@@ -110,6 +114,7 @@ static int ipq4019_mdio_read_c45(struct mii_bus *bus, int mii_id, int mmd,
data = readl(priv->membase + MDIO_MODE_REG);

data |= MDIO_MODE_C45;
+ data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);

writel(data, priv->membase + MDIO_MODE_REG);

@@ -151,6 +156,7 @@ static int ipq4019_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
data = readl(priv->membase + MDIO_MODE_REG);

data &= ~MDIO_MODE_C45;
+ data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);

writel(data, priv->membase + MDIO_MODE_REG);

@@ -183,6 +189,7 @@ static int ipq4019_mdio_write_c45(struct mii_bus *bus, int mii_id, int mmd,
data = readl(priv->membase + MDIO_MODE_REG);

data |= MDIO_MODE_C45;
+ data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);

writel(data, priv->membase + MDIO_MODE_REG);

@@ -226,6 +233,7 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
data = readl(priv->membase + MDIO_MODE_REG);

data &= ~MDIO_MODE_C45;
+ data |= FIELD_PREP(MDIO_CLK_DIV_MASK, priv->clk_div);

writel(data, priv->membase + MDIO_MODE_REG);

@@ -397,6 +405,39 @@ static int ipq_mdio_reset(struct mii_bus *bus)
return ret;
}

+static int ipq_mdio_clk_set(struct platform_device *pdev, int *clk_div)
+{
+ int freq;
+
+ /* Keep the MDIO clock divider as the hardware default value 0xff if
+ * the MDIO property "clock-frequency" is not specified.
+ */
+ if (of_property_read_u32(pdev->dev.of_node, "clock-frequency", &freq)) {
+ *clk_div = 0xff;
+ return 0;
+ }
+
+ /* MDC frequency is SYS_CLK/(MDIO_CLK_DIV + 1), SYS_CLK is fixed
+ * to 100MHz, the MDIO_CLK_DIV can be only configured the valid
+ * values, other values cause malfunction.
+ */
+ switch (freq) {
+ case 12500000:
+ case 6250000:
+ case 3125000:
+ case 1562500:
+ case 781250:
+ case 390625:
+ *clk_div = DIV_ROUND_UP(IPQ_MDIO_CLK_RATE, freq) - 1;
+ break;
+ default:
+ dev_err(&pdev->dev, "Invalid clock frequency %dHZ\n", freq);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int ipq4019_mdio_probe(struct platform_device *pdev)
{
struct ipq4019_mdio_data *priv;
@@ -459,6 +500,10 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
return PTR_ERR(priv->clk[index]);
}

+ ret = ipq_mdio_clk_set(pdev, &priv->clk_div);
+ if (ret)
+ return ret;
+
bus->name = "ipq4019_mdio";
bus->read = ipq4019_mdio_read_c22;
bus->write = ipq4019_mdio_write_c22;
--
2.42.0

2023-12-12 11:53:01

[permalink] [raw]

Subject: [PATCH v2 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

The reference clock of CMN PLL block is selectable, the internal
48MHZ is used by default.

The output clock of CMN PLL block is for providing the clock
source of ethernet device(such as qca8084), there are 1 * 25MHZ
and 3 * 50MHZ output clocks available for the ethernet devices.

Signed-off-by: Luo Jie <[email protected]>
---
drivers/net/mdio/mdio-ipq4019.c | 137 +++++++++++++++++++++++++++++++-
1 file changed, 136 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
index 582e41ab0990..8d3c6bae379f 100644
--- a/drivers/net/mdio/mdio-ipq4019.c
+++ b/drivers/net/mdio/mdio-ipq4019.c
@@ -44,6 +44,25 @@
/* Maximum SOC PCS(uniphy) number on IPQ platform */
#define ETH_LDO_RDY_CNT 3

+#define CMN_PLL_REFERENCE_SOURCE_SEL 0x28
+#define CMN_PLL_REFCLK_SOURCE_DIV GENMASK(9, 8)
+
+#define CMN_PLL_REFERENCE_CLOCK 0x784
+#define CMN_PLL_REFCLK_EXTERNAL BIT(9)
+#define CMN_PLL_REFCLK_DIV GENMASK(8, 4)
+#define CMN_PLL_REFCLK_INDEX GENMASK(3, 0)
+
+#define CMN_PLL_POWER_ON_AND_RESET 0x780
+#define CMN_ANA_EN_SW_RSTN BIT(6)
+
+#define CMN_REFCLK_INTERNAL_48MHZ 0
+#define CMN_REFCLK_EXTERNAL_25MHZ 1
+#define CMN_REFCLK_EXTERNAL_31250KHZ 2
+#define CMN_REFCLK_EXTERNAL_40MHZ 3
+#define CMN_REFCLK_EXTERNAL_48MHZ 4
+#define CMN_REFCLK_EXTERNAL_50MHZ 5
+#define CMN_REFCLK_INTERNAL_96MHZ 6
+
enum mdio_clk_id {
MDIO_CLK_MDIO_AHB,
MDIO_CLK_UNIPHY0_AHB,
@@ -55,6 +74,7 @@ enum mdio_clk_id {

struct ipq4019_mdio_data {
void __iomem *membase;
+ void __iomem *cmn_membase;
void __iomem *eth_ldo_rdy[ETH_LDO_RDY_CNT];
struct clk *clk[MDIO_CLK_CNT];
};
@@ -227,12 +247,116 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
return 0;
}

+/* For the CMN PLL block, the reference clock can be configured according to
+ * the device tree property "cmn-reference-clock", the internal 48MHZ is used
+ * by default on the ipq533 platform.
+ *
+ * The output clock of CMN PLL block is provided to the ethernet devices,
+ * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
+ *
+ * Such as the output 50M clock for the qca8084 ethernet PHY.
+ */
+static int ipq_cmn_clock_config(struct mii_bus *bus)
+{
+ int ret;
+ u32 reg_val, src_sel, ref_clk;
+ struct ipq4019_mdio_data *priv;
+
+ priv = bus->priv;
+ if (priv->cmn_membase) {
+ reg_val = readl(priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
+
+ /* Select reference clock source */
+ ret = of_property_read_u32(bus->parent->of_node,
+ "cmn-reference-clock",
+ &ref_clk);
+ if (!ret) {
+ switch (ref_clk) {
+ case CMN_REFCLK_INTERNAL_48MHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+ break;
+ case CMN_REFCLK_EXTERNAL_25MHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+ FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3));
+ break;
+ case CMN_REFCLK_EXTERNAL_31250KHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+ FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4));
+ break;
+ case CMN_REFCLK_EXTERNAL_40MHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+ FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6));
+ break;
+ case CMN_REFCLK_EXTERNAL_48MHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+ FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7));
+ break;
+ case CMN_REFCLK_EXTERNAL_50MHZ:
+ reg_val &= ~(CMN_PLL_REFCLK_EXTERNAL |
+ CMN_PLL_REFCLK_INDEX);
+ reg_val |= (CMN_PLL_REFCLK_EXTERNAL |
+ FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8));
+ break;
+ case CMN_REFCLK_INTERNAL_96MHZ:
+ src_sel = readl(priv->cmn_membase +
+ CMN_PLL_REFERENCE_SOURCE_SEL);
+ src_sel &= ~CMN_PLL_REFCLK_SOURCE_DIV;
+ src_sel |= FIELD_PREP(CMN_PLL_REFCLK_SOURCE_DIV, 0);
+ writel(src_sel, priv->cmn_membase +
+ CMN_PLL_REFERENCE_SOURCE_SEL);
+
+ reg_val &= ~CMN_PLL_REFCLK_DIV;
+ reg_val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
+ break;
+ default:
+ return -EINVAL;
+ }
+ } else if (ret == -EINVAL) {
+ /* If the cmn-reference-clock is not specified,
+ * the internal 48MHZ is selected by default.
+ */
+ reg_val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+ } else {
+ return ret;
+ }
+
+ writel(reg_val, priv->cmn_membase + CMN_PLL_REFERENCE_CLOCK);
+
+ /* assert CMN PLL */
+ reg_val = readl(priv->cmn_membase + CMN_PLL_POWER_ON_AND_RESET);
+ reg_val &= ~CMN_ANA_EN_SW_RSTN;
+ writel(reg_val, priv->cmn_membase);
+ fsleep(IPQ_PHY_SET_DELAY_US);
+
+ /* deassert CMN PLL */
+ reg_val |= CMN_ANA_EN_SW_RSTN;
+ writel(reg_val, priv->cmn_membase + CMN_PLL_POWER_ON_AND_RESET);
+ fsleep(IPQ_PHY_SET_DELAY_US);
+ }
+
+ return 0;
+}
+
static int ipq_mdio_reset(struct mii_bus *bus)
{
struct ipq4019_mdio_data *priv = bus->priv;
int ret, index;
unsigned long rate;

+ ret = ipq_cmn_clock_config(bus);
+ if (ret)
+ return ret;
+
/* For the platform ipq5332, there are two SoC uniphies available
* for connecting with ethernet PHY, the SoC uniphy gcc clock
* should be enabled for resetting the connected device such
@@ -296,7 +420,7 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
/* This resource are optional */
for (index = 0; index < ETH_LDO_RDY_CNT; index++) {
res = platform_get_resource(pdev, IORESOURCE_MEM, index + 1);
- if (res) {
+ if (res && strcmp(res->name, "cmn_blk")) {
priv->eth_ldo_rdy[index] = devm_ioremap(&pdev->dev,
res->start,
resource_size(res));
@@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
}
}

+ /* The CMN block resource is for providing clock source to ethernet,
+ * which can be optionally configured on the platform ipq9574 and
+ * ipq5332.
+ */
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
+ if (res) {
+ priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->cmn_membase))
+ return PTR_ERR(priv->cmn_membase);
+ }
+
for (index = 0; index < MDIO_CLK_CNT; index++) {
priv->clk[index] = devm_clk_get_optional(&pdev->dev,
mdio_clk_name[index]);
--
2.42.0

2023-12-12 11:53:06

[permalink] [raw]

Subject: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

Update the yaml file for the new DTS properties.

1. cmn-reference-clock for the CMN PLL source clock select.
2. clock-frequency for MDIO clock frequency config.
3. add uniphy AHB & SYS GCC clocks.
4. add reset-gpios for MDIO bus level reset.

Signed-off-by: Luo Jie <[email protected]>
---
.../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
1 file changed, 153 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
index 3407e909e8a7..9546a6ad7841 100644
--- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
@@ -20,6 +20,8 @@ properties:
- enum:
- qcom,ipq6018-mdio
- qcom,ipq8074-mdio
+ - qcom,ipq9574-mdio
+ - qcom,ipq5332-mdio
- const: qcom,ipq4019-mdio

"#address-cells":
@@ -30,19 +32,71 @@ properties:

reg:
minItems: 1
- maxItems: 2
+ maxItems: 5
description:
- the first Address and length of the register set for the MDIO controller.
- the second Address and length of the register for ethernet LDO, this second
- address range is only required by the platform IPQ50xx.
+ the first Address and length of the register set for the MDIO controller,
+ the optional second, third and fourth address and length of the register
+ for ethernet LDO, these three address range are required by the platform
+ IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
+ to select the reference clock.
+
+ reg-names:
+ minItems: 1
+ maxItems: 5

clocks:
+ minItems: 1
items:
- description: MDIO clock source frequency fixed to 100MHZ
+ - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
+ - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
+ - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
+ - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ

clock-names:
+ minItems: 1
items:
- const: gcc_mdio_ahb_clk
+ - const: gcc_uniphy0_ahb_clk
+ - const: gcc_uniphy1_ahb_clk
+ - const: gcc_uniphy0_sys_clk
+ - const: gcc_uniphy1_sys_clk
+
+ cmn-reference-clock:
+ oneOf:
+ - items:
+ - enum:
+ - 0 # CMN PLL reference internal 48MHZ
+ - 1 # CMN PLL reference external 25MHZ
+ - 2 # CMN PLL reference external 31250KHZ
+ - 3 # CMN PLL reference external 40MHZ
+ - 4 # CMN PLL reference external 48MHZ
+ - 5 # CMN PLL reference external 50MHZ
+ - 6 # CMN PLL reference internal 96MHZ
+
+ clock-frequency:
+ oneOf:
+ - items:
+ - enum:
+ - 12500000
+ - 6250000
+ - 3125000
+ - 1562500
+ - 781250
+ - 390625
+ description:
+ The MDIO bus clock that must be output by the MDIO bus hardware,
+ only the listed frequecies above can be configured, other frequency
+ will cause malfunction. If absent, the default hardware value is used.
+
+ reset-gpios:
+ maxItems: 1
+
+ reset-assert-us:
+ maxItems: 1
+
+ reset-deassert-us:
+ maxItems: 1

required:
- compatible
@@ -61,6 +115,8 @@ allOf:
- qcom,ipq5018-mdio
- qcom,ipq6018-mdio
- qcom,ipq8074-mdio
+ - qcom,ipq5332-mdio
+ - qcom,ipq9574-mdio
then:
required:
- clocks
@@ -70,6 +126,40 @@ allOf:
clocks: false
clock-names: false

+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq5332-mdio
+ then:
+ properties:
+ clocks:
+ minItems: 5
+ maxItems: 5
+ reg-names:
+ items:
+ - const: mdio
+ - const: eth_ldo1
+ - const: eth_ldo2
+ - const: cmn_blk
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq9574-mdio
+ then:
+ properties:
+ reg-names:
+ items:
+ - const: mdio
+ - const: eth_ldo1
+ - const: eth_ldo2
+ - const: eth_ldo3
+ - const: cmn_blk
+
unevaluatedProperties: false

examples:
@@ -100,3 +190,62 @@ examples:
reg = <4>;
};
};
+
+ - |
+ #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ mdio@90000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "qcom,ipq5332-mdio",
+ "qcom,ipq4019-mdio";
+ cmn-reference-clock = <0>;
+ clock-frequency = <6250000>;
+
+ reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>;
+ reset-assert-us = <100000>;
+ reset-deassert-us = <100000>;
+
+ reg = <0x90000 0x64>,
+ <0x7A00610 0x4>,
+ <0x7A10610 0x4>,
+ <0x9B000 0x800>;
+
+ reg-names = "mdio",
+ "eth_ldo1",
+ "eth_ldo2",
+ "cmn_blk";
+
+ clocks = <&gcc GCC_MDIO_AHB_CLK>,
+ <&gcc GCC_UNIPHY0_AHB_CLK>,
+ <&gcc GCC_UNIPHY1_AHB_CLK>,
+ <&gcc GCC_UNIPHY0_SYS_CLK>,
+ <&gcc GCC_UNIPHY1_SYS_CLK>;
+
+ clock-names = "gcc_mdio_ahb_clk",
+ "gcc_uniphy0_ahb_clk",
+ "gcc_uniphy1_ahb_clk",
+ "gcc_uniphy0_sys_clk",
+ "gcc_uniphy1_sys_clk";
+
+ qca8kphy0: ethernet-phy@1 {
+ compatible = "ethernet-phy-id004d.d180";
+ reg = <1>;
+ };
+
+ qca8kphy1: ethernet-phy@2 {
+ compatible = "ethernet-phy-id004d.d180";
+ reg = <2>;
+ };
+
+ qca8kphy2: ethernet-phy@3 {
+ compatible = "ethernet-phy-id004d.d180";
+ reg = <3>;
+ };
+
+ qca8kphy3: ethernet-phy@4 {
+ compatible = "ethernet-phy-id004d.d180";
+ reg = <4>;
+ };
+ };
--
2.42.0

2023-12-12 12:54:32

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

Hello,

I have some more minor comments for yoi :)

On Tue, 12 Dec 2023 19:51:48 +0800
Luo Jie <[email protected]> wrote:

> The reference clock of CMN PLL block is selectable, the internal
> 48MHZ is used by default.
>
> The output clock of CMN PLL block is for providing the clock
> source of ethernet device(such as qca8084), there are 1 * 25MHZ
> and 3 * 50MHZ output clocks available for the ethernet devices.
>
> Signed-off-by: Luo Jie <[email protected]>
> ---

[...]

> +/* For the CMN PLL block, the reference clock can be configured according to
> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
> + * by default on the ipq533 platform.
> + *
> + * The output clock of CMN PLL block is provided to the ethernet devices,
> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
> + *
> + * Such as the output 50M clock for the qca8084 ethernet PHY.
> + */
> +static int ipq_cmn_clock_config(struct mii_bus *bus)
> +{
> + int ret;
> + u32 reg_val, src_sel, ref_clk;
> + struct ipq4019_mdio_data *priv;

Here you should also use reverse christmas-tree notation

[...]

> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
> }
> }
>
> + /* The CMN block resource is for providing clock source to ethernet,
> + * which can be optionally configured on the platform ipq9574 and
> + * ipq5332.
> + */
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> + if (res) {
> + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->cmn_membase))
> + return PTR_ERR(priv->cmn_membase);
> + }
> +

And here you can simplify a bit by using
devm_platform_ioremap_resource_byname()

Thanks,

Maxime

2023-12-12 13:24:52

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Tue, 12 Dec 2023 19:51:50 +0800, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
>
> 1. cmn-reference-clock for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.
> 4. add reset-gpios for MDIO bus level reset.
>
> Signed-off-by: Luo Jie <[email protected]>
> ---
> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
> 1 file changed, 153 insertions(+), 4 deletions(-)
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml: cmn-reference-clock: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

2023-12-12 16:07:04

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

Attachments:

(No filename) (3.79 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-12 19:13:35

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

On Tue, Dec 12, 2023 at 01:54:17PM +0100, Maxime Chevallier wrote:
> Hello,
>
> I have some more minor comments for yoi :)
>
> On Tue, 12 Dec 2023 19:51:48 +0800
> Luo Jie <[email protected]> wrote:
> > + /* The CMN block resource is for providing clock source to ethernet,
> > + * which can be optionally configured on the platform ipq9574 and
> > + * ipq5332.
> > + */
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> > + if (res) {
> > + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv->cmn_membase))
> > + return PTR_ERR(priv->cmn_membase);
> > + }
> > +
>
> And here you can simplify a bit by using
> devm_platform_ioremap_resource_byname()

Not if the resource is optional.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2023-12-12 20:06:49

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
> Update the yaml file for the new DTS properties.
>
> 1. cmn-reference-clock for the CMN PLL source clock select.
> 2. clock-frequency for MDIO clock frequency config.
> 3. add uniphy AHB & SYS GCC clocks.
> 4. add reset-gpios for MDIO bus level reset.
>
> Signed-off-by: Luo Jie <[email protected]>
> ---
> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
> 1 file changed, 153 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..9546a6ad7841 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -20,6 +20,8 @@ properties:
> - enum:
> - qcom,ipq6018-mdio
> - qcom,ipq8074-mdio
> + - qcom,ipq9574-mdio
> + - qcom,ipq5332-mdio
> - const: qcom,ipq4019-mdio

A driver can function without knowing about all these new registers and
clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio".

>
> "#address-cells":
> @@ -30,19 +32,71 @@ properties:
>
> reg:
> minItems: 1
> - maxItems: 2
> + maxItems: 5
> description:
> - the first Address and length of the register set for the MDIO controller.
> - the second Address and length of the register for ethernet LDO, this second
> - address range is only required by the platform IPQ50xx.
> + the first Address and length of the register set for the MDIO controller,
> + the optional second, third and fourth address and length of the register
> + for ethernet LDO, these three address range are required by the platform
> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
> + to select the reference clock.
> +
> + reg-names:
> + minItems: 1
> + maxItems: 5
>
> clocks:
> + minItems: 1
> items:
> - description: MDIO clock source frequency fixed to 100MHZ
> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ

These are all clock inputs to this h/w block and not some other clocks
you want to manage?

>
> clock-names:
> + minItems: 1
> items:
> - const: gcc_mdio_ahb_clk
> + - const: gcc_uniphy0_ahb_clk
> + - const: gcc_uniphy1_ahb_clk
> + - const: gcc_uniphy0_sys_clk
> + - const: gcc_uniphy1_sys_clk

"gcc" is presumably the name of the clock controller in QCom chips.
Well, the clock source should not be part of the binding. The names
should be local for what they are for. So drop 'gcc_'. And '_clk' is
also redundant, so drop it too. Unfortunately you are stuck with the
name of the 1st entry.

> +
> + cmn-reference-clock:
> + oneOf:
> + - items:
> + - enum:
> + - 0 # CMN PLL reference internal 48MHZ
> + - 1 # CMN PLL reference external 25MHZ
> + - 2 # CMN PLL reference external 31250KHZ
> + - 3 # CMN PLL reference external 40MHZ
> + - 4 # CMN PLL reference external 48MHZ
> + - 5 # CMN PLL reference external 50MHZ
> + - 6 # CMN PLL reference internal 96MHZ
> +
> + clock-frequency:
> + oneOf:
> + - items:
> + - enum:
> + - 12500000
> + - 6250000
> + - 3125000
> + - 1562500
> + - 781250
> + - 390625
> + description:
> + The MDIO bus clock that must be output by the MDIO bus hardware,
> + only the listed frequecies above can be configured, other frequency
> + will cause malfunction. If absent, the default hardware value is used.
> +
> + reset-gpios:
> + maxItems: 1
> +
> + reset-assert-us:
> + maxItems: 1
> +
> + reset-deassert-us:
> + maxItems: 1
>
> required:
> - compatible
> @@ -61,6 +115,8 @@ allOf:
> - qcom,ipq5018-mdio
> - qcom,ipq6018-mdio
> - qcom,ipq8074-mdio
> + - qcom,ipq5332-mdio
> + - qcom,ipq9574-mdio
> then:
> required:
> - clocks
> @@ -70,6 +126,40 @@ allOf:
> clocks: false
> clock-names: false
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,ipq5332-mdio
> + then:
> + properties:
> + clocks:
> + minItems: 5
> + maxItems: 5
> + reg-names:
> + items:
> + - const: mdio
> + - const: eth_ldo1
> + - const: eth_ldo2
> + - const: cmn_blk

Perhaps cmn_blk should come 2nd, so all the variants have the same entry
indices. Then you can move this to the top level and just say 'minItems:
4' here.

> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,ipq9574-mdio
> + then:
> + properties:
> + reg-names:
> + items:
> + - const: mdio
> + - const: eth_ldo1
> + - const: eth_ldo2
> + - const: eth_ldo3
> + - const: cmn_blk

And 'minItems: 5' here.

The ipq9574 adds the CMN block, but none of the clocks? Weird.

Rob

2023-12-13 08:10:52

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

On 12/12/2023 8:54 PM, Maxime Chevallier wrote:
> Hello,
>
> I have some more minor comments for yoi :)
>
> On Tue, 12 Dec 2023 19:51:48 +0800
> Luo Jie <[email protected]> wrote:
>
>> The reference clock of CMN PLL block is selectable, the internal
>> 48MHZ is used by default.
>>
>> The output clock of CMN PLL block is for providing the clock
>> source of ethernet device(such as qca8084), there are 1 * 25MHZ
>> and 3 * 50MHZ output clocks available for the ethernet devices.
>>
>> Signed-off-by: Luo Jie <[email protected]>
>> ---
>
> [...]
>
>> +/* For the CMN PLL block, the reference clock can be configured according to
>> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
>> + * by default on the ipq533 platform.
>> + *
>> + * The output clock of CMN PLL block is provided to the ethernet devices,
>> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
>> + *
>> + * Such as the output 50M clock for the qca8084 ethernet PHY.
>> + */
>> +static int ipq_cmn_clock_config(struct mii_bus *bus)
>> +{
>> + int ret;
>> + u32 reg_val, src_sel, ref_clk;
>> + struct ipq4019_mdio_data *priv;
>
> Here you should also use reverse christmas-tree notation

Ok, will correct this, thanks.

>
> [...]
>
>> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
>> }
>> }
>>
>> + /* The CMN block resource is for providing clock source to ethernet,
>> + * which can be optionally configured on the platform ipq9574 and
>> + * ipq5332.
>> + */
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
>> + if (res) {
>> + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(priv->cmn_membase))
>> + return PTR_ERR(priv->cmn_membase);
>> + }
>> +
>
> And here you can simplify a bit by using
> devm_platform_ioremap_resource_byname()
>
> Thanks,
>
> Maxime
>
As Russell mentioned, since this resource is optional,
so devm_platform_ioremap_resource_byname can't be used here.

2023-12-13 08:27:31

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/13/2023 12:06 AM, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>> Update the yaml file for the new DTS properties.
>>
>> 1. cmn-reference-clock for the CMN PLL source clock select.
>> 2. clock-frequency for MDIO clock frequency config.
>> 3. add uniphy AHB & SYS GCC clocks.
>> 4. add reset-gpios for MDIO bus level reset.
>>
>> Signed-off-by: Luo Jie <[email protected]>
>> ---
>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..9546a6ad7841 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -20,6 +20,8 @@ properties:
>> - enum:
>> - qcom,ipq6018-mdio
>> - qcom,ipq8074-mdio
>> + - qcom,ipq9574-mdio
>> + - qcom,ipq5332-mdio
>> - const: qcom,ipq4019-mdio
>>
>> "#address-cells":
>> @@ -30,19 +32,71 @@ properties:
>>
>> reg:
>> minItems: 1
>> - maxItems: 2
>> + maxItems: 5
>> description:
>> - the first Address and length of the register set for the MDIO controller.
>> - the second Address and length of the register for ethernet LDO, this second
>> - address range is only required by the platform IPQ50xx.
>> + the first Address and length of the register set for the MDIO controller,
>> + the optional second, third and fourth address and length of the register
>> + for ethernet LDO, these three address range are required by the platform
>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>> + to select the reference clock.
>> +
>> + reg-names:
>> + minItems: 1
>> + maxItems: 5
>>
>> clocks:
>> + minItems: 1
>> items:
>> - description: MDIO clock source frequency fixed to 100MHZ
>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>
>> clock-names:
>> + minItems: 1
>> items:
>> - const: gcc_mdio_ahb_clk
>> + - const: gcc_uniphy0_ahb_clk
>> + - const: gcc_uniphy1_ahb_clk
>> + - const: gcc_uniphy0_sys_clk
>> + - const: gcc_uniphy1_sys_clk
>
>> + cmn-reference-clock:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - 0 # CMN PLL reference internal 48MHZ
>> + - 1 # CMN PLL reference external 25MHZ
>> + - 2 # CMN PLL reference external 31250KHZ
>> + - 3 # CMN PLL reference external 40MHZ
>> + - 4 # CMN PLL reference external 48MHZ
>> + - 5 # CMN PLL reference external 50MHZ
>> + - 6 # CMN PLL reference internal 96MHZ
>
> Why is this not represented by an element of the clocks property?

This property is for the reference clock source selection of CMN PLL,
CMN PLL generates the different clock rates for the different Ethernet
blocks, this CMN PLL configuration is not located in the GCC, so the
clock framework can't be used, which is the general hardware register
instead of RCG register for GCC.

>
>> + clock-frequency:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - 12500000
>> + - 6250000
>> + - 3125000
>> + - 1562500
>> + - 781250
>> + - 390625
>> + description:
>> + The MDIO bus clock that must be output by the MDIO bus hardware,
>> + only the listed frequecies above can be configured, other frequency
>> + will cause malfunction. If absent, the default hardware value is used.
>
> Likewise.
>
> Your commit message contains a bullet point list of what you are doing,
> but there's no explanation here for why custom properties are required
> to provide clock information.
>
> Thanks,
> Conor.

Hi Conor,
This property clock-frequency is optional to configure the MDIO working
clock rate, and this is the MDIO general DT property, since the hardware
default clock rate is 390625HZ, there is requirement for higher clock
rate in the normal working case, i will update this information in the
next patch set.

2023-12-13 08:42:56

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/13/2023 4:06 AM, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>> Update the yaml file for the new DTS properties.
>>
>> 1. cmn-reference-clock for the CMN PLL source clock select.
>> 2. clock-frequency for MDIO clock frequency config.
>> 3. add uniphy AHB & SYS GCC clocks.
>> 4. add reset-gpios for MDIO bus level reset.
>>
>> Signed-off-by: Luo Jie <[email protected]>
>> ---
>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> index 3407e909e8a7..9546a6ad7841 100644
>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>> @@ -20,6 +20,8 @@ properties:
>> - enum:
>> - qcom,ipq6018-mdio
>> - qcom,ipq8074-mdio
>> + - qcom,ipq9574-mdio
>> + - qcom,ipq5332-mdio
>> - const: qcom,ipq4019-mdio
>
> A driver can function without knowing about all these new registers and
> clocks? If not, then it can't be compatible with "qcom,ipq4019-mdio".

Yes, the driver can work without knowing the compatible string.
the configuration is decided by the DT property defined or not.

>
>>
>> "#address-cells":
>> @@ -30,19 +32,71 @@ properties:
>>
>> reg:
>> minItems: 1
>> - maxItems: 2
>> + maxItems: 5
>> description:
>> - the first Address and length of the register set for the MDIO controller.
>> - the second Address and length of the register for ethernet LDO, this second
>> - address range is only required by the platform IPQ50xx.
>> + the first Address and length of the register set for the MDIO controller,
>> + the optional second, third and fourth address and length of the register
>> + for ethernet LDO, these three address range are required by the platform
>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>> + to select the reference clock.
>> +
>> + reg-names:
>> + minItems: 1
>> + maxItems: 5
>>
>> clocks:
>> + minItems: 1
>> items:
>> - description: MDIO clock source frequency fixed to 100MHZ
>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>
> These are all clock inputs to this h/w block and not some other clocks
> you want to manage?

Yes, for ipq5332, these 5 clocks are need to be managed, for the legacy
platform such as ipq8074, only MDIO clock is needed.

No other more clock needs to be managed for the current IPQ platforms.

>
>>
>> clock-names:
>> + minItems: 1
>> items:
>> - const: gcc_mdio_ahb_clk
>> + - const: gcc_uniphy0_ahb_clk
>> + - const: gcc_uniphy1_ahb_clk
>> + - const: gcc_uniphy0_sys_clk
>> + - const: gcc_uniphy1_sys_clk
>
> "gcc" is presumably the name of the clock controller in QCom chips.
> Well, the clock source should not be part of the binding. The names
> should be local for what they are for. So drop 'gcc_'. And '_clk' is
> also redundant, so drop it too. Unfortunately you are stuck with the
> name of the 1st entry.

Yes, gcc is the name of QCOM SOC clock controller.
will remove the "gcc_" and "_clk" for the new added clocks.

we should keep the existed DT gcc_mdio_ahb_clk unmodified, right?
since it has been used in the current device tree.

>
>> +
>> + cmn-reference-clock:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - 0 # CMN PLL reference internal 48MHZ
>> + - 1 # CMN PLL reference external 25MHZ
>> + - 2 # CMN PLL reference external 31250KHZ
>> + - 3 # CMN PLL reference external 40MHZ
>> + - 4 # CMN PLL reference external 48MHZ
>> + - 5 # CMN PLL reference external 50MHZ
>> + - 6 # CMN PLL reference internal 96MHZ
>> +
>> + clock-frequency:
>> + oneOf:
>> + - items:
>> + - enum:
>> + - 12500000
>> + - 6250000
>> + - 3125000
>> + - 1562500
>> + - 781250
>> + - 390625
>> + description:
>> + The MDIO bus clock that must be output by the MDIO bus hardware,
>> + only the listed frequecies above can be configured, other frequency
>> + will cause malfunction. If absent, the default hardware value is used.
>> +
>> + reset-gpios:
>> + maxItems: 1
>> +
>> + reset-assert-us:
>> + maxItems: 1
>> +
>> + reset-deassert-us:
>> + maxItems: 1
>>
>> required:
>> - compatible
>> @@ -61,6 +115,8 @@ allOf:
>> - qcom,ipq5018-mdio
>> - qcom,ipq6018-mdio
>> - qcom,ipq8074-mdio
>> + - qcom,ipq5332-mdio
>> + - qcom,ipq9574-mdio
>> then:
>> required:
>> - clocks
>> @@ -70,6 +126,40 @@ allOf:
>> clocks: false
>> clock-names: false
>>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - qcom,ipq5332-mdio
>> + then:
>> + properties:
>> + clocks:
>> + minItems: 5
>> + maxItems: 5
>> + reg-names:
>> + items:
>> + - const: mdio
>> + - const: eth_ldo1
>> + - const: eth_ldo2
>> + - const: cmn_blk
>
> Perhaps cmn_blk should come 2nd, so all the variants have the same entry
> indices. Then you can move this to the top level and just say 'minItems:
> 4' here.

Thanks Rob for the suggestion, i will update to move cmn_blk to the 2nd
location.

>
>
>> +
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + enum:
>> + - qcom,ipq9574-mdio
>> + then:
>> + properties:
>> + reg-names:
>> + items:
>> + - const: mdio
>> + - const: eth_ldo1
>> + - const: eth_ldo2
>> + - const: eth_ldo3
>> + - const: cmn_blk
>
> And 'minItems: 5' here.
>
> The ipq9574 adds the CMN block, but none of the clocks? Weird.
>
> Rob

For ipq9574, only mdio clock is needed, the uniphy ahb and sys clock is
not needed to configure.

Yes, there is some Ethernet design delta between ipq9574 and ipq5332.

2023-12-13 10:09:13

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [PATCH v2 3/5] net: mdio: ipq4019: configure CMN PLL clock for ipq5332

On Wed, 13 Dec 2023 16:09:53 +0800
Jie Luo <[email protected]> wrote:

> On 12/12/2023 8:54 PM, Maxime Chevallier wrote:
> > Hello,
> >
> > I have some more minor comments for yoi :)
> >
> > On Tue, 12 Dec 2023 19:51:48 +0800
> > Luo Jie <[email protected]> wrote:
> >
> >> The reference clock of CMN PLL block is selectable, the internal
> >> 48MHZ is used by default.
> >>
> >> The output clock of CMN PLL block is for providing the clock
> >> source of ethernet device(such as qca8084), there are 1 * 25MHZ
> >> and 3 * 50MHZ output clocks available for the ethernet devices.
> >>
> >> Signed-off-by: Luo Jie <[email protected]>
> >> ---
> >
> > [...]
> >
> >> +/* For the CMN PLL block, the reference clock can be configured according to
> >> + * the device tree property "cmn-reference-clock", the internal 48MHZ is used
> >> + * by default on the ipq533 platform.
> >> + *
> >> + * The output clock of CMN PLL block is provided to the ethernet devices,
> >> + * threre are 4 CMN PLL output clocks (1*25MHZ + 3*50MHZ) enabled by default.
> >> + *
> >> + * Such as the output 50M clock for the qca8084 ethernet PHY.
> >> + */
> >> +static int ipq_cmn_clock_config(struct mii_bus *bus)
> >> +{
> >> + int ret;
> >> + u32 reg_val, src_sel, ref_clk;
> >> + struct ipq4019_mdio_data *priv;
> >
> > Here you should also use reverse christmas-tree notation
>
> Ok, will correct this, thanks.
>
> >
> > [...]
> >
> >> @@ -317,6 +441,17 @@ static int ipq4019_mdio_probe(struct platform_device *pdev)
> >> }
> >> }
> >>
> >> + /* The CMN block resource is for providing clock source to ethernet,
> >> + * which can be optionally configured on the platform ipq9574 and
> >> + * ipq5332.
> >> + */
> >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cmn_blk");
> >> + if (res) {
> >> + priv->cmn_membase = devm_ioremap_resource(&pdev->dev, res);
> >> + if (IS_ERR(priv->cmn_membase))
> >> + return PTR_ERR(priv->cmn_membase);
> >> + }
> >> +
> >
> > And here you can simplify a bit by using
> > devm_platform_ioremap_resource_byname()
> >
> > Thanks,
> >
> > Maxime
> >
> As Russell mentioned, since this resource is optional,
> so devm_platform_ioremap_resource_byname can't be used here.
>

Indeed, my bad I missed that point. Sorry for the noise :/

Thanks,

Maxime

2023-12-14 17:13:42

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>
>
> On 12/13/2023 12:06 AM, Conor Dooley wrote:
> > On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
> > > Update the yaml file for the new DTS properties.
> > >
> > > 1. cmn-reference-clock for the CMN PLL source clock select.
> > > 2. clock-frequency for MDIO clock frequency config.
> > > 3. add uniphy AHB & SYS GCC clocks.
> > > 4. add reset-gpios for MDIO bus level reset.
> > >
> > > Signed-off-by: Luo Jie <[email protected]>
> > > ---
> > > .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
> > > 1 file changed, 153 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > index 3407e909e8a7..9546a6ad7841 100644
> > > --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> > > @@ -20,6 +20,8 @@ properties:
> > > - enum:
> > > - qcom,ipq6018-mdio
> > > - qcom,ipq8074-mdio
> > > + - qcom,ipq9574-mdio
> > > + - qcom,ipq5332-mdio
> > > - const: qcom,ipq4019-mdio
> > > "#address-cells":
> > > @@ -30,19 +32,71 @@ properties:
> > > reg:
> > > minItems: 1
> > > - maxItems: 2
> > > + maxItems: 5
> > > description:
> > > - the first Address and length of the register set for the MDIO controller.
> > > - the second Address and length of the register for ethernet LDO, this second
> > > - address range is only required by the platform IPQ50xx.
> > > + the first Address and length of the register set for the MDIO controller,
> > > + the optional second, third and fourth address and length of the register
> > > + for ethernet LDO, these three address range are required by the platform
> > > + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
> > > + to select the reference clock.
> > > +
> > > + reg-names:
> > > + minItems: 1
> > > + maxItems: 5
> > > clocks:
> > > + minItems: 1
> > > items:
> > > - description: MDIO clock source frequency fixed to 100MHZ
> > > + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
> > > + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
> > > + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
> > > + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
> > > clock-names:
> > > + minItems: 1
> > > items:
> > > - const: gcc_mdio_ahb_clk
> > > + - const: gcc_uniphy0_ahb_clk
> > > + - const: gcc_uniphy1_ahb_clk
> > > + - const: gcc_uniphy0_sys_clk
> > > + - const: gcc_uniphy1_sys_clk
> >
> > > + cmn-reference-clock:
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - 0 # CMN PLL reference internal 48MHZ
> > > + - 1 # CMN PLL reference external 25MHZ
> > > + - 2 # CMN PLL reference external 31250KHZ
> > > + - 3 # CMN PLL reference external 40MHZ
> > > + - 4 # CMN PLL reference external 48MHZ
> > > + - 5 # CMN PLL reference external 50MHZ
> > > + - 6 # CMN PLL reference internal 96MHZ
> >
> > Why is this not represented by an element of the clocks property?
>
> This property is for the reference clock source selection of CMN PLL,
> CMN PLL generates the different clock rates for the different Ethernet
> blocks, this CMN PLL configuration is not located in the GCC, so the
> clock framework can't be used, which is the general hardware register
> instead of RCG register for GCC.

I don't see how the clock being provided by the "GCC" (whatever that is)
or by some other clock controller or fixed clock makes a difference.
Why can't the other clock provider be represented in the devicetree?

> > > + clock-frequency:
> > > + oneOf:
> > > + - items:
> > > + - enum:
> > > + - 12500000
> > > + - 6250000
> > > + - 3125000
> > > + - 1562500
> > > + - 781250
> > > + - 390625
> > > + description:
> > > + The MDIO bus clock that must be output by the MDIO bus hardware,
> > > + only the listed frequecies above can be configured, other frequency
> > > + will cause malfunction. If absent, the default hardware value is used.
> >
> > Likewise.
> >
> > Your commit message contains a bullet point list of what you are doing,
> > but there's no explanation here for why custom properties are required
> > to provide clock information.

> This property clock-frequency is optional to configure the MDIO working
> clock rate, and this is the MDIO general DT property, since the hardware
> default clock rate is 390625HZ, there is requirement for higher clock rate
> in the normal working case, i will update this information in the
> next patch set.

I'm just realising that this particular one is not a custom property,
the unusual `oneOf: - items: - enum:` structure here threw me. This can
just be
clock-frequency:
enum:
- 12500000
- 6250000
- 3125000
- 1562500
- 781250
- 390625

but you're missing a default, given your commit about the last element
in that list being one.

Thanks,
Conor.

Attachments:

(No filename) (5.48 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-15 06:47:24

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 1:12 AM, Conor Dooley wrote:
> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>
>>
>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>> Update the yaml file for the new DTS properties.
>>>>
>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>> 2. clock-frequency for MDIO clock frequency config.
>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>
>>>> Signed-off-by: Luo Jie <[email protected]>
>>>> ---
>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>>>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>> @@ -20,6 +20,8 @@ properties:
>>>> - enum:
>>>> - qcom,ipq6018-mdio
>>>> - qcom,ipq8074-mdio
>>>> + - qcom,ipq9574-mdio
>>>> + - qcom,ipq5332-mdio
>>>> - const: qcom,ipq4019-mdio
>>>> "#address-cells":
>>>> @@ -30,19 +32,71 @@ properties:
>>>> reg:
>>>> minItems: 1
>>>> - maxItems: 2
>>>> + maxItems: 5
>>>> description:
>>>> - the first Address and length of the register set for the MDIO controller.
>>>> - the second Address and length of the register for ethernet LDO, this second
>>>> - address range is only required by the platform IPQ50xx.
>>>> + the first Address and length of the register set for the MDIO controller,
>>>> + the optional second, third and fourth address and length of the register
>>>> + for ethernet LDO, these three address range are required by the platform
>>>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>>>> + to select the reference clock.
>>>> +
>>>> + reg-names:
>>>> + minItems: 1
>>>> + maxItems: 5
>>>> clocks:
>>>> + minItems: 1
>>>> items:
>>>> - description: MDIO clock source frequency fixed to 100MHZ
>>>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>> clock-names:
>>>> + minItems: 1
>>>> items:
>>>> - const: gcc_mdio_ahb_clk
>>>> + - const: gcc_uniphy0_ahb_clk
>>>> + - const: gcc_uniphy1_ahb_clk
>>>> + - const: gcc_uniphy0_sys_clk
>>>> + - const: gcc_uniphy1_sys_clk
>>>
>>>> + cmn-reference-clock:
>>>> + oneOf:
>>>> + - items:
>>>> + - enum:
>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>> + - 1 # CMN PLL reference external 25MHZ
>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>> + - 3 # CMN PLL reference external 40MHZ
>>>> + - 4 # CMN PLL reference external 48MHZ
>>>> + - 5 # CMN PLL reference external 50MHZ
>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>
>>> Why is this not represented by an element of the clocks property?
>>
>> This property is for the reference clock source selection of CMN PLL,
>> CMN PLL generates the different clock rates for the different Ethernet
>> blocks, this CMN PLL configuration is not located in the GCC, so the
>> clock framework can't be used, which is the general hardware register
>> instead of RCG register for GCC.
>
> I don't see how the clock being provided by the "GCC" (whatever that is)
> or by some other clock controller or fixed clock makes a difference.
> Why can't the other clock provider be represented in the devicetree?
>

cmn-reference-clock is for selecting the reference clock source for the
whole Ethernet block, which is just the standalone configure register.
however the clock provider has the logical register distribution, such
as for one clock tree, there is RCG, DIVIDER and branch registers in
the qcom soc chip.

The clock consumer defines the clock IDs of device tree to reference the
clocks provided by the clock controller, and these clock IDs are
provided by the header file of clock provider.

like this,
clocks = <&gcc GCC_MDIO_AHB_CLK>,

<&gcc GCC_UNIPHY0_AHB_CLK>,

<&gcc GCC_UNIPHY1_AHB_CLK>,

<&gcc GCC_UNIPHY0_SYS_CLK>,

<&gcc GCC_UNIPHY1_SYS_CLK>;

gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the
clock ID.

>>>> + clock-frequency:
>>>> + oneOf:
>>>> + - items:
>>>> + - enum:
>>>> + - 12500000
>>>> + - 6250000
>>>> + - 3125000
>>>> + - 1562500
>>>> + - 781250
>>>> + - 390625
>>>> + description:
>>>> + The MDIO bus clock that must be output by the MDIO bus hardware,
>>>> + only the listed frequecies above can be configured, other frequency
>>>> + will cause malfunction. If absent, the default hardware value is used.
>>>
>>> Likewise.
>>>
>>> Your commit message contains a bullet point list of what you are doing,
>>> but there's no explanation here for why custom properties are required
>>> to provide clock information.
>
>> This property clock-frequency is optional to configure the MDIO working
>> clock rate, and this is the MDIO general DT property, since the hardware
>> default clock rate is 390625HZ, there is requirement for higher clock rate
>> in the normal working case, i will update this information in the
>> next patch set.
>
> I'm just realising that this particular one is not a custom property,
> the unusual `oneOf: - items: - enum:` structure here threw me. This can
> just be
> clock-frequency:
> enum:
> - 12500000
> - 6250000
> - 3125000
> - 1562500
> - 781250
> - 390625
>
> but you're missing a default, given your commit about the last element
> in that list being one.
>
> Thanks,
> Conor.

Ok, will update this in the next patch set, thanks for the comments.

2023-12-15 07:30:07

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 15/12/2023 07:46, Jie Luo wrote:
>
>
> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>
>>>
>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>> Update the yaml file for the new DTS properties.
>>>>>
>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>
>>>>> Signed-off-by: Luo Jie <[email protected]>
>>>>> ---
>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>>>>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>> @@ -20,6 +20,8 @@ properties:
>>>>> - enum:
>>>>> - qcom,ipq6018-mdio
>>>>> - qcom,ipq8074-mdio
>>>>> + - qcom,ipq9574-mdio
>>>>> + - qcom,ipq5332-mdio
>>>>> - const: qcom,ipq4019-mdio
>>>>> "#address-cells":
>>>>> @@ -30,19 +32,71 @@ properties:
>>>>> reg:
>>>>> minItems: 1
>>>>> - maxItems: 2
>>>>> + maxItems: 5
>>>>> description:
>>>>> - the first Address and length of the register set for the MDIO controller.
>>>>> - the second Address and length of the register for ethernet LDO, this second
>>>>> - address range is only required by the platform IPQ50xx.
>>>>> + the first Address and length of the register set for the MDIO controller,
>>>>> + the optional second, third and fourth address and length of the register
>>>>> + for ethernet LDO, these three address range are required by the platform
>>>>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>>>>> + to select the reference clock.
>>>>> +
>>>>> + reg-names:
>>>>> + minItems: 1
>>>>> + maxItems: 5
>>>>> clocks:
>>>>> + minItems: 1
>>>>> items:
>>>>> - description: MDIO clock source frequency fixed to 100MHZ
>>>>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>> clock-names:
>>>>> + minItems: 1
>>>>> items:
>>>>> - const: gcc_mdio_ahb_clk
>>>>> + - const: gcc_uniphy0_ahb_clk
>>>>> + - const: gcc_uniphy1_ahb_clk
>>>>> + - const: gcc_uniphy0_sys_clk
>>>>> + - const: gcc_uniphy1_sys_clk
>>>>
>>>>> + cmn-reference-clock:
>>>>> + oneOf:
>>>>> + - items:
>>>>> + - enum:
>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>
>>>> Why is this not represented by an element of the clocks property?
>>>
>>> This property is for the reference clock source selection of CMN PLL,
>>> CMN PLL generates the different clock rates for the different Ethernet
>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>> clock framework can't be used, which is the general hardware register
>>> instead of RCG register for GCC.
>>
>> I don't see how the clock being provided by the "GCC" (whatever that is)
>> or by some other clock controller or fixed clock makes a difference.
>> Why can't the other clock provider be represented in the devicetree?
>>
>
> cmn-reference-clock is for selecting the reference clock source for the
> whole Ethernet block, which is just the standalone configure register.

Sure, you are aware though that all clocks are just configure registers?

Which clocks are these mentioned in the property? From where do they come?

Anyway, property is in existing form is not correct - this is not a
generic property.

> however the clock provider has the logical register distribution, such
> as for one clock tree, there is RCG, DIVIDER and branch registers in
> the qcom soc chip.
>
> The clock consumer defines the clock IDs of device tree to reference the
> clocks provided by the clock controller, and these clock IDs are
> provided by the header file of clock provider.
>
> like this,
> clocks = <&gcc GCC_MDIO_AHB_CLK>,
>
> <&gcc GCC_UNIPHY0_AHB_CLK>,
>
> <&gcc GCC_UNIPHY1_AHB_CLK>,
>
> <&gcc GCC_UNIPHY0_SYS_CLK>,
>
> <&gcc GCC_UNIPHY1_SYS_CLK>;
>
> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the
> clock ID.

Best regards,
Krzysztof

2023-12-15 09:50:24

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 07:46, Jie Luo wrote:
>>
>>
>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>> Update the yaml file for the new DTS properties.
>>>>>>
>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>
>>>>>> Signed-off-by: Luo Jie <[email protected]>
>>>>>> ---
>>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>>>>>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>> - enum:
>>>>>> - qcom,ipq6018-mdio
>>>>>> - qcom,ipq8074-mdio
>>>>>> + - qcom,ipq9574-mdio
>>>>>> + - qcom,ipq5332-mdio
>>>>>> - const: qcom,ipq4019-mdio
>>>>>> "#address-cells":
>>>>>> @@ -30,19 +32,71 @@ properties:
>>>>>> reg:
>>>>>> minItems: 1
>>>>>> - maxItems: 2
>>>>>> + maxItems: 5
>>>>>> description:
>>>>>> - the first Address and length of the register set for the MDIO controller.
>>>>>> - the second Address and length of the register for ethernet LDO, this second
>>>>>> - address range is only required by the platform IPQ50xx.
>>>>>> + the first Address and length of the register set for the MDIO controller,
>>>>>> + the optional second, third and fourth address and length of the register
>>>>>> + for ethernet LDO, these three address range are required by the platform
>>>>>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>>>>>> + to select the reference clock.
>>>>>> +
>>>>>> + reg-names:
>>>>>> + minItems: 1
>>>>>> + maxItems: 5
>>>>>> clocks:
>>>>>> + minItems: 1
>>>>>> items:
>>>>>> - description: MDIO clock source frequency fixed to 100MHZ
>>>>>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>> clock-names:
>>>>>> + minItems: 1
>>>>>> items:
>>>>>> - const: gcc_mdio_ahb_clk
>>>>>> + - const: gcc_uniphy0_ahb_clk
>>>>>> + - const: gcc_uniphy1_ahb_clk
>>>>>> + - const: gcc_uniphy0_sys_clk
>>>>>> + - const: gcc_uniphy1_sys_clk
>>>>>
>>>>>> + cmn-reference-clock:
>>>>>> + oneOf:
>>>>>> + - items:
>>>>>> + - enum:
>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>
>>>>> Why is this not represented by an element of the clocks property?
>>>>
>>>> This property is for the reference clock source selection of CMN PLL,
>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>> clock framework can't be used, which is the general hardware register
>>>> instead of RCG register for GCC.
>>>
>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>> or by some other clock controller or fixed clock makes a difference.
>>> Why can't the other clock provider be represented in the devicetree?
>>>
>>
>> cmn-reference-clock is for selecting the reference clock source for the
>> whole Ethernet block, which is just the standalone configure register.
>
> Sure, you are aware though that all clocks are just configure registers?
>
> Which clocks are these mentioned in the property? From where do they come?
>
> Anyway, property is in existing form is not correct - this is not a
> generic property.
>

This property cmn-reference-clock is just the hardware register
configuration, since the different IPQ platform needs to select
the different reference clock source for the CMN PLL block that
provides the various clock outputs to the all kinds of Ethernet
devices, which is not from GCC provider.

This is indeed not a generic property, which is the Ethernet
function configs same as clock-frequency.

>
>> however the clock provider has the logical register distribution, such
>> as for one clock tree, there is RCG, DIVIDER and branch registers in
>> the qcom soc chip.
>>
>> The clock consumer defines the clock IDs of device tree to reference the
>> clocks provided by the clock controller, and these clock IDs are
>> provided by the header file of clock provider.
>>
>> like this,
>> clocks = <&gcc GCC_MDIO_AHB_CLK>,
>>
>> <&gcc GCC_UNIPHY0_AHB_CLK>,
>>
>> <&gcc GCC_UNIPHY1_AHB_CLK>,
>>
>> <&gcc GCC_UNIPHY0_SYS_CLK>,
>>
>> <&gcc GCC_UNIPHY1_SYS_CLK>;
>>
>> gcc is the device node of clock provider, and GCC_MDIO_AHB_CLK is the
>> clock ID.
>
>
>
> Best regards,
> Krzysztof
>

2023-12-15 10:19:44

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 15/12/2023 10:49, Jie Luo wrote:
>
>
> On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2023 07:46, Jie Luo wrote:
>>>
>>>
>>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>>
>>>>>
>>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>>> Update the yaml file for the new DTS properties.
>>>>>>>
>>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>>
>>>>>>> Signed-off-by: Luo Jie <[email protected]>
>>>>>>> ---
>>>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>>>>>>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>>> - enum:
>>>>>>> - qcom,ipq6018-mdio
>>>>>>> - qcom,ipq8074-mdio
>>>>>>> + - qcom,ipq9574-mdio
>>>>>>> + - qcom,ipq5332-mdio
>>>>>>> - const: qcom,ipq4019-mdio
>>>>>>> "#address-cells":
>>>>>>> @@ -30,19 +32,71 @@ properties:
>>>>>>> reg:
>>>>>>> minItems: 1
>>>>>>> - maxItems: 2
>>>>>>> + maxItems: 5
>>>>>>> description:
>>>>>>> - the first Address and length of the register set for the MDIO controller.
>>>>>>> - the second Address and length of the register for ethernet LDO, this second
>>>>>>> - address range is only required by the platform IPQ50xx.
>>>>>>> + the first Address and length of the register set for the MDIO controller,
>>>>>>> + the optional second, third and fourth address and length of the register
>>>>>>> + for ethernet LDO, these three address range are required by the platform
>>>>>>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>>>>>>> + to select the reference clock.
>>>>>>> +
>>>>>>> + reg-names:
>>>>>>> + minItems: 1
>>>>>>> + maxItems: 5
>>>>>>> clocks:
>>>>>>> + minItems: 1
>>>>>>> items:
>>>>>>> - description: MDIO clock source frequency fixed to 100MHZ
>>>>>>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>>> clock-names:
>>>>>>> + minItems: 1
>>>>>>> items:
>>>>>>> - const: gcc_mdio_ahb_clk
>>>>>>> + - const: gcc_uniphy0_ahb_clk
>>>>>>> + - const: gcc_uniphy1_ahb_clk
>>>>>>> + - const: gcc_uniphy0_sys_clk
>>>>>>> + - const: gcc_uniphy1_sys_clk
>>>>>>
>>>>>>> + cmn-reference-clock:
>>>>>>> + oneOf:
>>>>>>> + - items:
>>>>>>> + - enum:
>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>
>>>>>> Why is this not represented by an element of the clocks property?
>>>>>
>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>> clock framework can't be used, which is the general hardware register
>>>>> instead of RCG register for GCC.
>>>>
>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>> or by some other clock controller or fixed clock makes a difference.
>>>> Why can't the other clock provider be represented in the devicetree?
>>>>
>>>
>>> cmn-reference-clock is for selecting the reference clock source for the
>>> whole Ethernet block, which is just the standalone configure register.
>>
>> Sure, you are aware though that all clocks are just configure registers?
>>
>> Which clocks are these mentioned in the property? From where do they come?
>>
>> Anyway, property is in existing form is not correct - this is not a
>> generic property.
>>
>
> This property cmn-reference-clock is just the hardware register
> configuration, since the different IPQ platform needs to select
> the different reference clock source for the CMN PLL block that
> provides the various clock outputs to the all kinds of Ethernet
> devices, which is not from GCC provider.

AGAIN: where do the clocks come from? Which device generates them?

>
> This is indeed not a generic property, which is the Ethernet
> function configs same as clock-frequency.

Then it should not be made as a generic property...

Best regards,
Krzysztof

2023-12-15 10:34:15

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 6:19 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 10:49, Jie Luo wrote:
>>
>>
>> On 12/15/2023 3:29 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 07:46, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/15/2023 1:12 AM, Conor Dooley wrote:
>>>>> On Wed, Dec 13, 2023 at 04:26:56PM +0800, Jie Luo wrote:
>>>>>>
>>>>>>
>>>>>> On 12/13/2023 12:06 AM, Conor Dooley wrote:
>>>>>>> On Tue, Dec 12, 2023 at 07:51:50PM +0800, Luo Jie wrote:
>>>>>>>> Update the yaml file for the new DTS properties.
>>>>>>>>
>>>>>>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>>>>>>> 2. clock-frequency for MDIO clock frequency config.
>>>>>>>> 3. add uniphy AHB & SYS GCC clocks.
>>>>>>>> 4. add reset-gpios for MDIO bus level reset.
>>>>>>>>
>>>>>>>> Signed-off-by: Luo Jie <[email protected]>
>>>>>>>> ---
>>>>>>>> .../bindings/net/qcom,ipq4019-mdio.yaml | 157 +++++++++++++++++-
>>>>>>>> 1 file changed, 153 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> index 3407e909e8a7..9546a6ad7841 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>>>>>>> @@ -20,6 +20,8 @@ properties:
>>>>>>>> - enum:
>>>>>>>> - qcom,ipq6018-mdio
>>>>>>>> - qcom,ipq8074-mdio
>>>>>>>> + - qcom,ipq9574-mdio
>>>>>>>> + - qcom,ipq5332-mdio
>>>>>>>> - const: qcom,ipq4019-mdio
>>>>>>>> "#address-cells":
>>>>>>>> @@ -30,19 +32,71 @@ properties:
>>>>>>>> reg:
>>>>>>>> minItems: 1
>>>>>>>> - maxItems: 2
>>>>>>>> + maxItems: 5
>>>>>>>> description:
>>>>>>>> - the first Address and length of the register set for the MDIO controller.
>>>>>>>> - the second Address and length of the register for ethernet LDO, this second
>>>>>>>> - address range is only required by the platform IPQ50xx.
>>>>>>>> + the first Address and length of the register set for the MDIO controller,
>>>>>>>> + the optional second, third and fourth address and length of the register
>>>>>>>> + for ethernet LDO, these three address range are required by the platform
>>>>>>>> + IPQ50xx/IPQ5332/IPQ9574, the last address and length is for the CMN clock
>>>>>>>> + to select the reference clock.
>>>>>>>> +
>>>>>>>> + reg-names:
>>>>>>>> + minItems: 1
>>>>>>>> + maxItems: 5
>>>>>>>> clocks:
>>>>>>>> + minItems: 1
>>>>>>>> items:
>>>>>>>> - description: MDIO clock source frequency fixed to 100MHZ
>>>>>>>> + - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>>>>>>> + - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>>>>>>> + - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>>>>>>> + - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>>>>>> clock-names:
>>>>>>>> + minItems: 1
>>>>>>>> items:
>>>>>>>> - const: gcc_mdio_ahb_clk
>>>>>>>> + - const: gcc_uniphy0_ahb_clk
>>>>>>>> + - const: gcc_uniphy1_ahb_clk
>>>>>>>> + - const: gcc_uniphy0_sys_clk
>>>>>>>> + - const: gcc_uniphy1_sys_clk
>>>>>>>
>>>>>>>> + cmn-reference-clock:
>>>>>>>> + oneOf:
>>>>>>>> + - items:
>>>>>>>> + - enum:
>>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>>
>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>
>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>> clock framework can't be used, which is the general hardware register
>>>>>> instead of RCG register for GCC.
>>>>>
>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>
>>>>
>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>> whole Ethernet block, which is just the standalone configure register.
>>>
>>> Sure, you are aware though that all clocks are just configure registers?
>>>
>>> Which clocks are these mentioned in the property? From where do they come?
>>>
>>> Anyway, property is in existing form is not correct - this is not a
>>> generic property.
>>>
>>
>> This property cmn-reference-clock is just the hardware register
>> configuration, since the different IPQ platform needs to select
>> the different reference clock source for the CMN PLL block that
>> provides the various clock outputs to the all kinds of Ethernet
>> devices, which is not from GCC provider.
>
> AGAIN: where do the clocks come from? Which device generates them?

Oh, OK, the reference clock is from wifi that provides 48MHZ to
Ethernet block.

>
>>
>> This is indeed not a generic property, which is the Ethernet
>> function configs same as clock-frequency.
>
>
> Then it should not be made as a generic property...

sure, i saw your another comments, will prefix qcom_ on this property.

>
>
>
> Best regards,
> Krzysztof
>

2023-12-15 10:38:03

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>> + cmn-reference-clock:
>>>>>>>>> + oneOf:
>>>>>>>>> + - items:
>>>>>>>>> + - enum:
>>>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>>>
>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>
>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>> instead of RCG register for GCC.
>>>>>>
>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>
>>>>>
>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>
>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>
>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>
>>>> Anyway, property is in existing form is not correct - this is not a
>>>> generic property.
>>>>
>>>
>>> This property cmn-reference-clock is just the hardware register
>>> configuration, since the different IPQ platform needs to select
>>> the different reference clock source for the CMN PLL block that
>>> provides the various clock outputs to the all kinds of Ethernet
>>> devices, which is not from GCC provider.
>>
>> AGAIN: where do the clocks come from? Which device generates them?
>
> Oh, OK, the reference clock is from wifi that provides 48MHZ to
> Ethernet block.

Then WiFi should be providing you the clock and this device should be
clock consumer, right?

Best regards,
Krzysztof

2023-12-15 10:41:40

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>> + cmn-reference-clock:
>>>>>>>>>> + oneOf:
>>>>>>>>>> + - items:
>>>>>>>>>> + - enum:
>>>>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>>>>
>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>
>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>> instead of RCG register for GCC.
>>>>>>>
>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>
>>>>>>
>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>
>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>
>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>
>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>> generic property.
>>>>>
>>>>
>>>> This property cmn-reference-clock is just the hardware register
>>>> configuration, since the different IPQ platform needs to select
>>>> the different reference clock source for the CMN PLL block that
>>>> provides the various clock outputs to the all kinds of Ethernet
>>>> devices, which is not from GCC provider.
>>>
>>> AGAIN: where do the clocks come from? Which device generates them?
>>
>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>> Ethernet block.
>
> Then WiFi should be providing you the clock and this device should be
> clock consumer, right?

Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
for this 48MHZ clock output, it is the hardware PIN connection.
>
>
>
> Best regards,
> Krzysztof
>

2023-12-15 10:42:48

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

> > > This is indeed not a generic property, which is the Ethernet
> > > function configs same as clock-frequency.
> >
> >
> > Then it should not be made as a generic property...
>
> sure, i saw your another comments, will prefix qcom_ on this property.

iff the property stays, that would be a prefix of "qcom," not "qcom_".

Cheers,
Conor.

Attachments:

(No filename) (357.00 B)
signature.asc (235.00 B)
Download all attachments

2023-12-15 10:53:47

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 15/12/2023 11:40, Jie Luo wrote:
>
>
> On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>>> + cmn-reference-clock:
>>>>>>>>>>> + oneOf:
>>>>>>>>>>> + - items:
>>>>>>>>>>> + - enum:
>>>>>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>>>>>
>>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>>
>>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>>> instead of RCG register for GCC.
>>>>>>>>
>>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>>
>>>>>>>
>>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>>
>>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>>
>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>
>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>> generic property.
>>>>>>
>>>>>
>>>>> This property cmn-reference-clock is just the hardware register
>>>>> configuration, since the different IPQ platform needs to select
>>>>> the different reference clock source for the CMN PLL block that
>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>> devices, which is not from GCC provider.
>>>>
>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>
>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>> Ethernet block.
>>
>> Then WiFi should be providing you the clock and this device should be
>> clock consumer, right?
>
> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> for this 48MHZ clock output, it is the hardware PIN connection.

All clocks are some hardware pin connections.

Best regards,
Krzysztof

2023-12-15 11:42:58

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 6:53 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 11:40, Jie Luo wrote:
>>
>>
>> On 12/15/2023 6:37 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 11:33, Jie Luo wrote:
>>>>>>>>>>>> + cmn-reference-clock:
>>>>>>>>>>>> + oneOf:
>>>>>>>>>>>> + - items:
>>>>>>>>>>>> + - enum:
>>>>>>>>>>>> + - 0 # CMN PLL reference internal 48MHZ
>>>>>>>>>>>> + - 1 # CMN PLL reference external 25MHZ
>>>>>>>>>>>> + - 2 # CMN PLL reference external 31250KHZ
>>>>>>>>>>>> + - 3 # CMN PLL reference external 40MHZ
>>>>>>>>>>>> + - 4 # CMN PLL reference external 48MHZ
>>>>>>>>>>>> + - 5 # CMN PLL reference external 50MHZ
>>>>>>>>>>>> + - 6 # CMN PLL reference internal 96MHZ
>>>>>>>>>>>
>>>>>>>>>>> Why is this not represented by an element of the clocks property?
>>>>>>>>>>
>>>>>>>>>> This property is for the reference clock source selection of CMN PLL,
>>>>>>>>>> CMN PLL generates the different clock rates for the different Ethernet
>>>>>>>>>> blocks, this CMN PLL configuration is not located in the GCC, so the
>>>>>>>>>> clock framework can't be used, which is the general hardware register
>>>>>>>>>> instead of RCG register for GCC.
>>>>>>>>>
>>>>>>>>> I don't see how the clock being provided by the "GCC" (whatever that is)
>>>>>>>>> or by some other clock controller or fixed clock makes a difference.
>>>>>>>>> Why can't the other clock provider be represented in the devicetree?
>>>>>>>>>
>>>>>>>>
>>>>>>>> cmn-reference-clock is for selecting the reference clock source for the
>>>>>>>> whole Ethernet block, which is just the standalone configure register.
>>>>>>>
>>>>>>> Sure, you are aware though that all clocks are just configure registers?
>>>>>>>
>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>
>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>> generic property.
>>>>>>>
>>>>>>
>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>> configuration, since the different IPQ platform needs to select
>>>>>> the different reference clock source for the CMN PLL block that
>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>> devices, which is not from GCC provider.
>>>>>
>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>
>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>> Ethernet block.
>>>
>>> Then WiFi should be providing you the clock and this device should be
>>> clock consumer, right?
>>
>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>> for this 48MHZ clock output, it is the hardware PIN connection.
>
> All clocks are some hardware pin connections.
>
> Best regards,
> Krzysztof
>

Yes, all reference clocks here are from hardware pin connection.

2023-12-15 11:43:57

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 6:42 PM, Conor Dooley wrote:
>>>> This is indeed not a generic property, which is the Ethernet
>>>> function configs same as clock-frequency.
>>>
>>>
>>> Then it should not be made as a generic property...
>>
>> sure, i saw your another comments, will prefix qcom_ on this property.
>
> iff the property stays, that would be a prefix of "qcom," not "qcom_".
>
> Cheers,
> Conor.

Ok, thanks.

2023-12-15 12:19:27

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>
>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>> generic property.
>>>>>>>>
>>>>>>>
>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>> devices, which is not from GCC provider.
>>>>>>
>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>
>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>> Ethernet block.
>>>>
>>>> Then WiFi should be providing you the clock and this device should be
>>>> clock consumer, right?
>>>
>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>
>> All clocks are some hardware pin connections.
>>
>> Best regards,
>> Krzysztof
>>
>
> Yes, all reference clocks here are from hardware pin connection.

You keep answering with short sentences without touching the root of the
problem. I don't know exactly why, but I feel this discussion leads
nowhere. After long discussion you finally admitted that clocks came
from another device - Wifi. It took us like 6 emails?

So last statement: if you have clock provider and clock consumer, you
must represent it in the bindings or provide rationale why it should not
or must not be represented in the bindings. So far I do not see any of
such arguments.

If you use arguments like:
"My driver....": sorry, bindings are not about drivers
"I don't have clock driver for WiFi": sorry, it does not matter if you
can write one, right?

Please reach internally your colleagues to solve these problems and make
review process smoother.

Best regards,
Krzysztof

2023-12-15 12:41:10

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>
>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>> generic property.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>> devices, which is not from GCC provider.
>>>>>>>
>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>
>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>> Ethernet block.
>>>>>
>>>>> Then WiFi should be providing you the clock and this device should be
>>>>> clock consumer, right?
>>>>
>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>
>>> All clocks are some hardware pin connections.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Yes, all reference clocks here are from hardware pin connection.
>
> You keep answering with short sentences without touching the root of the
> problem. I don't know exactly why, but I feel this discussion leads
> nowhere. After long discussion you finally admitted that clocks came
> from another device - Wifi. It took us like 6 emails?
>
> So last statement: if you have clock provider and clock consumer, you
> must represent it in the bindings or provide rationale why it should not
> or must not be represented in the bindings. So far I do not see any of
> such arguments.
>
> If you use arguments like:
> "My driver....": sorry, bindings are not about drivers
> "I don't have clock driver for WiFi": sorry, it does not matter if you
> can write one, right?
>
> Please reach internally your colleagues to solve these problems and make
> review process smoother.
>
> Best regards,
> Krzysztof
>
>

These reference clocks source do not need the hardware configuration,
that is the reason why the clock provider is not needed, some reference
clock source are even from external crystal.

There is also no enable control for the reference clocks since it is
inputted by the hardware PIN connection, i will update these description
in the DT to make it more clear.

2023-12-15 13:39:58

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

> > You keep answering with short sentences without touching the root of the
> > problem. I don't know exactly why, but I feel this discussion leads
> > nowhere. After long discussion you finally admitted that clocks came
> > from another device - Wifi. It took us like 6 emails?
> >
> > So last statement: if you have clock provider and clock consumer, you
> > must represent it in the bindings or provide rationale why it should not
> > or must not be represented in the bindings. So far I do not see any of
> > such arguments.
> >
> > If you use arguments like:
> > "My driver....": sorry, bindings are not about drivers
> > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > can write one, right?
> >
> > Please reach internally your colleagues to solve these problems and make
> > review process smoother.

Yes, i strongly agree with this. Its not our job as maintainers to
educate big companies like Qualcomm how to write Linux drivers. There
are more experienced driver writer within Qualcomm, you need to make
contact with them, and get them to help you. Or you need to outsource
the driver development to one of the companies which write mainline
Linux drivers.

Andrew

2023-12-15 13:43:54

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>
>
> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > On 15/12/2023 12:42, Jie Luo wrote:
> > > > > > > > > > Which clocks are these mentioned in the property? From where do they come?
> > > > > > > > > >
> > > > > > > > > > Anyway, property is in existing form is not correct - this is not a
> > > > > > > > > > generic property.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > This property cmn-reference-clock is just the hardware register
> > > > > > > > > configuration, since the different IPQ platform needs to select
> > > > > > > > > the different reference clock source for the CMN PLL block that
> > > > > > > > > provides the various clock outputs to the all kinds of Ethernet
> > > > > > > > > devices, which is not from GCC provider.
> > > > > > > >
> > > > > > > > AGAIN: where do the clocks come from? Which device generates them?
> > > > > > >
> > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to
> > > > > > > Ethernet block.
> > > > > >
> > > > > > Then WiFi should be providing you the clock and this device should be
> > > > > > clock consumer, right?
> > > > >
> > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> > > > > for this 48MHZ clock output, it is the hardware PIN connection.
> > > >
> > > > All clocks are some hardware pin connections.
> > > >
> > > > Best regards,
> > > > Krzysztof
> > > >
> > >
> > > Yes, all reference clocks here are from hardware pin connection.
> >
> > You keep answering with short sentences without touching the root of the
> > problem. I don't know exactly why, but I feel this discussion leads
> > nowhere. After long discussion you finally admitted that clocks came
> > from another device - Wifi. It took us like 6 emails?
> >
> > So last statement: if you have clock provider and clock consumer, you
> > must represent it in the bindings or provide rationale why it should not
> > or must not be represented in the bindings. So far I do not see any of
> > such arguments.
> >
> > If you use arguments like:
> > "My driver....": sorry, bindings are not about drivers
> > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > can write one, right?
> >
> > Please reach internally your colleagues to solve these problems and make
> > review process smoother.

> These reference clocks source do not need the hardware configuration,
> that is the reason why the clock provider is not needed, some reference
> clock source are even from external crystal.

I fail to understand how that makes this clock different to the clocks
on any other platform. Clocks from external crystals are present in many
many systems. See for example fixed-clock.yaml.

> There is also no enable control for the reference clocks since it is
> inputted by the hardware PIN connection, i will update these description
> in the DT to make it more clear.

Again, this does not justify having custom properties for this clock,
as it is no different to other platforms. As far as I can tell, the only
thing that a standard "clocks" property cannot convey here is the
internal reference. I would suggest that since there is only one
internal clock frequency, the absence of this particular clock in the
"clocks" property can be used to determine that the reference is the
internal one.

Thanks,
Conor.

Attachments:

(No filename) (3.39 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-16 13:17:58

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 9:41 PM, Conor Dooley wrote:
> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>>
>>
>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
>>> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>>>
>>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>>>> generic property.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>>>> devices, which is not from GCC provider.
>>>>>>>>>
>>>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>>>
>>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>>>> Ethernet block.
>>>>>>>
>>>>>>> Then WiFi should be providing you the clock and this device should be
>>>>>>> clock consumer, right?
>>>>>>
>>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>>>
>>>>> All clocks are some hardware pin connections.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Yes, all reference clocks here are from hardware pin connection.
>>>
>>> You keep answering with short sentences without touching the root of the
>>> problem. I don't know exactly why, but I feel this discussion leads
>>> nowhere. After long discussion you finally admitted that clocks came
>>> from another device - Wifi. It took us like 6 emails?
>>>
>>> So last statement: if you have clock provider and clock consumer, you
>>> must represent it in the bindings or provide rationale why it should not
>>> or must not be represented in the bindings. So far I do not see any of
>>> such arguments.
>>>
>>> If you use arguments like:
>>> "My driver....": sorry, bindings are not about drivers
>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>> can write one, right?
>>>
>>> Please reach internally your colleagues to solve these problems and make
>>> review process smoother.
>
>> These reference clocks source do not need the hardware configuration,
>> that is the reason why the clock provider is not needed, some reference
>> clock source are even from external crystal.
>
> I fail to understand how that makes this clock different to the clocks
> on any other platform. Clocks from external crystals are present in many
> many systems. See for example fixed-clock.yaml.

Hi Conor,
The reference clock rate has no meaning to the CMN PLL block, since the
software can't control the behavior of CMN PLL, and various output
clocks of CMN PLL block are fixed, adding this custom property is just
for selecting the different reference clock source, since different
IPQ platform needs to be configured the different reference clock source
for the CMN PLL block.

let's say if we register 48MHZ reference clock as the fix clock, we
can't distinguish it is internal 48MHZ or external 48MHZ, for these
two reference clock sources, there are different hardware configuration
of CMN PLL block, and this reference clock selection is not applicable
for the IPQ4019 platform.

>
>> There is also no enable control for the reference clocks since it is
>> inputted by the hardware PIN connection, i will update these description
>> in the DT to make it more clear.
>
> Again, this does not justify having custom properties for this clock,
> as it is no different to other platforms. As far as I can tell, the only
> thing that a standard "clocks" property cannot convey here is the
> internal reference. I would suggest that since there is only one
> internal clock frequency, the absence of this particular clock in the
> "clocks" property can be used to determine that the reference is the
> internal one.
>
> Thanks,
> Conor.

Yes, we can get the clock rate of the clocks property if we register
these as the fix clock to distinguish the different clock source.

Since the reference clock rate value has no matter with the CMN clock
configuration, it is just the reference clock source selection, so
i did not use the fix clock for this.

Thanks for this suggestion, i will verify the fix clock register solution.

2023-12-16 13:31:46

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/15/2023 9:39 PM, Andrew Lunn wrote:
>>> You keep answering with short sentences without touching the root of the
>>> problem. I don't know exactly why, but I feel this discussion leads
>>> nowhere. After long discussion you finally admitted that clocks came
>>> from another device - Wifi. It took us like 6 emails?
>>>
>>> So last statement: if you have clock provider and clock consumer, you
>>> must represent it in the bindings or provide rationale why it should not
>>> or must not be represented in the bindings. So far I do not see any of
>>> such arguments.
>>>
>>> If you use arguments like:
>>> "My driver....": sorry, bindings are not about drivers
>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>> can write one, right?
>>>
>>> Please reach internally your colleagues to solve these problems and make
>>> review process smoother.
>
> Yes, i strongly agree with this. Its not our job as maintainers to
> educate big companies like Qualcomm how to write Linux drivers. There
> are more experienced driver writer within Qualcomm, you need to make
> contact with them, and get them to help you. Or you need to outsource
> the driver development to one of the companies which write mainline
> Linux drivers.
>
> Andrew

Understand this, sorry for some easy mistakes happens here.
i will sync with internal team before pushing the next patch set.

Thanks a lot.

2023-12-16 14:17:08

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
>
>
> On 12/15/2023 9:41 PM, Conor Dooley wrote:
> > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
> > >
> > >
> > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > > > On 15/12/2023 12:42, Jie Luo wrote:
> > > > > > > > > > > > Which clocks are these mentioned in the property? From where do they come?
> > > > > > > > > > > >
> > > > > > > > > > > > Anyway, property is in existing form is not correct - this is not a
> > > > > > > > > > > > generic property.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > This property cmn-reference-clock is just the hardware register
> > > > > > > > > > > configuration, since the different IPQ platform needs to select
> > > > > > > > > > > the different reference clock source for the CMN PLL block that
> > > > > > > > > > > provides the various clock outputs to the all kinds of Ethernet
> > > > > > > > > > > devices, which is not from GCC provider.
> > > > > > > > > >
> > > > > > > > > > AGAIN: where do the clocks come from? Which device generates them?
> > > > > > > > >
> > > > > > > > > Oh, OK, the reference clock is from wifi that provides 48MHZ to
> > > > > > > > > Ethernet block.
> > > > > > > >
> > > > > > > > Then WiFi should be providing you the clock and this device should be
> > > > > > > > clock consumer, right?
> > > > > > >
> > > > > > > Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
> > > > > > > for this 48MHZ clock output, it is the hardware PIN connection.
> > > > > >
> > > > > > All clocks are some hardware pin connections.
> > > > > >
> > > > > > Best regards,
> > > > > > Krzysztof
> > > > > >
> > > > >
> > > > > Yes, all reference clocks here are from hardware pin connection.
> > > >
> > > > You keep answering with short sentences without touching the root of the
> > > > problem. I don't know exactly why, but I feel this discussion leads
> > > > nowhere. After long discussion you finally admitted that clocks came
> > > > from another device - Wifi. It took us like 6 emails?
> > > >
> > > > So last statement: if you have clock provider and clock consumer, you
> > > > must represent it in the bindings or provide rationale why it should not
> > > > or must not be represented in the bindings. So far I do not see any of
> > > > such arguments.
> > > >
> > > > If you use arguments like:
> > > > "My driver....": sorry, bindings are not about drivers
> > > > "I don't have clock driver for WiFi": sorry, it does not matter if you
> > > > can write one, right?
> > > >
> > > > Please reach internally your colleagues to solve these problems and make
> > > > review process smoother.
> >
> > > These reference clocks source do not need the hardware configuration,
> > > that is the reason why the clock provider is not needed, some reference
> > > clock source are even from external crystal.
> >
> > I fail to understand how that makes this clock different to the clocks
> > on any other platform. Clocks from external crystals are present in many
> > many systems. See for example fixed-clock.yaml.
>
> The reference clock rate has no meaning to the CMN PLL block, since the
> software can't control the behavior of CMN PLL, and various output
> clocks of CMN PLL block are fixed, adding this custom property is just
> for selecting the different reference clock source, since different
> IPQ platform needs to be configured the different reference clock source
> for the CMN PLL block.

Many, many other systems are in the same situation, where clocks are
provided to a peripheral that has no control over the clock rate, but
has to pick internal dividers or set bits in a config register depending
on what clock rate is provided to it. That is not something special
about this particular platform and other systems are able to use the
clocks property for this purpose.

> let's say if we register 48MHZ reference clock as the fix clock, we
> can't distinguish it is internal 48MHZ or external 48MHZ, for these
> two reference clock sources, there are different hardware configuration
> of CMN PLL block

That's easy, if the reference is external, it is provided by the clocks
property. If it internal, then there will be no clocks property
providing it.

> and this reference clock selection is not applicable
> for the IPQ4019 platform.

Isn't this a patch for the IPQ4019? Why would it not be relevant?

> > > There is also no enable control for the reference clocks since it is
> > > inputted by the hardware PIN connection, i will update these description
> > > in the DT to make it more clear.
> >
> > Again, this does not justify having custom properties for this clock,
> > as it is no different to other platforms. As far as I can tell, the only
> > thing that a standard "clocks" property cannot convey here is the
> > internal reference. I would suggest that since there is only one
> > internal clock frequency, the absence of this particular clock in the
> > "clocks" property can be used to determine that the reference is the
> > internal one.

I'm surprised you didn't pick up on this, but there are actually _2_
internal references, which I have just noticed while double checking the
binding patch.

What is the impact of using the 48 MHz or 96 MHz internal reference?

Thanks,
Conor.

> Yes, we can get the clock rate of the clocks property if we register
> these as the fix clock to distinguish the different clock source.
>
> Since the reference clock rate value has no matter with the CMN clock
> configuration, it is just the reference clock source selection, so
> i did not use the fix clock for this.
>
> Thanks for this suggestion, i will verify the fix clock register solution.

Attachments:

(No filename) (5.71 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-16 15:37:53

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/16/2023 10:16 PM, Conor Dooley wrote:
> On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
>>
>>
>> On 12/15/2023 9:41 PM, Conor Dooley wrote:
>>> On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
>>>>
>>>>
>>>> On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
>>>>> On 15/12/2023 12:42, Jie Luo wrote:
>>>>>>>>>>>>> Which clocks are these mentioned in the property? From where do they come?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Anyway, property is in existing form is not correct - this is not a
>>>>>>>>>>>>> generic property.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This property cmn-reference-clock is just the hardware register
>>>>>>>>>>>> configuration, since the different IPQ platform needs to select
>>>>>>>>>>>> the different reference clock source for the CMN PLL block that
>>>>>>>>>>>> provides the various clock outputs to the all kinds of Ethernet
>>>>>>>>>>>> devices, which is not from GCC provider.
>>>>>>>>>>>
>>>>>>>>>>> AGAIN: where do the clocks come from? Which device generates them?
>>>>>>>>>>
>>>>>>>>>> Oh, OK, the reference clock is from wifi that provides 48MHZ to
>>>>>>>>>> Ethernet block.
>>>>>>>>>
>>>>>>>>> Then WiFi should be providing you the clock and this device should be
>>>>>>>>> clock consumer, right?
>>>>>>>>
>>>>>>>> Yes, wifi provides 48MHz clock to CMM PLL block, there is no GCC
>>>>>>>> for this 48MHZ clock output, it is the hardware PIN connection.
>>>>>>>
>>>>>>> All clocks are some hardware pin connections.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>>>
>>>>>>
>>>>>> Yes, all reference clocks here are from hardware pin connection.
>>>>>
>>>>> You keep answering with short sentences without touching the root of the
>>>>> problem. I don't know exactly why, but I feel this discussion leads
>>>>> nowhere. After long discussion you finally admitted that clocks came
>>>>> from another device - Wifi. It took us like 6 emails?
>>>>>
>>>>> So last statement: if you have clock provider and clock consumer, you
>>>>> must represent it in the bindings or provide rationale why it should not
>>>>> or must not be represented in the bindings. So far I do not see any of
>>>>> such arguments.
>>>>>
>>>>> If you use arguments like:
>>>>> "My driver....": sorry, bindings are not about drivers
>>>>> "I don't have clock driver for WiFi": sorry, it does not matter if you
>>>>> can write one, right?
>>>>>
>>>>> Please reach internally your colleagues to solve these problems and make
>>>>> review process smoother.
>>>
>>>> These reference clocks source do not need the hardware configuration,
>>>> that is the reason why the clock provider is not needed, some reference
>>>> clock source are even from external crystal.
>>>
>>> I fail to understand how that makes this clock different to the clocks
>>> on any other platform. Clocks from external crystals are present in many
>>> many systems. See for example fixed-clock.yaml.
>>
>> The reference clock rate has no meaning to the CMN PLL block, since the
>> software can't control the behavior of CMN PLL, and various output
>> clocks of CMN PLL block are fixed, adding this custom property is just
>> for selecting the different reference clock source, since different
>> IPQ platform needs to be configured the different reference clock source
>> for the CMN PLL block.
>
> Many, many other systems are in the same situation, where clocks are
> provided to a peripheral that has no control over the clock rate, but
> has to pick internal dividers or set bits in a config register depending
> on what clock rate is provided to it. That is not something special
> about this particular platform and other systems are able to use the
> clocks property for this purpose.

Sure, Thanks Conor for this information.
i will try to replace this custom property with clocks property and
verify the drive.

>
>> let's say if we register 48MHZ reference clock as the fix clock, we
>> can't distinguish it is internal 48MHZ or external 48MHZ, for these
>> two reference clock sources, there are different hardware configuration
>> of CMN PLL block
>
> That's easy, if the reference is external, it is provided by the clocks
> property. If it internal, then there will be no clocks property
> providing it.

Thanks for this detail.

>
>> and this reference clock selection is not applicable
>> for the IPQ4019 platform.
>
> Isn't this a patch for the IPQ4019? Why would it not be relevant?
IPQ4019 is the legacy chip, and the same MDIO bus driver is also
extended to support the new IPQ platform, since the MDIO hardware
is leveraged by the new IPQ platform.

For the CMN PLL block, which is not existed on the legacy platform
IPQ4019, but it does not matter, we can also distinguish it according
to the CMN register base defined or not, the CMN reference clocks is
configured only when the CMN register base defined in the reg property.

>
>>>> There is also no enable control for the reference clocks since it is
>>>> inputted by the hardware PIN connection, i will update these description
>>>> in the DT to make it more clear.
>>>
>>> Again, this does not justify having custom properties for this clock,
>>> as it is no different to other platforms. As far as I can tell, the only
>>> thing that a standard "clocks" property cannot convey here is the
>>> internal reference. I would suggest that since there is only one
>>> internal clock frequency, the absence of this particular clock in the
>>> "clocks" property can be used to determine that the reference is the
>>> internal on
>
> I'm surprised you didn't pick up on this, but there are actually _2_
> internal references, which I have just noticed while double checking the
> binding patch.

i noticed this, the reference clock source can be supported by clocks as
you suggested here, it is really helpful.
>
> What is the impact of using the 48 MHz or 96 MHz internal reference?
They works on the different IPQ platform, 96MHZ internal reference is
used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
same as what you describe above, the different clock source rate is
selected as the different register value, then the PLL can do the
corresponding config to output the correct clock rate, the external
clock source is also same if the clock rate is same, just the different
hardware PIN is selected if the external reference source is configured.

Thanks.

>
> Thanks,
> Conor.
>
>> Yes, we can get the clock rate of the clocks property if we register
>> these as the fix clock to distinguish the different clock source.
>>
>> Since the reference clock rate value has no matter with the CMN clock
>> configuration, it is just the reference clock source selection, so
>> i did not use the fix clock for this.
>>
>> Thanks for this suggestion, i will verify the fix clock register solution.

2023-12-19 15:47:54

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On Sat, Dec 16, 2023 at 11:37:08PM +0800, Jie Luo wrote:
> On 12/16/2023 10:16 PM, Conor Dooley wrote:
> > On Sat, Dec 16, 2023 at 09:16:49PM +0800, Jie Luo wrote:
> > > On 12/15/2023 9:41 PM, Conor Dooley wrote:
> > > > On Fri, Dec 15, 2023 at 08:40:20PM +0800, Jie Luo wrote:
> > > > > On 12/15/2023 8:19 PM, Krzysztof Kozlowski wrote:
> > > > > > On 15/12/2023 12:42, Jie Luo wrote:

> > > > > There is also no enable control for the reference clocks since it is
> > > > > inputted by the hardware PIN connection, i will update these description
> > > > > in the DT to make it more clear.
> > > >
> > > > Again, this does not justify having custom properties for this clock,
> > > > as it is no different to other platforms. As far as I can tell, the only
> > > > thing that a standard "clocks" property cannot convey here is the
> > > > internal reference. I would suggest that since there is only one
> > > > internal clock frequency, the absence of this particular clock in the
> > > > "clocks" property can be used to determine that the reference is the
> > > > internal on
> >
> > I'm surprised you didn't pick up on this, but there are actually _2_
> > internal references, which I have just noticed while double checking the
> > binding patch.
>
> i noticed this, the reference clock source can be supported by clocks as
> you suggested here, it is really helpful.
>
> > What is the impact of using the 48 MHz or 96 MHz internal reference?
> They works on the different IPQ platform, 96MHZ internal reference is
> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
> same as what you describe above, the different clock source rate is
> selected as the different register value, then the PLL can do the
> corresponding config to output the correct clock rate, the external
> clock source is also same if the clock rate is same, just the different
> hardware PIN is selected if the external reference source is configured.

Ah, so there is only one internal reference frequency per device. Then
my suggestion to use the presence of the clock in the clocks property
should work, just the fallback to the internal reference is going to
depend on the compatible.

Thanks,
Conor.

Attachments:

(No filename) (2.20 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-20 07:33:03

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 16/12/2023 16:37, Jie Luo wrote:
>>
>> I'm surprised you didn't pick up on this, but there are actually _2_
>> internal references, which I have just noticed while double checking the
>> binding patch.
>
> i noticed this, the reference clock source can be supported by clocks as
> you suggested here, it is really helpful.
>>
>> What is the impact of using the 48 MHz or 96 MHz internal reference?
> They works on the different IPQ platform, 96MHZ internal reference is
> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is

So the binding is just incorrect. Why do you even consider configuring
96 MHz internal reference on IPQ5332?

Best regards,
Krzysztof

2023-12-20 10:17:00

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

On 12/20/2023 3:28 PM, Krzysztof Kozlowski wrote:
> On 16/12/2023 16:37, Jie Luo wrote:
>>>
>>> I'm surprised you didn't pick up on this, but there are actually _2_
>>> internal references, which I have just noticed while double checking the
>>> binding patch.
>>
>> i noticed this, the reference clock source can be supported by clocks as
>> you suggested here, it is really helpful.
>>>
>>> What is the impact of using the 48 MHz or 96 MHz internal reference?
>> They works on the different IPQ platform, 96MHZ internal reference is
>> used on IPQ5018, the internal 48MHZ is used on the IPQ5332, that is
>
> So the binding is just incorrect. Why do you even consider configuring
> 96 MHz internal reference on IPQ5332?
>
>
> Best regards,
> Krzysztof
>

Normally there is the fix reference clock source used per each IPQ
platform, but we should keep it configurable, the CMN PLL block is
same among the supported IPQ platforms, Maybe there is special case
to switch the reference clock in the future.
i will also update the dtbinding doc to limit the reference clock based
on the compatible string.

2023-12-20 10:17:50