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

Groups > linux.kernel > #1642421 > unrolled thread

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

Started byNikita Yushchenko <nikita.yoush@cogentembedded.com>
First post2017-05-16 13:20 +0200
Last post2017-05-17 12:00 +0200
Articles 8 — 3 participants

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:20 +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:40 +0200
        Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Dong Aisheng <dongas86@gmail.com> - 2017-05-17 07:50 +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:00 +0200
            Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register  support Dong Aisheng <dongas86@gmail.com> - 2017-05-17 08:20 +0200
          Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-17 12:00 +0200
    Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-17 12:00 +0200

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

FromNikita Yushchenko <nikita.yoush@cogentembedded.com>
Date2017-05-16 13:20 +0200
SubjectRe: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Message-ID<tHIGf-4Ub-17@gated-at.bofh.it>
>  static u32 lpuart32_read(void __iomem *addr)
>  {
> -	return ioread32be(addr);
> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
>  }
>  
>  static void lpuart32_write(u32 val, void __iomem *addr)
>  {
> -	iowrite32be(val, addr);
> +	if (lpuart_is_be)
> +		iowrite32be(val, addr);
> +	else
> +		writel(val, addr);
>  }

What if this is ever executed on big endian system?

[toc] | [next] | [standalone]

#1642930

FromDong Aisheng <dongas86@gmail.com>
Date2017-05-17 05:40 +0200
Message-ID<tHXYC-6cB-5@gated-at.bofh.it>
In reply to#1642421
On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >  static u32 lpuart32_read(void __iomem *addr)
> >  {
> > -	return ioread32be(addr);
> > +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
> >  }
> >  
> >  static void lpuart32_write(u32 val, void __iomem *addr)
> >  {
> > -	iowrite32be(val, addr);
> > +	if (lpuart_is_be)
> > +		iowrite32be(val, addr);
> > +	else
> > +		writel(val, addr);
> >  }
> 
> What if this is ever executed on big endian system?
> 

Sorry, not catching the point...

What issues will meet?

Regards
Dong Aisheng

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

#1642984

FromNikita Yushchenko <nikita.yoush@cogentembedded.com>
Date2017-05-17 07:40 +0200
Message-ID<tHZQJ-7s4-3@gated-at.bofh.it>
In reply to#1642930

17.05.2017 06:39, Dong Aisheng wrote:
> On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>>>  static u32 lpuart32_read(void __iomem *addr)
>>>  {
>>> -	return ioread32be(addr);
>>> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>>  }
>>>  
>>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>>  {
>>> -	iowrite32be(val, addr);
>>> +	if (lpuart_is_be)
>>> +		iowrite32be(val, addr);
>>> +	else
>>> +		writel(val, addr);
>>>  }
>>
>> What if this is ever executed on big endian system?
>>
> 
> Sorry, not catching the point...
> 
> What issues will meet?

Isn't writel() in host endian?

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

#1642993

FromDong Aisheng <dongas86@gmail.com>
Date2017-05-17 07:50 +0200
Message-ID<tI00p-7v7-23@gated-at.bofh.it>
In reply to#1642984
On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> 
> 
> 17.05.2017 06:39, Dong Aisheng wrote:
> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >>>  static u32 lpuart32_read(void __iomem *addr)
> >>>  {
> >>> -	return ioread32be(addr);
> >>> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
> >>>  }
> >>>  
> >>>  static void lpuart32_write(u32 val, void __iomem *addr)
> >>>  {
> >>> -	iowrite32be(val, addr);
> >>> +	if (lpuart_is_be)
> >>> +		iowrite32be(val, addr);
> >>> +	else
> >>> +		writel(val, addr);
> >>>  }
> >>
> >> What if this is ever executed on big endian system?
> >>
> > 
> > Sorry, not catching the point...
> > 
> > What issues will meet?
> 
> Isn't writel() in host endian?

On big endian systems, it is supposed to run iowrite32be.

Regards
Dong Aisheng

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

#1642998

FromNikita Yushchenko <nikita.yoush@cogentembedded.com>
Date2017-05-17 08:00 +0200
Message-ID<tI0a6-7yt-3@gated-at.bofh.it>
In reply to#1642993
>>>>>  static u32 lpuart32_read(void __iomem *addr)
>>>>>  {
>>>>> -	return ioread32be(addr);
>>>>> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>>>>  }
>>>>>  
>>>>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>>>>  {
>>>>> -	iowrite32be(val, addr);
>>>>> +	if (lpuart_is_be)
>>>>> +		iowrite32be(val, addr);
>>>>> +	else
>>>>> +		writel(val, addr);
>>>>>  }
>>>>
>>>> What if this is ever executed on big endian system?
>>>>
>>>
>>> Sorry, not catching the point...
>>>
>>> What issues will meet?
>>
>> Isn't writel() in host endian?
> 
> On big endian systems, it is supposed to run iowrite32be.

Your code states, "force BE if lpuart_is_be, don't care otherwise".
This semantics looks questionable for code reviewer.
If driver handles endian, should't it be explicit in both cases?
And if indeed driver means handling BE explicitly, but don't caring
otherwise, maybe variable name should suggest that (i.e. "force_be")?

Although driver maintainer could think differently. I won't insist on this.

Nikita

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

#1643003

FromDong Aisheng <dongas86@gmail.com>
Date2017-05-17 08:20 +0200
Message-ID<tI0tr-7Un-1@gated-at.bofh.it>
In reply to#1642998
On Wed, May 17, 2017 at 08:50:39AM +0300, Nikita Yushchenko wrote:
> >>>>>  static u32 lpuart32_read(void __iomem *addr)
> >>>>>  {
> >>>>> -	return ioread32be(addr);
> >>>>> +	return lpuart_is_be ? ioread32be(addr) : readl(addr);
> >>>>>  }
> >>>>>  
> >>>>>  static void lpuart32_write(u32 val, void __iomem *addr)
> >>>>>  {
> >>>>> -	iowrite32be(val, addr);
> >>>>> +	if (lpuart_is_be)
> >>>>> +		iowrite32be(val, addr);
> >>>>> +	else
> >>>>> +		writel(val, addr);
> >>>>>  }
> >>>>
> >>>> What if this is ever executed on big endian system?
> >>>>
> >>>
> >>> Sorry, not catching the point...
> >>>
> >>> What issues will meet?
> >>
> >> Isn't writel() in host endian?
> > 
> > On big endian systems, it is supposed to run iowrite32be.
> 
> Your code states, "force BE if lpuart_is_be, don't care otherwise".
> This semantics looks questionable for code reviewer.
> If driver handles endian, should't it be explicit in both cases?
> And if indeed driver means handling BE explicitly, but don't caring
> otherwise, maybe variable name should suggest that (i.e. "force_be")?
> 

For lpuart32, only two cases that LS platforms is Big endian while
IMX is little endian.

It's SoC IP native property, i don't think force_be is better.

Regards
Dong Aisheng

> Although driver maintainer could think differently. I won't insist on this.
> 
> Nikita

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

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

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-05-17 12:00 +0200
SubjectRe: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Message-ID<tI3Um-1pv-33@gated-at.bofh.it>
In reply to#1642993
On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
>>
>>
>> 17.05.2017 06:39, Dong Aisheng wrote:
>> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
>> >>>  static u32 lpuart32_read(void __iomem *addr)
>> >>>  {
>> >>> - return ioread32be(addr);
>> >>> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
>> >>>  }
>> >>>
>> >>>  static void lpuart32_write(u32 val, void __iomem *addr)
>> >>>  {
>> >>> - iowrite32be(val, addr);
>> >>> + if (lpuart_is_be)
>> >>> +         iowrite32be(val, addr);
>> >>> + else
>> >>> +         writel(val, addr);
>> >>>  }
>> >>
>> >> What if this is ever executed on big endian system?
>> >>
>> >
>> > Sorry, not catching the point...
>> >
>> > What issues will meet?
>>
>> Isn't writel() in host endian?
>
> On big endian systems, it is supposed to run iowrite32be.

It looks like you substituting *bus* side with CPU *side* of communication.

If you are talking about CPU side
__raw_readl() / __raw_writel() will do the trick.

-- 
With Best Regards,
Andy Shevchenko

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

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

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2017-05-17 12:00 +0200
SubjectRe: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
Message-ID<tI3Ul-1pv-11@gated-at.bofh.it>
In reply to#1642421
On Tue, May 16, 2017 at 2:15 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>>  static u32 lpuart32_read(void __iomem *addr)
>>  {
>> -     return ioread32be(addr);
>> +     return lpuart_is_be ? ioread32be(addr) : readl(addr);
>>  }
>>
>>  static void lpuart32_write(u32 val, void __iomem *addr)
>>  {
>> -     iowrite32be(val, addr);
>> +     if (lpuart_is_be)
>> +             iowrite32be(val, addr);
>> +     else
>> +             writel(val, addr);
>>  }
>
> What if this is ever executed on big endian system?

Above is apparently about bus side of communication and it looks like
it needs to be CPU side. Is this what you are trying to discuss?


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web