[PATCH 0/7] Add clock config and pm support to bcm iProc mdio mux

[PATCH 0/7] Add clock config and pm support to bcm iProc mdio mux

[Arun Parameswaran]

Hi,
The patchset is based on David Miller's "net" repo.

The patchset extends the Broadcom iProc mdio mux to add support for
suspend/resume and the ability to configure the internal clock
divider.

The base address of the mdio-mux-bcm-iproc is modified to point to the
start of the mdio block's address space, to be able to access all the
mdio's registers. The missing registers are required to configure the
internal clock divider registers in some of the Broadcom SoC's.

Thanks
Arun

Arun Parameswaran (7):
  dt-bindings: net: Fix Broadcom iProc mdio mux driver base address
  net: phy: Fix the register offsets in Broadcom iProc mdio mux driver
  arm64: dts: Fix the base address of the Broadcom iProc mdio mux
  dt-bindings: net: Add clock handle to Broadcom iProc mdio mux
  net: phy: Add support to configure clock in Broadcom iProc mdio mux
  net: phy: Add pm support to Broadcom iProc mdio mux driver
  net: phy: Add pm support for scan ctrl register to bcm mdio mux

 .../bindings/net/brcm,mdio-mux-iproc.txt           |  7 +-
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi   |  4 +-
 .../arm64/boot/dts/broadcom/stingray/stingray.dtsi |  4 +-
 drivers/net/phy/mdio-mux-bcm-iproc.c               | 89 ++++++++++++++++++++--
 4 files changed, 92 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH 1/7] dt-bindings: net: Fix Broadcom iProc mdio mux driver base address

[Arun Parameswaran]

Modify the base address passed to the Broadcom iProc MDIO mux driver
to point to the start of the block's register address space.

Fixes: ce8d5dbfd64f ("Add DT binding doc for Broadcom MDIO bus multiplexer")
Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
index dfe287a..dc8aa68 100644
--- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
+++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
@@ -18,9 +18,9 @@ at- Documentation/devicetree/bindings/net/mdio-mux.txt
 
 
 for example:
-		mdio_mux_iproc: mdio-mux@6602023c {
+		mdio_mux_iproc: mdio-mux@66020000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x6602023c 0x14>;
+			reg = <0x66020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
1.9.1

Re: [PATCH 1/7] dt-bindings: net: Fix Broadcom iProc mdio mux driver base address

[Andrew Lunn]

On Thu, Jul 26, 2018 at 11:36:18AM -0700, Arun Parameswaran wrote:
> Modify the base address passed to the Broadcom iProc MDIO mux driver
> to point to the start of the block's register address space.
> 
> Fixes: ce8d5dbfd64f ("Add DT binding doc for Broadcom MDIO bus multiplexer")

Hi Arun

Fixes generally means something explodes, throws an Opps, userspace
does the wrong thing. Documentation/process/stable-kernel-rules.rst
says:

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

Please explain how this is a real problem and what people it bothers.

I also don't see any attempt to keep backwards compatibility with
older device tree blobs. Is it intentional you will break such old
blobs?

Thanks
	Andrew


> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
> ---
>  Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
> index dfe287a..dc8aa68 100644
> --- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
> +++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
> @@ -18,9 +18,9 @@ at- Documentation/devicetree/bindings/net/mdio-mux.txt
>  
>  
>  for example:
> -		mdio_mux_iproc: mdio-mux@6602023c {
> +		mdio_mux_iproc: mdio-mux@66020000 {
>  			compatible = "brcm,mdio-mux-iproc";
> -			reg = <0x6602023c 0x14>;
> +			reg = <0x66020000 0x250>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  
> -- 
> 1.9.1
> 

Re: [PATCH 1/7] dt-bindings: net: Fix Broadcom iProc mdio mux driver base address

[Arun Parameswaran]

Hi Andrew

On 18-07-26 12:01 PM, Andrew Lunn wrote:
> On Thu, Jul 26, 2018 at 11:36:18AM -0700, Arun Parameswaran wrote:
>> Modify the base address passed to the Broadcom iProc MDIO mux driver
>> to point to the start of the block's register address space.
>>
>> Fixes: ce8d5dbfd64f ("Add DT binding doc for Broadcom MDIO bus multiplexer")
> 
> Hi Arun
> 
> Fixes generally means something explodes, throws an Opps, userspace
> does the wrong thing. Documentation/process/stable-kernel-rules.rst
> says:
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> Please explain how this is a real problem and what people it bothers.
> 
It is only relevant when we need to modify the registers which are not
currently accessible.

I will remove the 'fixes' and use the 'net-next' repo for the change
set.

> I also don't see any attempt to keep backwards compatibility with
> older device tree blobs. Is it intentional you will break such old
> blobs?
> 
I will modify the patch to keep backward compatibility like you
suggested in the other patch.

Thanks
Arun

> Thanks
> 	Andrew
> 
> 
>> Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
>> ---
>>  Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
>> index dfe287a..dc8aa68 100644
>> --- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
>> +++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
>> @@ -18,9 +18,9 @@ at- Documentation/devicetree/bindings/net/mdio-mux.txt
>>  
>>  
>>  for example:
>> -		mdio_mux_iproc: mdio-mux@6602023c {
>> +		mdio_mux_iproc: mdio-mux@66020000 {
>>  			compatible = "brcm,mdio-mux-iproc";
>> -			reg = <0x6602023c 0x14>;
>> +			reg = <0x66020000 0x250>;
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  
>> -- 
>> 1.9.1
>>

[PATCH 2/7] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Arun Parameswaran]

Modify the register offsets in the Broadcom iProc mdio mux to start
from the top of the register address space.

Earlier the base address specified was from the middle of the block's
register space. The base address will now point to the start of the
mdio's address space. The offsets have been fixed to match this.

Fixes: 98bc865a1ec8 ("net: mdio-mux: Add MDIO mux driver for iProc SoCs")
Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 0831b71..dc65e95 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -22,7 +22,7 @@
 #include <linux/mdio-mux.h>
 #include <linux/delay.h>
 
-#define MDIO_PARAM_OFFSET		0x00
+#define MDIO_PARAM_OFFSET		0x23c
 #define MDIO_PARAM_MIIM_CYCLE		29
 #define MDIO_PARAM_INTERNAL_SEL		25
 #define MDIO_PARAM_BUS_ID		22
@@ -30,15 +30,15 @@
 #define MDIO_PARAM_PHY_ID		16
 #define MDIO_PARAM_PHY_DATA		0
 
-#define MDIO_READ_OFFSET		0x04
+#define MDIO_READ_OFFSET		0x240
 #define MDIO_READ_DATA_MASK		0xffff
-#define MDIO_ADDR_OFFSET		0x08
+#define MDIO_ADDR_OFFSET		0x244
 
-#define MDIO_CTRL_OFFSET		0x0C
+#define MDIO_CTRL_OFFSET		0x248
 #define MDIO_CTRL_WRITE_OP		0x1
 #define MDIO_CTRL_READ_OP		0x2
 
-#define MDIO_STAT_OFFSET		0x10
+#define MDIO_STAT_OFFSET		0x24c
 #define MDIO_STAT_DONE			1
 
 #define BUS_MAX_ADDR			32
-- 
1.9.1

Re: [PATCH 2/7] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Andrew Lunn]

On Thu, Jul 26, 2018 at 11:36:19AM -0700, Arun Parameswaran wrote:
> Modify the register offsets in the Broadcom iProc mdio mux to start
> from the top of the register address space.
> 
> Earlier the base address specified was from the middle of the block's
> register space. The base address will now point to the start of the
> mdio's address space. The offsets have been fixed to match this.

Hi Arun

Did you consider a change something like:

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 0831b7142df7..2d53e609498c 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -169,6 +169,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
        md->dev = &pdev->dev;
 
        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+       if (res->start & 0xfff != 0) {
+               dev_info(&pdev->dev, "Please upgrade your device tree blob.\n");
+               res->start &= ~0xfff;
+       }
+
        md->base = devm_ioremap_resource(&pdev->dev, res);
        if (IS_ERR(md->base)) {
                dev_err(&pdev->dev, "failed to ioremap register\n");


	Andrew

Re: [PATCH 2/7] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Arun Parameswaran]

On 18-07-26 12:06 PM, Andrew Lunn wrote:
> On Thu, Jul 26, 2018 at 11:36:19AM -0700, Arun Parameswaran wrote:
>> Modify the register offsets in the Broadcom iProc mdio mux to start
>> from the top of the register address space.
>>
>> Earlier the base address specified was from the middle of the block's
>> register space. The base address will now point to the start of the
>> mdio's address space. The offsets have been fixed to match this.
> 
> Hi Arun
> 
> Did you consider a change something like:
That looks good. I will make this change to the patch.

Thanks
Arun
> 
> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
> index 0831b7142df7..2d53e609498c 100644
> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
> @@ -169,6 +169,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>         md->dev = &pdev->dev;
>  
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       if (res->start & 0xfff != 0) {
> +               dev_info(&pdev->dev, "Please upgrade your device tree blob.\n");
> +               res->start &= ~0xfff;
> +       }
> +
>         md->base = devm_ioremap_resource(&pdev->dev, res);
>         if (IS_ERR(md->base)) {
>                 dev_err(&pdev->dev, "failed to ioremap register\n");
> 
> 
> 	Andrew
> 

Re: [PATCH 2/7] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Ray Jui]

On 7/26/2018 12:16 PM, Arun Parameswaran wrote:
> 
> 
> On 18-07-26 12:06 PM, Andrew Lunn wrote:
>> On Thu, Jul 26, 2018 at 11:36:19AM -0700, Arun Parameswaran wrote:
>>> Modify the register offsets in the Broadcom iProc mdio mux to start
>>> from the top of the register address space.
>>>
>>> Earlier the base address specified was from the middle of the block's
>>> register space. The base address will now point to the start of the
>>> mdio's address space. The offsets have been fixed to match this.
>>
>> Hi Arun
>>
>> Did you consider a change something like:
> That looks good. I will make this change to the patch.
> 
> Thanks
> Arun

To make it backward compatible, then length of the resource also needs 
to be adjusted from 0x14 to 0x250 in the driver?

Otherwise you will end up accessing areas out of 0x14 defined in old DT 
that is not mapped?

Thanks,

Ray

>>
>> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
>> index 0831b7142df7..2d53e609498c 100644
>> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
>> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
>> @@ -169,6 +169,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>>          md->dev = &pdev->dev;
>>   
>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +       if (res->start & 0xfff != 0) {
>> +               dev_info(&pdev->dev, "Please upgrade your device tree blob.\n");
>> +               res->start &= ~0xfff;
>> +       }
>> +
>>          md->base = devm_ioremap_resource(&pdev->dev, res);
>>          if (IS_ERR(md->base)) {
>>                  dev_err(&pdev->dev, "failed to ioremap register\n");
>>
>>
>> 	Andrew
>>

Re: [PATCH 2/7] net: phy: Fix the register offsets in Broadcom iProc mdio mux driver

[Andrew Lunn]

On Thu, Jul 26, 2018 at 12:25:24PM -0700, Ray Jui wrote:
> 
> 
> On 7/26/2018 12:16 PM, Arun Parameswaran wrote:
> >
> >
> >On 18-07-26 12:06 PM, Andrew Lunn wrote:
> >>On Thu, Jul 26, 2018 at 11:36:19AM -0700, Arun Parameswaran wrote:
> >>>Modify the register offsets in the Broadcom iProc mdio mux to start
> >>>from the top of the register address space.
> >>>
> >>>Earlier the base address specified was from the middle of the block's
> >>>register space. The base address will now point to the start of the
> >>>mdio's address space. The offsets have been fixed to match this.
> >>
> >>Hi Arun
> >>
> >>Did you consider a change something like:
> >That looks good. I will make this change to the patch.
> >
> >Thanks
> >Arun
> 
> To make it backward compatible, then length of the resource also needs to be
> adjusted from 0x14 to 0x250 in the driver?
> 
> Otherwise you will end up accessing areas out of 0x14 defined in old DT that
> is not mapped?

struct resource {
       resource_size_t start;
       resource_size_t end;
       const char *name;
       unsigned long flags;
       unsigned long desc;
       struct resource *parent, *sibling, *child;
};

"end" suggests an address, not a length. But it would be good to look
deeper into the code to be sure.

       Andrew

[PATCH 3/7] arm64: dts: Fix the base address of the Broadcom iProc mdio mux

[Arun Parameswaran]

Modify the base address of the mdio mux driver to point to the start of
the mdio mux block's register address space.

Fixes: fd898f75dac7 ("arm64: dts: Add MDIO multiplexer DT node for Stingray")
Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi    | 4 ++--
 arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
index 4057197..1a406a7 100644
--- a/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/northstar2/ns2.dtsi
@@ -482,9 +482,9 @@
 			status = "disabled";
 		};
 
-		mdio_mux_iproc: mdio-mux@6602023c {
+		mdio_mux_iproc: mdio-mux@66020000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x6602023c 0x14>;
+			reg = <0x66020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index b203152..a70e8dd 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -278,9 +278,9 @@
 
 		#include "stingray-pinctrl.dtsi"
 
-		mdio_mux_iproc: mdio-mux@2023c {
+		mdio_mux_iproc: mdio-mux@20000 {
 			compatible = "brcm,mdio-mux-iproc";
-			reg = <0x0002023c 0x14>;
+			reg = <0x00020000 0x250>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-- 
1.9.1

[PATCH 4/7] dt-bindings: net: Add clock handle to Broadcom iProc mdio mux

[Arun Parameswaran]

Add clock phandle, of the core clock driving the mdio block, as an
optional property to the Broadcom iProc mdio mux block.

The clock, when specified, will be used to setup the rate adjust registers
in the mdio to derrive the mdio's operating frequency.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
index dc8aa68..b58843f 100644
--- a/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
+++ b/Documentation/devicetree/bindings/net/brcm,mdio-mux-iproc.txt
@@ -13,6 +13,9 @@ MDIO multiplexer node:
 Every non-ethernet PHY requires a compatible so that it could be probed based
 on this compatible string.
 
+Optional properties:
+- clocks: phandle of the core clock which drives the mdio block.
+
 Additional information regarding generic multiplexer properties can be found
 at- Documentation/devicetree/bindings/net/mdio-mux.txt
 
-- 
1.9.1

[PATCH 5/7] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Add support to configure the internal rate adjust register based on the
core clock supplied through device tree in the Broadcom iProc mdio mux.

The operating frequency of the mdio mux block is 11MHz. This is derrived
by dividing the clock to the mdio mux with the rate adjust register.

In some SoC's the default values of the rate adjust register do not yield
11MHz. These SoC's are required to specify the clock via the device tree
for proper operation.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index dc65e95..6b400dd 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -13,7 +13,7 @@
  * You should have received a copy of the GNU General Public License
  * version 2 (GPLv2) along with this source code.
  */
-
+#include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/device.h>
 #include <linux/of_mdio.h>
@@ -22,6 +22,10 @@
 #include <linux/mdio-mux.h>
 #include <linux/delay.h>
 
+#define MDIO_RATE_ADJ_EXT_OFFSET	0x000
+#define MDIO_RATE_ADJ_INT_OFFSET	0x004
+#define MDIO_RATE_ADJ_DIVIDENT_SHIFT	16
+
 #define MDIO_PARAM_OFFSET		0x23c
 #define MDIO_PARAM_MIIM_CYCLE		29
 #define MDIO_PARAM_INTERNAL_SEL		25
@@ -44,13 +48,32 @@
 #define BUS_MAX_ADDR			32
 #define EXT_BUS_START_ADDR		16
 
+#define MDIO_OPERATING_FREQUENCY	11000000
+#define MDIO_RATE_ADJ_DIVIDENT		1
+
 struct iproc_mdiomux_desc {
 	void *mux_handle;
 	void __iomem *base;
 	struct device *dev;
 	struct mii_bus *mii_bus;
+	struct clk *core_clk;
 };
 
+static void mdio_mux_iproc_config_clk(struct iproc_mdiomux_desc *md)
+{
+	u32 val;
+	u32 divisor;
+
+	if (md->core_clk) {
+		divisor = clk_get_rate(md->core_clk) / MDIO_OPERATING_FREQUENCY;
+		divisor = divisor / (MDIO_RATE_ADJ_DIVIDENT + 1);
+		val = divisor;
+		val |= MDIO_RATE_ADJ_DIVIDENT << MDIO_RATE_ADJ_DIVIDENT_SHIFT;
+		writel(val, md->base + MDIO_RATE_ADJ_EXT_OFFSET);
+		writel(val, md->base + MDIO_RATE_ADJ_INT_OFFSET);
+	}
+}
+
 static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
 {
 	unsigned int timeout = 1000; /* loop for 1s */
@@ -175,6 +198,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 		return PTR_ERR(md->base);
 	}
 
+	md->core_clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(md->core_clk)) {
+		dev_info(&pdev->dev, "core_clk not specified\n");
+		md->core_clk = NULL;
+	}
+
 	md->mii_bus = mdiobus_alloc();
 	if (!md->mii_bus) {
 		dev_err(&pdev->dev, "mdiomux bus alloc failed\n");
@@ -206,6 +235,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
 		goto out_register;
 	}
 
+	mdio_mux_iproc_config_clk(md);
+
 	dev_info(md->dev, "iProc mdiomux registered\n");
 	return 0;
 
-- 
1.9.1

Re: [PATCH 5/7] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

> @@ -175,6 +198,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>  		return PTR_ERR(md->base);
>  	}
>  
> +	md->core_clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(md->core_clk)) {
> +		dev_info(&pdev->dev, "core_clk not specified\n");
> +		md->core_clk = NULL;
> +	}
> +

In the binding, you say the clock is optional. This is a rather strong
message for something which is optional. I think it would be better to
not output anything.

    Andrew

Re: [PATCH 5/7] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

On 18-07-26 12:13 PM, Andrew Lunn wrote:
>> @@ -175,6 +198,12 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>>  		return PTR_ERR(md->base);
>>  	}
>>  
>> +	md->core_clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(md->core_clk)) {
>> +		dev_info(&pdev->dev, "core_clk not specified\n");
>> +		md->core_clk = NULL;
>> +	}
>> +
> 
> In the binding, you say the clock is optional. This is a rather strong
> message for something which is optional. I think it would be better to
> not output anything.
> 
>     Andrew
> 
Will remove the message.

Thanks
Arun

Re: [PATCH 5/7] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Andrew Lunn]

> +static void mdio_mux_iproc_config_clk(struct iproc_mdiomux_desc *md)
> +{
> +	u32 val;
> +	u32 divisor;
> +
> +	if (md->core_clk) {
> +		divisor = clk_get_rate(md->core_clk) / MDIO_OPERATING_FREQUENCY;

/**
 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 * 		  This is only valid once the clock source has been enabled.
 * @clk: clock source
 */
unsigned long clk_get_rate(struct clk *clk);

It is generally good practice to call clk_prepare_enable() sometime
before clk_get_rate() to ensure the clock is ticking, and to show this
driver is making use of the clock, so it does not get turned off.

> +		divisor = divisor / (MDIO_RATE_ADJ_DIVIDENT + 1);
> +		val = divisor;
> +		val |= MDIO_RATE_ADJ_DIVIDENT << MDIO_RATE_ADJ_DIVIDENT_SHIFT;
> +		writel(val, md->base + MDIO_RATE_ADJ_EXT_OFFSET);
> +		writel(val, md->base + MDIO_RATE_ADJ_INT_OFFSET);
> +	}
> +}

Re: [PATCH 5/7] net: phy: Add support to configure clock in Broadcom iProc mdio mux

[Arun Parameswaran]

Hi Andrew

On 18-07-26 12:26 PM, Andrew Lunn wrote:
>> +static void mdio_mux_iproc_config_clk(struct iproc_mdiomux_desc *md)
>> +{
>> +	u32 val;
>> +	u32 divisor;
>> +
>> +	if (md->core_clk) {
>> +		divisor = clk_get_rate(md->core_clk) / MDIO_OPERATING_FREQUENCY;
> 
> /**
>  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>  * 		  This is only valid once the clock source has been enabled.
>  * @clk: clock source
>  */
> unsigned long clk_get_rate(struct clk *clk);
> 
> It is generally good practice to call clk_prepare_enable() sometime
> before clk_get_rate() to ensure the clock is ticking, and to show this
> driver is making use of the clock, so it does not get turned off.

Will add 'clk_prepare_enable()' to the probe.

Thanks
Arun

> 
>> +		divisor = divisor / (MDIO_RATE_ADJ_DIVIDENT + 1);
>> +		val = divisor;
>> +		val |= MDIO_RATE_ADJ_DIVIDENT << MDIO_RATE_ADJ_DIVIDENT_SHIFT;
>> +		writel(val, md->base + MDIO_RATE_ADJ_EXT_OFFSET);
>> +		writel(val, md->base + MDIO_RATE_ADJ_INT_OFFSET);
>> +	}
>> +}

[PATCH 6/7] net: phy: Add pm support to Broadcom iProc mdio mux driver

[Arun Parameswaran]

Add support for suspend and resume to the Broadcom iProc mdio
mux driver.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 6b400dd..89c18d6 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -51,13 +51,25 @@
 #define MDIO_OPERATING_FREQUENCY	11000000
 #define MDIO_RATE_ADJ_DIVIDENT		1
 
+#define MDIO_NUM_OF_REGS_TO_RESTORE	2
+
 struct iproc_mdiomux_desc {
 	void *mux_handle;
 	void __iomem *base;
 	struct device *dev;
 	struct mii_bus *mii_bus;
 	struct clk *core_clk;
+#ifdef CONFIG_PM_SLEEP
+	u32 restore_regs[MDIO_NUM_OF_REGS_TO_RESTORE];
+#endif
+};
+
+#ifdef CONFIG_PM_SLEEP
+static const u16 restore_reg_offsets[MDIO_NUM_OF_REGS_TO_RESTORE] = {
+	MDIO_RATE_ADJ_EXT_OFFSET,
+	MDIO_RATE_ADJ_INT_OFFSET,
 };
+#endif
 
 static void mdio_mux_iproc_config_clk(struct iproc_mdiomux_desc *md)
 {
@@ -258,6 +270,35 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mdio_mux_iproc_suspend(struct device *dev)
+{
+	int i;
+	struct iproc_mdiomux_desc *md = dev_get_drvdata(dev);
+
+	for (i = 0; i < MDIO_NUM_OF_REGS_TO_RESTORE; i++)
+		md->restore_regs[i] = readl(md->base +
+					    restore_reg_offsets[i]);
+
+	return 0;
+}
+
+static int mdio_mux_iproc_resume(struct device *dev)
+{
+	int i;
+	struct iproc_mdiomux_desc *md = dev_get_drvdata(dev);
+
+	for (i = 0; i < MDIO_NUM_OF_REGS_TO_RESTORE; i++)
+		writel(md->restore_regs[i],
+		       md->base + restore_reg_offsets[i]);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mdio_mux_iproc_pm_ops,
+		mdio_mux_iproc_suspend, mdio_mux_iproc_resume);
+
 static const struct of_device_id mdio_mux_iproc_match[] = {
 	{
 		.compatible = "brcm,mdio-mux-iproc",
@@ -270,6 +311,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
 	.driver = {
 		.name		= "mdio-mux-iproc",
 		.of_match_table = mdio_mux_iproc_match,
+		.pm		= &mdio_mux_iproc_pm_ops,
 	},
 	.probe		= mdio_mux_iproc_probe,
 	.remove		= mdio_mux_iproc_remove,
-- 
1.9.1

Re: [PATCH 6/7] net: phy: Add pm support to Broadcom iProc mdio mux driver

[Andrew Lunn]

On Thu, Jul 26, 2018 at 11:36:23AM -0700, Arun Parameswaran wrote:
> Add support for suspend and resume to the Broadcom iProc mdio
> mux driver.

> +#ifdef CONFIG_PM_SLEEP
> +static int mdio_mux_iproc_suspend(struct device *dev)
> +{
> +	int i;
> +	struct iproc_mdiomux_desc *md = dev_get_drvdata(dev);
> +
> +	for (i = 0; i < MDIO_NUM_OF_REGS_TO_RESTORE; i++)
> +		md->restore_regs[i] = readl(md->base +
> +					    restore_reg_offsets[i]);
> +
> +	return 0;
> +}
> +
> +static int mdio_mux_iproc_resume(struct device *dev)
> +{
> +	int i;
> +	struct iproc_mdiomux_desc *md = dev_get_drvdata(dev);
> +
> +	for (i = 0; i < MDIO_NUM_OF_REGS_TO_RESTORE; i++)
> +		writel(md->restore_regs[i],
> +		       md->base + restore_reg_offsets[i]);
> +
> +	return 0;
> +}

Hi Arun

Would it not be simpler to just call mdio_mux_iproc_config_clk() on
resume?

	Andrew

[PATCH 7/7] net: phy: Add pm support for scan ctrl register to bcm mdio mux

[Arun Parameswaran]

Add support for saving and restoring the 'scan control' register
in the Broadcom iProc mdio mux driver.

Signed-off-by: Arun Parameswaran <arun.parameswaran@broadcom.com>
---
 drivers/net/phy/mdio-mux-bcm-iproc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 89c18d6..d1fce1e 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -26,6 +26,9 @@
 #define MDIO_RATE_ADJ_INT_OFFSET	0x004
 #define MDIO_RATE_ADJ_DIVIDENT_SHIFT	16
 
+#define MDIO_SCAN_CTRL_OFFSET	0x008
+#define MDIO_SCAN_CTRL_OVERRIDE_EXT_MSTR	28
+
 #define MDIO_PARAM_OFFSET		0x23c
 #define MDIO_PARAM_MIIM_CYCLE		29
 #define MDIO_PARAM_INTERNAL_SEL		25
@@ -51,7 +54,7 @@
 #define MDIO_OPERATING_FREQUENCY	11000000
 #define MDIO_RATE_ADJ_DIVIDENT		1
 
-#define MDIO_NUM_OF_REGS_TO_RESTORE	2
+#define MDIO_NUM_OF_REGS_TO_RESTORE	3
 
 struct iproc_mdiomux_desc {
 	void *mux_handle;
@@ -68,6 +71,7 @@ struct iproc_mdiomux_desc {
 static const u16 restore_reg_offsets[MDIO_NUM_OF_REGS_TO_RESTORE] = {
 	MDIO_RATE_ADJ_EXT_OFFSET,
 	MDIO_RATE_ADJ_INT_OFFSET,
+	MDIO_SCAN_CTRL_OVERRIDE_EXT_MSTR,
 };
 #endif
 
-- 
1.9.1

Re: [PATCH 7/7] net: phy: Add pm support for scan ctrl register to bcm mdio mux

[Andrew Lunn]

On Thu, Jul 26, 2018 at 11:36:24AM -0700, Arun Parameswaran wrote:
> Add support for saving and restoring the 'scan control' register
> in the Broadcom iProc mdio mux driver.

Hi Arun

I don't see anything setting this register. So why is it necessary to
save it over suspend/resume?

     Andrew

Re: [PATCH 7/7] net: phy: Add pm support for scan ctrl register to bcm mdio mux

[Arun Parameswaran]

On 18-07-26 12:20 PM, Andrew Lunn wrote:
> On Thu, Jul 26, 2018 at 11:36:24AM -0700, Arun Parameswaran wrote:
>> Add support for saving and restoring the 'scan control' register
>> in the Broadcom iProc mdio mux driver.
> 
> Hi Arun
> 
> I don't see anything setting this register. So why is it necessary to
> save it over suspend/resume?
> 
>      Andrew
> 
Hi Andrew
In the Omega SoC (only), this register was defaulted incorrectly (to use
a test mode) by the hardware. So we added the setup of the register to
the early bootloader (ATF).

We still need to restore this register while resuming from sleep.

Thanks
Arun

Re: [PATCH 7/7] net: phy: Add pm support for scan ctrl register to bcm mdio mux

[Andrew Lunn]

On Thu, Jul 26, 2018 at 12:33:05PM -0700, Arun Parameswaran wrote:
> 
> 
> On 18-07-26 12:20 PM, Andrew Lunn wrote:
> > On Thu, Jul 26, 2018 at 11:36:24AM -0700, Arun Parameswaran wrote:
> >> Add support for saving and restoring the 'scan control' register
> >> in the Broadcom iProc mdio mux driver.
> > 
> > Hi Arun
> > 
> > I don't see anything setting this register. So why is it necessary to
> > save it over suspend/resume?
> > 
> >      Andrew
> > 
> Hi Andrew
> In the Omega SoC (only), this register was defaulted incorrectly (to use
> a test mode) by the hardware. So we added the setup of the register to
> the early bootloader (ATF).
> 
> We still need to restore this register while resuming from sleep.

Hi Arun

It is better to not rely on the bootloader and have the driver do it.
That makes it easier for people to swap to alternative bootloaders.

     Andrew

Re: [PATCH 7/7] net: phy: Add pm support for scan ctrl register to bcm mdio mux

[Arun Parameswaran]

Hi Andrew,

On 18-07-26 12:36 PM, Andrew Lunn wrote:
> On Thu, Jul 26, 2018 at 12:33:05PM -0700, Arun Parameswaran wrote:
>>
>>
>> On 18-07-26 12:20 PM, Andrew Lunn wrote:
>>> On Thu, Jul 26, 2018 at 11:36:24AM -0700, Arun Parameswaran wrote:
>>>> Add support for saving and restoring the 'scan control' register
>>>> in the Broadcom iProc mdio mux driver.
>>>
>>> Hi Arun
>>>
>>> I don't see anything setting this register. So why is it necessary to
>>> save it over suspend/resume?
>>>
>>>      Andrew
>>>
>> Hi Andrew
>> In the Omega SoC (only), this register was defaulted incorrectly (to use
>> a test mode) by the hardware. So we added the setup of the register to
>> the early bootloader (ATF).
>>
>> We still need to restore this register while resuming from sleep.
> 
> Hi Arun
> 
> It is better to not rely on the bootloader and have the driver do it.
> That makes it easier for people to swap to alternative bootloaders.
> 
>      Andrew
> 
I will make the change to set the register in the driver.

Thanks
Arun

Re: [PATCH 0/7] Add clock config and pm support to bcm iProc mdio mux

[Andrew Lunn]

On Thu, Jul 26, 2018 at 11:36:17AM -0700, Arun Parameswaran wrote:
> Hi,
> The patchset is based on David Miller's "net" repo.

Hi Arun

"net" is intended for bug fixes. This looks like new features, so
should be against net-next". Please read the netdev FAQ.

       Andrew

Re: [PATCH 0/7] Add clock config and pm support to bcm iProc mdio mux

[Arun Parameswaran]

On 18-07-26 11:52 AM, Andrew Lunn wrote:
> On Thu, Jul 26, 2018 at 11:36:17AM -0700, Arun Parameswaran wrote:
>> Hi,
>> The patchset is based on David Miller's "net" repo.
> 
> Hi Arun
> 
> "net" is intended for bug fixes. This looks like new features, so
> should be against net-next". Please read the netdev FAQ.
> 
>        Andrew
> 
Hi Andrew
The first three patches are 'fixes', and the subsequent patches
(which are new features) depend on the first 3 patches.

Since I had the 'fixes', and the dependency, I used the 'net'
repo.

Please advise if I should still use 'net-next' for all of them.

Thanks
Arun