PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card reader on my Intel NUC6CAYH box.

The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp() was added to rtsx_pci_init_hw().
At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in rtsx_pci_init_ocp() the cardreader
gets disabled.

I've avoided this by making excution code that results in the reader being disabled conditional on the device
not being an RTS5229. Of course, other rtsxxx card readers may also be disabled by this bug. I don't have the
knowledge to address that, so I'll leave to the driver maintainers.

The patch to avoid the bug is attached.

Fixes: bede03a579b3 ("misc: rtsx: Enable OCP for rts522a rts524a rts525a rts5260")
Link: https://marc.info/?l=linux-kernel&m=159105912832257
Link: https://bugzilla.kernel.org/show_bug.cgi?id=204003
Signed-off-by: Chris Clayton <chris2553@googlemail.com>

bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card reader on my Intel NUC6CAYH box.

The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp() was added to rtsx_pci_init_hw().
At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in rtsx_pci_init_ocp() the cardreader
gets disabled.

I've avoided this by making excution code that results in the reader being disabled conditional on the device
not being an RTS5229. Of course, other rtsxxx card readers may also be disabled by this bug. I don't have the
knowledge to address that, so I'll leave to the driver maintainers.

The patch to avoid the bug is attached.

Chris

[-- Attachment #2: dont-disable-rts5229-cardreader-on-intel-NUC.patch --]
[-- Type: text/x-patch, Size: 697 bytes --]

--- linux-5.7.12/drivers/misc/cardreader/rtsx_pcr.c.orig	2020-08-02 13:36:50.216947944 +0100
+++ linux-5.7.12/drivers/misc/cardreader/rtsx_pcr.c	2020-08-02 18:37:30.456610731 +0100
@@ -1200,9 +1200,13 @@ void rtsx_pci_init_ocp(struct rtsx_pcr *
 				SD_OCP_GLITCH_MASK, pcr->hw_param.ocp_glitch);
 			rtsx_pci_enable_ocp(pcr);
 		} else {
-			/* OC power down */
-			rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
-				OC_POWER_DOWN);
+			/* On (some?) Intel NUC platforms, this disables
+			 * the rts5229 cardreader, so don't do it
+			 */
+			if(!CHK_PCI_PID(pcr, 0x5229))
+				/* OC power down */
+				rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
+					OC_POWER_DOWN);
 		}
 	}
 }

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

Sorry, I should have said that the patch is against 5.7.12. It applies to upstream, but with offsets.

On 02/08/2020 20:48, Chris Clayton wrote:
> bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card reader on my Intel NUC6CAYH box.
> 
> The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp() was added to rtsx_pci_init_hw().
> At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in rtsx_pci_init_ocp() the cardreader
> gets disabled.
> 
> I've avoided this by making excution code that results in the reader being disabled conditional on the device
> not being an RTS5229. Of course, other rtsxxx card readers may also be disabled by this bug. I don't have the
> knowledge to address that, so I'll leave to the driver maintainers.
> 
> The patch to avoid the bug is attached.
> 
> Fixes: bede03a579b3 ("misc: rtsx: Enable OCP for rts522a rts524a rts525a rts5260")
> Link: https://marc.info/?l=linux-kernel&m=159105912832257
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=204003
> Signed-off-by: Chris Clayton <chris2553@googlemail.com>
> 
> bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card reader on my Intel NUC6CAYH box.
> 
> The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp() was added to rtsx_pci_init_hw().
> At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in rtsx_pci_init_ocp() the cardreader
> gets disabled.
> 
> I've avoided this by making excution code that results in the reader being disabled conditional on the device
> not being an RTS5229. Of course, other rtsxxx card readers may also be disabled by this bug. I don't have the
> knowledge to address that, so I'll leave to the driver maintainers.
> 
> The patch to avoid the bug is attached.
> 
> Chris
> 

RE: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[吳昊澄 Ricky]

Hi Chris，

We don’t think this is our bug...
This register(FPDCTL) write to OC_POWER_DOWN is for our power saving feature, not to disable the reader
In your case, we cannot reproduce this on our side that we mention before, we don’t have the platform(Intel NUC Tall Arches Canyon NUC6CAYH Celeron J345) to see this issue
But we think this issue maybe only on this platform, our RTS5229 works well on the new kernel all platform that we have  

Ricky    

> -----Original Message-----
> From: Chris Clayton [mailto:chris2553@googlemail.com]
> Sent: Monday, August 03, 2020 3:59 AM
> To: LKML; 吳昊澄 Ricky; gregkh@linuxfoundation.org; rdunlap@infradead.org;
> philquadra@gmail.com; Arnd Bergmann
> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> Intel NUC boxes
> 
> Sorry, I should have said that the patch is against 5.7.12. It applies to upstream,
> but with offsets.
> 
> On 02/08/2020 20:48, Chris Clayton wrote:
> > bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
> reader on my Intel NUC6CAYH box.
> >
> > The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
> was added to rtsx_pci_init_hw().
> > At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
> rtsx_pci_init_ocp() the cardreader
> > gets disabled.
> >
> > I've avoided this by making excution code that results in the reader being
> disabled conditional on the device
> > not being an RTS5229. Of course, other rtsxxx card readers may also be
> disabled by this bug. I don't have the
> > knowledge to address that, so I'll leave to the driver maintainers.
> >
> > The patch to avoid the bug is attached.
> >
> > Fixes: bede03a579b3 ("misc: rtsx: Enable OCP for rts522a rts524a rts525a
> rts5260")
> > Link: https://marc.info/?l=linux-kernel&m=159105912832257
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=204003
> > Signed-off-by: Chris Clayton <chris2553@googlemail.com>
> >
> > bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
> reader on my Intel NUC6CAYH box.
> >
> > The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
> was added to rtsx_pci_init_hw().
> > At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
> rtsx_pci_init_ocp() the cardreader
> > gets disabled.
> >
> > I've avoided this by making excution code that results in the reader being
> disabled conditional on the device
> > not being an RTS5229. Of course, other rtsxxx card readers may also be
> disabled by this bug. I don't have the
> > knowledge to address that, so I'll leave to the driver maintainers.
> >
> > The patch to avoid the bug is attached.
> >
> > Chris
> >
> 
> ------Please consider the environment before printing this e-mail.

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

Hi, Ricky

On 03/08/2020 04:01, 吳昊澄 Ricky wrote:
> Hi Chris，
> 
> We don’t think this is our bug...
> This register(FPDCTL) write to OC_POWER_DOWN is for our power saving feature, not to disable the reader
> In your case, we cannot reproduce this on our side that we mention before, we don’t have the platform(Intel NUC Tall Arches Canyon NUC6CAYH Celeron J345) to see this issue
> But we think this issue maybe only on this platform, our RTS5229 works well on the new kernel all platform that we have  
> 
> Ricky    

Perhaps I should have used the word regression rather than bug. I didn't buy the machine until earlier this year, but
other people who have reported this problem have indicated that until bede03a579b3 was applied (during the 5.1 merge
window), the driver supported the card reader on this on the Intel NUC boxes. I know that is true because I built and
tested a 5.0 kernel and the card reader worked fine. I've also built and tested an 5.1-rc1 kernel and the card reader no
longer works. Whether by design or by accident, the card reader worked and now it doesn't. That's a regression in my book!

Since you signed off the patch that caused the regression, I believe it is your bug.

Thanks.

Chris
> 
>> -----Original Message-----
>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>> Sent: Monday, August 03, 2020 3:59 AM
>> To: LKML; 吳昊澄 Ricky; gregkh@linuxfoundation.org; rdunlap@infradead.org;
>> philquadra@gmail.com; Arnd Bergmann
>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>> Intel NUC boxes
>>
>> Sorry, I should have said that the patch is against 5.7.12. It applies to upstream,
>> but with offsets.
>>
>> On 02/08/2020 20:48, Chris Clayton wrote:
>>> bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
>> reader on my Intel NUC6CAYH box.
>>>
>>> The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
>> was added to rtsx_pci_init_hw().
>>> At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
>> rtsx_pci_init_ocp() the cardreader
>>> gets disabled.
>>>
>>> I've avoided this by making excution code that results in the reader being
>> disabled conditional on the device
>>> not being an RTS5229. Of course, other rtsxxx card readers may also be
>> disabled by this bug. I don't have the
>>> knowledge to address that, so I'll leave to the driver maintainers.
>>>
>>> The patch to avoid the bug is attached.
>>>
>>> Fixes: bede03a579b3 ("misc: rtsx: Enable OCP for rts522a rts524a rts525a
>> rts5260")
>>> Link: https://marc.info/?l=linux-kernel&m=159105912832257
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=204003
>>> Signed-off-by: Chris Clayton <chris2553@googlemail.com>
>>>
>>> bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
>> reader on my Intel NUC6CAYH box.
>>>
>>> The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
>> was added to rtsx_pci_init_hw().
>>> At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
>> rtsx_pci_init_ocp() the cardreader
>>> gets disabled.
>>>
>>> I've avoided this by making excution code that results in the reader being
>> disabled conditional on the device
>>> not being an RTS5229. Of course, other rtsxxx card readers may also be
>> disabled by this bug. I don't have the
>>> knowledge to address that, so I'll leave to the driver maintainers.
>>>
>>> The patch to avoid the bug is attached.
>>>
>>> Chris
>>>
>>
>> ------Please consider the environment before printing this e-mail.

RE: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[吳昊澄 Ricky]

Hi Chris,

rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN);
This register operation saved power under 1mA, so if do not care the 1mA power we can have a patch to remove it, make compatible with NUC6
We tested others our card reader that remove it, we did not see any side effect 

Hi Greg k-h,

Do you have any comments? 

thanks

Ricky
> -----Original Message-----
> From: Chris Clayton [mailto:chris2553@googlemail.com]
> Sent: Monday, August 03, 2020 3:59 AM
> To: LKML; 吳昊澄 Ricky; gregkh@linuxfoundation.org; rdunlap@infradead.org;
> philquadra@gmail.com; Arnd Bergmann
> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> Intel NUC boxes
> 
> Sorry, I should have said that the patch is against 5.7.12. It applies to upstream,
> but with offsets.
> 
> On 02/08/2020 20:48, Chris Clayton wrote:
> > bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
> reader on my Intel NUC6CAYH box.
> >
> > The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
> was added to rtsx_pci_init_hw().
> > At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
> rtsx_pci_init_ocp() the cardreader
> > gets disabled.
> >
> > I've avoided this by making excution code that results in the reader being
> disabled conditional on the device
> > not being an RTS5229. Of course, other rtsxxx card readers may also be
> disabled by this bug. I don't have the
> > knowledge to address that, so I'll leave to the driver maintainers.
> >
> > The patch to avoid the bug is attached.
> >
> > Fixes: bede03a579b3 ("misc: rtsx: Enable OCP for rts522a rts524a rts525a
> rts5260")
> > Link: https://marc.info/?l=linux-kernel&m=159105912832257
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=204003
> > Signed-off-by: Chris Clayton <chris2553@googlemail.com>
> >
> > bede03a579b3 introduced a bug which leaves the rts5229 PCI Express card
> reader on my Intel NUC6CAYH box.
> >
> > The bug is in drivers/misc/cardreader/rtsx_pcr.c. A call to rtsx_pci_init_ocp()
> was added to rtsx_pci_init_hw().
> > At the call point, pcr->ops->init_ocp is NULL and pcr->option.ocp_en is 0, so in
> rtsx_pci_init_ocp() the cardreader
> > gets disabled.
> >
> > I've avoided this by making excution code that results in the reader being
> disabled conditional on the device
> > not being an RTS5229. Of course, other rtsxxx card readers may also be
> disabled by this bug. I don't have the
> > knowledge to address that, so I'll leave to the driver maintainers.
> >
> > The patch to avoid the bug is attached.
> >
> > Chris
> >
> 
> ------Please consider the environment before printing this e-mail.

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[gregkh]

On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
> Hi Chris,
> 
> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN);
> This register operation saved power under 1mA, so if do not care the 1mA power we can have a patch to remove it, make compatible with NUC6
> We tested others our card reader that remove it, we did not see any side effect 
> 
> Hi Greg k-h,
> 
> Do you have any comments? 

comments on what?  I don't know what you are responding to here, sorry.

greg k-h

RE: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[吳昊澄 Ricky]

> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, August 04, 2020 3:49 PM
> To: 吳昊澄 Ricky
> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd
> Bergmann
> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> Intel NUC boxes
> 
> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
> > Hi Chris,
> >
> > rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN);
> > This register operation saved power under 1mA, so if do not care the 1mA
> power we can have a patch to remove it, make compatible with NUC6
> > We tested others our card reader that remove it, we did not see any side effect
> >
> > Hi Greg k-h,
> >
> > Do you have any comments?
> 
> comments on what?  I don't know what you are responding to here, sorry.
> 
Can we have a patch to kernel for NUC6? It may cause more power(1mA) but it will have more compatibility

> greg k-h
> 
> ------Please consider the environment before printing this e-mail.

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[gregkh]

On Tue, Aug 04, 2020 at 08:08:10AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, August 04, 2020 3:49 PM
> > To: 吳昊澄 Ricky
> > Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd
> > Bergmann
> > Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> > Intel NUC boxes
> > 
> > On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
> > > Hi Chris,
> > >
> > > rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN);
> > > This register operation saved power under 1mA, so if do not care the 1mA
> > power we can have a patch to remove it, make compatible with NUC6
> > > We tested others our card reader that remove it, we did not see any side effect
> > >
> > > Hi Greg k-h,
> > >
> > > Do you have any comments?
> > 
> > comments on what?  I don't know what you are responding to here, sorry.
> > 
> Can we have a patch to kernel for NUC6? It may cause more power(1mA)
> but it will have more compatibility

I do not understand at all, sorry.

greg k-h

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
>> -----Original Message-----
>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
>> Sent: Tuesday, August 04, 2020 3:49 PM
>> To: 吳昊澄 Ricky
>> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd
>> Bergmann
>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>> Intel NUC boxes
>>
>> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
>>> Hi Chris,
>>>
>>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN, OC_POWER_DOWN);
>>> This register operation saved power under 1mA, so if do not care the 1mA
>> power we can have a patch to remove it, make compatible with NUC6
>>> We tested others our card reader that remove it, we did not see any side effect
>>>
>>> Hi Greg k-h,
>>>
>>> Do you have any comments?
>>
>> comments on what?  I don't know what you are responding to here, sorry.
>>
> Can we have a patch to kernel for NUC6? It may cause more power(1mA) but it will have more compatibility
> 

Ricky,

I don't understand why you want to completely remove the code that sets up the 1mA power saving. That code was there
even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I assume it benefits some of the Realtek card
readers. Before your patch however, rtsx_pci_init_ocp() was not called for the rts5229 reader, but the patch introduced
an unconditional call to that function into rtsx_pci_init_hw(), which is run for the rts5229. That is what now disables
the card reader.

Now, I don't know whether other cards are affected, although I don't recall seeing any reported as I searched the kernel
and ubuntu bugzillas for any analysis of the problem. I know this is not what the patch I sent does, but having thought
about it more, seems to me that the simplest fix is to skip the new call to rtsx_pci_init_ocp() if the reader is an rts5229.

If you agree, I can prepare a patch and send it to you. Whatever the solution is, it will also be needed in the stable
kernels later than 5.0.

Chris
>> greg k-h
>>
>> ------Please consider the environment before printing this e-mail.

RE: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[吳昊澄 Ricky]

> -----Original Message-----
> From: Chris Clayton [mailto:chris2553@googlemail.com]
> Sent: Tuesday, August 04, 2020 4:51 PM
> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> Intel NUC boxes
> 
> 
> 
> On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
> >> -----Original Message-----
> >> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> >> Sent: Tuesday, August 04, 2020 3:49 PM
> >> To: 吳昊澄 Ricky
> >> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com;
> Arnd
> >> Bergmann
> >> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> >> Intel NUC boxes
> >>
> >> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
> >>> Hi Chris,
> >>>
> >>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN,
> OC_POWER_DOWN);
> >>> This register operation saved power under 1mA, so if do not care the 1mA
> >> power we can have a patch to remove it, make compatible with NUC6
> >>> We tested others our card reader that remove it, we did not see any side
> effect
> >>>
> >>> Hi Greg k-h,
> >>>
> >>> Do you have any comments?
> >>
> >> comments on what?  I don't know what you are responding to here, sorry.
> >>
> > Can we have a patch to kernel for NUC6? It may cause more power(1mA) but it
> will have more compatibility
> >
> 
> Ricky,
> 
> I don't understand why you want to completely remove the code that sets up the
> 1mA power saving. That code was there
> even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I
> assume it benefits some of the Realtek card
> readers. Before your patch however, rtsx_pci_init_ocp() was not called for the
> rts5229 reader, but the patch introduced
> an unconditional call to that function into rtsx_pci_init_hw(), which is run for the
> rts5229. That is what now disables
> the card reader.
> 
> Now, I don't know whether other cards are affected, although I don't recall
> seeing any reported as I searched the kernel
> and ubuntu bugzillas for any analysis of the problem. I know this is not what the
> patch I sent does, but having thought
> about it more, seems to me that the simplest fix is to skip the new call to
> rtsx_pci_init_ocp() if the reader is an rts5229.
>

Because we are thinking about if others our card reader that not belong A series(my ocp patch coverage) also on NUC6 platform maybe have the same problem... 
 
> If you agree, I can prepare a patch and send it to you. Whatever the solution is, it
> will also be needed in the stable
> kernels later than 5.0.
> 

OK, I agree your opinion, for now can only patch rts5229 first make NUC6 user can work well

Thank you 

> Chris
> >> greg k-h
> >>
> >> ------Please consider the environment before printing this e-mail.

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

On 04/08/2020 11:46, 吳昊澄 Ricky wrote:
>> -----Original Message-----
>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>> Sent: Tuesday, August 04, 2020 4:51 PM
>> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
>> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>> Intel NUC boxes
>>
>>
>>
>> On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
>>>> -----Original Message-----
>>>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
>>>> Sent: Tuesday, August 04, 2020 3:49 PM
>>>> To: 吳昊澄 Ricky
>>>> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com;
>> Arnd
>>>> Bergmann
>>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>>>> Intel NUC boxes
>>>>
>>>> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
>>>>> Hi Chris,
>>>>>
>>>>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN,
>> OC_POWER_DOWN);
>>>>> This register operation saved power under 1mA, so if do not care the 1mA
>>>> power we can have a patch to remove it, make compatible with NUC6
>>>>> We tested others our card reader that remove it, we did not see any side
>> effect
>>>>>
>>>>> Hi Greg k-h,
>>>>>
>>>>> Do you have any comments?
>>>>
>>>> comments on what?  I don't know what you are responding to here, sorry.
>>>>
>>> Can we have a patch to kernel for NUC6? It may cause more power(1mA) but it
>> will have more compatibility
>>>
>>
>> Ricky,
>>
>> I don't understand why you want to completely remove the code that sets up the
>> 1mA power saving. That code was there
>> even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I
>> assume it benefits some of the Realtek card
>> readers. Before your patch however, rtsx_pci_init_ocp() was not called for the
>> rts5229 reader, but the patch introduced
>> an unconditional call to that function into rtsx_pci_init_hw(), which is run for the
>> rts5229. That is what now disables
>> the card reader.
>>
>> Now, I don't know whether other cards are affected, although I don't recall
>> seeing any reported as I searched the kernel
>> and ubuntu bugzillas for any analysis of the problem. I know this is not what the
>> patch I sent does, but having thought
>> about it more, seems to me that the simplest fix is to skip the new call to
>> rtsx_pci_init_ocp() if the reader is an rts5229.
>>
> 
> Because we are thinking about if others our card reader that not belong A series(my ocp patch coverage) also on NUC6 platform maybe have the same problem... 
>  

OK. What if we do make the new call but only for the card readers that are in the A series? Are they the ones that have
PID_5nnn defines in include/linux/rtsx_pci.h? Or is there another simple way of identifying that a reader is a member of
the A series?

I'm thinking of something like:
static bool rtsx_pci_is_series_A(pcr)
{
	unsigned short device = pcr->pci->device;

	return device == PID524A || device == PID_5249 || device == PID_5250 || device == PID_525A
			|| device == PID_525A || device == PID_5260 || device == PID_5261;
}

then in rtsx_pci_init_hw() change the unconditional call to:

	if rtsx_pci_is_series_A(pcr)
		rtsx_pci_init_ocp();

Does that seem OK?

>> If you agree, I can prepare a patch and send it to you. Whatever the solution is, it
>> will also be needed in the stable
>> kernels later than 5.0.
>>
> 
> OK, I agree your opinion, for now can only patch rts5229 first make NUC6 user can work well
> 
> Thank you 
> 
>> Chris
>>>> greg k-h
>>>>
>>>> ------Please consider the environment before printing this e-mail.

RE: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[吳昊澄 Ricky]

> -----Original Message-----
> From: Chris Clayton [mailto:chris2553@googlemail.com]
> Sent: Tuesday, August 04, 2020 7:52 PM
> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> Intel NUC boxes
> 
> 
> 
> On 04/08/2020 11:46, 吳昊澄 Ricky wrote:
> >> -----Original Message-----
> >> From: Chris Clayton [mailto:chris2553@googlemail.com]
> >> Sent: Tuesday, August 04, 2020 4:51 PM
> >> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
> >> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
> >> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
> >> Intel NUC boxes
> >>
> >>
> >>
> >> On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
> >>>> -----Original Message-----
> >>>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> >>>> Sent: Tuesday, August 04, 2020 3:49 PM
> >>>> To: 吳昊澄 Ricky
> >>>> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com;
> >> Arnd
> >>>> Bergmann
> >>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader
> on
> >>>> Intel NUC boxes
> >>>>
> >>>> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
> >>>>> Hi Chris,
> >>>>>
> >>>>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN,
> >> OC_POWER_DOWN);
> >>>>> This register operation saved power under 1mA, so if do not care the 1mA
> >>>> power we can have a patch to remove it, make compatible with NUC6
> >>>>> We tested others our card reader that remove it, we did not see any side
> >> effect
> >>>>>
> >>>>> Hi Greg k-h,
> >>>>>
> >>>>> Do you have any comments?
> >>>>
> >>>> comments on what?  I don't know what you are responding to here, sorry.
> >>>>
> >>> Can we have a patch to kernel for NUC6? It may cause more power(1mA) but
> it
> >> will have more compatibility
> >>>
> >>
> >> Ricky,
> >>
> >> I don't understand why you want to completely remove the code that sets up
> the
> >> 1mA power saving. That code was there
> >> even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I
> >> assume it benefits some of the Realtek card
> >> readers. Before your patch however, rtsx_pci_init_ocp() was not called for the
> >> rts5229 reader, but the patch introduced
> >> an unconditional call to that function into rtsx_pci_init_hw(), which is run for
> the
> >> rts5229. That is what now disables
> >> the card reader.
> >>
> >> Now, I don't know whether other cards are affected, although I don't recall
> >> seeing any reported as I searched the kernel
> >> and ubuntu bugzillas for any analysis of the problem. I know this is not what
> the
> >> patch I sent does, but having thought
> >> about it more, seems to me that the simplest fix is to skip the new call to
> >> rtsx_pci_init_ocp() if the reader is an rts5229.
> >>
> >
> > Because we are thinking about if others our card reader that not belong A
> series(my ocp patch coverage) also on NUC6 platform maybe have the same
> problem...
> >
> 
> OK. What if we do make the new call but only for the card readers that are in the
> A series? Are they the ones that have
> PID_5nnn defines in include/linux/rtsx_pci.h? Or is there another simple way of
> identifying that a reader is a member of
> the A series?
> 
> I'm thinking of something like:
> static bool rtsx_pci_is_series_A(pcr)
> {
> 	unsigned short device = pcr->pci->device;
> 
> 	return device == PID524A || device == PID_5249 || device == PID_5250 ||
> device == PID_525A
> 			|| device == PID_525A || device == PID_5260 || device ==
> PID_5261;
> }
> 
> then in rtsx_pci_init_hw() change the unconditional call to:
> 
> 	if rtsx_pci_is_series_A(pcr)
> 		rtsx_pci_init_ocp();
> 
> Does that seem OK?
> 
Previously, I want to remove
else {
		/* OC power down */
		rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
			OC_POWER_DOWN);
}
Because in our A-series card Reader we already assigned option->ocp_en to 1 in self init_params() , this is an easy way to fix this problem

> >> If you agree, I can prepare a patch and send it to you. Whatever the solution is,
> it
> >> will also be needed in the stable
> >> kernels later than 5.0.
> >>
> >
> > OK, I agree your opinion, for now can only patch rts5229 first make NUC6 user
> can work well
> >
> > Thank you
> >
> >> Chris
> >>>> greg k-h
> >>>>
> >>>> ------Please consider the environment before printing this e-mail.

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

Thanks, Ricky.

On 05/08/2020 03:35, 吳昊澄 Ricky wrote:
> 
> 
>> -----Original Message-----
>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>> Sent: Tuesday, August 04, 2020 7:52 PM
>> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
>> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>> Intel NUC boxes
>>
>>
>>
>> On 04/08/2020 11:46, 吳昊澄 Ricky wrote:
>>>> -----Original Message-----
>>>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>>>> Sent: Tuesday, August 04, 2020 4:51 PM
>>>> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
>>>> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
>>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>>>> Intel NUC boxes
>>>>
>>>>
>>>>
>>>> On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
>>>>>> -----Original Message-----
>>>>>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
>>>>>> Sent: Tuesday, August 04, 2020 3:49 PM
>>>>>> To: 吳昊澄 Ricky
>>>>>> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com;
>>>> Arnd
>>>>>> Bergmann
>>>>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader
>> on
>>>>>> Intel NUC boxes
>>>>>>
>>>>>> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN,
>>>> OC_POWER_DOWN);
>>>>>>> This register operation saved power under 1mA, so if do not care the 1mA
>>>>>> power we can have a patch to remove it, make compatible with NUC6
>>>>>>> We tested others our card reader that remove it, we did not see any side
>>>> effect
>>>>>>>
>>>>>>> Hi Greg k-h,
>>>>>>>
>>>>>>> Do you have any comments?
>>>>>>
>>>>>> comments on what?  I don't know what you are responding to here, sorry.
>>>>>>
>>>>> Can we have a patch to kernel for NUC6? It may cause more power(1mA) but
>> it
>>>> will have more compatibility
>>>>>
>>>>
>>>> Ricky,
>>>>
>>>> I don't understand why you want to completely remove the code that sets up
>> the
>>>> 1mA power saving. That code was there
>>>> even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I
>>>> assume it benefits some of the Realtek card
>>>> readers. Before your patch however, rtsx_pci_init_ocp() was not called for the
>>>> rts5229 reader, but the patch introduced
>>>> an unconditional call to that function into rtsx_pci_init_hw(), which is run for
>> the
>>>> rts5229. That is what now disables
>>>> the card reader.
>>>>
>>>> Now, I don't know whether other cards are affected, although I don't recall
>>>> seeing any reported as I searched the kernel
>>>> and ubuntu bugzillas for any analysis of the problem. I know this is not what
>> the
>>>> patch I sent does, but having thought
>>>> about it more, seems to me that the simplest fix is to skip the new call to
>>>> rtsx_pci_init_ocp() if the reader is an rts5229.
>>>>
>>>
>>> Because we are thinking about if others our card reader that not belong A
>> series(my ocp patch coverage) also on NUC6 platform maybe have the same
>> problem...
>>>
>>
>> OK. What if we do make the new call but only for the card readers that are in the
>> A series? Are they the ones that have
>> PID_5nnn defines in include/linux/rtsx_pci.h? Or is there another simple way of
>> identifying that a reader is a member of
>> the A series?
>>
>> I'm thinking of something like:
>> static bool rtsx_pci_is_series_A(pcr)
>> {
>> 	unsigned short device = pcr->pci->device;
>>
>> 	return device == PID524A || device == PID_5249 || device == PID_5250 ||
>> device == PID_525A
>> 			|| device == PID_525A || device == PID_5260 || device ==
>> PID_5261;
>> }
>>
>> then in rtsx_pci_init_hw() change the unconditional call to:
>>
>> 	if rtsx_pci_is_series_A(pcr)
>> 		rtsx_pci_init_ocp();
>>
>> Does that seem OK?
>>
> Previously, I want to remove
> else {
> 		/* OC power down */
> 		rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
> 			OC_POWER_DOWN);
> }
> Because in our A-series card Reader we already assigned option->ocp_en to 1 in self init_params() , this is an easy way to fix this problem
> 

Ah, OK. I'll prepare the patch and send it to you once I've tested it.

Chris

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

Hi Ricky

On 05/08/2020 06:51, Chris Clayton wrote:
> Thanks, Ricky.
> 
> On 05/08/2020 03:35, 吳昊澄 Ricky wrote:
>>
>>
>>> -----Original Message-----
>>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>>> Sent: Tuesday, August 04, 2020 7:52 PM
>>> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
>>> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>>> Intel NUC boxes
>>>
>>>
>>>
>>> On 04/08/2020 11:46, 吳昊澄 Ricky wrote:
>>>>> -----Original Message-----
>>>>> From: Chris Clayton [mailto:chris2553@googlemail.com]
>>>>> Sent: Tuesday, August 04, 2020 4:51 PM
>>>>> To: 吳昊澄 Ricky; gregkh@linuxfoundation.org
>>>>> Cc: LKML; rdunlap@infradead.org; philquadra@gmail.com; Arnd Bergmann
>>>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on
>>>>> Intel NUC boxes
>>>>>
>>>>>
>>>>>
>>>>> On 04/08/2020 09:08, 吳昊澄 Ricky wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
>>>>>>> Sent: Tuesday, August 04, 2020 3:49 PM
>>>>>>> To: 吳昊澄 Ricky
>>>>>>> Cc: Chris Clayton; LKML; rdunlap@infradead.org; philquadra@gmail.com;
>>>>> Arnd
>>>>>>> Bergmann
>>>>>>> Subject: Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader
>>> on
>>>>>>> Intel NUC boxes
>>>>>>>
>>>>>>> On Tue, Aug 04, 2020 at 02:44:41AM +0000, 吳昊澄 Ricky wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> rtsx_pci_write_register(pcr, FPDTL, OC_POWER_DOWN,
>>>>> OC_POWER_DOWN);
>>>>>>>> This register operation saved power under 1mA, so if do not care the 1mA
>>>>>>> power we can have a patch to remove it, make compatible with NUC6
>>>>>>>> We tested others our card reader that remove it, we did not see any side
>>>>> effect
>>>>>>>>
>>>>>>>> Hi Greg k-h,
>>>>>>>>
>>>>>>>> Do you have any comments?
>>>>>>>
>>>>>>> comments on what?  I don't know what you are responding to here, sorry.
>>>>>>>
>>>>>> Can we have a patch to kernel for NUC6? It may cause more power(1mA) but
>>> it
>>>>> will have more compatibility
>>>>>>
>>>>>
>>>>> Ricky,
>>>>>
>>>>> I don't understand why you want to completely remove the code that sets up
>>> the
>>>>> 1mA power saving. That code was there
>>>>> even before your patch (bede03a579b3b4a036003c4862cc1baa4ddc351f), so I
>>>>> assume it benefits some of the Realtek card
>>>>> readers. Before your patch however, rtsx_pci_init_ocp() was not called for the
>>>>> rts5229 reader, but the patch introduced
>>>>> an unconditional call to that function into rtsx_pci_init_hw(), which is run for
>>> the
>>>>> rts5229. That is what now disables
>>>>> the card reader.
>>>>>
>>>>> Now, I don't know whether other cards are affected, although I don't recall
>>>>> seeing any reported as I searched the kernel
>>>>> and ubuntu bugzillas for any analysis of the problem. I know this is not what
>>> the
>>>>> patch I sent does, but having thought
>>>>> about it more, seems to me that the simplest fix is to skip the new call to
>>>>> rtsx_pci_init_ocp() if the reader is an rts5229.
>>>>>
>>>>
>>>> Because we are thinking about if others our card reader that not belong A
>>> series(my ocp patch coverage) also on NUC6 platform maybe have the same
>>> problem...
>>>>
>>>
>>> OK. What if we do make the new call but only for the card readers that are in the
>>> A series? Are they the ones that have
>>> PID_5nnn defines in include/linux/rtsx_pci.h? Or is there another simple way of
>>> identifying that a reader is a member of
>>> the A series?
>>>
>>> I'm thinking of something like:
>>> static bool rtsx_pci_is_series_A(pcr)
>>> {
>>> 	unsigned short device = pcr->pci->device;
>>>
>>> 	return device == PID524A || device == PID_5249 || device == PID_5250 ||
>>> device == PID_525A
>>> 			|| device == PID_525A || device == PID_5260 || device ==
>>> PID_5261;
>>> }
>>>
>>> then in rtsx_pci_init_hw() change the unconditional call to:
>>>
>>> 	if rtsx_pci_is_series_A(pcr)
>>> 		rtsx_pci_init_ocp();
>>>
>>> Does that seem OK?
>>>
>> Previously, I want to remove
>> else {
>> 		/* OC power down */
>> 		rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
>> 			OC_POWER_DOWN);
>> }
>> Because in our A-series card Reader we already assigned option->ocp_en to 1 in self init_params() , this is an easy way to fix this problem
>>
> 
> Ah, OK. I'll prepare the patch and send it to you once I've tested it.
> 

Please see the patch included below. As you suggested, it removes the code that does the OC power down on card readers
that are not members of your A series. The patch is against a fresh pull of Linus's tree this morning
(v5.8-2768-g4da9f3302615).

I've tested the resultant kernel on my Intel NUC6CAYH box (which contains an NUC66AYB board) and the rts5229 works fine.
I've also tested it on my laptop which also has a card reader supported by the rtsx_pci driver (an RTL8411B) and that
also works fine.

As I mentioned yesterday, I think it's a candidate for the 5.4 ans 5.7 stable series.

Thanks for your patience!

Chris

Signed-off-by: Chris Clayton <chris2553@googlemail.com>

--- a/drivers/misc/cardreader/rtsx_pcr.c        2020-08-05 07:10:21.752072515 +0100
+++ b/drivers/misc/cardreader/rtsx_pcr.c        2020-08-05 07:11:05.568074278 +0100
@@ -1172,10 +1172,6 @@ void rtsx_pci_init_ocp(struct rtsx_pcr *
                        rtsx_pci_write_register(pcr, REG_OCPGLITCH,
                                SD_OCP_GLITCH_MASK, pcr->hw_param.ocp_glitch);
                        rtsx_pci_enable_ocp(pcr);
-               } else {
-                       /* OC power down */
-                       rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
-                               OC_POWER_DOWN);
                }
        }
 }

Re: PATCH: rtsx_pci driver - don't disable the rts5229 card reader on Intel NUC boxes

[Chris Clayton]

Hi Ricky.

On 05/08/2020 13:48, Chris Clayton wrote:
> Hi Ricky
<snip>
>>
>> Ah, OK. I'll prepare the patch and send it to you once I've tested it.
>>
> 
> Please see the patch included below. As you suggested, it removes the code that does the OC power down on card readers
> that are not members of your A series. The patch is against a fresh pull of Linus's tree this morning
> (v5.8-2768-g4da9f3302615).
> 
> I've tested the resultant kernel on my Intel NUC6CAYH box (which contains an NUC66AYB board) and the rts5229 works fine.
> I've also tested it on my laptop which also has a card reader supported by the rtsx_pci driver (an RTL8411B) and that
> also works fine.
> 
> As I mentioned yesterday, I think it's a candidate for the 5.4 ans 5.7 stable series.
> 
> Thanks for your patience!
> 
> Chris
> 
> Signed-off-by: Chris Clayton <chris2553@googlemail.com>
> 
> --- a/drivers/misc/cardreader/rtsx_pcr.c        2020-08-05 07:10:21.752072515 +0100
> +++ b/drivers/misc/cardreader/rtsx_pcr.c        2020-08-05 07:11:05.568074278 +0100
> @@ -1172,10 +1172,6 @@ void rtsx_pci_init_ocp(struct rtsx_pcr *
>                         rtsx_pci_write_register(pcr, REG_OCPGLITCH,
>                                 SD_OCP_GLITCH_MASK, pcr->hw_param.ocp_glitch);
>                         rtsx_pci_enable_ocp(pcr);
> -               } else {
> -                       /* OC power down */
> -                       rtsx_pci_write_register(pcr, FPDCTL, OC_POWER_DOWN,
> -                               OC_POWER_DOWN);
>                 }
>         }
>  }
> 
> 

Is there some problem with my patch? If you are too busy to deal with it, perhaps I can submit directly it to Greg/Arnd.

Thanks

Chris