Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Wed, Nov 21, 2018 at 12:04 AM Alexander Graf wrote: > On 20.11.18 19:32, Andy Shevchenko wrote: > > On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote: > >> On 15 November 2018 at 09:58, Andy Shevchenko > >> wrote: > >>> + u64 addr; > >> > >> ulong > > > > This, unfortunately, will not work. ACPI takes the address as 32-bit halves, > > and shift to 32 on 32-bit platform is UB. > > I guess what Simon wanted to get to is more something like "phys_addr_t" > which actually tells you what the variable is supposed to transfer. > > Keep in mind that this is only for interim data. You can put this into a > u64 before putting it into the ACPI table inside your ACPI specific code > just fine. I have learned today that U-Boot inherited lower_32_bits() and upper_32_bits() macros, so, I use ulong. -- With Best Regards, Andy Shevchenko ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On 20.11.18 19:32, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote:
>> On 15 November 2018 at 09:58, Andy Shevchenko
>> wrote:
>
>>> +/* REVISIT: ACPI GAS specification implied */
>>> +struct serial_device_info {
>>> + unsigned int baudrate;
>>> + u8 addr_space; /* 0 - MMIO, 1 - IO */
>>
>> Please make this an enum
>
> OK.
>
>>
>>> + u8 reg_width;
>>> + u8 reg_offset;
>>> + u8 reg_shift;
>>> + u64 addr;
>>
>> ulong
>
> This, unfortunately, will not work. ACPI takes the address as 32-bit halves,
> and shift to 32 on 32-bit platform is UB.
I guess what Simon wanted to get to is more something like "phys_addr_t"
which actually tells you what the variable is supposed to transfer.
Keep in mind that this is only for interim data. You can put this into a
u64 before putting it into the ACPI table inside your ACPI specific code
just fine.
Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Thu, Nov 15, 2018 at 11:45:32AM -0800, Simon Glass wrote:
> On 15 November 2018 at 09:58, Andy Shevchenko
> wrote:
> > +/* REVISIT: ACPI GAS specification implied */
> > +struct serial_device_info {
> > + unsigned int baudrate;
> > + u8 addr_space; /* 0 - MMIO, 1 - IO */
>
> Please make this an enum
OK.
>
> > + u8 reg_width;
> > + u8 reg_offset;
> > + u8 reg_shift;
> > + u64 addr;
>
> ulong
This, unfortunately, will not work. ACPI takes the address as 32-bit halves,
and shift to 32 on 32-bit platform is UB.
> Needs a struct comment as I don't know what most of these do.
OK.
> What about parity, number of bits, etc?
As discussed before, it will be filled thru getconfig().
Though, I would add necessary members explicitly.
--
With Best Regards,
Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Thu, Nov 15, 2018 at 09:09:55PM +0100, Alexander Graf wrote: > > > On 15.11.18 20:31, Andy Shevchenko wrote: > > On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf wrote: > >> On 15.11.18 18:58, Andy Shevchenko wrote: > >>> New callback will give a necessary information to fill up ACPI SPCR table, > >>> for example. Maybe used later for other purposes. > > > >>> +/* REVISIT: ACPI GAS specification implied */ > >> > >> What does this REVISIT tag mean? > > > > Had you chance to read my cover letter? > > There is a section called "Known issues", item 3 there might answer to > > your question. > > The usual tag for "not finalized, please don't apply yet" in upstream > patch submissions is "RFC". If you don't think your code is ready to be > applied, just tag the patches with RFC, like this: > > [RFC] serial: Introduce ->getinfo() callback > > Then everyone knows that you don't think the patch is good enough yet. > Otherwise, every submission really implies that you think it should get > applied. > > As for the data structure itself, I think I gave you a bit of feedback. > I wouldn't worry too much about getting everything right from the start, > as this is a monolithic open source project. So changing the structure > later on is easy. Thanks, noted. -- With Best Regards, Andy Shevchenko ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Thu, Nov 15, 2018 at 12:22:31PM -0800, Simon Glass wrote: > On 15 November 2018 at 11:51, Andy Shevchenko > wrote: > > On Thu, Nov 15, 2018 at 9:46 PM Simon Glass wrote: > >> On 15 November 2018 at 09:58, Andy Shevchenko > >> wrote: > >> > +int serial_getinfo(struct serial_device_info *info) > >> > >> This should use driver model, so: > >> > >> int serial_getinfo(struct udevice *dev, struct serial_device_info *info) > > > > Oh, sure! I have to withdraw this comment based on two points: - the rest of API is using it in this way - it is all about current console IIUC, so, anyway we would need the same code to retrieve it Are you still thinking that serial_getinfo(dev, info) would be preferable? -- With Best Regards, Andy Shevchenko ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
Hi Andy,
On 15 November 2018 at 11:51, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 9:46 PM Simon Glass wrote:
>> On 15 November 2018 at 09:58, Andy Shevchenko
>> wrote:
>> >
>> > New callback will give a necessary information to fill up ACPI SPCR table,
>> > for example. Maybe used later for other purposes.
>
>> Seems useful to me.
>
> Thanks. What do you think about introducing ->getconfig() at some point?
Let's do it.
>
>> Please add a test to test/dm/serial.c
>
> Will do.
>
>> > +int serial_getinfo(struct serial_device_info *info)
>>
>> This should use driver model, so:
>>
>> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
>
> Oh, sure!
>
>> > +/* REVISIT: ACPI GAS specification implied */
>> > +struct serial_device_info {
>> > + unsigned int baudrate;
>> > + u8 addr_space; /* 0 - MMIO, 1 - IO */
>>
>> Please make this an enum
>>
>> > + u8 reg_width;
>> > + u8 reg_offset;
>> > + u8 reg_shift;
>> > + u64 addr;
>>
>> ulong
>>
>> Needs a struct comment as I don't know what most of these do.
>>
>> What about parity, number of bits, etc?
>
> What about splitting up some parameters structure with U-Boot defined
> semantics?
> In the acpi_create_spcr() we can convert it to what ACPI expects w/o
> polluting U-Boot generic code.
>
> That's why it has "REVISIT" tag, I would like to hear proposals how
> these data structures should look like. Also I have no clue about
> non-16550 drivers.
Well so long as you document the struct members I think this is fine.
But I think you should add serial-format info (the 8n1 / 7e1 business)
to this.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On 15.11.18 20:31, Andy Shevchenko wrote: > On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf wrote: >> On 15.11.18 18:58, Andy Shevchenko wrote: >>> New callback will give a necessary information to fill up ACPI SPCR table, >>> for example. Maybe used later for other purposes. > >>> +/* REVISIT: ACPI GAS specification implied */ >> >> What does this REVISIT tag mean? > > Had you chance to read my cover letter? > There is a section called "Known issues", item 3 there might answer to > your question. The usual tag for "not finalized, please don't apply yet" in upstream patch submissions is "RFC". If you don't think your code is ready to be applied, just tag the patches with RFC, like this: [RFC] serial: Introduce ->getinfo() callback Then everyone knows that you don't think the patch is good enough yet. Otherwise, every submission really implies that you think it should get applied. As for the data structure itself, I think I gave you a bit of feedback. I wouldn't worry too much about getting everything right from the start, as this is a monolithic open source project. So changing the structure later on is easy. Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Thu, Nov 15, 2018 at 9:46 PM Simon Glass wrote:
> On 15 November 2018 at 09:58, Andy Shevchenko
> wrote:
> >
> > New callback will give a necessary information to fill up ACPI SPCR table,
> > for example. Maybe used later for other purposes.
> Seems useful to me.
Thanks. What do you think about introducing ->getconfig() at some point?
> Please add a test to test/dm/serial.c
Will do.
> > +int serial_getinfo(struct serial_device_info *info)
>
> This should use driver model, so:
>
> int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
Oh, sure!
> > +/* REVISIT: ACPI GAS specification implied */
> > +struct serial_device_info {
> > + unsigned int baudrate;
> > + u8 addr_space; /* 0 - MMIO, 1 - IO */
>
> Please make this an enum
>
> > + u8 reg_width;
> > + u8 reg_offset;
> > + u8 reg_shift;
> > + u64 addr;
>
> ulong
>
> Needs a struct comment as I don't know what most of these do.
>
> What about parity, number of bits, etc?
What about splitting up some parameters structure with U-Boot defined semantics?
In the acpi_create_spcr() we can convert it to what ACPI expects w/o
polluting U-Boot generic code.
That's why it has "REVISIT" tag, I would like to hear proposals how
these data structures should look like. Also I have no clue about
non-16550 drivers.
> > +};
--
With Best Regards,
Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
Hi Andy,
On 15 November 2018 at 09:58, Andy Shevchenko
wrote:
>
> New callback will give a necessary information to fill up ACPI SPCR table,
> for example. Maybe used later for other purposes.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/serial/serial-uclass.c | 21 +
> include/common.h | 3 +++
> include/serial.h | 17 +
> 3 files changed, 41 insertions(+)
>
Seems useful to me.
Please add a test to test/dm/serial.c
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 665cca85cb..274734d059 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -308,6 +308,25 @@ int serial_setconfig(uint config)
> return 0;
> }
>
> +int serial_getinfo(struct serial_device_info *info)
This should use driver model, so:
int serial_getinfo(struct udevice *dev, struct serial_device_info *info)
> +{
> + struct dm_serial_ops *ops;
> +
> + if (!gd->cur_serial_dev)
> + return -ENODEV;
> +
> + if (!info)
> + return -EINVAL;
> +
> + info->baudrate = gd->baudrate;
> +
> + ops = serial_get_ops(gd->cur_serial_dev);
> + if (ops->getinfo)
> + return ops->getinfo(gd->cur_serial_dev, info);
> +
> + return -EINVAL;
> +}
> +
> void serial_stdio_init(void)
> {
> }
> @@ -425,6 +444,8 @@ static int serial_post_probe(struct udevice *dev)
> if (ops->loop)
> ops->loop += gd->reloc_off;
> #endif
> + if (ops->getinfo)
> + ops->getinfo += gd->reloc_off;
> #endif
> /* Set the baud rate */
> if (ops->setbrg) {
> diff --git a/include/common.h b/include/common.h
> index 8b9f859c07..1f9c98e735 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -349,6 +349,8 @@ void smp_set_core_boot_addr(unsigned long addr, int
> corenr);
> void smp_kick_all_cpus(void);
>
> /* $(CPU)/serial.c */
> +struct serial_device_info;
> +
> intserial_init (void);
> void serial_setbrg (void);
> void serial_putc (const char);
> @@ -357,6 +359,7 @@ voidserial_puts (const char *);
> intserial_getc (void);
> intserial_tstc (void);
> intserial_setconfig(uint config);
> +intserial_getinfo(struct serial_device_info *info);
See above - please don't add any more non-DM functions.
>
> /* $(CPU)/speed.c */
> intget_clocks (void);
> diff --git a/include/serial.h b/include/serial.h
> index 020cd392e8..33531b7791 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -111,6 +111,16 @@ enum serial_stop {
> SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
> SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
>
> +/* REVISIT: ACPI GAS specification implied */
> +struct serial_device_info {
> + unsigned int baudrate;
> + u8 addr_space; /* 0 - MMIO, 1 - IO */
Please make this an enum
> + u8 reg_width;
> + u8 reg_offset;
> + u8 reg_shift;
> + u64 addr;
ulong
Needs a struct comment as I don't know what most of these do.
What about parity, number of bits, etc?
> +};
> +
> /**
> * struct struct dm_serial_ops - Driver model serial operations
> *
> @@ -201,6 +211,13 @@ struct dm_serial_ops {
> * @return 0 if OK, -ve on error
> */
> int (*setconfig)(struct udevice *dev, uint serial_config);
> + /**
> +* getinfo() - Get serial device information
> +*
> +* @dev: Device pointer
> +* @info: struct serial_device_info to fill
> +*/
> + int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
This bit looks good
> };
>
> /**
> --
> 2.19.1
>
Regards,
SImon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On Thu, Nov 15, 2018 at 8:22 PM Alexander Graf wrote:
> On 15.11.18 18:58, Andy Shevchenko wrote:
> > New callback will give a necessary information to fill up ACPI SPCR table,
> > for example. Maybe used later for other purposes.
> > +/* REVISIT: ACPI GAS specification implied */
>
> What does this REVISIT tag mean?
Had you chance to read my cover letter?
There is a section called "Known issues", item 3 there might answer to
your question.
> > +struct serial_device_info {
> > + unsigned int baudrate;
> > + u8 addr_space; /* 0 - MMIO, 1 - IO */
> > + u8 reg_width;
> > + u8 reg_offset;
> > + u8 reg_shift;
> > + u64 addr;
> > +};
--
With Best Regards,
Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/5] serial: Introduce ->getinfo() callback
On 15.11.18 18:58, Andy Shevchenko wrote:
> New callback will give a necessary information to fill up ACPI SPCR table,
> for example. Maybe used later for other purposes.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/serial/serial-uclass.c | 21 +
> include/common.h | 3 +++
> include/serial.h | 17 +
> 3 files changed, 41 insertions(+)
>
> diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> index 665cca85cb..274734d059 100644
> --- a/drivers/serial/serial-uclass.c
> +++ b/drivers/serial/serial-uclass.c
> @@ -308,6 +308,25 @@ int serial_setconfig(uint config)
> return 0;
> }
>
> +int serial_getinfo(struct serial_device_info *info)
> +{
> + struct dm_serial_ops *ops;
> +
> + if (!gd->cur_serial_dev)
> + return -ENODEV;
> +
> + if (!info)
> + return -EINVAL;
> +
> + info->baudrate = gd->baudrate;
> +
> + ops = serial_get_ops(gd->cur_serial_dev);
> + if (ops->getinfo)
> + return ops->getinfo(gd->cur_serial_dev, info);
> +
> + return -EINVAL;
> +}
> +
> void serial_stdio_init(void)
> {
> }
> @@ -425,6 +444,8 @@ static int serial_post_probe(struct udevice *dev)
> if (ops->loop)
> ops->loop += gd->reloc_off;
> #endif
> + if (ops->getinfo)
> + ops->getinfo += gd->reloc_off;
> #endif
> /* Set the baud rate */
> if (ops->setbrg) {
> diff --git a/include/common.h b/include/common.h
> index 8b9f859c07..1f9c98e735 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -349,6 +349,8 @@ void smp_set_core_boot_addr(unsigned long addr, int
> corenr);
> void smp_kick_all_cpus(void);
>
> /* $(CPU)/serial.c */
> +struct serial_device_info;
> +
> int serial_init (void);
> void serial_setbrg (void);
> void serial_putc (const char);
> @@ -357,6 +359,7 @@ void serial_puts (const char *);
> int serial_getc (void);
> int serial_tstc (void);
> int serial_setconfig(uint config);
> +int serial_getinfo(struct serial_device_info *info);
>
> /* $(CPU)/speed.c */
> int get_clocks (void);
> diff --git a/include/serial.h b/include/serial.h
> index 020cd392e8..33531b7791 100644
> --- a/include/serial.h
> +++ b/include/serial.h
> @@ -111,6 +111,16 @@ enum serial_stop {
> SERIAL_8_BITS << SERIAL_BITS_SHIFT | \
> SERIAL_ONE_STOP << SERIAL_STOP_SHIFT
>
> +/* REVISIT: ACPI GAS specification implied */
What does this REVISIT tag mean?
Alex
> +struct serial_device_info {
> + unsigned int baudrate;
> + u8 addr_space; /* 0 - MMIO, 1 - IO */
> + u8 reg_width;
> + u8 reg_offset;
> + u8 reg_shift;
> + u64 addr;
> +};
> +
> /**
> * struct struct dm_serial_ops - Driver model serial operations
> *
> @@ -201,6 +211,13 @@ struct dm_serial_ops {
>* @return 0 if OK, -ve on error
>*/
> int (*setconfig)(struct udevice *dev, uint serial_config);
> + /**
> + * getinfo() - Get serial device information
> + *
> + * @dev: Device pointer
> + * @info: struct serial_device_info to fill
> + */
> + int (*getinfo)(struct udevice *dev, struct serial_device_info *info);
> };
>
> /**
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
