[RFC PATCH] powerpc, qe: add DTS support for parallel I/O ports

[Qiang Zhao]

On 2020/4/15 12:46 Heiko Schocher <hs at denx.de>

> -----Original Message-----
> From: Heiko Schocher <hs at denx.de>
> Sent: 2020年4月15日 12:46
> To: Qiang Zhao <qiang.zhao at nxp.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Holger Brunck
> <holger.brunck at ch.abb.com>; Mario Six <mario.six at gdsys.cc>; Wolfgang Denk
> <wd at denx.de>; Holger Brunck <holger.brunck at ch.abb.com>
> Subject: Re: [RFC PATCH] powerpc, qe: add DTS support for parallel I/O ports
> 
> Hello Qiang Zhao,
> 
> Am 09.04.2020 um 08:58 schrieb Qiang Zhao:
> > On 2020/2/18 17:05, Heiko Schocher <hs at denx.de <mailto:hs at denx.de>>
> wrote:
> >
> >> -----Original Message-----
> >
> >> From: Heiko Schocher <hs at denx.de>
> >
> >> Sent: 2020年2月18日17:05
> >
> >> To: U-Boot Mailing List <u-boot at lists.denx.de>
> >
> >> Cc: Heiko Schocher <hs at denx.de>; Holger Brunck
> >
> >> <holger.brunck at ch.abb.com>; Mario Six <mario.six at gdsys.cc>; Qiang
> >> Zhao
> >
> >> <qiang.zhao at nxp.com>; Wolfgang Denk <wd at denx.de>
> >
> >> Subject: [RFC PATCH] powerpc, qe: add DTS support for parallel I/O
> >> ports
> >
> >>
> >
> >> add DM support for parallel I/O ports on QUICC Engine Block
> >
> >>
> >
> >> Signed-off-by: Heiko Schocher <hs at denx.de <mailto:hs at denx.de>>
> >
> >> ---
> >
> >>
> >
> >>
> >
> >>  arch/powerpc/cpu/mpc83xx/cpu_init.c |   8 ++
> >
> >>  arch/powerpc/cpu/mpc83xx/qe_io.c    | 193
> >
> >> +++++++++++++++++++++++++++-
> >
> >>  include/fsl_qe.h                    |   3 +
> >
> >>  3 files changed, 201 insertions(+), 3 deletions(-)
> >
> >>
> >
> >> diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c
> >
> >> b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> >
> >> index af8facad53..cfcc16607c 100644
> >
> >> --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c
> >
> >> +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c
> >
> > *//*
> >
> > How about mpc85xx?
> 
> Yes, this is a Todo (one reason why this patch is RFC). I currently have reworked
> this for the mpc83xx based keymile (now abb) boards (which are locally
> running with complete DM/DTS support now).

You mean you have tested RFC patch on your side? That's great!
Would you like to push the dts patch too?

> 
> I have one mpx85xx based board I can try, but may others are working on this
> support, so I wanted to post my patches very early, so we can work together on
> it...
> 
> As my first question in patch comment is:
> 
> may we should move this part to drivers/pinctrl ?

Yes, it is more better to move to drivers/pinctrl.

> 
> >> @@ -11,6 +11,9 @@
> >
> >>  #ifdef CONFIG_USB_EHCI_FSL
> >
> >>  #include <usb/ehci-ci.h>
> >
> >>  #endif
> >
> >> +#ifdef CONFIG_QE
> >
> >> +#include <fsl_qe.h>
> >
> >> +#endif
> >
> > *//*
> >
> > If include this headfile, the function declaration for qe_init and
> > qe_reset in this file should be removed as below:
> >
> > extern void qe_init(uint qe_base);
> >
> > extern void qe_reset(void);
> 
> Yes, of course! But this part of code I did not touch... may this needs a cleanup
> at all.
 
Just two lines extern declaration in this file.

> 
> >
> >>
> >
> >>  #include "lblaw/lblaw.h"
> >
> >>  #include "elbc/elbc.h"
> >
> >> @@ -27,6 +30,7 @@ extern void qe_config_iopin(u8 port, u8 pin, int
> >> dir,
> >
> >> extern void qe_init(uint qe_base);  extern void qe_reset(void);
> >
> >>
> >
> >> +/**
> >
> >> + * par_io_of_config_node  config
> >
> >> + * @dev:     pointer to pinctrl device
> >
> >> + * @pio:      ofnode of pinconfig property
> >
> >> + */
> >
> >> +static int par_io_of_config_node(struct udevice *dev, ofnode pio) {
> >
> >> +   struct qe_io_platdata      *plat = dev->platdata;
> >
> >> +   volatile qepio83xx_t        *par_io = plat->base;
> >
> >> +   const unsigned int *pio_map;
> >
> >> +   int pio_map_len;
> >
> >> +
> >
> >> +   pio_map = ofnode_get_property(pio, "pio-map", &pio_map_len);
> >
> >> +   if (pio_map == NULL)
> >
> >> +            return -ENOENT;
> >
> >> +
> >
> >> +   pio_map_len /= sizeof(unsigned int);
> >
> >> +   if ((pio_map_len % 6) != 0) {
> >
> >> +            printk(KERN_ERR "pio-map format wrong!\n");
> >
> >> +            return -EINVAL;
> >
> >> +   }
> >
> >> +
> >
> >> +   while (pio_map_len > 0) {
> >
> >> +            /*
> >
> >> +            * column pio_map[5] from linux (has_irq) not
> >
> >> +            * supported in u-boot yet.
> >
> >> +            */
> >
> > *//*
> >
> > remove or keep pio_map[5] in uboot dts, which is better?
> 
> I think we keep it as it is in linux dts. We simply ignore it (yet) in U-Boot.
 
That's OK.

> 
> >
> >> +
> >
> >> +const struct pinctrl_ops par_io_pinctrl_ops = {
> >
> >> +   .set_state = par_io_pinctrl_set_state, };
> >
> >> +
> >
> >> +static const struct udevice_id par_io_pinctrl_match[] = {
> >
> >> +   { .compatible = "fsl,mpc8360-par_io"},
> >
> > *//*
> >
> > Why is*//*fsl,mpc8360-par_io, maybe fsl,qe-par-io are more better.*//*
> 
> Yes, more common, but in Linux is already:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kerne
> l.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree
> %2Farch%2Fpowerpc%2Fboot%2Fdts%2Fkmeter1.dts%23n133&data=02%
> 7C01%7Cqiang.zhao%40nxp.com%7C783271f84fe043eaf21708d7e0f7df46%7
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637225227517025151
> &sdata=5VZsJPI2QwZBnS5bgSDC1cTF1lKh%2BGQXHpP%2FF4jGzCU%3D&
> amp;reserved=0
> 
> or
> compatible = "fsl,mpc8323-qe-pario";
> 
> for board
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kerne
> l.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree
> %2Farch%2Fpowerpc%2Fboot%2Fdts%2Fmpc832x_rdb.dts%23n163&data
> =02%7C01%7Cqiang.zhao%40nxp.com%7C783271f84fe043eaf21708d7e0f7df4
> 6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637225227517025
> 151&sdata=AwMW4NtqID9GwbM9RnZcqqDQVZ7RvuaRw5U6LFC3VWA%
> 3D&reserved=0
> 
> Unfortunately, it is more or less a mess in linux, as each board scans in board
> code for the node *name* not compatible entry ! See as an example:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kerne
> l.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree
> %2Farch%2Fpowerpc%2Fplatforms%2F83xx%2Fmpc832x_rdb.c%23n198&amp
> ;data=02%7C01%7Cqiang.zhao%40nxp.com%7C783271f84fe043eaf21708d7e0
> f7df46%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6372252275
> 17025151&sdata=RG2pIeiGMFZwL16CLjRHWrvC12YBEDDi5H7kNoZXz0A
> %3D&reserved=0
> 
> So may we are free here, to add here a new compatible entry "fsl,qe-par-io" ?
> (But I want to be compatible with linux and accept the same compatible strings
> as linux has.)

OK!

Best Regards
Qiang Zhao