Re: [PATCH v3 3/3] serial: 8250_em: Add serial_out() to struct serial8250_em_hw_info

[Jiri Slaby <jirislaby@kernel.org>]

On 13. 02. 23, 10:22, Ilpo Järvinen wrote:
> On Mon, 13 Feb 2023, Jiri Slaby wrote:
> 
>> On 13. 02. 23, 10:10, Jiri Slaby wrote:
>>> On 13. 02. 23, 9:53, Biju Das wrote:
>>>>>> +static void serial8250_rzv2m_reg_update(struct uart_port *p, int off,
>>>>>> +int value) {
>>>>>> +    unsigned int ier, fcr, lcr, mcr, hcr0;
>>>>>> +
>>>>>> +    ier = serial8250_em_serial_in(p, UART_IER);
>>>>>> +    lcr = serial8250_em_serial_in(p, UART_LCR);
>>>>>> +    mcr = serial8250_em_serial_in(p, UART_MCR);
>>>>>> +    hcr0 = serial8250_em_serial_in(p, UART_HCR0);
>>>>>> +    /*
>>>>>> +     * The value of UART_IIR and UART_FCR are 2, but corresponding
>>>>>> +     * RZ/V2M address offset are different(0x08 and 0x0c). So we need
>>>>>> to
>>>>>> +     * use readl() here.
>>>>>> +     */
>>>>>> +    fcr = readl(p->membase + ((UART_FCR + 1) << 2));
>>>>>
>>>>> I don't get the meaning of that comment. It doesn't seem to match what
>>>>> your
>>>>> code does as the code seemingly has nothing to do with IIR (and none of
>>>>> you
>>>>> changelogs refer to IIR either)?
>>>>
>>>> The generic macro UART_IIR and UART_FCR in linux/serial_reg.h has a value
>>>> of 2.
>>>
>>> Sure, IIR is normally WO and FCR RO and share the same register. I would
>>> simply define UART_FCR_RZ (or alike)
>>
>> Or even UART_FCR_RO_RZ?
>>
>>> for 0x12.
>>
>> I mean 12 or 0xc.
> 
> Won't that collide with LCR reads then? They are currently mapped by
> return 0; but after adding a case for UART_FCR_RO_RZ they'll read from
> what is the FCR register on this HW?

LCR is WO, right? But maybe I'm confused by this really weird HW design?

-- 
js
suse labs