Re: [PATCH v4 1/2] arm64: dts: meson-g12b-odroid-n2: Enable RTC controller node

From: Neil Armstrong <narmstrong@baylibre.com>
To: Anand Moon <linux.amoon@gmail.com>
Cc: Kevin Hilman <khilman@baylibre.com>,
	devicetree <devicetree@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-amlogic@lists.infradead.org,
	Rob Herring <robh+dt@kernel.org>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Christian Hewitt <christianshewitt@gmail.com>
Subject: Re: [PATCH v4 1/2] arm64: dts: meson-g12b-odroid-n2: Enable RTC controller node
Date: Wed, 2 Sep 2020 09:18:19 +0200	[thread overview]
Message-ID: <0845cbc4-cf95-5c00-4294-1b98b159727d@baylibre.com> (raw)
In-Reply-To: <CANAwSgRyV_Sz3GbLZ__-Ed8nhw5WCbChULVX8hhFLmgSs1tb-A@mail.gmail.com>

On 01/09/2020 17:29, Anand Moon wrote:
> Hi Neil,
> 
> On Tue, 1 Sep 2020 at 18:42, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> On 01/09/2020 12:14, Anand Moon wrote:
>>> Hi Kevin,
>>>
>>> Thanks for your review comments and testing.
>>>
>>> On Tue, 1 Sep 2020 at 01:36, Kevin Hilman <khilman@baylibre.com> wrote:
>>>>
>>>> Anand Moon <linux.amoon@gmail.com> writes:
>>>>
>>>>> Enable RTC PCF8563 node on Odroid-N2 SBC, In order to
>>>>> support the RTC wakealarm feature for suspend and resume.
>>>>> Also assign an alias to the pcf8563 to rtc0 and meson-vrtc to rtc1
>>>>> timer device to prevent it being assigned to /dev/rtc0
>>>>> which disto userspace tools assume is a clock device.
>>>>>
>>>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>>>> Cc: Kevin Hilman <khilman@baylibre.com>
>>>>> Suggested-by: Christian Hewitt <christianshewitt@gmail.com>
>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> ---
>>>>> Changes v4
>>>>> --Add gpio interrupt for GPIOAO.BIT7 as suggested by Neil.
>>>>> Changes v3
>>>>> --Drop the INI GPIOAO.BIT7 pinctrl.
>>>>> --Added missing RTC alias so that rtc get assigned correcly,
>>>>>   as suggested by Chris Hewitt.
>>>>> changes v2
>>>>> --Fix the missing INT (GPIOAO.BIT7) pinctrl.
>>>>> --Fix the missing rtcwakeup.
>>>>> --Drop the clock not required clock property by the PCF8563 driver.
>>>>> ---
>>>>>  .../boot/dts/amlogic/meson-g12b-odroid-n2.dts   | 17 +++++++++++++++++
>>>>>  1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
>>>>> index 34fffa6d859d..3e2aaa6f48e5 100644
>>>>> --- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
>>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dts
>>>>> @@ -19,6 +19,8 @@ / {
>>>>>       aliases {
>>>>>               serial0 = &uart_AO;
>>>>>               ethernet0 = &ethmac;
>>>>> +             rtc0 = &rtc0;
>>>>> +             rtc1 = &vrtc;
>>>>>       };
>>>>>
>>>>>       dioo2133: audio-amplifier-0 {
>>>>> @@ -477,6 +479,21 @@ hdmi_tx_tmds_out: endpoint {
>>>>>       };
>>>>>  };
>>>>>
>>>>> +&i2c3 {
>>>>> +     pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
>>>>> +     pinctrl-names = "default";
>>>>> +     status = "okay";
>>>>> +
>>>>> +     rtc0: rtc@51 {
>>>>> +             reg = <0x51>;
>>>>> +             compatible = "nxp,pcf8563";
>>>>> +             /* RTC INT */
>>>>> +             interrupts = <GPIOAO_7 IRQ_TYPE_LEVEL_LOW>;
>>>>> +             interrupt-parent = <&gpio_intc>;
>>>>> +             wakeup-source;
>>>>> +     };
>>>>> +};
>>>>
>>>> There's still no pinctrl definition for the GPIO pin being used as the
>>>> IRQ.  It looks like you discussed this with Martin and he pointed you in
>>>> the right direction in your v3 series, but I don't see it in this
>>>> patch.
>>>>
>>> Yes I had followed the approach suggested by Martin on previous email and IRC.
>>> but it really did not work out for me in the testing.
>>>
>>> rtc-pcf8563 driver does not handle such gpio configuration.
>>> so the rtc probe will fail if we add gpio pinctl to *pinctrl-0*.
>>
>> No need for multiple pinctrl-*, simple add a new pinctrl to the rtc node like:
>>
>>
>> @@ -18,6 +18,8 @@
>>         aliases {
>>                 serial0 = &uart_AO;
>>                 ethernet0 = &ethmac;
>> +               rtc0 = &rtc0;
>> +               rtc1 = &vrtc;
>>         };
>>
>>         chosen {
>> @@ -266,6 +268,17 @@
>>         status = "okay";
>>  };
>>
>> +&ao_pinctrl {
>> +       rtc_int_pins: rtc-int {
>> +               mux {
>> +                       groups = "GPIOAO_7";
>> +                       function = "gpio_aobus";
>> +                       bias-disable;
>> +                       output-disable;
>> +               };
>> +       };
>> +};
>> +
>>  &cec_AO {
>>         pinctrl-0 = <&cec_ao_a_h_pins>;
>>         pinctrl-names = "default";
>> @@ -391,6 +404,23 @@
>>         };
>>  };
>>
>> +&i2c3 {
>> +       pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
>> +       pinctrl-names = "default";
>> +       status = "okay";
>> +
>> +       rtc0: rtc@51 {
>> +             pinctrl-0 = <&rtc_int_pins>;
>> +             pinctrl-names = "default";
>> +             reg = <0x51>;
>> +             compatible = "nxp,pcf8563";
>> +             /* RTC INT */
>> +             interrupts = <GPIOAO_7 IRQ_TYPE_LEVEL_LOW>;
>> +             interrupt-parent = <&gpio_intc>;
>> +             wakeup-source;
>> +       };
>> +};
>> +
>>  &ir {
>>         status = "okay";
>>         pinctrl-0 = <&remote_input_ao_pins>;
>>
>> DISCALIMER: not built or run tested
>>
>> Neil
>>
> 
> I have built this and tested, but still this gpio pin

Can you remove the "bias-disable" in rtc_int_pins and retry ?

The default bias is pull-up, removing it may cause this.

If it works, can you try "bias-pull-up" ? if it works, keep it and resend with my ack.

Neil

> 
> # cat /sys/kernel/debug/gpio
> gpiochip1: GPIOs 412-426, parent:
> platform/ff800000.sys-ctrl:pinctrl@14, aobus-banks:
>  gpio-414 (                    |enable              ) out lo
>  gpio-420 (                    |regulator-tflash_vdd) out hi
>  gpio-421 (                    |TF_IO               ) out lo
>  gpio-423 (                    |n2:blue             ) out lo
> 
> gpiochip0: GPIOs 427-511, parent: platform/ff634400.bus:pinctrl@40,
> periphs-banks:
>  gpio-442 (                    |PHY reset           ) out hi ACTIVE LOW
>  gpio-447 (                    |usb-hub-reset       ) out hi
>  gpio-448 (                    |regulator-hub_5v    ) out hi
>  gpio-449 (                    |regulator-usb_pwr_en) out lo
>  gpio-464 (                    |reset               ) out hi ACTIVE LOW
>  gpio-474 (                    |cd                  ) in  lo ACTIVE LOW
> 
> This change fails on *u-boot Hardkernel*
> There is some timing issue, some time it wakes up and
> some time it gets stuck.
> 
> # rtcwake -d /dev/rtc0 -s 10
> rtcwake: wakeup from "mem" using /dev/rtc0 at Tue Sep  1 14:05:47 2020
> [   35.565415] PM: suspend entry (deep)
> [   35.565589] Filesystems sync: 0.000 seconds
> [   72.670162] cfg80211: failed to load regulatory.db
> [   72.670164] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [   72.676235] OOM killer disabled.
> [   72.679418] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) done.
> [   72.688636] meson8b-dwmac ff3f0000.ethernet eth0: Link is Down
> [   72.709953] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [   72.830228] Disabling non-boot CPUs ...
> [   72.831157] CPU1: shutdown
> [   72.832204] psci: CPU1 killed (polled 0 ms)
> [   72.837376] CPU2: shutdown
> [   72.837974] psci: CPU2 killed (polled 0 ms)
> [   72.843880] CPU3: shutdown
> [   72.844742] psci: CPU3 killed (polled 0 ms)
> [   72.850925] CPU4: shutdown
> [   72.851557] psci: CPU4 killed (polled 0 ms)
> [   72.858185] CPU5: shutdown
> [   72.858372] psci: CPU5 killed (polled 0 ms)
> bl30 get wakeup sources!
> process command 00000006
> bl30 enter suspend!
> Little core clk suspend rate 66700000
> Big core clk suspend rate 24000000
> store restore gp0 pll
> suspend_counter: 1
> Enter ddr suspend
> ddr suspend time: 17us
> alarm=0S
> process command 00000001
> cec ver:2018/04/19
> CEC cfg:0x0000
> WAKEUP GPIO cfg:0x00000000
> use vddee new table!
> kern log_addr:0x00
> cec T: 00
> err: tx not finish flag
> cec reset
> Set cec pinmux:0x11
> Set cec log_addr:0x10,ADDR0:10
> customer pwrkeys for IR is NULL, use defaults!
> 
> Best Regards
> -Anand
> 