| Message ID | 1521071931-9294-1-git-send-email-kramasub@codeaurora.org |
|---|---|
| Headers | show |
| Series | Introduce GENI SE Controller Driver | expand |
Hi, On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> wrote: > Add I2C master controller support for a built-in test I2C slave. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > --- > arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 19 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sdm845.dtsi | 29 +++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > index ea3efc5..69445f1 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts > @@ -27,6 +27,10 @@ > serial@a84000 { > status = "okay"; > }; > + > + i2c@a88000 { > + status = "okay"; Any idea what clock frequency is appropriate for MTP? You've got some pretty beefy 2.2K external pulls I think, so presumably 400 KHz is what you're aiming for? It would be nice to specify one so you don't end up the the spam in the log "Bus frequency not specified ..." > + }; > }; > > pinctrl@3400000 { > @@ -50,5 +54,20 @@ > bias-pull-down; > }; > }; > + > + qup-i2c10-default { nit: probably in the pinctrl section we want to sort alphabetically? We could try to sort by "lowest numbered pin", but IMHO alphabetical makes the most sense. > + pinconf { > + pins = "gpio55", "gpio56"; > + drive-strength = <2>; > + bias-disable; > + }; > + }; > + > + qup-i2c10-sleep { > + pinconf { > + pins = "gpio55", "gpio56"; > + bias-pull-up; Are you sure that you want pullups enabled for sleep here? There are external pulls on this line (as there are on many i2c busses) so doing this will double-enable pulls. It probably won't hurt, but I'm curious if there's some sort of reason here. > + }; > + }; > }; > }; > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index 59334d9..9ef056f 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -209,6 +209,21 @@ > pins = "gpio4", "gpio5"; > }; > }; > + > + qup_i2c10_default: qup-i2c10-default { > + pinmux { > + function = "qup10"; > + pins = "gpio55", "gpio56"; > + }; > + }; > + > + qup_i2c10_sleep: qup-i2c10-sleep { > + pinmux { > + function = "gpio"; > + pins = "gpio55", "gpio56"; > + }; > + }; > + > }; > > timer@17c90000 { > @@ -309,6 +324,20 @@ > interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; > status = "disabled"; > }; > + > + i2c10: i2c@a88000 { Seems like it might be nice to add all the i2c busses into the main sdm845.dtsi file. Sure, most won't be enabled, but it seems like it would avoid churn later. ...if you're sure you want to add only one i2c controller, subject of this patch should indicate that. > + compatible = "qcom,geni-i2c"; > + reg = <0xa88000 0x4000>; > + clock-names = "se"; > + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&qup_i2c10_default>; > + pinctrl-1 = <&qup_i2c10_sleep>; > + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > }; > }; > }; > -- > 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-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> wrote: > This bus driver supports the GENI based i2c hardware controller in the > Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable > module supporting a wide range of serial interfaces including I2C. The > driver supports FIFO mode and DMA mode of transfer and switches modes > dynamically depending on the size of the transfer. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/i2c/busses/Kconfig | 13 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-qcom-geni.c | 648 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 662 insertions(+) Since I reviewed v3 of the i2c patch, can you make sure that you CC me on future patches? Thanks! :) Overall this looks pretty nice to me now. A few minor comments... > +/* > + * Hardware uses the underlying formula to calculate time periods of > + * SCL clock cycle. > + * > + * time of high period of SCL: t_high = (t_high_cnt * clk_div) / source_clock > + * time of low period of SCL: t_low = (t_low_cnt * clk_div) / source_clock > + * time of full period of SCL: t_cycle = (t_cycle_cnt * clk_div) / source_clock > + * clk_freq_out = t / t_cycle > + * source_clock = 19.2 MHz > + */ > +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { > + {KHZ(100), 7, 10, 11, 26}, > + {KHZ(400), 2, 5, 12, 24}, > + {KHZ(1000), 1, 3, 9, 18}, > +}; Thanks for the docs. ...though if these docs are indeed correct and there's no extra magic fudge factor I'm still a bit baffled why the frequency isn't out of spec for 100 kHz and 1 MHz. I know you said hardware validated it, but are you really certain they validated 100 kHz and 1 MHz? >>> 19200. / (1 * 18) 1066.6666666666667 >>> 19200. / (2 * 24) 400.0 >>> 19200. / (7 * 26) 105.49450549450549 Specifically: either the docs you wrote above are wrong (and there's a magic fudge factor that you didn't document) or your hardware guys are wrong. See the I2C spec at <https://www.nxp.com/docs/en/user-guide/UM10204.pdf>. Table 10. ("Characteristics of the SDA and SCL bus lines for Standard, Fast, and Fast-mode Plus I2C-bus devices") says fSCL Max is 100 kHz, 400 kHz, or 1000 kHz. I could believe that 99.9% of all devices that support 100 kHz also support 105.5 kHz and that 99.9% of all devices that support 1000 kHz also support 1066.7 kHz. However, it's still not in spec. If you want to enable a turbo boost mode that runs 5% faster (really?) then I suppose you could add that as an optional feature, but this shouldn't be the default. > +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c) > +{ > + u32 val; > + unsigned long time_left = ABORT_TIMEOUT; > + unsigned long flags; > + > + spin_lock_irqsave(&gi2c->lock, flags); > + geni_i2c_err(gi2c, GENI_TIMEOUT); > + gi2c->cur = NULL; > + geni_se_abort_m_cmd(&gi2c->se); > + spin_unlock_irqrestore(&gi2c->lock, flags); > + do { > + time_left = wait_for_completion_timeout(&gi2c->done, time_left); > + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS); > + } while (!(val & M_CMD_ABORT_EN) && time_left); > + > + if (!(val & M_CMD_ABORT_EN) && !time_left) Why do you need to check !time_left? Just "if (!(val & M_CMD_ABORT_EN))". > + dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n"); > +} > + > +static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c) > +{ > + u32 val; > + unsigned long time_left = RST_TIMEOUT; > + > + writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST); > + do { > + time_left = wait_for_completion_timeout(&gi2c->done, time_left); > + val = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT); > + } while (!(val & RX_RESET_DONE) && time_left); > + > + if (!(val & RX_RESET_DONE) && !time_left) Similar. Don't need "&& !time_left" > + dev_err(gi2c->se.dev, "Timeout resetting RX_FSM\n"); > +} > + > +static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) > +{ > + u32 val; > + unsigned long time_left = RST_TIMEOUT; > + > + writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST); > + do { > + time_left = wait_for_completion_timeout(&gi2c->done, time_left); > + val = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT); > + } while (!(val & TX_RESET_DONE) && time_left); > + > + if (!(val & TX_RESET_DONE) && !time_left) Similar. Don't need "&& !time_left" > +static int geni_i2c_probe(struct platform_device *pdev) > +{ > + struct geni_i2c_dev *gi2c; > + struct resource *res; > + u32 proto, tx_depth; > + int ret; > + > + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL); > + if (!gi2c) > + return -ENOMEM; > + > + gi2c->se.dev = &pdev->dev; > + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(gi2c->se.base)) > + return PTR_ERR(gi2c->se.base); > + > + gi2c->se.clk = devm_clk_get(&pdev->dev, "se"); > + if (IS_ERR(gi2c->se.clk)) { > + ret = PTR_ERR(gi2c->se.clk); > + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); > + return ret; > + } > + > + ret = device_property_read_u32(&pdev->dev, "clock-frequency", > + &gi2c->clk_freq_out); > + if (ret) { > + /* All GENI I2C slaves support 400kHz. So default to 400kHz. */ Can you explain this comment? Are you saying that GENI only supports talking to I2C devices (devices are also known as "slaves" and GENI should be the I2C master) that talk 400 kHz I2C or better? Why do you even have 100 kHz in your table above then? I don't think this is what you meant... Perhaps you meant to say "All GENI I2C masters support at least 400 kHz, so default ot 400 kHz" ...however, even if you changed the comment as I suggested I'm still not a fan. As I said in my review of the prevous version: > I feel like it should default to 100KHz. i2c_parse_fw_timings() > defaults to this and to me the wording "New drivers almost always > should use the defaults" makes me feel this should be the defaults. I would also say that even if all GENI I2C masters support at least 400 kHz that doesn't mean that all I2C devices on the bus support 400 kHz. You could certainly imagine someone sticking something on this bus that was super low cost and supported only 100 kHz I2C. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doug, Thanks for reviewing the patch. On 3/16/2018 5:54 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian > <kramasub@codeaurora.org> wrote: >> Add I2C master controller support for a built-in test I2C slave. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> --- >> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 19 +++++++++++++++++++ >> arch/arm64/boot/dts/qcom/sdm845.dtsi | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> index ea3efc5..69445f1 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts >> @@ -27,6 +27,10 @@ >> serial@a84000 { >> status = "okay"; >> }; >> + >> + i2c@a88000 { >> + status = "okay"; > > Any idea what clock frequency is appropriate for MTP? You've got some > pretty beefy 2.2K external pulls I think, so presumably 400 KHz is > what you're aiming for? It would be nice to specify one so you don't > end up the the spam in the log "Bus frequency not specified ..." > > Yes, we plan to use 400Khz. We will specify it here. >> + }; >> }; >> >> pinctrl@3400000 { >> @@ -50,5 +54,20 @@ >> bias-pull-down; >> }; >> }; >> + >> + qup-i2c10-default { > > nit: probably in the pinctrl section we want to sort alphabetically? > We could try to sort by "lowest numbered pin", but IMHO alphabetical > makes the most sense. > Sure. > >> + pinconf { >> + pins = "gpio55", "gpio56"; >> + drive-strength = <2>; >> + bias-disable; >> + }; >> + }; >> + >> + qup-i2c10-sleep { >> + pinconf { >> + pins = "gpio55", "gpio56"; >> + bias-pull-up; > > Are you sure that you want pullups enabled for sleep here? There are > external pulls on this line (as there are on many i2c busses) so doing > this will double-enable pulls. It probably won't hurt, but I'm > curious if there's some sort of reason here. > 1. We need the lines to remain high to avoid slaves sensing a false start-condition (this can happen if the SDA goes down before SCL). 2. Disclaimer: I'm not a HW expert, but we were told that tri-state/bias-disabled lines can draw more current. I will find out more about that. > >> + }; >> + }; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> index 59334d9..9ef056f 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> @@ -209,6 +209,21 @@ >> pins = "gpio4", "gpio5"; >> }; >> }; >> + >> + qup_i2c10_default: qup-i2c10-default { >> + pinmux { >> + function = "qup10"; >> + pins = "gpio55", "gpio56"; >> + }; >> + }; >> + >> + qup_i2c10_sleep: qup-i2c10-sleep { >> + pinmux { >> + function = "gpio"; >> + pins = "gpio55", "gpio56"; >> + }; >> + }; >> + >> }; >> >> timer@17c90000 { >> @@ -309,6 +324,20 @@ >> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >> status = "disabled"; >> }; >> + >> + i2c10: i2c@a88000 { > > Seems like it might be nice to add all the i2c busses into the main > sdm845.dtsi file. Sure, most won't be enabled, but it seems like it > would avoid churn later. > > ...if you're sure you want to add only one i2c controller, subject of > this patch should indicate that. > 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? Thanks Sagar > >> + compatible = "qcom,geni-i2c"; >> + reg = <0xa88000 0x4000>; >> + clock-names = "se"; >> + clocks = <&gcc GCC_QUPV3_WRAP1_S2_CLK>; >> + pinctrl-names = "default", "sleep"; >> + pinctrl-0 = <&qup_i2c10_default>; >> + pinctrl-1 = <&qup_i2c10_sleep>; >> + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "disabled"; >> + }; >> }; >> }; >> }; >> -- >> 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-arm-msm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi, On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> wrote: >>> + pinconf { >>> + pins = "gpio55", "gpio56"; >>> + drive-strength = <2>; >>> + bias-disable; >>> + }; >>> + }; >>> + >>> + qup-i2c10-sleep { >>> + pinconf { >>> + pins = "gpio55", "gpio56"; >>> + bias-pull-up; >> >> Are you sure that you want pullups enabled for sleep here? There are >> external pulls on this line (as there are on many i2c busses) so doing >> this will double-enable pulls. It probably won't hurt, but I'm >> curious if there's some sort of reason here. >> > 1. We need the lines to remain high to avoid slaves sensing a false > start-condition (this can happen if the SDA goes down before SCL). > 2. Disclaimer: I'm not a HW expert, but we were told that > tri-state/bias-disabled lines can draw more current. I will find out > more about that. Agreed that they need to remain high, but you've got very strong pullups external to the SoC. Those will keep it high. You don't need the internal ones too. As extra evidence that the external pullups _must_ be present on your board: you specify bias-disable in the active state. That can only work if there are external pullups (or if there were some special extra secret internal pullups that were part of geni). i2c is an open-drain bus and thus there must be pullups on the bus in order to communicate. >>> + }; >>> + }; >>> }; >>> }; >>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> index 59334d9..9ef056f 100644 >>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>> @@ -209,6 +209,21 @@ >>> pins = "gpio4", "gpio5"; >>> }; >>> }; >>> + >>> + qup_i2c10_default: qup-i2c10-default { >>> + pinmux { >>> + function = "qup10"; >>> + pins = "gpio55", "gpio56"; >>> + }; >>> + }; >>> + >>> + qup_i2c10_sleep: qup-i2c10-sleep { >>> + pinmux { >>> + function = "gpio"; >>> + pins = "gpio55", "gpio56"; >>> + }; >>> + }; >>> + >>> }; >>> >>> timer@17c90000 { >>> @@ -309,6 +324,20 @@ >>> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >>> status = "disabled"; >>> }; >>> + >>> + i2c10: i2c@a88000 { >> >> Seems like it might be nice to add all the i2c busses into the main >> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it >> would avoid churn later. >> >> ...if you're sure you want to add only one i2c controller, subject of >> this patch should indicate that. >> > > 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. ...but yeah, I'm asking for them all to be listed with status="disabled". -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-03-19 16:56:27) > On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 0000000..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 = &port->uport; > + if (oops_in_progress) > + locked = spin_trylock_irqsave(&uport->lock, flags); > + else > + spin_lock_irqsave(&uport->lock, flags); > + > + /* Cancel the current write to log the fault */ > + if (!locked) { > + geni_se_cancel_m_cmd(&port->se); > + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, > + M_CMD_CANCEL_EN, true)) { > + geni_se_abort_m_cmd(&port->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(&uport->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 = &uport->state->port; > + for (i = 0; i < bytes; ) { > + int c; > + int chunk = min_t(int, bytes - i, port->rx_bytes_pw); > + > + ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, buf, 1); > + i += chunk; > + if (drop) > + continue; > + > + for (c = 0; c < chunk; c++) { > + int sysrq; > + > + uport->icount.rx++; > + if (port->brk && buf[c] == 0) { > + port->brk = false; > + if (uart_handle_break(uport)) > + continue; Thanks! > + } > + > + sysrq = uart_handle_sysrq_char(uport, buf[c]); > + if (!sysrq) > + tty_insert_flip_char(tport, buf[c], TTY_NORMAL); > + } > + } > + if (!drop) > + tty_flip_buffer_push(tport); > + return 0; > +} [...] > + > +static void qcom_geni_serial_handle_tx(struct uart_port *uport) > +{ > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + struct circ_buf *xmit = &uport->state->xmit; > + size_t avail; > + size_t remaining; > + int i; > + u32 status; > + unsigned int chunk; > + int tail; > + > + chunk = uart_circ_chars_pending(xmit); > + status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS); > + /* Both FIFO and framework buffer are drained */ > + if (chunk == port->xmit_size && !status) { > + port->xmit_size = 0; > + uart_circ_clear(xmit); > + qcom_geni_serial_stop_tx(uport); > + goto out_write_wakeup; > + } > + chunk -= port->xmit_size; > + > + avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw; > + tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1); > + if (chunk > (UART_XMIT_SIZE - tail)) > + chunk = UART_XMIT_SIZE - tail; > + if (chunk > avail) > + chunk = avail; chunk = min3(chunk, UART_XMIT_SIZE - tail, avail); > + > + if (!chunk) > + goto out_write_wakeup; > + > + qcom_geni_serial_setup_tx(uport, chunk); > + > + remaining = chunk; > + for (i = 0; i < chunk; ) { > + unsigned int tx_bytes; > + unsigned int buf = 0; Make buf into a u8 array of 4? > + int c; > + > + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw); > + for (c = 0; c < tx_bytes ; c++) > + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE)); And then just put buf[c] = xmit->buf[tail + c] here? > + > + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn); This also needs to be an iowrite32_rep(uport->membase, buf, 1) for endian reasons. > + > + i += tx_bytes; > + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1); > + uport->icount.tx += tx_bytes; > + remaining -= tx_bytes; > + } > + qcom_geni_serial_poll_tx_done(uport); > + port->xmit_size += chunk; > +out_write_wakeup: > + uart_write_wakeup(uport); > +} > + > +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev) > +{ > + unsigned int m_irq_status; > + unsigned int s_irq_status; > + struct uart_port *uport = dev; > + unsigned long flags; > + unsigned int m_irq_en; > + bool drop_rx = false; > + struct tty_port *tport = &uport->state->port; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + > + if (uport->suspended) > + return IRQ_HANDLED; Is it a spurious IRQ if this happens? Return IRQ_NONE instead? > + > + spin_lock_irqsave(&uport->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? > + port->brk = true; > + } > + > + if (s_irq_status & S_RX_FIFO_WATERMARK_EN || > + s_irq_status & S_RX_FIFO_LAST_EN) > + qcom_geni_serial_handle_rx(uport, drop_rx); > + > +out_unlock: > + spin_unlock_irqrestore(&uport->lock, flags); > + return IRQ_HANDLED; > +} > + > +static int get_tx_fifo_size(struct qcom_geni_serial_port *port) > +{ > + struct uart_port *uport; > + > + if (!port) > + return -ENODEV; This happens? > + > + uport = &port->uport; > + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se); > + port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se); > + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se); > + uport->fifosize = > + (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE; > + return 0; > +} > + [...] > + > +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE > +static int __init qcom_geni_console_setup(struct console *co, char *options) > +{ > + struct uart_port *uport; > + struct qcom_geni_serial_port *port; > + int baud; > + int bits = 8; > + int parity = 'n'; > + int flow = 'n'; > + > + if (co->index >= GENI_UART_CONS_PORTS || co->index < 0) > + return -ENXIO; > + > + port = get_port_from_line(co->index); > + if (IS_ERR(port)) { > + pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port)); Use %ld to avoid casting the error value? Or just don't print it at all because it doesn't really help the end user. > + return PTR_ERR(port); > + } > + > + uport = &port->uport; > + > + if (unlikely(!uport->membase)) > + return -ENXIO; > + > + if (geni_se_resources_on(&port->se)) { > + dev_err(port->se.dev, "Error turning on resources\n"); > + return -ENXIO; > + } > + > + if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) { > + geni_se_resources_off(&port->se); > + return -ENXIO; > + } > + > + if (!port->setup) { > + port->tx_bytes_pw = 1; > + port->rx_bytes_pw = RX_BYTES_PW; > + qcom_geni_serial_stop_rx(uport); > + qcom_geni_serial_port_setup(uport); > + } > + > + if (options) > + uart_parse_options(options, &baud, &parity, &bits, &flow); > + > + return uart_set_options(uport, co, baud, parity, bits, flow); > +} [..] > + > +static int qcom_geni_serial_probe(struct platform_device *pdev) > +{ > + int ret = 0; > + int line = -1; > + struct qcom_geni_serial_port *port; > + struct uart_port *uport; > + struct resource *res; > + > + if (pdev->dev.of_node) > + line = of_alias_get_id(pdev->dev.of_node, "serial"); > + else > + line = pdev->id; Do we need to support the non-alias method? > + > + if (line < 0 || line >= GENI_UART_CONS_PORTS) > + return -ENXIO; > + port = get_port_from_line(line); > + if (IS_ERR(port)) { > + ret = PTR_ERR(port); > + dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret); > + return ret; > + } > + > + uport = &port->uport; > + /* Don't allow 2 drivers to access the same port */ > + if (uport->private_data) > + return -ENODEV; > + > + uport->dev = &pdev->dev; > + port->se.dev = &pdev->dev; > + port->se.wrapper = dev_get_drvdata(pdev->dev.parent); > + port->se.clk = devm_clk_get(&pdev->dev, "se"); > + if (IS_ERR(port->se.clk)) { > + ret = PTR_ERR(port->se.clk); > + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); > + return ret; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (IS_ERR(res)) > + return PTR_ERR(res); > + uport->mapbase = res->start; > + > + port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS; > + port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS; > + port->tx_fifo_width = DEF_FIFO_WIDTH_BITS; > + > + uport->irq = platform_get_irq(pdev, 0); > + if (uport->irq < 0) { > + dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq); > + return uport->irq; > + } > + > + uport->private_data = &qcom_geni_console_driver; > + platform_set_drvdata(pdev, port); > + port->handle_rx = handle_rx_console; > + port->setup = false; This would be set to false already? > + return uart_add_one_port(&qcom_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, &port->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 = &port->uport; > + > + uart_suspend_port(uport->private_data, uport); > + return 0; > +} > + > +static int __maybe_unused qcom_geni_serial_sys_resume_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 = &port->uport; > + > + if (console_suspend_enabled && uport->suspended) { > + uart_resume_port(uport->private_data, uport); > + disable_irq(uport->irq); I missed the enable_irq() part. Is this still necessary? > + } > + return 0; > +} > + > +static int __init qcom_geni_serial_init(void) > +{ > + int ret; > + > + qcom_geni_console_port.uport.iotype = UPIO_MEM; > + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops; > + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF; > + qcom_geni_console_port.uport.line = 0; Why can't this be done statically? > + > + ret = console_register(&qcom_geni_console_driver); > + if (ret) > + return ret; > + > + ret = platform_driver_register(&qcom_geni_serial_platform_driver); > + if (ret) > + console_unregister(&qcom_geni_console_driver); > + return ret; > +} > +module_init(qcom_geni_serial_init); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Karthik, On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian < kramasub@codeaurora.org> wrote: > + > +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > + int offset, int field, bool set) > +{ > + u32 reg; > + struct qcom_geni_serial_port *port; > + unsigned int baud; > + unsigned int fifo_bits; > + unsigned long timeout_us = 20000; > + > + /* Ensure polling is not re-ordered before the prior writes/reads */ > + mb(); These barriers sprinkled around everywhere are new. Did you find that you needed them for something? In this case, the readl_poll_timeout_atomic should already have a read barrier (nor do you probably care about read reordering, right?) Perhaps this should only be a mmiowb rather than a full barrier, because you really just want to say "make sure all my old writes got out to hardware before spinning" > + > + if (uport->private_data) { > + port = to_dev_port(uport, uport); > + baud = port->baud; > + if (!baud) > + baud = 115200; > + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width; > + /* > + * Total polling iterations based on FIFO worth of bytes to be > + * sent at current baud. Add a little fluff to the wait. > + */ > + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > + } > + > + return !readl_poll_timeout_atomic(uport->membase + offset, reg, > + (bool)(reg & field) == set, 10, timeout_us); > +} [...] > + > +static void qcom_geni_serial_start_tx(struct uart_port *uport) > +{ > + u32 irq_en; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + u32 status; > + > + if (port->xfer_mode == GENI_SE_FIFO) { > + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > + if (status & M_GENI_CMD_ACTIVE) > + return; > + > + if (!qcom_geni_serial_tx_empty(uport)) > + return; > + > + /* > + * Ensure writing to IRQ_EN & watermark registers are not > + * re-ordered before checking the status of the Serial > + * Engine and TX FIFO > + */ > + mb(); Here's another one. You should just be able to use a regular readl when reading SE_GENI_STATUS and remove this barrier, since readl has an rmb which orders your read of M_IRQ_EN below. I don't think you need to worry about the writes below going above the read above, since there's logic in between that needs the result of the read. Maybe that also saves you in the read case, too. Either way, this barrier seems heavy handed. > + > + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > + irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN; > + > + writel_relaxed(port->tx_wm, uport->membase + > + SE_GENI_TX_WATERMARK_REG); > + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > + } > +} > + > +static void qcom_geni_serial_stop_tx(struct uart_port *uport) > +{ > + u32 irq_en; > + u32 status; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + > + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > + irq_en &= ~M_CMD_DONE_EN; > + if (port->xfer_mode == GENI_SE_FIFO) { > + irq_en &= ~M_TX_FIFO_WATERMARK_EN; > + writel_relaxed(0, uport->membase + > + SE_GENI_TX_WATERMARK_REG); > + } > + port->xmit_size = 0; > + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > + /* Possible stop tx is called multiple times. */ > + if (!(status & M_GENI_CMD_ACTIVE)) > + return; > + > + /* > + * Ensure cancel command write is not re-ordered before checking > + * the status of the Primary Sequencer. > + */ > + mb(); I don't see how what's stated in your comment could happen, as that would be a logic error. This barrier seems unneeded to me. > + > + geni_se_cancel_m_cmd(&port->se); > + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, > + M_CMD_CANCEL_EN, true)) { > + geni_se_abort_m_cmd(&port->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); > +} > + > +static void qcom_geni_serial_start_rx(struct uart_port *uport) > +{ > + u32 irq_en; > + u32 status; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + > + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > + if (status & S_GENI_CMD_ACTIVE) > + qcom_geni_serial_stop_rx(uport); > + > + /* > + * Ensure setup command write is not re-ordered before checking > + * the status of the Secondary Sequencer. > + */ > + mb(); Also here, I think the reordering you're worried about would mean the CPU is executing incorrectly. > + > + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0); > + > + if (port->xfer_mode == GENI_SE_FIFO) { > + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); > + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN; > + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); > + > + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN; > + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > + } > +} > + > +static void qcom_geni_serial_stop_rx(struct uart_port *uport) > +{ > + u32 irq_en; > + u32 status; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > + u32 irq_clear = S_CMD_DONE_EN; > + > + if (port->xfer_mode == GENI_SE_FIFO) { > + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); > + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN); > + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); > + > + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > + } > + > + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > + /* Possible stop rx is called multiple times. */ > + if (!(status & S_GENI_CMD_ACTIVE)) > + return; > + > + /* > + * Ensure cancel command write is not re-ordered before checking > + * the status of the Secondary Sequencer. > + */ > + mb(); Same deal here. > + > + geni_se_cancel_s_cmd(&port->se); > + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG, > + S_GENI_CMD_CANCEL, false); > + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR); > + if (status & S_GENI_CMD_ACTIVE) > + qcom_geni_serial_abort_rx(uport); > +} > + Sorry gmail decided to wrap some of the context lines. -Evan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = &uart2; > + }; > + > + chosen { > + stdout-path = "serial0"; Also add :115200n8 ? > + }; > +}; > + > +&soc { I think the method is to put these inside soc node without using the phandle reference. So indent everything once more. > + geniqup@ac0000 { > + serial@a84000 { > + status = "okay"; > + }; > + }; > + > + pinctrl@3400000 { > + 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 <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/qcom,gcc-sdm845.h> > > / { > interrupt-parent = <&intc>; > @@ -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@17c90000 { > @@ -272,5 +287,28 @@ > #interrupt-cells = <4>; > cell-index = <0>; > }; > + > + geniqup@ac0000 { > + compatible = "qcom,geni-se-qup"; > + reg = <0xac0000 0x6000>; > + clock-names = "m-ahb", "s-ahb"; > + clocks = <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, > + <&gcc 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 GCC_QUPV3_WRAP1_S1_CLK>; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&qup_uart2_default>; > + pinctrl-1 = <&qup_uart2_sleep>; > + interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; > + status = "disabled"; > + }; > + }; > }; > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/19/2018 3:08 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian > <kramasub@codeaurora.org> wrote: >> This bus driver supports the GENI based i2c hardware controller in the >> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable >> module supporting a wide range of serial interfaces including I2C. The >> driver supports FIFO mode and DMA mode of transfer and switches modes >> dynamically depending on the size of the transfer. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> >> --- >> drivers/i2c/busses/Kconfig | 13 + >> drivers/i2c/busses/Makefile | 1 + >> drivers/i2c/busses/i2c-qcom-geni.c | 648 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 662 insertions(+) > > Since I reviewed v3 of the i2c patch, can you make sure that you CC me > on future patches? Thanks! :) > > Overall this looks pretty nice to me now. A few minor comments... Sorry for not keeping you in CC for this patch. I will keep you in the CC going forward. > > >> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c) >> +{ >> + u32 val; >> + unsigned long time_left = ABORT_TIMEOUT; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&gi2c->lock, flags); >> + geni_i2c_err(gi2c, GENI_TIMEOUT); >> + gi2c->cur = NULL; >> + geni_se_abort_m_cmd(&gi2c->se); >> + spin_unlock_irqrestore(&gi2c->lock, flags); >> + do { >> + time_left = wait_for_completion_timeout(&gi2c->done, time_left); >> + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS); >> + } while (!(val & M_CMD_ABORT_EN) && time_left); >> + >> + if (!(val & M_CMD_ABORT_EN) && !time_left) > > Why do you need to check !time_left? Just "if (!(val & M_CMD_ABORT_EN))". I will remove here and in the other places you mentioned. > > >> + >> + gi2c->se.dev = &pdev->dev; >> + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent); >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(gi2c->se.base)) >> + return PTR_ERR(gi2c->se.base); >> + >> + gi2c->se.clk = devm_clk_get(&pdev->dev, "se"); >> + if (IS_ERR(gi2c->se.clk)) { >> + ret = PTR_ERR(gi2c->se.clk); >> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); >> + return ret; >> + } >> + >> + ret = device_property_read_u32(&pdev->dev, "clock-frequency", >> + &gi2c->clk_freq_out); >> + if (ret) { >> + /* All GENI I2C slaves support 400kHz. So default to 400kHz. */ > > Can you explain this comment? Are you saying that GENI only supports > talking to I2C devices (devices are also known as "slaves" and GENI > should be the I2C master) that talk 400 kHz I2C or better? Why do you > even have 100 kHz in your table above then? I don't think this is > what you meant... > > Perhaps you meant to say "All GENI I2C masters support at least 400 > kHz, so default ot 400 kHz" > > ...however, even if you changed the comment as I suggested I'm still > not a fan. As I said in my review of the prevous version: > >> I feel like it should default to 100KHz. i2c_parse_fw_timings() >> defaults to this and to me the wording "New drivers almost always >> should use the defaults" makes me feel this should be the defaults. > > I would also say that even if all GENI I2C masters support at least > 400 kHz that doesn't mean that all I2C devices on the bus support 400 > kHz. You could certainly imagine someone sticking something on this > bus that was super low cost and supported only 100 kHz I2C. We felt that all the current slaves that are connected to our masters support 400kHz. Hence we used that as a default frequency. I agree with you regarding 100kHz as default frequency. I will update that way in the next patch. > > > -Doug > Regards, Karthik.
Hi, On 3/19/2018 5:56 PM, Doug Anderson wrote: > Hi, > > On Mon, Mar 19, 2018 at 3:15 PM, Sagar Dharia <sdharia@codeaurora.org> wrote: >>>> + pinconf { >>>> + pins = "gpio55", "gpio56"; >>>> + drive-strength = <2>; >>>> + bias-disable; >>>> + }; >>>> + }; >>>> + >>>> + qup-i2c10-sleep { >>>> + pinconf { >>>> + pins = "gpio55", "gpio56"; >>>> + bias-pull-up; >>> >>> Are you sure that you want pullups enabled for sleep here? There are >>> external pulls on this line (as there are on many i2c busses) so doing >>> this will double-enable pulls. It probably won't hurt, but I'm >>> curious if there's some sort of reason here. >>> >> 1. We need the lines to remain high to avoid slaves sensing a false >> start-condition (this can happen if the SDA goes down before SCL). >> 2. Disclaimer: I'm not a HW expert, but we were told that >> tri-state/bias-disabled lines can draw more current. I will find out >> more about that. > > Agreed that they need to remain high, but you've got very strong > pullups external to the SoC. Those will keep it high. You don't need > the internal ones too. > > As extra evidence that the external pullups _must_ be present on your > board: you specify bias-disable in the active state. That can only > work if there are external pullups (or if there were some special > extra secret internal pullups that were part of geni). i2c is an > open-drain bus and thus there must be pullups on the bus in order to > communicate. > You are right, I followed up about the pull-up recommendation and that was for a GPIO where there was no external pull-up (GPIO was not used for I2C). It's safe to assume I2C will always have external pullup. We will change sleep-config of I2C GPIOs to no-pull. > >>>> + }; >>>> + }; >>>> }; >>>> }; >>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> index 59334d9..9ef056f 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >>>> @@ -209,6 +209,21 @@ >>>> pins = "gpio4", "gpio5"; >>>> }; >>>> }; >>>> + >>>> + qup_i2c10_default: qup-i2c10-default { >>>> + pinmux { >>>> + function = "qup10"; >>>> + pins = "gpio55", "gpio56"; >>>> + }; >>>> + }; >>>> + >>>> + qup_i2c10_sleep: qup-i2c10-sleep { >>>> + pinmux { >>>> + function = "gpio"; >>>> + pins = "gpio55", "gpio56"; >>>> + }; >>>> + }; >>>> + >>>> }; >>>> >>>> timer@17c90000 { >>>> @@ -309,6 +324,20 @@ >>>> interrupts = <GIC_SPI 354 IRQ_TYPE_LEVEL_HIGH>; >>>> status = "disabled"; >>>> }; >>>> + >>>> + i2c10: i2c@a88000 { >>> >>> Seems like it might be nice to add all the i2c busses into the main >>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it >>> would avoid churn later. >>> >>> ...if you're sure you want to add only one i2c controller, subject of >>> this patch should indicate that. >>> >> >> 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. > > ...but yeah, I'm asking for them all to be listed with status="disabled". > Sure, we will change the subject of this patch to indicate that we are adding 1 controller as of now. Later we will add all I2C controllers to dtsi as another patch since that will need pinctrl settings for GPIOs used by those instances and the wrappers devices needed by them. Thanks Sagar > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi Doug, On 3/19/2018 3:08 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian > <kramasub@codeaurora.org> wrote: >> This bus driver supports the GENI based i2c hardware controller in the >> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable >> module supporting a wide range of serial interfaces including I2C. The >> driver supports FIFO mode and DMA mode of transfer and switches modes >> dynamically depending on the size of the transfer. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > >> +/* >> + * Hardware uses the underlying formula to calculate time periods of >> + * SCL clock cycle. >> + * >> + * time of high period of SCL: t_high = (t_high_cnt * clk_div) / source_clock >> + * time of low period of SCL: t_low = (t_low_cnt * clk_div) / source_clock >> + * time of full period of SCL: t_cycle = (t_cycle_cnt * clk_div) / source_clock >> + * clk_freq_out = t / t_cycle >> + * source_clock = 19.2 MHz >> + */ >> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = { >> + {KHZ(100), 7, 10, 11, 26}, >> + {KHZ(400), 2, 5, 12, 24}, >> + {KHZ(1000), 1, 3, 9, 18}, >> +}; > > Thanks for the docs. ...though if these docs are indeed correct and > there's no extra magic fudge factor I'm still a bit baffled why the > frequency isn't out of spec for 100 kHz and 1 MHz. I know you said > hardware validated it, but are you really certain they validated 100 > kHz and 1 MHz? > >>>> 19200. / (1 * 18) > 1066.6666666666667 > >>>> 19200. / (2 * 24) > 400.0 > >>>> 19200. / (7 * 26) > 105.49450549450549 > > Specifically: either the docs you wrote above are wrong (and there's a > magic fudge factor that you didn't document) or your hardware guys are > wrong. See the I2C spec at > <https://www.nxp.com/docs/en/user-guide/UM10204.pdf>. Table 10. > ("Characteristics of the SDA and SCL bus lines for Standard, Fast, and > Fast-mode Plus I2C-bus devices") says fSCL Max is 100 kHz, 400 kHz, or > 1000 kHz. > > > I could believe that 99.9% of all devices that support 100 kHz also > support 105.5 kHz and that 99.9% of all devices that support 1000 kHz > also support 1066.7 kHz. However, it's still not in spec. If you > want to enable a turbo boost mode that runs 5% faster (really?) then I > suppose you could add that as an optional feature, but this shouldn't > be the default. > Yes, we will document that there is a fudge-factor (cycles needed to run the firmware per inputs from HW team). The actual frequencies seen for 100KHz and 1MHz configurations were close to 98KHz, and 960KHz respectively. t-high, and t-low were within specs for all frequencies. Thanks Sagar > >> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c) >> +{ >> + u32 val; >> + unsigned long time_left = ABORT_TIMEOUT; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&gi2c->lock, flags); >> + geni_i2c_err(gi2c, GENI_TIMEOUT); >> + gi2c->cur = NULL; >> + geni_se_abort_m_cmd(&gi2c->se); >> + spin_unlock_irqrestore(&gi2c->lock, flags); >> + do { >> + time_left = wait_for_completion_timeout(&gi2c->done, time_left); >> + val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS); >> + } while (!(val & M_CMD_ABORT_EN) && time_left); >> + >> + if (!(val & M_CMD_ABORT_EN) && !time_left) > > Why do you need to check !time_left? Just "if (!(val & M_CMD_ABORT_EN))". > > >> + dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n"); >> +} >> + >> +static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c) >> +{ >> + u32 val; >> + unsigned long time_left = RST_TIMEOUT; >> + >> + writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST); >> + do { >> + time_left = wait_for_completion_timeout(&gi2c->done, time_left); >> + val = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT); >> + } while (!(val & RX_RESET_DONE) && time_left); >> + >> + if (!(val & RX_RESET_DONE) && !time_left) > > Similar. Don't need "&& !time_left" > > >> + dev_err(gi2c->se.dev, "Timeout resetting RX_FSM\n"); >> +} >> + >> +static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) >> +{ >> + u32 val; >> + unsigned long time_left = RST_TIMEOUT; >> + >> + writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST); >> + do { >> + time_left = wait_for_completion_timeout(&gi2c->done, time_left); >> + val = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT); >> + } while (!(val & TX_RESET_DONE) && time_left); >> + >> + if (!(val & TX_RESET_DONE) && !time_left) > > Similar. Don't need "&& !time_left" > > >> +static int geni_i2c_probe(struct platform_device *pdev) >> +{ >> + struct geni_i2c_dev *gi2c; >> + struct resource *res; >> + u32 proto, tx_depth; >> + int ret; >> + >> + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL); >> + if (!gi2c) >> + return -ENOMEM; >> + >> + gi2c->se.dev = &pdev->dev; >> + gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent); >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + gi2c->se.base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(gi2c->se.base)) >> + return PTR_ERR(gi2c->se.base); >> + >> + gi2c->se.clk = devm_clk_get(&pdev->dev, "se"); >> + if (IS_ERR(gi2c->se.clk)) { >> + ret = PTR_ERR(gi2c->se.clk); >> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); >> + return ret; >> + } >> + >> + ret = device_property_read_u32(&pdev->dev, "clock-frequency", >> + &gi2c->clk_freq_out); >> + if (ret) { >> + /* All GENI I2C slaves support 400kHz. So default to 400kHz. */ > > Can you explain this comment? Are you saying that GENI only supports > talking to I2C devices (devices are also known as "slaves" and GENI > should be the I2C master) that talk 400 kHz I2C or better? Why do you > even have 100 kHz in your table above then? I don't think this is > what you meant... > > Perhaps you meant to say "All GENI I2C masters support at least 400 > kHz, so default ot 400 kHz" > > ...however, even if you changed the comment as I suggested I'm still > not a fan. As I said in my review of the prevous version: > >> I feel like it should default to 100KHz. i2c_parse_fw_timings() >> defaults to this and to me the wording "New drivers almost always >> should use the defaults" makes me feel this should be the defaults. > > I would also say that even if all GENI I2C masters support at least > 400 kHz that doesn't mean that all I2C devices on the bus support 400 > kHz. You could certainly imagine someone sticking something on this > bus that was super low cost and supported only 100 kHz I2C. > > > -Doug >
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 0000000..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. > >> +} >> + >> +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? Ok. > >> + 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? Ok. > >> + 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 = &port->uport; >> + if (oops_in_progress) >> + locked = spin_trylock_irqsave(&uport->lock, flags); >> + else >> + spin_lock_irqsave(&uport->lock, flags); >> + >> + /* Cancel the current write to log the fault */ >> + if (!locked) { >> + geni_se_cancel_m_cmd(&port->se); >> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, >> + M_CMD_CANCEL_EN, true)) { >> + geni_se_abort_m_cmd(&port->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(&uport->lock, flags); >> +} > > 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. > >> + >> +static void qcom_geni_serial_handle_tx(struct uart_port *uport) >> +{ >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + struct circ_buf *xmit = &uport->state->xmit; >> + size_t avail; >> + size_t remaining; >> + int i; >> + u32 status; >> + unsigned int chunk; >> + int tail; >> + >> + chunk = uart_circ_chars_pending(xmit); >> + status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS); >> + /* Both FIFO and framework buffer are drained */ >> + if (chunk == port->xmit_size && !status) { >> + port->xmit_size = 0; >> + uart_circ_clear(xmit); >> + qcom_geni_serial_stop_tx(uport); >> + goto out_write_wakeup; >> + } >> + chunk -= port->xmit_size; >> + >> + avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw; >> + tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1); >> + if (chunk > (UART_XMIT_SIZE - tail)) >> + chunk = UART_XMIT_SIZE - tail; >> + if (chunk > avail) >> + chunk = avail; > > chunk = min3(chunk, UART_XMIT_SIZE - tail, avail); Ok. > >> + >> + if (!chunk) >> + goto out_write_wakeup; >> + >> + qcom_geni_serial_setup_tx(uport, chunk); >> + >> + remaining = chunk; >> + for (i = 0; i < chunk; ) { >> + unsigned int tx_bytes; >> + unsigned int buf = 0; > > Make buf into a u8 array of 4? Ok. > >> + int c; >> + >> + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw); >> + for (c = 0; c < tx_bytes ; c++) >> + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE)); > > And then just put buf[c] = xmit->buf[tail + c] here? Ok. > >> + >> + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn); > > This also needs to be an iowrite32_rep(uport->membase, buf, 1) for > endian reasons. Ok. > >> + >> + i += tx_bytes; >> + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1); >> + uport->icount.tx += tx_bytes; >> + remaining -= tx_bytes; >> + } >> + qcom_geni_serial_poll_tx_done(uport); >> + port->xmit_size += chunk; >> +out_write_wakeup: >> + uart_write_wakeup(uport); >> +} >> + >> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev) >> +{ >> + unsigned int m_irq_status; >> + unsigned int s_irq_status; >> + struct uart_port *uport = dev; >> + unsigned long flags; >> + unsigned int m_irq_en; >> + bool drop_rx = false; >> + struct tty_port *tport = &uport->state->port; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + >> + if (uport->suspended) >> + return IRQ_HANDLED; > > Is it a spurious IRQ if this happens? Return IRQ_NONE instead? For the debug UART, this is a spurious IRQ. We can return IRQ_NONE. > >> + >> + spin_lock_irqsave(&uport->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. > >> + port->brk = true; >> + } >> + >> + if (s_irq_status & S_RX_FIFO_WATERMARK_EN || >> + s_irq_status & S_RX_FIFO_LAST_EN) >> + qcom_geni_serial_handle_rx(uport, drop_rx); >> + >> +out_unlock: >> + spin_unlock_irqrestore(&uport->lock, flags); >> + return IRQ_HANDLED; >> +} >> + >> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port) >> +{ >> + struct uart_port *uport; >> + >> + if (!port) >> + return -ENODEV; > > This happens? It should not happen. I will remove the check. > >> + >> + uport = &port->uport; >> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se); >> + port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se); >> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se); >> + uport->fifosize = >> + (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE; >> + return 0; >> +} >> + > [...] >> + >> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE >> +static int __init qcom_geni_console_setup(struct console *co, char *options) >> +{ >> + struct uart_port *uport; >> + struct qcom_geni_serial_port *port; >> + int baud; >> + int bits = 8; >> + int parity = 'n'; >> + int flow = 'n'; >> + >> + if (co->index >= GENI_UART_CONS_PORTS || co->index < 0) >> + return -ENXIO; >> + >> + port = get_port_from_line(co->index); >> + if (IS_ERR(port)) { >> + pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port)); > > Use %ld to avoid casting the error value? Or just don't print it at all > because it doesn't really help the end user. Currently get_port_from_line returns only one error code. So I will remove it from printing. > >> + return PTR_ERR(port); >> + } >> + >> + uport = &port->uport; >> + >> + if (unlikely(!uport->membase)) >> + return -ENXIO; >> + >> + if (geni_se_resources_on(&port->se)) { >> + dev_err(port->se.dev, "Error turning on resources\n"); >> + return -ENXIO; >> + } >> + >> + if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) { >> + geni_se_resources_off(&port->se); >> + return -ENXIO; >> + } >> + >> + if (!port->setup) { >> + port->tx_bytes_pw = 1; >> + port->rx_bytes_pw = RX_BYTES_PW; >> + qcom_geni_serial_stop_rx(uport); >> + qcom_geni_serial_port_setup(uport); >> + } >> + >> + if (options) >> + uart_parse_options(options, &baud, &parity, &bits, &flow); >> + >> + return uart_set_options(uport, co, baud, parity, bits, flow); >> +} > [..] >> + >> +static int qcom_geni_serial_probe(struct platform_device *pdev) >> +{ >> + int ret = 0; >> + int line = -1; >> + struct qcom_geni_serial_port *port; >> + struct uart_port *uport; >> + struct resource *res; >> + >> + if (pdev->dev.of_node) >> + line = of_alias_get_id(pdev->dev.of_node, "serial"); >> + else >> + line = pdev->id; > > Do we need to support the non-alias method? Sorry, I forgot to remove the else part. I remember we agreed to go the alias method. > >> + >> + if (line < 0 || line >= GENI_UART_CONS_PORTS) >> + return -ENXIO; >> + port = get_port_from_line(line); >> + if (IS_ERR(port)) { >> + ret = PTR_ERR(port); >> + dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret); >> + return ret; >> + } >> + >> + uport = &port->uport; >> + /* Don't allow 2 drivers to access the same port */ >> + if (uport->private_data) >> + return -ENODEV; >> + >> + uport->dev = &pdev->dev; >> + port->se.dev = &pdev->dev; >> + port->se.wrapper = dev_get_drvdata(pdev->dev.parent); >> + port->se.clk = devm_clk_get(&pdev->dev, "se"); >> + if (IS_ERR(port->se.clk)) { >> + ret = PTR_ERR(port->se.clk); >> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret); >> + return ret; >> + } >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (IS_ERR(res)) >> + return PTR_ERR(res); >> + uport->mapbase = res->start; >> + >> + port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS; >> + port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS; >> + port->tx_fifo_width = DEF_FIFO_WIDTH_BITS; >> + >> + uport->irq = platform_get_irq(pdev, 0); >> + if (uport->irq < 0) { >> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq); >> + return uport->irq; >> + } >> + >> + uport->private_data = &qcom_geni_console_driver; >> + platform_set_drvdata(pdev, port); >> + port->handle_rx = handle_rx_console; >> + port->setup = false; > > This would be set to false already? Yes, it should be set to false already. This is redundant. > >> + return uart_add_one_port(&qcom_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, &port->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 = &port->uport; >> + >> + uart_suspend_port(uport->private_data, uport); >> + return 0; >> +} >> + >> +static int __maybe_unused qcom_geni_serial_sys_resume_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 = &port->uport; >> + >> + if (console_suspend_enabled && uport->suspended) { >> + uart_resume_port(uport->private_data, uport); >> + disable_irq(uport->irq); > > I missed the enable_irq() part. Is this still necessary? Suspending the uart console port invokes the uart port shutdown operation. The shutdown operation disables and frees the concerned IRQ. Resuming the uart console port invokes the uart port startup operation which requests for the IRQ. The request_irq operation auto-enables the IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to an unbalanced IRQ enable warning. This disable_irq() helps with suppressing that warning. > >> + } >> + return 0; >> +} >> + >> +static int __init qcom_geni_serial_init(void) >> +{ >> + int ret; >> + >> + qcom_geni_console_port.uport.iotype = UPIO_MEM; >> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops; >> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF; >> + qcom_geni_console_port.uport.line = 0; > > Why can't this be done statically? It can be done statically. >> + >> + ret = console_register(&qcom_geni_console_driver); >> + if (ret) >> + return ret; >> + >> + ret = platform_driver_register(&qcom_geni_serial_platform_driver); >> + if (ret) >> + console_unregister(&qcom_geni_console_driver); >> + return ret; >> +} >> +module_init(qcom_geni_serial_init); Regards, Karthik.
On 3/20/2018 12:39 PM, Evan Green wrote: > Hi Karthik, > > On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian < > kramasub@codeaurora.org> wrote: > >> + >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, >> + int offset, int field, bool set) >> +{ >> + u32 reg; >> + struct qcom_geni_serial_port *port; >> + unsigned int baud; >> + unsigned int fifo_bits; >> + unsigned long timeout_us = 20000; >> + >> + /* Ensure polling is not re-ordered before the prior writes/reads > */ >> + mb(); > > These barriers sprinkled around everywhere are new. Did > you find that you needed them for something? In this case, the > readl_poll_timeout_atomic should already have a read barrier (nor do you > probably care about read reordering, right?) Perhaps this should > only be a mmiowb rather than a full barrier, because you really just want > to say "make sure all my old writes got out to hardware before spinning" These barriers have been there from v3. Regarding this barrier - since readl_poll_timeout_atomic has a read barrier, this one can be converted to just write barrier. > >> + >> + if (uport->private_data) { >> + port = to_dev_port(uport, uport); >> + baud = port->baud; >> + if (!baud) >> + baud = 115200; >> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width; >> + /* >> + * Total polling iterations based on FIFO worth of bytes > to be >> + * sent at current baud. Add a little fluff to the wait. >> + */ >> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; >> + } >> + >> + return !readl_poll_timeout_atomic(uport->membase + offset, reg, >> + (bool)(reg & field) == set, 10, timeout_us); >> +} > [...] >> + >> +static void qcom_geni_serial_start_tx(struct uart_port *uport) >> +{ >> + u32 irq_en; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + u32 status; >> + >> + if (port->xfer_mode == GENI_SE_FIFO) { >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); >> + if (status & M_GENI_CMD_ACTIVE) >> + return; >> + >> + if (!qcom_geni_serial_tx_empty(uport)) >> + return; >> + >> + /* >> + * Ensure writing to IRQ_EN & watermark registers are not >> + * re-ordered before checking the status of the Serial >> + * Engine and TX FIFO >> + */ >> + mb(); > > Here's another one. You should just be able to use a regular readl when > reading SE_GENI_STATUS and remove this barrier, since readl has an rmb > which orders your read of M_IRQ_EN below. I don't think you need to worry > about the writes below going above the read above, since there's logic in > between that needs the result of the read. Maybe that also saves you in the > read case, too. Either way, this barrier seems heavy handed. Write to the watermark register does not have any dependency on the reads. Hence it can be re-ordered. But writing to that register alone without enabling the watermark interrupt will not have any impact. From that perspective, using readl while checking the GENI_STATUS is sufficient to maintain the required order. I will put a comment regarding the use of readl so that the reason is not lost. > >> + >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); >> + irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN; >> + >> + writel_relaxed(port->tx_wm, uport->membase + >> + SE_GENI_TX_WATERMARK_REG); >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); >> + } >> +} >> + >> +static void qcom_geni_serial_stop_tx(struct uart_port *uport) >> +{ >> + u32 irq_en; >> + u32 status; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); >> + irq_en &= ~M_CMD_DONE_EN; >> + if (port->xfer_mode == GENI_SE_FIFO) { >> + irq_en &= ~M_TX_FIFO_WATERMARK_EN; >> + writel_relaxed(0, uport->membase + >> + SE_GENI_TX_WATERMARK_REG); >> + } >> + port->xmit_size = 0; >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); >> + /* Possible stop tx is called multiple times. */ >> + if (!(status & M_GENI_CMD_ACTIVE)) >> + return; >> + >> + /* >> + * Ensure cancel command write is not re-ordered before checking >> + * the status of the Primary Sequencer. >> + */ >> + mb(); > > I don't see how what's stated in your comment could happen, as that would > be a logic error. This barrier seems unneeded to me. Issuing a cancel command to the primary sequencer, makes the primary sequencer to go to inactive state. Even though they are 2 different registers, writing to one register impacts the other. From that perspective, we want to ensure that the order is maintained. Else if the cancel command goes through and then the GENI_STATUS is read, it might say that the primary sequencer is not active and the function might return prematurely. Same argument applies for the below mentioned cases. > >> + >> + geni_se_cancel_m_cmd(&port->se); >> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, >> + M_CMD_CANCEL_EN, true)) { >> + geni_se_abort_m_cmd(&port->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); >> +} >> + >> +static void qcom_geni_serial_start_rx(struct uart_port *uport) >> +{ >> + u32 irq_en; >> + u32 status; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); >> + if (status & S_GENI_CMD_ACTIVE) >> + qcom_geni_serial_stop_rx(uport); >> + >> + /* >> + * Ensure setup command write is not re-ordered before checking >> + * the status of the Secondary Sequencer. >> + */ >> + mb(); > > Also here, I think the reordering you're worried about would mean the CPU > is executing incorrectly. > >> + >> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0); >> + >> + if (port->xfer_mode == GENI_SE_FIFO) { >> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); >> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN; >> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); >> + >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); >> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN; >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); >> + } >> +} >> + >> +static void qcom_geni_serial_stop_rx(struct uart_port *uport) >> +{ >> + u32 irq_en; >> + u32 status; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> + u32 irq_clear = S_CMD_DONE_EN; >> + >> + if (port->xfer_mode == GENI_SE_FIFO) { >> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); >> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN); >> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); >> + >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); >> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); >> + } >> + >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); >> + /* Possible stop rx is called multiple times. */ >> + if (!(status & S_GENI_CMD_ACTIVE)) >> + return; >> + >> + /* >> + * Ensure cancel command write is not re-ordered before checking >> + * the status of the Secondary Sequencer. >> + */ >> + mb(); > > Same deal here. > >> + >> + geni_se_cancel_s_cmd(&port->se); >> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG, >> + S_GENI_CMD_CANCEL, false); >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); >> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR); >> + if (status & S_GENI_CMD_ACTIVE) >> + qcom_geni_serial_abort_rx(uport); >> +} >> + > > Sorry gmail decided to wrap some of the context lines. > -Evan > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Regards, Karthik.
On Tue, Mar 20, 2018 at 4:44 PM Karthik Ramasubramanian < kramasub@codeaurora.org> wrote: > On 3/20/2018 12:39 PM, Evan Green wrote: > > Hi Karthik, > > > > On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian < > > kramasub@codeaurora.org> wrote: > > > >> + > >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > >> + int offset, int field, bool set) > >> +{ > >> + u32 reg; > >> + struct qcom_geni_serial_port *port; > >> + unsigned int baud; > >> + unsigned int fifo_bits; > >> + unsigned long timeout_us = 20000; > >> + > >> + /* Ensure polling is not re-ordered before the prior writes/reads > > */ > >> + mb(); > > > > These barriers sprinkled around everywhere are new. Did > > you find that you needed them for something? In this case, the > > readl_poll_timeout_atomic should already have a read barrier (nor do you > > probably care about read reordering, right?) Perhaps this should > > only be a mmiowb rather than a full barrier, because you really just want > > to say "make sure all my old writes got out to hardware before spinning" > These barriers have been there from v3. Regarding this barrier - since > readl_poll_timeout_atomic has a read barrier, this one can be converted > to just write barrier. Thanks. I must have missed them in v3. Is that mmiowb that would go there, or wmb? I'm unsure. > > > >> + > >> + if (uport->private_data) { > >> + port = to_dev_port(uport, uport); > >> + baud = port->baud; > >> + if (!baud) > >> + baud = 115200; > >> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width; > >> + /* > >> + * Total polling iterations based on FIFO worth of bytes > > to be > >> + * sent at current baud. Add a little fluff to the wait. > >> + */ > >> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > >> + } > >> + > >> + return !readl_poll_timeout_atomic(uport->membase + offset, reg, > >> + (bool)(reg & field) == set, 10, timeout_us); > >> +} > > [...] > >> + > >> +static void qcom_geni_serial_start_tx(struct uart_port *uport) > >> +{ > >> + u32 irq_en; > >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > >> + u32 status; > >> + > >> + if (port->xfer_mode == GENI_SE_FIFO) { > >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > >> + if (status & M_GENI_CMD_ACTIVE) > >> + return; > >> + > >> + if (!qcom_geni_serial_tx_empty(uport)) > >> + return; > >> + > >> + /* > >> + * Ensure writing to IRQ_EN & watermark registers are not > >> + * re-ordered before checking the status of the Serial > >> + * Engine and TX FIFO > >> + */ > >> + mb(); > > > > Here's another one. You should just be able to use a regular readl when > > reading SE_GENI_STATUS and remove this barrier, since readl has an rmb > > which orders your read of M_IRQ_EN below. I don't think you need to worry > > about the writes below going above the read above, since there's logic in > > between that needs the result of the read. Maybe that also saves you in the > > read case, too. Either way, this barrier seems heavy handed. > Write to the watermark register does not have any dependency on the > reads. Hence it can be re-ordered. But writing to that register alone > without enabling the watermark interrupt will not have any impact. From > that perspective, using readl while checking the GENI_STATUS is > sufficient to maintain the required order. I will put a comment > regarding the use of readl so that the reason is not lost. Yes, I see what you mean, and without the branch I'd agree. My thinking though was that you have a branch between the read and writes. So to determine whether or not to even execute the writes, you must have successfully evaluated the read, since program flow depends on it. I will admit this is where my barrier knowledge gets fuzzy. Can speculative execution perform register writes? (ie if that branch was guessed wrong before the read actually completes, and then the writes happen before the read? That seems like it couldn't possibly happen, as it would result in weird spurious speculative writes to registers. Perhaps the mapping bits prevent this sort of thing...) If what I've said makes any sort of sense, and you still want to keep the barriers here and below, then I'll let it go. > > > >> + > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > >> + irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN; > >> + > >> + writel_relaxed(port->tx_wm, uport->membase + > >> + SE_GENI_TX_WATERMARK_REG); > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > >> + } > >> +} > >> + > >> +static void qcom_geni_serial_stop_tx(struct uart_port *uport) > >> +{ > >> + u32 irq_en; > >> + u32 status; > >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > >> + > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > >> + irq_en &= ~M_CMD_DONE_EN; > >> + if (port->xfer_mode == GENI_SE_FIFO) { > >> + irq_en &= ~M_TX_FIFO_WATERMARK_EN; > >> + writel_relaxed(0, uport->membase + > >> + SE_GENI_TX_WATERMARK_REG); > >> + } > >> + port->xmit_size = 0; > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > >> + /* Possible stop tx is called multiple times. */ > >> + if (!(status & M_GENI_CMD_ACTIVE)) > >> + return; > >> + > >> + /* > >> + * Ensure cancel command write is not re-ordered before checking > >> + * the status of the Primary Sequencer. > >> + */ > >> + mb(); > > > > I don't see how what's stated in your comment could happen, as that would > > be a logic error. This barrier seems unneeded to me. > Issuing a cancel command to the primary sequencer, makes the primary > sequencer to go to inactive state. Even though they are 2 different > registers, writing to one register impacts the other. From that > perspective, we want to ensure that the order is maintained. Else if the > cancel command goes through and then the GENI_STATUS is read, it might > say that the primary sequencer is not active and the function might > return prematurely. Same argument applies for the below mentioned cases. > > > >> + > >> + geni_se_cancel_m_cmd(&port->se); > >> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS, > >> + M_CMD_CANCEL_EN, true)) { > >> + geni_se_abort_m_cmd(&port->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); > >> +} > >> + > >> +static void qcom_geni_serial_start_rx(struct uart_port *uport) > >> +{ > >> + u32 irq_en; > >> + u32 status; > >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > >> + > >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > >> + if (status & S_GENI_CMD_ACTIVE) > >> + qcom_geni_serial_stop_rx(uport); > >> + > >> + /* > >> + * Ensure setup command write is not re-ordered before checking > >> + * the status of the Secondary Sequencer. > >> + */ > >> + mb(); > > > > Also here, I think the reordering you're worried about would mean the CPU > > is executing incorrectly. > > > >> + > >> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0); > >> + > >> + if (port->xfer_mode == GENI_SE_FIFO) { > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); > >> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN; > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); > >> + > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > >> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN; > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > >> + } > >> +} > >> + > >> +static void qcom_geni_serial_stop_rx(struct uart_port *uport) > >> +{ > >> + u32 irq_en; > >> + u32 status; > >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > >> + u32 irq_clear = S_CMD_DONE_EN; > >> + > >> + if (port->xfer_mode == GENI_SE_FIFO) { > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN); > >> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN); > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN); > >> + > >> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > >> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > >> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN); > >> + } > >> + > >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > >> + /* Possible stop rx is called multiple times. */ > >> + if (!(status & S_GENI_CMD_ACTIVE)) > >> + return; > >> + > >> + /* > >> + * Ensure cancel command write is not re-ordered before checking > >> + * the status of the Secondary Sequencer. > >> + */ > >> + mb(); > > > > Same deal here. > > > >> + > >> + geni_se_cancel_s_cmd(&port->se); > >> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG, > >> + S_GENI_CMD_CANCEL, false); > >> + status = readl_relaxed(uport->membase + SE_GENI_STATUS); > >> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR); > >> + if (status & S_GENI_CMD_ACTIVE) > >> + qcom_geni_serial_abort_rx(uport); > >> +} > >> + > > > > Sorry gmail decided to wrap some of the context lines. > > -Evan > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Regards, > Karthik. > -- > 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Mar 20, 2018 at 3:16 PM, Sagar Dharia <sdharia@codeaurora.org> wrote: >>>>> + pinconf { >>>>> + pins = "gpio55", "gpio56"; >>>>> + drive-strength = <2>; >>>>> + bias-disable; >>>>> + }; >>>>> + }; >>>>> + >>>>> + qup-i2c10-sleep { >>>>> + pinconf { >>>>> + pins = "gpio55", "gpio56"; >>>>> + bias-pull-up; >>>> >>>> Are you sure that you want pullups enabled for sleep here? There are >>>> external pulls on this line (as there are on many i2c busses) so doing >>>> this will double-enable pulls. It probably won't hurt, but I'm >>>> curious if there's some sort of reason here. >>>> >>> 1. We need the lines to remain high to avoid slaves sensing a false >>> start-condition (this can happen if the SDA goes down before SCL). >>> 2. Disclaimer: I'm not a HW expert, but we were told that >>> tri-state/bias-disabled lines can draw more current. I will find out >>> more about that. >> >> Agreed that they need to remain high, but you've got very strong >> pullups external to the SoC. Those will keep it high. You don't need >> the internal ones too. >> >> As extra evidence that the external pullups _must_ be present on your >> board: you specify bias-disable in the active state. That can only >> work if there are external pullups (or if there were some special >> extra secret internal pullups that were part of geni). i2c is an >> open-drain bus and thus there must be pullups on the bus in order to >> communicate. >> > > You are right, I followed up about the pull-up recommendation and that > was for a GPIO where there was no external pull-up (GPIO was not used > for I2C). It's safe to assume I2C will always have external pullup. It is even more safe to say that I2C will always have an external pullup on the SDM845-MTP. Remember that the pullup config is in the board device tree file, not the SoC one. So even if someone out there decides that the internal pull is somehow good enough for their own board and they don't stuff external ones, then it will be up to them to turn the pull up on in their own board file. > We > will change sleep-config of I2C GPIOs to no-pull. Even better IMHO: don't specify the bias in the sleep config. I don't believe it's possible for the sleep config to take effect without the default config since the default config applies at probe time. ...so you'll always get the default config applied at probe time and you don't need to touch the bias at sleep time. >>>>> + i2c10: i2c@a88000 { >>>> >>>> Seems like it might be nice to add all the i2c busses into the main >>>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it >>>> would avoid churn later. >>>> >>>> ...if you're sure you want to add only one i2c controller, subject of >>>> this patch should indicate that. >>>> >>> >>> 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. >> >> ...but yeah, I'm asking for them all to be listed with status="disabled". >> > > Sure, we will change the subject of this patch to indicate that we are > adding 1 controller as of now. Later we will add all I2C controllers to > dtsi as another patch since that will need pinctrl settings for GPIOs > used by those instances and the wrappers devices needed by them. Yeah, it's fine to just change the subject of this patch. It would be nice to add all the other controllers in sooner rather than later, but it doesn't have to be today. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/21/2018 1:09 AM, Stephen Boyd wrote: > 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 = &uart2; >> + }; >> + >> + chosen { >> + stdout-path = "serial0"; > > Also add :115200n8 ? > > >> + }; >> +}; >> + >> +&soc { > > I think the method is to put these inside soc node without using the > phandle reference. So indent everything once more. Some of this was discussed in the previous versions [1] and we arrived at a consensus to follow this way of doing it. Bjorn also said he was going to do a series to move all the existing dts files to follow similar convention so its all consistent. https://lkml.org/lkml/2018/2/6/676 > >> + geniqup@ac0000 { >> + serial@a84000 { >> + status = "okay"; >> + }; >> + }; >> + >> + pinctrl@3400000 { >> + 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 <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/clock/qcom,gcc-sdm845.h> >> >> / { >> interrupt-parent = <&intc>; >> @@ -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? Again, this was discussed in the previous versions, and we decided it makes sense to have the pinmux (default) which rarely changes across boards in the SoC file, and have boards specify the pinconf (electrical) properties. And get rid of all the soc-pins/board-pins/pmic-pins files. https://lkml.org/lkml/2018/2/6/693
Hi Doug On 3/20/2018 9:47 PM, Doug Anderson wrote: > Hi, > > On Tue, Mar 20, 2018 at 3:16 PM, Sagar Dharia <sdharia@codeaurora.org> wrote: >>>>>> + pinconf { >>>>>> + pins = "gpio55", "gpio56"; >>>>>> + drive-strength = <2>; >>>>>> + bias-disable; >>>>>> + }; >>>>>> + }; >>>>>> + >>>>>> + qup-i2c10-sleep { >>>>>> + pinconf { >>>>>> + pins = "gpio55", "gpio56"; >>>>>> + bias-pull-up; >>>>> >>>>> Are you sure that you want pullups enabled for sleep here? There are >>>>> external pulls on this line (as there are on many i2c busses) so doing >>>>> this will double-enable pulls. It probably won't hurt, but I'm >>>>> curious if there's some sort of reason here. >>>>> >>>> 1. We need the lines to remain high to avoid slaves sensing a false >>>> start-condition (this can happen if the SDA goes down before SCL). >>>> 2. Disclaimer: I'm not a HW expert, but we were told that >>>> tri-state/bias-disabled lines can draw more current. I will find out >>>> more about that. >>> >>> Agreed that they need to remain high, but you've got very strong >>> pullups external to the SoC. Those will keep it high. You don't need >>> the internal ones too. >>> >>> As extra evidence that the external pullups _must_ be present on your >>> board: you specify bias-disable in the active state. That can only >>> work if there are external pullups (or if there were some special >>> extra secret internal pullups that were part of geni). i2c is an >>> open-drain bus and thus there must be pullups on the bus in order to >>> communicate. >>> >> >> You are right, I followed up about the pull-up recommendation and that >> was for a GPIO where there was no external pull-up (GPIO was not used >> for I2C). It's safe to assume I2C will always have external pullup. > > It is even more safe to say that I2C will always have an external > pullup on the SDM845-MTP. Remember that the pullup config is in the > board device tree file, not the SoC one. So even if someone out there > decides that the internal pull is somehow good enough for their own > board and they don't stuff external ones, then it will be up to them > to turn the pull up on in their own board file. > > >> We >> will change sleep-config of I2C GPIOs to no-pull. > > Even better IMHO: don't specify the bias in the sleep config. I don't > believe it's possible for the sleep config to take effect without the > default config since the default config applies at probe time. ...so > you'll always get the default config applied at probe time and you > don't need to touch the bias at sleep time. Good point, we will remove the bias from the sleep config for i2c GPIOs. Thanks Sagar > > >>>>>> + i2c10: i2c@a88000 { >>>>> >>>>> Seems like it might be nice to add all the i2c busses into the main >>>>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it >>>>> would avoid churn later. >>>>> >>>>> ...if you're sure you want to add only one i2c controller, subject of >>>>> this patch should indicate that. >>>>> >>>> >>>> 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. >>> >>> ...but yeah, I'm asking for them all to be listed with status="disabled". >>> >> >> Sure, we will change the subject of this patch to indicate that we are >> adding 1 controller as of now. Later we will add all I2C controllers to >> dtsi as another patch since that will need pinctrl settings for GPIOs >> used by those instances and the wrappers devices needed by them. > > Yeah, it's fine to just change the subject of this patch. It would be > nice to add all the other controllers in sooner rather than later, but > it doesn't have to be today. > > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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 0000000..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(&uport->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(&qcom_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, &port->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 = &port->uport; > >> + > >> + uart_suspend_port(uport->private_data, uport); > >> + return 0; > >> +} > >> + > >> +static int __maybe_unused qcom_geni_serial_sys_resume_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 = &port->uport; > >> + > >> + if (console_suspend_enabled && uport->suspended) { > >> + uart_resume_port(uport->private_data, uport); > >> + disable_irq(uport->irq); > > > > I missed the enable_irq() part. Is this still necessary? > Suspending the uart console port invokes the uart port shutdown > operation. The shutdown operation disables and frees the concerned IRQ. > Resuming the uart console port invokes the uart port startup operation > which requests for the IRQ. The request_irq operation auto-enables the > IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to > an unbalanced IRQ enable warning. > > This disable_irq() helps with suppressing that warning. That's not obvious so we need a big comment here. I thought we would move this to the normal suspend/resume callbacks and skip the noirq variants. That would make this disable_irq() unnecessary then? BTW, free_irq() should disable the irq itself, so it looks odd to have a disable_irq() followed directly by a free_irq(). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/21/2018 11:20 AM, Stephen Boyd wrote: > 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 0000000..1442777 >>>> --- /dev/null >>>> +++ b/drivers/tty/serial/qcom_geni_serial.c >>>> @@ -0,0 +1,1158 @@ >>>> + > >>> >>> 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. Yes it is definitely in the plan. Since the serial driver got merged, I will address the pending comments in this patch series. I will upload the early console support in another patch. > >>> >>> >>>> + >>>> +static int __maybe_unused qcom_geni_serial_sys_resume_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 = &port->uport; >>>> + >>>> + if (console_suspend_enabled && uport->suspended) { >>>> + uart_resume_port(uport->private_data, uport); >>>> + disable_irq(uport->irq); >>> >>> I missed the enable_irq() part. Is this still necessary? >> Suspending the uart console port invokes the uart port shutdown >> operation. The shutdown operation disables and frees the concerned IRQ. >> Resuming the uart console port invokes the uart port startup operation >> which requests for the IRQ. The request_irq operation auto-enables the >> IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to >> an unbalanced IRQ enable warning. >> >> This disable_irq() helps with suppressing that warning. > > That's not obvious so we need a big comment here. > > I thought we would move this to the normal suspend/resume callbacks and > skip the noirq variants. That would make this disable_irq() unnecessary > then? For a non-console UART(eg. 4-wire UART), to reduce the wakeup latency _noirq variant is used so that the resources can be turned on as soon as possible. The idea is to use the same suspend and resume operation for both debug-uart and regular uart. Hence using the _noirq variant. I will add a detailed comment regarding why we are disabling the IRQ. > > BTW, free_irq() should disable the irq itself, so it looks odd to have a > disable_irq() followed directly by a free_irq(). I will update to just free_irq. > Regards, Karthik.
Generic Interface (GENI) firmware based Qualcomm Universal Peripheral (QUP) Wrapper is a next generation 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 (SE). The protocol supported by each interface is determined by the firmware loaded to the Serial Engine. This patch series introduces GENI SE Driver to manage the GENI based QUP Wrapper and the common aspects of all SEs inside the QUP Wrapper. This patch series also introduces the UART and I2C Controller drivers to drive the SEs that are programmed with the respective protocols. [v4] * Add SPI controller information in device tree binding * Add support for debug UART & I2C controllers in SDM845 device tree * Remove any unnecessary parenthesis & casting * Identify break character in UART line and pass it to the framework * Transmit data from fault handler reliably in debug UART * Map the register block when the UART port is requested * Move concise exported functions as macros or inlines in public header * Move the clock performance table from the wrapper to serial engines * Add a lock to synchronize between IRQ & error handling in I2C controller * Remove any compiler optimization hints like likely/unlikely * Update documentation to clarify tables and hardware blocks [v3] * Update the driver dependencies * Use the SPDX License Expression * Squash all the controller device tree bindings together * Use kernel doc format for documentation * Add additional documentation for packing configuration * Use clk_bulk_* API for related clocks * Remove driver references to pinctrl and their states * Replace magic numbers with appropriate macros * Update memory barrier usage and associated comments * Reduce interlacing of register reads/writes * Fix poll_get_char() operation in console UART driver under polling mode * Address other comments from Bjorn Andersson to improve code readability [v2] * Updated device tree bindings to describe the hardware * Updated SE DT node as child node of QUP Wrapper DT node * Moved common AHB clocks to QUP Wrapper DT node * Use the standard "clock-frequency" I2C property * Update compatible field in UART Controller to reflect hardware manual * Addressed other device tree binding specific comments from Rob Herring Karthikeyan Ramasubramanian (5): dt-bindings: soc: qcom: Add device tree binding for GENI SE soc: qcom: Add GENI based QUP Wrapper driver i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP arm64: dts: sdm845: Add I2C controller support Rajendra Nayak (1): arm64: dts: sdm845: Add serial console support .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 123 +++ arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 58 + arch/arm64/boot/dts/qcom/sdm845.dtsi | 67 ++ drivers/i2c/busses/Kconfig | 13 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-qcom-geni.c | 648 +++++++++++ drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/qcom-geni-se.c | 748 +++++++++++++ drivers/tty/serial/Kconfig | 15 + drivers/tty/serial/Makefile | 1 + drivers/tty/serial/qcom_geni_serial.c | 1158 ++++++++++++++++++++ include/linux/qcom-geni-se.h | 425 +++++++ 13 files changed, 3267 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c create mode 100644 drivers/soc/qcom/qcom-geni-se.c create mode 100644 drivers/tty/serial/qcom_geni_serial.c create mode 100644 include/linux/qcom-geni-se.h