Re: [patch net-next 01/11] net: hisilicon: add support of acpi for hns-mdio

From: Andy Shevchenko
Date: Fri May 13 2016 - 08:58:30 EST

Next message: Andy Shevchenko: "Re: [patch net-next 04/11] net: hns: enet specify a reference to dsaf by fwnode_handle"
Previous message: Sergei Shtylyov: "Re: [PATCH] ixgbe: take online CPU number as MQ max limit when alloc_etherdev_mq()"
In reply to: Yisen Zhuang: "[patch net-next 01/11] net: hisilicon: add support of acpi for hns-mdio"
Next in thread: Yisen Zhuang: "[patch net-next 10/11] net: hns: implement the reset sequence by asl"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2016-05-13 at 16:19 +0800, Yisen Zhuang wrote:
> From: Kejian Yan <yankejian@xxxxxxxxxx>
>
> hns-mdio needs to register itself to mii-bus. The info of the device
> can
> be read by both OF and ACPI.
> HNS tries to call Linux PHY driver to help access PHY-devices, the HNS
> hardware topology is as below. The MDIO controller may control several
> PHY-devices, and each PHY-device connects to a MAC device. The MDIO
> will
> be registered to mdiobus, then PHY-devices will register when each mac
> find PHY device.
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcpu
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂÂ-------------------------------------------
> ÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdsafÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂMDIOÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂMDIO
> ÂÂÂÂ|ÂÂÂÂÂÂ---------------------------ÂÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂMACÂÂÂÂÂÂÂMACÂÂÂÂÂÂÂMACÂÂÂÂÂMACÂÂÂÂÂÂÂÂ|
> ÂÂÂÂ|ÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂ|ÂÂÂÂÂÂÂÂÂ|
> ÂÂÂÂÂ---- |-------- |-------- |ÂÂÂÂÂÂÂ| --------
> ÂÂÂÂÂÂÂÂÂ||ÂÂÂÂÂÂÂÂ||ÂÂÂÂÂÂÂÂ||ÂÂÂÂÂÂÂ||
> ÂÂÂÂÂÂÂÂÂPHYÂÂÂÂÂÂÂPHYÂÂÂÂÂÂÂPHYÂÂÂÂÂPHY
>
> And the driver can handle reset sequence by _DSD method in DSDT in
> ACPI case.
>
> Signed-off-by: Kejian Yan <yankejian@xxxxxxxxxx>
> Signed-off-by: Yisen Zhuang <Yisen.Zhuang@xxxxxxxxxx>
> ---
> Âdrivers/net/ethernet/hisilicon/hns_mdio.c | 145 ++++++++++++++++++---
> ---------
> Â1 file changed, 90 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns_mdio.c
> b/drivers/net/ethernet/hisilicon/hns_mdio.c
> index 765ddb3..4b779df 100644
> --- a/drivers/net/ethernet/hisilicon/hns_mdio.c
> +++ b/drivers/net/ethernet/hisilicon/hns_mdio.c
> @@ -7,6 +7,7 @@
> Â * (at your option) any later version.
> Â */
> Â
> +#include <linux/acpi.h>
> Â#include <linux/errno.h>
> Â#include <linux/etherdevice.h>
> Â#include <linux/init.h>
> @@ -354,64 +355,72 @@ static int hns_mdio_reset(struct mii_bus *bus)
> Â struct hns_mdio_device *mdio_dev = (struct hns_mdio_device
> *)bus->priv;
> Â int ret;
> Â
> - if (!mdio_dev->subctrl_vbase) {
> - dev_err(&bus->dev, "mdio sys ctl reg has not
> maped\n");
> - return -ENODEV;
> - }
> -
> - /*1. reset req, and read reset st check*/
> - ret = mdio_sc_cfg_reg_write(mdio_dev, MDIO_SC_RESET_REQ, 0x1,
> - ÂÂÂÂMDIO_SC_RESET_ST, 0x1,
> - ÂÂÂÂMDIO_CHECK_SET_ST);
> - if (ret) {
> - dev_err(&bus->dev, "MDIO reset fail\n");
> - return ret;
> - }
> + if (IS_ENABLED(CONFIG_OF) && bus->parent->of_node) {

Can you keep indentation the same?

Also I suggest to use struct fwnode_handle, and this will be something
like

if (is_of_node(...))

> + if (!mdio_dev->subctrl_vbase) {
> + dev_err(&bus->dev, "mdio sys ctl reg has not
> maped\n");
> + return -ENODEV;
> + }
> Â
> - /*2. dis clk, and read clk st check*/
> - ret = mdio_sc_cfg_reg_write(mdio_dev, MDIO_SC_CLK_DIS,
> - ÂÂÂÂ0x1, MDIO_SC_CLK_ST, 0x1,
> - ÂÂÂÂMDIO_CHECK_CLR_ST);
> - if (ret) {
> - dev_err(&bus->dev, "MDIO dis clk fail\n");
> - return ret;
> - }
> + /*1. reset req, and read reset st check*/
> + ret = mdio_sc_cfg_reg_write(mdio_dev,
> MDIO_SC_RESET_REQ, 0x1,
> + ÂÂÂÂMDIO_SC_RESET_ST, 0x1,
> + ÂÂÂÂMDIO_CHECK_SET_ST);
> + if (ret) {
> + dev_err(&bus->dev, "MDIO reset fail\n");
> + return ret;
> + }
> Â
> - /*3. reset dreq, and read reset st check*/
> - ret = mdio_sc_cfg_reg_write(mdio_dev, MDIO_SC_RESET_DREQ,
> 0x1,
> - ÂÂÂÂMDIO_SC_RESET_ST, 0x1,
> - ÂÂÂÂMDIO_CHECK_CLR_ST);
> - if (ret) {
> - dev_err(&bus->dev, "MDIO dis clk fail\n");
> - return ret;
> - }
> + /*2. dis clk, and read clk st check*/
> + ret = mdio_sc_cfg_reg_write(mdio_dev,
> MDIO_SC_CLK_DIS,
> + ÂÂÂÂ0x1, MDIO_SC_CLK_ST, 0x1,
> + ÂÂÂÂMDIO_CHECK_CLR_ST);
> + if (ret) {
> + dev_err(&bus->dev, "MDIO dis clk fail\n");
> + return ret;
> + }
> Â
> - /*4. en clk, and read clk st check*/
> - ret = mdio_sc_cfg_reg_write(mdio_dev, MDIO_SC_CLK_EN,
> - ÂÂÂÂ0x1, MDIO_SC_CLK_ST, 0x1,
> - ÂÂÂÂMDIO_CHECK_SET_ST);
> - if (ret)
> - dev_err(&bus->dev, "MDIO en clk fail\n");
> + /*3. reset dreq, and read reset st check*/
> + ret = mdio_sc_cfg_reg_write(mdio_dev,
> MDIO_SC_RESET_DREQ, 0x1,
> + ÂÂÂÂMDIO_SC_RESET_ST, 0x1,
> + ÂÂÂÂMDIO_CHECK_CLR_ST);
> + if (ret) {
> + dev_err(&bus->dev, "MDIO dis clk fail\n");
> + return ret;
> + }
> Â
> + /*4. en clk, and read clk st check*/
> + ret = mdio_sc_cfg_reg_write(mdio_dev, MDIO_SC_CLK_EN,
> + ÂÂÂÂ0x1, MDIO_SC_CLK_ST, 0x1,
> + ÂÂÂÂMDIO_CHECK_SET_ST);
> + if (ret)
> + dev_err(&bus->dev, "MDIO en clk fail\n");
> + } else if (ACPI_COMPANION(bus->parent)) {
> + acpi_status s;
> +
> + s = acpi_evaluate_object(ACPI_HANDLE(bus->parent),
> + Â"_RST", NULL, NULL);
> + if (ACPI_FAILURE(s)) {
> + dev_err(&bus->dev, "Reset failed,
> return:%#x\n", s);
> + ret = -EBUSY;
> + } else {
> + ret = 0;
> + }
> + } else {
> + dev_err(&bus->dev, "cannot get cfd data from of or
> acpi\n");

cannot -> Can not

WTF cfd?

of or acpi -> DT or ACPI

> + ret = -ENXIO;
> + }
> Â return ret;
> Â}
> Â
> Â/**
> Â * hns_mdio_bus_name - get mdio bus name
> Â * @name: mdio bus name
> - * @np: mdio device node pointer
> + * @addr: mdio physical address
> Â */
> -static void hns_mdio_bus_name(char *name, struct device_node *np)
> +static void hns_mdio_bus_name(char *name, phys_addr_t addr)
> Â{
> - const u32 *addr;
> - u64 taddr = OF_BAD_ADDR;
> -
> - addr = of_get_address(np, 0, NULL, NULL);
> - if (addr)
> - taddr = of_translate_address(np, addr);
> -
> - snprintf(name, MII_BUS_ID_SIZE, "%s@%llx", np->name,
> - Â(unsigned long long)taddr);
> + snprintf(name, MII_BUS_ID_SIZE,
> + Â"hns-mdio@%llx", (unsigned long long)addr);
> Â}
> Â
> Â/**
> @@ -422,7 +431,6 @@ static void hns_mdio_bus_name(char *name, struct
> device_node *np)
> Â */
> Âstatic int hns_mdio_probe(struct platform_device *pdev)
> Â{
> - struct device_node *np;
> Â struct hns_mdio_device *mdio_dev;
> Â struct mii_bus *new_bus;
> Â struct resource *res;
> @@ -432,7 +440,7 @@ static int hns_mdio_probe(struct platform_device
> *pdev)
> Â dev_err(NULL, "pdev is NULL!\r\n");
> Â return -ENODEV;
> Â }
> - np = pdev->dev.of_node;
> +
> Â mdio_dev = devm_kzalloc(&pdev->dev, sizeof(*mdio_dev),
> GFP_KERNEL);
> Â if (!mdio_dev)
> Â return -ENOMEM;
> @@ -448,7 +456,6 @@ static int hns_mdio_probe(struct platform_device
> *pdev)
> Â new_bus->write = hns_mdio_write;
> Â new_bus->reset = hns_mdio_reset;
> Â new_bus->priv = mdio_dev;
> - hns_mdio_bus_name(new_bus->id, np);
> Â
> Â res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> Â mdio_dev->vbase = devm_ioremap_resource(&pdev->dev, res);
> @@ -457,16 +464,37 @@ static int hns_mdio_probe(struct platform_device
> *pdev)
> Â return ret;
> Â }
> Â
> - mdio_dev->subctrl_vbase =
> - syscon_node_to_regmap(of_parse_phandle(np, "subctrl-
> vbase", 0));
> - if (IS_ERR(mdio_dev->subctrl_vbase)) {
> - dev_warn(&pdev->dev, "no syscon hisilicon,peri-c-
> subctrl\n");
> - mdio_dev->subctrl_vbase = NULL;
> + hns_mdio_bus_name(new_bus->id, res->start);
> + if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {

is_of_node()

> + mdio_dev->subctrl_vbase = syscon_node_to_regmap(
> + of_parse_phandle(pdev->dev.of_node,
> + Â"subctrl-vbase", 0));
> + if (IS_ERR(mdio_dev->subctrl_vbase)) {
> + dev_warn(&pdev->dev, "no syscon
> hisilicon,peri-c-subctrl\n");
> + mdio_dev->subctrl_vbase = NULL;
> + }
> Â }
> Â new_bus->parent = &pdev->dev;
> Â platform_set_drvdata(pdev, new_bus);
> Â
> - ret = of_mdiobus_register(new_bus, np);
> + if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {

Ditto

> + ret = of_mdiobus_register(new_bus, pdev-
> >dev.of_node);
> + } else if (ACPI_COMPANION(&pdev->dev)) {

is_acpi_node()

> + /* Clear all the IRQ properties */
> + memset(new_bus->irq, PHY_POLL, 4 * PHY_MAX_ADDR);
> +
> + /* Mask out all PHYs from auto probing. */
> + new_bus->phy_mask = ~0;
> +
> + /* Register the MDIO bus */
> + ret = mdiobus_register(new_bus);
> + if (ret)
> + return ret;
> + } else {
> + dev_err(&pdev->dev, "cannot get cfd data from of or
> acpi\n");

Same as for previous message.

> + return -ENXIO;
> + }
> +
> Â if (ret) {
> Â dev_err(&pdev->dev, "Cannot register as MDIO
> bus!\n");
> Â platform_set_drvdata(pdev, NULL);
> @@ -499,12 +527,19 @@ static const struct of_device_id
> hns_mdio_match[] = {
> Â {}
> Â};
> Â
> +static const struct acpi_device_id hns_mdio_acpi_match[] = {
> + { "HISI0141", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, hns_mdio_acpi_match);
> +
> Âstatic struct platform_driver hns_mdio_driver = {
> Â .probe = hns_mdio_probe,
> Â .remove = hns_mdio_remove,
> Â .driver = {
> Â ÂÂÂ.name = MDIO_DRV_NAME,
> Â ÂÂÂ.of_match_table = hns_mdio_match,
> + ÂÂÂ.acpi_match_table = ACPI_PTR(hns_mdio_acpi_match),
> Â ÂÂÂ},
> Â};
> Â

So, I suggest to split this to two logical changes:

1. Move to use fwnode_handle
2. Add ACPI bits

--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy

Next message: Andy Shevchenko: "Re: [patch net-next 04/11] net: hns: enet specify a reference to dsaf by fwnode_handle"
Previous message: Sergei Shtylyov: "Re: [PATCH] ixgbe: take online CPU number as MQ max limit when alloc_etherdev_mq()"
In reply to: Yisen Zhuang: "[patch net-next 01/11] net: hisilicon: add support of acpi for hns-mdio"
Next in thread: Yisen Zhuang: "[patch net-next 10/11] net: hns: implement the reset sequence by asl"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]