Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs
On 05/03/2021 18:04, Hector Martin wrote:
> On 06/03/2021 00.28, Andy Shevchenko wrote:
>>> + case TYPE_APPLE_S5L:
>>> + WARN_ON(1); // No DMA
>>
>> Oh, no, please use the ONCE variant.
>
> Thanks, changing this for v4.
>
>>
>> ...
>>
>>> + /* Apple types use these bits for IRQ masks */
>>> + if (ourport->info->type != TYPE_APPLE_S5L) {
>>> + ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
>>> + S3C64XX_UCON_EMPTYINT_EN |
>>> + S3C64XX_UCON_DMASUS_EN |
>>> + S3C64XX_UCON_TIMEOUT_EN);
>>> + ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
>>
>> Can you spell 0xf with named constant(s), please?
>>
>> In case they are repetitive via the code, introduce either a temporary
>> variable (in case it scoped to one function only), or define it as a
>> constant.
>
> I'm just moving this code; as far as I can tell this is a timeout value
> (so just an integer), but I don't know if there is any special meaning
> to 0xf here. Note that this codepath is for *non-Apple* chips, as the
> Apple ones don't even have this field (at least not here).
I agree here with Hector. Andi, you propose here unrelated change (which
without documentation might not be doable by Hector).
>
>>> + irqreturn_t ret = IRQ_NONE;
>>
>> Redundant. You may return directly.
>
> What if both interrupts are pending?
>
>> No IO serialization?
>
> There is no DMA on the Apple variants (as far as I know; it's not
> implemented anyway), so there is no need for serializing IO with DMA. In
> any case, dealing with that is the DMA code's job, the interrupt handler
> shouldn't need to care.
>
> If you mean serializing IO with the IRQ: CPU-wise, I would hope that's
> the irqchip's job (AIC does this with a readl on the event). If you mean
> ensuring all writes are complete (i.e. posted write issue), on the Apple
> chips everything is non-posted as explained in the previous patches.
>
>> Extra blank line (check your entire series for a such)
>
> Thanks, noted. I'll check the declaration blocks in other patches.
>
>>> + ourport->rx_enabled = 1;
>>> + ourport->tx_enabled = 0;
>>
>> How are these protected against race?
>
> The serial core should be holding the port mutex for pretty much every
> call into the driver, as far as I can tell.
>
>>
>> ...
>>
>>> + case TYPE_APPLE_S5L: {
>>> + unsigned int ucon;
>>> + int ret;
>>> +
>>> + ret = clk_prepare_enable(ourport->clk);
>>> + if (ret) {
>>> + dev_err(dev, "clk_enable clk failed: %d\n",
>>> ret);
>>> + return ret;
>>> + }
>>> + if (!IS_ERR(ourport->baudclk)) {
>>> + ret = clk_prepare_enable(ourport->baudclk);
>>> + if (ret) {
>>> + dev_err(dev, "clk_enable baudclk
>>> failed: %d\n", ret);
>>> + clk_disable_unprepare(ourport->clk);
>>> + return ret;
>>> + }
>>> + }
>>
>> Wouldn't it be better to use CLK bulk API?
>
> Ah, I guess that could save a line or two of code here, even though it
> requires setting up the array. I'll give it a shot.
>
>>> +#ifdef CONFIG_ARCH_APPLE
>>
>> Why? Wouldn't you like the one kernel to work on many SoCs?
>
> This *adds* Apple support, it is not mutually exclusive with all the
> other SoCs. You can enable all of those options and get a driver that
> works on all of them. This is the same pattern used throughout the
> driver for all the other Samsung variants. There is no reason to have
> Apple SoC support in the samsung driver if the rest of the kernel
> doesn't have Apple SoC support either, of course.
How ifdef on ARCH_APLLE makes it non-working on many SoCs? All new
platforms are multi... The true question is - do the ifdefs in the code
make it more difficult to read/review?
Best regards,
Krzysztof
Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs
On 06/03/2021 00.28, Andy Shevchenko wrote:
+ case TYPE_APPLE_S5L:
+ WARN_ON(1); // No DMA
Oh, no, please use the ONCE variant.
Thanks, changing this for v4.
...
+ /* Apple types use these bits for IRQ masks */
+ if (ourport->info->type != TYPE_APPLE_S5L) {
+ ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
+ S3C64XX_UCON_EMPTYINT_EN |
+ S3C64XX_UCON_DMASUS_EN |
+ S3C64XX_UCON_TIMEOUT_EN);
+ ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
Can you spell 0xf with named constant(s), please?
In case they are repetitive via the code, introduce either a temporary
variable (in case it scoped to one function only), or define it as a
constant.
I'm just moving this code; as far as I can tell this is a timeout value
(so just an integer), but I don't know if there is any special meaning
to 0xf here. Note that this codepath is for *non-Apple* chips, as the
Apple ones don't even have this field (at least not here).
+ irqreturn_t ret = IRQ_NONE;
Redundant. You may return directly.
What if both interrupts are pending?
No IO serialization?
There is no DMA on the Apple variants (as far as I know; it's not
implemented anyway), so there is no need for serializing IO with DMA. In
any case, dealing with that is the DMA code's job, the interrupt handler
shouldn't need to care.
If you mean serializing IO with the IRQ: CPU-wise, I would hope that's
the irqchip's job (AIC does this with a readl on the event). If you mean
ensuring all writes are complete (i.e. posted write issue), on the Apple
chips everything is non-posted as explained in the previous patches.
Extra blank line (check your entire series for a such)
Thanks, noted. I'll check the declaration blocks in other patches.
+ ourport->rx_enabled = 1;
+ ourport->tx_enabled = 0;
How are these protected against race?
The serial core should be holding the port mutex for pretty much every
call into the driver, as far as I can tell.
...
+ case TYPE_APPLE_S5L: {
+ unsigned int ucon;
+ int ret;
+
+ ret = clk_prepare_enable(ourport->clk);
+ if (ret) {
+ dev_err(dev, "clk_enable clk failed: %d\n",
ret);
+ return ret;
+ }
+ if (!IS_ERR(ourport->baudclk)) {
+ ret = clk_prepare_enable(ourport->baudclk);
+ if (ret) {
+ dev_err(dev, "clk_enable baudclk failed:
%d\n", ret);
+ clk_disable_unprepare(ourport->clk);
+ return ret;
+ }
+ }
Wouldn't it be better to use CLK bulk API?
Ah, I guess that could save a line or two of code here, even though it
requires setting up the array. I'll give it a shot.
+#ifdef CONFIG_ARCH_APPLE
Why? Wouldn't you like the one kernel to work on many SoCs?
This *adds* Apple support, it is not mutually exclusive with all the
other SoCs. You can enable all of those options and get a driver that
works on all of them. This is the same pattern used throughout the
driver for all the other Samsung variants. There is no reason to have
Apple SoC support in the samsung driver if the rest of the kernel
doesn't have Apple SoC support either, of course.
+#define APPLE_S5L_UCON_RXTO_ENA_MSK(1 << APPLE_S5L_UCON_RXTO_ENA)
+#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK(1 <<
APPLE_S5L_UCON_RXTHRESH_ENA)
+#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK(1 <<
APPLE_S5L_UCON_TXTHRESH_ENA)
BIT() ?
I'm trying to keep the style of the rest of the file here, which doesn't
use BIT() anywhere. I agree this header could use some work though... I
wonder if I've met my required quota of cleanups to this driver for this
patchset ;-)
+#define APPLE_S5L_UCON_DEFAULT (S3C2410_UCON_TXIRQMODE | \
+S3C2410_UCON_RXIRQMODE | \
+S3C2410_UCON_RXFIFO_TOI)
Indentation level is too high. Hint: start a value of the definition
on the new line.
Is it that bad? It's within 80 cols, putting one bit per line is more
readable than several on one line, and this is how the rest of the
header is written. Is it really better to do
#define APPLE_S5L_UCON_DEFAULT \
(S3C2410_UCON_TXIRQMODE | S3C2410_UCON_RXIRQMODE | \
S3C2410_UCON_RXFIFO_TOI)
or
#define APPLE_S5L_UCON_DEFAULT \
(S3C2410_UCON_TXIRQMODE | \
S3C2410_UCON_RXIRQMODE | \
S3C2410_UCON_RXFIFO_TOI)
here? Those don't look like an obvious improvement to me, I'd even say
overlapping the bits and the macro name in the same columns makes
Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs
On Thu, Mar 4, 2021 at 11:42 PM Hector Martin wrote:
>
> Apple SoCs are a distant descendant of Samsung designs and use yet
> another variant of their UART style, with different interrupt handling.
>
> In particular, this variant has the following differences with existing
> ones:
>
> * It includes a built-in interrupt controller with different registers,
> using only a single platform IRQ
>
> * Internal interrupt sources are treated as edge-triggered, even though
> the IRQ output is level-triggered. This chiefly affects the TX IRQ
> path: the driver can no longer rely on the TX buffer empty IRQ
> immediately firing after TX is enabled, but instead must prime the
> FIFO with data directly.
...
> + case TYPE_APPLE_S5L:
> + WARN_ON(1); // No DMA
Oh, no, please use the ONCE variant.
...
> + /* Apple types use these bits for IRQ masks */
> + if (ourport->info->type != TYPE_APPLE_S5L) {
> + ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
> + S3C64XX_UCON_EMPTYINT_EN |
> + S3C64XX_UCON_DMASUS_EN |
> + S3C64XX_UCON_TIMEOUT_EN);
> + ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
Can you spell 0xf with named constant(s), please?
In case they are repetitive via the code, introduce either a temporary
variable (in case it scoped to one function only), or define it as a
constant.
> + S3C64XX_UCON_TIMEOUT_EN;
> + }
...
> +/* interrupt handler for Apple SoC's.*/
> +static irqreturn_t apple_serial_handle_irq(int irq, void *id)
> +{
> + struct s3c24xx_uart_port *ourport = id;
> + struct uart_port *port = >port;
> + unsigned int pend = rd_regl(port, S3C2410_UTRSTAT);
> + irqreturn_t ret = IRQ_NONE;
Redundant. You may return directly.
> +
> + if (pend & (APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO)) {
> + wr_regl(port, S3C2410_UTRSTAT,
> + APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO);
> + ret = s3c24xx_serial_rx_irq(irq, id);
> + }
> + if (pend & APPLE_S5L_UTRSTAT_TXTHRESH) {
> + wr_regl(port, S3C2410_UTRSTAT, APPLE_S5L_UTRSTAT_TXTHRESH);
> + ret = s3c24xx_serial_tx_irq(irq, id);
> + }
No IO serialization?
> + return ret;
> +}
...
> +static void apple_s5l_serial_shutdown(struct uart_port *port)
> +{
> + struct s3c24xx_uart_port *ourport = to_ourport(port);
> +
Extra blank line (check your entire series for a such)
> + unsigned int ucon;
> + ourport->tx_in_progress = 0;
> +}
...
> + ourport->rx_enabled = 1;
> + ourport->tx_enabled = 0;
How are these protected against race?
...
> + case TYPE_APPLE_S5L: {
> + unsigned int ucon;
> + int ret;
> +
> + ret = clk_prepare_enable(ourport->clk);
> + if (ret) {
> + dev_err(dev, "clk_enable clk failed: %d\n",
> ret);
> + return ret;
> + }
> + if (!IS_ERR(ourport->baudclk)) {
> + ret = clk_prepare_enable(ourport->baudclk);
> + if (ret) {
> + dev_err(dev, "clk_enable baudclk
> failed: %d\n", ret);
> + clk_disable_unprepare(ourport->clk);
> + return ret;
> + }
> + }
Wouldn't it be better to use CLK bulk API?
> + ucon = rd_regl(port, S3C2410_UCON);
> +
> + ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTO_ENA_MSK);
> +
> + if (ourport->tx_enabled)
> + ucon |= APPLE_S5L_UCON_TXTHRESH_ENA_MSK;
> + if (ourport->rx_enabled)
> + ucon |= APPLE_S5L_UCON_RXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTO_ENA_MSK;
> +
> + wr_regl(port, S3C2410_UCON, ucon);
> +
> + if (!IS_ERR(ourport->baudclk))
> + clk_disable_unprepare(ourport->baudclk);
> + clk_disable_unprepare(ourport->clk);
> + break;
> + }
...
> +#ifdef CONFIG_ARCH_APPLE
Why? Wouldn't you like the one kernel to work on many SoCs?
> +static struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
> + .info = &(struct s3c24xx_uart_info) {
> + .name = "Apple S5L UART",
> + .type = TYPE_APPLE_S5L,
> + .port_type = PORT_8250,
> +
Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs
On 04/03/2021 22:38, Hector Martin wrote: > Apple SoCs are a distant descendant of Samsung designs and use yet > another variant of their UART style, with different interrupt handling. > > In particular, this variant has the following differences with existing > ones: > > * It includes a built-in interrupt controller with different registers, > using only a single platform IRQ > > * Internal interrupt sources are treated as edge-triggered, even though > the IRQ output is level-triggered. This chiefly affects the TX IRQ > path: the driver can no longer rely on the TX buffer empty IRQ > immediately firing after TX is enabled, but instead must prime the > FIFO with data directly. > > Signed-off-by: Hector Martin > --- > drivers/tty/serial/Kconfig | 2 +- > drivers/tty/serial/samsung_tty.c | 238 +-- > include/linux/serial_s3c.h | 16 +++ > 3 files changed, 247 insertions(+), 9 deletions(-) > Reviewed-by: Krzysztof Kozlowski Tested-by: Krzysztof Kozlowski Best regards, Krzysztof
