[U-Boot] [PATCH v9 12/12] dm: exynos: gpio: Convert to driver model

[Simon Glass]

Hi,

On 20 October 2014 10:06, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> Hello again,
>
>
> On 10/20/2014 05:53 PM, Przemyslaw Marczak wrote:
>>
>> Hello,
>>
>> On 10/20/2014 05:31 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 20 October 2014 08:55, Przemyslaw Marczak <p.marczak at samsung.com>
>>> wrote:
>>>>
>>>> Hello Simon,
>>>>
>>>>
>>>> On 10/15/2014 10:38 AM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Convert the exynos GPIO driver to driver model. This implements the
>>>>> generic
>>>>> GPIO interface but not the extra Exynos-specific functions.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v9:
>>>>> - Add missing compatible strings from exynos_gpio_ids[]
>>>>>
>>>>> Changes in v8:
>>>>> - Enable driver model for smdkc100 and s5p_goni separately
>>>>>
>>>>> Changes in v7:
>>>>> - Bring in patches from the SPI series to move post-reloc DM init
>>>>> earlier
>>>>>
>>>>> Changes in v6: None
>>>>> Changes in v5:
>>>>> - Remove RFC label now that build errors are fixed
>>>>> - Tidy up and update cover letter message
>>>>> - Avoid reordering functions
>>>>>
>>>>> Changes in v4:
>>>>> - Add patches for exynos GPIO support
>>>>>
>>>>>    drivers/gpio/s5p_gpio.c         | 424
>>>>> +++++++++++++++++++++++++++-------------
>>>>>    include/configs/exynos-common.h |   4 +
>>>>>    include/configs/s5p_goni.h      |   3 +
>>>>>    include/configs/smdkc100.h      |   3 +
>>>>>    4 files changed, 298 insertions(+), 136 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
>>>>> index 99f2dd8..13d74eb 100644
>>>>> --- a/drivers/gpio/s5p_gpio.c
>>>>> +++ b/drivers/gpio/s5p_gpio.c
>>>>> @@ -6,8 +6,15 @@
>>>>>     */
>>>>>
>>>>
>>>> ..snip..
>>>>
>>>>> +/**
>>>>> + * We have a top-level GPIO device with no actual GPIOs. It has a
>>>>> child
>>>>> + * device for each Exynos GPIO bank.
>>>>> + */
>>>>> +static int gpio_exynos_bind(struct udevice *parent)
>>>>> +{
>>>>> +       struct exynos_gpio_platdata *plat = parent->platdata;
>>>>> +       struct s5p_gpio_bank *bank, *base;
>>>>> +       const void *blob = gd->fdt_blob;
>>>>> +       int node;
>>>>> +
>>>>> +       /* If this is a child device, there is nothing to do here */
>>>>> +       if (plat)
>>>>> +               return 0;
>>>>> +
>>>>> +       base = (struct s5p_gpio_bank *)fdtdec_get_addr(gd->fdt_blob,
>>>>> +                                                  parent->of_offset,
>>>>> "reg");
>>>>> +       for (node = fdt_first_subnode(blob, parent->of_offset), bank =
>>>>> base;
>>>>> +            node > 0;
>>>>> +            node = fdt_next_subnode(blob, node), bank++) {
>>>>
>>>>
>>>>
>>>> Here (bank++) you assume, that order of subnodes of each pinctrl is
>>>> the same
>>>> like in dts file - as I wrote in comment to patch 04 - it isn't.
>>>>
>>>>> +               struct exynos_gpio_platdata *plat;
>>>>> +               struct udevice *dev;
>>>>> +               fdt_addr_t reg;
>>>>> +               int ret;
>>>>> +
>>>>> +               if (!fdtdec_get_bool(blob, node, "gpio-controller"))
>>>>> +                       continue;
>>>>> +               plat = calloc(1, sizeof(*plat));
>>>>> +               if (!plat)
>>>>> +                       return -ENOMEM;
>>>>
>>>>
>>>>
>>>> Some parts of GPIO banks, for which "reg" can be found are prober.
>>>>
>>>>
>>>>> +               reg = fdtdec_get_addr(blob, node, "reg");
>>>>> +               if (reg != FDT_ADDR_T_NONE)
>>>>> +                       bank = (struct s5p_gpio_bank *)((ulong)base +
>>>>> reg);
>>>>> +               plat->bank = bank;
>>>>> +               plat->bank_name = fdt_get_name(blob, node, NULL);
>>>>> +               debug("dev at %p: %s\n", bank, plat->bank_name);
>>>>> +
>>>>> +               ret = device_bind(parent, parent->driver,
>>>>> +                                       plat->bank_name, plat, -1,
>>>>> &dev);
>>>>> +               if (ret)
>>>>> +                       return ret;
>>>>> +               dev->of_offset = parent->of_offset;
>>>>> +       }
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct udevice_id exynos_gpio_ids[] = {
>>>>> +       { .compatible = "samsung,s5pc100-pinctrl" },
>>>>> +       { .compatible = "samsung,s5pc110-pinctrl" },
>>>>> +       { .compatible = "samsung,exynos4210-pinctrl" },
>>>>> +       { .compatible = "samsung,exynos4x12-pinctrl" },
>>>>> +       { .compatible = "samsung,exynos5250-pinctrl" },
>>>>> +       { .compatible = "samsung,exynos5420-pinctrl" },
>>>>> +       { }
>>>>
>>>>
>>>>
>>>> ... snip ...
>>>>
>>>> And this is not the end of issues:)
>>>>
>>>> Unfortunately Exynos4xxx has a gaps in GPIO bank address space, and
>>>> the big
>>>> gpio enum lists includes that here:
>>>> arch/arm/include/asm/arch-exynos/gpio.h
>>>>
>>>> This works for Exynos5, because it's enum list is linear.
>>>>
>>>> And the driver gpio-uclass.c - assumes that the gpio numbers are linear.
>>>> When I removed the gaps in enum for 4x12 -then gpio is mapped good.
>>>>
>>>> And after remove the enum gaps, it also requires fitting the gpios in
>>>> dts
>>>> files, e.g. cd-gpio, pwr-gpios for sdhci host.
>>>>
>>>> Sorry for such late reply in v9 but, before it seems working fine - I
>>>> didn't
>>>> try to check the sd card before.
>>>>
>>>> I hope that after that fixes, everything will work fine:)
>>>
>>>
>>> Thanks very much for testing this. As mentioned on the other patch,
>>> please try u-boot-dm/next and see how things look.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> No problem, it's also important for me. I will check the "next" in a few
>> minutes.
>>
>> Regards,
>
>
> After a quick look into the code I suppose that the issues are not fixed,
> but it is tested for sure:)
>
> So the main required changes:
> 1. Remove/fix all "**pinctrl-uboot.dtsi" files and move the cells size into
> "pinctrl.dtsi"

I was asked to keep the U-Boot changes separate, at least until they
can be sent to the kernel. I believe the new approach in that tree
should fix the problem, since I had the files in the wrong order.

>
> 2. Remove the gaps in Exynos4xxx gpio enum:
> arch/arm/include/asm/arch-exynos/gpio.
>
> and now I checked that the same is for s5pc110, here:
> arch/arm/include/asm/arch-s5pc1xx/gpio.h (only S5PC110_GPIO_H00)

OK I'll remove that.

>
> 3. Fix exynos4xxx boards *.dts after remove the gaps in gpio enum.

Will remove. Those gaps were a really horrible hack IMO!

I think I'll resend the whole series with these changes.

Regards,
iImon