Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Doug Anderson <dianders@chromium.org>]

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