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.