diff mbox

[RESEND,net,06/10] net: ethernet: mediatek: fix the loss of pin-mux setting for GMAC2

Message ID 1472121901-15629-7-git-send-email-sean.wang@mediatek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Wang Aug. 25, 2016, 10:44 a.m. UTC
ommited the setting about pin-mux which results in incorrect signals
being routed on GMAC2.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 14 ++++++++++++++
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 +++
 2 files changed, 17 insertions(+)

Comments

Andrew Lunn Aug. 25, 2016, 1:30 p.m. UTC | #1
On Thu, Aug 25, 2016 at 06:44:57PM +0800, Sean Wang wrote:
> ommited the setting about pin-mux which results in incorrect signals
> being routed on GMAC2.

Hi Sean

Please could you explain this some more. I don't know too much about
pinctrl, but i've never seen a driver have to do anything with it. The
core driver code handles it all, selecting the default state. See
seeing this here makes me wonder if it is correct.

Thanks
	Andrew

> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 14 ++++++++++++++
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 5bd31f8..0a4c782 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -1415,6 +1415,7 @@ static int __init mtk_hw_init(struct mtk_eth *eth)
>  	usleep_range(10, 20);
>  	reset_control_deassert(eth->rstc);
>  	usleep_range(10, 20);
> +	pinctrl_select_state(eth->pins, eth->ephy_default);
>  
>  	/* Set GE2 driving and slew rate */
>  	regmap_write(eth->pctl, GPIO_DRV_SEL10, 0xa00);
> @@ -1858,6 +1859,19 @@ static int mtk_probe(struct platform_device *pdev)
>  			return -ENODEV;
>  	}
>  
> +	eth->pins = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(eth->pins)) {
> +		dev_err(&pdev->dev, "cannot get pinctrl\n");
> +		return PTR_ERR(eth->pins);
> +	}
> +
> +	eth->ephy_default =
> +		pinctrl_lookup_state(eth->pins, "default");
> +	if (IS_ERR(eth->ephy_default)) {
> +		dev_err(&pdev->dev, "cannot get pinctrl state\n");
> +		return PTR_ERR(eth->ephy_default);
> +	}
> +
>  	clk_prepare_enable(eth->clk_ethif);
>  	clk_prepare_enable(eth->clk_esw);
>  	clk_prepare_enable(eth->clk_gp1);
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index f82e3ac..13d3f1b 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -404,6 +404,9 @@ struct mtk_eth {
>  	struct clk			*clk_esw;
>  	struct clk			*clk_gp1;
>  	struct clk			*clk_gp2;
> +	struct pinctrl			*pins;
> +	struct pinctrl_state		*ephy_default;
> +
>  	struct mii_bus			*mii_bus;
>  	struct work_struct		pending_work;
>  };
> -- 
> 1.9.1
>
Sean Wang Aug. 26, 2016, 3:33 a.m. UTC | #2
On Thu, 25 Aug 2016 15:30:34 +0200, Andrew Lunn wrote:
>On Thu, Aug 25, 2016 at 06:44:57PM +0800, Sean Wang wrote:
>> ommited the setting about pin-mux which results in incorrect signals
>> being routed on GMAC2.
>
>Hi Sean
>
>Please could you explain this some more. I don't know too much about
>pinctrl, but i've never seen a driver have to do anything with it. The
>core driver code handles it all, selecting the default state. See
>seeing this here makes me wonder if it is correct.
>
>Thanks
>	Andrew
>
>>

Hi Andrew,

Here pinctrl is used to setup what function the group of the pins is for.

The group of the pins could be configured for the function provided 
by the SoC, such as general purpose I/O or specific function such as
ethernet depending on what products or boards you design for various 
customers or vendors. Thanks for device tree introducing, it is easy 
to find what resources the board needs including the pins usage is 
also defined here.
 
Pins are limited resource that is also meant for the cost 
so that it is common way seen on embedded system.

thanks,
Sean

>> +	eth->pins = devm_pinctrl_get(&pdev->dev);
>> +	if (IS_ERR(eth->pins)) {
>> +		dev_err(&pdev->dev, "cannot get pinctrl\n");
>> +		return PTR_ERR(eth->pins);
>> +	}
>> +
>> +	eth->ephy_default =
>> +		pinctrl_lookup_state(eth->pins, "default");
>> +	if (IS_ERR(eth->ephy_default)) {
>> +		dev_err(&pdev->dev, "cannot get pinctrl state\n");
>> +		return PTR_ERR(eth->ephy_default);
>> +	}
>> +
>>  	clk_prepare_enable(eth->clk_ethif);
>>  	clk_prepare_enable(eth->clk_esw);
>>  	clk_prepare_enable(eth->clk_gp1);
>> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> index f82e3ac..13d3f1b 100644
>> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
>> @@ -404,6 +404,9 @@ struct mtk_eth {
>>  	struct clk			*clk_esw;
>>  	struct clk			*clk_gp1;
>>  	struct clk			*clk_gp2;
>> +	struct pinctrl			*pins;
>> +	struct pinctrl_state		*ephy_default;
>> +
>>  	struct mii_bus			*mii_bus;
>>  	struct work_struct		pending_work;
>>  };
>> -- 
>> 1.9.1
Andrew Lunn Aug. 26, 2016, 2:17 p.m. UTC | #3
> Hi Andrew,
> 
> Here pinctrl is used to setup what function the group of the pins is
> for.

Agreed.
 
> The group of the pins could be configured for the function provided 
> by the SoC, such as general purpose I/O or specific function such as
> ethernet depending on what products or boards you design for various 
> customers or vendors. Thanks for device tree introducing, it is easy 
> to find what resources the board needs including the pins usage is 
> also defined here.

All clear. However, if the ethernet driver has loaded, it means the
device tree says the ethernet should be loaded, unless it happens to
be on some discoverable bus. And so the device tree node for the
ethernet should also contain the needed pinctrl properties.  The core
driver code should of seen these properties and already enabled the
correct pinctrl state before the driver probes.

This is how every other driver works. Like i said, i don't think i've
seen any other driver do its own pinctrl. So i just need a simple
description, what is different here, why does this driver need to do
it, when no other does?

    Andrew
Sean Wang Aug. 29, 2016, 4:27 a.m. UTC | #4
Date: Fri, 26 Aug 2016 16:17:59 +0200, Andrew Lunn wrote:
>> Hi Andrew,
>> 
>> Here pinctrl is used to setup what function the group of the pins is
>> for.
>
>Agreed.
> 
>> The group of the pins could be configured for the function provided 
>> by the SoC, such as general purpose I/O or specific function such as
>> ethernet depending on what products or boards you design for various 
>> customers or vendors. Thanks for device tree introducing, it is easy 
>> to find what resources the board needs including the pins usage is 
>> also defined here.
>
>All clear. However, if the ethernet driver has loaded, it means the
>device tree says the ethernet should be loaded, unless it happens to
>be on some discoverable bus. And so the device tree node for the
>ethernet should also contain the needed pinctrl properties.  The core
>driver code should of seen these properties and already enabled the
>correct pinctrl state before the driver probes.
>
>This is how every other driver works. Like i said, i don't think i've
>seen any other driver do its own pinctrl. So i just need a simple
>description, what is different here, why does this driver need to do
>it, when no other does?
>
>    Andrew
>

You are right
all that I need about pinctrl are all being done with core driver 
as you said, so the patch I did seems the redundant work and i will remove 
it from the patch set.

thanks for your patient and careful reviewing and that also helps me getting 
familiar with based driver with pinctrl more :)

Sean
diff mbox

Patch

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 5bd31f8..0a4c782 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1415,6 +1415,7 @@  static int __init mtk_hw_init(struct mtk_eth *eth)
 	usleep_range(10, 20);
 	reset_control_deassert(eth->rstc);
 	usleep_range(10, 20);
+	pinctrl_select_state(eth->pins, eth->ephy_default);
 
 	/* Set GE2 driving and slew rate */
 	regmap_write(eth->pctl, GPIO_DRV_SEL10, 0xa00);
@@ -1858,6 +1859,19 @@  static int mtk_probe(struct platform_device *pdev)
 			return -ENODEV;
 	}
 
+	eth->pins = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(eth->pins)) {
+		dev_err(&pdev->dev, "cannot get pinctrl\n");
+		return PTR_ERR(eth->pins);
+	}
+
+	eth->ephy_default =
+		pinctrl_lookup_state(eth->pins, "default");
+	if (IS_ERR(eth->ephy_default)) {
+		dev_err(&pdev->dev, "cannot get pinctrl state\n");
+		return PTR_ERR(eth->ephy_default);
+	}
+
 	clk_prepare_enable(eth->clk_ethif);
 	clk_prepare_enable(eth->clk_esw);
 	clk_prepare_enable(eth->clk_gp1);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index f82e3ac..13d3f1b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -404,6 +404,9 @@  struct mtk_eth {
 	struct clk			*clk_esw;
 	struct clk			*clk_gp1;
 	struct clk			*clk_gp2;
+	struct pinctrl			*pins;
+	struct pinctrl_state		*ephy_default;
+
 	struct mii_bus			*mii_bus;
 	struct work_struct		pending_work;
 };