| Message ID | 1533146186-8374-8-git-send-email-arun.parameswaran@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add clock config and pm support to bcm iProc mdio mux | expand |
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 <arun.parameswaran@broadcom.com> > --- [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; >
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))
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.
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
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;
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 | 42 ++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)