Hi Patrice,
On 31 July 2018 at 07:53, Patrice CHOTARD <patrice.chot...@st.com> wrote:
>
> Hi Simon
>
> On 07/31/2018 01:52 PM, Simon Glass wrote:
> > Hi Patrice,
> >
> > On 30 July 2018 at 09:23, Patrice Chotard <patrice.chot...@st.com> wrote:
> >> From: Patrick Delaunay <patrick.delau...@st.com>
> >>
> >> Replace setparity by more generic setconfig ops
> >> to allow uart parity, bits word length and stop bits
> >> number change.
> >>
> >> Adds SERIAL_GET_PARITY/BITS/STOP macros.
> >>
> >> Signed-off-by: Patrick Delaunay <patrick.delau...@st.com>
> >> Signed-off-by: Patrice Chotard <patrice.chot...@st.com>
> >> ---
> >>
> >> drivers/serial/serial-uclass.c | 14 ++++++++++++++
> >> include/serial.h | 42
> >> +++++++++++++++++++++++++++++++++++++++---
> >> 2 files changed, 53 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/serial/serial-uclass.c
> >> b/drivers/serial/serial-uclass.c
> >> index 321d23ee93bf..9f523751ce17 100644
> >> --- a/drivers/serial/serial-uclass.c
> >> +++ b/drivers/serial/serial-uclass.c
> >> @@ -287,6 +287,18 @@ void serial_setbrg(void)
> >> ops->setbrg(gd->cur_serial_dev, gd->baudrate);
> >> }
> >>
> >> +void serial_setconfig(u8 config)
> >> +{
> >> + struct dm_serial_ops *ops;
> >> +
> >> + if (!gd->cur_serial_dev)
> >> + return;
> >> +
> >> + ops = serial_get_ops(gd->cur_serial_dev);
> >> + if (ops->setconfig)
> >> + ops->setconfig(gd->cur_serial_dev, config);
> >> +}
> >> +
> >> void serial_stdio_init(void)
> >> {
> >> }
> >> @@ -398,6 +410,8 @@ static int serial_post_probe(struct udevice *dev)
> >> ops->pending += gd->reloc_off;
> >> if (ops->clear)
> >> ops->clear += gd->reloc_off;
> >> + if (ops->setconfig)
> >> + ops->setconfig += gd->reloc_off;
> >> #if CONFIG_POST & CONFIG_SYS_POST_UART
> >> if (ops->loop)
> >> ops->loop += gd->reloc_off
> >> diff --git a/include/serial.h b/include/serial.h
> >> index b9ef6d91c9c5..61c1362e9e32 100644
> >> --- a/include/serial.h
> >> +++ b/include/serial.h
> >> @@ -73,6 +73,39 @@ enum serial_par {
> >> SERIAL_PAR_EVEN
> >> };
> >>
> >> +#define SERIAL_PAR_MASK 0x03
> >> +#define SERIAL_PAR_SHIFT 0
> >> +#define SERIAL_GET_PARITY(config) \
> >> + ((config & SERIAL_PAR_MASK) >> SERIAL_PAR_SHIFT)
> >> +
> >> +enum serial_bits {
> >> + SERIAL_5_BITS,
> >> + SERIAL_6_BITS,
> >> + SERIAL_7_BITS,
> >> + SERIAL_8_BITS
> >> +};
> >> +
> >> +#define SERIAL_BITS_MASK 0x0C
> >> +#define SERIAL_BITS_SHIFT 2
> >
> > For consistency:
> >
> > #define SERIAL_BITS_SHIFT 2
> > #define SERIAL_BITS_MASK (0x3 << SERIAL_BITS_SHIFT)
>
> Ok
>
> >
> >> +#define SERIAL_GET_BITS(config) \
> >> + ((config & SERIAL_BITS_MASK) >> SERIAL_BITS_SHIFT)
> >> +
> >> +enum serial_stop {
> >> + SERIAL_HALF_STOP, /* 0.5 stop bit */
> >> + SERIAL_ONE_STOP, /* 1 stop bit */
> >> + SERIAL_ONE_HALF_STOP, /* 1.5 stop bit */
> >> + SERIAL_TWO_STOP /* 2 stop bit */
> >> +};
> >> +
> >> +#define SERIAL_STOP_MASK 0x30
> >> +#define SERIAL_STOP_SHIFT 4
> >
> > same here
>
> ok
>
> >
> >> +#define SERIAL_GET_STOP(config) \
> >> + ((config & SERIAL_STOP_MASK) >> SERIAL_STOP_SHIFT)
> >> +
> >> +#define SERIAL_DEFAULT_CONFIG SERIAL_PAR_NONE << SERIAL_PAR_SHIFT | \
> >> + SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
> >> + SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
> >> +
> >> /**
> >> * struct struct dm_serial_ops - Driver model serial operations
> >> *
> >> @@ -150,15 +183,18 @@ struct dm_serial_ops {
> >> int (*loop)(struct udevice *dev, int on);
> >> #endif
> >> /**
> >> - * setparity() - Set up the parity
> >> + * setconfig() - Set up the uart configuration
> >> + * (parity, 5/6/7/8 bits word length, stop bits)
> >> *
> >> - * Set up a new parity for this device.
> >> + * Set up a new config for this device.
> >> *
> >> * @dev: Device pointer
> >> * @parity: parity to use
> >> + * @bits: bits number to use
> >> + * @stop: stop bits number to use
> >> * @return 0 if OK, -ve on error
> >> */
> >> - int (*setparity)(struct udevice *dev, enum serial_par parity);
> >> + int (*setconfig)(struct udevice *dev, u8 serial_config);
> >
> > Please make this uint instead of u8. There is no point in using u8
> > since the compiler will use a register anyway. It can only make code
> > size worse, if the compile add masking, etc.
>
> ok
>
> >
> >> };
> >>
> >> /**
> >> --
> >> 1.9.1
> >>
> >
> > Also you need a serial_setconfig() function call in the uclass so
> > people can call it.
>
> I already add serial_setconfig() function call in serial-uclass at the
> beginning of this patch ;-)
OK good. Please change u8 there too :-)
>
> >
> > Perhaps that could have separate parameters for each setting, so that
> > the caller does not have to make up a mask? I'm not sure if that is
> > better or not.
>
> Don't know what is better, currently only STM32 platforms will use it,
> internally we already use this API.
OK well I think what you have will work. We can separate out the
parameters later if we see a need.
>
> >
> > Also we need to call this from sandbox code for testing purposes,
> Ok i will add a test for this.
>
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
