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:
quoted
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
quoted
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
quoted
+ 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
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
Date: Fri, 26 Aug 2016 16:17:59 +0200, Andrew Lunn wrote:
quoted
Hi Andrew,
Here pinctrl is used to setup what function the group of the pins is
for.
Agreed.
quoted
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