On the CPSW and ICSS peripherals, there is a possibility that the MDIO
interface returns corrupt data on MDIO reads or writes incorrect data
on MDIO writes. There is also a possibility for the MDIO interface to
become unavailable until the next peripheral reset.
The workaround is to configure the MDIO in manual mode and disable the
MDIO state machine and emulate the MDIO protocol by reading and writing
appropriate fields in MDIO_MANUAL_IF_REG register of the MDIO controller
to manipulate the MDIO clock and data pins.
More details about the errata i2329 and the workaround is available in:
https://www.ti.com/lit/er/sprz487a/sprz487a.pdf
Add implementation to disable MDIO state machine, configure MDIO in manual
mode and achieve MDIO read and writes via MDIO Bitbanging
Signed-off-by: Ravi Gunasekaran <[email protected]>
---
Changelog:
v1 -> v2:
1. Use the existing MDIO bitbang framework
v1: https://lore.kernel.org/netdev/[email protected]/
drivers/net/ethernet/ti/davinci_mdio.c | 255 +++++++++++++++++++++++--
1 file changed, 244 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index ea3772618043..387244e6332b 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -26,6 +26,8 @@
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/mdio-bitbang.h>
+#include <linux/sys_soc.h>
/*
* This timeout definition is a worst-case ultra defensive measure against
@@ -41,6 +43,7 @@
struct davinci_mdio_of_param {
int autosuspend_delay_ms;
+ bool manual_mode;
};
struct davinci_mdio_regs {
@@ -49,6 +52,15 @@ struct davinci_mdio_regs {
#define CONTROL_IDLE BIT(31)
#define CONTROL_ENABLE BIT(30)
#define CONTROL_MAX_DIV (0xffff)
+#define CONTROL_CLKDIV GENMASK(15, 0)
+
+#define MDIO_MAN_MDCLK_O BIT(2)
+#define MDIO_MAN_OE BIT(1)
+#define MDIO_MAN_PIN BIT(0)
+#define MDIO_MANUALMODE BIT(31)
+
+#define MDIO_PIN 0
+
u32 alive;
u32 link;
@@ -59,7 +71,9 @@ struct davinci_mdio_regs {
u32 userintmasked;
u32 userintmaskset;
u32 userintmaskclr;
- u32 __reserved_1[20];
+ u32 manualif;
+ u32 poll;
+ u32 __reserved_1[18];
struct {
u32 access;
@@ -90,6 +104,8 @@ struct davinci_mdio_data {
*/
bool skip_scan;
u32 clk_div;
+ bool manual_mode;
+ struct mdiobb_ctrl bb_ctrl;
};
static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
@@ -128,9 +144,136 @@ static void davinci_mdio_enable(struct davinci_mdio_data *data)
writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
}
-static int davinci_mdio_reset(struct mii_bus *bus)
+static void davinci_mdio_disable(struct davinci_mdio_data *data)
+{
+ u32 reg;
+
+ /* Disable MDIO state machine */
+ reg = readl(&data->regs->control);
+
+ reg &= ~CONTROL_CLKDIV;
+ reg |= data->clk_div;
+
+ reg &= ~CONTROL_ENABLE;
+ writel(reg, &data->regs->control);
+}
+
+static void davinci_mdio_enable_manual_mode(struct davinci_mdio_data *data)
+{
+ u32 reg;
+ /* set manual mode */
+ reg = readl(&data->regs->poll);
+ reg |= MDIO_MANUALMODE;
+ writel(reg, &data->regs->poll);
+}
+
+static void davinci_set_mdc(struct mdiobb_ctrl *ctrl, int level)
+{
+ struct davinci_mdio_data *data;
+ u32 reg;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+ reg = readl(&data->regs->manualif);
+
+ if (level)
+ reg |= MDIO_MAN_MDCLK_O;
+ else
+ reg &= ~MDIO_MAN_MDCLK_O;
+
+ writel(reg, &data->regs->manualif);
+}
+
+static void davinci_set_mdio_dir(struct mdiobb_ctrl *ctrl, int output)
+{
+ struct davinci_mdio_data *data;
+ u32 reg;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+ reg = readl(&data->regs->manualif);
+
+ if (output)
+ reg |= MDIO_MAN_OE;
+ else
+ reg &= ~MDIO_MAN_OE;
+
+ writel(reg, &data->regs->manualif);
+}
+
+static void davinci_set_mdio_data(struct mdiobb_ctrl *ctrl, int value)
+{
+ struct davinci_mdio_data *data;
+ u32 reg;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+ reg = readl(&data->regs->manualif);
+
+ if (value)
+ reg |= MDIO_MAN_PIN;
+ else
+ reg &= ~MDIO_MAN_PIN;
+
+ writel(reg, &data->regs->manualif);
+}
+
+static int davinci_get_mdio_data(struct mdiobb_ctrl *ctrl)
+{
+ struct davinci_mdio_data *data;
+ unsigned long reg;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+ reg = readl(&data->regs->manualif);
+ return test_bit(MDIO_PIN, ®);
+}
+
+static int davinci_mdiobb_read(struct mii_bus *bus, int phy, int reg)
+{
+ int ret;
+ struct mdiobb_ctrl *ctrl = bus->priv;
+ struct davinci_mdio_data *data;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+
+ if (phy & ~PHY_REG_MASK || reg & ~PHY_ID_MASK)
+ return -EINVAL;
+
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret < 0)
+ return ret;
+
+ ret = mdiobb_read(bus, phy, reg);
+
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
+
+ return ret;
+}
+
+static int davinci_mdiobb_write(struct mii_bus *bus, int phy, int reg,
+ u16 val)
+{
+ int ret;
+ struct mdiobb_ctrl *ctrl = bus->priv;
+ struct davinci_mdio_data *data;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+
+ if (phy & ~PHY_REG_MASK || reg & ~PHY_ID_MASK)
+ return -EINVAL;
+
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret < 0)
+ return ret;
+
+ ret = mdiobb_write(bus, phy, reg, val);
+
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
+
+ return ret;
+}
+
+static int davinci_mdio_common_reset(struct davinci_mdio_data *data)
{
- struct davinci_mdio_data *data = bus->priv;
u32 phy_mask, ver;
int ret;
@@ -138,6 +281,11 @@ static int davinci_mdio_reset(struct mii_bus *bus)
if (ret < 0)
return ret;
+ if (data->manual_mode) {
+ davinci_mdio_disable(data);
+ davinci_mdio_enable_manual_mode(data);
+ }
+
/* wait for scan logic to settle */
msleep(PHY_MAX_ADDR * data->access_time);
@@ -171,6 +319,23 @@ static int davinci_mdio_reset(struct mii_bus *bus)
return 0;
}
+static int davinci_mdio_reset(struct mii_bus *bus)
+{
+ struct davinci_mdio_data *data = bus->priv;
+
+ return davinci_mdio_common_reset(data);
+}
+
+static int davinci_mdiobb_reset(struct mii_bus *bus)
+{
+ struct mdiobb_ctrl *ctrl = bus->priv;
+ struct davinci_mdio_data *data;
+
+ data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
+
+ return davinci_mdio_common_reset(data);
+}
+
/* wait until hardware is ready for another user access */
static inline int wait_for_user_access(struct davinci_mdio_data *data)
{
@@ -319,6 +484,28 @@ static int davinci_mdio_probe_dt(struct mdio_platform_data *data,
}
#if IS_ENABLED(CONFIG_OF)
+struct k3_mdio_soc_data {
+ bool manual_mode;
+};
+
+static const struct k3_mdio_soc_data am65_mdio_soc_data = {
+ .manual_mode = true,
+};
+
+static const struct soc_device_attribute k3_mdio_socinfo[] = {
+ { .family = "AM62X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+ { .family = "AM64X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+ { .family = "AM64X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
+ { .family = "AM65X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+ { .family = "AM65X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
+ { .family = "J7200", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+ { .family = "J7200", .revision = "SR2.0", .data = &am65_mdio_soc_data },
+ { .family = "J721E", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+ { .family = "J721E", .revision = "SR2.0", .data = &am65_mdio_soc_data },
+ { .family = "J721S2", .revision = "SR1.0", .data = &am65_mdio_soc_data},
+ { /* sentinel */ },
+};
+
static const struct davinci_mdio_of_param of_cpsw_mdio_data = {
.autosuspend_delay_ms = 100,
};
@@ -331,6 +518,14 @@ static const struct of_device_id davinci_mdio_of_mtable[] = {
MODULE_DEVICE_TABLE(of, davinci_mdio_of_mtable);
#endif
+static const struct mdiobb_ops davinci_mdiobb_ops = {
+ .owner = THIS_MODULE,
+ .set_mdc = davinci_set_mdc,
+ .set_mdio_dir = davinci_set_mdio_dir,
+ .set_mdio_data = davinci_set_mdio_data,
+ .get_mdio_data = davinci_get_mdio_data,
+};
+
static int davinci_mdio_probe(struct platform_device *pdev)
{
struct mdio_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -340,12 +535,30 @@ static int davinci_mdio_probe(struct platform_device *pdev)
struct phy_device *phy;
int ret, addr;
int autosuspend_delay_ms = -1;
+ const struct soc_device_attribute *soc_match_data;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
- data->bus = devm_mdiobus_alloc(dev);
+ data->manual_mode = false;
+ data->bb_ctrl.ops = &davinci_mdiobb_ops;
+
+ if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+ soc_match_data = soc_device_match(k3_mdio_socinfo);
+ if (soc_match_data && soc_match_data->data) {
+ const struct k3_mdio_soc_data *socdata =
+ soc_match_data->data;
+
+ data->manual_mode = socdata->manual_mode;
+ }
+ }
+
+ if (data->manual_mode)
+ data->bus = alloc_mdio_bitbang(&data->bb_ctrl);
+ else
+ data->bus = devm_mdiobus_alloc(dev);
+
if (!data->bus) {
dev_err(dev, "failed to alloc mii bus\n");
return -ENOMEM;
@@ -371,11 +584,20 @@ static int davinci_mdio_probe(struct platform_device *pdev)
}
data->bus->name = dev_name(dev);
- data->bus->read = davinci_mdio_read;
- data->bus->write = davinci_mdio_write;
- data->bus->reset = davinci_mdio_reset;
+
+ if (data->manual_mode) {
+ data->bus->read = davinci_mdiobb_read;
+ data->bus->write = davinci_mdiobb_write;
+ data->bus->reset = davinci_mdiobb_reset;
+
+ dev_info(dev, "Configuring MDIO in manual mode\n");
+ } else {
+ data->bus->read = davinci_mdio_read;
+ data->bus->write = davinci_mdio_write;
+ data->bus->reset = davinci_mdio_reset;
+ data->bus->priv = data;
+ }
data->bus->parent = dev;
- data->bus->priv = data;
data->clk = devm_clk_get(dev, "fck");
if (IS_ERR(data->clk)) {
@@ -433,9 +655,13 @@ static int davinci_mdio_remove(struct platform_device *pdev)
{
struct davinci_mdio_data *data = platform_get_drvdata(pdev);
- if (data->bus)
+ if (data->bus) {
mdiobus_unregister(data->bus);
+ if (data->manual_mode)
+ free_mdio_bitbang(data->bus);
+ }
+
pm_runtime_dont_use_autosuspend(&pdev->dev);
pm_runtime_disable(&pdev->dev);
@@ -452,7 +678,9 @@ static int davinci_mdio_runtime_suspend(struct device *dev)
ctrl = readl(&data->regs->control);
ctrl &= ~CONTROL_ENABLE;
writel(ctrl, &data->regs->control);
- wait_for_idle(data);
+
+ if (!data->manual_mode)
+ wait_for_idle(data);
return 0;
}
@@ -461,7 +689,12 @@ static int davinci_mdio_runtime_resume(struct device *dev)
{
struct davinci_mdio_data *data = dev_get_drvdata(dev);
- davinci_mdio_enable(data);
+ if (data->manual_mode) {
+ davinci_mdio_disable(data);
+ davinci_mdio_enable_manual_mode(data);
+ } else {
+ davinci_mdio_enable(data);
+ }
return 0;
}
#endif
--
2.17.1
> +static int davinci_mdiobb_read(struct mii_bus *bus, int phy, int reg)
> +{
> + int ret;
> + struct mdiobb_ctrl *ctrl = bus->priv;
> + struct davinci_mdio_data *data;
> +
> + data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
> +
> + if (phy & ~PHY_REG_MASK || reg & ~PHY_ID_MASK)
> + return -EINVAL;
You don't need this. Leave it up to the bit banging code to do the
validation. This also breaks C45, which the bit banging code can do,
and it looks like the hardware cannot.
> +
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = mdiobb_read(bus, phy, reg);
> +
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
Once you take the validation out, this function then all becomes about
runtime power management. Should the bit banging core actually be
doing this? It seems like it is something which could be useful for
other devices.
struct mii_bus has a parent member. If set, you could apply these run
time PM functions to that. Please add a patch to modify the core bit
banging code, and then you should be able to remove these helpers.
> static int davinci_mdio_probe(struct platform_device *pdev)
> {
> struct mdio_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -340,12 +535,30 @@ static int davinci_mdio_probe(struct platform_device *pdev)
> struct phy_device *phy;
> int ret, addr;
> int autosuspend_delay_ms = -1;
> + const struct soc_device_attribute *soc_match_data;
netdev uses reverse christmas tree. Variables should be sorted longest
first, shortest last.
Andrew
On 11/08/22 6:00 am, Andrew Lunn wrote:
>> +static int davinci_mdiobb_read(struct mii_bus *bus, int phy, int reg)
>> +{
>> + int ret;
>> + struct mdiobb_ctrl *ctrl = bus->priv;
>> + struct davinci_mdio_data *data;
>> +
>> + data = container_of(ctrl, struct davinci_mdio_data, bb_ctrl);
>> +
>> + if (phy & ~PHY_REG_MASK || reg & ~PHY_ID_MASK)
>> + return -EINVAL;
>
> You don't need this. Leave it up to the bit banging code to do the
> validation. This also breaks C45, which the bit banging code can do,
> and it looks like the hardware cannot.
I will remove these validation.
>> +
>> + ret = pm_runtime_resume_and_get(data->dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = mdiobb_read(bus, phy, reg);
>> +
>> + pm_runtime_mark_last_busy(data->dev);
>> + pm_runtime_put_autosuspend(data->dev);
>
> Once you take the validation out, this function then all becomes about
> runtime power management. Should the bit banging core actually be
> doing this? It seems like it is something which could be useful for
> other devices.
>
> struct mii_bus has a parent member. If set, you could apply these run
> time PM functions to that. Please add a patch to modify the core bit
> banging code, and then you should be able to remove these helpers.
Devices may or may not be configured for runtime autosuspend, and
perhaps may not even use runtime PM. pm_runtime_enabled() and the
autosuspend configuration could be addressed by checking against
dev->power.use_autosuspend flag. But if the runtime PM functions are
added to the bit banging core, would it not restrict the usage of
pm_runtime_put_*() variants for others?
There is atleast one device sh_eth, which is not configured for
autosuspend but uses the bit bang core in sh_mdiobb_read() and invokes
regular runtime PM functions.
>> static int davinci_mdio_probe(struct platform_device *pdev)
>> {
>> struct mdio_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> @@ -340,12 +535,30 @@ static int davinci_mdio_probe(struct platform_device *pdev)
>> struct phy_device *phy;
>> int ret, addr;
>> int autosuspend_delay_ms = -1;
>> + const struct soc_device_attribute *soc_match_data;
>
> netdev uses reverse christmas tree. Variables should be sorted longest
> first, shortest last.
Noted. I will fix this as well.
--
Regards,
Ravi
> Devices may or may not be configured for runtime autosuspend, and perhaps
> may not even use runtime PM. pm_runtime_enabled() and the autosuspend
> configuration could be addressed by checking against
> dev->power.use_autosuspend flag. But if the runtime PM functions are added
> to the bit banging core, would it not restrict the usage of
> pm_runtime_put_*() variants for others?
My assumption is, any calls to pm_runtime_* functions will effectively
do nothing if the driver does not have support for it. I could be
wrong about this, and it jumps through a NULL pointer and explodes,
but that would be a bad design.
> There is atleast one device sh_eth, which is not configured for autosuspend
> but uses the bit bang core in sh_mdiobb_read() and invokes regular runtime
> PM functions.
And that is the point of moving it into the core. It would of just
worked for you.
If you don't feel comfortable with making this unconditional, please
put runtime pm enabled version of mdiobb_read/mdiobb_write() in the
core and swap sh_eth and any other drivers to using them.
Andrew
>> There is atleast one device sh_eth, which is not configured for autosuspend
>> but uses the bit bang core in sh_mdiobb_read() and invokes regular runtime
>> PM functions.
>
> And that is the point of moving it into the core. It would of just
> worked for you.
>
> If you don't feel comfortable with making this unconditional, please
> put runtime pm enabled version of mdiobb_read/mdiobb_write() in the
> core and swap sh_eth and any other drivers to using them.
>
sh_eth is not configured for autosuspend and uses only pm_runtime_put().
davinci_mdio is configured for autosuspend and it must invoke
pm_runtime_mark_last_busy() before calling pm_runtime_put_autosuspend().
So it looks like, there needs to be a runtime PM version of
mdiobb_read/mdiobb_write() for each pm_runtime_put_*(). As of now, it's
only sh_eth which is currently using runtime PM and davinci_mdio would
be the next one. So at least in this case, two variants of
mdiobb_read/mdiobb_write() could be added at the moment. By checking
against the dev->power.use_autosuspend flag, it is possible to support
both via a single version.
That being said, I'm quite inclined towards the existing implementation,
where drivers can have wrappers written around
mdiobb_read/mdiobb_write(). But I might be failing to see the broader
picture. If having multiple runtime PM versions of
mdiobb_read/mdiobb_write() benefits many other future drivers, then I
will go ahead and add the variant(s) in the bitbang core.
Please provide your views on this. Your inputs on the next course of
action would be helpful.
--
Regards,
Ravi
> sh_eth is not configured for autosuspend and uses only pm_runtime_put().
I don't know the runtime power management code very well. We should
probably ask somebody how does. However:
https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L168
This suggests it should be safe to perform an auto suspend on a device
which is not configured for auto suspend. To me, it looks like is
should directly suspend.
Devices are in a tree. If you suspend a leaf, you can walk up the tree
and suspend its parent as well, if it is not needed. Similarly, if you
wake a leaf, you need its parents awake as well, so you need to walk
up the tree and wake them. In order for this to work reliably, i
expect runtime PM to be very tolerant. If a device is not configured
for runtime PM, actions should be a NOP. If it is not configured for
auto suspend, and you ask it to auto suspend, to me, it would make
sense for it to immediately suspend, etc.
> Please provide your views on this. Your inputs on the next course of action
> would be helpful.
I would suggest you talk to somebody who knows about runtime PM.
Andrew
Hi Ravi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Ravi-Gunasekaran/net-ethernet-ti-davinci_mdio-Add-workaround-for-errata-i2329/20220810-191718
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git f86d1fbbe7858884d6754534a0afbb74fc30bc26
config: arm-randconfig-r026-20220810 (https://download.01.org/0day-ci/archive/20220813/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/c62c93111418d5468f6add98d244f0a594dbe352
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ravi-Gunasekaran/net-ethernet-ti-davinci_mdio-Add-workaround-for-errata-i2329/20220810-191718
git checkout c62c93111418d5468f6add98d244f0a594dbe352
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/net/ethernet/ti/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/net/ethernet/ti/davinci_mdio.c:548:37: error: use of undeclared identifier 'k3_mdio_socinfo'
soc_match_data = soc_device_match(k3_mdio_socinfo);
^
>> drivers/net/ethernet/ti/davinci_mdio.c:553:31: error: incomplete definition of type 'struct k3_mdio_soc_data'
data->manual_mode = socdata->manual_mode;
~~~~~~~^
drivers/net/ethernet/ti/davinci_mdio.c:550:17: note: forward declaration of 'struct k3_mdio_soc_data'
const struct k3_mdio_soc_data *socdata =
^
2 errors generated.
vim +/k3_mdio_socinfo +548 drivers/net/ethernet/ti/davinci_mdio.c
528
529 static int davinci_mdio_probe(struct platform_device *pdev)
530 {
531 struct mdio_platform_data *pdata = dev_get_platdata(&pdev->dev);
532 struct device *dev = &pdev->dev;
533 struct davinci_mdio_data *data;
534 struct resource *res;
535 struct phy_device *phy;
536 int ret, addr;
537 int autosuspend_delay_ms = -1;
538 const struct soc_device_attribute *soc_match_data;
539
540 data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
541 if (!data)
542 return -ENOMEM;
543
544 data->manual_mode = false;
545 data->bb_ctrl.ops = &davinci_mdiobb_ops;
546
547 if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> 548 soc_match_data = soc_device_match(k3_mdio_socinfo);
549 if (soc_match_data && soc_match_data->data) {
550 const struct k3_mdio_soc_data *socdata =
551 soc_match_data->data;
552
> 553 data->manual_mode = socdata->manual_mode;
554 }
555 }
556
557 if (data->manual_mode)
558 data->bus = alloc_mdio_bitbang(&data->bb_ctrl);
559 else
560 data->bus = devm_mdiobus_alloc(dev);
561
562 if (!data->bus) {
563 dev_err(dev, "failed to alloc mii bus\n");
564 return -ENOMEM;
565 }
566
567 if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
568 const struct davinci_mdio_of_param *of_mdio_data;
569
570 ret = davinci_mdio_probe_dt(&data->pdata, pdev);
571 if (ret)
572 return ret;
573 snprintf(data->bus->id, MII_BUS_ID_SIZE, "%s", pdev->name);
574
575 of_mdio_data = of_device_get_match_data(&pdev->dev);
576 if (of_mdio_data) {
577 autosuspend_delay_ms =
578 of_mdio_data->autosuspend_delay_ms;
579 }
580 } else {
581 data->pdata = pdata ? (*pdata) : default_pdata;
582 snprintf(data->bus->id, MII_BUS_ID_SIZE, "%s-%x",
583 pdev->name, pdev->id);
584 }
585
586 data->bus->name = dev_name(dev);
587
588 if (data->manual_mode) {
589 data->bus->read = davinci_mdiobb_read;
590 data->bus->write = davinci_mdiobb_write;
591 data->bus->reset = davinci_mdiobb_reset;
592
593 dev_info(dev, "Configuring MDIO in manual mode\n");
594 } else {
595 data->bus->read = davinci_mdio_read;
596 data->bus->write = davinci_mdio_write;
597 data->bus->reset = davinci_mdio_reset;
598 data->bus->priv = data;
599 }
600 data->bus->parent = dev;
601
602 data->clk = devm_clk_get(dev, "fck");
603 if (IS_ERR(data->clk)) {
604 dev_err(dev, "failed to get device clock\n");
605 return PTR_ERR(data->clk);
606 }
607
608 dev_set_drvdata(dev, data);
609 data->dev = dev;
610
611 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
612 if (!res)
613 return -EINVAL;
614 data->regs = devm_ioremap(dev, res->start, resource_size(res));
615 if (!data->regs)
616 return -ENOMEM;
617
618 davinci_mdio_init_clk(data);
619
620 pm_runtime_set_autosuspend_delay(&pdev->dev, autosuspend_delay_ms);
621 pm_runtime_use_autosuspend(&pdev->dev);
622 pm_runtime_enable(&pdev->dev);
623
624 /* register the mii bus
625 * Create PHYs from DT only in case if PHY child nodes are explicitly
626 * defined to support backward compatibility with DTs which assume that
627 * Davinci MDIO will always scan the bus for PHYs detection.
628 */
629 if (dev->of_node && of_get_child_count(dev->of_node))
630 data->skip_scan = true;
631
632 ret = of_mdiobus_register(data->bus, dev->of_node);
633 if (ret)
634 goto bail_out;
635
636 /* scan and dump the bus */
637 for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
638 phy = mdiobus_get_phy(data->bus, addr);
639 if (phy) {
640 dev_info(dev, "phy[%d]: device %s, driver %s\n",
641 phy->mdio.addr, phydev_name(phy),
642 phy->drv ? phy->drv->name : "unknown");
643 }
644 }
645
646 return 0;
647
648 bail_out:
649 pm_runtime_dont_use_autosuspend(&pdev->dev);
650 pm_runtime_disable(&pdev->dev);
651 return ret;
652 }
653
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 12/08/22 9:24 pm, Andrew Lunn wrote:
>> sh_eth is not configured for autosuspend and uses only pm_runtime_put().
>
> I don't know the runtime power management code very well. We should
> probably ask somebody how does. However:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L168
>
> This suggests it should be safe to perform an auto suspend on a device
> which is not configured for auto suspend. To me, it looks like is
> should directly suspend.
I checked the pm_runtime_put() and pm_runtime_put_autosuspend() and I
have quite a few unanswered questions. I do not have much knowledge
about the runtime PM, so I'm not confident about adding the runtime PM
mdiobb_read/mdiobb_write variants in the mdio bitbang core. I would
rather take a safe approach for now and stick with the implementation
submitted in this v2 patch.
Thanks for your feedback on this submission so far. I will fix the two
other comments and send out a v3 patch.
--
Regards,
Ravi