[U-Boot] [PATCH v5] Exynos5: Pinmux: Add fdt for pinmux

[Simon Glass]

Hi Akshay,

On Fri, Mar 8, 2013 at 4:08 AM, Akshay Saraswat <akshay.s at samsung.com>wrote:

> Hi Simon,
>
> >Hi Akshay,
> >
> >On Thu, Mar 7, 2013 at 6:09 AM, Akshay Saraswat <akshay.s at samsung.com>
> wrote:
> >> This patch adds fdt nodes for peripherals which require
> >> pin muxing and configuration. Device tree bindings for pinctrl
> >> are kept same as required for Linux. Existing pinmux code
> >> modified to retrieve gpio range and function related info from fdt.
> >>
> >> Depends-on: [U-Boot] [PATCH 0/4 V3] EXYNOS5: Add GPIO numbering feature
> >> URL: http://lists.denx.de/pipermail/u-boot/2013-February/146151.html
> >>
> >> Signed-off-by: Akshay Saraswat <akshay.s at samsung.com>
> >> ---
> >> Changes since v1:
> >>         - Device tree bindings changed to linux style.
> >>         - Added documentation for samsung pinctrl.
> >>
> >> Changes since v2:
> >>         - Rebased as per new version of GPIO numbering patch-set.
> >>
> >> Changes since v3:
> >>         - Added comments to reduce ambiguity and increase readability.
> >>         - Fixed few other nits.
> >>
> >> Changes since v4:
> >>         - Added support for reading peripheral pinctrl subnode names
> from preipheral's node instead of hard coding.
> >>
> >
> >Well I have to say this is looking really nice.  From what I can tell
> >you are using the Linux binding for samsung,pinctrl-names - please can
> >you add in that binding information to your binding file - I can't see
> >it there?
> >
>
> "samsung,pinctrl-names" is already mentioned in example 3 of
> doc/device-tree-bindings/samsung-pinctrl.txt.
> samsung-pinctrl.txt in u-boot and kernel are same except the name exynos5
> replaced exynos4210.
> Please tell me, if I misunderstood the requirement.
>

I don't see 'samsung,pinctrl-names' in example 3, but also, is it possible
to mention this in the binding somehow?

The names should match the kernel if possible.


>
> >
> >The interesting thing is that you should at some point (further patch)
> >be able to remove the alias stuff and have the driver's pass their
> >node offset into this function. Then we can drop the peripheral IDs
> >altogether perhaps?
> >
>
> I think for now we should keep PERIPH_ID's because same pinmux.c and
> functions are being used by
> exynos4 and exynos4x12. Since, I thought we should not break support for
> these SoC's, I kept node
> retrieval in Pinmux itself instead of drivers or smdk5250.c. Please
> suggest.
>

That's fine. I suspect perhaps driver could pass their node offset, but
let's worry about that another time.


>
> >
> >>  arch/arm/cpu/armv7/exynos/pinmux.c           | 357 +++++++-------
> >>  arch/arm/dts/exynos5250-pinctrl.dtsi         | 675
> +++++++++++++++++++++++++++
> >>  arch/arm/dts/exynos5250.dtsi                 |  92 ++++
> >>  board/samsung/dts/exynos5250-smdk5250.dts    |  11 +
> >>  doc/device-tree-bindings/samsung-pinctrl.txt | 253 ++++++++++
> >>  include/fdtdec.h                             |   4 +
> >>  lib/fdtdec.c                                 |   4 +
> >>  7 files changed, 1231 insertions(+), 165 deletions(-)
> >>  create mode 100644 arch/arm/dts/exynos5250-pinctrl.dtsi
> >>  create mode 100644 doc/device-tree-bindings/samsung-pinctrl.txt
> >>
> >[..]
> >
> >> diff --git a/include/fdtdec.h b/include/fdtdec.h
> >> index 77f244f..26692e5 100644
> >> --- a/include/fdtdec.h
> >> +++ b/include/fdtdec.h
> >> @@ -81,6 +81,10 @@ enum fdt_compat_id {
> >>         COMPAT_SAMSUNG_EXYNOS_EHCI,     /* Exynos EHCI controller */
> >>         COMPAT_SAMSUNG_EXYNOS_USB_PHY,  /* Exynos phy controller for
> usb2.0 */
> >>         COMPAT_MAXIM_MAX77686_PMIC,     /* MAX77686 PMIC */
> >> +       COMPAT_SAMSUNG_EXYNOS_UART,     /* Exynos serial */
> >> +       COMPAT_SAMSUNG_EXYNOS_MSHC,     /* Exynos MMC controller */
> >> +       COMPAT_SAMSUNG_EXYNOS_I2S,      /* Exynos MMC controller */
>

This comment should be I2C.


> >> +       COMPAT_SAMSUNG_PINCTRL,         /* PINCTRL */
> >>
> >>         COMPAT_COUNT,
> >>  };
> >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> >> index 3ae348d..88dd669 100644
> >> --- a/lib/fdtdec.c
> >> +++ b/lib/fdtdec.c
> >> @@ -56,6 +56,10 @@ static const char * const compat_names[COMPAT_COUNT]
> = {
> >>         COMPAT(SAMSUNG_EXYNOS_EHCI, "samsung,exynos-ehci"),
> >>         COMPAT(SAMSUNG_EXYNOS_USB_PHY, "samsung,exynos-usb-phy"),
> >>         COMPAT(MAXIM_MAX77686_PMIC, "maxim,max77686_pmic"),
> >> +       COMPAT(SAMSUNG_EXYNOS_UART, "samsung,exynos-uart"),
> >> +       COMPAT(SAMSUNG_EXYNOS_MSHC, "samsung,exynos-mshc"),
> >> +       COMPAT(SAMSUNG_EXYNOS_I2S, "samsung,exynos-i2s"),
> >> +       COMPAT(SAMSUNG_PINCTRL, "samsung,pinctrl"),
> >
> >Do these match the kernel names?
> >
>
> In kernel these are mentioned as ""samsung,exynos5250-dw-mshc"" &
> "samsung,exynos4210-uart".
> I wanted to keep it consistent with others like "samsung,exynos-ehci",
> "samsung,exynos-spi" etc.,
> hence, made "samsung,exynos-mshc" & "samsung,exynos-uart". Shall I change
> it ?
>

Yes, sorry, but I think we should keep things the same as the kernel.
Perhaps that should be a separate patch to sync up the names?

Regards,
Simon


>
> >
> >>  };
> >>
> >
>
> Regards,
> Akshay Saraswat
>