| Message ID | 20210518133935.571298-2-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] dt-bindings: net: nfc: s3fwrn5: Add optional clock | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi, Thanks for the patch. 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. > > This commit adds support for this by requesting an optional clock Don't write "This commit". https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L89 > and keeping it permanently enabled. Note that the actual (physical) > clock won't be permanently enabled since this will depend on the > output of NFC_CLK_REQ from S3FWRN5. What pin is that "NFC_CLK_REQ"? I cannot find such name. Is it GPIO2? What clock are you talking here? The one going to the modem part? I also don't see here how this clock is going to be automatically on-off... driver does not perform such. Unless you speak about your particular HW configuration where the GPIO is somehow connected with AND (but then it is not relevant to the code). > > In the future (when needed by some other device) this could be > extended to work for the second case (monitoring the NFC_CLK_REQ > GPIO and enabling the clock from the kernel when needed). > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > drivers/nfc/s3fwrn5/i2c.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > Best regards, Krzysztof
Hi, 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. > > > > This commit adds support for this by requesting an optional clock > > Don't write "This commit". > https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L89 > OK, will fix this in v2 (I guess there will be a v2 to clarify things at least...) > > and keeping it permanently enabled. Note that the actual (physical) > > clock won't be permanently enabled since this will depend on the > > output of NFC_CLK_REQ from S3FWRN5. > > What pin is that "NFC_CLK_REQ"? I cannot find such name. Is it GPIO2? > What clock are you talking here? The one going to the modem part? > It's indeed GPIO2 on S3FWRN5, but that's pretty much all I can say since I can't seem to find any datasheet for S3FWRN5. :( I don't know what it is used for. As I mentioned above, BB_CLK2 goes to "XI" on S3FWRN5. > I also don't see here how this clock is going to be automatically > on-off... driver does not perform such. Unless you speak about your > particular HW configuration where the GPIO is somehow connected with AND > (but then it is not relevant to the code). > I hope I covered this above already and it's a bit clearer now. Sorry for the confusion! Thanks! Stephan
On 18/05/2021 11:00, Stephan Gerhold wrote: > Hi, > > 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. In such case there is no point to do anything more with the provided clock than what you are doing - enable it when device is on, disable when off. I think it is enough to rephrase the msg: 1. Add at beginning that device has one clock input (XI pin). The clock input was so far ignored (assumed to be routed to some always-on oscillator). 2. The device should enable the clock when running. 3. Add all of your paragraph about detailed logic on GPIO. Since the GPIO is non-controllable, it actually does not matter that much for the driver, so you can add it for relevance, but not as main point of the patch. > >>> >>> This commit adds support for this by requesting an optional clock >> >> Don't write "This commit". >> https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L89 >> > > OK, will fix this in v2 (I guess there will be a v2 to clarify things > at least...) > >>> and keeping it permanently enabled. Note that the actual (physical) >>> clock won't be permanently enabled since this will depend on the >>> output of NFC_CLK_REQ from S3FWRN5. >> >> What pin is that "NFC_CLK_REQ"? I cannot find such name. Is it GPIO2? >> What clock are you talking here? The one going to the modem part? >> > > It's indeed GPIO2 on S3FWRN5, but that's pretty much all I can say since > I can't seem to find any datasheet for S3FWRN5. :( I don't know what it > is used for. As I mentioned above, BB_CLK2 goes to "XI" on S3FWRN5. > >> I also don't see here how this clock is going to be automatically >> on-off... driver does not perform such. Unless you speak about your >> particular HW configuration where the GPIO is somehow connected with AND >> (but then it is not relevant to the code). >> > > I hope I covered this above already and it's a bit clearer now. > Sorry for the confusion! Yes, thanks! Best regards, Krzysztof
Hi, On Tue, May 18, 2021 at 11:25:55AM -0400, Krzysztof Kozlowski wrote: > On 18/05/2021 11:00, Stephan Gerhold wrote: > > Hi, > > > > 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. > > In such case there is no point to do anything more with the provided > clock than what you are doing - enable it when device is on, disable > when off. > > I think it is enough to rephrase the msg: > 1. Add at beginning that device has one clock input (XI pin). The clock > input was so far ignored (assumed to be routed to some always-on > oscillator). > 2. The device should enable the clock when running. > 3. Add all of your paragraph about detailed logic on GPIO. > > Since the GPIO is non-controllable, it actually does not matter that > much for the driver, so you can add it for relevance, but not as main > point of the patch. > The GPIO does not matter for the driver in my case (and requesting it from the s3fwrn5 driver would likely break my special pinctrl setup that muxes it to the "AND" in PM8916). However, I did see some alternative code in the vendor NFC driver where they request it, set up an interrupt for it and then do the "clk_prepare_enable()" when it's asserted and clk_disable_unprepare() when de-asserted. I guess there are like 3 typical setups for the clock: 1. Always-on oscillator 2. GPIO2 magically handled by clock provider (my case) 3. GPIO2 connected to SoC, driver must monitor it We might need 3. at some point, but I don't think it makes sense to add it until someone actually needs it (and can test it). I will try to reword the message a bit and send a v2 tomorrow or so. Thanks! Stephan
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. 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. :) Stephan
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? > > 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? Best regards, Krzysztof
On Wed, May 19, 2021 at 11:58:38AM -0400, Krzysztof Kozlowski wrote: > > > > 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? > I don't know exactly but I don't think it's lost. At least the firmware and rfreg configuration seem to be preserved since I've never provided the driver with the necessary firmware. It just works without :) Stephan
On Thu, May 20, 2021 at 12:58 AM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> 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. > > Best regards, > Krzysztof
On 20/05/2021 07:40, Bongsu Jeon wrote: > On Thu, May 20, 2021 at 12:58 AM Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> 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
diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c index 897394167522..d8910f97ee4a 100644 --- a/drivers/nfc/s3fwrn5/i2c.c +++ b/drivers/nfc/s3fwrn5/i2c.c @@ -6,6 +6,7 @@ * Robert Baldyga <r.baldyga@samsung.com> */ +#include <linux/clk.h> #include <linux/i2c.h> #include <linux/gpio.h> #include <linux/delay.h> @@ -22,6 +23,7 @@ struct s3fwrn5_i2c_phy { struct phy_common common; struct i2c_client *i2c_dev; + struct clk *clk; unsigned int irq_skip:1; }; @@ -207,17 +209,42 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client, if (ret < 0) return ret; + phy->clk = devm_clk_get_optional(&client->dev, NULL); + if (IS_ERR(phy->clk)) + return dev_err_probe(&client->dev, PTR_ERR(phy->clk), + "failed to get clock\n"); + + /* + * 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. For now only the first case is handled here. + * We keep the clock "on" permanently so the clock provider will begin + * looking at NFC_CLK_REQ and enables the clock when necessary. + */ + ret = clk_prepare_enable(phy->clk); + if (ret < 0) { + dev_err(&client->dev, "failed to enable clock: %d\n", ret); + return ret; + } + ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev, &i2c_phy_ops); if (ret < 0) - return ret; + goto disable_clk; ret = devm_request_threaded_irq(&client->dev, phy->i2c_dev->irq, NULL, s3fwrn5_i2c_irq_thread_fn, IRQF_ONESHOT, S3FWRN5_I2C_DRIVER_NAME, phy); if (ret) - s3fwrn5_remove(phy->common.ndev); + goto s3fwrn5_remove; + return 0; + +s3fwrn5_remove: + s3fwrn5_remove(phy->common.ndev); +disable_clk: + clk_disable_unprepare(phy->clk); return ret; } @@ -226,6 +253,7 @@ static int s3fwrn5_i2c_remove(struct i2c_client *client) struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client); s3fwrn5_remove(phy->common.ndev); + clk_disable_unprepare(phy->clk); return 0; }
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 commit adds support for this by requesting an optional clock and keeping it permanently enabled. Note that the actual (physical) clock won't be permanently enabled since this will depend on the output of NFC_CLK_REQ from S3FWRN5. In the future (when needed by some other device) this could be extended to work for the second case (monitoring the NFC_CLK_REQ GPIO and enabling the clock from the kernel when needed). Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- drivers/nfc/s3fwrn5/i2c.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)