| Message ID | 1472121901-15629-7-git-send-email-sean.wang@mediatek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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 >
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
> 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: >> 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 --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; };
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(+)