2021-05-06 11:16:59

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 0/5] phy: ralink: mt7621-pci-phy: some improvements

Hi all,

This series contains some improvements in the pci phy driver
for MT7621 SoCs.

MT7621 SoC clock driver has already mainlined in
'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'

Because of this we can update schema documentation and device tree
to add related clock entries and avoid custom architecture code
in favour of using the clock kernel framework to retrieve clock
frequency needed to properly configure the PCIe related Phys.

After this changes there is no problem to properly enable this
driver for COMPILE_TEST.

Configuration has also modified from 'tristate' to 'bool' depending
on PCI_MT7621 which seems to have more sense.

Thanks in advance for your time.

Best regards,
Sergio Paracuellos


Sergio Paracuellos (5):
staging: mt7621-dts: use clock in pci phy nodes
dt-bindings: phy: mediatek,mt7621-pci-phy: add clock entries
phy: ralink: phy-mt7621-pci: use kernel clock APIS
phy: ralink: Kconfig: enable COMPILE_TEST on mt7621-pci-phy driver
phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'

.../bindings/phy/mediatek,mt7621-pci-phy.yaml | 12 +++++++
drivers/phy/ralink/Kconfig | 4 +--
drivers/phy/ralink/phy-mt7621-pci.c | 33 +++++++++++--------
drivers/staging/mt7621-dts/mt7621.dtsi | 4 +++
4 files changed, 38 insertions(+), 15 deletions(-)

--
2.25.1


2021-05-06 11:17:02

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 1/5] staging: mt7621-dts: use clock in pci phy nodes

MT7621 SoC clock driver has already mainlined in
'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'
Hence we can use the clock in pcie phy nodes to
be able to get it from there in driver code.

Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/staging/mt7621-dts/mt7621.dtsi | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 5623d542bcf2..001ff8f51033 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -549,12 +549,16 @@ pcie@2,0 {
pcie0_phy: pcie-phy@1e149000 {
compatible = "mediatek,mt7621-pci-phy";
reg = <0x1e149000 0x0700>;
+ clocks = <&sysc MT7621_CLK_XTAL>;
+ clock-names = "sys_clk";
#phy-cells = <1>;
};

pcie2_phy: pcie-phy@1e14a000 {
compatible = "mediatek,mt7621-pci-phy";
reg = <0x1e14a000 0x0700>;
+ clocks = <&sysc MT7621_CLK_XTAL>;
+ clock-names = "sys_clk";
#phy-cells = <1>;
};
};
--
2.25.1

2021-05-06 11:18:06

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 4/5] phy: ralink: Kconfig: enable COMPILE_TEST on mt7621-pci-phy driver

After use the clock apis and avoid custom architecture
code this driver can properly be enabled for COMPILE_TEST.

Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/phy/ralink/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/ralink/Kconfig b/drivers/phy/ralink/Kconfig
index ecc309ba9fee..c2373b30b8a6 100644
--- a/drivers/phy/ralink/Kconfig
+++ b/drivers/phy/ralink/Kconfig
@@ -4,7 +4,7 @@
#
config PHY_MT7621_PCI
tristate "MediaTek MT7621 PCI PHY Driver"
- depends on RALINK && OF
+ depends on (RALINK && OF) || COMPILE_TEST
select GENERIC_PHY
select REGMAP_MMIO
help
--
2.25.1

2021-05-06 11:18:06

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 3/5] phy: ralink: phy-mt7621-pci: use kernel clock APIS

MT7621 SoC clock driver has already mainlined in
'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'
This allow us to properly use kernel clock apis to get
the clock frequency needed for the phy configuration
instead of use custom architecture code to do the same.

Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/phy/ralink/phy-mt7621-pci.c | 33 +++++++++++++++++------------
1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/ralink/phy-mt7621-pci.c b/drivers/phy/ralink/phy-mt7621-pci.c
index 753cb5bab930..5222edc7be10 100644
--- a/drivers/phy/ralink/phy-mt7621-pci.c
+++ b/drivers/phy/ralink/phy-mt7621-pci.c
@@ -5,6 +5,7 @@
*/

#include <dt-bindings/phy/phy.h>
+#include <linux/clk.h>
#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/module.h>
@@ -14,8 +15,6 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/sys_soc.h>
-#include <mt7621.h>
-#include <ralink_regs.h>

#define RG_PE1_PIPE_REG 0x02c
#define RG_PE1_PIPE_RST BIT(12)
@@ -62,8 +61,6 @@

#define RG_PE1_FRC_MSTCKDIV BIT(5)

-#define XTAL_MASK GENMASK(8, 6)
-
#define MAX_PHYS 2

/**
@@ -71,6 +68,7 @@
* @dev: pointer to device
* @regmap: kernel regmap pointer
* @phy: pointer to the kernel PHY device
+ * @sys_clk: pointer to the system XTAL clock
* @port_base: base register
* @has_dual_port: if the phy has dual ports.
* @bypass_pipe_rst: mark if 'mt7621_bypass_pipe_rst'
@@ -80,6 +78,7 @@ struct mt7621_pci_phy {
struct device *dev;
struct regmap *regmap;
struct phy *phy;
+ struct clk *sys_clk;
void __iomem *port_base;
bool has_dual_port;
bool bypass_pipe_rst;
@@ -116,12 +115,14 @@ static void mt7621_bypass_pipe_rst(struct mt7621_pci_phy *phy)
}
}

-static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
+static int mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
{
struct device *dev = phy->dev;
- u32 xtal_mode;
+ unsigned long clk_rate;

- xtal_mode = FIELD_GET(XTAL_MASK, rt_sysc_r32(SYSC_REG_SYSTEM_CONFIG0));
+ clk_rate = clk_get_rate(phy->sys_clk);
+ if (!clk_rate)
+ return -EINVAL;

/* Set PCIe Port PHY to disable SSC */
/* Debug Xtal Type */
@@ -139,13 +140,13 @@ static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
RG_PE1_PHY_EN, RG_PE1_FRC_PHY_EN);
}

- if (xtal_mode <= 5 && xtal_mode >= 3) { /* 40MHz Xtal */
+ if (clk_rate == 40000000) { /* 40MHz Xtal */
/* Set Pre-divider ratio (for host mode) */
mt7621_phy_rmw(phy, RG_PE1_H_PLL_REG, RG_PE1_H_PLL_PREDIV,
FIELD_PREP(RG_PE1_H_PLL_PREDIV, 0x01));

dev_dbg(dev, "Xtal is 40MHz\n");
- } else if (xtal_mode >= 6) { /* 25MHz Xal */
+ } else if (clk_rate == 25000000) { /* 25MHz Xal */
mt7621_phy_rmw(phy, RG_PE1_H_PLL_REG, RG_PE1_H_PLL_PREDIV,
FIELD_PREP(RG_PE1_H_PLL_PREDIV, 0x00));

@@ -196,13 +197,15 @@ static void mt7621_set_phy_for_ssc(struct mt7621_pci_phy *phy)
mt7621_phy_rmw(phy, RG_PE1_H_PLL_BR_REG, RG_PE1_H_PLL_BR,
FIELD_PREP(RG_PE1_H_PLL_BR, 0x00));

- if (xtal_mode <= 5 && xtal_mode >= 3) { /* 40MHz Xtal */
+ if (clk_rate == 40000000) { /* 40MHz Xtal */
/* set force mode enable of da_pe1_mstckdiv */
mt7621_phy_rmw(phy, RG_PE1_MSTCKDIV_REG,
RG_PE1_MSTCKDIV | RG_PE1_FRC_MSTCKDIV,
FIELD_PREP(RG_PE1_MSTCKDIV, 0x01) |
RG_PE1_FRC_MSTCKDIV);
}
+
+ return 0;
}

static int mt7621_pci_phy_init(struct phy *phy)
@@ -212,9 +215,7 @@ static int mt7621_pci_phy_init(struct phy *phy)
if (mphy->bypass_pipe_rst)
mt7621_bypass_pipe_rst(mphy);

- mt7621_set_phy_for_ssc(mphy);
-
- return 0;
+ return mt7621_set_phy_for_ssc(mphy);
}

static int mt7621_pci_phy_power_on(struct phy *phy)
@@ -324,6 +325,12 @@ static int mt7621_pci_phy_probe(struct platform_device *pdev)
return PTR_ERR(phy->phy);
}

+ phy->sys_clk = devm_clk_get(dev, "sys_clk");
+ if (IS_ERR(phy->sys_clk)) {
+ dev_err(dev, "failed to get phy clock\n");
+ return PTR_ERR(phy->sys_clk);
+ }
+
phy_set_drvdata(phy->phy, phy);

provider = devm_of_phy_provider_register(dev, mt7621_pcie_phy_of_xlate);
--
2.25.1

2021-05-06 11:18:22

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 5/5] phy: ralink: Kconfig: convert mt7621-pci-phy into 'bool'

Make dependant on PCI_MT7621 configuration option and mark
this pci phy configuration as bool which has more sense.

Signed-off-by: Sergio Paracuellos <[email protected]>
---
drivers/phy/ralink/Kconfig | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/ralink/Kconfig b/drivers/phy/ralink/Kconfig
index c2373b30b8a6..ed0c71eff2c4 100644
--- a/drivers/phy/ralink/Kconfig
+++ b/drivers/phy/ralink/Kconfig
@@ -3,8 +3,8 @@
# PHY drivers for Ralink platforms.
#
config PHY_MT7621_PCI
- tristate "MediaTek MT7621 PCI PHY Driver"
- depends on (RALINK && OF) || COMPILE_TEST
+ bool "MediaTek MT7621 PCI PHY Driver"
+ depends on (RALINK && OF && PCI_MT7621) || COMPILE_TEST
select GENERIC_PHY
select REGMAP_MMIO
help
--
2.25.1

2021-05-06 18:12:18

by Sergio Paracuellos

[permalink] [raw]
Subject: [PATCH 2/5] dt-bindings: phy: mediatek,mt7621-pci-phy: add clock entries

MT7621 SoC clock driver has already mainlined in
'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'
Hence update schema with the add of the entries related to
clock.

Signed-off-by: Sergio Paracuellos <[email protected]>
---
.../bindings/phy/mediatek,mt7621-pci-phy.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
index 0ccaded3f245..d8614ef8995c 100644
--- a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
@@ -16,6 +16,14 @@ properties:
reg:
maxItems: 1

+ clocks:
+ maxItems: 1
+ description:
+ PHY reference clock. Must contain an entry in clock-names.
+
+ clock-names:
+ const: sys_clk
+
"#phy-cells":
const: 1
description: selects if the phy is dual-ported
@@ -23,6 +31,8 @@ properties:
required:
- compatible
- reg
+ - clocks
+ - clock-names
- "#phy-cells"

additionalProperties: false
@@ -32,5 +42,7 @@ examples:
pcie0_phy: pcie-phy@1e149000 {
compatible = "mediatek,mt7621-pci-phy";
reg = <0x1e149000 0x0700>;
+ clocks = <&sysc 0>;
+ clock-names = "sys_clk";
#phy-cells = <1>;
};
--
2.25.1

2021-05-06 23:19:27

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 4/5] phy: ralink: Kconfig: enable COMPILE_TEST on mt7621-pci-phy driver

Hi Sergio,

I love your patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on linus/master next-20210506]
[cannot apply to robh/for-next v5.12]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Sergio-Paracuellos/phy-ralink-mt7621-pci-phy-some-improvements/20210506-191714
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git e120332923e13d8d9386594a83dc7cbf327e3edf
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/44e8bb824a0a4af6fc41d67b70ec3a678bd8125f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sergio-Paracuellos/phy-ralink-mt7621-pci-phy-some-improvements/20210506-191714
git checkout 44e8bb824a0a4af6fc41d67b70ec3a678bd8125f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sparc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/device.h:15,
from include/linux/node.h:18,
from include/linux/cpu.h:17,
from include/linux/of_device.h:5,
from drivers/phy/ralink/phy-mt7621-pci.c:13:
drivers/phy/ralink/phy-mt7621-pci.c: In function 'mt7621_pcie_phy_of_xlate':
>> drivers/phy/ralink/phy-mt7621-pci.c:277:4: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
277 | (unsigned int)mt7621_phy->port_base, mt7621_phy->has_dual_port);
| ^
include/linux/dev_printk.h:118:33: note: in definition of macro 'dev_info'
118 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~


vim +277 drivers/phy/ralink/phy-mt7621-pci.c

d87da32372a03c Sergio Paracuellos 2020-11-21 265
d87da32372a03c Sergio Paracuellos 2020-11-21 266 static struct phy *mt7621_pcie_phy_of_xlate(struct device *dev,
d87da32372a03c Sergio Paracuellos 2020-11-21 267 struct of_phandle_args *args)
d87da32372a03c Sergio Paracuellos 2020-11-21 268 {
d87da32372a03c Sergio Paracuellos 2020-11-21 269 struct mt7621_pci_phy *mt7621_phy = dev_get_drvdata(dev);
d87da32372a03c Sergio Paracuellos 2020-11-21 270
d87da32372a03c Sergio Paracuellos 2020-11-21 271 if (WARN_ON(args->args[0] >= MAX_PHYS))
d87da32372a03c Sergio Paracuellos 2020-11-21 272 return ERR_PTR(-ENODEV);
d87da32372a03c Sergio Paracuellos 2020-11-21 273
d87da32372a03c Sergio Paracuellos 2020-11-21 274 mt7621_phy->has_dual_port = args->args[0];
d87da32372a03c Sergio Paracuellos 2020-11-21 275
d87da32372a03c Sergio Paracuellos 2020-11-21 276 dev_info(dev, "PHY for 0x%08x (dual port = %d)\n",
d87da32372a03c Sergio Paracuellos 2020-11-21 @277 (unsigned int)mt7621_phy->port_base, mt7621_phy->has_dual_port);
d87da32372a03c Sergio Paracuellos 2020-11-21 278
d87da32372a03c Sergio Paracuellos 2020-11-21 279 return mt7621_phy->phy;
d87da32372a03c Sergio Paracuellos 2020-11-21 280 }
d87da32372a03c Sergio Paracuellos 2020-11-21 281

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (3.73 kB)
.config.gz (67.98 kB)
Download all attachments

2021-05-07 06:44:55

by Sergio Paracuellos

[permalink] [raw]
Subject: Re: [PATCH 4/5] phy: ralink: Kconfig: enable COMPILE_TEST on mt7621-pci-phy driver

Hi,

On Thu, May 6, 2021 at 5:59 PM kernel test robot <[email protected]> wrote:
>
> Hi Sergio,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on staging/staging-testing]
> [also build test WARNING on linus/master next-20210506]
> [cannot apply to robh/for-next v5.12]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Sergio-Paracuellos/phy-ralink-mt7621-pci-phy-some-improvements/20210506-191714
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git e120332923e13d8d9386594a83dc7cbf327e3edf
> config: sparc-allyesconfig (attached as .config)
> compiler: sparc64-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/0day-ci/linux/commit/44e8bb824a0a4af6fc41d67b70ec3a678bd8125f
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Sergio-Paracuellos/phy-ralink-mt7621-pci-phy-some-improvements/20210506-191714
> git checkout 44e8bb824a0a4af6fc41d67b70ec3a678bd8125f
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=sparc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/device.h:15,
> from include/linux/node.h:18,
> from include/linux/cpu.h:17,
> from include/linux/of_device.h:5,
> from drivers/phy/ralink/phy-mt7621-pci.c:13:
> drivers/phy/ralink/phy-mt7621-pci.c: In function 'mt7621_pcie_phy_of_xlate':
> >> drivers/phy/ralink/phy-mt7621-pci.c:277:4: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> 277 | (unsigned int)mt7621_phy->port_base, mt7621_phy->has_dual_port);
> | ^
> include/linux/dev_printk.h:118:33: note: in definition of macro 'dev_info'
> 118 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
> | ^~~~~~~~~~~
>

Thanks!, COMPILE_TEST is doing a good job there :). Ok, so I have just
sent a patch for this:
See: https://lkml.org/lkml/2021/5/7/28

Best regards,
Sergio Paracuellos

>
> vim +277 drivers/phy/ralink/phy-mt7621-pci.c
>
> d87da32372a03c Sergio Paracuellos 2020-11-21 265
> d87da32372a03c Sergio Paracuellos 2020-11-21 266 static struct phy *mt7621_pcie_phy_of_xlate(struct device *dev,
> d87da32372a03c Sergio Paracuellos 2020-11-21 267 struct of_phandle_args *args)
> d87da32372a03c Sergio Paracuellos 2020-11-21 268 {
> d87da32372a03c Sergio Paracuellos 2020-11-21 269 struct mt7621_pci_phy *mt7621_phy = dev_get_drvdata(dev);
> d87da32372a03c Sergio Paracuellos 2020-11-21 270
> d87da32372a03c Sergio Paracuellos 2020-11-21 271 if (WARN_ON(args->args[0] >= MAX_PHYS))
> d87da32372a03c Sergio Paracuellos 2020-11-21 272 return ERR_PTR(-ENODEV);
> d87da32372a03c Sergio Paracuellos 2020-11-21 273
> d87da32372a03c Sergio Paracuellos 2020-11-21 274 mt7621_phy->has_dual_port = args->args[0];
> d87da32372a03c Sergio Paracuellos 2020-11-21 275
> d87da32372a03c Sergio Paracuellos 2020-11-21 276 dev_info(dev, "PHY for 0x%08x (dual port = %d)\n",
> d87da32372a03c Sergio Paracuellos 2020-11-21 @277 (unsigned int)mt7621_phy->port_base, mt7621_phy->has_dual_port);
> d87da32372a03c Sergio Paracuellos 2020-11-21 278
> d87da32372a03c Sergio Paracuellos 2020-11-21 279 return mt7621_phy->phy;
> d87da32372a03c Sergio Paracuellos 2020-11-21 280 }
> d87da32372a03c Sergio Paracuellos 2020-11-21 281
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]

2021-05-07 22:15:52

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 2/5] dt-bindings: phy: mediatek,mt7621-pci-phy: add clock entries

On Thu, May 06, 2021 at 01:15:28PM +0200, Sergio Paracuellos wrote:
> MT7621 SoC clock driver has already mainlined in
> 'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'
> Hence update schema with the add of the entries related to
> clock.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
> .../bindings/phy/mediatek,mt7621-pci-phy.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> index 0ccaded3f245..d8614ef8995c 100644
> --- a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> @@ -16,6 +16,14 @@ properties:
> reg:
> maxItems: 1
>
> + clocks:
> + maxItems: 1
> + description:
> + PHY reference clock. Must contain an entry in clock-names.
> +
> + clock-names:
> + const: sys_clk

You don't really need -names when there is only 1.

> +
> "#phy-cells":
> const: 1
> description: selects if the phy is dual-ported
> @@ -23,6 +31,8 @@ properties:
> required:
> - compatible
> - reg
> + - clocks
> + - clock-names

Technically, you can't add new properties and make them required as that
breaks the ABI. If that's okay here, explain it in the commit message.

> - "#phy-cells"
>
> additionalProperties: false
> @@ -32,5 +42,7 @@ examples:
> pcie0_phy: pcie-phy@1e149000 {
> compatible = "mediatek,mt7621-pci-phy";
> reg = <0x1e149000 0x0700>;
> + clocks = <&sysc 0>;
> + clock-names = "sys_clk";
> #phy-cells = <1>;
> };
> --
> 2.25.1
>

2021-05-08 06:41:44

by Sergio Paracuellos

[permalink] [raw]
Subject: Re: [PATCH 2/5] dt-bindings: phy: mediatek,mt7621-pci-phy: add clock entries

Hi Rob,

On Sat, May 8, 2021 at 12:12 AM Rob Herring <[email protected]> wrote:
>
> On Thu, May 06, 2021 at 01:15:28PM +0200, Sergio Paracuellos wrote:
> > MT7621 SoC clock driver has already mainlined in
> > 'commit 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")'
> > Hence update schema with the add of the entries related to
> > clock.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> > .../bindings/phy/mediatek,mt7621-pci-phy.yaml | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> > index 0ccaded3f245..d8614ef8995c 100644
> > --- a/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml
> > @@ -16,6 +16,14 @@ properties:
> > reg:
> > maxItems: 1
> >
> > + clocks:
> > + maxItems: 1
> > + description:
> > + PHY reference clock. Must contain an entry in clock-names.
> > +
> > + clock-names:
> > + const: sys_clk
>
> You don't really need -names when there is only 1.

Ok, I will drop this property then and
>
> > +
> > "#phy-cells":
> > const: 1
> > description: selects if the phy is dual-ported
> > @@ -23,6 +31,8 @@ properties:
> > required:
> > - compatible
> > - reg
> > + - clocks
> > + - clock-names
>
> Technically, you can't add new properties and make them required as that
> breaks the ABI. If that's okay here, explain it in the commit message.

So until now no clock driver existed and things were not properly
being done in driver code directly accessing registers to get the
clock frequency to properly configure the phy. Since the new clock
driver enters into the scene, make this mandatory force to update both
driver and dtb, which is something pretty normal when upgrading the
kind of devices using this SoC. So I think it should be finde to make
this a requirement.

>
> > - "#phy-cells"
> >
> > additionalProperties: false
> > @@ -32,5 +42,7 @@ examples:
> > pcie0_phy: pcie-phy@1e149000 {
> > compatible = "mediatek,mt7621-pci-phy";
> > reg = <0x1e149000 0x0700>;
> > + clocks = <&sysc 0>;
> > + clock-names = "sys_clk";
> > #phy-cells = <1>;
> > };
> > --
> > 2.25.1
> >

Best regards,
Sergio Paracuellos