Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp1401109pxj; Fri, 21 May 2021 13:20:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxhAJD/919phanSO8O836VHRJTxn6uJFlG3abVz6t5jD1odMudZEjdIbfTYAcg02wDj8XED X-Received: by 2002:a02:630e:: with SMTP id j14mr6849191jac.115.1621628447514; Fri, 21 May 2021 13:20:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1621628447; cv=none; d=google.com; s=arc-20160816; b=Q3mTYVmKWbALqVV5za8f5IY/27uVF/HSk4pdkBHs1ARMqSf8L+nhD4Hadwu3hpHsYv UCC5GDnVa7ZHNkcNIFQKFcI6tqhDWXCIb3rJdrZeipo/qaPWZdmxQ4ksvYERWqTF23j0 Pk7qFUzVk8ZlsFshy2hwxltF895wwtK/9aRKsF+Mul7kIxZELiimIdUCFhG37qZgJPcU PWTtJMp9O80ek09Uzqvo4JRRDuK3lyY6NnPtDvQDb383kqMe2R/MMYb2LvzXbuqwEcfd Bmq5eUeb8x/O3iOr20rwB7ZecR7zPCbBbDfg3zGEC4NTRf+xaAmCRLApvclJB5mVBlxL oeBg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=+DCiOMUmNCEbgSGkeFS7cL87hlbtrGvRjWxrSQX4kzE=; b=WLQ/ZWa8iI7B788Asw+1sieqsIA2Dsn/BiY16gB/v0OtOKTtahAhnsPcILc1MVTSzJ npyTRPMSvomrekIRyWCzpPBcDvM25OmjnHmSBl+/1OASWjeycCLDxmrvMnm3TMzWpZL6 UsIVXV6HUGuIuBxMX4I7DVA4PuC+2Jr+9tCHBqG+QGc8xad93whXySY3QRkLR0MrUJ0M 4TAtXjjAbqEx5J/1Q7hDKezrfhPY64e6/jnRGY5UKcmMD9BSve4K4J6mcph0bwzXGDuw LRTL8CzqW22P/QepBbbOx/FGWR7K1AxqXvyEXtJho4v0xuP0ZEVccD27e955h6I3s07j 52tQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d25si5968632jam.64.2021.05.21.13.20.35; Fri, 21 May 2021 13:20:47 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237211AbhEUPD7 (ORCPT + 99 others); Fri, 21 May 2021 11:03:59 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:47391 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237218AbhEUPD4 (ORCPT ); Fri, 21 May 2021 11:03:56 -0400 Received: from mail-vk1-f199.google.com ([209.85.221.199]) by youngberry.canonical.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1lk6fX-0004ur-Nn for linux-kernel@vger.kernel.org; Fri, 21 May 2021 15:02:31 +0000 Received: by mail-vk1-f199.google.com with SMTP id e7-20020a1f1e070000b02901ffbfb6bcd1so1864169vke.22 for ; Fri, 21 May 2021 08:02:31 -0700 (PDT) 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=+DCiOMUmNCEbgSGkeFS7cL87hlbtrGvRjWxrSQX4kzE=; b=rIxva0txm/NT9qaTGlmKUbW9FKWkNBMJovCb4v0+HBBAiB1cotX77ZpW7BYmhhCcdZ dgOdefa3fJ7Tr9R9YbGQKU+J2fc3Oglqg9SGVI8qZpH/U5lp0Xv7qhQxp0496HujIPyt H20OT5GNj7BUbItZ6g233fWyNIYETZqU01TGFtIhK/s7E9+5DvjD4wVZ8b6v+qcSao4U aM72EHoHz8R1b6TLVJ6y1VOAG1AbspUIMm7Bj/T9VIBDj29R+T0WvzhGVKL5CiK/fBEo HCjMhqm2CealKtw87ZYhTEu6mBqhK+DhiKgpuV+7TaJXZK98aUAAu6E30nayTyRrcpCc HYhg== X-Gm-Message-State: AOAM532I5IWx3SCIxZAPDRacQ4GIqoUxhE7hi4aUh9CPCOOCU/e6QG3J F4P8prweKzSmWT9oz0lEfssNuilBdQxakqk8K5GwbRvJSuXm8Vy2fQpo6UkkR1Yz/KtB33n3Roj 6GofagXwnM/20FNLBbNWGxUOuvZwKuG8Mi1BaJ6A45w== X-Received: by 2002:a67:2c85:: with SMTP id s127mr10899683vss.7.1621609350806; Fri, 21 May 2021 08:02:30 -0700 (PDT) X-Received: by 2002:a67:2c85:: with SMTP id s127mr10899652vss.7.1621609350576; Fri, 21 May 2021 08:02:30 -0700 (PDT) Received: from [192.168.1.4] ([45.237.48.6]) by smtp.gmail.com with ESMTPSA id f65sm960175vke.43.2021.05.21.08.02.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 May 2021 08:02:30 -0700 (PDT) Subject: Re: [linux-nfc] Re: [PATCH 2/2] nfc: s3fwrn5: i2c: Enable optional clock from device tree To: Bongsu Jeon Cc: Stephan Gerhold , Bongsu Jeon , "David S. Miller" , Jakub Kicinski , Rob Herring , linux-nfc@lists.01.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht References: <20210518133935.571298-1-stephan@gerhold.net> <20210518133935.571298-2-stephan@gerhold.net> <8b14159f-dca9-a213-031f-83ab2b3840a4@canonical.com> <10b3a50e-877c-d5b1-3e35-e5dff4ef53d8@canonical.com> From: Krzysztof Kozlowski Message-ID: <20c1367d-a8c7-603e-2b07-ec3ddcd89a38@canonical.com> Date: Fri, 21 May 2021 11:02:26 -0400 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: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/05/2021 07:40, Bongsu Jeon wrote: > On Thu, May 20, 2021 at 12:58 AM Krzysztof Kozlowski > wrote: >> >> On 19/05/2021 04:07, Stephan Gerhold wrote: >>> On Tue, May 18, 2021 at 11:25:55AM -0400, Krzysztof Kozlowski wrote: >>>> On 18/05/2021 11:00, Stephan Gerhold wrote: >>>>> On Tue, May 18, 2021 at 10:30:43AM -0400, Krzysztof Kozlowski wrote: >>>>>> On 18/05/2021 09:39, Stephan Gerhold wrote: >>>>>>> s3fwrn5 has a NFC_CLK_REQ output GPIO, which is asserted whenever >>>>>>> the clock is needed for the current operation. This GPIO can be either >>>>>>> connected directly to the clock provider, or must be monitored by >>>>>>> this driver. >>>>>>> >>>>>>> As an example for the first case, on many Qualcomm devices the >>>>>>> NFC clock is provided by the main PMIC. The clock can be either >>>>>>> permanently enabled (clocks = <&rpmcc RPM_SMD_BB_CLK2>) or enabled >>>>>>> only when requested through a special input pin on the PMIC >>>>>>> (clocks = <&rpmcc RPM_SMD_BB_CLK2_PIN>). >>>>>>> >>>>>>> On the Samsung Galaxy A3/A5 (2015, Qualcomm MSM8916) this mechanism >>>>>>> is used with S3FWRN5's NFC_CLK_REQ output GPIO to enable the clock >>>>>>> only when necessary. However, to make that work the s3fwrn5 driver >>>>>>> must keep the RPM_SMD_BB_CLK2_PIN clock enabled. >>>>>> >>>>>> This contradicts the code. You wrote that pin should be kept enabled >>>>>> (somehow... by driver? by it's firmware?) but your code requests the >>>>>> clock from provider. >>>>>> >>>>> >>>>> Yeah, I see how that's a bit confusing. Let me try to explain it a bit >>>>> better. So the Samsung Galaxy A5 (2015) has a "S3FWRN5XS1-YF30", some >>>>> variant of S3FWRN5 I guess. That S3FWRN5 has a "XI" and "XO" pin in the >>>>> schematics. "XO" seems to be floating, but "XI" goes to "BB_CLK2" >>>>> on PM8916 (the main PMIC). >>>>> >>>>> Then, there is "GPIO2/NFC_CLK_REQ" on the S3FWRN5. This goes to >>>>> GPIO_2_NFC_CLK_REQ on PM8916. (Note: I'm talking about two different >>>>> GPIO2 here, one on S3FWRN5 and one on PM8916, they just happen to have >>>>> the same number...) >>>>> >>>>> So in other words, S3FWRN5 gets some clock from BB_CLK2 on PM8916, >>>>> and can tell PM8916 that it needs the clock via GPIO2/NFC_CLK_REQ. >>>>> >>>>> Now the confusing part is that the rpmcc/clk-smd-rpm driver has two >>>>> clocks that represent BB_CLK2 (see include/dt-bindings/clock/qcom,rpmcc.h): >>>>> >>>>> - RPM_SMD_BB_CLK2 >>>>> - RPM_SMD_BB_CLK2_PIN >>>>> >>>>> (There are also *_CLK2_A variants but they are even more confusing >>>>> and not needed here...) >>>>> >>>>> Those end up in different register settings in PM8916. There is one bit >>>>> to permanently enable BB_CLK2 (= RPM_SMD_BB_CLK2), and one bit to enable >>>>> BB_CLK2 based on the status of GPIO_2_NFC_CLK_REQ on PM8916 >>>>> (= RPM_SMD_BB_CLK2_PIN). >>>>> >>>>> So there is indeed some kind of "AND" inside PM8916 (the register bit >>>>> and "NFC_CLK_REQ" input pin). To make that "AND" work I need to make >>>>> some driver (here: the s3fwrn5 driver) enable the clock so the register >>>>> bit in PM8916 gets set. >>>> >>>> Thanks for the explanation, it sounds good. The GPIO2 (or how you call >>>> it NFC_CLK_REQ) on S3FWRN5 looks like non-configurable from Linux point >>>> of view. Probably the device firmware plays with it always or at least >>>> handles it in an unknown way for us. >>>> >>> >>> FWIW, I was looking at some more s3fwrn5 code yesterday and came >>> across this (in s3fwrn5_nci_rf_configure()): >>> >>> /* Set default clock configuration for external crystal */ >>> fw_cfg.clk_type = 0x01; >>> fw_cfg.clk_speed = 0xff; >>> fw_cfg.clk_req = 0xff; >>> ret = nci_prop_cmd(info->ndev, NCI_PROP_FW_CFG, >>> sizeof(fw_cfg), (__u8 *)&fw_cfg); >>> if (ret < 0) >>> goto out; >>> >>> It does look quite suspiciously like that configures how s3fwrn5 expects >>> the clock and possibly (fw_cfg.clk_req?) how GPIO2 behaves. But it's not >>> particularly useful without some documentation for the magic numbers. >> >> Right, without documentation of FW protocol there is not much we can >> deduct here. There is no proof even that the comment matches actual code. >> >> Dear Bongsu, >> Maybe you could share some details about clock selection? > > These configuration values depend on the HW circuit for NFC. > > There are two types of fw_cfg.clk_type for N5. > 0x01 : external XTAL ( don't need to control the clock because XTAL > always supplies > the NFC clock automatically.) > 0x00 : PLL clock (need to control clock. ) > > There are three types of fw_cfg.clk_speed for N5. > 0xFF : for external XTAL > 0x00 : 24M for PLL. > 0x01 : 19.12M for PLL. > > There are two types of fw_cfg.clk_req for N5. > 0xFF: NFC firmware controls CLK Req when NFC needs the external clock. > 0xF0: NFC firmware doesn't control CLK Req. > >> >>> >>> Personally, I just skip all firmware/RF configuration (which works thanks >>> to commit 4fb7b98c7be3 ("nfc: s3fwrn5: skip the NFC bootloader mode")). >>> That way, S3FWRN5 just continues using the proper configuration >>> that was loaded by the vendor drivers at some point. :) >> >> But isn't that configuration lost after power off? >> > > If you skip all firmware/RF configuration, you can use the preserved > firmware and > RF configuration on the chip. Thank you for sharing these details. Much appreciated! Best regards, Krzysztof