Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1642414 > unrolled thread

Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-16 13:10 +0200
    Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Dong Aisheng <dongas86@gmail.com> - 2017-05-17 05:40 +0200
      Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-17 07:50 +0200
        RE: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support "A.S. Dong" <aisheng.dong@nxp.com> - 2017-05-17 08:10 +0200
          Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-17 08:30 +0200
            Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Dong Aisheng <dongas86@gmail.com> - 2017-05-17 09:10 +0200
              Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-17 10:10 +0200
                Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Dong Aisheng <dongas86@gmail.com> - 2017-05-19 17:10 +0200
                  Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-05-23 07:30 +0200

#1642414 — Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support

> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
>  	}
>  	sport->port.line = ret;
>  	sport->lpuart32 = sdata->is_32;
> +	lpuart_is_be = sdata->is_be;

Setting a global variable in per-device routine is quite bad design.

[toc] | [next] | [standalone]

#1642931

On Tue, May 16, 2017 at 02:08:12PM +0300, Nikita Yushchenko wrote:
> > @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
> >  	}
> >  	sport->port.line = ret;
> >  	sport->lpuart32 = sdata->is_32;
> > +	lpuart_is_be = sdata->is_be;
> 
> Setting a global variable in per-device routine is quite bad design.
> 

There is a reason for that we don't want to change the exist 
lpuart32_read[write] API which is widely used in driver.
Making a global lpuart_is_be is the simplest way to do it.

Any strong blocking reason?

Regards
Dong Aisheng

[toc] | [prev] | [next] | [standalone]

#1642992

>>> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device *pdev)
>>>  	}
>>>  	sport->port.line = ret;
>>>  	sport->lpuart32 = sdata->is_32;
>>> +	lpuart_is_be = sdata->is_be;
>>
>> Setting a global variable in per-device routine is quite bad design.
>>
> 
> There is a reason for that we don't want to change the exist 
> lpuart32_read[write] API which is widely used in driver.
> Making a global lpuart_is_be is the simplest way to do it.
> 
> Any strong blocking reason?

Code should be consistent.

There is no good reason to have sport->lpuart32 inside sport, but
lpuart_is_be outside of it. Both these values describe properties of
particular device, and thus should be in per-device structure.

If that implies adding sport arg to lpuart32_(read|write), just do that.

[toc] | [prev] | [next] | [standalone]

#1643000

> -----Original Message-----
> From: Nikita Yushchenko [mailto:nikita.yoush@cogentembedded.com]
> Sent: Wednesday, May 17, 2017 1:44 PM
> To: Dong Aisheng
> Cc: A.S. Dong; linux-serial@vger.kernel.org; Andy Duan;
> gregkh@linuxfoundation.org; Y.B. Lu; linux-kernel@vger.kernel.org;
> stefan@agner.ch; Mingkai Hu; jslaby@suse.com; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit
> register support
> 
> >>> @@ -2000,6 +2007,7 @@ static int lpuart_probe(struct platform_device
> *pdev)
> >>>  	}
> >>>  	sport->port.line = ret;
> >>>  	sport->lpuart32 = sdata->is_32;
> >>> +	lpuart_is_be = sdata->is_be;
> >>
> >> Setting a global variable in per-device routine is quite bad design.
> >>
> >
> > There is a reason for that we don't want to change the exist
> > lpuart32_read[write] API which is widely used in driver.
> > Making a global lpuart_is_be is the simplest way to do it.
> >
> > Any strong blocking reason?
> 
> Code should be consistent.
> 

Yes.

> There is no good reason to have sport->lpuart32 inside sport, but
> lpuart_is_be outside of it. Both these values describe properties of
> particular device, and thus should be in per-device structure.
> 

That's for special case, normally we wouldn't do that.

> If that implies adding sport arg to lpuart32_(read|write), just do that.

There's another reason that we have to deal with earlycon which is
executed much early before driver probe.

And I need specificly align the endian data.
e.g.
static int __init lpuart32_early_console_setup(struct earlycon_device *device,
                                          const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = true;
        device->con->write = lpuart32_early_write;
        return 0;
}

static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
                                                   const char *opt)
{
        if (!device->port.membase)
                return -ENODEV;

        lpuart_is_be = false;
        device->port.membase += IMX_REG_OFF;
        device->con->write = lpuart32_early_write;

        return 0;
}

Regards
Dong Aisheng

[toc] | [prev] | [next] | [standalone]

#1643004

>> Code should be consistent.
>>
> 
> Yes.
> 
>> There is no good reason to have sport->lpuart32 inside sport, but
>> lpuart_is_be outside of it. Both these values describe properties of
>> particular device, and thus should be in per-device structure.
>>
> 
> That's for special case, normally we wouldn't do that.

For me this "special case" looks like "let's break data structure
consistency to reuse several lines of code".

With code snippets you show, it looks even worse: you assign same global
variable in several places for different uses. implicitly assuming that
it is for same device. Which can be true in your current system, but not
elsewhere (e.g. why not having lpuart programmed into fpga)?

Alternative solution could be - have separate write path for earlycon.
At a glance, it is dozen lines of code.

[toc] | [prev] | [next] | [standalone]

#1643025

On Wed, May 17, 2017 at 09:25:51AM +0300, Nikita Yushchenko wrote:
> >> Code should be consistent.
> >>
> > 
> > Yes.
> > 
> >> There is no good reason to have sport->lpuart32 inside sport, but
> >> lpuart_is_be outside of it. Both these values describe properties of
> >> particular device, and thus should be in per-device structure.
> >>
> > 
> > That's for special case, normally we wouldn't do that.
> 
> For me this "special case" looks like "let's break data structure
> consistency to reuse several lines of code".
> 
> With code snippets you show, it looks even worse: you assign same global
> variable in several places for different uses. 

If you mean lpuart_is_be, it's not for different uses.
The purpose is the same to align the correct endian but in two places.

> implicitly assuming that
> it is for same device. Which can be true in your current system, but not
> elsewhere (e.g. why not having lpuart programmed into fpga)?
> 

Sorry, What issues for fpga?

> Alternative solution could be - have separate write path for earlycon.

It looks to me having the same issue with a separate write patch
for earlycon as we still need distinguish Little or Big endian
for Layerscape and IMX.

> At a glance, it is dozen lines of code.

Would you please show some sample code?
Then we probably may understand better with each other.

Anyway, thanks for detailed review.

Regards
Dong Aisheng

[toc] | [prev] | [next] | [standalone]

#1643090

Hi

My view of your statement is:
- you currently assume only a few cases for this driver - builtin UART
in vf610, ls1012a and imx7,
- in each of these cases, all lpuart instances share same endian, thus
having that in global var works for these cases,
- having that in global var makes it possible for you to write less
lines of code

My complain is:
- in Linux, we are trying to keep drivers generic,
- in Linux, having less lines of code has never been sufficient to break
basic data structure consistency,
- having driver to keep per-device capability in global var is a clear
case of breaking consistency.


>>> That's for special case, normally we wouldn't do that.
>>
>> For me this "special case" looks like "let's break data structure
>> consistency to reuse several lines of code".
>>
>> With code snippets you show, it looks even worse: you assign same global
>> variable in several places for different uses. 
> 
> If you mean lpuart_is_be, it's not for different uses.
> The purpose is the same to align the correct endian but in two places.

_probe() routine called for device X alters state already in use for
device Y.

> 
>> implicitly assuming that
>> it is for same device. Which can be true in your current system, but not
>> elsewhere (e.g. why not having lpuart programmed into fpga)?
>>
> 
> Sorry, What issues for fpga?

Connect FPGA to IMX7 based system and program LS1012a version of lpuart
core into it.  Have your console on system UART broken at time when
driver gets registered.


> 
>> Alternative solution could be - have separate write path for earlycon.
> 
> It looks to me having the same issue with a separate write patch
> for earlycon as we still need distinguish Little or Big endian
> for Layerscape and IMX.
> 
>> At a glance, it is dozen lines of code.
> 
> Would you please show some sample code?

Do not reuse lpuart32_console_putchar() in earlycon code.

Have two sets of early_setup/early_write/putchar - for BE and
defaut-endian earlycon. And in these putchar's do not use
lpuart_(read|write).


As far as I can see, fsl_lpuart.c already has two drivers in one -
there is separate set of routines for 8bit and 32bit cases.
And those routines that are common, have if blocks that separate cases.
I think these drivers will be cleaner if separated.
However that's completely different story.

[toc] | [prev] | [next] | [standalone]

#1645672

On Wed, May 17, 2017 at 11:04:04AM +0300, Nikita Yushchenko wrote:
> Hi
> 
> My view of your statement is:
> - you currently assume only a few cases for this driver - builtin UART
> in vf610, ls1012a and imx7,
> - in each of these cases, all lpuart instances share same endian, thus
> having that in global var works for these cases,
> - having that in global var makes it possible for you to write less
> lines of code
> 
> My complain is:
> - in Linux, we are trying to keep drivers generic,
> - in Linux, having less lines of code has never been sufficient to break
> basic data structure consistency,
> - having driver to keep per-device capability in global var is a clear
> case of breaking consistency.
> 

Yes, i do understand your concern and i absolutely agree with the rule
you mentioned.

> 
> >>> That's for special case, normally we wouldn't do that.
> >>
> >> For me this "special case" looks like "let's break data structure
> >> consistency to reuse several lines of code".
> >>
> >> With code snippets you show, it looks even worse: you assign same global
> >> variable in several places for different uses. 
> > 
> > If you mean lpuart_is_be, it's not for different uses.
> > The purpose is the same to align the correct endian but in two places.
> 
> _probe() routine called for device X alters state already in use for
> device Y.
> 

Okay, you're saying two different types of devices appeared in one SoC.

> > 
> >> implicitly assuming that
> >> it is for same device. Which can be true in your current system, but not
> >> elsewhere (e.g. why not having lpuart programmed into fpga)?
> >>
> > 
> > Sorry, What issues for fpga?
> 
> Connect FPGA to IMX7 based system and program LS1012a version of lpuart
> core into it.  Have your console on system UART broken at time when
> driver gets registered.
> 

Well, theoretically it may happen.

> 
> > 
> >> Alternative solution could be - have separate write path for earlycon.
> > 
> > It looks to me having the same issue with a separate write patch
> > for earlycon as we still need distinguish Little or Big endian
> > for Layerscape and IMX.
> > 
> >> At a glance, it is dozen lines of code.
> > 
> > Would you please show some sample code?
> 
> Do not reuse lpuart32_console_putchar() in earlycon code.
> 
> Have two sets of early_setup/early_write/putchar - for BE and
> defaut-endian earlycon. And in these putchar's do not use
> lpuart_(read|write).
> 

Isn't that introducing another consistency break after fix one
consistency break?

If doing that, we then have two register read/write APIs.
One for normal driver operation by dynamically checking lpuart_is_be
property to distinguish the endian difference problem.
Another is specifically implemented for only early console read/write
and use hardcoded way to read/write register directly instead of using
the standard API lpuart32_read/write, like follows:
e.g.
lpuart32_le_console_write() {
	 writel();
}

lpuart32_be_console_write() {
	 iowrite32be()
}
This also makes the driver a bit strange and ugly.

It looks to me both way are trade offs and the later one seems sacrifice
more. And i doubt if it's really necessary for probably a no real gain
purpose as the FPGA you mentioned is a theoretical case and less
possibility to exist.

I'm still wondering how about keep using the exist way and adding more
information in code to explain why use a global var?

Regards
Dong Aisheng

> 
> As far as I can see, fsl_lpuart.c already has two drivers in one -
> there is separate set of routines for 8bit and 32bit cases.
> And those routines that are common, have if blocks that separate cases.
> I think these drivers will be cleaner if separated.
> However that's completely different story.

[toc] | [prev] | [next] | [standalone]

#1647643

Hi,

>>>> Alternative solution could be - have separate write path for earlycon.
>>>
>>> It looks to me having the same issue with a separate write patch
>>> for earlycon as we still need distinguish Little or Big endian
>>> for Layerscape and IMX.
>>>
>>>> At a glance, it is dozen lines of code.
>>>
>>> Would you please show some sample code?
>>
>> Do not reuse lpuart32_console_putchar() in earlycon code.
>>
>> Have two sets of early_setup/early_write/putchar - for BE and
>> defaut-endian earlycon. And in these putchar's do not use
>> lpuart_(read|write).
>>
> 
> Isn't that introducing another consistency break after fix one
> consistency break?
> 
> If doing that, we then have two register read/write APIs.
> One for normal driver operation by dynamically checking lpuart_is_be
> property to distinguish the endian difference problem.
> Another is specifically implemented for only early console read/write
> and use hardcoded way to read/write register directly instead of using
> the standard API lpuart32_read/write, like follows:
> e.g.
> lpuart32_le_console_write() {
> 	 writel();
> }
> 
> lpuart32_be_console_write() {
> 	 iowrite32be()
> }
> This also makes the driver a bit strange and ugly.
> 
> It looks to me both way are trade offs and the later one seems sacrifice
> more. And i doubt if it's really necessary for probably a no real gain
> purpose as the FPGA you mentioned is a theoretical case and less
> possibility to exist.
> 
> I'm still wondering how about keep using the exist way and adding more
> information in code to explain why use a global var?

I've checked other driver under drivers/tty/serial/, for examples of
similar cases.

Please look at serial8250_early_in() / serial8250_early_out() ?
These do handle different endian, via port->iotype

Another example is drivers/tty/serial/samsung.c, where
port->private_data is initialized and used.

Nikita

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web