Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Stephen Boyd]

Quoting Karthik Ramasubramanian (2018-03-20 15:53:25)
> 
> 
> On 3/20/2018 9:37 AM, Stephen Boyd wrote:
> > Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)
> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
> >> b/drivers/tty/serial/qcom_geni_serial.c
> >> new file mode 100644
> >> index 000..1442777
> >> --- /dev/null
> >> +++ b/drivers/tty/serial/qcom_geni_serial.c
> >> @@ -0,0 +1,1158 @@
> >> +
> >> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> >> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> >> +{
> >> +   writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
> > 
> > Does this expect the whole word to have data to write? Or does the FIFO
> > output a character followed by three NUL bytes each time it gets
> > written? The way that uart_console_write() works is to take each
> > character a byte at a time, put it into an int (so extend that byte with
> > zero) and then pass it to the putchar function. I would expect that at
> > this point the hardware sees the single character and then 3 NULs enter
> > the FIFO each time.
> > 
> > For previous MSM uarts I had to handle this oddity by packing the words
> > into the fifo four at a time. You may need to do the same here.
> The packing configuration 1 * 8 (done using geni_se_config_packing)
> ensures that only one byte per FIFO word needs to be transmitted. From
> that perspective, we need not have such oddity.

Ok! That's good to hear.

> > 
> > Can you also support the OF_EARLYCON_DECLARE method of console writing
> > so we can get an early printk style debug console?
> Do you prefer that as part of this patch itself or is it ok if I upload
> the earlycon support once this gets merged.

I think this already got merged? So just split it out into another patch
would be fine. I see the config is already selecting the earlycon
support so it must be planned.

> > 
> > 
> >> +
> >> +   spin_lock_irqsave(>lock, flags);
> >> +   m_irq_status = readl_relaxed(uport->membase + 
> >> SE_GENI_M_IRQ_STATUS);
> >> +   s_irq_status = readl_relaxed(uport->membase + 
> >> SE_GENI_S_IRQ_STATUS);
> >> +   m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> >> +   writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> >> +   writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> >> +
> >> +   if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
> >> +   goto out_unlock;
> >> +
> >> +   if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
> >> +   uport->icount.overrun++;
> >> +   tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> >> +   }
> >> +
> >> +   if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
> >> +   m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> >> +   qcom_geni_serial_handle_tx(uport);
> >> +
> >> +   if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) {
> >> +   if (s_irq_status & S_GP_IRQ_0_EN)
> >> +   uport->icount.parity++;
> >> +   drop_rx = true;
> >> +   } else if (s_irq_status & S_GP_IRQ_2_EN ||
> >> +   s_irq_status & S_GP_IRQ_3_EN) {
> >> +   uport->icount.brk++;
> > 
> > Maybe move this stat accounting to the place where brk is handled?
> Since other error accounting like overrun, parity are happening here, it
> feels logical to keep that accounting here.

Alright.

> >> +   return uart_add_one_port(_geni_console_driver, uport);
> >> +}
> >> +
> >> +static int qcom_geni_serial_remove(struct platform_device *pdev)
> >> +{
> >> +   struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> >> +   struct uart_driver *drv = port->uport.private_data;
> >> +
> >> +   uart_remove_one_port(drv, >uport);
> >> +   return 0;
> >> +}
> >> +
> >> +static int __maybe_unused qcom_geni_serial_sys_suspend_noirq(struct 
> >> device *dev)
> >> +{
> >> +   struct platform_device *pdev = to_platform_device(dev);
> >> +   struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> >> +   struct uart_port *uport = >uport;
> >> +
> >> +   uart_suspend_port(uport-&

Re: [PATCH v4 5/6] arm64: dts: sdm845: Add serial console support

[Stephen Boyd]

Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:50)
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts 
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 979ab49..ea3efc5 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -12,4 +12,43 @@
>  / {
> model = "Qualcomm Technologies, Inc. SDM845 MTP";
> compatible = "qcom,sdm845-mtp";
> +
> +   aliases {
> +   serial0 = 
> +   };
> +
> +   chosen {
> +   stdout-path = "serial0";

Also add :115200n8 ?


> +   };
> +};
> +
> + {

I think the method is to put these inside soc node without using the
phandle reference. So indent everything once more.

> +   geniqup@ac {
> +   serial@a84000 {
> +   status = "okay";
> +   };
> +   };
> +
> +   pinctrl@340 {
> +   qup-uart2-default {
> +   pinconf_tx {
> +   pins = "gpio4";
> +   drive-strength = <2>;
> +   bias-disable;
> +   };
> +
> +   pinconf_rx {
> +   pins = "gpio5";
> +   drive-strength = <2>;
> +   bias-pull-up;
> +   };
> +   };
> +
> +   qup-uart2-sleep {
> +   pinconf {
> +   pins = "gpio4", "gpio5";
> +   bias-pull-down;
> +   };
> +   };
> +   };
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 32f8561..59334d9 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -6,6 +6,7 @@
>   */
>  
>  #include 
> +#include 
>  
>  / {
> interrupt-parent = <>;
> @@ -194,6 +195,20 @@
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> +
> +   qup_uart2_default: qup-uart2-default {
> +   pinmux {
> +   function = "qup9";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };
> +
> +   qup_uart2_sleep: qup-uart2-sleep {
> +   pinmux {
> +   function = "gpio";
> +   pins = "gpio4", "gpio5";
> +   };
> +   };

Are these supposed to go to the board file?

> };
>  
> timer@17c9 {
> @@ -272,5 +287,28 @@
> #interrupt-cells = <4>;
> cell-index = <0>;
> };
> +
> +   geniqup@ac {
> +   compatible = "qcom,geni-se-qup";
> +   reg = <0xac 0x6000>;
> +   clock-names = "m-ahb", "s-ahb";
> +   clocks = < GCC_QUPV3_WRAP_1_M_AHB_CLK>,
> +< GCC_QUPV3_WRAP_1_S_AHB_CLK>;
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   ranges;

Add a status = "disabled" here too.

> +
> +   uart2: serial@a84000 {
> +   compatible = "qcom,geni-debug-uart";
> +   reg = <0xa84000 0x4000>;
> +   clock-names = "se";
> +   clocks = < GCC_QUPV3_WRAP1_S1_CLK>;
> +   pinctrl-names = "default", "sleep";
> +   pinctrl-0 = <_uart2_default>;
> +   pinctrl-1 = <_uart2_sleep>;
> +   interrupts =  IRQ_TYPE_LEVEL_HIGH>;
> +   status = "disabled";
> +   };
> +   };
> };
>  };
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/6] dt-bindings: soc: qcom: Add device tree binding for GENI SE

[Stephen Boyd]

Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:46)
> Add device tree binding support for the QCOM GENI SE driver.
> 
> Signed-off-by: Karthikeyan Ramasubramanian <krama...@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdha...@codeaurora.org>
> Signed-off-by: Girish Mahadevan <giri...@codeaurora.org>

Assuming Rob's comment is addressed:

Reviewed-by: Stephen Boyd <swb...@chromium.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Stephen Boyd]

Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)
> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
> b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 000..1442777
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1158 @@
> +
> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> +{
> +   writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);

Does this expect the whole word to have data to write? Or does the FIFO
output a character followed by three NUL bytes each time it gets
written? The way that uart_console_write() works is to take each
character a byte at a time, put it into an int (so extend that byte with
zero) and then pass it to the putchar function. I would expect that at
this point the hardware sees the single character and then 3 NULs enter
the FIFO each time.

For previous MSM uarts I had to handle this oddity by packing the words
into the fifo four at a time. You may need to do the same here.

> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> +unsigned int count)
> +{
> +   int i;
> +   u32 bytes_to_send = count;
> +
> +   for (i = 0; i < count; i++) {
> +   if (s[i] == '\n')

Can you add a comment that uart_console_write() adds a carriage return
for each newline?

> +   bytes_to_send++;
> +   }
> +
> +   writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +   qcom_geni_serial_setup_tx(uport, bytes_to_send);
> +   for (i = 0; i < count; ) {
> +   size_t chars_to_write = 0;
> +   size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM;
> +
> +   /*
> +* If the WM bit never set, then the Tx state machine is not
> +* in a valid state, so break, cancel/abort any existing
> +* command. Unfortunately the current data being written is
> +* lost.
> +*/
> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_TX_FIFO_WATERMARK_EN, true))
> +   break;
> +   chars_to_write = min_t(size_t, (size_t)(count - i), avail / 
> 2);

The _t part of min_t should do the casting already, so we can drop the
cast on count - i?

> +   uart_console_write(uport, s + i, chars_to_write,
> +   qcom_geni_serial_wr_char);
> +   writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> +   SE_GENI_M_IRQ_CLEAR);
> +   i += chars_to_write;
> +   }
> +   qcom_geni_serial_poll_tx_done(uport);
> +}
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> +   struct uart_port *uport;
> +   struct qcom_geni_serial_port *port;
> +   bool locked = true;
> +   unsigned long flags;
> +
> +   WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> +
> +   port = get_port_from_line(co->index);
> +   if (IS_ERR(port))
> +   return;
> +
> +   uport = >uport;
> +   if (oops_in_progress)
> +   locked = spin_trylock_irqsave(>lock, flags);
> +   else
> +   spin_lock_irqsave(>lock, flags);
> +
> +   /* Cancel the current write to log the fault */
> +   if (!locked) {
> +   geni_se_cancel_m_cmd(>se);
> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_CANCEL_EN, true)) {
> +   geni_se_abort_m_cmd(>se);
> +   qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_ABORT_EN, true);
> +   writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> +   SE_GENI_M_IRQ_CLEAR);
> +   }
> +   writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> +   SE_GENI_M_IRQ_CLEAR);
> +   }
> +
> +   __qcom_geni_serial_console_write(uport, s, count);
> +   if (locked)
> +   spin_unlock_irqrestore(>lock, flags);
> +}

Can you also support the OF_EARLYCON_DECLARE method of console writing
so we can get an early printk style debug console?

> +
> +static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
> +{
> +   u32 i;
> +   unsigned char buf[sizeof(u32)];
> +   struct tty_port *tport;
> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> +   tport = >state->port;
> +   for (i = 0; i < bytes; ) {
> +   int c;
> +   int 

Re: [PATCH v4 6/6] arm64: dts: sdm845: Add I2C controller support

[Stephen Boyd]

Quoting Doug Anderson (2018-03-19 16:56:27)
> On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia  wrote:
> >
> > Yes, we typically have a "platform(sdm845 here)-qupv3.dtsi" defining
> > most of the serial-bus instances (i2c, spi, and uart with
> > status=disabled) that we include from the common header. The boards
> > enable instances they need.
> > Will that be okay?
> 
> Unless you really feel the need to put these in a separate file I'd
> just put them straight in sdm845.dtsi.  Yeah, it'll get big, but
> that's OK by me.  I _think_ this matches what Bjorn was suggesting on
> previous device tree patches, but CCing him just in case.  I'm
> personally OK with whatever Bjorn and other folks with more Qualcomm
> history would like.

Upstream puts all SoC nodes in the SoC file. The split file by
functional area method is to avoid merge conflicts in the SoC file and
that doesn't make sense outside of the internal workflow.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks

[Stephen Boyd]

Quoting Paul Cercueil (2018-03-17 16:28:55)
> This header provides clock numbers for the ingenic,tcu
> DT binding.
> 
> Signed-off-by: Paul Cercueil <p...@crapouillou.net>
> Reviewed-by: Rob Herring <r...@kernel.org>
> ---

Acked-by: Stephen Boyd <sb...@kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Documentation: clk: enable lock is not held for clk_is_enabled API

[Stephen Boyd]

Quoting Dong Aisheng (2018-01-19 05:37:15)
> The core does not need to hold enable lock for clk_is_enabled API.
> Update the doc to reflect it.
> 
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Stephen Boyd <sb...@codeaurora.org>
> Suggested-by: Stephen Boyd <sb...@codeaurora.org>
> Signed-off-by: Dong Aisheng <aisheng.d...@nxp.com>
> ---

Applied to clk-next

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Stephen Boyd]

Quoting Karthik Ramasubramanian (2018-03-07 22:06:29)
> 
> 
> On 3/6/2018 2:45 PM, Stephen Boyd wrote:
> > Quoting Karthik Ramasubramanian (2018-03-05 16:51:23)
> >> On 3/2/2018 3:11 PM, Stephen Boyd wrote:
> > 
> > Ok. I've seen similar designs in some mmc drivers. For example, you can
> > look at drivers/mmc/host/meson-gx-mmc.c and see that they register a
> > clk_ops and then just start using that clk directly from the driver.
> > There are also some helper functions for dividers that would hopefully
> > make the job of calculating the best divider easier.
> Thanks for the pointers. I will take a look at it. In the meanwhile I 
> had discussions with our clock team. They pointed out that the register 
> to write the divider value here is outside the scope of clock controller 
> which makes it trickier to implement your suggestion. They are already 
> in the mailing list and we will discuss further and get back to you in 
> this regard.

Ok. Let me know if I can help answer any questions.

> >>>
> >>> Why are these noirq variants? Please add a comment.
> >> The intention is to enable the console UART port usage as late as
> >> possible in the suspend cycle. Hence noirq variants. I will add this as
> >> a comment. Please let me know if the usage does not match the intention.
> > 
> > Hm.. Does no_console_suspend not work? Or not work well enough?
> It works. When console suspend is disabled, the suspend operation does 
> not get triggered and the resume operation checks if the console suspend 
> is disabled and performs the needed thing.

Ok so then do we need the noirq variants? Or console suspend is special
enough for this to not matter?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver

[Stephen Boyd]

Quoting Karthik Ramasubramanian (2018-03-02 16:58:23)
> 
> 
> On 3/2/2018 1:41 PM, Stephen Boyd wrote:
> > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
> >> +
> >> +/**
> >> + * geni_se_get_qup_hw_version() - Read the QUP wrapper Hardware version
> >> + * @se:Pointer to the corresponding Serial Engine.
> >> + * @major: Buffer for Major Version field.
> >> + * @minor: Buffer for Minor Version field.
> >> + * @step:  Buffer for Step Version field.
> >> + */
> >> +void geni_se_get_qup_hw_version(struct geni_se *se, unsigned int *major,
> >> +   unsigned int *minor, unsigned int *step)
> >> +{
> >> +   unsigned int version;
> >> +   struct geni_wrapper *wrapper = se->wrapper;
> >> +
> >> +   version = readl_relaxed(wrapper->base + QUP_HW_VER_REG);
> >> +   *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT;
> >> +   *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT;
> >> +   *step = version & HW_VER_STEP_MASK;
> >> +}
> >> +EXPORT_SYMBOL(geni_se_get_qup_hw_version);
> > 
> > Is this used?
> SPI controller driver uses this API and it will be uploaded sooner.

Ok. Maybe it can also be a macro to get the u32 and then some more
macros on top of that to pick out the major/minor/step out of the u32
that you read.

> > 
> >> +
> >> +/**
> >> + * geni_se_read_proto() - Read the protocol configured for a Serial Engine
> >> + * @se:Pointer to the concerned Serial Engine.
> >> + *
> >> + * Return: Protocol value as configured in the serial engine.
> >> + */
> >> +u32 geni_se_read_proto(struct geni_se *se)
> >> +{
> >> +   u32 val;
> >> +
> >> +   val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
> >> +
> >> +   return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
> >> +}
> >> +EXPORT_SYMBOL(geni_se_read_proto);
> > 
> > Is this API really needed outside of this file? It would seem like the
> > drivers that implement the protocol, which are child devices, would only
> > use this API to confirm that the protocol chosen is for their particular
> > protocol.
> No, this API is meant for the protocol drivers to confirm that the 
> serial engine is programmed with the firmware for the concerned protocol 
> before using the serial engine. If the check fails, the protocol drivers 
> stop using the serial engine.

Ok maybe we don't really need it then?

> >> + * RX fifo of the serial engine.
> >> + *
> >> + * Return: RX fifo depth in units of FIFO words
> >> + */
> >> +u32 geni_se_get_rx_fifo_depth(struct geni_se *se)
> >> +{
> >> +   u32 val;
> >> +
> >> +   val = readl_relaxed(se->base + SE_HW_PARAM_1);
> >> +
> >> +   return (val & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT;
> >> +}
> >> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth);
> > 
> > These ones too, can probably just be static inline.
> Ok. Just for my knowledge - is there any reference guideline regarding 
> when to use static inline myself and when to let the compiler do the 
> clever thing?

Not that I'm aware of. It's really up to you to decide.

> > 
> >> +
> >> +   ret = geni_se_clks_on(se);
> >> +   if (ret)
> >> +   return ret;
> >> +
> >> +   ret = pinctrl_pm_select_default_state(se->dev);
> >> +   if (ret)
> >> +   geni_se_clks_off(se);
> >> +
> >> +   return ret;
> >> +}
> >> +EXPORT_SYMBOL(geni_se_resources_on);
> > 
> > IS there a reason why we can't use runtime PM or normal linux PM
> > infrastructure to power on the wrapper and keep it powered while the
> > protocol driver is active?
> Besides turning on the clocks & pinctrl settings, wrapper also has to do 
> the bus scaling votes. The bus scaling votes depend on the individual 
> serial interface bandwidth requirements. The bus scaling votes is not 
> present currently. But once the support comes in, this function enables 
> adding it.

Ok, but that would basically be some code consolidation around picking a
bandwidth and enabling/disabling? It sounds like it could go into either
the serial interface drivers or into the runtime PM path of the wrapper.

> > 
> >> +
> >> +/**
> >> + * geni_se_clk_tbl_get() - 

Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Stephen Boyd]

Quoting Karthik Ramasubramanian (2018-03-05 16:51:23)
> On 3/2/2018 3:11 PM, Stephen Boyd wrote:
> > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09)
> > 
> >> +   size_t chars_to_write = 0;
> >> +   size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM;
> >> +
> >> +   /*
> >> +* If the WM bit never set, then the Tx state machine is 
> >> not
> >> +* in a valid state, so break, cancel/abort any existing
> >> +* command. Unfortunately the current data being written is
> >> +* lost.
> >> +*/
> >> +   while (!qcom_geni_serial_poll_bit(uport, 
> >> SE_GENI_M_IRQ_STATUS,
> >> +   M_TX_FIFO_WATERMARK_EN, 
> >> true))
> > 
> > Does this ever timeout? So many nested while loops makes it hard to
> > follow.
> Yes. Based on the baud rate of 115200 and the FIFO Depth & Width of (16 
> * 32), the poll should not take more than 5 ms under the timeout scenario.

Sure, but I'm asking if this has any sort of timeout associated with it.
It looks to be a while loop that could possibly go forever?

> >> +static void qcom_geni_serial_console_write(struct console *co, const char 
> >> *s,
> >> + unsigned int count)
> >> +{
> >> +   struct uart_port *uport;
> >> +   struct qcom_geni_serial_port *port;
> >> +   bool locked = true;
> >> +   unsigned long flags;
> >> +
> >> +   WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> >> +
> >> +   port = get_port_from_line(co->index);
> >> +   if (IS_ERR(port))
> >> +   return;
> >> +
> >> +   uport = >uport;
> >> +   if (oops_in_progress)
> >> +   locked = spin_trylock_irqsave(>lock, flags);
> >> +   else
> >> +   spin_lock_irqsave(>lock, flags);
> >> +
> >> +   if (locked) {
> >> +   __qcom_geni_serial_console_write(uport, s, count);
> > 
> > So if oops is in progress, and we didn't lock here, we don't output
> > data? I'd think we would always want to write to the fifo, just make the
> > lock grab/release conditional.
> If we fail to grab the lock, then there is another active writer. So 
> trying to write to the fifo will put the hardware in bad state because 
> writer has programmed the hardware to write 'x' number of words and this 
> thread will over-write it with 'y' number of words. Also the data that 
> you might see in the console might be garbled.

I suspect that because this is the serial console, and we want it to
always output stuff even when we're going down in flames, we may want to
ignore that case and just wait for the fifo to free up and then
overwrite the number of words that we want to output and push out more
characters.

I always get confused though because there seem to be lots of different
ways to do things in serial drivers and not too much clear documentation
that I've read describing what to do.

> > 
> >> +   spin_unlock_irqrestore(>lock, flags);
> >> +   }
> >> +}
[...]
> >> +
> >> +   if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
> >> +   m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> >> +   qcom_geni_serial_handle_tx(uport);
> >> +
> >> +   if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) {
> >> +   if (s_irq_status & S_GP_IRQ_0_EN)
> >> +   uport->icount.parity++;
> >> +   drop_rx = true;
> >> +   } else if (s_irq_status & S_GP_IRQ_2_EN ||
> >> +   s_irq_status & S_GP_IRQ_3_EN) {
> >> +   uport->icount.brk++;
> > 
> > How does break character handling work? I see the accounting here, but
> > don't see any uart_handle_break() call anywhere.
> The reason it is not added is because the hardware does not indicate 
> when the break character occured. It can be any one of the FIFO words. 
> The statistics is updated to give an idea that the break happened. We 
> can add uart_handle_break() but it may not be at an accurate position 
> for the above mentioned reason.

Sounds like the previous uart design. We would want uart_handle_break()
support for kgdb to work over serial. Do we still get an interrupt
signal that a break ch

Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Stephen Boyd]

Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09)
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 3682fd3..c6b1500 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE
> select SERIAL_CORE_CONSOLE
> select SERIAL_EARLYCON
>  
> +config SERIAL_QCOM_GENI
> +   bool "QCOM on-chip GENI based serial port support"

This can be tristate.

> +   depends on ARCH_QCOM

|| COMPILE_TEST
?

> +   depends on QCOM_GENI_SE
> +   select SERIAL_CORE

This can stay.

> +   select SERIAL_CORE_CONSOLE
> +   select SERIAL_EARLYCON

These two can go to a new config option, like SERIAL_QCOM_GENI_CONSOLE,
and that would be bool. Please take a look at the existing SERIAL_MSM
and SERIAL_MSM_CONSOLE setup to understand how to do it.

> +   help
> + Serial driver for Qualcomm Technologies Inc's GENI based QUP
> + hardware.
> +
>  config SERIAL_VT8500
> bool "VIA VT8500 on-chip serial port support"
> depends on ARCH_VT8500
> diff --git a/drivers/tty/serial/qcom_geni_serial.c 
> b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 000..8536b7d
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* UART specific GENI registers */
> +#define SE_UART_TX_TRANS_CFG   0x25c
> +#define SE_UART_TX_WORD_LEN0x268
> +#define SE_UART_TX_STOP_BIT_LEN0x26c
> +#define SE_UART_TX_TRANS_LEN   0x270
> +#define SE_UART_RX_TRANS_CFG   0x280
> +#define SE_UART_RX_WORD_LEN0x28c
> +#define SE_UART_RX_STALE_CNT   0x294
> +#define SE_UART_TX_PARITY_CFG  0x2a4
> +#define SE_UART_RX_PARITY_CFG  0x2a8
> +
> +/* SE_UART_TRANS_CFG */
> +#define UART_TX_PAR_EN BIT(0)
> +#define UART_CTS_MASK  BIT(1)
> +
> +/* SE_UART_TX_WORD_LEN */
> +#define TX_WORD_LEN_MSKGENMASK(9, 0)
> +
> +/* SE_UART_TX_STOP_BIT_LEN */
> +#define TX_STOP_BIT_LEN_MSKGENMASK(23, 0)
> +#define TX_STOP_BIT_LEN_1  0
> +#define TX_STOP_BIT_LEN_1_51
> +#define TX_STOP_BIT_LEN_2  2
> +
> +/* SE_UART_TX_TRANS_LEN */
> +#define TX_TRANS_LEN_MSK   GENMASK(23, 0)
> +
> +/* SE_UART_RX_TRANS_CFG */
> +#define UART_RX_INS_STATUS_BIT BIT(2)
> +#define UART_RX_PAR_EN BIT(3)
> +
> +/* SE_UART_RX_WORD_LEN */
> +#define RX_WORD_LEN_MASK   GENMASK(9, 0)
> +
> +/* SE_UART_RX_STALE_CNT */
> +#define RX_STALE_CNT   GENMASK(23, 0)
> +
> +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */
> +#define PAR_CALC_ENBIT(0)
> +#define PAR_MODE_MSK   GENMASK(2, 1)
> +#define PAR_MODE_SHFT  1
> +#define PAR_EVEN   0x00
> +#define PAR_ODD0x01
> +#define PAR_SPACE  0x10
> +#define PAR_MARK   0x11
> +
> +/* UART M_CMD OP codes */
> +#define UART_START_TX  0x1
> +#define UART_START_BREAK   0x4
> +#define UART_STOP_BREAK0x5
> +/* UART S_CMD OP codes */
> +#define UART_START_READ0x1
> +#define UART_PARAM 0x1
> +
> +#define UART_OVERSAMPLING  32
> +#define STALE_TIMEOUT  16
> +#define DEFAULT_BITS_PER_CHAR  10
> +#define GENI_UART_CONS_PORTS   1
> +#define DEF_FIFO_DEPTH_WORDS   16
> +#define DEF_TX_WM  2
> +#define DEF_FIFO_WIDTH_BITS32
> +#define UART_CONSOLE_RX_WM 2
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +#define RX_BYTES_PW 1
> +#else
> +#define RX_BYTES_PW 4
> +#endif
> +
> +struct qcom_geni_serial_port {
> +   struct uart_port uport;
> +   struct geni_se se;
> +   char name[20];
> +   u32 tx_fifo_depth;
> +   u32 tx_fifo_width;
> +   u32 rx_fifo_depth;
> +   u32 tx_wm;
> +   u32 rx_wm;
> +   u32 rx_rfr;
> +   int xfer_mode;

Can this be an enum?

> +   bool port_setup;

Maybe just 'setup'? Port is in the type already.

> +   int (*handle_rx)(struct uart_port *uport,
> +   u32 rx_bytes, bool drop_rx);

s/rx_bytes/bytes/
s/drop_rx/drop/

> +   unsigned int xmit_size;
> +   unsigned int cur_baud;

s/cur//

> +   unsigned int tx_bytes_pw;
> +   unsigned int rx_bytes_pw;
> +};
> +
> +static const struct uart_ops qcom_geni_serial_pops;
> +static struct uart_driver qcom_geni_console_driver;
> +static int handle_rx_console(struct uart_port *uport,
> +   u32 rx_bytes, bool drop_rx);

s/rx_bytes/bytes/
s/drop_rx/drop/

> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> +

Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver

[Stephen Boyd]

Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
> This driver manages the Generic Interface (GENI) firmware based Qualcomm
> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
> programmable module composed of multiple Serial Engines (SE) and supports
> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
> driver also enables managing the serial interface independent aspects of
> Serial Engines.
> 
> Signed-off-by: Karthikeyan Ramasubramanian 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Girish Mahadevan 
> ---
>  drivers/soc/qcom/Kconfig|   9 +
>  drivers/soc/qcom/Makefile   |   1 +
>  drivers/soc/qcom/qcom-geni-se.c | 971 
> 
>  include/linux/qcom-geni-se.h| 247 ++
>  4 files changed, 1228 insertions(+)
>  create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>  create mode 100644 include/linux/qcom-geni-se.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index e050eb8..cc460d0 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -3,6 +3,15 @@
>  #
>  menu "Qualcomm SoC drivers"
>  
> +config QCOM_GENI_SE
> +   tristate "QCOM GENI Serial Engine Driver"
> +   depends on ARCH_QCOM

Add || COMPILE_TEST?

> +   help
> + This module is used to manage Generic Interface (GENI) firmware 
> based

s/module/driver?

> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This
> + module is also used to manage the common aspects of multiple Serial
s/module/driver?

> + Engines present in the QUP.
> +
>  config QCOM_GLINK_SSR
> tristate "Qualcomm Glink SSR driver"
> depends on RPMSG
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> new file mode 100644
> index 000..61335b8
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -0,0 +1,971 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

#include 

> +
> +/**
> + * DOC: Overview
> + *
> + * Generic Interface (GENI) Serial Engine (SE) Wrapper driver is introduced
> + * to manage GENI firmware based Qualcomm Universal Peripheral (QUP) Wrapper
> + * controller. QUP Wrapper is designed to support various serial bus 
> protocols
> + * like UART, SPI, I2C, I3C, etc.
> + */
> +
> +/**
> + * DOC: Hardware description
> + *
> + * GENI based QUP is a highly-flexible and programmable module for supporting
> + * a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. A single
> + * QUP module can provide upto 8 Serial Interfaces, using its internal
> + * Serial Engines. The actual configuration is determined by the target
> + * platform configuration. The protocol supported by each interface is
> + * determined by the firmware loaded to the Serial Engine. Each SE consists
> + * of a DMA Engine and GENI sub modules which enable Serial Engines to
> + * support FIFO and DMA modes of operation.
> + *
> + *
> + *  +-+
> + *  |QUP Wrapper  |
> + *  | ++  |
> + *   --QUP & SE Clocks--> | Serial Engine N|  +-IO-->
> + *  | | ...|  | Interface
> + *   <---Clock Perf.+++---+|  |
> + * State Interface  || Serial Engine 1||  |
> + *  ||||  |
> + *  ||||  |
> + *   |||  |
> + *  ||++  |
> + *  |||   |
> + *  |||   |
> + *   <--SE IRQ--+++   |
> + *  | |
> + *  +-+
> + *
> + * Figure 1: GENI based QUP Wrapper

The code talks about primary and secondary sequencers, but this hardware
description doesn't talk about it. Can you add some more information
here about that aspect too?

> + */
> +
> +/**
> + * DOC: Software description
> + *
> + * GENI SE Wrapper driver is structured into 2 parts:
> + *
> + * geni_wrapper represents QUP Wrapper controller. This part of the driver
> + * manages QUP Wrapper information such as hardware version, clock
> + * performance table that is common to all the internal Serial Engines.
> + *
> + * geni_se represents Serial 

[PATCHv2] of: Document exactly what of_find_node_by_name() puts

[Stephen Boyd]

It isn't clear if this function of_node_put()s the 'from'
argument, or the node it searches. Clearly indicate which
variable is touched. Fold in some more fixes from Randy too
because we're in the area.

Cc: Randy Dunlap <rdun...@infradead.org>
Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
---

Changes from v1:
 * Fold in Randy's fixes

 drivers/of/base.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 63897531cd75..4368a878df88 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -863,10 +863,10 @@ EXPORT_SYMBOL(of_find_node_opts_by_path);
 
 /**
  * of_find_node_by_name - Find a node by its "name" property
- * @from:  The node to start searching from or NULL, the node
+ * @from:  The node to start searching from or NULL; the node
  * you pass will not be searched, only the next one
- * will; typically, you pass what the previous call
- * returned. of_node_put() will be called on it
+ * will. Typically, you pass what the previous call
+ * returned. of_node_put() will be called on @from.
  * @name:  The name string to match against
  *
  * Returns a node pointer with refcount incremented, use
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Document exactly what of_find_node_by_name() puts

[Stephen Boyd]

On 11/10, Randy Dunlap wrote:
> On 11/10/2017 05:45 PM, Stephen Boyd wrote:
> > It isn't clear if this function of_node_put()s the 'from'
> > argument, or the node it finds in the search. Clearly indicate
> > which variable is touched.
> > 
> > Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
> > ---
> >  drivers/of/base.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index 63897531cd75..602c5d65734a 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -866,7 +866,7 @@ EXPORT_SYMBOL(of_find_node_opts_by_path);>   *  @from:  
> > The node to start searching from or NULL, the node
> 
> * @from:  The node to start searching from or %NULL; the node
> 
> >   * you pass will not be searched, only the next one
> >   * will; typically, you pass what the previous call
> 
> * will. Typically, you pass what the previous call
> 

Rob, do you want me to fold this in?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] of: Document exactly what of_find_node_by_name() puts

[Stephen Boyd]

It isn't clear if this function of_node_put()s the 'from'
argument, or the node it finds in the search. Clearly indicate
which variable is touched.

Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
---
 drivers/of/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 63897531cd75..602c5d65734a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -866,7 +866,7 @@ EXPORT_SYMBOL(of_find_node_opts_by_path);
  * @from:  The node to start searching from or NULL, the node
  * you pass will not be searched, only the next one
  * will; typically, you pass what the previous call
- * returned. of_node_put() will be called on it
+ * returned. of_node_put() will be called on @from.
  * @name:  The name string to match against
  *
  * Returns a node pointer with refcount incremented, use
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 7/9] clk: hi6220: add debug APB clock

[Stephen Boyd]

On 03/26, Leo Yan wrote:
> The debug APB clock is absent in hi6220 driver, so this patch is to add
> support for it.
> 
> Signed-off-by: Leo Yan 
> ---

Applied to clk-next. I suspect we don't need a topic branch for
the DT header because arm-soc won't be taking the dts side of the
changes?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/9] coresight: use const for device_node structures

[Stephen Boyd]

On 03/26, Leo Yan wrote:
> Almost low level functions from open firmware have used const to
> qualify device_node structures, so add const for device_node
> parameters in of_coresight related functions.
> 
> Signed-off-by: Leo Yan <leo@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sb...@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 34/46] clk: pwm: switch to the atomic API

[Stephen Boyd]

On 03/30, Boris Brezillon wrote:
> diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> index ebcd738..49ec5b1 100644
> --- a/drivers/clk/clk-pwm.c
> +++ b/drivers/clk/clk-pwm.c
> @@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw 
> *hw)
>  static int clk_pwm_prepare(struct clk_hw *hw)
>  {
>   struct clk_pwm *clk_pwm = to_clk_pwm(hw);
> + struct pwm_state pstate;
>  
> - return pwm_enable(clk_pwm->pwm);
> + pwm_get_state(clk_pwm->pwm, );
> + if (pstate.enabled)
> + return 0;
> +
> + pstate.enabled = true;
> +
> + return pwm_apply_state(clk_pwm->pwm, );

This doesn't seem atomic anymore if we're checking the state and
then not calling apply_state if it's already enabled. But I
assume this doesn't matter because we "own" the pwm here?
Otherwise I would think this would be unconditional apply state
and duplicates would be ignored in the pwm framework.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

[Stephen Boyd]

On 03/30, Boris Brezillon wrote:
> @@ -74,6 +74,23 @@ enum pwm_polarity {
>   PWM_POLARITY_INVERSED,
>  };
>  
> +/**
> + * struct pwm_args - PWM arguments
> + * @period: reference period
> + * @polarity: reference polarity
> + *
> + * This structure describe board-dependent arguments attached to a PWM

s/describe/describes/

> + * device. Those arguments are usually retrieved from the PWM lookup table or
> + * DT definition.
> + * This should not be confused with the PWM state: PWM args not representing

s/not representing/don't represent/ ?

> + * the current PWM state, but the configuration the PWM user plan to use

s/plan/plans/

> + * on this PWM device.
> + */
> +struct pwm_args {
> + unsigned int period;
> + enum pwm_polarity polarity;
> +};
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html