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

[Heiko Schocher]

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).

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 ?

>> @@ -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.

> 
>> 
> 
>>  #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.

> 
>> +
> 
>> +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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/boot/dts/kmeter1.dts#n133

or
compatible = "fsl,mpc8323-qe-pario";

for board
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/boot/dts/mpc832x_rdb.dts#n163

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://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/83xx/mpc832x_rdb.c#n198

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.)

> 
>> +   { /* sentinel */ }
> 
>> +};
> 
>> +
> 
>> +U_BOOT_DRIVER(par_io_pinctrl) = {
> 
>> +   .name = "par-io-pinctrl",
> 
>> +   .id = UCLASS_PINCTRL,
> 
>> +   .of_match = of_match_ptr(par_io_pinctrl_match),
> 
>> +   .probe = par_io_pinctrl_probe,
> 
>> +   .ofdata_to_platdata = qe_io_ofdata_to_platdata,
> 
>> +   .platdata_auto_alloc_size = sizeof(struct qe_io_platdata),
> 
>> +   .ops = &par_io_pinctrl_ops,
> 
>> +#if CONFIG_IS_ENABLED(OF_CONTROL)
> 
>> && !CONFIG_IS_ENABLED(OF_PLATDATA)
> 
>> +   .flags        = DM_FLAG_PRE_RELOC,
> 
>> +#endif
> 
>> +};
> 
>> +#endif
> 
>> diff --git a/include/fsl_qe.h b/include/fsl_qe.h index d4eba82436..cb5c91bdf1
> 
>> 100644
> 
>> --- a/include/fsl_qe.h
> 
>> +++ b/include/fsl_qe.h
> 
>> @@ -295,4 +295,7 @@ int u_qe_firmware_resume(const struct qe_firmware
> 
>> *firmware,
> 
>>                      qe_map_t *qe_immrr);
> 
>>  #endif
> 
>> 
> 
>> +#if defined(CONFIG_PINCTRL)
> 
>> +int par_io_of_config(struct udevice *dev); #endif
> 
>>  #endif /* __QE_H__ */
> 
> *//*
> 
> I don’t find this function is called anywhere.
> 
>> --
> 
>> 2.24.1
> 
> Best Regards
> 
> Qiang Zhao
> 

Thanks for your comments

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de