| Message ID | 1521836461-6515-1-git-send-email-kramasub@codeaurora.org |
|---|---|
| Headers | show |
| Series | Introduce GENI SE Controller Driver | expand |
Hi, On Fri, Mar 23, 2018 at 1:20 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 | 650 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 664 insertions(+) [...] > +/* > + * Hardware uses the underlying formula to calculate time periods of > + * SCL clock cycle. Firmware uses some additional cycles excluded from the > + * below formula and it is confirmed that the time periods are within > + * specification limits. I was hoping for more than just "oh, and there's a fudge factor", but I guess this is the best I'm going to get? > +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) { > + /* Clock frequency not specified, so default to 100kHz. */ > + dev_info(&pdev->dev, > + "Bus frequency not specified, default to 100kHz.\n"); If you happen to spin again, can you remove the comment since it's obvious from the string in the print? It looks a lot like this code: /* Print hello, world */ printf("hello, world\n"); In any case, that's a pretty minor nit, so I'll add: Reviewed-by: Douglas Anderson <dianders@chromium.org> ...assuming that the bindings and "geni" code get Acked / landed somewhere. Ideally let's not land this before the geni code lands since if the geni API changes for some reason it'll cause us grief. -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, On Fri, Mar 23, 2018 at 1:21 PM, Karthikeyan Ramasubramanian <kramasub@codeaurora.org> wrote: > + i2c10: i2c@a88000 { > + 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>; > + clock-frequency = <400000>; Please move the clock-frequency to the board file. Not all devices on all boards will support 400 kHz, so we should be at 100 kHz by default and boards should explicitly say that they support 400 kHz. Other than that things look good to me and you can add my Reviewed-by tag. -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 On 3/23/2018 5:34 PM, Doug Anderson wrote: > Hi, > > On Fri, Mar 23, 2018 at 1:20 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 | 650 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 664 insertions(+) > > [...] > >> +/* >> + * Hardware uses the underlying formula to calculate time periods of >> + * SCL clock cycle. Firmware uses some additional cycles excluded from the >> + * below formula and it is confirmed that the time periods are within >> + * specification limits. > > I was hoping for more than just "oh, and there's a fudge factor", but > I guess this is the best I'm going to get? > According to our HW/FW team: "We use over sampling in our FW and we use 1-2 NOPE extra in some cases. this is why we can’t give a exact formula." > >> +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) { >> + /* Clock frequency not specified, so default to 100kHz. */ >> + dev_info(&pdev->dev, >> + "Bus frequency not specified, default to 100kHz.\n"); > > If you happen to spin again, can you remove the comment since it's > obvious from the string in the print? It looks a lot like this code: > > /* Print hello, world */ > printf("hello, world\n"); > > > In any case, that's a pretty minor nit, so I'll add: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > ...assuming that the bindings and "geni" code get Acked / landed > somewhere. Ideally let's not land this before the geni code lands > since if the geni API changes for some reason it'll cause us grief. > Sure, thanks a lot for reviewing the patches! -Sagar > > -Doug >
Wolfram, On Fri, Mar 23, 2018 at 4:34 PM, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Fri, Mar 23, 2018 at 1:20 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 | 650 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 664 insertions(+) > > [...] > >> +/* >> + * Hardware uses the underlying formula to calculate time periods of >> + * SCL clock cycle. Firmware uses some additional cycles excluded from the >> + * below formula and it is confirmed that the time periods are within >> + * specification limits. > > I was hoping for more than just "oh, and there's a fudge factor", but > I guess this is the best I'm going to get? > > >> +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) { >> + /* Clock frequency not specified, so default to 100kHz. */ >> + dev_info(&pdev->dev, >> + "Bus frequency not specified, default to 100kHz.\n"); > > If you happen to spin again, can you remove the comment since it's > obvious from the string in the print? It looks a lot like this code: > > /* Print hello, world */ > printf("hello, world\n"); > > > In any case, that's a pretty minor nit, so I'll add: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > ...assuming that the bindings and "geni" code get Acked / landed > somewhere. Ideally let's not land this before the geni code lands > since if the geni API changes for some reason it'll cause us grief. The bindings and "geni" code have landed in Andy's tree, so whenever you get a chance it would be super if you could land this i2c driver (assuming it looks good to you). I know at least a few people have been poking at this and it seems to work for basic transfers. Thanks! -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, On Fri, Mar 23, 2018 at 02:20:59PM -0600, Karthikeyan Ramasubramanian 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> Is one of these people interested in maintaining this driver? Then, an entry for MAINTAINERS would be needed, too. (Same goes for drivers/soc/qcom/ IMHO, but this is not my realm, so just saying) > +static const struct geni_i2c_err_log gi2c_log[] = { > + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"}, > + [NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its power/reset-ln"}, > + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"}, > + [BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"}, > + [ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"}, > + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"}, > + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"}, > + [GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state machine"}, > + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"}, > + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"}, > +}; Please check Documentation/i2c/fault-codes for better -ERRNO values, especially for NACK and ARB_LOST. Rest looks good from a glimpse. Thanks, Wolfram
On 5/21/2018 2:49 PM, Wolfram Sang wrote: > Hi, > > On Fri, Mar 23, 2018 at 02:20:59PM -0600, Karthikeyan Ramasubramanian 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> > > Is one of these people interested in maintaining this driver? Then, an > entry for MAINTAINERS would be needed, too. (Same goes for > drivers/soc/qcom/ IMHO, but this is not my realm, so just saying) One of us will maintain this driver and I will update the MAINTAINERS appropriately. > >> +static const struct geni_i2c_err_log gi2c_log[] = { >> + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"}, >> + [NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its power/reset-ln"}, >> + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"}, >> + [BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"}, >> + [ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"}, >> + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"}, >> + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"}, >> + [GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state machine"}, >> + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"}, >> + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"}, >> +}; > > Please check Documentation/i2c/fault-codes for better -ERRNO values, > especially for NACK and ARB_LOST. I will check the fault-codes and fix the error codes here. > > Rest looks good from a glimpse. > > Thanks, > > Wolfram > 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. [v5] * Remove Linux specific property from the device tree binding * Clarify I2C SCL time period documentation * Remove redundant checks in I2C controller driver during timeout * Use 100kHz as the default clock frequency in the I2C controller driver * Disable Wrapper controller by default in the SDM845 device tree and enable it explicitly for SDM845 MTP * Specify I2C clock frequency in the SDM845 device tree * Remove bias configuration for I2C pins under sleep state in device tree * Drop the serial driver from the patch series since it is merged * Specify the UART port options in the SDM845 device tree [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 (4): 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 arm64: dts: sdm845: Add support for an instance of I2C controller Rajendra Nayak (1): arm64: dts: sdm845: Add serial console support .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 119 ++++ arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 59 ++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 68 ++ drivers/i2c/busses/Kconfig | 13 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-qcom-geni.c | 650 ++++++++++++++++++ drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/qcom-geni-se.c | 748 +++++++++++++++++++++ include/linux/qcom-geni-se.h | 425 ++++++++++++ 10 files changed, 2093 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 include/linux/qcom-geni-se.h