2015-06-01 17:28 GMT+08:00 Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>:
> On Mon, Jun 01, 2015 at 10:51:32AM +0800, Jun Nie wrote:
>> Both ARM and ST uart use the same pl011 IP, but some registers
>> have different offset. Support the different offset with look
>> up table.
>>
>> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
>> Reviewed-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/tty/serial/amba-pl011.c | 320 ++++++++++++++++++++++++----------------
>> include/linux/amba/serial.h | 2 +
>> 2 files changed, 194 insertions(+), 128 deletions(-)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 6f5a072..b32f61a 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -29,7 +29,6 @@
>> * and hooked into this driver.
>> */
>>
>> -
>> #if defined(CONFIG_SERIAL_AMBA_PL011_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
>> #define SUPPORT_SYSRQ
>> #endif
>> @@ -67,6 +66,8 @@
>> #define SERIAL_AMBA_NR UART_NR
>>
>> #define AMBA_ISR_PASS_LIMIT 256
>> +#define REG_NR 19
>> +#define IDX(x) ((x) >> 2)
>>
>> #define UART_DR_ERROR (UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE)
>> #define UART_DUMMY_DR_RX (1 << 16)
>> @@ -74,8 +75,7 @@
>> /* There is by now at least one vendor with differing details, so handle it */
>> struct vendor_data {
>> unsigned int ifls;
>> - unsigned int lcrh_tx;
>> - unsigned int lcrh_rx;
>> + u8 *reg_lut;
>> bool oversampling;
>> bool dma_threshold;
>> bool cts_event_workaround;
>> @@ -88,10 +88,32 @@ static unsigned int get_fifosize_arm(struct amba_device *dev)
>> return amba_rev(dev) < 3 ? 16 : 32;
>> }
>>
>> +static u8 arm_reg[] = {
>> + /* All registers offset are in order except [8]=0x2c */
>> + [IDX(UART01x_DR)] = UART01x_DR,
>> + [IDX(UART01x_RSR)] = UART01x_RSR,
>> + [IDX(ST_UART011_DMAWM)] = ST_UART011_DMAWM,
>
> I thought I already commented on things like this. It's sad to see the
> same problems appearing in these patches with new iterations.
>
> The naming scheme for the registers include the ST_ suffix on definitions
> which are _only_ applicable to the ST devices, but not the ARM devices.
>
> Why do we have a ST_* register name on the right hand side in the _ARM_
> lookup table? ST_UART011_DMAWM does not exist on the ARM part.
The wrong name prefix disappear in later patch, but you still can say
it is a issue in patch 1.
>
> I think the whole patch series needs to be done in the reverse order
> (which, again, I think I already commented about.)
>
> Create the enum for the register offsets first:
>
> enum {
> REG_DR = UART01x_DR,
> REG_RSR = UART01x_RSR,
> REG_ST_DMAWM = ST_UART011_DMAWM,
> ...
> };
>
> and subsitute the UART01x_DR (etc) definitions in the driver for the
> REG_* enumerated type. When it comes to the LCRH registers, use this:
>
> REG_ST_LCRH_RX = ST_UART011_LCRH_RX,
> REG_LCRH = UART01x_LCRH,
>
> and do not introduce REG_ST_LCRH_RX and REG_ST_LCRH_TX etc. Keep the
> .lcrh_tx and .lcrh_rx in the vendor data structure, and do not try to
> eliminate these.
I want to confirm with you on:
1. You want to introduce REG_ST_LCRH_RX, but not REG_ST_LCRH_TX,
right? Because still see you mention REG_ST_LCRH_RX below.
2. You do not remove .lcrh_tx and .lcrh_rx even we already have
vendor specific look up table? I see you are setting REG_ST_LCRH_RX as
~0 and testing it with is_implemented(). But removing .lcrh_*x and
introducing REG_LCRH_RX can centralize vendor specific register
information in the table. For ARM UART case that has no LCRH_RX, we
can fill it with [REG_LCRH_RX] = UART01x_LCRH. It is OK if you
still want to keep LCRH RX function register un-centralized. Or I
misunderstand you here?
>
> Then, when you introduce the accessors, you can change the above to be:
>
> enum {
> REG_DR = IDX(UART01x_DR),
> REG_RSR = IDX(UART01x_RSR),
> REG_ST_DMAWM = IDX(ST_UART011_DMAWM),
> ...
> };
>
> Finally, you can then introduce the lookup tables with this:
>
> static u16 arm_reg[] = {
> [REG_DR] = UART01x_DR,
> [REG_RSR] = UART01x_RSR,
> [REG_ST_DMAWM] = ~0, /* not implemented */
> ...
> [REG_ST_LCRH_RX] = ~0,
> [REG_LCRH] = UART01x_LCRH,
> };
>
> Remembering to check for that "undefined" value. This table needs to be
> at least u16, and not u8 as the register offsets do extend above 255 bytes:
>
> #define ST_UART011_ABCR 0x100 /* Autobaud control register. */
> #define ST_UART011_ABIMSC 0x15C /* Autobaud interrupt mask/clear register */
With "#define IDX(x) (x >> 2)", u8 is enough to contain all
registers. Any different idea on definition?
>
> An "improvement" patch on top of that would then be changing the driver
> decision on the LCRH_TX/LCRH_RX registers, potentially using:
>
> uap->reg_lut[REG_ST_LCRH_RX] != (u16)~0
>
> or better:
>
> is_implemented(uap, REG_ST_LCRH_RX)
>
> where is_implemented() is:
>
> static bool is_implemented(struct uart_amba_port *uap, unsigned int reg)
> {
> return uap->reg_lut[reg] != (u16)~0;
> }
>
> If done properly, the first three patches should _just_ be obvious
> transformations of the existing code: step 1, renaming the definitions.
> step 2, adding the accessors. step 3, adding the lookup tables.
>
> Then comes the functional change in the way the driver makes decisions
> as the final step.
>
> This way, if some problem is later identified with the functional changes,
> the remainder can stay behind.
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
