From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on archive.lwn.net X-Spam-Level: X-Spam-Status: No, score=-4.7 required=5.0 tests=DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,T_DKIM_INVALID, T_RP_MATCHES_RCVD autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by archive.lwn.net (Postfix) with ESMTP id DFA6A7DD31 for ; Tue, 20 Mar 2018 22:23:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751423AbeCTWXj (ORCPT ); Tue, 20 Mar 2018 18:23:39 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:45742 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751334AbeCTWXh (ORCPT ); Tue, 20 Mar 2018 18:23:37 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id A291660C54; Tue, 20 Mar 2018 22:23:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1521584616; bh=ORBCtnsnCfdF7DVC0tl1Ilwt6jCRf2FJY8sZFZsQbuM=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=JDf1Z311Q4JZJEhg3OTQNTbElAJA5SaqshnRzjziF8DORJyzwz3R+9I8Elw+LrRZ5 jXcFggzAQV9jyNtNTye+2qzAt49U3DVqCCIqTKzDaGco7fThBDGfiZ8CuPlT+HbgCT GaYe7qAL0hiswVisj1UflBsV6fUf7dYb1ZHg3XP8= Received: from [10.226.58.86] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: sdharia@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id D3C116081C; Tue, 20 Mar 2018 22:23:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1521584605; bh=ORBCtnsnCfdF7DVC0tl1Ilwt6jCRf2FJY8sZFZsQbuM=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=o42hPFbaYQ2BU1CQfXTm5ayuqstQOenFroo+raHVDtZoabxmb48rSP9rOQtiZnK/3 Qi9vEuAUYUDoC3NXBTucbbQP7hlmy9jXIh5D2/rvqPiEHWj9aoB8VKLexNOwXvzuHd HHS5jyTHSF2GWUEOzf99DDeuszcy0xyOMquyf54w= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org D3C116081C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sdharia@codeaurora.org Subject: Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller To: Doug Anderson , Karthikeyan Ramasubramanian Cc: Jonathan Corbet , Andy Gross , David Brown , Rob Herring , Mark Rutland , Wolfram Sang , Greg Kroah-Hartman , linux-doc@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, Jiri Slaby , evgreen@chromium.org, acourbot@chromium.org, swboyd@chromium.org, Girish Mahadevan References: <1521071931-9294-1-git-send-email-kramasub@codeaurora.org> <1521071931-9294-4-git-send-email-kramasub@codeaurora.org> From: Sagar Dharia Message-ID: Date: Tue, 20 Mar 2018 16:23:23 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org Hi Doug, On 3/19/2018 3:08 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, 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 >> Signed-off-by: Sagar Dharia >> Signed-off-by: Girish Mahadevan > >> +/* >> + * 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 > . 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 > -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller References: <1521071931-9294-1-git-send-email-kramasub@codeaurora.org> <1521071931-9294-4-git-send-email-kramasub@codeaurora.org> From: Sagar Dharia Message-ID: Date: Tue, 20 Mar 2018 16:23:23 -0600 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: Doug Anderson , Karthikeyan Ramasubramanian Cc: Jonathan Corbet , Andy Gross , David Brown , Rob Herring , Mark Rutland , Wolfram Sang , Greg Kroah-Hartman , linux-doc@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-serial@vger.kernel.org, Jiri Slaby , evgreen@chromium.org, acourbot@chromium.org, swboyd@chromium.org, Girish Mahadevan List-ID: Hi Doug, On 3/19/2018 3:08 PM, Doug Anderson wrote: > Hi, > > On Wed, Mar 14, 2018 at 4:58 PM, 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 >> Signed-off-by: Sagar Dharia >> Signed-off-by: Girish Mahadevan > >> +/* >> + * 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 > . 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 > -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project