From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 140C2C433C1 for ; Wed, 24 Mar 2021 12:26:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D554961A0D for ; Wed, 24 Mar 2021 12:26:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232536AbhCXM0Y (ORCPT ); Wed, 24 Mar 2021 08:26:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51310 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234499AbhCXM0P (ORCPT ); Wed, 24 Mar 2021 08:26:15 -0400 Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 995CDC061763 for ; Wed, 24 Mar 2021 05:26:14 -0700 (PDT) Received: by mail-lf1-x12b.google.com with SMTP id g8so24525269lfv.12 for ; Wed, 24 Mar 2021 05:26:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=iT8uCrZuHT3QZDUcvfmjktpdW+qD+ADtTYLgoO7f6Ys=; b=SA9oE0xSQoIp8P7EwHFaO2HG6zjZVZIrTuHkgJay0WRv1Sgj/fzID1oGr9XpeN+fll ZQuXG9ZDZQD7k1Wohn1Y9NdRXflBEaO/Tl5brOhnGSP0hSOQ7NYzB8mzRIQhox8/Ra4u g/zfXZUra67553qyjsJaHXNdvH/Hn6XpldGm7jh7IFzdivi2xEYnmCIr5u4H98uk/iKr qujBa6Yd5/DiUTTg2mkv2HnFuHLLClQRNpeohAUdcqCJqfJ29H03TJivBYrKRNz86g5Z 3LtsmgwuQQrRk5mtQt1BAQn/mQtvGuV6vMO50AX4wgIPQxemYHrLpRQkbV8W9CJmCVK5 xWcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=iT8uCrZuHT3QZDUcvfmjktpdW+qD+ADtTYLgoO7f6Ys=; b=T9t7tVY4GkMal+zSR0sIlOOJO0uyB2BDsM3TUE+379DLiWn63QHcjvlp02/R7Vmmc9 kHjoDwEHDpZTflLl9ViYoy4WyiYZKSNPXXMKIMcjm4tMUjWOfWA3hpC9bhBAnoI3v+om Q6oKIDiccnwmuf4e175NRGcowwzrZE//j+YtNr0LnQ40Dj7O3xhDUMVcH6cA2Kf2SlZF XnCOq7utXEG1hhiG0Mt0gHS3Giaa8UZnHP3zHJHNKBTjKSWzGz+cpQhGnMQzpqLMp18u HZJfZcrzXGHg6l/EmfiBzc9jCKMWJqEurlS3giG52/CcUDGq9EiLHM0bcZJ95N0BTXX1 smFg== X-Gm-Message-State: AOAM532bdOMUZuzk0CVRnWmGPfwDJJHo/VkfiBk9j/6Xcv7TR3YzYLd0 nms+22db4R7Q5fup4BdG8vM= X-Google-Smtp-Source: ABdhPJw00rBAnipnAPaQNq09hKkJKvsgbFXlrCzJlRI2YMuc6mmlrjDHNJhb0NcOkaBGuc6dkZS3WQ== X-Received: by 2002:ac2:5feb:: with SMTP id s11mr1771651lfg.558.1616588773073; Wed, 24 Mar 2021 05:26:13 -0700 (PDT) Received: from [192.168.2.145] (109-252-193-60.dynamic.spd-mgts.ru. [109.252.193.60]) by smtp.googlemail.com with ESMTPSA id y8sm318011ljk.9.2021.03.24.05.26.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 24 Mar 2021 05:26:12 -0700 (PDT) Subject: Re: [PATCH v3 2/3] i2c: add support for HiSilicon I2C controller To: Yicong Yang , wsa@kernel.org, linux-i2c@vger.kernel.org Cc: andriy.shevchenko@linux.intel.com, treding@nvidia.com, jarkko.nikula@linux.intel.com, rmk+kernel@armlinux.org.uk, song.bao.hua@hisilicon.com, john.garry@huawei.com, prime.zeng@huawei.com, linuxarm@huawei.com References: <1616411413-7177-1-git-send-email-yangyicong@hisilicon.com> <1616411413-7177-3-git-send-email-yangyicong@hisilicon.com> <7801d460-c1f4-5088-0ba0-47a07d187a2a@gmail.com> From: Dmitry Osipenko Message-ID: <816f8e2d-6e8d-c118-dfd2-af5348c30a48@gmail.com> Date: Wed, 24 Mar 2021 15:26:12 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-i2c@vger.kernel.org 24.03.2021 12:30, Yicong Yang пишет: ... >>> +static void hisi_i2c_enable_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel(mask, ctlr->iobase + HISI_I2C_INT_MASK); >> >> Why you don't use relaxed versions of readl/writel? Do you really need >> to insert memory barriers? >> > > this will not be used during the transfer process, so a relaxed version of readl/writel > will not have performance enhancement. > > the barriers are necessary as i want to make the operations in order to avoid potential > problems caused by reordering. The iomap is strongly ordered, hence register accesses are always ordered. The barrier ensures that CPU memory accesses are finished before h/w registers are touched. Looks like you don't need to worry about the memory barrier in the case of this driver. >>> +} >>> + >>> +static void hisi_i2c_disable_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel((~mask) & HISI_I2C_INT_ALL, ctlr->iobase + HISI_I2C_INT_MASK); >>> +} >>> + >>> +static void hisi_i2c_clear_int(struct hisi_i2c_controller *ctlr, u32 mask) >>> +{ >>> + writel(mask, ctlr->iobase + HISI_I2C_INT_CLR); >>> +} >>> + >>> +static void hisi_i2c_handle_errors(struct hisi_i2c_controller *ctlr) >>> +{ >>> + u32 int_err = ctlr->xfer_err, reg; >>> + >>> + if (int_err & HISI_I2C_INT_FIFO_ERR) { >>> + reg = readl(ctlr->iobase + HISI_I2C_FIFO_STATE); >>> + >>> + if (reg & HISI_I2C_FIFO_STATE_RX_RERR) >>> + dev_err(ctlr->dev, "rx fifo error read.\n"); >> >> The dot "." in the end of error messages is unnecessary. >> > > i'd like to keep this, as i think this is rather driver specific and not > violating any rules. The common kernel style is *not* to have the dot + some other messages in this driver already don't have it. Should be better if you could remove it. >>> +/* >>> + * Initialize the transfer information and start the I2C bus transfer. >>> + * We only configure the transfer and do some pre/post works here, and >>> + * wait for the transfer done. The major transfer process is performed >>> + * in the IRQ handler. >>> + */ >>> +static int hisi_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, >>> + int num) >>> +{ >>> + struct hisi_i2c_controller *ctlr = i2c_get_adapdata(adap); >>> + DECLARE_COMPLETION_ONSTACK(done); >>> + int ret = num; >>> + >>> + hisi_i2c_reset_xfer(ctlr); >>> + ctlr->completion = &done; >>> + ctlr->msg_num = num; >>> + ctlr->msgs = msgs; >>> + >>> + hisi_i2c_start_xfer(ctlr); >>> + >>> + if (!wait_for_completion_timeout(ctlr->completion, adap->timeout)) { >>> + hisi_i2c_disable_int(ctlr, HISI_I2C_INT_ALL); >> >> This doesn't save you from racing with the interrupt handler. It looks >> like you need to enable/disable IRQ around the completion, similarly to >> what NVIDIA Tegra I2C driver does. >> > > thanks for suggestion. > > the hardware between tegra and this one is a little different as we don't provide > a way to reinit the controller. so {synchronize,disable}_irq() after mask > the interrupt here will avoid the race. The disable/enable will be ideal, but synchronize should be good enough as well.