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.
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.
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 */
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
