Re: [PATCH] RM9000 serial driver

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Thomas Koeller wrote:

>>>>I would like to return to the port type vs. iotype  stuff once again.

>>>From what you wrote I seem to understand that the iotype is not just

>>>>a method of accessing device registers, but also the primary means of
>>>>discrimination between different h/w implementations, and hence every
>>>>code to support a nonstandard device must define an iotype of its own,
>>>>even though one of the existing iotypes would work just fine?

>>>iotype is all about the access method used to access the registers of
>>>the device, be it by byte or word, and it also takes account of any
>>>variance in the addressing of the registers.

>>>It does not refer to features or bugs in any particular implementation.

>>    Well, the introduction of the UPIO_TSI case seems to contradict this --
>>it's exactly about the bugs in the particular UART implementation
>>(otherwise well described by UPIO_MEM). Its only function was to mask 2
>>hardware issues. And the UUE bit workaround seems like an abuse to me. The
>>driver could just skip the UUE test altogether based on iotype == UPIO_TSI
>>(or at least not to ignore writes with UUE set completely like it does but
>>just mask off UUE bit). With no provision to pass the implicit UART type
>>for platform devices (and skip the autoconfiguation), the introduction of
>>UPIO_TSI seems again the necessary evil. Otherwise, we could have this
>>handled with a distinct TSI UART type...

>>WBR, Sergei

> after a long delay (for which I apologize) I would like to return to

    I should also apologize for my discussion style back then -- I just was 
short of time (and now I am as well), and might have gotten too nervous for 
that reason...

> this topic. Since so much time has passed since the discussion ceased, I
> think I should summarize its state at that point:

> 1. In the RM9000 serial driver patch I submitted, I had introduced a new
>    port type PORT_RM9000. I set the iotype to UPIO_MEM32. Wherever I
>    needed to handle any peculiarities of the RM9000 h/w, I did so based
>    upon the port type. AFAICT this is in agreement with Russel's view as
>    quoted above.

    It was good in theory but the practice has shown that this approach had a 
breache.

> 2. Sergei says this is wrong, I need to introduce a new iotype instead,
>    and make the code conditional upon that, like the AU1000 does (UPIO_AU).
>    The reason he gives (see quote above) is that there is no way to pass
>    the port type along with a platform device. Is this argument still valid?

    Of course.

>    What about including the port type in the information attached to
>    platform_device.platform_data?

    As they say, "patches welcome". :-)

> Btw., I had a look ath the existing code dealing with platform
> devices. Is there a particular reason for not using the standard
> platform_device.resource mechanism of passing mem/io/irq resources from
> the platform to the driver?

    Good question...
    It was probably easier to get all info from one place than to parse the 
resources. :-)

> Another topic discussed was the use (or abuse) of dl_serial_read()/
> dl_serial_write() to get/set the divisor registers.

    More like necessary evil since Pantelis decided to merge support for the 
Alchemy UARTs... :-)

> 3. Russel says (qoute from an earlier mail),
>    > It's worse than that - this code is there to read the ID from the divisor
>    > registers implemented in some UARTs. If it isn't one of those UARTs,
>    > it's expected to return zero.

    Could you remind regarding which exactly code Russell has said this,
the one in autoconfig_read_divisor_id()?  Are you sure this code gets actually 
called for your UART or Alchemy one?  From looking at the code, I could 
conclude that it shouldn't be -- IIR (sharing address with EFR) shouldn't be 
reading as zero unless the modem status interrupt is pending -- that's what 
the EFR detection code counts on...
     But anyway, I don't see why this code is unsafe to convert to using the 
dl_serail_read/write accessors...

> 4. I followed the AU1X00 code in this case, using the functions to read/write
>    the divisor value. My hardware has its UART_DLL and UART_DLM registers at
>    nonstandard addresses, and so the only other option would have been to
>    monitor every write to the LCR register for setting or clearing the DLAB
>    bit, and to switch between different mapping tables accordingly. In the

    Why, if we already have a working approach?

>    light of this, would it still be unacceptable to use the dl_*() functions
>    tho access the divisor registers?

    Why would it be unacceptable, if the like code is already there, in the 
driver?

> Once these issues have been resolved, I will submit an updated version of
> my patch.

    I'm afraid you're on your own with resolving the issues now, as the serial 
drivers are no longer maintaned by anybody (so, you probably will have to work 
with Andrew Morton)...

> tk

MBR, Sergei