| Message ID | 20231115032515.4249-1-quic_luoj@quicinc.com |
|---|---|
| Headers | show |
| Series | add MDIO changes on ipq5332 platform | expand |
On 11/16/2023 7:57 PM, Krzysztof Kozlowski wrote: > On 15/11/2023 04:25, Luo Jie wrote: >> There are two PCS(UNIPHY) supported in SOC side on ipq5332, >> and three PCS(UNIPHY) supported on ipq9574. >> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> >> --- >> drivers/net/mdio/mdio-ipq4019.c | 55 +++++++++++++++++++-------------- >> 1 file changed, 32 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c >> index abd8b508ec16..9d444f5f7efb 100644 >> --- a/drivers/net/mdio/mdio-ipq4019.c >> +++ b/drivers/net/mdio/mdio-ipq4019.c >> @@ -18,28 +18,31 @@ >> #define MDIO_DATA_WRITE_REG 0x48 >> #define MDIO_DATA_READ_REG 0x4c >> #define MDIO_CMD_REG 0x50 >> -#define MDIO_CMD_ACCESS_BUSY BIT(16) >> -#define MDIO_CMD_ACCESS_START BIT(8) >> -#define MDIO_CMD_ACCESS_CODE_READ 0 >> -#define MDIO_CMD_ACCESS_CODE_WRITE 1 >> -#define MDIO_CMD_ACCESS_CODE_C45_ADDR 0 >> -#define MDIO_CMD_ACCESS_CODE_C45_WRITE 1 >> -#define MDIO_CMD_ACCESS_CODE_C45_READ 2 >> +#define MDIO_CMD_ACCESS_BUSY BIT(16) >> +#define MDIO_CMD_ACCESS_START BIT(8) >> +#define MDIO_CMD_ACCESS_CODE_READ 0 >> +#define MDIO_CMD_ACCESS_CODE_WRITE 1 >> +#define MDIO_CMD_ACCESS_CODE_C45_ADDR 0 >> +#define MDIO_CMD_ACCESS_CODE_C45_WRITE 1 >> +#define MDIO_CMD_ACCESS_CODE_C45_READ 2 > > Where is anything related to ipq5332 here? This is for alignment format, will keep it untouched in the next patch set. > > > .. > >> bus->name = "ipq4019_mdio"; >> bus->read = ipq4019_mdio_read_c22; >> @@ -288,6 +296,7 @@ static void ipq4019_mdio_remove(struct platform_device *pdev) >> static const struct of_device_id ipq4019_mdio_dt_ids[] = { >> { .compatible = "qcom,ipq4019-mdio" }, >> { .compatible = "qcom,ipq5018-mdio" }, >> + { .compatible = "qcom,ipq5332-mdio" }, > > How user comes before binding? The new added compatible is for the GCC uniphy AHB/SYS clocks configured on the ipq5332 platform, will move this change into the following patch that involves the ipq5332 to make it clear. <net: mdio: ipq4019: Enable the clocks for ipq5332 platform>. > > Best regards, > Krzysztof >
On 17/11/2023 11:36, Jie Luo wrote: >>> clocks: >>> - items: >>> - - description: MDIO clock source frequency fixed to 100MHZ >>> + minItems: 1 >>> + maxItems: 5 >>> + description: >> >> Doesn't this make all other variants with incorrect constraints? > > There are 5 clock items, the first one is the legacy MDIO clock, the > other clocks are new added for ipq5332 platform, will describe it more > clearly in the next patch set. OTHER variants. Not this one. > >> >>> + MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy >>> + clocks enabled for resetting ethernet PHY. >>> >>> clock-names: >>> - items: >>> - - const: gcc_mdio_ahb_clk >>> + minItems: 1 >>> + maxItems: 5 >>> + >>> + phy-reset-gpio: >> >> No, for multiple reasons. It's gpios first of all. Where do you see such >> property? Where is the existing definition? > > will remove this property, and update to use the exited PHY GPIO reset. > >> >> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties >> in MDIO? >> >>> + minItems: 1 >>> + maxItems: 3 >>> + description: >>> + GPIO used to reset the PHY, each GPIO is for resetting the connected >>> + ethernet PHY device. >>> + >>> + phyaddr-fixup: >>> + description: Register address for programing MDIO address of PHY devices >> >> You did not test code which you sent. > > Hi Krzysztof, > This patch is passed with the following command in my workspace. > i will upgrade and install yamllint to make sure there is no > warning reported anymore. > > make dt_bg_check No clue what's this, but no, I do not believe you tested it at all. It's not about yamllint. It's was not tested. Look at errors reported on mailing list. > DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml Best regards, Krzysztof
On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote: > On 17/11/2023 11:36, Jie Luo wrote: >>>> clocks: >>>> - items: >>>> - - description: MDIO clock source frequency fixed to 100MHZ >>>> + minItems: 1 >>>> + maxItems: 5 >>>> + description: >>> >>> Doesn't this make all other variants with incorrect constraints? >> >> There are 5 clock items, the first one is the legacy MDIO clock, the >> other clocks are new added for ipq5332 platform, will describe it more >> clearly in the next patch set. > > OTHER variants. Not this one. The change here is for the clock number added for the ipq5332 platform, the other platforms still use only legacy MDIO clock. so i add minItems and maxItems, i will check other .yaml files for the reference. > >> >>> >>>> + MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy >>>> + clocks enabled for resetting ethernet PHY. >>>> >>>> clock-names: >>>> - items: >>>> - - const: gcc_mdio_ahb_clk >>>> + minItems: 1 >>>> + maxItems: 5 >>>> + >>>> + phy-reset-gpio: >>> >>> No, for multiple reasons. It's gpios first of all. Where do you see such >>> property? Where is the existing definition? >> >> will remove this property, and update to use the exited PHY GPIO reset. >> >>> >>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties >>> in MDIO? >>> >>>> + minItems: 1 >>>> + maxItems: 3 >>>> + description: >>>> + GPIO used to reset the PHY, each GPIO is for resetting the connected >>>> + ethernet PHY device. >>>> + >>>> + phyaddr-fixup: >>>> + description: Register address for programing MDIO address of PHY devices >>> >>> You did not test code which you sent. >> >> Hi Krzysztof, >> This patch is passed with the following command in my workspace. >> i will upgrade and install yamllint to make sure there is no >> warning reported anymore. >> >> make dt_bg_check > > No clue what's this, but no, I do not believe you tested it at all. It's > not about yamllint. It's was not tested. Look at errors reported on > mailing list. > >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml > Hi Krzysztof, Here is the output when i run the dt check with this patch applied in my workspace, md64 is the alias for compiling the arm64 platform. md64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml warning: python package 'yamllint' not installed, skipping CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json DTEX Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dts DTC_CHK Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.example.dtb > > Best regards, > Krzysztof >
On 11/17/2023 8:43 PM, Krzysztof Kozlowski wrote: > On 17/11/2023 12:20, Jie Luo wrote: >> >> >> On 11/17/2023 6:40 PM, Krzysztof Kozlowski wrote: >>> On 17/11/2023 11:36, Jie Luo wrote: >>>>>> clocks: >>>>>> - items: >>>>>> - - description: MDIO clock source frequency fixed to 100MHZ >>>>>> + minItems: 1 >>>>>> + maxItems: 5 >>>>>> + description: >>>>> >>>>> Doesn't this make all other variants with incorrect constraints? >>>> >>>> There are 5 clock items, the first one is the legacy MDIO clock, the >>>> other clocks are new added for ipq5332 platform, will describe it more >>>> clearly in the next patch set. >>> >>> OTHER variants. Not this one. >> >> The change here is for the clock number added for the ipq5332 platform, >> the other platforms still use only legacy MDIO clock. > > Then your patch is wrong as I said. You now affect other variants. I > don't quite get your responses. Style of them suggests that you > disagree, but you are not providing any relevant argument. The clock arguments are provided in the later part as below. i will also provide more detail clock names for the new added clocks for the ipq5332 platform in description. - if: properties: compatible: contains: enum: - qcom,ipq5332-mdio then: properties: clocks: items: - description: MDIO clock source frequency fixed to 100MHZ - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ clock-names: items: - const: gcc_mdio_ahb_clk - const: gcc_uniphy0_ahb_clk - const: gcc_uniphy0_sys_clk - const: gcc_uniphy1_ahb_clk - const: gcc_uniphy1_sys_clk > >> >> so i add minItems and maxItems, i will check other .yaml files for the >> reference. >> >>> >>>> >>>>> >>>>>> + MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy >>>>>> + clocks enabled for resetting ethernet PHY. >>>>>> >>>>>> clock-names: >>>>>> - items: >>>>>> - - const: gcc_mdio_ahb_clk >>>>>> + minItems: 1 >>>>>> + maxItems: 5 >>>>>> + >>>>>> + phy-reset-gpio: >>>>> >>>>> No, for multiple reasons. It's gpios first of all. Where do you see such >>>>> property? Where is the existing definition? >>>> >>>> will remove this property, and update to use the exited PHY GPIO reset. >>>> >>>>> >>>>> Then it is "reset-gpios" if this is MDIO. Why do you put phy properties >>>>> in MDIO? >>>>> >>>>>> + minItems: 1 >>>>>> + maxItems: 3 >>>>>> + description: >>>>>> + GPIO used to reset the PHY, each GPIO is for resetting the connected >>>>>> + ethernet PHY device. >>>>>> + >>>>>> + phyaddr-fixup: >>>>>> + description: Register address for programing MDIO address of PHY devices >>>>> >>>>> You did not test code which you sent. >>>> >>>> Hi Krzysztof, >>>> This patch is passed with the following command in my workspace. >>>> i will upgrade and install yamllint to make sure there is no >>>> warning reported anymore. >>>> >>>> make dt_bg_check >>> >>> No clue what's this, but no, I do not believe you tested it at all. It's >>> not about yamllint. It's was not tested. Look at errors reported on >>> mailing list. >>> >>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml >>> >> >> Hi Krzysztof, >> Here is the output when i run the dt check with this patch applied in my >> workspace, md64 is the alias for compiling the arm64 platform. > > We still do not know your base and dtschema version. The code base is the commit id: 5ba73bec5e7b0494da7fdca3e003d8b97fa932cd <Add linux-next specific files for 20231114> The dtschema version is as below. dt-doc-validate --version 2023.9 > > > Best regards, > Krzysztof >
> The clock arguments are provided in the later part as below. i will also > provide more detail clock names for the new added clocks for the ipq5332 > platform in description. > > - if: > > properties: > > compatible: > > contains: > > enum: > > - qcom,ipq5332-mdio > > then: > > properties: > > clocks: > > items: > > - description: MDIO clock source frequency fixed to 100MHZ > > - description: UNIPHY0 AHB clock source frequency fixed to > 100MHZ > - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ > - description: UNIPHY1 AHB clock source frequency fixed to > 100MHZ > - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ As i said before, the frequency of the clocks does not matter here. That appears to be the drivers problem. I assume every board design, with any sort of PHY, needs the same clock configuration? Andrew
On 11/17/2023 1:20 AM, Andrew Lunn wrote: >> FYI, here is the sequence to bring up qca8084. >> a. enable clock output to qca8084. >> b. do gpio reset of qca8084. >> c. customize MDIO address and initialization configurations. >> d. the PHY ID can be acquired. > > This all sounds like it is specific to the qca8084, so it should be in > the driver for the qca8084. > > Its been pointed out you can get the driver to load by using the PHY > ID in the compatible. You want the SoC clock driver to export a CCF > clock, which the PHY driver can use. The PHY driver should also be > able to get the GPIO. So i think the PHY driver can do all this. > > Andrew Hi Andrew, If i put the GPIO reset in the PHY device tree node, the PHY probe function will be postponed to be called instead of being called during the MDIO bus register, which leads to the PCS can't be created correctly because of reading PHY capability failed before the PHY probe function called. my device tree nodes are as below. ethernet_device { phy-handle = <&phy3>; phy-mode = "2500base-x"; ... }; mdio@90000 { phy3: ethernet-phy@3 { compatible = "ethernet-phy-id004d.d180"; reg = <4>; reset-gpios = <&tlmm 51 GPIO_ACTIVE_LOW>; reset-assert-us = <100000>; reset-deassert-us = <100000>; clocks = <...>; clock-names = "..."; }; }; Since the PHY probe function of phy3 is postponed instead of called during the MDIO bus driver register, and the initialization of qca8084 is not called when the ethernet_device driver is called to create PCS, where the phy3 capability is checked, which is failed since the qca8084 PHY probe is not called. Any idea to resolve this call sequence issue? Thanks.