Hi,
The patchset is based on David Miller's "net-next" 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 patchset also sets the scan control register to
disable external master access.
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.
Changes from v2:
- Addressed Andrew's comments:
- Moved to using devm_mdiobus_alloc. Added this as a separate patch.
- Changed to reverse christmas tree order for variable declaration in
the clock patch
- Addressed Florian's comments:
- Removed null checks for the clock before calling unprepare in
both clock and pm patches.
- Added check for EPROBE_DEFER when fetching the clock in the clock
patch.
- The patch to use the devm API has been added before the clock & pm
patches. This patch is now patch '5' in the series.
- Added reviewed-by tags to commit messages of patches which remain
unmodified from v2.
- Modified PM patch to use platform_get_drvdata() in suspend/resume
API's, similar to the recent fix that went in for the remove()
api.
Changes from v1:
- Addressed Andrew's comments.
- Reworked the patches to be based on 'net-next'
- Removed 'fixes' from the commit messages, the changes are related
to the new features being added.
- Maintained backward compatibility to older dt-blob's specifying
base addresse with an offset. The correction is applied in the
driver and a message is printed to update the dt-blob.
- Re-worked and re-ordered the last four patches (4-7).
- Added setting of the scan control register as a new patch
- Added a call to 'clk_prepare_enable()' in the patch that adds
the clock config support, removed the debug message when clock
is not passed.
- Simplified the pm support patch (removed the array used for the
save/restore logic).
Thanks
Arun Parameswaran (8):
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
net: phy: Disable external master access in bcm mdio mux driver
net: phy: Use devm api for mdio bus allocation in bcm 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
.../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 | 111 +++++++++++++++++++--
4 files changed, 109 insertions(+), 17 deletions(-)
--
1.9.1
Modify the base address of the mdio mux driver to point to the
start of the mdio mux block's register address space.
Signed-off-by: Arun Parameswaran <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
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
Configure the scan control register in the Broadcom iProc
mdio mux driver to disable access to external master.
In some SoC's, the scan control register defaults to an incorrect
value.
Signed-off-by: Arun Parameswaran <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 48bb74a..c36ce4b 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -22,6 +22,9 @@
#include <linux/mdio-mux.h>
#include <linux/delay.h>
+#define MDIO_SCAN_CTRL_OFFSET 0x008
+#define MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR 28
+
#define MDIO_PARAM_OFFSET 0x23c
#define MDIO_PARAM_MIIM_CYCLE 29
#define MDIO_PARAM_INTERNAL_SEL 25
@@ -53,6 +56,16 @@ struct iproc_mdiomux_desc {
struct mii_bus *mii_bus;
};
+static void mdio_mux_iproc_config(struct iproc_mdiomux_desc *md)
+{
+ u32 val;
+
+ /* Disable external mdio master access */
+ val = readl(md->base + MDIO_SCAN_CTRL_OFFSET);
+ val |= BIT(MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR);
+ writel(val, md->base + MDIO_SCAN_CTRL_OFFSET);
+}
+
static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
{
unsigned int timeout = 1000; /* loop for 1s */
@@ -216,6 +229,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
goto out_register;
}
+ mdio_mux_iproc_config(md);
+
dev_info(md->dev, "iProc mdiomux registered\n");
return 0;
--
1.9.1
Add support for suspend and resume to the Broadcom iProc mdio
mux driver.
Signed-off-by: Arun Parameswaran <[email protected]>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 5127010..4e2c51b 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -289,6 +289,32 @@ 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)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct iproc_mdiomux_desc *md = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(md->core_clk);
+
+ return 0;
+}
+
+static int mdio_mux_iproc_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct iproc_mdiomux_desc *md = platform_get_drvdata(pdev);
+
+ clk_prepare_enable(md->core_clk);
+ mdio_mux_iproc_config(md);
+
+ 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",
@@ -301,6 +327,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
Use devm_mdiobus_alloc() instead of mdiobus_alloc() in the Broadcom
iProc mdio mux driver.
Also, clear the driver data on error from probe or when remove() is
called.
Signed-off-by: Arun Parameswaran <[email protected]>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index c36ce4b..5640d78 100644
--- a/drivers/net/phy/mdio-mux-bcm-iproc.c
+++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
@@ -198,7 +198,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
return PTR_ERR(md->base);
}
- md->mii_bus = mdiobus_alloc();
+ md->mii_bus = devm_mdiobus_alloc(&pdev->dev);
if (!md->mii_bus) {
dev_err(&pdev->dev, "mdiomux bus alloc failed\n");
return -ENOMEM;
@@ -217,7 +217,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
rc = mdiobus_register(bus);
if (rc) {
dev_err(&pdev->dev, "mdiomux registration failed\n");
- goto out;
+ return rc;
}
platform_set_drvdata(pdev, md);
@@ -226,6 +226,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
&md->mux_handle, md, md->mii_bus);
if (rc) {
dev_info(md->dev, "mdiomux initialization failed\n");
+ platform_set_drvdata(pdev, NULL);
goto out_register;
}
@@ -236,8 +237,6 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
out_register:
mdiobus_unregister(bus);
-out:
- mdiobus_free(bus);
return rc;
}
@@ -247,7 +246,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
mdio_mux_uninit(md->mux_handle);
mdiobus_unregister(md->mii_bus);
- mdiobus_free(md->mii_bus);
+ platform_set_drvdata(pdev, NULL);
return 0;
}
--
1.9.1
Add clock phandle, of the core clock driving the mdio block, as an
optional property to the Broadcom iProc mdio mux.
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 <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 42 ++++++++++++++++++++++++++++++++++--
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
index 5640d78..5127010 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_SCAN_CTRL_OFFSET 0x008
#define MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR 28
@@ -49,21 +53,38 @@
#define MDIO_REG_ADDR_SPACE_SIZE 0x250
+#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(struct iproc_mdiomux_desc *md)
{
+ u32 divisor;
u32 val;
/* Disable external mdio master access */
val = readl(md->base + MDIO_SCAN_CTRL_OFFSET);
val |= BIT(MDIO_SCAN_CTRL_OVRIDE_EXT_MSTR);
writel(val, md->base + MDIO_SCAN_CTRL_OFFSET);
+
+ if (md->core_clk) {
+ /* use rate adjust regs to derrive the mdio's operating
+ * frequency from the specified core clock
+ */
+ 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)
@@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
return -ENOMEM;
}
+ md->core_clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(md->core_clk)) {
+ if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ md->core_clk = NULL;
+ } else {
+ rc = clk_prepare_enable(md->core_clk);
+ if (rc) {
+ dev_err(&pdev->dev, "failed to enable core clk\n");
+ return rc;
+ }
+ }
+
bus = md->mii_bus;
bus->priv = md;
bus->name = "iProc MDIO mux bus";
@@ -217,7 +252,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
rc = mdiobus_register(bus);
if (rc) {
dev_err(&pdev->dev, "mdiomux registration failed\n");
- return rc;
+ goto out_clk;
}
platform_set_drvdata(pdev, md);
@@ -237,6 +272,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
out_register:
mdiobus_unregister(bus);
+out_clk:
+ clk_disable_unprepare(md->core_clk);
return rc;
}
@@ -246,6 +283,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
mdio_mux_uninit(md->mux_handle);
mdiobus_unregister(md->mii_bus);
+ clk_disable_unprepare(md->core_clk);
platform_set_drvdata(pdev, NULL);
return 0;
--
1.9.1
Modify the register offsets in the Broadcom iProc mdio mux to start
from the top of the register address space.
Earlier, the base address pointed to the end 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.
Signed-off-by: Arun Parameswaran <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/mdio-mux-bcm-iproc.c | 20 +++++++++++++++-----
1 file changed, 15 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..48bb74a 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,20 +30,22 @@
#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
#define EXT_BUS_START_ADDR 16
+#define MDIO_REG_ADDR_SPACE_SIZE 0x250
+
struct iproc_mdiomux_desc {
void *mux_handle;
void __iomem *base;
@@ -169,6 +171,14 @@ 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) {
+ /* For backward compatibility in case the
+ * base address is specified with an offset.
+ */
+ dev_info(&pdev->dev, "fix base address in dt-blob\n");
+ res->start &= ~0xfff;
+ res->end = res->start + MDIO_REG_ADDR_SPACE_SIZE - 1;
+ }
md->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(md->base)) {
dev_err(&pdev->dev, "failed to ioremap register\n");
--
1.9.1
Modify the base address of the Broadcom iProc MDIO mux driver to
point to the start of the block's register address space.
Signed-off-by: Arun Parameswaran <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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
On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> Use devm_mdiobus_alloc() instead of mdiobus_alloc() in the Broadcom
> iProc mdio mux driver.
>
> Also, clear the driver data on error from probe or when remove() is
> called.
>
> Signed-off-by: Arun Parameswaran <[email protected]>
> ---
> drivers/net/phy/mdio-mux-bcm-iproc.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/phy/mdio-mux-bcm-iproc.c b/drivers/net/phy/mdio-mux-bcm-iproc.c
> index c36ce4b..5640d78 100644
> --- a/drivers/net/phy/mdio-mux-bcm-iproc.c
> +++ b/drivers/net/phy/mdio-mux-bcm-iproc.c
> @@ -198,7 +198,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> return PTR_ERR(md->base);
> }
>
> - md->mii_bus = mdiobus_alloc();
> + md->mii_bus = devm_mdiobus_alloc(&pdev->dev);
> if (!md->mii_bus) {
> dev_err(&pdev->dev, "mdiomux bus alloc failed\n");
> return -ENOMEM;
> @@ -217,7 +217,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> rc = mdiobus_register(bus);
> if (rc) {
> dev_err(&pdev->dev, "mdiomux registration failed\n");
> - goto out;
> + return rc;
> }
>
> platform_set_drvdata(pdev, md);
> @@ -226,6 +226,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> &md->mux_handle, md, md->mii_bus);
> if (rc) {
> dev_info(md->dev, "mdiomux initialization failed\n");
> + platform_set_drvdata(pdev, NULL);
This is minor, but I don't think this is necessary, the core device
driver layer does that already.
> goto out_register;
> }
>
> @@ -236,8 +237,6 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>
> out_register:
> mdiobus_unregister(bus);
> -out:
> - mdiobus_free(bus);
> return rc;
> }
>
> @@ -247,7 +246,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
>
> mdio_mux_uninit(md->mux_handle);
> mdiobus_unregister(md->mii_bus);
> - mdiobus_free(md->mii_bus);
> + platform_set_drvdata(pdev, NULL);
Likewise.
>
> return 0;
> }
>
--
Florian
On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> Add support for suspend and resume to the Broadcom iProc mdio
> mux driver.
>
> Signed-off-by: Arun Parameswaran <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> 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 <[email protected]>
> ---
[snip]
> static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
> @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> return -ENOMEM;
> }
>
> + md->core_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(md->core_clk)) {
> + if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + md->core_clk = NULL;
I would simplify this a bit:
if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)
return PTR_ERR(md->core_clk);
else
md->core_clk = NULL;
rc = clk_prepare_enable(md->core_clk);
and continue that way.
> + } else {
> + rc = clk_prepare_enable(md->core_clk);
> + if (rc) {
> + dev_err(&pdev->dev, "failed to enable core clk\n");
> + return rc;
> + }
> + }
> +
> bus = md->mii_bus;
> bus->priv = md;
> bus->name = "iProc MDIO mux bus";
> @@ -217,7 +252,7 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> rc = mdiobus_register(bus);
> if (rc) {
> dev_err(&pdev->dev, "mdiomux registration failed\n");
> - return rc;
> + goto out_clk;
> }
>
> platform_set_drvdata(pdev, md);
> @@ -237,6 +272,8 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>
> out_register:
> mdiobus_unregister(bus);
> +out_clk:
> + clk_disable_unprepare(md->core_clk);
> return rc;
> }
>
> @@ -246,6 +283,7 @@ static int mdio_mux_iproc_remove(struct platform_device *pdev)
>
> mdio_mux_uninit(md->mux_handle);
> mdiobus_unregister(md->mii_bus);
> + clk_disable_unprepare(md->core_clk);
> platform_set_drvdata(pdev, NULL);
>
> return 0;
>
--
Florian
On Wed, Aug 01, 2018 at 11:40:33AM -0700, Florian Fainelli wrote:
> On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
> > static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
> > @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
> > return -ENOMEM;
> > }
> >
> > + md->core_clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(md->core_clk)) {
> > + if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > +
> > + md->core_clk = NULL;
>
> I would simplify this a bit:
>
> if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)
Even better is:
if (md->core_clk == ERR_PTR(-EPROBE_DEFER))
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
Hi Russell, Florian,
On 18-08-01 11:46 AM, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2018 at 11:40:33AM -0700, Florian Fainelli wrote:
>> On 08/01/2018 10:56 AM, Arun Parameswaran wrote:
>>> static int iproc_mdio_wait_for_idle(void __iomem *base, bool result)
>>> @@ -204,6 +225,20 @@ static int mdio_mux_iproc_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>> }
>>>
>>> + md->core_clk = devm_clk_get(&pdev->dev, NULL);
>>> + if (IS_ERR(md->core_clk)) {
>>> + if (PTR_ERR(md->core_clk) == -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>> +
>>> + md->core_clk = NULL;
>>
>> I would simplify this a bit:
>>
>> if (IS_ERR(md->core_clk) && PTR_ERR(md->core_clk) == -EPROBE_DEFER)
>
> Even better is:
>
> if (md->core_clk == ERR_PTR(-EPROBE_DEFER))
>
The clock is an optional parameter. But I realize, that I did not handle all
the error cases.
My intention was to proceed with probe if the clock is not found in the DT.
If the clock is specified in the DT, make use of it.
So, should I modify to something like:
md->core_clk = devm_clk_get(&pdev->dev, NULL);
if (md->core_clk == ERR_PTR(-ENOENT)) {
/* clock is optional, without it the default
* rate divider register values will be used
*/
md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
return PTR_ERR(md->core_clk);
} else {
rc = clk_prepare_enable(md->core_clk);
...
}
Thanks
Arun
> My intention was to proceed with probe if the clock is not found in the DT.
> If the clock is specified in the DT, make use of it.
You might want to look at what phy_optional_get() does. If the phy
does not exist in DT, it does not return an error, but a NULL phy.
You can pass NULL to all the other calls generic phy_ calls. If there
is a real error, it returns it. That makes the driver code much
simpler.
You might want to consider adding clk_optional_get() and
devm_clk_optional_get().
Andrew
On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
> You might want to consider adding clk_optional_get() and
> devm_clk_optional_get().
I think there's attempts to add such APIs but I don't think it's
trivial - it seems to require a _lot_ of discussion.
I think part of that is because of the quirky use of error codes.
If you look at clk_get(), it calls __of_clk_get_by_name() which
returns:
-ENOENT if DT is disabled
-ENOENT if the device has no DT node
-EPROBE_DEFER if the lookup in DT succeeds but there's no registered
clock
-EINVAL if the device has a DT node but the lookup of the name
failed (in otherwords, the optional clock was omitted)
-ENOENT if the clocks = property has not enough clocks for the
clock-names property
-ENOMEM if we fail to allocate the clk
-ENOENT if __clk_get() fails
or any other error code returned via of_clk_provider's ->get() method.
The use of -EINVAL, one of the most common error codes, makes it
difficult to be sure that the clock is not specified in DT.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
On Wed, Aug 01, 2018 at 09:23:14PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
> > You might want to consider adding clk_optional_get() and
> > devm_clk_optional_get().
>
> I think there's attempts to add such APIs but I don't think it's
> trivial - it seems to require a _lot_ of discussion.
>
> I think part of that is because of the quirky use of error codes.
> If you look at clk_get(), it calls __of_clk_get_by_name() which
> returns:
>
> -ENOENT if DT is disabled
> -ENOENT if the device has no DT node
> -EPROBE_DEFER if the lookup in DT succeeds but there's no registered
> clock
> -EINVAL if the device has a DT node but the lookup of the name
> failed (in otherwords, the optional clock was omitted)
> -ENOENT if the clocks = property has not enough clocks for the
> clock-names property
> -ENOMEM if we fail to allocate the clk
> -ENOENT if __clk_get() fails
That makes it hard. I added phy_optional_get() early on, when the
error cases were simple. Hopefully they remain simple...
Andrew
Hi Andrew, Russell,
On 18-08-01 01:38 PM, Andrew Lunn wrote:
> On Wed, Aug 01, 2018 at 09:23:14PM +0100, Russell King - ARM Linux wrote:
>> On Wed, Aug 01, 2018 at 10:07:12PM +0200, Andrew Lunn wrote:
>>> You might want to consider adding clk_optional_get() and
>>> devm_clk_optional_get().
>>
>> I think there's attempts to add such APIs but I don't think it's
>> trivial - it seems to require a _lot_ of discussion.
>>
>> I think part of that is because of the quirky use of error codes.
>> If you look at clk_get(), it calls __of_clk_get_by_name() which
>> returns:
>>
>> -ENOENT if DT is disabled
>> -ENOENT if the device has no DT node
>> -EPROBE_DEFER if the lookup in DT succeeds but there's no registered
>> clock
>> -EINVAL if the device has a DT node but the lookup of the name
>> failed (in otherwords, the optional clock was omitted)
>> -ENOENT if the clocks = property has not enough clocks for the
>> clock-names property
>> -ENOMEM if we fail to allocate the clk
>> -ENOENT if __clk_get() fails
>
> That makes it hard. I added phy_optional_get() early on, when the
> error cases were simple. Hopefully they remain simple...
>
> Andrew
>
I traced the devm_clk_get() and ran into various error codes, so I
figured I should return error on all the other errors (like ENOMEM, EINVAL
& EPROBE_DEFER) but 'ENOENT'.
But, looking at the errors from Russell's email, it looks like I
should look for both 'ENOENT' and 'EINVAL' and consider both these
errors as 'clock not specified' ?
Can I do something like this in the code for this patchset:
md->core_clk = devm_clk_get(&pdev->dev, NULL);
if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
/* clock is optional, without it the default
* rate divider register values will be used
*/
md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
return PTR_ERR(md->core_clk);
} else {
rc = clk_prepare_enable(md->core_clk);
...
}
Thanks
Arun
> md->core_clk = devm_clk_get(&pdev->dev, NULL);
> if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
> /* clock is optional, without it the default
> * rate divider register values will be used
> */
> md->core_clk = NULL;
> } else if (IS_ERR(md->core_clk)) {
> return PTR_ERR(md->core_clk);
> } else {
> rc = clk_prepare_enable(md->core_clk);
> ...
> }
As Florian pointed out, the clk_ API is happy to take a NULL pointer
for a clock. So you don't need this last else.
Andrew
Hi Andrew,
On 18-08-01 02:55 PM, Andrew Lunn wrote:
>> md->core_clk = devm_clk_get(&pdev->dev, NULL);
>> if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
>> /* clock is optional, without it the default
>> * rate divider register values will be used
>> */
>> md->core_clk = NULL;
>> } else if (IS_ERR(md->core_clk)) {
>> return PTR_ERR(md->core_clk);
>> } else {
>> rc = clk_prepare_enable(md->core_clk);
>> ...
>> }
>
> As Florian pointed out, the clk_ API is happy to take a NULL pointer
> for a clock. So you don't need this last else.
>
> Andrew
>
I do return with an error from the probe if the clk_prepare_enable() fails,
so I was calling the prepare with a valid clock.
In the other places where I used the clock, the return values were being
ignored.
The 'else' part should have been, I apologize for the confusion.
if (md->core_clk == ERR_PTR(-ENOENT) || md->core_clk == ERR_PTR(-EINVAL)) {
/* clock is optional, without it the default
* rate divider register values will be used
*/
md->core_clk = NULL;
} else if (IS_ERR(md->core_clk)) {
return PTR_ERR(md->core_clk);
} else {
rc = clk_prepare_enable(md->core_clk);
if (rc) {
dev_err(&pdev->dev, "failed to enable core clk\n");
return rc;
}
}
Thanks
Arun
> > As Florian pointed out, the clk_ API is happy to take a NULL pointer
> > for a clock. So you don't need this last else.
> >
> > Andrew
> >
> I do return with an error from the probe if the clk_prepare_enable() fails,
> so I was calling the prepare with a valid clock.
How many times do we need to say it?
NULL is a valid clock.
int clk_prepare(struct clk *clk)
{
if (!clk)
return 0;
return clk_core_prepare_lock(clk->core);
}
int clk_enable(struct clk *clk)
{
if (!clk)
return 0;
return clk_core_enable_lock(clk->core);
}
clk_prepare_enable(NULL) returns 0, which is not an error.
Andrew