[v1,1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem Bay SOC

Commit Message

Zulkifli, Muhammad Husaini Nov. 6, 2020, 12:09 p.m. UTC

From: Muhammad Husaini Zulkifli <[e-mail]>

Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations handling")
introduces platform specific SDHCI clock operation.

This patch declares the clock operation for Keem Bay.
Add clk_ops for SD, EMMC and SDIO operations.

Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel Keem Bay")

Signed-off-by: Muhammad Husaini Zulkifli <[e-mail]>
Reviewed-by: Adrian Hunter <[e-mail]>
---
 drivers/mmc/host/sdhci-of-arasan.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Ulf Hansson Nov. 16, 2020, 11:41 a.m. UTC | #1

On Fri, 6 Nov 2020 at 05:10, <[e-mail]> wrote:
>
> From: Muhammad Husaini Zulkifli <[e-mail]>
>
> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations handling")
> introduces platform specific SDHCI clock operation.
>
> This patch declares the clock operation for Keem Bay.
> Add clk_ops for SD, EMMC and SDIO operations.

The above commit message doesn't really tell why or what goes on here.
Can please try to clarify that.

>
> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel Keem Bay")

Is $subject patch fixing a bug/regression?

Kind regards
Uffe

>
> Signed-off-by: Muhammad Husaini Zulkifli <[e-mail]>
> Reviewed-by: Adrian Hunter <[e-mail]>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 829ccef87426..012d52e1abee 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -1199,16 +1199,19 @@ static struct sdhci_arasan_of_data sdhci_arasan_versal_data = {
>  static struct sdhci_arasan_of_data intel_keembay_emmc_data = {
>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>         .pdata = &sdhci_keembay_emmc_pdata,
> +       .clk_ops = &arasan_clk_ops,
>  };
>
>  static struct sdhci_arasan_of_data intel_keembay_sd_data = {
>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>         .pdata = &sdhci_keembay_sd_pdata,
> +       .clk_ops = &arasan_clk_ops,
>  };
>
>  static struct sdhci_arasan_of_data intel_keembay_sdio_data = {
>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>         .pdata = &sdhci_keembay_sdio_pdata,
> +       .clk_ops = &arasan_clk_ops,
>  };
>
>  static const struct of_device_id sdhci_arasan_of_match[] = {
> --
> 2.17.1
>

Zulkifli, Muhammad Husaini Nov. 16, 2020, 2:22 p.m. UTC | #2

Hi Ulf,

Thanks for your review comments. I replied inline

>-----Original Message-----
>From: Ulf Hansson <[e-mail]>
>Sent: Monday, November 16, 2020 7:41 PM
>To: Zulkifli, Muhammad Husaini <[e-mail]>
>Cc: [e-mail]; Linux ARM <linux-arm-
>[e-mail]>; Linux Kernel Mailing List <linux-
>[e-mail]>; Hunter, Adrian <[e-mail]>; Raja
>Subramanian, Lakshmi Bai <[e-mail]>; Wan
>Mohamad, Wan Ahmad Zainie
><[e-mail]>; David E. Box
><[e-mail]>
>Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem
>Bay SOC
>
>On Fri, 6 Nov 2020 at 05:10, <[e-mail]> wrote:
>>
>> From: Muhammad Husaini Zulkifli <[e-mail]>
>>
>> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations
>> handling") introduces platform specific SDHCI clock operation.
>>
>> This patch declares the clock operation for Keem Bay.
>> Add clk_ops for SD, EMMC and SDIO operations.
>
>The above commit message doesn't really tell why or what goes on here.
>Can please try to clarify that.

We missed out the clock operation handling patch (Commit 16ada730a759)
and did not handle the clk_ops  for Keem Bay SOC devices. 
These 2 patches (Commit 16ada730a759) and (Commit 36c6aadaae86)  are merged at around the same time.
We catch the issue later when trying to boot into v5.10-rc1 with Keem Bay EVM.
That is why I created this patch to handle the clk_ops for Keem Bay devices.

>
>>
>> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel Keem
>> Bay")
>
>Is $subject patch fixing a bug/regression?

This is to fix issue on previous commit. It is a bug fix.

>
>Kind regards
>Uffe
>
>>
>> Signed-off-by: Muhammad Husaini Zulkifli
>> <[e-mail]>
>> Reviewed-by: Adrian Hunter <[e-mail]>
>> ---
>>  drivers/mmc/host/sdhci-of-arasan.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index 829ccef87426..012d52e1abee 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -1199,16 +1199,19 @@ static struct sdhci_arasan_of_data
>> sdhci_arasan_versal_data = {  static struct sdhci_arasan_of_data
>intel_keembay_emmc_data = {
>>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>>         .pdata = &sdhci_keembay_emmc_pdata,
>> +       .clk_ops = &arasan_clk_ops,
>>  };
>>
>>  static struct sdhci_arasan_of_data intel_keembay_sd_data = {
>>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>>         .pdata = &sdhci_keembay_sd_pdata,
>> +       .clk_ops = &arasan_clk_ops,
>>  };
>>
>>  static struct sdhci_arasan_of_data intel_keembay_sdio_data = {
>>         .soc_ctl_map = &intel_keembay_soc_ctl_map,
>>         .pdata = &sdhci_keembay_sdio_pdata,
>> +       .clk_ops = &arasan_clk_ops,
>>  };
>>
>>  static const struct of_device_id sdhci_arasan_of_match[] = {
>> --
>> 2.17.1
>>

Ulf Hansson Nov. 17, 2020, 11:27 a.m. UTC | #3

On Mon, 16 Nov 2020 at 15:22, Zulkifli, Muhammad Husaini
<[e-mail]> wrote:
>
> Hi Ulf,
>
> Thanks for your review comments. I replied inline
>
> >-----Original Message-----
> >From: Ulf Hansson <[e-mail]>
> >Sent: Monday, November 16, 2020 7:41 PM
> >To: Zulkifli, Muhammad Husaini <[e-mail]>
> >Cc: [e-mail]; Linux ARM <linux-arm-
> >[e-mail]>; Linux Kernel Mailing List <linux-
> >[e-mail]>; Hunter, Adrian <[e-mail]>; Raja
> >Subramanian, Lakshmi Bai <[e-mail]>; Wan
> >Mohamad, Wan Ahmad Zainie
> ><[e-mail]>; David E. Box
> ><[e-mail]>
> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem
> >Bay SOC
> >
> >On Fri, 6 Nov 2020 at 05:10, <[e-mail]> wrote:
> >>
> >> From: Muhammad Husaini Zulkifli <[e-mail]>
> >>
> >> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations
> >> handling") introduces platform specific SDHCI clock operation.
> >>
> >> This patch declares the clock operation for Keem Bay.
> >> Add clk_ops for SD, EMMC and SDIO operations.
> >
> >The above commit message doesn't really tell why or what goes on here.
> >Can please try to clarify that.
>
> We missed out the clock operation handling patch (Commit 16ada730a759)
> and did not handle the clk_ops  for Keem Bay SOC devices.
> These 2 patches (Commit 16ada730a759) and (Commit 36c6aadaae86)  are merged at around the same time.
> We catch the issue later when trying to boot into v5.10-rc1 with Keem Bay EVM.
> That is why I created this patch to handle the clk_ops for Keem Bay devices.

Alright, so the previous changes were simply not sufficient to
complete the support.

>
> >
> >>
> >> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel Keem
> >> Bay")
> >
> >Is $subject patch fixing a bug/regression?
>
> This is to fix issue on previous commit. It is a bug fix.

Has it ever worked?

Did the commit you point to with the fixes tag, break some other
existing functionality?

[...]

Kind regards
Uffe

Zulkifli, Muhammad Husaini Nov. 17, 2020, 1:46 p.m. UTC | #4

Hi Ulf,

>-----Original Message-----
>From: Ulf Hansson <[e-mail]>
>Sent: Tuesday, November 17, 2020 7:27 PM
>To: Zulkifli, Muhammad Husaini <[e-mail]>
>Cc: [e-mail]; Linux ARM <linux-arm-
>[e-mail]>; Linux Kernel Mailing List <linux-
>[e-mail]>; Hunter, Adrian <[e-mail]>; Raja
>Subramanian, Lakshmi Bai <[e-mail]>; Wan
>Mohamad, Wan Ahmad Zainie
><[e-mail]>; David E. Box
><[e-mail]>
>Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem Bay
>SOC
>
>On Mon, 16 Nov 2020 at 15:22, Zulkifli, Muhammad Husaini
><[e-mail]> wrote:
>>
>> Hi Ulf,
>>
>> Thanks for your review comments. I replied inline
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <[e-mail]>
>> >Sent: Monday, November 16, 2020 7:41 PM
>> >To: Zulkifli, Muhammad Husaini <[e-mail]>
>> >Cc: [e-mail]; Linux ARM <linux-arm-
>> >[e-mail]>; Linux Kernel Mailing List <linux-
>> >[e-mail]>; Hunter, Adrian <[e-mail]>;
>> >Raja Subramanian, Lakshmi Bai
>> ><[e-mail]>; Wan Mohamad, Wan Ahmad
>> >Zainie <[e-mail]>; David E. Box
>> ><[e-mail]>
>> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops
>> >for Keem Bay SOC
>> >
>> >On Fri, 6 Nov 2020 at 05:10, <[e-mail]> wrote:
>> >>
>> >> From: Muhammad Husaini Zulkifli
>> >> <[e-mail]>
>> >>
>> >> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations
>> >> handling") introduces platform specific SDHCI clock operation.
>> >>
>> >> This patch declares the clock operation for Keem Bay.
>> >> Add clk_ops for SD, EMMC and SDIO operations.
>> >
>> >The above commit message doesn't really tell why or what goes on here.
>> >Can please try to clarify that.
>>
>> We missed out the clock operation handling patch (Commit 16ada730a759)
>> and did not handle the clk_ops  for Keem Bay SOC devices.
>> These 2 patches (Commit 16ada730a759) and (Commit 36c6aadaae86)  are
>merged at around the same time.
>> We catch the issue later when trying to boot into v5.10-rc1 with Keem Bay
>EVM.
>> That is why I created this patch to handle the clk_ops for Keem Bay devices.
>
>Alright, so the previous changes were simply not sufficient to complete the
>support.
>
>>
>> >
>> >>
>> >> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel
>> >> Keem
>> >> Bay")
>> >
>> >Is $subject patch fixing a bug/regression?
>>
>> This is to fix issue on previous commit. It is a bug fix.
>
>Has it ever worked?

Yes it is working. Tested with Keem Bay EVM. 
SD/EMMC/SDIO devices registered successfully with this fix.  

>
>Did the commit you point to with the fixes tag, break some other existing
>functionality?

It should not break other existing functionality. 
This is specific for Keem Bay SOC only.

>
>[...]
>
>Kind regards
>Uffe

Ulf Hansson Nov. 17, 2020, 5:14 p.m. UTC | #5

On Tue, 17 Nov 2020 at 14:46, Zulkifli, Muhammad Husaini
<[e-mail]> wrote:
>
> Hi Ulf,
>
> >-----Original Message-----
> >From: Ulf Hansson <[e-mail]>
> >Sent: Tuesday, November 17, 2020 7:27 PM
> >To: Zulkifli, Muhammad Husaini <[e-mail]>
> >Cc: [e-mail]; Linux ARM <linux-arm-
> >[e-mail]>; Linux Kernel Mailing List <linux-
> >[e-mail]>; Hunter, Adrian <[e-mail]>; Raja
> >Subramanian, Lakshmi Bai <[e-mail]>; Wan
> >Mohamad, Wan Ahmad Zainie
> ><[e-mail]>; David E. Box
> ><[e-mail]>
> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem Bay
> >SOC
> >
> >On Mon, 16 Nov 2020 at 15:22, Zulkifli, Muhammad Husaini
> ><[e-mail]> wrote:
> >>
> >> Hi Ulf,
> >>
> >> Thanks for your review comments. I replied inline
> >>
> >> >-----Original Message-----
> >> >From: Ulf Hansson <[e-mail]>
> >> >Sent: Monday, November 16, 2020 7:41 PM
> >> >To: Zulkifli, Muhammad Husaini <[e-mail]>
> >> >Cc: [e-mail]; Linux ARM <linux-arm-
> >> >[e-mail]>; Linux Kernel Mailing List <linux-
> >> >[e-mail]>; Hunter, Adrian <[e-mail]>;
> >> >Raja Subramanian, Lakshmi Bai
> >> ><[e-mail]>; Wan Mohamad, Wan Ahmad
> >> >Zainie <[e-mail]>; David E. Box
> >> ><[e-mail]>
> >> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops
> >> >for Keem Bay SOC
> >> >
> >> >On Fri, 6 Nov 2020 at 05:10, <[e-mail]> wrote:
> >> >>
> >> >> From: Muhammad Husaini Zulkifli
> >> >> <[e-mail]>
> >> >>
> >> >> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations
> >> >> handling") introduces platform specific SDHCI clock operation.
> >> >>
> >> >> This patch declares the clock operation for Keem Bay.
> >> >> Add clk_ops for SD, EMMC and SDIO operations.
> >> >
> >> >The above commit message doesn't really tell why or what goes on here.
> >> >Can please try to clarify that.
> >>
> >> We missed out the clock operation handling patch (Commit 16ada730a759)
> >> and did not handle the clk_ops  for Keem Bay SOC devices.
> >> These 2 patches (Commit 16ada730a759) and (Commit 36c6aadaae86)  are
> >merged at around the same time.
> >> We catch the issue later when trying to boot into v5.10-rc1 with Keem Bay
> >EVM.
> >> That is why I created this patch to handle the clk_ops for Keem Bay devices.
> >
> >Alright, so the previous changes were simply not sufficient to complete the
> >support.
> >
> >>
> >> >
> >> >>
> >> >> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel
> >> >> Keem
> >> >> Bay")
> >> >
> >> >Is $subject patch fixing a bug/regression?
> >>
> >> This is to fix issue on previous commit. It is a bug fix.
> >
> >Has it ever worked?
>
> Yes it is working. Tested with Keem Bay EVM.
> SD/EMMC/SDIO devices registered successfully with this fix.

I am not asking if $subject patch makes it work, I understand that it does.

>
> >
> >Did the commit you point to with the fixes tag, break some other existing
> >functionality?
>
> It should not break other existing functionality.
> This is specific for Keem Bay SOC only.

So, you are saying that it has never worked for Keem Bay? But $subject
patch makes it work?

Can you please re-spin and update the commit message to better reflect
what goes on, then I can apply.

Kind regards
Uffe

Zulkifli, Muhammad Husaini Nov. 18, 2020, 1:04 a.m. UTC | #6

Hi Ulf,

>-----Original Message-----
>From: Ulf Hansson <[e-mail]>
>Sent: Wednesday, November 18, 2020 1:14 AM
>To: Zulkifli, Muhammad Husaini <[e-mail]>
>Cc: [e-mail]; Linux ARM <linux-arm-
>[e-mail]>; Linux Kernel Mailing List <linux-
>[e-mail]>; Hunter, Adrian <[e-mail]>; Raja
>Subramanian, Lakshmi Bai <[e-mail]>; Wan
>Mohamad, Wan Ahmad Zainie
><[e-mail]>; David E. Box
><[e-mail]>
>Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops for Keem Bay
>SOC
>
>On Tue, 17 Nov 2020 at 14:46, Zulkifli, Muhammad Husaini
><[e-mail]> wrote:
>>
>> Hi Ulf,
>>
>> >-----Original Message-----
>> >From: Ulf Hansson <[e-mail]>
>> >Sent: Tuesday, November 17, 2020 7:27 PM
>> >To: Zulkifli, Muhammad Husaini <[e-mail]>
>> >Cc: [e-mail]; Linux ARM <linux-arm-
>> >[e-mail]>; Linux Kernel Mailing List <linux-
>> >[e-mail]>; Hunter, Adrian <[e-mail]>;
>> >Raja Subramanian, Lakshmi Bai
>> ><[e-mail]>; Wan Mohamad, Wan Ahmad
>> >Zainie <[e-mail]>; David E. Box
>> ><[e-mail]>
>> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops
>> >for Keem Bay SOC
>> >
>> >On Mon, 16 Nov 2020 at 15:22, Zulkifli, Muhammad Husaini
>> ><[e-mail]> wrote:
>> >>
>> >> Hi Ulf,
>> >>
>> >> Thanks for your review comments. I replied inline
>> >>
>> >> >-----Original Message-----
>> >> >From: Ulf Hansson <[e-mail]>
>> >> >Sent: Monday, November 16, 2020 7:41 PM
>> >> >To: Zulkifli, Muhammad Husaini
>> >> ><[e-mail]>
>> >> >Cc: [e-mail]; Linux ARM <linux-arm-
>> >> >[e-mail]>; Linux Kernel Mailing List <linux-
>> >> >[e-mail]>; Hunter, Adrian <[e-mail]>;
>> >> >Raja Subramanian, Lakshmi Bai
>> >> ><[e-mail]>; Wan Mohamad, Wan
>Ahmad
>> >> >Zainie <[e-mail]>; David E. Box
>> >> ><[e-mail]>
>> >> >Subject: Re: [PATCH v1 1/1] mmc: sdhci-of-arasan: Specify .clk_ops
>> >> >for Keem Bay SOC
>> >> >
>> >> >On Fri, 6 Nov 2020 at 05:10, <[e-mail]>
>wrote:
>> >> >>
>> >> >> From: Muhammad Husaini Zulkifli
>> >> >> <[e-mail]>
>> >> >>
>> >> >> Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock
>> >> >> operations
>> >> >> handling") introduces platform specific SDHCI clock operation.
>> >> >>
>> >> >> This patch declares the clock operation for Keem Bay.
>> >> >> Add clk_ops for SD, EMMC and SDIO operations.
>> >> >
>> >> >The above commit message doesn't really tell why or what goes on here.
>> >> >Can please try to clarify that.
>> >>
>> >> We missed out the clock operation handling patch (Commit
>> >> 16ada730a759) and did not handle the clk_ops  for Keem Bay SOC devices.
>> >> These 2 patches (Commit 16ada730a759) and (Commit 36c6aadaae86)
>> >> are
>> >merged at around the same time.
>> >> We catch the issue later when trying to boot into v5.10-rc1 with
>> >> Keem Bay
>> >EVM.
>> >> That is why I created this patch to handle the clk_ops for Keem Bay
>devices.
>> >
>> >Alright, so the previous changes were simply not sufficient to
>> >complete the support.
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Fixes: 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for
>> >> >> Intel Keem
>> >> >> Bay")
>> >> >
>> >> >Is $subject patch fixing a bug/regression?
>> >>
>> >> This is to fix issue on previous commit. It is a bug fix.
>> >
>> >Has it ever worked?
>>
>> Yes it is working. Tested with Keem Bay EVM.
>> SD/EMMC/SDIO devices registered successfully with this fix.
>
>I am not asking if $subject patch makes it work, I understand that it does.
>
>>
>> >
>> >Did the commit you point to with the fixes tag, break some other
>> >existing functionality?
>>
>> It should not break other existing functionality.
>> This is specific for Keem Bay SOC only.
>
>So, you are saying that it has never worked for Keem Bay? But $subject patch
>makes it work?

It is working for Keem Bay before until the Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations handling") 
came into the picture. Both Commit 16ada730a759 ("mmc: sdhci-of-arasan: Modify clock operations handling") and 
Commit 36c6aadaae86 ("mmc: sdhci-of-arasan: Add support for Intel Keem") are merged about the same time. 
We did not have "Modify clock operations handling" patch when rebasing and testing during upstreaming process.

I noticed of this issue when using latest mainline v5.10-rcx recently.

>
>Can you please re-spin and update the commit message to better reflect what
>goes on, then I can apply.

Noted. I will update the commit message and re-send again.
Thanks

>
>Kind regards
>Uffe

Patch

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 829ccef87426..012d52e1abee 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1199,16 +1199,19 @@  static struct sdhci_arasan_of_data sdhci_arasan_versal_data = {
 static struct sdhci_arasan_of_data intel_keembay_emmc_data = {
 	.soc_ctl_map = &intel_keembay_soc_ctl_map,
 	.pdata = &sdhci_keembay_emmc_pdata,
+	.clk_ops = &arasan_clk_ops,
 };
 
 static struct sdhci_arasan_of_data intel_keembay_sd_data = {
 	.soc_ctl_map = &intel_keembay_soc_ctl_map,
 	.pdata = &sdhci_keembay_sd_pdata,
+	.clk_ops = &arasan_clk_ops,
 };
 
 static struct sdhci_arasan_of_data intel_keembay_sdio_data = {
 	.soc_ctl_map = &intel_keembay_soc_ctl_map,
 	.pdata = &sdhci_keembay_sdio_pdata,
+	.clk_ops = &arasan_clk_ops,
 };
 
 static const struct of_device_id sdhci_arasan_of_match[] = {