On 01/29/2016 12:21 AM, Oleksij Rempel wrote:
> Hi Peter,
>
> thank you for comments.
>
> Am 28.01.2016 um 17:40 schrieb Peter Hurley:
>> Hi Oleksij,
>>
>> On 12/14/2015 11:24 AM, Oleksij Rempel wrote:
>>> Alphascale ASM9260 uart IP has some common registers with
>>> Freescale STMP37XX. This patch provide changes which
>>> allow to reuse mxs-auart.c code for ASM9260.
>>>
>>> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>>> ---
>>> .../devicetree/bindings/serial/fsl-mxs-auart.txt | 10 +-
>>> drivers/tty/serial/Kconfig | 5 +-
>>> drivers/tty/serial/mxs-auart.c | 667 +++++++++++++++++----
>>> 3 files changed, 577 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> index 7c408c8..5bb2e32 100644
>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> @@ -1,8 +1,8 @@
>>> * Freescale MXS Application UART (AUART)
>>>
>>> -Required properties:
>>> +Required properties for all SoCs:
>>> - compatible : Should be "fsl,<soc>-auart". The supported SoCs include
>>> - imx23 and imx28.
>>> + imx23 and imx28. Or "alphascale,asm9260-auart".
>>
>> Can you rewrite this to make it clear which compatible string is
>> appropriate for which SoC?
>
> I would assume it is self descriptive, since "compatible" usually
> contains: company name; SoC name and component. Any way, if it is
> required i will change it.
I'm thinking of the next 5 additions that go:
- imx23 and imx28. Or "alphascale,asm9260-auart".
+ imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart".
- imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart".
+ imx23 and imx28. Or "alphascale,asm9260-auart". Or "mySoC, that-uart". Or
+ "theirSoC, their-uart".
etc, etc. Better just to format it cleanly now.
>>> - reg : Address and length of the register set for the device
>>> - interrupts : Should contain the auart interrupt numbers
>>> - dmas: DMA specifier, consisting of a phandle to DMA controller node
>>> #define AUART_STAT 0x00000070
>>> #define AUART_DEBUG 0x00000080
>>> @@ -136,11 +123,387 @@
>>> #define AUART_STAT_FERR (1 << 16)
>>> #define AUART_STAT_RXCOUNT_MASK 0xffff
>>>
>>> +/* Start of Alphascale asm9260 defines */
>>
>> Why are you (re)defining all these bits below when they're identical
>> to the MXS register layout?
>
> To provide some kind of documentation. As you can see, there are more
> registers as by iMX controller. Not one else have access to docs. In any
> case, if it is not welcome i'll remove it.
For any bits that are not like iMX either in behavior or location,
that'd be great. Otherwise, it's just noise.
>>> +/* RX ctrl register */
>>> +#define ASM9260_HW_CTRL0 0x0000
>>> +/* RW. Set to zero for normal operation. */
> ....
>>> static inline bool auart_dma_enabled(struct mxs_auart_port *s)
>>> {
>>> return s->flags & MXS_AUART_DMA_ENABLED;
>>> @@ -295,19 +670,19 @@ static void mxs_auart_tx_chars(struct mxs_auart_port *s)
>>> }
>>>
>>>
>>> - while (!(readl(s->port.membase + AUART_STAT) &
>>> + while (!(readl(s->regs.stat) &
>>> AUART_STAT_TXFF)) {
>>> if (s->port.x_char) {
>>> s->port.icount.tx++;
>>> writel(s->port.x_char,
>>> - s->port.membase + AUART_DATA);
>>> + s->regs.data);
>>
>> The convention for adding clones that trivially move register addresses
>> is to add i/o accessors that perform the register lookup. For example,
>>
>> writel(s->port.xchar, s->port.membase + AUART_DATA);
>>
>> becomes
>>
>> mxs_write(s->port.xchar, s, AUART_DATA);
>
> I assume we have here some confusion.
> It was:
>
> writel(s->port.x_char, s->port.membase + AUART_DATA);
>
> and i changed it to:
>
> writel(s->port.x_char, s->regs.data);
>
> what kind of advantage will bring new IO function?
Ease of maintenance.
Plus the inevitable follow-on clone that specializes other registers.
Just went through this with the amba-pl011 driver.
Regards,
Peter Hurley
--
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
