[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Romain Perier]

Currently, selecting state simple is done by the device driver model,
prior probing the driver. The problem is that it's done and called on
the pinctrl node with "gmac" as the "periph" struct udevice *. So
pinctrl-rk3288 is looking for an interrupt property that is not found,
and then gmac_config is never called.

This commits toggles the pinctrl on the right node from the probe
function of the driver.

Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/net/gmac_rockchip.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
index 5e2ca76302..8581026b4a 100644
--- a/drivers/net/gmac_rockchip.c
+++ b/drivers/net/gmac_rockchip.c
@@ -160,6 +160,10 @@ static int gmac_rockchip_probe(struct udevice *dev)
 	struct clk clk;
 	int ret;
 
+	ret = pinctrl_select_state(dev, "simple");
+	if (ret)
+		return ret;
+
 	ret = clk_get_by_index(dev, 0, &clk);
 	if (ret)
 		return ret;
-- 
2.11.0

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Simon Glass]

+Philipp

Hi Romain,

On 3 July 2017 at 01:13, Romain Perier <romain.perier@collabora.com> wrote:
> Currently, selecting state simple is done by the device driver model,
> prior probing the driver. The problem is that it's done and called on
> the pinctrl node with "gmac" as the "periph" struct udevice *. So
> pinctrl-rk3288 is looking for an interrupt property that is not found,
> and then gmac_config is never called.
>
> This commits toggles the pinctrl on the right node from the probe
> function of the driver.

Is it possible to fix this while still using driver-model automatic pinctrl?

>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/net/gmac_rockchip.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
> index 5e2ca76302..8581026b4a 100644
> --- a/drivers/net/gmac_rockchip.c
> +++ b/drivers/net/gmac_rockchip.c
> @@ -160,6 +160,10 @@ static int gmac_rockchip_probe(struct udevice *dev)
>         struct clk clk;
>         int ret;
>
> +       ret = pinctrl_select_state(dev, "simple");
> +       if (ret)
> +               return ret;
> +
>         ret = clk_get_by_index(dev, 0, &clk);
>         if (ret)
>                 return ret;
> --
> 2.11.0
>

Regards,
Simon

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Romain Perier]

Le 07/07/2017 à 05:58, Simon Glass a écrit :
> +Philipp
>
> Hi Romain,
>
> On 3 July 2017 at 01:13, Romain Perier <romain.perier@collabora.com> wrote:
>> Currently, selecting state simple is done by the device driver model,
>> prior probing the driver. The problem is that it's done and called on
>> the pinctrl node with "gmac" as the "periph" struct udevice *. So
>> pinctrl-rk3288 is looking for an interrupt property that is not found,
>> and then gmac_config is never called.
>>
>> This commits toggles the pinctrl on the right node from the probe
>> function of the driver.
> Is it possible to fix this while still using driver-model automatic pinctrl?

This is what I have tried to do, without success. The purpose of this
patch is also to discuss about the possible solutions we have.

Regards,
Romain

>
>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>> ---
>>  drivers/net/gmac_rockchip.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
>> index 5e2ca76302..8581026b4a 100644
>> --- a/drivers/net/gmac_rockchip.c
>> +++ b/drivers/net/gmac_rockchip.c
>> @@ -160,6 +160,10 @@ static int gmac_rockchip_probe(struct udevice *dev)
>>         struct clk clk;
>>         int ret;
>>
>> +       ret = pinctrl_select_state(dev, "simple");
>> +       if (ret)
>> +               return ret;
>> +
>>         ret = clk_get_by_index(dev, 0, &clk);
>>         if (ret)
>>                 return ret;
>> --
>> 2.11.0
>>
> Regards,
> Simon

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Dr. Philipp Tomsich]

> On 10 Jul 2017, at 08:43, Romain Perier <romain.perier@collabora.com> wrote:
> 
> Le 07/07/2017 à 05:58, Simon Glass a écrit :
>> +Philipp
>> 
>> Hi Romain,
>> 
>> On 3 July 2017 at 01:13, Romain Perier <romain.perier@collabora.com> wrote:
>>> Currently, selecting state simple is done by the device driver model,
>>> prior probing the driver. The problem is that it's done and called on
>>> the pinctrl node with "gmac" as the "periph" struct udevice *. So
>>> pinctrl-rk3288 is looking for an interrupt property that is not found,
>>> and then gmac_config is never called.
>>> 
>>> This commits toggles the pinctrl on the right node from the probe
>>> function of the driver.
>> Is it possible to fix this while still using driver-model automatic pinctrl?
> 
> This is what I have tried to do, without success. The purpose of this
> patch is also to discuss about the possible solutions we have.

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Dr. Philipp Tomsich]

> On 07 Jul 2017, at 05:58, Simon Glass <sjg@chromium.org> wrote:
> 
> +Philipp
> 
> Hi Romain,
> 
> On 3 July 2017 at 01:13, Romain Perier <romain.perier@collabora.com> wrote:
>> Currently, selecting state simple is done by the device driver model,
>> prior probing the driver. The problem is that it's done and called on
>> the pinctrl node with "gmac" as the "periph" struct udevice *. So
>> pinctrl-rk3288 is looking for an interrupt property that is not found,
>> and then gmac_config is never called.
>> 
>> This commits toggles the pinctrl on the right node from the probe
>> function of the driver.
> 
> Is it possible to fix this while still using driver-model automatic pinctrl?

I agree that we need to get a handle on the underlying issue and
solve that instead of putting a band-aid on.

>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>> ---
>> drivers/net/gmac_rockchip.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/net/gmac_rockchip.c b/drivers/net/gmac_rockchip.c
>> index 5e2ca76302..8581026b4a 100644
>> --- a/drivers/net/gmac_rockchip.c
>> +++ b/drivers/net/gmac_rockchip.c
>> @@ -160,6 +160,10 @@ static int gmac_rockchip_probe(struct udevice *dev)
>>        struct clk clk;
>>        int ret;
>> 
>> +       ret = pinctrl_select_state(dev, "simple");
>> +       if (ret)
>> +               return ret;
>> +
>>        ret = clk_get_by_index(dev, 0, &clk);
>>        if (ret)
>>                return ret;
>> --
>> 2.11.0
>> 
> 
> Regards,
> Simon

[U-Boot] [PATCH] net: gmac_rockchip: Add support for pinctrl

[Romain Perier]

Hello,


Le 11/07/2017 à 15:43, Dr. Philipp Tomsich a écrit :
>> On 10 Jul 2017, at 08:43, Romain Perier <romain.perier@collabora.com> wrote:
>>
>> Le 07/07/2017 à 05:58, Simon Glass a écrit :
>>> +Philipp
>>>
>>> Hi Romain,
>>>
>>> On 3 July 2017 at 01:13, Romain Perier <romain.perier@collabora.com> wrote:
>>>> Currently, selecting state simple is done by the device driver model,
>>>> prior probing the driver. The problem is that it's done and called on
>>>> the pinctrl node with "gmac" as the "periph" struct udevice *. So
>>>> pinctrl-rk3288 is looking for an interrupt property that is not found,
>>>> and then gmac_config is never called.
>>>>
>>>> This commits toggles the pinctrl on the right node from the probe
>>>> function of the driver.
>>> Is it possible to fix this while still using driver-model automatic pinctrl?
>> This is what I have tried to do, without success. The purpose of this
>> patch is also to discuss about the possible solutions we have.
> From looking at pinctrl_rk3288.c, I wonder if there’s a problem with
> pinctrl on the RK3288 outside of SPL: I see that the (full) set_state
> is also implemented outside of SPL (#ifndef CONFIG_SPL_BUILD)
> and not guarded by a CONFIG_IS_ENABLED(PINCTRL_FULL). So
> something might go down the wrong path.
>
> Could start narrowing this down by enabling DEBUG in pinctrl_rk3288
> to see whether this is even the simple pinctrl that gets called?

Yeah, the simple pinctrl is called at least for "hdmi" and the function
for configuring hdmi is called internally.

Romain