| Message ID | 20200218090518.1506961-1-hs@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] powerpc, qe: add DTS support for parallel I/O ports | expand |
Hello Priyanka, Am 18.02.2020 um 10:05 schrieb Heiko Schocher: > add DM support for parallel I/O ports on QUICC Engine Block > > Signed-off-by: Heiko Schocher <hs at denx.de> > --- > Travis build: > > https://travis-ci.org/hsdenx/u-boot-test/builds/651400509 > > Open questions / discussion: > > - may we should move this part to drivers/pinctrl ? > > - I let the old none DM based implementation in code > so boards should work with old implementation. > > This should be removed if all boards are converted to > DM/DTS. > > - Unfortunately linux DTS does not use "pinctrl-" > properties, instead "pio-handle" properties. > > Even worser old U-Boot code initializes all pins > defined in "const qe_iop_conf_t qe_iop_conf_tab[]" > table in board code. As linux does the same I decided > to also scan through all subnodes containing "pio-map" > property and initialize them too. > > The proper solution would be to check for "pio-handle" > when a device is probed. > > > 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(-) Any comments? Thanks! bye, Heiko
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? > @@ -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); > > #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? > + > +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. > + { /* 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
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
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& > ;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
Hello Qiang Zhao, Am 15.04.2020 um 10:10 schrieb 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! Yes, on the following boards: "kmcoge5ne tuxx1 kmopti2 kmtegr1 tuge1 kmsupx5" Not all in mainline (yet) @Holger: should/could we mainline the missing ones? This boards work with full DM/DTS support now, and have no compiletime warnign like: ===================== WARNING ====================== This board does not use CONFIG_DM_ETH (Driver Model anymore... > Would you like to push the dts patch too? Yes, but give me some time, until I can test the changed patchset again. >> 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. Ok, I must look into it. > >> >>>> @@ -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. I remove them in seperate patch. > >> >>> >>>> >>> >>>> ?#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& >> ;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 > bye, Heiko
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 @@ -11,6 +11,9 @@ #ifdef CONFIG_USB_EHCI_FSL #include <usb/ehci-ci.h> #endif +#ifdef CONFIG_QE +#include <fsl_qe.h> +#endif #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); +#if !defined(CONFIG_PINCTRL) static void config_qe_ioports(void) { u8 port, pin; @@ -43,6 +47,7 @@ static void config_qe_ioports(void) } } #endif +#endif /* * Breathe some life into the CPU... @@ -189,10 +194,13 @@ void cpu_init_f (volatile immap_t * im) __raw_writel(CONFIG_SYS_OBIR, &im->sysconf.obir); #endif +#if !defined(CONFIG_PINCTRL) #ifdef CONFIG_QE /* Config QE ioports */ config_qe_ioports(); #endif +#endif + /* Set up preliminary BR/OR regs */ init_early_memctl_regs(); diff --git a/arch/powerpc/cpu/mpc83xx/qe_io.c b/arch/powerpc/cpu/mpc83xx/qe_io.c index 88aa689551..f6939dc920 100644 --- a/arch/powerpc/cpu/mpc83xx/qe_io.c +++ b/arch/powerpc/cpu/mpc83xx/qe_io.c @@ -11,16 +11,48 @@ #include <asm/io.h> #include <asm/immap_83xx.h> +#if defined(CONFIG_PINCTRL) +#include <dm.h> +#include <dm/device_compat.h> +#include <dm/pinctrl.h> +#include <linux/ioport.h> + +/** + * struct qe_io_platdata + * + * @base: Base register address + * @num_par_io_ports number of io ports + */ +struct qe_io_platdata { + volatile qepio83xx_t *base; + u32 num_io_ports; +}; +#endif + #define NUM_OF_PINS 32 -void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign) + +/** qe_cfg_iopin configure one io pin setting + * + * @par_io: pointer to parallel I/O base + * @port: io pin port + * @pin: io pin number which get configured + * @dir: direction of io pin 2 bits valid + * 00 = pin disabled + * 01 = output + * 10 = input + * 11 = pin is I/O + * @open_drain: is pin open drain + * @assign: pin assignment registers select the function of the pin + */ +static void +qe_cfg_iopin(volatile qepio83xx_t *par_io, u8 port, u8 pin, int dir, + int open_drain, int assign) { u32 pin_2bit_mask; u32 pin_2bit_dir; u32 pin_2bit_assign; u32 pin_1bit_mask; u32 tmp_val; - volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR; - volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio; /* Calculate pin location and 2bit mask and dir */ pin_2bit_mask = (u32)(0x3 << (NUM_OF_PINS-(pin%(NUM_OF_PINS/2)+1)*2)); @@ -66,3 +98,158 @@ void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign) out_be32(&par_io->ioport[port].ppar1, pin_2bit_assign | tmp_val); } } + +#if !defined(CONFIG_PINCTRL) +/** qe_config_iopin configure one io pin setting + * + * @port: io pin port + * @pin: io pin number which get configured + * @dir: direction of io pin 2 bits valid + * 00 = pin disabled + * 01 = output + * 10 = input + * 11 = pin is I/O + * @open_drain: is pin open drain + * @assign: pin assignment registers select the function of the pin + */ +void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign) +{ + volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR; + volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio; + + qe_cfg_iopin(par_io, port, pin, dir, open_drain, assign); +} +#else +static int qe_io_ofdata_to_platdata(struct udevice *dev) +{ + struct qe_io_platdata *plat = dev->platdata; + fdt_addr_t addr; + + addr = dev_read_addr(dev); + if (addr == FDT_ADDR_T_NONE) + return -EINVAL; + + plat->base = (qepio83xx_t *)addr; + if (dev_read_u32(dev, "num-ports", &plat->num_io_ports)) + return -EINVAL; + + return 0; +} + +/** + * 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. + */ + qe_cfg_iopin(par_io, (u8) pio_map[0], (u8) pio_map[1], + (int) pio_map[2], (int) pio_map[3], + (int) pio_map[4]); + pio_map += 6; + pio_map_len -= 6; + } + return 0; +} + +int par_io_of_config(struct udevice *dev) +{ + u32 phandle; + ofnode pio; + int err; + + err = ofnode_read_u32(dev_ofnode(dev), "pio-handle", &phandle); + if (err) { + dev_err(dev, "%s: pio-handle not available\n", __func__); + return err; + } + + pio = ofnode_get_by_phandle(phandle); + if (!ofnode_valid(pio)) { + dev_err(dev, "%s: unable to find node\n", __func__); + return -EINVAL; + } + + /* To Do: find pinctrl device and pass it */ + return par_io_of_config_node(NULL, pio); +} + +/* + * This is not nice! + * pinsettings should work with "pinctrl-" properties. + * Unfortunately on mpc83xx powerpc linux device trees + * devices handle this with "pio-handle" properties ... + * + * Even worser, old board code inits all par_io + * pins in one step, if U-Boot uses the device + * or not. So init all par_io definitions here too + * as linux does this also. + */ +static void config_qe_ioports(struct udevice *dev) +{ + ofnode ofn; + + for (ofn = dev_read_first_subnode(dev); ofnode_valid(ofn); + ofn = dev_read_next_subnode(ofn)) { + /* + * ignore errors here, as may the subnode + * has no pio-handle + */ + par_io_of_config_node(dev, ofn); + } +} + +static int par_io_pinctrl_probe(struct udevice *dev) +{ + config_qe_ioports(dev); + + return 0; +} + +static int par_io_pinctrl_set_state(struct udevice *dev, struct udevice *config) +{ + return 0; +} + +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"}, + { /* 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__ */
add DM support for parallel I/O ports on QUICC Engine Block Signed-off-by: Heiko Schocher <hs at denx.de> --- Travis build: https://travis-ci.org/hsdenx/u-boot-test/builds/651400509 Open questions / discussion: - may we should move this part to drivers/pinctrl ? - I let the old none DM based implementation in code so boards should work with old implementation. This should be removed if all boards are converted to DM/DTS. - Unfortunately linux DTS does not use "pinctrl-" properties, instead "pio-handle" properties. Even worser old U-Boot code initializes all pins defined in "const qe_iop_conf_t qe_iop_conf_tab[]" table in board code. As linux does the same I decided to also scan through all subnodes containing "pio-map" property and initialize them too. The proper solution would be to check for "pio-handle" when a device is probed. 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(-)