Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Thu, Mar 10, 2016 at 04:29:55PM -, yga...@codeaurora.org wrote: >> > On Thu, Mar 10, 2016 at 03:52:54PM -, yga...@codeaurora.org wrote: >> >> > On Wed, Mar 09, 2016 at 08:52:59PM -, yga...@codeaurora.org >> wrote: >> >> >> > On Wed, Mar 09, 2016 at 07:09:49PM -, yga...@codeaurora.org >> >> wrote: >> >> >> >> > On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> >> >> >> >> This patch exposes the ioctl interface for UFS driver via >> SCSI >> >> >> device >> >> >> >> >> ioctl interface. As of now UFS driver would provide the >> ioctl >> >> for >> >> >> >> query >> >> >> >> >> interface to connected UFS device. >> >> >> >> >> >> >> >> >> >> Reviewed-by: Subhash Jadavani >> >> >> >> >> Signed-off-by: Dolev Raviv >> >> >> >> >> Signed-off-by: Gilad Broner >> >> >> >> >> Signed-off-by: Yaniv Gardi >> >> >> >> > >> >> >> >> > What tool is going to use this ioctl? Why does userspcae >> want >> >> to >> >> >> do >> >> >> >> > something "special" with UFS devices? Shouldn't they just be >> >> >> treated >> >> >> >> > like any other normal block device? >> >> >> >> > >> >> >> >> >> >> >> >> Any userspace application can be a tool. >> >> >> >> We already implemented and used a user space application, that >> >> sent >> >> >> >> queries to the UFS devices in order to get information and >> >> >> descriptors. >> >> >> > >> >> >> > But do you want to do with that information? Why does userspace >> >> care? >> >> >> > >> >> >> >> >> >> i don't really understand the subtext of your question - >> >> >> as ANY ioctl cb, we decided to implement the ioctl callback of >> this >> >> scsi >> >> >> device in order to get information like UNIT DESC, DEVICE DESC, >> >> FLAGs, >> >> >> ATTRIBUTES. >> >> >> When dealing with UFS devices, one should be able to read the >> >> >> characteristics of the device. why ? well, why not ? >> >> > >> >> > Why aren't those characteristics just exported as sysfs attributes >> >> under >> >> > control by the UFS controller driver? Why do you need/want an >> ioctl >> >> for >> >> > this? >> >> > >> >> >> >> Hi greg k-h, >> >> >> >> in our code, we used the IOCTL during runtime, in order to determine >> >> some >> >> information about the RPMB well known lun. >> >> with the rpmb lun ID we could then go to /dev/sgX and issue >> >> UFS_IOCTL_QUERY to this lun and get the data - >> >> reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the >> >> QUERY_DESC_IDN_UNIT descriptor. >> >> >> >> this was crucial to the work we do in RPMB. >> > >> > What is RPMB? >> > >> > And again, why not just use sysfs attributes on your host controller >> > device? Why does this have to be a custom ioctl? >> > >> > greg k-h >> > >> >> RPMB is spcial Logical Unit in the UFS device. you can read about it in >> the UFS spec. Greg, you are insisting on sysfs, but i can't implement it >> now, as i don't have the Hardware anymore, or the time. > > If you don't have the hardware, how are you testing this patch? This patch is already tested and verified, and also was much helpful during development. Also, this patch is a few months old and was tested when previous version were uploaded. Currently HW is not available. > > And if you don't have the hardware I guess you don't need this change :) > >> This is a tested and verified code that was accepted and reviewed >> already, >> so i'm not sure what is wrong with this solution, not to say, it's >> already >> implemented, tested and verified. >> hope you are help us push it. > > That's a horrible reason to merge a patch that someone else is going to > have to support for 20+ years with an api that doesn't make much sense. > > If you don't have the hardware, then this isn't needed. But if you do, Even if currently HW is not available it doesn't mean this is not needed in the future again. > then please look into using sysfs for this, as I think that should be > the correct interface here, again, not some random ioctl. > Why sysfs makes more sense than this one ? > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Thu, Mar 10, 2016 at 03:52:54PM -, yga...@codeaurora.org wrote: >> > On Wed, Mar 09, 2016 at 08:52:59PM -, yga...@codeaurora.org wrote: >> >> > On Wed, Mar 09, 2016 at 07:09:49PM -, yga...@codeaurora.org >> wrote: >> >> >> > On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> >> >> >> This patch exposes the ioctl interface for UFS driver via SCSI >> >> device >> >> >> >> ioctl interface. As of now UFS driver would provide the ioctl >> for >> >> >> query >> >> >> >> interface to connected UFS device. >> >> >> >> >> >> >> >> Reviewed-by: Subhash Jadavani >> >> >> >> Signed-off-by: Dolev Raviv >> >> >> >> Signed-off-by: Gilad Broner >> >> >> >> Signed-off-by: Yaniv Gardi >> >> >> > >> >> >> > What tool is going to use this ioctl? Why does userspcae want >> to >> >> do >> >> >> > something "special" with UFS devices? Shouldn't they just be >> >> treated >> >> >> > like any other normal block device? >> >> >> > >> >> >> >> >> >> Any userspace application can be a tool. >> >> >> We already implemented and used a user space application, that >> sent >> >> >> queries to the UFS devices in order to get information and >> >> descriptors. >> >> > >> >> > But do you want to do with that information? Why does userspace >> care? >> >> > >> >> >> >> i don't really understand the subtext of your question - >> >> as ANY ioctl cb, we decided to implement the ioctl callback of this >> scsi >> >> device in order to get information like UNIT DESC, DEVICE DESC, >> FLAGs, >> >> ATTRIBUTES. >> >> When dealing with UFS devices, one should be able to read the >> >> characteristics of the device. why ? well, why not ? >> > >> > Why aren't those characteristics just exported as sysfs attributes >> under >> > control by the UFS controller driver? Why do you need/want an ioctl >> for >> > this? >> > >> >> Hi greg k-h, >> >> in our code, we used the IOCTL during runtime, in order to determine >> some >> information about the RPMB well known lun. >> with the rpmb lun ID we could then go to /dev/sgX and issue >> UFS_IOCTL_QUERY to this lun and get the data - >> reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the >> QUERY_DESC_IDN_UNIT descriptor. >> >> this was crucial to the work we do in RPMB. > > What is RPMB? > > And again, why not just use sysfs attributes on your host controller > device? Why does this have to be a custom ioctl? > > greg k-h > RPMB is spcial Logical Unit in the UFS device. you can read about it in the UFS spec. Greg, you are insisting on sysfs, but i can't implement it now, as i don't have the Hardware anymore, or the time. This is a tested and verified code that was accepted and reviewed already, so i'm not sure what is wrong with this solution, not to say, it's already implemented, tested and verified. hope you are help us push it. thanks, Yaniv -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Wed, Mar 09, 2016 at 08:52:59PM -, yga...@codeaurora.org wrote: >> > On Wed, Mar 09, 2016 at 07:09:49PM -, yga...@codeaurora.org wrote: >> >> > On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> >> >> This patch exposes the ioctl interface for UFS driver via SCSI >> device >> >> >> ioctl interface. As of now UFS driver would provide the ioctl for >> >> query >> >> >> interface to connected UFS device. >> >> >> >> >> >> Reviewed-by: Subhash Jadavani >> >> >> Signed-off-by: Dolev Raviv >> >> >> Signed-off-by: Gilad Broner >> >> >> Signed-off-by: Yaniv Gardi >> >> > >> >> > What tool is going to use this ioctl? Why does userspcae want to >> do >> >> > something "special" with UFS devices? Shouldn't they just be >> treated >> >> > like any other normal block device? >> >> > >> >> >> >> Any userspace application can be a tool. >> >> We already implemented and used a user space application, that sent >> >> queries to the UFS devices in order to get information and >> descriptors. >> > >> > But do you want to do with that information? Why does userspace care? >> > >> >> i don't really understand the subtext of your question - >> as ANY ioctl cb, we decided to implement the ioctl callback of this scsi >> device in order to get information like UNIT DESC, DEVICE DESC, FLAGs, >> ATTRIBUTES. >> When dealing with UFS devices, one should be able to read the >> characteristics of the device. why ? well, why not ? > > Why aren't those characteristics just exported as sysfs attributes under > control by the UFS controller driver? Why do you need/want an ioctl for > this? > Hi greg k-h, in our code, we used the IOCTL during runtime, in order to determine some information about the RPMB well known lun. with the rpmb lun ID we could then go to /dev/sgX and issue UFS_IOCTL_QUERY to this lun and get the data - reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the QUERY_DESC_IDN_UNIT descriptor. this was crucial to the work we do in RPMB. thanks, Yaniv >> during development of this driver, it was useful in many cases to be >> able >> to communicate with the device, by simple IOCTL command, rather than >> implementing ad-hock. > > Do other storage busses have these types of "custom" ioctls for their > bus-type alone? For simple attributes like this, shouldn't you be using > sysfs instead so that it is much easier for userspace tools to get > access to them? > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 06/17] scsi: ufs: separate device and host quirks
Martin, the only way i can avoid the circular dependency is to move the routines from ufs_quirks.c into ufshcd.c. i will upload V8 soon thanks, Yaniv >> "Yaniv" == Yaniv Gardi writes: > > Yaniv> Currently we use the host quirks mechanism in order to handle > Yaniv> both device and host controller quirks. In order to support > Yaniv> various of UFS devices we should separate handling the device > Yaniv> quirks from the host controller's. > > This patch causes a circular dependency: > > depmod: ERROR: Cycle detected: ufshcd -> ufs_quirks -> ufshcd > > Please fix! > > -- > Martin K. PetersenOracle Linux Engineering > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Wed, Mar 09, 2016 at 07:09:49PM -, yga...@codeaurora.org wrote: >> > On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> >> This patch exposes the ioctl interface for UFS driver via SCSI device >> >> ioctl interface. As of now UFS driver would provide the ioctl for >> query >> >> interface to connected UFS device. >> >> >> >> Reviewed-by: Subhash Jadavani >> >> Signed-off-by: Dolev Raviv >> >> Signed-off-by: Gilad Broner >> >> Signed-off-by: Yaniv Gardi >> > >> > What tool is going to use this ioctl? Why does userspcae want to do >> > something "special" with UFS devices? Shouldn't they just be treated >> > like any other normal block device? >> > >> >> Any userspace application can be a tool. >> We already implemented and used a user space application, that sent >> queries to the UFS devices in order to get information and descriptors. > > But do you want to do with that information? Why does userspace care? > i don't really understand the subtext of your question - as ANY ioctl cb, we decided to implement the ioctl callback of this scsi device in order to get information like UNIT DESC, DEVICE DESC, FLAGs, ATTRIBUTES. When dealing with UFS devices, one should be able to read the characteristics of the device. why ? well, why not ? during development of this driver, it was useful in many cases to be able to communicate with the device, by simple IOCTL command, rather than implementing ad-hock. regards, Yaniv >> Not only ioctl interface is a useful way to interact with the device, >> we used it, and found it very helpful in varies cases. > > In what case was it helpful? Why does userspace care about ufs > specifics, it should just treat it like any other block device and not > care at all. > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> This patch exposes the ioctl interface for UFS driver via SCSI device >> ioctl interface. As of now UFS driver would provide the ioctl for query >> interface to connected UFS device. >> >> Reviewed-by: Subhash Jadavani >> Signed-off-by: Dolev Raviv >> Signed-off-by: Gilad Broner >> Signed-off-by: Yaniv Gardi > > What tool is going to use this ioctl? Why does userspcae want to do > something "special" with UFS devices? Shouldn't they just be treated > like any other normal block device? > Any userspace application can be a tool. We already implemented and used a user space application, that sent queries to the UFS devices in order to get information and descriptors. Not only ioctl interface is a useful way to interact with the device, we used it, and found it very helpful in varies cases. hence, this patch. This patch has been already addressed all comments of Arnd Bergman from 5 months ago, and now, re-uploaded again. thanks, Yaniv > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] scsi: ufs: add ioctl interface for query request
> On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote: >> This patch exposes the ioctl interface for UFS driver via SCSI device >> ioctl interface. As of now UFS driver would provide the ioctl for query >> interface to connected UFS device. >> >> Reviewed-by: Subhash Jadavani >> Signed-off-by: Dolev Raviv >> Signed-off-by: Gilad Broner >> Signed-off-by: Yaniv Gardi > > What tool is going to use this ioctl? Why does userspcae want to do > something "special" with UFS devices? Shouldn't they just be treated > like any other normal block device? > Any userspace application can be a tool. We already implemented and used a user space application, that sent queries to the UFS devices in order to get information and descriptors. Not only ioctl interface is a useful way to interact with the device, we used it, and found it very helpful in varies cases. hence, this patch. This patch has been already addressed all comments of Arnd Bergman from 5 months ago, and now, re-uploaded again. thanks, Yaniv > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 03/17] scsi: ufs: implement scsi host timeout handler
> On 03/08/2016 02:01 PM, Hannes Reinecke wrote: >> On 03/08/2016 01:35 PM, Yaniv Gardi wrote: >>> A race condition exists between request requeueing and scsi layer >>> error handling: >>> When UFS driver queuecommand returns a busy status for a request, >>> it will be requeued and its tag will be freed and set to -1. >>> At the same time it is possible that the request will timeout and >>> scsi layer will start error handling for it. The scsi layer reuses >>> the request and its tag to send error related commands to the device, >>> however its tag is no longer valid. >>> As this request was never really sent to the device, there is no >>> point to start error handling with the device. >>> Implement the scsi error handling timeout callback and bypass SCSI >>> error handling for request that were not actually sent to the device. >>> For such requests simply reset the block layer timer. Otherwise, let >>> SCSI layer perform the usual error handling. >>> >>> Reviewed-by: Dolev Raviv >>> Signed-off-by: Gilad Broner >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 36 >>> 1 file changed, 36 insertions(+) >>> >> Having a timeout handler is always a good idea, even though this >> doesn't do anything here. >> Are we sure that the requests will return eventually? >> Does the UFS spec provide for a command abort? >> > In fact, looking at the UFS spec there _is_ a command abort. > I would recommend implementing a task management request UPIO with > type 'ABORT TASK' here for any task found to be pending. > In the end, you might run into a _valid_ timeout, at which point you > really want to abort the command... > but this is not what we'd like to achieve. we don't want to abort a task that was not even dispatched to the UFS driver. in those cases we need to re-queue the request and reset the timer. Hannes, i appreciate your time, but I really don't understand why you insist on coming up with suggestions, when we already implemented one that is working. more over, your solution doesn't fix the race condition which is the reason for this patch. as i don't have HW to test anything at the moment, I think it's better to stick with this solution that also fix the BUG and also was verified and tested. I'd really appreciate your approval for this patch, but, as already said, I can not implement anything else as i can't test it, and also - your suggestion will NOT fix the race condition. i think we shouldn't block the entire 17 patches series because of this patch. not to say - this patch is a BUG fix, so it must be included. thanks, Yaniv > Cheers, > > Hannes- > -- > Dr. Hannes Reinecke Teamlead Storage & Networking > h...@suse.de +49 911 74053 688 > SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton > HRB 21284 (AG Nürnberg) > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 03/17] scsi: ufs: implement scsi host timeout handler
> On 03/08/2016 01:35 PM, Yaniv Gardi wrote:
>> A race condition exists between request requeueing and scsi layer
>> error handling:
>> When UFS driver queuecommand returns a busy status for a request,
>> it will be requeued and its tag will be freed and set to -1.
>> At the same time it is possible that the request will timeout and
>> scsi layer will start error handling for it. The scsi layer reuses
>> the request and its tag to send error related commands to the device,
>> however its tag is no longer valid.
>> As this request was never really sent to the device, there is no
>> point to start error handling with the device.
>> Implement the scsi error handling timeout callback and bypass SCSI
>> error handling for request that were not actually sent to the device.
>> For such requests simply reset the block layer timer. Otherwise, let
>> SCSI layer perform the usual error handling.
>>
>> Reviewed-by: Dolev Raviv
>> Signed-off-by: Gilad Broner
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 36
>> 1 file changed, 36 insertions(+)
>>
> Having a timeout handler is always a good idea, even though this
> doesn't do anything here.
> Are we sure that the requests will return eventually?
> Does the UFS spec provide for a command abort?
>
I'm sorry, but I believe you are wrong in this case.
This timeout handler is doing exactly what we intend it to do,
and also, it is already tested and verified to fix the race condition i
explained a few threads back.
if the scsi command was dispatched to UFS and sent, let the usual SCSI
error handling handle it (return value is BLK_EH_NOT_HANDLED).
but, if the SCSI command was not actually dispatched to UFS driver, then
return BLK_EH_RESET_TIMER and reset the timer, so we don't get
>>unjustified<< timeout, for command that was never dispatched.
also, i will paste again, the race-condition scenario, if anyone is
interested:
--
I will describe a race condition happened to us a while ago, that was
quite difficult to understand and fix.
So, this patch is not about the "busy" returning to the scsi dispatch
routine. it's about the abort triggered after 30 seconds.
imagine a request being queued and sent to the scsi, and then to the ufs.
a timer, initialized to 30 seconds start ticking.
but the request is never sent to the ufs device, as queuecommand() returns
with "SCSI_MLQUEUE_HOST_BUSY" (which is normal behavior).
so, now, the request should be re-queued, and its timer should be reset.
(REMEMBER THIS POINT, let's call it "POINT A")
BUT, a context switch happens before it's actually re-queued, and CPU is
moving to other tasks, doing other things for 30 seconds. yes, sounds
crazy, but it did happen.
NOW, the timeout_handler invoked, and the scsi_abort() routine start
executing, (since 30 seconds passed with no completion).
so far, so good.
but hey, another context switch happens, right at the beginning of
scsi_abort() routine, before anything useful happens. (this is "POINT B")
so, now, context is going back "POINT A", to the blk_requeue_request()
routine, that is calling:
blk_delete_timer(rq); (which does nothing cause the timer already expired)
and then it calls:
blk_queue_end_tag()
which place "-1" in the tag field of the request, marking the request, as
"not tagged yet".
however, a context switch happens again, and we are back in scsi_abort()
routine ("POINT B"), that now needs to abort this very request, but hey,
in the "tag" field, what it sees is tag "-1" which is obviously wrong.
this patch fixes this very rare race condition:
1. upon timeout, blk_rq_timed_out() is called
2. then it calls rq_timed_out_fn() which eventually call
the new callback presented in this patch: "ufshcd_eh_timed_out()"
3. this routine returns with the right flag:
BLK_EH_NOT_HANDLED or BLK_EH_RESET_TIMER.
4. blk_rq_timed_out() checks the returned value:
in case of BLK_EH_HANDLED, it handles normally, meaning, calling scsi_abort()
in case of BLK_EH_RESET_TIMER it starts a new timer, and scsi_abort()
never called.
hope that helps.
regards,
Yaniv
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Teamlead Storage & Networking
> h...@suse.de +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler
Hello, Hannes,
Re-sending
thanks,
Yaniv
>> On 03/03/2016 05:10 PM, yga...@codeaurora.org wrote:
On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>> A race condition exists between request requeueing and scsi layer
>>> error handling:
>>> When UFS driver queuecommand returns a busy status for a request,
>>> it will be requeued and its tag will be freed and set to -1.
>>> At the same time it is possible that the request will timeout and
>>> scsi layer will start error handling for it. The scsi layer reuses
>>> the request and its tag to send error related commands to the
>>> device,
>>> however its tag is no longer valid.
>> Hmm. How can the host return a 'busy' status for a request?
>> From my understanding we have three possibilities:
>>
>> 1) queuecommand returns busy; however, that means that the command
>> has
>> never been send and this issue shouldn't occur
>> 2) The command returns with BUSY status. But in this case it has
>> already
>> been returned, so there cannot be any timeout coming in.
>> 3) The host receives a command with a tag which is already in-use.
>> However, that should have been prevented by the block-layer, which
>> really should ensure that this situation never happens.
>>
>> So either way I look at it, it really looks like a bug and adding a
>> timeout handler will just paper over it.
>> (Not that a timeout handler is a bad idea, in fact I'm convinced
>> that
>> you need one. Just not for this purpose.)
>>
>> So can you elaborate how this 'busy' status comes about?
>> Is the command sent to the device?
>>
>> Cheers,
>>
>> Hannes
>
>
> Hi Hannes,
>
> it's going to be a bit long :)
> I think you are missing the point.
> I will describe a race condition happened to us a while ago, that was
> quite difficult to understand and fix.
> So, this patch is not about the "busy" returning to the scsi dispatch
> routine. it's about the abort triggered after 30 seconds.
>
> imagine a request being queued and sent to the scsi, and then to the
> ufs.
> a timer, initialized to 30 seconds start ticking.
> but the request is never sent to the ufs device, as queuecommand()
> returns
> with "SCSI_MLQUEUE_HOST_BUSY"
> by looking at the code, this could happen, for example:
> err = ufshcd_hold(hba, true);
> if (err) {
> err = SCSI_MLQUEUE_HOST_BUSY;
> goto out;
> }
>
Uuhhh.
You probably should not have pointed me to that piece of code ...
open-coding loops in ufshcd_hold() ... shudder.
(Did I ever review that one? Must've ...)
_Anyway_: sleeping in queuecommand is always a bad idea, as then
precisely those issues you've just described will happen.
Couldn't you just call
ufshcd_hold(hba, false)
instead of
ufshcd_hold(hba, true)
?
The request will be requeued more-or-less immediately, avoiding the
issue with timeout handler kicking in.
And the queue will remain blocked until the ungate work item returns,
at
which point I/O submission will continue.
As the request will be requeued to the head of the queue there won't
be
other I/O competing with tags, so it shouldn't have any adverse
effects.
Wouldn't that work?
Cheers,
Hannes
>>>
>>> Hi Hannes
>>>
>>> This is a bug, and it should be fixed.
>> Oh, definitely agreed. The question is _where_.
>>
>>
>>> if you choose to bypass it, by calling ufshcd_hold(hba, false), not
>>> only
>>> the race condition is still there, and can pop-out at any other point
>>> in
>>> the future, but also, not sure what are the consequences of
>>> ufshcd_hold(hba, false) unstead of "true".
>> Well ... seeing it's your driver, I would've thought _you_ should know
>> ...
>>
>>> so, changing the already tested and working code, (not to return BUSY
>>> from
>>> queuecommand) is not a fix.
>> Hey, I did _not_ suggest not to retury BUSY from queuecommand.
>>
>> I was suggesting this patch:
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 9c1b94b..b9295ad 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1388,7 +1388,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> goto out;
>> }
>>
>> - err = ufshcd_hold(hba, true);
>> + err = ufshcd_hold(hba, false);
>> if (err) {
>> err = SCSI_MLQUEUE_HOST_BUSY;
>> clear_bit_unlock(tag, &hba->lrb_in_use);
>>
>> which, by reading the code, should be avoiding this issue.
>
>
> Hannes,
> we are not trying to avoid returning BUSY from queuecommand().
> On the contrary. By returning BUSY we actually re-queuing the request
> which is
Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler
Hello, Hannes,
Re-sending
thanks,
Yaniv
>> On 03/03/2016 05:10 PM, yga...@codeaurora.org wrote:
On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>>> A race condition exists between request requeueing and scsi layer
>>> error handling:
>>> When UFS driver queuecommand returns a busy status for a request,
>>> it will be requeued and its tag will be freed and set to -1.
>>> At the same time it is possible that the request will timeout and
>>> scsi layer will start error handling for it. The scsi layer reuses
>>> the request and its tag to send error related commands to the
>>> device,
>>> however its tag is no longer valid.
>> Hmm. How can the host return a 'busy' status for a request?
>> From my understanding we have three possibilities:
>>
>> 1) queuecommand returns busy; however, that means that the command
>> has
>> never been send and this issue shouldn't occur
>> 2) The command returns with BUSY status. But in this case it has
>> already
>> been returned, so there cannot be any timeout coming in.
>> 3) The host receives a command with a tag which is already in-use.
>> However, that should have been prevented by the block-layer, which
>> really should ensure that this situation never happens.
>>
>> So either way I look at it, it really looks like a bug and adding a
>> timeout handler will just paper over it.
>> (Not that a timeout handler is a bad idea, in fact I'm convinced
>> that
>> you need one. Just not for this purpose.)
>>
>> So can you elaborate how this 'busy' status comes about?
>> Is the command sent to the device?
>>
>> Cheers,
>>
>> Hannes
>
>
> Hi Hannes,
>
> it's going to be a bit long :)
> I think you are missing the point.
> I will describe a race condition happened to us a while ago, that was
> quite difficult to understand and fix.
> So, this patch is not about the "busy" returning to the scsi dispatch
> routine. it's about the abort triggered after 30 seconds.
>
> imagine a request being queued and sent to the scsi, and then to the
> ufs.
> a timer, initialized to 30 seconds start ticking.
> but the request is never sent to the ufs device, as queuecommand()
> returns
> with "SCSI_MLQUEUE_HOST_BUSY"
> by looking at the code, this could happen, for example:
> err = ufshcd_hold(hba, true);
> if (err) {
> err = SCSI_MLQUEUE_HOST_BUSY;
> goto out;
> }
>
Uuhhh.
You probably should not have pointed me to that piece of code ...
open-coding loops in ufshcd_hold() ... shudder.
(Did I ever review that one? Must've ...)
_Anyway_: sleeping in queuecommand is always a bad idea, as then
precisely those issues you've just described will happen.
Couldn't you just call
ufshcd_hold(hba, false)
instead of
ufshcd_hold(hba, true)
?
The request will be requeued more-or-less immediately, avoiding the
issue with timeout handler kicking in.
And the queue will remain blocked until the ungate work item returns,
at
which point I/O submission will continue.
As the request will be requeued to the head of the queue there won't
be
other I/O competing with tags, so it shouldn't have any adverse
effects.
Wouldn't that work?
Cheers,
Hannes
>>>
>>> Hi Hannes
>>>
>>> This is a bug, and it should be fixed.
>> Oh, definitely agreed. The question is _where_.
>>
>>
>>> if you choose to bypass it, by calling ufshcd_hold(hba, false), not
>>> only
>>> the race condition is still there, and can pop-out at any other point
>>> in
>>> the future, but also, not sure what are the consequences of
>>> ufshcd_hold(hba, false) unstead of "true".
>> Well ... seeing it's your driver, I would've thought _you_ should know
>> ...
>>
>>> so, changing the already tested and working code, (not to return BUSY
>>> from
>>> queuecommand) is not a fix.
>> Hey, I did _not_ suggest not to retury BUSY from queuecommand.
>>
>> I was suggesting this patch:
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 9c1b94b..b9295ad 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1388,7 +1388,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> goto out;
>> }
>>
>> - err = ufshcd_hold(hba, true);
>> + err = ufshcd_hold(hba, false);
>> if (err) {
>> err = SCSI_MLQUEUE_HOST_BUSY;
>> clear_bit_unlock(tag, &hba->lrb_in_use);
>>
>> which, by reading the code, should be avoiding this issue.
>
>
> Hannes,
> we are not trying to avoid returning BUSY from queuecommand().
> On the contrary. By returning BUSY we actually re-queuing the request
> which is
Re: [PATCH v5 15/15] scsi: ufs-qcom: set PA_Local_TX_LCC_Enable before link startup
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote: >> Some UFS devices (and may be host) have issues if LCC is >> enabled. So we are setting PA_Local_TX_LCC_Enable to 0 >> before link startup which will make sure that both host >> and device TX LCC are disabled once link startup is >> completed. >> >> This change also: >> - enables the device ref clock before changing to HS mode >> and disables it if entered to PWM mode >> - adds printouts of testbus debug registers >> > Please make that in two patches; these issues are unrelated and will > just confuse users if one ever need to dig through the git log. > done. in V6 this patch will be divided into 3 patches. 1. LX_LCC 2. device ref clock 3. debug printouts thanks, Yaniv > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > h...@suse.de+49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler
> On 03/03/2016 05:10 PM, yga...@codeaurora.org wrote:
>>> On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> A race condition exists between request requeueing and scsi layer
>> error handling:
>> When UFS driver queuecommand returns a busy status for a request,
>> it will be requeued and its tag will be freed and set to -1.
>> At the same time it is possible that the request will timeout and
>> scsi layer will start error handling for it. The scsi layer reuses
>> the request and its tag to send error related commands to the
>> device,
>> however its tag is no longer valid.
> Hmm. How can the host return a 'busy' status for a request?
> From my understanding we have three possibilities:
>
> 1) queuecommand returns busy; however, that means that the command
> has
> never been send and this issue shouldn't occur
> 2) The command returns with BUSY status. But in this case it has
> already
> been returned, so there cannot be any timeout coming in.
> 3) The host receives a command with a tag which is already in-use.
> However, that should have been prevented by the block-layer, which
> really should ensure that this situation never happens.
>
> So either way I look at it, it really looks like a bug and adding a
> timeout handler will just paper over it.
> (Not that a timeout handler is a bad idea, in fact I'm convinced that
> you need one. Just not for this purpose.)
>
> So can you elaborate how this 'busy' status comes about?
> Is the command sent to the device?
>
> Cheers,
>
> Hannes
Hi Hannes,
it's going to be a bit long :)
I think you are missing the point.
I will describe a race condition happened to us a while ago, that was
quite difficult to understand and fix.
So, this patch is not about the "busy" returning to the scsi dispatch
routine. it's about the abort triggered after 30 seconds.
imagine a request being queued and sent to the scsi, and then to the
ufs.
a timer, initialized to 30 seconds start ticking.
but the request is never sent to the ufs device, as queuecommand()
returns
with "SCSI_MLQUEUE_HOST_BUSY"
by looking at the code, this could happen, for example:
err = ufshcd_hold(hba, true);
if (err) {
err = SCSI_MLQUEUE_HOST_BUSY;
goto out;
}
>>> Uuhhh.
>>> You probably should not have pointed me to that piece of code ...
>>> open-coding loops in ufshcd_hold() ... shudder.
>>> (Did I ever review that one? Must've ...)
>>> _Anyway_: sleeping in queuecommand is always a bad idea, as then
>>> precisely those issues you've just described will happen.
>>>
>>> Couldn't you just call
>>> ufshcd_hold(hba, false)
>>> instead of
>>> ufshcd_hold(hba, true)
>>> ?
>>> The request will be requeued more-or-less immediately, avoiding the
>>> issue with timeout handler kicking in.
>>> And the queue will remain blocked until the ungate work item returns,
>>> at
>>> which point I/O submission will continue.
>>> As the request will be requeued to the head of the queue there won't be
>>> other I/O competing with tags, so it shouldn't have any adverse
>>> effects.
>>>
>>> Wouldn't that work?
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>> Hi Hannes
>>
>> This is a bug, and it should be fixed.
> Oh, definitely agreed. The question is _where_.
>
>
>> if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
>> the race condition is still there, and can pop-out at any other point in
>> the future, but also, not sure what are the consequences of
>> ufshcd_hold(hba, false) unstead of "true".
> Well ... seeing it's your driver, I would've thought _you_ should know ...
>
>> so, changing the already tested and working code, (not to return BUSY
>> from
>> queuecommand) is not a fix.
> Hey, I did _not_ suggest not to retury BUSY from queuecommand.
>
> I was suggesting this patch:
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9c1b94b..b9295ad 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1388,7 +1388,7 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
> goto out;
> }
>
> - err = ufshcd_hold(hba, true);
> + err = ufshcd_hold(hba, false);
> if (err) {
> err = SCSI_MLQUEUE_HOST_BUSY;
> clear_bit_unlock(tag, &hba->lrb_in_use);
>
> which, by reading the code, should be avoiding this issue.
Hannes,
we are not trying to avoid returning BUSY from queuecommand().
On the contrary. By returning BUSY we actually re-queuing the request
which is exactly what we need to do.
your patch doesn't fix the race condition.
thanks,
Yaniv
> I was just asking you if you could give this patch a spin and see if it
> works. If not (for whatever r
Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler
> On 03/01/2016 09:25 PM, yga...@codeaurora.org wrote:
>>> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
A race condition exists between request requeueing and scsi layer
error handling:
When UFS driver queuecommand returns a busy status for a request,
it will be requeued and its tag will be freed and set to -1.
At the same time it is possible that the request will timeout and
scsi layer will start error handling for it. The scsi layer reuses
the request and its tag to send error related commands to the device,
however its tag is no longer valid.
>>> Hmm. How can the host return a 'busy' status for a request?
>>> From my understanding we have three possibilities:
>>>
>>> 1) queuecommand returns busy; however, that means that the command has
>>> never been send and this issue shouldn't occur
>>> 2) The command returns with BUSY status. But in this case it has
>>> already
>>> been returned, so there cannot be any timeout coming in.
>>> 3) The host receives a command with a tag which is already in-use.
>>> However, that should have been prevented by the block-layer, which
>>> really should ensure that this situation never happens.
>>>
>>> So either way I look at it, it really looks like a bug and adding a
>>> timeout handler will just paper over it.
>>> (Not that a timeout handler is a bad idea, in fact I'm convinced that
>>> you need one. Just not for this purpose.)
>>>
>>> So can you elaborate how this 'busy' status comes about?
>>> Is the command sent to the device?
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>>
>> Hi Hannes,
>>
>> it's going to be a bit long :)
>> I think you are missing the point.
>> I will describe a race condition happened to us a while ago, that was
>> quite difficult to understand and fix.
>> So, this patch is not about the "busy" returning to the scsi dispatch
>> routine. it's about the abort triggered after 30 seconds.
>>
>> imagine a request being queued and sent to the scsi, and then to the
>> ufs.
>> a timer, initialized to 30 seconds start ticking.
>> but the request is never sent to the ufs device, as queuecommand()
>> returns
>> with "SCSI_MLQUEUE_HOST_BUSY"
>> by looking at the code, this could happen, for example:
>> err = ufshcd_hold(hba, true);
>> if (err) {
>> err = SCSI_MLQUEUE_HOST_BUSY;
>> goto out;
>> }
>>
> Uuhhh.
> You probably should not have pointed me to that piece of code ...
> open-coding loops in ufshcd_hold() ... shudder.
> (Did I ever review that one? Must've ...)
> _Anyway_: sleeping in queuecommand is always a bad idea, as then
> precisely those issues you've just described will happen.
>
> Couldn't you just call
> ufshcd_hold(hba, false)
> instead of
> ufshcd_hold(hba, true)
> ?
> The request will be requeued more-or-less immediately, avoiding the
> issue with timeout handler kicking in.
> And the queue will remain blocked until the ungate work item returns, at
> which point I/O submission will continue.
> As the request will be requeued to the head of the queue there won't be
> other I/O competing with tags, so it shouldn't have any adverse effects.
>
> Wouldn't that work?
>
> Cheers,
>
> Hannes
Hi Hannes
This is a bug, and it should be fixed.
if you choose to bypass it, by calling ufshcd_hold(hba, false), not only
the race condition is still there, and can pop-out at any other point in
the future, but also, not sure what are the consequences of
ufshcd_hold(hba, false) unstead of "true".
so, changing the already tested and working code, (not to return BUSY from
queuecommand) is not a fix.
I strongly recommend we upstream this race-condition fix.
thanks,
Yaniv
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.de+49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/15] scsi: ufs: verify hba controller hce reg value
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> Sometimes due to hw issues it takes some time to the
>> host controller register to update. In order to verify the register
>> has updated, a polling is done until its value is set.
>>
>> In addition the functions ufshcd_hba_stop() and
>> ufshcd_wait_for_register() was updated with an additional input
>> parameter, indicating the timeout between reads will
>> be done by sleeping or spinning the cpu.
>>
>> Signed-off-by: Raviv Shvili
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 53
>> ---
>> drivers/scsi/ufs/ufshcd.h | 12 +++
>> 2 files changed, 35 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 3400ceb..80031e6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -240,11 +240,13 @@ static inline void ufshcd_disable_irq(struct
>> ufs_hba *hba)
>> * @val - wait condition
>> * @interval_us - polling interval in microsecs
>> * @timeout_ms - timeout in millisecs
>> + * @can_sleep - perform sleep or just spin
>> *
>> * Returns -ETIMEDOUT on error, zero on success
>> */
>> -static int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32
>> mask,
>> -u32 val, unsigned long interval_us, unsigned long timeout_ms)
>> +int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>> +u32 val, unsigned long interval_us,
>> +unsigned long timeout_ms, bool can_sleep)
>> {
>> int err = 0;
>> unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>> @@ -253,9 +255,10 @@ static int ufshcd_wait_for_register(struct ufs_hba
>> *hba, u32 reg, u32 mask,
>> val = val & mask;
>>
>> while ((ufshcd_readl(hba, reg) & mask) != val) {
>> -/* wakeup within 50us of expiry */
>> -usleep_range(interval_us, interval_us + 50);
>> -
>> +if (can_sleep)
>> +usleep_range(interval_us, interval_us + 50);
>> +else
>> +udelay(interval_us);
>> if (time_after(jiffies, timeout)) {
>> if ((ufshcd_readl(hba, reg) & mask) != val)
>> err = -ETIMEDOUT;
>> @@ -1459,7 +1462,7 @@ ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>> */
>> err = ufshcd_wait_for_register(hba,
>> REG_UTP_TRANSFER_REQ_DOOR_BELL,
>> -mask, ~mask, 1000, 1000);
>> +mask, ~mask, 1000, 1000, true);
>>
>> return err;
>> }
>> @@ -2815,6 +2818,23 @@ out:
>> }
>>
>> /**
>> + * ufshcd_hba_stop - Send controller to reset state
>> + * @hba: per adapter instance
>> + * @can_sleep: perform sleep or just spin
>> + */
>> +static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>> +{
>> +int err;
>> +
>> +ufshcd_writel(hba, CONTROLLER_DISABLE, REG_CONTROLLER_ENABLE);
>> +err = ufshcd_wait_for_register(hba, REG_CONTROLLER_ENABLE,
>> +CONTROLLER_ENABLE, CONTROLLER_DISABLE,
>> +10, 1, can_sleep);
>> +if (err)
>> +dev_err(hba->dev, "%s: Controller disable failed\n", __func__);
>> +}
>> +
> Shouldn't you return an error here?
> If the controller disable failed you probably need a hard reset or
> something, otherwise I would assume that every other command from that
> point on will not work as expected.
>
> Cheers,
>
> Hannes
Hello Hannes,
The original routine signature is:
void ufshcd_hba_stop(struct ufs_hba *hba);
as you can see, no return value, the reason is simple - there is nothing
we can do if writing to the register fails.
all we wanted to do here, was to add a graceful time to change the
register value. also, we decided to add error msg in case the value is not
change within this timeout.
We can not do anything else, not to say, return error, as there is no
error handling in such case.
So, as far as i see it, we only improved the already exists logic, by
adding some graceful time to the register change, and also, by adding an
error message that was absent before.
thanks,
Yaniv
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.de+49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/15] scsi: ufs: implement scsi host timeout handler
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> A race condition exists between request requeueing and scsi layer
>> error handling:
>> When UFS driver queuecommand returns a busy status for a request,
>> it will be requeued and its tag will be freed and set to -1.
>> At the same time it is possible that the request will timeout and
>> scsi layer will start error handling for it. The scsi layer reuses
>> the request and its tag to send error related commands to the device,
>> however its tag is no longer valid.
> Hmm. How can the host return a 'busy' status for a request?
> From my understanding we have three possibilities:
>
> 1) queuecommand returns busy; however, that means that the command has
> never been send and this issue shouldn't occur
> 2) The command returns with BUSY status. But in this case it has already
> been returned, so there cannot be any timeout coming in.
> 3) The host receives a command with a tag which is already in-use.
> However, that should have been prevented by the block-layer, which
> really should ensure that this situation never happens.
>
> So either way I look at it, it really looks like a bug and adding a
> timeout handler will just paper over it.
> (Not that a timeout handler is a bad idea, in fact I'm convinced that
> you need one. Just not for this purpose.)
>
> So can you elaborate how this 'busy' status comes about?
> Is the command sent to the device?
>
> Cheers,
>
> Hannes
Hi Hannes,
it's going to be a bit long :)
I think you are missing the point.
I will describe a race condition happened to us a while ago, that was
quite difficult to understand and fix.
So, this patch is not about the "busy" returning to the scsi dispatch
routine. it's about the abort triggered after 30 seconds.
imagine a request being queued and sent to the scsi, and then to the ufs.
a timer, initialized to 30 seconds start ticking.
but the request is never sent to the ufs device, as queuecommand() returns
with "SCSI_MLQUEUE_HOST_BUSY"
by looking at the code, this could happen, for example:
err = ufshcd_hold(hba, true);
if (err) {
err = SCSI_MLQUEUE_HOST_BUSY;
goto out;
}
so, now, the request should be re-queued, and its timer should be reset.
(REMEMBER THIS POINT, let's call it "POINT A")
BUT, a context switch happens before it's actually re-queued, and CPU is
moving to other tasks, doing other things for 30 seconds. yes, sounds
crazy, but it did happen.
NOW, the timeout_handler invoked, and the scsi_abort() routine start
executing, (since 30 seconds passed with no completion).
so far, so good.
but hey, another context switch happens, right at the beginning of
scsi_abort() routine, before anything useful happens. (this is "POINT B")
so, now, context is going back "POINT A", to the blk_requeue_request()
routine, that is calling:
blk_delete_timer(rq); (which does nothing cause the timer already expired)
and then it calls:
blk_queue_end_tag()
which place "-1" in the tag field of the request, marking the request, as
"not tagged yet".
however, a context switch happens again, and we are back in scsi_abort()
routine ("POINT B"), that now needs to abort this very request, but hey,
in the "tag" field, what it sees is tag "-1" which is obviously wrong.
this patch fixes this very rare race condition:
1. upon timeout, blk_rq_timed_out() is called
2. then it calls rq_timed_out_fn() which eventually call
the new callback presented in this patch: "ufshcd_eh_timed_out()"
3. this routine returns with the right flag:
BLK_EH_NOT_HANDLED or BLK_EH_RESET_TIMER.
4. blk_rq_timed_out() checks the returned value:
in case of BLK_EH_HANDLED, it handles normally, meaning, calling scsi_abort()
in case of BLK_EH_RESET_TIMER it starts a new timer, and scsi_abort()
never called.
hope that helps.
regards,
Yaniv
> --
> Dr. Hannes Reinecke zSeries & Storage
> h...@suse.de+49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 05/15] scsi: ufs: add support to read device and string descriptors
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> This change adds support to read device descriptor and string descriptor
>> from a UFS device
>>
>> Reviewed-by: Gilad Broner
>> Signed-off-by: Raviv Shvili
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufs.h| 1 +
>> drivers/scsi/ufs/ufshcd.c | 88
>> ++-
>> drivers/scsi/ufs/ufshcd.h | 7
>> 3 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 54a16ce..aacb235 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -43,6 +43,7 @@
>> #define GENERAL_UPIU_REQUEST_SIZE 32
>> #define QUERY_DESC_MAX_SIZE 255
>> #define QUERY_DESC_MIN_SIZE 2
>> +#define QUERY_DESC_HDR_SIZE 2
>> #define QUERY_OSF_SIZE(GENERAL_UPIU_REQUEST_SIZE - \
>> (sizeof(struct utp_upiu_header)))
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 80031e6..e2ed415 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -39,7 +39,7 @@
>>
>> #include
>> #include
>> -
>> +#include
>> #include
>> #include "ufshcd.h"
>> #include "unipro.h"
>> @@ -232,6 +232,16 @@ static inline void ufshcd_disable_irq(struct
>> ufs_hba *hba)
>> }
>> }
>>
>> +/* replace non-printable or non-ASCII characters with spaces */
>> +static inline void ufshcd_remove_non_printable(char *val)
>> +{
>> +if (!val)
>> +return;
>> +
>> +if (*val < 0x20 || *val > 0x7e)
>> +*val = ' ';
>> +}
>> +
>> /*
>> * ufshcd_wait_for_register - wait for register value to change
>> * @hba - per-adapter interface
>> @@ -2021,6 +2031,82 @@ static inline int ufshcd_read_power_desc(struct
>> ufs_hba *hba,
>> return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf, size);
>> }
>>
>> +int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>> +{
>> +return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf, size);
>> +}
>> +EXPORT_SYMBOL(ufshcd_read_device_desc);
>> +
>> +/**
>> + * ufshcd_read_string_desc - read string descriptor
>> + * @hba: pointer to adapter instance
>> + * @desc_index: descriptor index
>> + * @buf: pointer to buffer where descriptor would be read
>> + * @size: size of buf
>> + * @ascii: if true convert from unicode to ascii characters
>> + *
>> + * Return 0 in case of success, non-zero otherwise
>> + */
>> +int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index, u8
>> *buf,
>> +u32 size, bool ascii)
>> +{
>> +int err = 0;
>> +
>> +err = ufshcd_read_desc(hba,
>> +QUERY_DESC_IDN_STRING, desc_index, buf, size);
>> +
>> +if (err) {
>> +dev_err(hba->dev, "%s: reading String Desc failed after %d
>> retries.
>> err = %d\n",
>> +__func__, QUERY_REQ_RETRIES, err);
>> +goto out;
>> +}
>> +
>> +if (ascii) {
>> +int desc_len;
>> +int ascii_len;
>> +int i;
>> +char *buff_ascii;
>> +
>> +desc_len = buf[0];
>> +/* remove header and divide by 2 to move from UTF16 to UTF8 */
>> +ascii_len = (desc_len - QUERY_DESC_HDR_SIZE) / 2 + 1;
>> +if (size < ascii_len + QUERY_DESC_HDR_SIZE) {
>> +dev_err(hba->dev, "%s: buffer allocated size is too
>> small\n",
>> +__func__);
>> +err = -ENOMEM;
>> +goto out;
>> +}
>> +
>> +buff_ascii = kmalloc(ascii_len, GFP_KERNEL);
>> +if (!buff_ascii) {
>> +err = -ENOMEM;
>> +goto out_free_buff;
>> +}
>> +
>> +/*
>> + * the descriptor contains string in UTF16 format
>> + * we need to convert to utf-8 so it can be displayed
>> + */
>> +utf16s_to_utf8s((wchar_t *)&buf[QUERY_DESC_HDR_SIZE],
>> +desc_len - QUERY_DESC_HDR_SIZE,
>> +UTF16_BIG_ENDIAN, buff_ascii, ascii_len);
>> +
>> +/* replace non-printable or non-ASCII characters with spaces */
>> +for (i = 0; i < ascii_len; i++)
>> +ufshcd_remove_non_printable(&buff_ascii[i]);
>> +
>> +memset(buf + QUERY_DESC_HDR_SIZE, 0,
>> +size - QUERY_DESC_HDR_SIZE);
>> +memcpy(buf + QUERY_DESC_HDR_SIZE, buff_ascii, ascii_len);
>> +buf[QUERY_DESC_LENGTH_OFFSET] = ascii_len + QUERY_DESC_HDR_SIZE;
>> +out_free_buff:
>> +kfree(buff_ascii);
>> +}
>> +out:
>> +return err;
>> +}
>> +EXPORT_SYMBOL(ufshcd_read_string_desc);
>> +
>> /**
>> * ufshcd_read_unit_desc_param - read the specified unit descriptor
>> parameter
>> * @hba: Pointer to adapter instance
>> diff --git a/drivers/scsi/uf
Re: [PATCH v5 08/15] scsi: ufs: make error handling bit faster
> On 02/28/2016 09:32 PM, Yaniv Gardi wrote:
>> UFS driver's error handler forcefully tries to clear all the pending
>> requests. For each pending request in the queue, it waits 1 sec for it
>> to get cleared. If we have multiple requests in the queue then it's
>> possible that we might end up waiting for those many seconds before
>> resetting the host. But note that resetting host would any way clear
>> all the pending requests from the hardware. Hence this change skips
>> the forceful clear of the pending requests if we are anyway going to
>> reset the host (for fatal errors).
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 155
>> +-
>> 1 file changed, 112 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 987cf27..dc096f1 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -133,9 +133,11 @@ enum {
>> /* UFSHCD UIC layer error flags */
>> enum {
>> UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
>> -UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
>> -UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
>> -UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
>> +UFSHCD_UIC_DL_NAC_RECEIVED_ERROR = (1 << 1), /* Data link layer error
>> */
>> +UFSHCD_UIC_DL_TCx_REPLAY_ERROR = (1 << 2), /* Data link layer error */
>> +UFSHCD_UIC_NL_ERROR = (1 << 3), /* Network layer error */
>> +UFSHCD_UIC_TL_ERROR = (1 << 4), /* Transport Layer error */
>> +UFSHCD_UIC_DME_ERROR = (1 << 5), /* DME error */
>> };
>>
>> /* Interrupt configuration options */
>> @@ -3465,31 +3467,18 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba
>> *hba, u32 intr_status)
>> }
>>
>> /**
>> - * ufshcd_transfer_req_compl - handle SCSI and query command completion
>> + * __ufshcd_transfer_req_compl - handle SCSI and query command
>> completion
>> * @hba: per adapter instance
>> + * @completed_reqs: requests to complete
>> */
>> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>> +static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>> +unsigned long completed_reqs)
>> {
>> struct ufshcd_lrb *lrbp;
>> struct scsi_cmnd *cmd;
>> -unsigned long completed_reqs;
>> -u32 tr_doorbell;
>> int result;
>> int index;
>>
>> -/* Resetting interrupt aggregation counters first and reading the
>> - * DOOR_BELL afterward allows us to handle all the completed requests.
>> - * In order to prevent other interrupts starvation the DB is read once
>> - * after reset. The down side of this solution is the possibility of
>> - * false interrupt if device completes another request after resetting
>> - * aggregation and before reading the DB.
>> - */
>> -if (ufshcd_is_intr_aggr_allowed(hba))
>> -ufshcd_reset_intr_aggr(hba);
>> -
>> -tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> -completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>> -
>> for_each_set_bit(index, &completed_reqs, hba->nutrs) {
>> lrbp = &hba->lrb[index];
>> cmd = lrbp->cmd;
>> @@ -3519,6 +3508,31 @@ static void ufshcd_transfer_req_compl(struct
>> ufs_hba *hba)
>> }
>>
>> /**
>> + * ufshcd_transfer_req_compl - handle SCSI and query command completion
>> + * @hba: per adapter instance
>> + */
>> +static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>> +{
>> +unsigned long completed_reqs;
>> +u32 tr_doorbell;
>> +
>> +/* Resetting interrupt aggregation counters first and reading the
>> + * DOOR_BELL afterward allows us to handle all the completed requests.
>> + * In order to prevent other interrupts starvation the DB is read once
>> + * after reset. The down side of this solution is the possibility of
>> + * false interrupt if device completes another request after resetting
>> + * aggregation and before reading the DB.
>> + */
>> +if (ufshcd_is_intr_aggr_allowed(hba))
>> +ufshcd_reset_intr_aggr(hba);
>> +
>> +tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> +completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>> +
>> +__ufshcd_transfer_req_compl(hba, completed_reqs);
>> +}
>> +
>> +/**
>> * ufshcd_disable_ee - disable exception event
>> * @hba: per-adapter instance
>> * @mask: exception event to disable
>> @@ -3773,6 +3787,13 @@ out:
>> return;
>> }
>>
>> +/* Complete requests that have door-bell cleared */
>> +static void ufshcd_complete_requests(struct ufs_hba *hba)
>> +{
>> +ufshcd_transfer_req_compl(hba);
>> +ufshcd_tmc_handler(hba);
>> +}
>> +
>> /**
>> * ufshcd_err_handler - handle UFS errors that require s/w attention
>> * @work: pointer to work structure
>> @@ -3785,6 +3806,7 @@ static void ufshcd_err_handler(struct work_st
RE: [v4 05/14] scsi: ufs: separate device and host quirks
>> Currently we use the host quirks mechanism in order to
>> handle both device and host controller quirks.
>> In order to support various of UFS devices we should separate
>> handling the device quirks from the host controller's.
>>
>> Reviewed-by: Gilad Broner
>> Signed-off-by: Raviv Shvili
>> Signed-off-by: Yaniv Gardi
>
>
>
> I would like to see this patch to be split into two
> 1. support for device descriptor read
> 2. support quirks
>
>
Hello Tom,
accepted your comment, and uploaded V5 in which patch v4 5/14 is divided
into 2 separate patches.
hope you can grant with your reviewed-by or acked-by
regards,
Yaniv
>>
>> ---
>> drivers/scsi/ufs/Makefile | 2 +-
>> drivers/scsi/ufs/ufs.h| 32 +++
>> drivers/scsi/ufs/ufs_quirks.c | 100 ++
>> drivers/scsi/ufs/ufs_quirks.h | 124
>> ++
>> drivers/scsi/ufs/ufshcd.c | 90 +-
>> drivers/scsi/ufs/ufshcd.h | 10
>> 6 files changed, 356 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/scsi/ufs/ufs_quirks.c
>> create mode 100644 drivers/scsi/ufs/ufs_quirks.h
>>
>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
>> index 8303bcc..8570d41 100644
>> --- a/drivers/scsi/ufs/Makefile
>> +++ b/drivers/scsi/ufs/Makefile
>> @@ -1,5 +1,5 @@
>> # UFSHCD makefile
>> obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>> -obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
>> +obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o ufs_quirks.o
>> obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>> obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 42c459a..8dd608b 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -43,6 +43,7 @@
>> #define GENERAL_UPIU_REQUEST_SIZE 32
>> #define QUERY_DESC_MAX_SIZE 255
>> #define QUERY_DESC_MIN_SIZE 2
>> +#define QUERY_DESC_HDR_SIZE 2
>> #define QUERY_OSF_SIZE(GENERAL_UPIU_REQUEST_SIZE - \
>> (sizeof(struct utp_upiu_header)))
>>
>> @@ -195,6 +196,37 @@ enum unit_desc_param {
>> UNIT_DESC_PARAM_LARGE_UNIT_SIZE_M1 = 0x22,
>> };
>>
>> +/* Device descriptor parameters offsets in bytes*/
>> +enum device_desc_param {
>> +DEVICE_DESC_PARAM_LEN = 0x0,
>> +DEVICE_DESC_PARAM_TYPE = 0x1,
>> +DEVICE_DESC_PARAM_DEVICE_TYPE = 0x2,
>> +DEVICE_DESC_PARAM_DEVICE_CLASS = 0x3,
>> +DEVICE_DESC_PARAM_DEVICE_SUB_CLASS = 0x4,
>> +DEVICE_DESC_PARAM_PRTCL = 0x5,
>> +DEVICE_DESC_PARAM_NUM_LU= 0x6,
>> +DEVICE_DESC_PARAM_NUM_WLU = 0x7,
>> +DEVICE_DESC_PARAM_BOOT_ENBL = 0x8,
>> +DEVICE_DESC_PARAM_DESC_ACCSS_ENBL = 0x9,
>> +DEVICE_DESC_PARAM_INIT_PWR_MODE = 0xA,
>> +DEVICE_DESC_PARAM_HIGH_PR_LUN = 0xB,
>> +DEVICE_DESC_PARAM_SEC_RMV_TYPE = 0xC,
>> +DEVICE_DESC_PARAM_SEC_LU= 0xD,
>> +DEVICE_DESC_PARAM_BKOP_TERM_LT = 0xE,
>> +DEVICE_DESC_PARAM_ACTVE_ICC_LVL = 0xF,
>> +DEVICE_DESC_PARAM_SPEC_VER = 0x10,
>> +DEVICE_DESC_PARAM_MANF_DATE = 0x12,
>> +DEVICE_DESC_PARAM_MANF_NAME = 0x14,
>> +DEVICE_DESC_PARAM_PRDCT_NAME= 0x15,
>> +DEVICE_DESC_PARAM_SN= 0x16,
>> +DEVICE_DESC_PARAM_OEM_ID= 0x17,
>> +DEVICE_DESC_PARAM_MANF_ID = 0x18,
>> +DEVICE_DESC_PARAM_UD_OFFSET = 0x1A,
>> +DEVICE_DESC_PARAM_UD_LEN= 0x1B,
>> +DEVICE_DESC_PARAM_RTT_CAP = 0x1C,
>> +DEVICE_DESC_PARAM_FRQ_RTC = 0x1D,
>> +};
>> +
>
> This list is correct only for UFS 2.0, UFS 1.0 has different offsets.
>
>> /*
>> * Logical Unit Write Protect
>> * 00h: LU not write protected
>> diff --git a/drivers/scsi/ufs/ufs_quirks.c
>> b/drivers/scsi/ufs/ufs_quirks.c
>> new file mode 100644
>> index 000..476ed01
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/ufs_quirks.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright (c) 2013-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include "ufshcd.h"
>> +#include "ufs_quirks.h"
>> +
>> +static struct ufs_dev_fix ufs_fixups[] = {
>> +/* UFS cards deviations table */
>> +UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL,
>> UFS_DEVICE_N
Re: [PATCH v6 07/15] scsi: ufs: set REQUEST_SENSE command size to 18 bytes
> On 10/28/2015 02:13 PM, Yaniv Gardi wrote: >> According to UFS device specification REQUEST_SENSE command can >> only report back up to 18 bytes of data. >> >> Reviewed-by: Dolev Raviv >> Signed-off-by: Gilad Broner >> Signed-off-by: Yaniv Gardi >> > Really? The spec only says that the inline sense code is 18 bytes; > if you issue a request sense directly there is not such limitation. > thanks Hannes, so for now, i will exclude this patch from the upcoming V7 regards, Yaniv > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > h...@suse.de+49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 01/15] scsi: ufs: clear UTRD, UPIU req and rsp before new transfers
> > >> +cdb_len = min_t(unsigned short, lrbp->cmd->cmd_len, MAX_CDB_SIZE); >> +memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd, cdb_len); >> +if (cdb_len < MAX_CDB_SIZE) >> +memset(ucd_req_ptr->sc.cdb + cdb_len, 0, >> + (MAX_CDB_SIZE - cdb_len)); > It's just 16 bytes, setting all to zero prior to copy will be as good as > this if statement > memset(ucd_req_ptr->sc.cdb, 0, MAX_CDB_SIZE); > i agree. will be modified in V7 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/17] scsi: ufs-qcom: fix compilation warnings
> On 10/27/2015 03:10 AM, yga...@codeaurora.org wrote: >>> On 10/26/2015 08:41 AM, Yaniv Gardi wrote: Tnis patch fixes the following compilation warnings: ...ufs-qcom.c:1201:40: warning: incorrect type in argument 1 (different address spaces) ...ufs-qcom.c:1201:40: expected void const *ptr ...ufs-qcom.c:1201:40: got void [noderef] *dev_ref_clk_ctrl_mmio ...ufs-qcom.c:1207:53: warning: incorrect type in argument 1 (different address spaces) ...ufs-qcom.c:1207:53: expected void const *ptr ...ufs-qcom.c:1207:53: got void [noderef] *dev_ref_clk_ctrl_mmio Signed-off-by: Yaniv Gardi --- >>> What version of sparse do you use? You shouldn't need to do this. >>> >> i just updated my sparse to the newest - >> so i'm using sparse 0.4.4 >> and the command i use is: >> make ARCH=arm C=1 CF="-Wsparse-all" >> and i still get all the warnings. >> with my patch, the are no warnings. >> > > That is not the latest version of sparse. I'm not sure when sparse was > updated, but I have the git version which shows v0.5.0-51-ga53cea28f0db > Stephen, is this something you insist on? i think the patch can stay as it is. please let me know, as i wouldn't like this issue to block the entire patch Yaniv > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers
>> 2015-10-25 23:40 GMT+09:00 :
2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
> Performing several writes to UFS host controller registers has
> no gurrantee of ordering, so we must make sure register writes
> to setup request list base address etc. are performed before the
> run/stop register is enabled.
> In addition, when setting up a task request, we must make sure
> the updating of descriptors takes places before ringing the
> doorbell, similarly to setting up a transfer request.
>
> Signed-off-by: Gilad Broner
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufshcd.c | 21 +++--
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index fc2a52d..298511a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32
> reg)
> * 1 UTRLRDY
> * 2 UTMRLRDY
> * 3 UCRDY
> -* 4 HEI
> -* 5 DEI
> -* 6-7 reserved
> +* 4-7 reserved
> */
> - return (((reg) & (0xFF)) >> 1) ^ (0x07);
> + return ((reg & 0xFF) >> 1) ^ 0x07;
> }
>
> /**
> @@ -2726,7 +2724,7 @@ out:
> * To bring UFS host controller to operational state,
> * 1. Enable required interrupts
> * 2. Configure interrupt aggregation
> - * 3. Program UTRL and UTMRL base addres
> + * 3. Program UTRL and UTMRL base address
> * 4. Configure run-stop-registers
> *
> * Returns 0 on success, non-zero value on failure
> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
> ufs_hba *hba)
> REG_UTP_TASK_REQ_LIST_BASE_H);
>
> /*
> +* Make sure base address and interrupt setup are updated
> before
> +* enabling the run/stop registers below.
> +*/
> + wmb();
> +
> + /*
> * UCRDY, UTMRLDY and UTRLRDY bits must be 1
> -* DEI, HEI bits must be 0
> */
> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
> if (!(ufshcd_get_lists_status(reg))) {
> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
> *hba, int lun_id, int task_id,
>
> /* send command to the controller */
> __set_bit(free_slot, &hba->outstanding_tasks);
> +
> + /* Make sure descriptors are ready before ringing the task
> doorbell */
> + wmb();
> +
> ufshcd_writel(hba, 1 << free_slot,
> REG_UTP_TASK_REQ_DOOR_BELL);
> + /* Make sure that doorbell is committed immediately */
> + wmb();
Is this wmb() after ringing tm doorbell is needed?
>>>
>>> Well, Mita, in the case of DB register (Request DB and TASK DB), every
>>> write operation to the DB should be barrier, as if not, out of order
>>> writing to this register might cause inconsistency in its value, and
>>> thus,
>>> un-handled requests/tasks.
>>
>> I couldn't fully understand why out of order writing to TASK DB
>> register causes inconsistency.
>> In the TASK request completion (ufshcd_tmc_handler), TASK DB register
>> is read before handling finished requests, so it ensures that all
>> write operations for TASK DB have been performed.
>>
>
> thanks, Mita
> your explanation makes sense.
> It will be fixed in v4.
>
sorry, it will fixed in v5.
Yaniv
>
>
> spin_unlock_irqrestore(host->host_lock, flags);
>
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
> member of Code Aurora Forum, hosted by The Linux Foundation
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/17] scsi: ufs-qcom: fix compilation warnings
> On 10/26/2015 08:41 AM, Yaniv Gardi wrote:
>> Tnis patch fixes the following compilation warnings:
>> ...ufs-qcom.c:1201:40:
>> warning: incorrect type in argument 1 (different address spaces)
>> ...ufs-qcom.c:1201:40:
>> expected void const *ptr
>> ...ufs-qcom.c:1201:40:
>> got void [noderef] *dev_ref_clk_ctrl_mmio
>> ...ufs-qcom.c:1207:53:
>> warning: incorrect type in argument 1 (different address spaces)
>> ...ufs-qcom.c:1207:53:
>> expected void const *ptr
>> ...ufs-qcom.c:1207:53:
>> got void [noderef] *dev_ref_clk_ctrl_mmio
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>
> What version of sparse do you use? You shouldn't need to do this.
>
i just updated my sparse to the newest -
so i'm using sparse 0.4.4
and the command i use is:
make ARCH=arm C=1 CF="-Wsparse-all"
and i still get all the warnings.
with my patch, the are no warnings.
>> drivers/scsi/ufs/ufs-qcom.c | 9 +
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 72b0ef7..b57f88a 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -1226,11 +1226,12 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>> if (res) {
>> host->dev_ref_clk_ctrl_mmio =
>> devm_ioremap_resource(dev, res);
>> -if (IS_ERR(host->dev_ref_clk_ctrl_mmio)) {
>> -dev_warn(dev,
>> -"%s: could not map
>> dev_ref_clk_ctrl_mmio, err %ld\n",
>> +if (IS_ERR((__force void const *)
>> + host->dev_ref_clk_ctrl_mmio)) {
>> +dev_warn(dev, "%s: could not map
>> dev_ref_clk_ctrl_mmio, err %ld\n",
>> __func__,
>> -PTR_ERR(host->dev_ref_clk_ctrl_mmio));
>> +PTR_ERR((__force void const *)
>> + host->dev_ref_clk_ctrl_mmio));
>> host->dev_ref_clk_ctrl_mmio = NULL;
>> }
>> host->dev_ref_clk_en_mask = BIT(5);
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 16/17] scsi: ufs: add delay before putting UFS rails in low power modes
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>> We put the UFS device in sleep state & UFS link in hibern8 state during
>> runtime suspaned. After this we put all the UFS rails in low power
>> modes immediately but it seems some devices may still draw more than
>> sleep current from UFS rails (especially from VCCQ rail) atleast for
>> 500us.
>> To avoid this situation, this change adds 2ms delay before putting
>> these UFS rails in LPM mode.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 20b4c0e..786df28 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5694,6 +5694,15 @@ out:
>> static void ufshcd_vreg_set_lpm(struct ufs_hba *hba)
>> {
>> /*
>> +* It seems some UFS devices may keep drawing more than sleep
>> current
>> +* (atleast for 500us) from UFS rails (especially from VCCQ
>> rail).
>> +* To avoid this situation, add 2ms delay before putting these
>> UFS
>> +* rails in LPM mode.
>> +*/
>> + if (!ufshcd_is_link_active(hba))
>> + usleep_range(2000, 2100);
>> +
>
> Shouldn't we define dev_quirks for this?
as you suggested, in v2 i added a device quirk for that.
>
>> + /*
>> * If UFS device is either in UFS_Sleep turn off VCC rail to
>> save some
>> * power.
>> *
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/17] scsi: ufs: tune UniPro parameters to optimize hibern8 exit time
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>> Optimal values of local UniPro parameters like PA_Hibern8Time &
>> PA_TActivate can help reduce the hibern8 exit latency. If both host and
>> device supports UniPro ver1.6 or later, these parameters will be
>> automatically tuned during link startup itself. But if either host or
>> device doesn't support UniPro ver 1.6 or later, we have to manually
>> tune them. But to keep manual tuning logic simple, we will only do
>> manual tuning if local unipro version doesn't support ver1.6 or later.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 121
>> ++
>> drivers/scsi/ufs/ufshcd.h | 1 +
>> drivers/scsi/ufs/ufshci.h | 2 +
>> drivers/scsi/ufs/unipro.h | 21
>> 4 files changed, 145 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index a8659a9..0938d6c 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -596,6 +596,34 @@ static inline int ufshcd_is_hba_active(struct
>> ufs_hba *hba)
>> return (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) & 0x1) ? 0 : 1;
>> }
>>
>> +u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba)
>> +{
>> + /* HCI version 1.0 and 1.1 supports UniPro 1.41 */
>> + if ((hba->ufs_version == UFSHCI_VERSION_10) ||
>> + (hba->ufs_version == UFSHCI_VERSION_11))
>> + return UFS_UNIPRO_VER_1_41;
>> + else
>> + return UFS_UNIPRO_VER_1_6;
>> +}
>> +EXPORT_SYMBOL(ufshcd_get_local_unipro_ver);
>> +
>> +static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>> +{
>> + /*
>> +* If both host and device support UniPro ver1.6 or later, PA
>> layer
>> +* parameters tuning happens during link startup itself.
>> +*
>> +* We can manually tune PA layer parameters if either host or
>> device
>> +* doesn't support UniPro ver 1.6 or later. But to keep manual
>> tuning
>> +* logic simple, we will only do manual tuning if local unipro
>> version
>> +* doesn't support ver1.6 or later.
>> +*/
>> + if (ufshcd_get_local_unipro_ver(hba) < UFS_UNIPRO_VER_1_6)
>> + return true;
>> + else
>> + return false;
>> +}
>> +
>> static void ufshcd_ungate_work(struct work_struct *work)
>> {
>> int ret;
>> @@ -4826,6 +4854,98 @@ out:
>> }
>>
>> /**
>> + * ufshcd_tune_pa_tactivate - Tunes PA_TActivate of local UniPro
>> + * @hba: per-adapter instance
>> + *
>> + * PA_TActivate parameter can be tuned manually if UniPro version is
>> less than
>> + * 1.61. PA_TActivate needs to be greater than or equal to peerM-PHY's
>> + * RX_MIN_ACTIVATETIME_CAPABILITY attribute. This optimal value can
>> help reduce
>> + * the hibern8 exit latency.
>> + *
>> + * Returns zero on success, non-zero error value on failure.
>> + */
>> +static int ufshcd_tune_pa_tactivate(struct ufs_hba *hba)
>> +{
>> + int ret = 0;
>> + u32 peer_rx_min_activatetime = 0, tuned_pa_tactivate;
>> +
>> + if (!ufshcd_is_unipro_pa_params_tuning_req(hba))
>> + return 0;
>
> This ufshcd_tune_pa_tactivate() is called only when
> ufshcd_is_unipro_pa_params_tuning_req() returns true. Is this
> second check needed?
>
i could think of having this second check for any future call to
ufshcd_tune_pa_tactivate() which is not protected with a wrapping check
like in this case.
but i guess every call to ufshcd_tune_pa_tactivate()
will use ufshcd_tune_unipro_params() so we are good.
i will remove the 2nd check
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 12/17] scsi: ufs: add retry for query descriptors
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>> Query commands have 100ms timeout and it may timeout if they are
>> issued in parallel to ongoing read/write SCSI commands, this change
>> adds the retry (max: 10) in case command timeouts.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 48
>> ---
>> 1 file changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index a649250..528e46e 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1906,21 +1906,7 @@ static int ufshcd_query_attr_retry(struct ufs_hba
>> *hba,
>> return ret;
>> }
>>
>> -/**
>> - * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>> - * hba: per-adapter instance
>> - * opcode: attribute opcode
>> - * idn: attribute idn to access
>> - * index: index field
>> - * selector: selector field
>> - * desc_buf: the buffer that contains the descriptor
>> - * buf_len: length parameter passed to the device
>> - *
>> - * Returns 0 for success, non-zero in case of failure.
>> - * The buf_len parameter will contain, on return, the length parameter
>> - * received on the response.
>> - */
>> -static int ufshcd_query_descriptor(struct ufs_hba *hba,
>> +static int __ufshcd_query_descriptor(struct ufs_hba *hba,
>> enum query_opcode opcode, enum desc_idn idn, u8
>> index,
>> u8 selector, u8 *desc_buf, int *buf_len)
>> {
>> @@ -1985,6 +1971,38 @@ out:
>> }
>>
>> /**
>> + * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>> + * hba: per-adapter instance
>> + * opcode: attribute opcode
>> + * idn: attribute idn to access
>> + * index: index field
>> + * selector: selector field
>> + * desc_buf: the buffer that contains the descriptor
>> + * buf_len: length parameter passed to the device
>> + *
>> + * Returns 0 for success, non-zero in case of failure.
>> + * The buf_len parameter will contain, on return, the length parameter
>> + * received on the response.
>> + */
>> +int ufshcd_query_descriptor(struct ufs_hba *hba,
>> + enum query_opcode opcode, enum desc_idn idn, u8
>> index,
>> + u8 selector, u8 *desc_buf, int *buf_len)
>> +{
>> + int err;
>> + int retries;
>> +
>> + for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>> + err = __ufshcd_query_descriptor(hba, opcode, idn, index,
>> + selector, desc_buf,
>> buf_len);
>> + if (!err || err == -EINVAL)
>> + break;
>> + }
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL(ufshcd_query_descriptor);
>
> You introduced query flag and attribute APIs for retry version with
> '_retry' suffix.
> This function retries but doesn't have '_retry' suffix.
> Should we have consistent function names for these query APIs?
>
sure. will fix this in v2
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 08/17] scsi: ufs: split broken LCC quirk
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>> Currently when UFSHCD_BROKEN_LCC quirk is defined, LCC is getting
>> disabled on both host and device side but there could be a need
>> where we don't want to disable the LCC on both side hence this change
>> splits the quirk in 2 parts one for host and one for device.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 0803a89..411ec17 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -3048,6 +3048,11 @@ static int ufshcd_disable_tx_lcc(struct ufs_hba
>> *hba, bool peer)
>> return err;
>> }
>>
>> +static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba)
>> +{
>> + return ufshcd_disable_tx_lcc(hba, false);
>> +}
>> +
>> static inline int ufshcd_disable_device_tx_lcc(struct ufs_hba *hba)
>> {
>> return ufshcd_disable_tx_lcc(hba, true);
>> @@ -3095,6 +3100,12 @@ static int ufshcd_link_startup(struct ufs_hba
>> *hba)
>> goto out;
>> }
>>
>> + if (hba->dev_quirks & UFS_DEVICE_QUIRK_BROKEN_LCC) {
>> + ret = ufshcd_disable_host_tx_lcc(hba);
>> + if (ret)
>> + goto out;
>> + }
>> +
>
> This dev_quirks is checked just after link startup. But dev_quirk is
> determined after ufshcd_complete_dev_init(). Is this quirk correctly
> be applied?
good comment :)
since there is no need in this quirk anymore (it is never set),
luckily we can totally remove the quirk.
i will remove this patch totally in v2
>
>> /* Include any host controller configuration via UIC commands */
>> ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
>> if (ret)
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 07/17] scsi: ufs: separate device and host quirks
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>
>> diff --git a/drivers/scsi/ufs/ufs_quirks.c
>> b/drivers/scsi/ufs/ufs_quirks.c
>> new file mode 100644
>> index 000..b649bbf
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/ufs_quirks.c
>> @@ -0,0 +1,101 @@
>> +/*
>> + * Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include "ufshcd.h"
>> +#include "ufs_quirks.h"
>> +
>> +
>
> Single blank line is enough.
done
>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e5a876c..0803a89 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -39,9 +39,10 @@
>>
>> #include
>> #include
>> -
>> +#include
>> #include
>> #include "ufshcd.h"
>> +#include "ufs_quirks.h"
>> #include "unipro.h"
>> #define UFSHCD_REQ_SENSE_SIZE 18
>>
>> @@ -259,6 +260,16 @@ static inline void ufshcd_disable_irq(struct
>> ufs_hba *hba)
>> }
>> }
>>
>> +/* replace non-printable or non-ASCII characters with spaces */
>> +static inline void ufshcd_remove_non_printable(char *val)
>> +{
>> + if (!val)
>> + return;
>> +
>> + if (*val < 0x20 || *val > 0x7e)
>> + *val = ' ';
>> +}
>> +
>> /*
>> * ufshcd_wait_for_register - wait for register value to change
>> * @hba - per-adapter interface
>> @@ -2052,6 +2063,82 @@ static inline int ufshcd_read_power_desc(struct
>> ufs_hba *hba,
>> return ufshcd_read_desc(hba, QUERY_DESC_IDN_POWER, 0, buf,
>> size);
>> }
>>
>> +int ufshcd_read_device_desc(struct ufs_hba *hba, u8 *buf, u32 size)
>> +{
>> + return ufshcd_read_desc(hba, QUERY_DESC_IDN_DEVICE, 0, buf,
>> size);
>> +}
>> +EXPORT_SYMBOL(ufshcd_read_device_desc);
>> +
>> +/**
>> + * ufshcd_read_string_desc - read string descriptor
>> + * @hba: pointer to adapter instance
>> + * @desc_index: descriptor index
>> + * @buf: pointer to buffer where descriptor would be read
>> + * @size: size of buf
>> + * @ascii: if true convert from unicode to ascii characters
>> + *
>> + * Return 0 in case of success, non-zero otherwise
>> + */
>> +int ufshcd_read_string_desc(struct ufs_hba *hba, int desc_index, u8
>> *buf,
>> + u32 size, bool ascii)
>> +{
>> + int err = 0;
>> +
>> + err = ufshcd_read_desc(hba,
>> + QUERY_DESC_IDN_STRING, desc_index, buf,
>> size);
>> +
>> + if (err) {
>> + dev_err(hba->dev, "%s: reading String Desc failed after
>> %d retries. err = %d\n",
>> + __func__, QUERY_REQ_RETRIES, err);
>> + goto out;
>> + }
>
> I actually tried this patch and ufshcd_read_desc() always returns
> -EINVAL due to the following check in the end of ufshcd_read_desc().
>
> if (ret || (buff_len < ufs_query_desc_max_size[desc_id]) ||
> (desc_buf[QUERY_DESC_LENGTH_OFFSET] !=
> ufs_query_desc_max_size[desc_id])
> || (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id)) {
> ...
> if (!ret)
> ret = -EINVAL;
>
> Could you also include a fix fir ufshcd_read_desc()?
i'm not sure i understand what should be fixed.
if the check you mentioned implies that something is wrong,
we shouldn't ignore the error. otherwise we might exceed the buf
boundaries for example.
can you elaborate what is that you think we should fix ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 06/17] scsi :ufs: verify hba controller hce reg value
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi : >> Sometimes due to hw issues it takes some time to the >> host controller register to update. In order to verify the register >> has updated, a polling is done until its value is set. >> >> In addition the functions ufshcd_hba_stop() and >> ufshcd_wait_for_register() was updated with an additional input >> parameter, indicating the timeout between reads will >> be done by sleeping or spinning the cpu. >> >> Signed-off-by: Raviv Shvili >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 54 >> --- >> drivers/scsi/ufs/ufshcd.h | 12 +++ >> 2 files changed, 35 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 6171da8..e5a876c 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -267,11 +267,12 @@ static inline void ufshcd_disable_irq(struct >> ufs_hba *hba) >> * @val - wait condition >> * @interval_us - polling interval in microsecs >> * @timeout_ms - timeout in millisecs >> - * >> + * @can_sleep - perform sleep or just spin >> * Returns -ETIMEDOUT on error, zero on success >> */ > > We usually put a blank line between @argument description and > return value description. > will be fixed in v2. thanks -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 02/17] scsi: ufs: add option to change default UFS power management level
> 2015-09-13 23:52 GMT+09:00 Yaniv Gardi :
>> UFS device and link can be put in multiple different low power modes
>> hence UFS driver supports multiple different low power modes.
>> By default UFS driver selects the default (optimal) low power mode
>> (which gives moderate power savings and have relatively less enter
>> and exit latencies) but we might have to tune this default power
>> mode for different chipset platforms to meet the low power
>> requirements/goals. Hence this patch adds option to change default
>> UFS low power mode (level).
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>
> ...
>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 3196197..1d26e49 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -1195,11 +1195,12 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>> if (res) {
>> host->dev_ref_clk_ctrl_mmio =
>> devm_ioremap_resource(dev, res);
>> - if (IS_ERR(host->dev_ref_clk_ctrl_mmio)) {
>> - dev_warn(dev,
>> - "%s: could not map
>> dev_ref_clk_ctrl_mmio, err %ld\n",
>> + if (IS_ERR((__force void const *)
>> + host->dev_ref_clk_ctrl_mmio)) {
>> + dev_warn(dev, "%s: could not map
>> dev_ref_clk_ctrl_mmio, err %ld\n",
>> __func__,
>> -
>> PTR_ERR(host->dev_ref_clk_ctrl_mmio));
>> + PTR_ERR((__force void const *)
>> +
>> host->dev_ref_clk_ctrl_mmio));
>> host->dev_ref_clk_ctrl_mmio = NULL;
>> }
>> host->dev_ref_clk_en_mask = BIT(5);
>
> This change looks unrelated. Should this be belong to other patch?
>
correct, it's minor change,
but i will separate it to a different patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers
> 2015-10-25 23:40 GMT+09:00 :
>>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
Performing several writes to UFS host controller registers has
no gurrantee of ordering, so we must make sure register writes
to setup request list base address etc. are performed before the
run/stop register is enabled.
In addition, when setting up a task request, we must make sure
the updating of descriptors takes places before ringing the
doorbell, similarly to setting up a transfer request.
Signed-off-by: Gilad Broner
Signed-off-by: Yaniv Gardi
---
drivers/scsi/ufs/ufshcd.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index fc2a52d..298511a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32
reg)
* 1 UTRLRDY
* 2 UTMRLRDY
* 3 UCRDY
-* 4 HEI
-* 5 DEI
-* 6-7 reserved
+* 4-7 reserved
*/
- return (((reg) & (0xFF)) >> 1) ^ (0x07);
+ return ((reg & 0xFF) >> 1) ^ 0x07;
}
/**
@@ -2726,7 +2724,7 @@ out:
* To bring UFS host controller to operational state,
* 1. Enable required interrupts
* 2. Configure interrupt aggregation
- * 3. Program UTRL and UTMRL base addres
+ * 3. Program UTRL and UTMRL base address
* 4. Configure run-stop-registers
*
* Returns 0 on success, non-zero value on failure
@@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
ufs_hba *hba)
REG_UTP_TASK_REQ_LIST_BASE_H);
/*
+* Make sure base address and interrupt setup are updated
before
+* enabling the run/stop registers below.
+*/
+ wmb();
+
+ /*
* UCRDY, UTMRLDY and UTRLRDY bits must be 1
-* DEI, HEI bits must be 0
*/
reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
if (!(ufshcd_get_lists_status(reg))) {
@@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
*hba, int lun_id, int task_id,
/* send command to the controller */
__set_bit(free_slot, &hba->outstanding_tasks);
+
+ /* Make sure descriptors are ready before ringing the task
doorbell */
+ wmb();
+
ufshcd_writel(hba, 1 << free_slot,
REG_UTP_TASK_REQ_DOOR_BELL);
+ /* Make sure that doorbell is committed immediately */
+ wmb();
>>>
>>> Is this wmb() after ringing tm doorbell is needed?
>>
>> Well, Mita, in the case of DB register (Request DB and TASK DB), every
>> write operation to the DB should be barrier, as if not, out of order
>> writing to this register might cause inconsistency in its value, and
>> thus,
>> un-handled requests/tasks.
>
> I couldn't fully understand why out of order writing to TASK DB
> register causes inconsistency.
> In the TASK request completion (ufshcd_tmc_handler), TASK DB register
> is read before handling finished requests, so it ensures that all
> write operations for TASK DB have been performed.
>
thanks, Mita
your explanation makes sense.
It will be fixed in v4.
>>>
spin_unlock_irqrestore(host->host_lock, flags);
--
1.8.5.2
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 13/15] scsi: ufs: add missing memory barriers
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
>> Performing several writes to UFS host controller registers has
>> no gurrantee of ordering, so we must make sure register writes
>> to setup request list base address etc. are performed before the
>> run/stop register is enabled.
>> In addition, when setting up a task request, we must make sure
>> the updating of descriptors takes places before ringing the
>> doorbell, similarly to setting up a transfer request.
>>
>> Signed-off-by: Gilad Broner
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 21 +++--
>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index fc2a52d..298511a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg)
>> * 1 UTRLRDY
>> * 2 UTMRLRDY
>> * 3 UCRDY
>> -* 4 HEI
>> -* 5 DEI
>> -* 6-7 reserved
>> +* 4-7 reserved
>> */
>> - return (((reg) & (0xFF)) >> 1) ^ (0x07);
>> + return ((reg & 0xFF) >> 1) ^ 0x07;
>> }
>>
>> /**
>> @@ -2726,7 +2724,7 @@ out:
>> * To bring UFS host controller to operational state,
>> * 1. Enable required interrupts
>> * 2. Configure interrupt aggregation
>> - * 3. Program UTRL and UTMRL base addres
>> + * 3. Program UTRL and UTMRL base address
>> * 4. Configure run-stop-registers
>> *
>> * Returns 0 on success, non-zero value on failure
>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct
>> ufs_hba *hba)
>> REG_UTP_TASK_REQ_LIST_BASE_H);
>>
>> /*
>> +* Make sure base address and interrupt setup are updated before
>> +* enabling the run/stop registers below.
>> +*/
>> + wmb();
>> +
>> + /*
>> * UCRDY, UTMRLDY and UTRLRDY bits must be 1
>> -* DEI, HEI bits must be 0
>> */
>> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS);
>> if (!(ufshcd_get_lists_status(reg))) {
>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>> *hba, int lun_id, int task_id,
>>
>> /* send command to the controller */
>> __set_bit(free_slot, &hba->outstanding_tasks);
>> +
>> + /* Make sure descriptors are ready before ringing the task
>> doorbell */
>> + wmb();
>> +
>> ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL);
>> + /* Make sure that doorbell is committed immediately */
>> + wmb();
>
> Is this wmb() after ringing tm doorbell is needed?
Well, Mita, in the case of DB register (Request DB and TASK DB), every
write operation to the DB should be barrier, as if not, out of order
writing to this register might cause inconsistency in its value, and thus,
un-handled requests/tasks.
>
>>
>> spin_unlock_irqrestore(host->host_lock, flags);
>>
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 12/15] scsi: ufs: reduce the interrupts for power mode change requests
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
>> DME commands such as Hibern8 enter/exit and gear switch generate 2
>> completion interrupts, one for confirmation that command is received
>> by local UniPro and 2nd one is the final confirmation after
>> communication
>> with remote UniPro. Currently both of these completions are registered
>> as interrupt events which is not quite necessary and instead we can
>> just wait for the interrupt of 2nd completion, this should reduce
>> the number of interrupts and could reduce the unnecessary CPU wakeups
>> to handle extra interrupts.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 41
>> +++--
>> 1 file changed, 27 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index f2d4301..fc2a52d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -986,13 +986,15 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
>> struct uic_command *uic_cmd)
>> * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
>> * @hba: per adapter instance
>> * @uic_cmd: UIC command
>> + * @completion: initialize the completion only if this is set to true
>> *
>> * Identical to ufshcd_send_uic_cmd() expect mutex. Must be called
>> * with mutex held and host_lock locked.
>> * Returns 0 only if success.
>> */
>> static int
>> -__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>> +__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
>> + bool completion)
>> {
>> if (!ufshcd_ready_for_uic_cmd(hba)) {
>> dev_err(hba->dev,
>> @@ -1000,7 +1002,8 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct
>> uic_command *uic_cmd)
>> return -EIO;
>> }
>>
>> - init_completion(&uic_cmd->done);
>> + if (completion)
>> + init_completion(&uic_cmd->done);
>>
>> ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>>
>> @@ -1025,7 +1028,7 @@ ufshcd_send_uic_cmd(struct ufs_hba *hba, struct
>> uic_command *uic_cmd)
>> ufshcd_add_delay_before_dme_cmd(hba);
>>
>> spin_lock_irqsave(hba->host->host_lock, flags);
>> - ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
>> + ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>> if (!ret)
>> ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
>> @@ -2346,6 +2349,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>> unsigned long flags;
>> u8 status;
>> int ret;
>> + bool reenable_intr = false;
>>
>> mutex_lock(&hba->uic_cmd_mutex);
>> init_completion(&uic_async_done);
>> @@ -2353,15 +2357,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>>
>> spin_lock_irqsave(hba->host->host_lock, flags);
>> hba->uic_async_done = &uic_async_done;
>> - ret = __ufshcd_send_uic_cmd(hba, cmd);
>> - spin_unlock_irqrestore(hba->host->host_lock, flags);
>> - if (ret) {
>> - dev_err(hba->dev,
>> - "pwr ctrl cmd 0x%x with mode 0x%x uic error
>> %d\n",
>> - cmd->command, cmd->argument3, ret);
>> - goto out;
>> + if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL)
>> {
>> + ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
>> + /*
>> +* Make sure UIC command completion interrupt is
>> disabled before
>> +* issuing UIC command.
>> +*/
>> + wmb();
>> + reenable_intr = true;
>> }
>> - ret = ufshcd_wait_for_uic_cmd(hba, cmd);
>> + ret = __ufshcd_send_uic_cmd(hba, cmd, false);
>> + spin_unlock_irqrestore(hba->host->host_lock, flags);
>> if (ret) {
>> dev_err(hba->dev,
>> "pwr ctrl cmd 0x%x with mode 0x%x uic error
>> %d\n",
>> @@ -2387,7 +2393,10 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
>> *hba, struct uic_command *cmd)
>> }
>> out:
>> spin_lock_irqsave(hba->host->host_lock, flags);
>> + hba->active_uic_cmd = NULL;
>> hba->uic_async_done = NULL;
>> + if (reenable_intr)
>> + ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>> mutex_unlock(&hba->uic_cmd_mutex);
>>
>> @@ -3812,16 +3821,20 @@ static void ufshcd_sl_intr(struct ufs_hba *hba,
>> u32 intr_status)
>> */
>> static irqreturn_t ufshcd_intr(int irq, void *__hba)
>> {
>> - u32 intr_status;
>> + u32 intr_status, enabled_intr_status;
>> irqreturn_t retval = IRQ_NONE;
>> struct ufs_hba *hba = __hba;
>>
>> spin_lock(hba->host->host_lock);
>> intr_st
Re: [PATCH v3 09/15] scsi: ufs: add retries for hibern8 enter
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
>> If hibern8 enter command fails then UFS link state may be unknown which
>> may result into timeout of all the commands issued after failure.
>>
>> This change does 2 things (for pre-defined number of retry counts) after
>> hibern8 enter failure:
>> 1. Recovers the UFS link to active state
>> 2. If link is recovered to active state, tries to put the UFS link in
>>hibern8 enter again until retry count expires.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 26 --
>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 8d5bdf0..5800b08 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -75,6 +75,9 @@
>> /* maximum number of link-startup retries */
>> #define DME_LINKSTARTUP_RETRIES 3
>>
>> +/* Maximum retries for Hibern8 enter */
>> +#define UIC_HIBERN8_ENTER_RETRIES 3
>> +
>> /* maximum number of reset retries before giving up */
>> #define MAX_HOST_RESET_RETRIES 5
>>
>> @@ -2389,13 +2392,32 @@ out:
>> return ret;
>> }
>>
>> -static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> +static int __ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> {
>> + int ret;
>> struct uic_command uic_cmd = {0};
>>
>> uic_cmd.command = UIC_CMD_DME_HIBER_ENTER;
>> + ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>> +
>> + if (ret)
>> + dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d",
>> + __func__, ret);
>
> This format string is not terminated with '\n'.
> (I found several same issues in ufshcd.c)
correct. will be modified in the next v4.
>
>> +
>> + return ret;
>> +}
>> +
>> +static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
>> +{
>> + int ret = 0, retries;
>>
>
> 'ret' will soon be initialized in the loop anyway, so it is unnecessary
> to initialized here.
correct but, in order to be on the safe side, better to leave this
initialization here, as if UIC_HIBERN8_ENTER_RETRIES is modified to 0, we
will never get into the for-loop, and thus, ret will be un initialized.
>
>> - return ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
>> + for (retries = UIC_HIBERN8_ENTER_RETRIES; retries > 0;
>> retries--) {
>> + ret = __ufshcd_uic_hibern8_enter(hba);
>> + if (!ret || ret == -ENOLINK)
>> + goto out;
>> + }
>> +out:
>> + return ret;
>> }
>>
>> static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 05/15] scsi: ufs: increase fDeviceInit query response timeout
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
>> fDeviceInit query response time for some devices is too long that
>> default
>> query request timeout of 100ms may not be enough. Experiments show that
>> fDeviceInit response sometimes takes 500ms so to be on safer side this
>> change sets the timeout to 600ms. Without this change, we might
>> unnecessarily have to retry fDeviceInit query requests multiple times
>> and
>> each query request timeout prints one error message.
>>
>> Signed-off-by: Subhash Jadavani
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 12 +++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e0b8755..573a8cb 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -58,6 +58,12 @@
>> #define QUERY_REQ_RETRIES 10
>> /* Query request timeout */
>> #define QUERY_REQ_TIMEOUT 30 /* msec */
>> +/*
>> + * Query request timeout for fDeviceInit flag
>> + * fDeviceInit query response time for some devices is too large that
>> default
>> + * QUERY_REQ_TIMEOUT may not be enough for such devices.
>> + */
>> +#define QUERY_FDEVICEINIT_REQ_TIMEOUT 600 /* msec */
>
> How about just increasing QUERY_REQ_TIMEOUT from 30ms to 600ms
> instead of conditionally setting timeout depending on ufs flag?
Your suggestion obviously could work (increasing the QUERY_REQ_TIMEOUT to
600ms), but in that case we bring extra delay of 570ms to error handling
of query timeout, and in such a case, instead of handling the error after
30ms we handle it after 600ms, which make the SW hang.
does it make sense ?
>
>>
>> /* Task management command timeout */
>> #define TM_CMD_TIMEOUT 100 /* msecs */
>> @@ -1651,6 +1657,7 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>> struct ufs_query_req *request = NULL;
>> struct ufs_query_res *response = NULL;
>> int err, index = 0, selector = 0;
>> + int timeout = QUERY_REQ_TIMEOUT;
>>
>> BUG_ON(!hba);
>>
>> @@ -1683,7 +1690,10 @@ static int ufshcd_query_flag(struct ufs_hba *hba,
>> enum query_opcode opcode,
>> goto out_unlock;
>> }
>>
>> - err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY,
>> QUERY_REQ_TIMEOUT);
>> + if (idn == QUERY_FLAG_IDN_FDEVICEINIT)
>> + timeout = QUERY_FDEVICEINIT_REQ_TIMEOUT;
>> +
>> + err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, timeout);
>>
>> if (err) {
>> dev_err(hba->dev,
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of Code Aurora Forum, hosted by The Linux Foundation
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 03/15] scsi: ufs: verify command tag validity
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi :
>> A race condition appear to exist between request completion when
>> scsi_done() is called to end the request and set the tag back to
>> -1 (at blk_queue_end_tag() scsi_end_request), and scsi layer error
>> handling which aborts the command and reuses it to request sense
>> data. Sending the request sense is done with tag which was set to -1
>> and so it is invalid.
>> Assert command tag passed from scsi layer is valid.
>>
>> Signed-off-by: Gilad Broner
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 24 ++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 2d3ebca..8860a57 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -190,6 +190,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
>> *hba,
>> struct ufs_pa_layer_attr *desired_pwr_mode);
>> static int ufshcd_change_power_mode(struct ufs_hba *hba,
>> struct ufs_pa_layer_attr *pwr_mode);
>> +static inline bool ufshcd_valid_tag(struct ufs_hba *hba, int tag)
>> +{
>> + return tag >= 0 && tag < hba->nutrs;
>> +}
>>
>> static inline int ufshcd_enable_irq(struct ufs_hba *hba)
>> {
>> @@ -1310,6 +1314,12 @@ static int ufshcd_queuecommand(struct Scsi_Host
>> *host, struct scsi_cmnd *cmd)
>> hba = shost_priv(host);
>>
>> tag = cmd->request->tag;
>> + if (!ufshcd_valid_tag(hba, tag)) {
>> + dev_err(hba->dev,
>> + "%s: invalid command tag %d: cmd=0x%p,
>> cmd->request=0x%p",
>> + __func__, tag, cmd, cmd->request);
>> + BUG();
>> + }
>
> Is it better to avoid BUG() by WARN_ON() and return if possible?
in this specific case, i think BUG() is the way to handle this scenario.
It is very rare, and if invalid_tag is sent, the SW can not proceed, and
it indicates something very wrong happened. either in the block layer that
allocated the tag, or in the UFS that reported nutrs.
So, if we actually hit this scenario, then recovering is not an option and
i believe we need to BUG. hope it makes sense.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 6/8] scsi: ufs: make the UFS variant a platform device
> On Thursday 22 October 2015 07:02:14 subha...@codeaurora.org wrote: >> > >> > Required properties: >> > -- compatible: compatible list, contains "jedec,ufs-1.1" >> > +- compatible: compatible list, contains "jedec,ufs-1.1" or >> > +"qcom,msm8994-ufshc" or "qcom,msm8996-ufshc" >> >> Are we really using these "qcom,msm8994-ufshc" or "qcom,msm8996-ufshc" >> compatible string anywhere? > > We should list them in either case, but it could be clarified as: > > compatible: must contain "jedec,ufs-1.1", may also list one or more > of the following: > "qcom,msm8994-ufshc" > "qcom,msm8996-ufshc" > "qcom,ufshc" > > Arnd > Shortly i will upload an updated v8. Yaniv -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device
> On Sun, Aug 23, 2015 at 8:09 AM, Yaniv Gardi > wrote: >> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS >> a platform device. >> In order to do so a few additional changes are required: >> 1. The ufshcd-pltfrm is no longer serves as a platform device. >>Now it only serves as a group of platform APIs such as PM APIs >>(runtime suspend/resume, system suspend/resume etc), parsers of >>clocks, regulators and pm_levels from DT. >> 2. What used to be the old platform "probe" is now "only" >>a pltfrm_init() routine, that does exactly the same, but only >>being called by the new probe function of the UFS variant. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 2 +- >> drivers/scsi/ufs/ufs-qcom.c| 78 >> +- >> drivers/scsi/ufs/ufshcd-pltfrm.c | 92 >> ++ >> drivers/scsi/ufs/ufshcd-pltfrm.h | 41 ++ >> drivers/scsi/ufs/ufshcd.c | 10 +++ >> drivers/scsi/ufs/ufshcd.h | 1 + >> 6 files changed, 152 insertions(+), 72 deletions(-) >> create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h >> >> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> index 5357919..b39e765 100644 >> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> @@ -4,7 +4,7 @@ UFSHC nodes are defined to describe on-chip UFS host >> controllers. >> Each UFS controller instance should have its own node. >> >> Required properties: >> -- compatible: compatible list, contains "jedec,ufs-1.1" >> +- compatible: compatible list, contains "jedec,ufs-1.1" or >> "qcom,ufshc" > > Replying again as I inadvertently dropped everyone. > > This should also have a more specific compatible string with the SOC > name/number in it. It may be "the same in all SOCs", but there is > always the possibility for bugs/limitations to be found that are > specific to an SOC even if all RTL versions are identical (e.g. > different max clock speeds). It is about making the dtb future proof, > not about what exactly you need today. You can keep qcom,ufshc for > driver matching if you want. > >> - interrupts: >> - reg : > > What about phy properties? No Unipro PHY block that requires setup? phy properties will be updated in V8 that shortly will be uploaded. > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device
> On Sun, Aug 30, 2015 at 3:43 AM, wrote: >>> On Sun, Aug 23, 2015 at 8:09 AM, Yaniv Gardi >>> wrote: This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS a platform device. In order to do so a few additional changes are required: 1. The ufshcd-pltfrm is no longer serves as a platform device. Now it only serves as a group of platform APIs such as PM APIs (runtime suspend/resume, system suspend/resume etc), parsers of clocks, regulators and pm_levels from DT. 2. What used to be the old platform "probe" is now "only" a pltfrm_init() routine, that does exactly the same, but only being called by the new probe function of the UFS variant. Signed-off-by: Yaniv Gardi --- .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 2 +- drivers/scsi/ufs/ufs-qcom.c| 78 +- drivers/scsi/ufs/ufshcd-pltfrm.c | 92 ++ drivers/scsi/ufs/ufshcd-pltfrm.h | 41 ++ drivers/scsi/ufs/ufshcd.c | 10 +++ drivers/scsi/ufs/ufshcd.h | 1 + 6 files changed, 152 insertions(+), 72 deletions(-) create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt index 5357919..b39e765 100644 --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt @@ -4,7 +4,7 @@ UFSHC nodes are defined to describe on-chip UFS host controllers. Each UFS controller instance should have its own node. Required properties: -- compatible: compatible list, contains "jedec,ufs-1.1" +- compatible: compatible list, contains "jedec,ufs-1.1" or "qcom,ufshc" >>> >>> Replying again as I inadvertently dropped everyone. >>> >>> This should also have a more specific compatible string with the SOC >>> name/number in it. It may be "the same in all SOCs", but there is >>> always the possibility for bugs/limitations to be found that are >>> specific to an SOC even if all RTL versions are identical (e.g. >>> different max clock speeds). It is about making the dtb future proof, >>> not about what exactly you need today. You can keep qcom,ufshc for >>> driver matching if you want. >> >> I see your point. >> I just would like to make sure, syntactically speaking, if what you mean >> should look like: >> >> compatible: compatible list, contains "jedec,ufs-1.1" or >> "qcom,ufshc" for msm8994, msm8996 SOCs. > > No, you need actual compatible strings: > > contains one of: > "jedec,ufs-1.1" > "qcom,msm8994-ufshc" or "qcom,msm8996-ufshc" followed by "qcom,ufshc" > > Really, we should probably never had allowed "jedec,ufs-1.1" by itself. > - interrupts: - reg : >>> >>> What about phy properties? No Unipro PHY block that requires setup? >>> >> >> yes, i will add another documentation file for it. > > And you need the "phy" property here with the phandle to the phy node. > The "phy" properties will be updated in V8 i'm about to uploade > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/8] Fix error message and present UFS variant
> Hi Yaniv, > > 2015-08-23 22:09 GMT+09:00 Yaniv Gardi : >> V3: fixes a few minor issues. >> >> V2: fixes a few issues of unnecessary EXPORT_SYMBOL, >> types of parameters in routine definition, >> build errors in case CONFIG_PM is not defined and some >> other minor fixes. > > I've checked outstanding issues I reported for v1 and v2 are fixed > in this version of series. So please feel free to add: > > Reviewed-by: Akinobu Mita > > I still think that we should introduce print_hex_dump_io() or > something simpler for dumping __iomem pointer instead of casting > 'void __force *'. But it is only used for debug dump function, so > I don't too much worry about it. > Thanks, Mita, as you say, since this is mainly for debug dump functions, so i agree that for now, we may keep it as is. Yaniv -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] scsi: ufs: add ioctl interface for query request
thanks Arnd for the comment. V4 is on its way
> On Thursday 08 October 2015 14:09:24 Yaniv Gardi wrote:
>> This patch exposes the ioctl interface for UFS driver via SCSI device
>> ioctl interface. As of now UFS driver would provide the ioctl for query
>> interface to connected UFS device.
>>
>> Signed-off-by: Dolev Raviv
>> Signed-off-by: Noa Rubens
>> Signed-off-by: Raviv Shvili
>> Signed-off-by: Gilad Broner
>> Signed-off-by: Yaniv Gardi
>
> Thanks for the changes, looks much better already
>
>
>> @@ -5106,6 +5308,10 @@ static struct scsi_host_template
>> ufshcd_driver_template = {
>> .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>> .eh_host_reset_handler = ufshcd_eh_host_reset_handler,
>> .eh_timed_out = ufshcd_eh_timed_out,
>> +.ioctl = ufshcd_ioctl,
>> +#ifdef CONFIG_COMPAT
>> +.compat_ioctl = ufshcd_ioctl,
>> +#endif
>> .this_id= -1,
>> .sg_tablesize = SG_ALL,
>> .cmd_per_lun= UFSHCD_CMD_PER_LUN,
>
> no need for the #ifdef here.
in include\scsi\scsi_host.h
the hook - compat_ioctl is defined inside #ifdef CONFIG_COMPAT
#ifdef CONFIG_COMPAT
int (* compat_ioctl)(struct scsi_device *dev, int cmd, void __user
*arg);
#endif
>
>> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
>> index e0a3398..fada160 100644
>> --- a/include/scsi/scsi.h
>> +++ b/include/scsi/scsi.h
>> @@ -284,6 +284,7 @@ static inline int scsi_is_wlun(u64 lun)
>> * Here are some scsi specific ioctl commands which are sometimes
>> useful.
>> *
>> * Note that include/linux/cdrom.h also defines IOCTL 0x5300 - 0x5395
>> + * include/uapi/scsi/ufs/ioctl.h defines 0x5388
>> */
>>
>
> The comment is now wrong.
>
>
> Hmm, the buffer now is not aligned to four bytes, which will make copying
> it
> a bit slower. If that can be a concern, adding a padding byte would help.
>
> Arnd
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi: ufs: add ioctl interface for query request
Thanks Arnd.
comments inline and will upload V3 shortly
Yaniv
> On Wednesday 07 October 2015 10:54:03 Yaniv Gardi wrote:
>>
>> +/* IOCTL opcode for command - ufs set device read only */
>> +#define UFS_IOCTL_BLKROSET BLKROSET
>> +
>
> What is this for? Can't you just use the normal BLKROSET definition in
> user space?
>
correct. will be removed
>> +
>> + ioctl_data = kzalloc(sizeof(struct ufs_ioctl_query_data),
>> GFP_KERNEL);
>> + if (!ioctl_data) {
>> + err = -ENOMEM;
>> + goto out;
>> + }
>
> ufs_ioctl_query_data is short enough to just be on the stack.
ok
>
>> +}
>> +length = min_t(int, QUERY_DESC_MAX_SIZE,
>> +ioctl_data->buf_size);
>> +desc = kzalloc(length, GFP_KERNEL);
>> +if (!desc) {
>> +dev_err(hba->dev, "%s: Failed allocating %d bytes\n",
>> +__func__, length);
>> +err = -ENOMEM;
>> +goto out_release_mem;
>> +}
>
> Better check for a maximum length as well, not just a minimum length.
> For overly long requests, just return an error without trying the kzalloc.
>
> You can also use memdup_user to avoid the extra zeroing and and simplify
> the calling code.
>
>> +/**
>> + * ufshcd_ioctl - ufs ioctl callback registered in scsi_host
>> + * @dev: scsi device required for per LUN queries
>> + * @cmd: command opcode
>> + * @buffer: user space buffer for transferring data
>> + *
>> + * Supported commands:
>> + * UFS_IOCTL_QUERY
>> + */
>> +static int ufshcd_ioctl(struct scsi_device *dev, int cmd, void __user
>> *buffer)
>> +{
>> +struct ufs_hba *hba = shost_priv(dev->host);
>> +int err = 0;
>> +
>> +BUG_ON(!hba);
>> +if (!buffer) {
>> +dev_err(hba->dev, "%s: User buffer is NULL!\n", __func__);
>> +return -EINVAL;
>> +}
>
> Don't print an error here, you don't want users to be able to flood
> syslog that easily, and the program that caused the error does not
> see the log anyway.
>
ok
>> +
>> +switch (cmd) {
>> +case UFS_IOCTL_QUERY:
>> +pm_runtime_get_sync(hba->dev);
>> +err = ufshcd_query_ioctl(hba, ufshcd_scsi_to_upiu_lun(dev->lun),
>> +buffer);
>> +pm_runtime_put_sync(hba->dev);
>> +break;
>> +case UFS_IOCTL_BLKROSET:
>> +err = -ENOIOCTLCMD;
>> +break;
>> +default:
>> +err = -EINVAL;
>> +dev_err(hba->dev, "%s: Illegal ufs-IOCTL cmd %d\n", __func__,
>> +cmd);
>> +break;
>
> same here.
>
ok
>> +}
>> +
>> +return err;
>> +}
>> +
>> +/**
>> * ufshcd_async_scan - asynchronous execution for probing hba
>> * @data: data pointer to pass to this function
>> * @cookie: cookie data
>> @@ -5106,6 +5322,7 @@ static struct scsi_host_template
>> ufshcd_driver_template = {
>> .eh_device_reset_handler = ufshcd_eh_device_reset_handler,
>> .eh_host_reset_handler = ufshcd_eh_host_reset_handler,
>> .eh_timed_out = ufshcd_eh_timed_out,
>> +.ioctl = ufshcd_ioctl,
>> .this_id= -1,
>> .sg_tablesize = SG_ALL,
>> .cmd_per_lun= UFSHCD_CMD_PER_LUN,
>
> The ioctl data is compatible between 32-bit and 64-bit user space, so
> better add a .compat_ioctl line right away to make this work on 64-bit
> architectures with 32-bit user space.
>
ok
>> +
>> +/*
>> + * IOCTL opcode for ufs queries has the following opcode after
>> + * SCSI_IOCTL_GET_PCI
>> + */
>> +#define UFS_IOCTL_QUERY 0x5388
>
> Use _IOWR() to define that number with the correct argument length
i'd rather leave it as is, to be compatible with the SCSI_IOCTL
opcodes that are defined in scsi_ioctl.h
>
>> +/**
>> + * struct ufs_ioctl_query_data - used to transfer data to and from
>> + * user via ioctl
>> + * @opcode: type of data to query (descriptor/attribute/flag)
>> + * @idn: id of the data structure
>> + * @buf_size: number of allocated bytes/data size on return
>> + * @buffer: data location
>> + *
>> + * Received: buffer and buf_size (available space for transferred data)
>> + * Submitted: opcode, idn, length, buf_size
>> + */
>> +struct ufs_ioctl_query_data {
>> +/*
>> + * User should select one of the opcode defined in "enum
>> query_opcode".
>> + * Please check include/uapi/scsi/ufs/ufs.h for the definition of it.
>> + * Note that only UPIU_QUERY_OPCODE_READ_DESC,
>> + * UPIU_QUERY_OPCODE_READ_ATTR & UPIU_QUERY_OPCODE_READ_FLAG are
>> + * supported as of now. All other query_opcode would be considered
>> + * invalid.
>> + * As of now only read query operations are supported.
>> + */
>> +__u32 opcode;
>> +/*
>> + * User should select one of the idn from "enum flag_idn" or "enum
>> + * attr_idn" or "enum desc_idn" based
Re: [PATCH v5 6/8] scsi: ufs: make the UFS variant a platform device
> On Wed, Sep 2, 2015 at 3:32 AM, Yaniv Gardi wrote:
>> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS
>> a platform device.
>> In order to do so a few additional changes are required:
>> 1. The ufshcd-pltfrm is no longer serves as a platform device.
>>Now it only serves as a group of platform APIs such as PM APIs
>>(runtime suspend/resume, system suspend/resume etc), parsers of
>>clocks, regulators and pm_levels from DT.
>> 2. What used to be the old platform "probe" is now "only"
>>a pltfrm_init() routine, that does exactly the same, but only
>>being called by the new probe function of the UFS variant.
>>
>> Signed-off-by: Yaniv Gardi
>
> [...]
>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 329ac84..5179250 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -19,6 +19,7 @@
>>
>> #include
>> #include "ufshcd.h"
>> +#include "ufshcd-pltfrm.h"
>> #include "unipro.h"
>> #include "ufs-qcom.h"
>> #include "ufshci.h"
>> @@ -1036,7 +1037,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba
>> *hba)
>> * The variant operations configure the necessary controller and PHY
>> * handshake during initialization.
>> */
>> -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>> +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>> .name = "qcom",
>> .init = ufs_qcom_init,
>> .exit = ufs_qcom_exit,
>> @@ -1050,4 +1051,75 @@ static const struct ufs_hba_variant_ops
>> ufs_hba_qcom_vops = {
>> .resume = ufs_qcom_resume,
>> };
>>
>> +/**
>> + * ufs_qcom_probe - probe routine of the driver
>> + * @pdev: pointer to Platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_probe(struct platform_device *pdev)
>> +{
>> + int err;
>> + struct device *dev = &pdev->dev;
>> + struct ufs_hba *hba;
>> +
>> + /* Perform generic probe */
>> + err = ufshcd_pltfrm_init(pdev, &ufs_hba_qcom_vops);
>> + if (err) {
>> + dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
>> + goto out;
>> + }
>> +
>
> All of this:
>
>> + hba = platform_get_drvdata(pdev);
>> +
>> + return 0;
>> +
>> +dealloc_host:
>> + /* disconnect the bind between the qcom host and the hba */
>> + ufshcd_set_variant(hba, NULL);
>> + ufshcd_dealloc_host(hba);
>
> To this is dead code. You should get a warning that dealloc_host is
> unused. Please check and fix all warnings.
>
> Rob
>
correct. V7 has taken care of that.
thanks,
Yaniv
>> +out:
>> + return err;
>> +}
>> +
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 6/8] scsi: ufs: make the UFS variant a platform device
> On Sun, Aug 30, 2015 at 9:52 AM, Yaniv Gardi
> wrote:
>> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS
>> a platform device.
>> In order to do so a few additional changes are required:
>> 1. The ufshcd-pltfrm is no longer serves as a platform device.
>>Now it only serves as a group of platform APIs such as PM APIs
>>(runtime suspend/resume, system suspend/resume etc), parsers of
>>clocks, regulators and pm_levels from DT.
>> 2. What used to be the old platform "probe" is now "only"
>>a pltfrm_init() routine, that does exactly the same, but only
>>being called by the new probe function of the UFS variant.
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 57 ++
>> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 4 +-
>
> For the binding:
>
> Reviewed-by: Rob Herring
>
> A comment on the driver part still.
>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 329ac84..8027435 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>
> [...]
>
>> +static int ufs_qcom_probe(struct platform_device *pdev)
>> +{
>> + int err;
>> + struct device *dev = &pdev->dev;
>> + struct ufs_hba *hba;
>> +
>> + /* Perform generic probe */
>> + err = ufshcd_pltfrm_init(pdev, &ufs_hba_qcom_vops);
>> + if (err) {
>> + dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
>> + goto out;
>> + }
>> +
>> + hba = platform_get_drvdata(pdev);
>
> I thought this was not necessary?
skipped my eye. i'm uploading another version without this redundant check
>
>> + if (unlikely(!hba)) {
>> + dev_err(dev, "no hba structure after successful
>> probing\n");
>> + goto dealloc_host;
>> + }
>> +
>> + return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Thu, Aug 27, 2015 at 7:11 AM, wrote: >>> On Tue, Aug 25, 2015 at 7:36 AM, wrote: > On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >> >> Add a write memory barrier to make sure descriptors prepared are >> actually >> written to memory before ringing the doorbell. We have also added >> the >> write memory barrier after ringing the doorbell register so that >> controller sees the new request immediately. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fef0660..876148b 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >> unsigned int task_tag) >> ufshcd_clk_scaling_start_busy(hba); >> __set_bit(task_tag, &hba->outstanding_reqs); >> ufshcd_writel(hba, 1 << task_tag, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); >> + /* Make sure that doorbell is committed immediately */ >> + wmb(); > > Is this really necessary? Is there a measurable difference? I'm not sure if there is a measurable difference, but as the Door-Bell register is the one that actually responsible for the HW execution of the requests, anyhow, it's recommended to its value will be written instantly to the memory. >>> >>> A barrier doesn't guarantee speed, only ordering. Unless you can >>> measure the difference, you should not have it. >> >> Rob, >> let me have an example: >> context#1 updates outstanding_reqs variable and write(DOOR_BELL) >> context#2 upon interrupt of a request completion the following happens: >> report completion on each one of the bits in: >> outstanding_reqs ^ read(DOOR_BELL); >> >> 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in >> slot 0) >> 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in >> slot >> 0 and 1) >> 2. the new value 0x3 is still not written to the DR so DORR_BELL is >> still >> 0x1, but outstanding_reqs is already updated = 0x3 >> 3. the request in slot 0 just completed, and interrupt happens, so >> DORR_BELL is now 0 (request in slot 0 completed) >> 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 => >> wrong conclusion since the request in slot 1 never completed, and >> actually >> never started. > > Barriers alone will never solve this problem. They may narrow the > window possibly, but the problem is still there. What you have to have > is a spinlock around all accesses to both outstanding_reqs and > doorbell register. And guess what, spinlocks have appropriate barriers > to ensure visibility of what they protect. Or perhaps the h/w provides > another way to signal what slots have completed. Using the same > register for doorbell and completion status is not ideal. > can i assume spin_lock_irqsave() and spin_unlock_irqrestore() both provide barriers ? i couldn't find the barrier instruction when following the call chain... > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device
> On Sun, Aug 23, 2015 at 8:09 AM, Yaniv Gardi > wrote: >> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS >> a platform device. >> In order to do so a few additional changes are required: >> 1. The ufshcd-pltfrm is no longer serves as a platform device. >>Now it only serves as a group of platform APIs such as PM APIs >>(runtime suspend/resume, system suspend/resume etc), parsers of >>clocks, regulators and pm_levels from DT. >> 2. What used to be the old platform "probe" is now "only" >>a pltfrm_init() routine, that does exactly the same, but only >>being called by the new probe function of the UFS variant. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 2 +- >> drivers/scsi/ufs/ufs-qcom.c| 78 >> +- >> drivers/scsi/ufs/ufshcd-pltfrm.c | 92 >> ++ >> drivers/scsi/ufs/ufshcd-pltfrm.h | 41 ++ >> drivers/scsi/ufs/ufshcd.c | 10 +++ >> drivers/scsi/ufs/ufshcd.h | 1 + >> 6 files changed, 152 insertions(+), 72 deletions(-) >> create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h >> >> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> index 5357919..b39e765 100644 >> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt >> @@ -4,7 +4,7 @@ UFSHC nodes are defined to describe on-chip UFS host >> controllers. >> Each UFS controller instance should have its own node. >> >> Required properties: >> -- compatible: compatible list, contains "jedec,ufs-1.1" >> +- compatible: compatible list, contains "jedec,ufs-1.1" or >> "qcom,ufshc" > > Replying again as I inadvertently dropped everyone. > > This should also have a more specific compatible string with the SOC > name/number in it. It may be "the same in all SOCs", but there is > always the possibility for bugs/limitations to be found that are > specific to an SOC even if all RTL versions are identical (e.g. > different max clock speeds). It is about making the dtb future proof, > not about what exactly you need today. You can keep qcom,ufshc for > driver matching if you want. I see your point. I just would like to make sure, syntactically speaking, if what you mean should look like: compatible: compatible list, contains "jedec,ufs-1.1" or "qcom,ufshc" for msm8994, msm8996 SOCs. > >> - interrupts: >> - reg : > > What about phy properties? No Unipro PHY block that requires setup? > yes, i will add another documentation file for it. > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >>> >>> Add a write memory barrier to make sure descriptors prepared are >>> actually >>> written to memory before ringing the doorbell. We have also added the >>> write memory barrier after ringing the doorbell register so that >>> controller sees the new request immediately. >>> >>> Signed-off-by: Yaniv Gardi >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 6 ++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index fef0660..876148b 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >>> unsigned int task_tag) >>> ufshcd_clk_scaling_start_busy(hba); >>> __set_bit(task_tag, &hba->outstanding_reqs); >>> ufshcd_writel(hba, 1 << task_tag, >>> REG_UTP_TRANSFER_REQ_DOOR_BELL); >>> + /* Make sure that doorbell is committed immediately */ >>> + wmb(); >> >> Is this really necessary? Is there a measurable difference? > > I'm not sure if there is a measurable difference, but as the Door-Bell > register is the one that actually responsible for the HW execution of the > requests, anyhow, it's recommended to its value will be written > instantly to the memory. > > Also, as the Interrupt context reads this register, and compare it to the > SW mirroring value (hba->outstanding_reqs) in order to realize what > requests are already completed, it's important to get the correct value > by reading this register, otherwise we might realize a request completion > while it was never even submitted. > >> >>> } >>> >>> /** >>> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host >>> *host, struct scsi_cmnd *cmd) >>> goto out; >>> } >>> >>> + /* Make sure descriptors are ready before ringing the doorbell >>> */ >>> + wmb(); >> >> The writel for the doorbell will do a barrier first. (I didn't check >> what exactly ufshcd_writel does, but that is why I don't like these >> private access wrappers.) the barrier here is important and a must. before it we prepare descriptors. after it we write to DOOR-BELL. (and after the DOOR-BELL we have another one.) if we remove it, we might get the DOOR-BELL written, before the descriptors written. >> >>> /* issue command to the controller */ >>> spin_lock_irqsave(hba->host->host_lock, flags); >>> ufshcd_send_command(hba, tag); >>> @@ -1627,6 +1631,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba >>> *hba, >>> >>> hba->dev_cmd.complete = &wait; >>> >>> + /* Make sure descriptors are ready before ringing the doorbell >>> */ >>> + wmb(); >>> spin_lock_irqsave(hba->host->host_lock, flags); >>> ufshcd_send_command(hba, tag); >>> spin_unlock_irqrestore(hba->host->host_lock, flags); >>> -- >>> 1.8.5.2 >>> >>> -- >>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>> member of Code Aurora Forum, hosted by The Linux Foundation >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Tue, Aug 25, 2015 at 7:36 AM, wrote: >>> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: Add a write memory barrier to make sure descriptors prepared are actually written to memory before ringing the doorbell. We have also added the write memory barrier after ringing the doorbell register so that controller sees the new request immediately. Signed-off-by: Yaniv Gardi --- drivers/scsi/ufs/ufshcd.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index fef0660..876148b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag) ufshcd_clk_scaling_start_busy(hba); __set_bit(task_tag, &hba->outstanding_reqs); ufshcd_writel(hba, 1 << task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL); + /* Make sure that doorbell is committed immediately */ + wmb(); >>> >>> Is this really necessary? Is there a measurable difference? >> >> I'm not sure if there is a measurable difference, but as the Door-Bell >> register is the one that actually responsible for the HW execution of >> the >> requests, anyhow, it's recommended to its value will be written >> instantly to the memory. > > A barrier doesn't guarantee speed, only ordering. Unless you can > measure the difference, you should not have it. Rob, let me have an example: context#1 updates outstanding_reqs variable and write(DOOR_BELL) context#2 upon interrupt of a request completion the following happens: report completion on each one of the bits in: outstanding_reqs ^ read(DOOR_BELL); 0. let's assume the DOOR_BELL = 0x1 (which means 1 active request in slot 0) 1. context#1: update the DOOR_BELL to be 0x3; (2 active requests: in slot 0 and 1) 2. the new value 0x3 is still not written to the DR so DORR_BELL is still 0x1, but outstanding_reqs is already updated = 0x3 3. the request in slot 0 just completed, and interrupt happens, so DORR_BELL is now 0 (request in slot 0 completed) 4. context#2: outstanding_reqs ^ read(DOOR_BELL) = 0x3 ^ 0x0 = 0x3 => wrong conclusion since the request in slot 1 never completed, and actually never started. > >> Also, as the Interrupt context reads this register, and compare it to >> the >> SW mirroring value (hba->outstanding_reqs) in order to realize what >> requests are already completed, it's important to get the correct value >> by reading this register, otherwise we might realize a request >> completion >> while it was never even submitted. > > If a register read can pass a register write out of order, then your > h/w is broken. Plus what if the interrupt occurs before the barrier. > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote:
>>
>> Sometimes queries from the device might return a failure so it is
>> recommended to retry sending the query, before giving up.
>> This change adds a wrapper to retry sending a query attribute,
>> in cases where we need to wait longer, before we continue,
>> or before reporting a failure.
>
> Why not just always retry? Are there cases where retrying would be a
> problem?
There is no problem to retry whenever we encounter a query that returns
with and error.
>
>
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 51
>> ---
>> 1 file changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 876148b..bfef67d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1827,6 +1827,43 @@ out:
>> }
>>
>> /**
>> + * ufshcd_query_attr_retry() - API function for sending query
>> + * attribute with retries
>> + * @hba: per-adapter instance
>> + * @opcode: attribute opcode
>> + * @idn: attribute idn to access
>> + * @index: index field
>> + * @selector: selector field
>> + * @attr_val: the attribute value after the query request
>> + * completes
>> + *
>> + * Returns 0 for success, non-zero in case of failure
>> +*/
>> +static int ufshcd_query_attr_retry(struct ufs_hba *hba,
>> + enum query_opcode opcode, enum attr_idn idn, u8 index, u8
>> selector,
>> + u32 *attr_val)
>> +{
>> + int ret = 0;
>> + u32 retries;
>> +
>> +for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>> + ret = ufshcd_query_attr(hba, opcode, idn, index,
>> + selector, attr_val);
>> + if (ret)
>> + dev_dbg(hba->dev, "%s: failed with error %d,
>> retries %d\n",
>> + __func__, ret, retries);
>> + else
>> + break;
>> + }
>> +
>> + if (ret)
>> + dev_err(hba->dev,
>> + "%s: query attribute, idn %d, failed with error
>> %d after %d retires\n",
>> + __func__, idn, ret, retries);
>
> The retry count will be wrong here.
you are correct. will be fixed in V2
>
>> + return ret;
>> +}
>> +
>> +/**
>> * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>> * hba: per-adapter instance
>> * opcode: attribute opcode
>> @@ -3407,7 +3444,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba,
>> u16 mask)
>>
>> val = hba->ee_ctrl_mask & ~mask;
>> val &= 0x; /* 2 bytes */
>> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> if (!err)
>> hba->ee_ctrl_mask &= ~mask;
>> @@ -3435,7 +3472,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba,
>> u16 mask)
>>
>> val = hba->ee_ctrl_mask | mask;
>> val &= 0x; /* 2 bytes */
>> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> if (!err)
>> hba->ee_ctrl_mask |= mask;
>> @@ -3541,7 +3578,7 @@ static void ufshcd_force_reset_auto_bkops(struct
>> ufs_hba *hba)
>>
>> static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32
>> *status)
>> {
>> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
>> }
>>
>> @@ -3604,7 +3641,7 @@ static int ufshcd_urgent_bkops(struct ufs_hba
>> *hba)
>>
>> static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32
>> *status)
>> {
>> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
>> }
>>
>> @@ -4360,9 +4397,9 @@ static void ufshcd_init_icc_levels(struct ufs_hba
>> *hba)
>> dev_dbg(hba->dev, "%s: setting icc_level 0x%x",
>> __func__, hba->init_prefetch_data.icc_level);
>>
>> - ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> - QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
>> - &hba->init_prefetch_data.icc_level);
>> + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
>> + &hba->init_prefetch_data.icc_level);
>>
>> if (ret)
>> dev_err(hba->dev,
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote:
>>
>> Sometimes queries from the device might return a failure so it is
>> recommended to retry sending the query, before giving up.
>> This change adds a wrapper to retry sending a query attribute,
>> in cases where we need to wait longer, before we continue,
>> or before reporting a failure.
>
> Why not just always retry? Are there cases where retrying would be a
> problem?
There is no problem to retry whenever we encounter a query that returns
with and error.
In the code, it's recommended to replace any call to
>
>
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 51
>> ---
>> 1 file changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 876148b..bfef67d 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1827,6 +1827,43 @@ out:
>> }
>>
>> /**
>> + * ufshcd_query_attr_retry() - API function for sending query
>> + * attribute with retries
>> + * @hba: per-adapter instance
>> + * @opcode: attribute opcode
>> + * @idn: attribute idn to access
>> + * @index: index field
>> + * @selector: selector field
>> + * @attr_val: the attribute value after the query request
>> + * completes
>> + *
>> + * Returns 0 for success, non-zero in case of failure
>> +*/
>> +static int ufshcd_query_attr_retry(struct ufs_hba *hba,
>> + enum query_opcode opcode, enum attr_idn idn, u8 index, u8
>> selector,
>> + u32 *attr_val)
>> +{
>> + int ret = 0;
>> + u32 retries;
>> +
>> +for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
>> + ret = ufshcd_query_attr(hba, opcode, idn, index,
>> + selector, attr_val);
>> + if (ret)
>> + dev_dbg(hba->dev, "%s: failed with error %d,
>> retries %d\n",
>> + __func__, ret, retries);
>> + else
>> + break;
>> + }
>> +
>> + if (ret)
>> + dev_err(hba->dev,
>> + "%s: query attribute, idn %d, failed with error
>> %d after %d retires\n",
>> + __func__, idn, ret, retries);
>
> The retry count will be wrong here.
you are correct. will be fixed in V2
>
>> + return ret;
>> +}
>> +
>> +/**
>> * ufshcd_query_descriptor - API function for sending descriptor
>> requests
>> * hba: per-adapter instance
>> * opcode: attribute opcode
>> @@ -3407,7 +3444,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba,
>> u16 mask)
>>
>> val = hba->ee_ctrl_mask & ~mask;
>> val &= 0x; /* 2 bytes */
>> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> if (!err)
>> hba->ee_ctrl_mask &= ~mask;
>> @@ -3435,7 +3472,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba,
>> u16 mask)
>>
>> val = hba->ee_ctrl_mask | mask;
>> val &= 0x; /* 2 bytes */
>> - err = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>> if (!err)
>> hba->ee_ctrl_mask |= mask;
>> @@ -3541,7 +3578,7 @@ static void ufshcd_force_reset_auto_bkops(struct
>> ufs_hba *hba)
>>
>> static inline int ufshcd_get_bkops_status(struct ufs_hba *hba, u32
>> *status)
>> {
>> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> QUERY_ATTR_IDN_BKOPS_STATUS, 0, 0, status);
>> }
>>
>> @@ -3604,7 +3641,7 @@ static int ufshcd_urgent_bkops(struct ufs_hba
>> *hba)
>>
>> static inline int ufshcd_get_ee_status(struct ufs_hba *hba, u32
>> *status)
>> {
>> - return ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> + return ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> QUERY_ATTR_IDN_EE_STATUS, 0, 0, status);
>> }
>>
>> @@ -4360,9 +4397,9 @@ static void ufshcd_init_icc_levels(struct ufs_hba
>> *hba)
>> dev_dbg(hba->dev, "%s: setting icc_level 0x%x",
>> __func__, hba->init_prefetch_data.icc_level);
>>
>> - ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> - QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
>> - &hba->init_prefetch_data.icc_level);
>> + ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>> + QUERY_ATTR_IDN_ACTIVE_ICC_LVL, 0, 0,
>> + &hba->init_prefetch_data.icc_level);
>>
>> if (ret)
>> dev_err(hba->dev,
>> --
>> 1.8.5.2
>>
>> --
>> QUALCOMM ISRAEL, on beh
Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell
> On Aug 21, 2015 3:10 PM, "Yaniv Gardi" wrote: >> >> Add a write memory barrier to make sure descriptors prepared are >> actually >> written to memory before ringing the doorbell. We have also added the >> write memory barrier after ringing the doorbell register so that >> controller sees the new request immediately. >> >> Signed-off-by: Yaniv Gardi >> >> --- >> drivers/scsi/ufs/ufshcd.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fef0660..876148b 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -833,6 +833,8 @@ void ufshcd_send_command(struct ufs_hba *hba, >> unsigned int task_tag) >> ufshcd_clk_scaling_start_busy(hba); >> __set_bit(task_tag, &hba->outstanding_reqs); >> ufshcd_writel(hba, 1 << task_tag, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); >> + /* Make sure that doorbell is committed immediately */ >> + wmb(); > > Is this really necessary? Is there a measurable difference? I'm not sure if there is a measurable difference, but as the Door-Bell register is the one that actually responsible for the HW execution of the requests, anyhow, it's recommended to its value will be written instantly to the memory. Also, as the Interrupt context reads this register, and compare it to the SW mirroring value (hba->outstanding_reqs) in order to realize what requests are already completed, it's important to get the correct value by reading this register, otherwise we might realize a request completion while it was never even submitted. > >> } >> >> /** >> @@ -1418,6 +1420,8 @@ static int ufshcd_queuecommand(struct Scsi_Host >> *host, struct scsi_cmnd *cmd) >> goto out; >> } >> >> + /* Make sure descriptors are ready before ringing the doorbell >> */ >> + wmb(); > > The writel for the doorbell will do a barrier first. (I didn't check > what exactly ufshcd_writel does, but that is why I don't like these > private access wrappers.) > >> /* issue command to the controller */ >> spin_lock_irqsave(hba->host->host_lock, flags); >> ufshcd_send_command(hba, tag); >> @@ -1627,6 +1631,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba >> *hba, >> >> hba->dev_cmd.complete = &wait; >> >> + /* Make sure descriptors are ready before ringing the doorbell >> */ >> + wmb(); >> spin_lock_irqsave(hba->host->host_lock, flags); >> ufshcd_send_command(hba, tag); >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> -- >> 1.8.5.2 >> >> -- >> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >> member of Code Aurora Forum, hosted by The Linux Foundation > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/8] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component
Reviewed-by: Akinobu Mita > This change is required in order to be able to build the component > as a module. > > Signed-off-by: Yaniv Gardi > > --- > drivers/scsi/ufs/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig > index e945383..5f45307 100644 > --- a/drivers/scsi/ufs/Kconfig > +++ b/drivers/scsi/ufs/Kconfig > @@ -72,7 +72,7 @@ config SCSI_UFSHCD_PLATFORM > If unsure, say N. > > config SCSI_UFS_QCOM > - bool "QCOM specific hooks to UFS controller platform driver" > + tristate "QCOM specific hooks to UFS controller platform driver" > depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM > select PHY_QCOM_UFS > help > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/8] scsi: ufs-qcom: fix compilation warning if compiled as a module
Reviewed-by: Akinobu Mita
> This change fixes a compilation warning that happens if SCSI_UFS_QCOM
> is compiled as a module.
> Also this patch fixes an error happens when insmod the module:
> "ufs_qcom: module license 'unspecified' taints kernel."
>
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 4cdffa4..6c23bbf 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -917,12 +917,15 @@ out:
>
> #define ANDROID_BOOT_DEV_MAX30
> static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> -static int get_android_boot_dev(char *str)
> +
> +#ifndef MODULE
> +static int __init get_android_boot_dev(char *str)
> {
> strlcpy(android_boot_dev, str, ANDROID_BOOT_DEV_MAX);
> return 1;
> }
> __setup("androidboot.bootdevice=", get_android_boot_dev);
> +#endif
>
> /**
> * ufs_qcom_init - bind phy with controller
> @@ -1047,3 +1050,5 @@ static const struct ufs_hba_variant_ops
> ufs_hba_qcom_vops = {
> .resume = ufs_qcom_resume,
> };
> EXPORT_SYMBOL(ufs_hba_qcom_vops);
> +
> +MODULE_LICENSE("GPL v2");
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/8] add ufshcd_get_variant ufshcd_set_variant
Reviewed-by: Akinobu Mita
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 34 +-
> drivers/scsi/ufs/ufshcd.h | 21 +
> 2 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 6c23bbf..64c54b7 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -155,7 +155,7 @@ out:
>
> static int ufs_qcom_link_startup_post_change(struct ufs_hba *hba)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct phy *phy = host->generic_phy;
> u32 tx_lanes;
> int err = 0;
> @@ -211,7 +211,7 @@ static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
>
> static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct phy *phy = host->generic_phy;
> int ret = 0;
> bool is_rate_B = (UFS_QCOM_LIMIT_HS_RATE == PA_HS_MODE_B)
> @@ -273,7 +273,7 @@ static void ufs_qcom_enable_hw_clk_gating(struct
> ufs_hba *hba)
>
> static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba, bool status)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> int err = 0;
>
> switch (status) {
> @@ -307,7 +307,7 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba
> *hba, bool status)
> static unsigned long
> ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear, u32 hs, u32 rate)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct ufs_clk_info *clki;
> u32 core_clk_period_in_ns;
> u32 tx_clk_cycles_per_us = 0;
> @@ -448,7 +448,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba
> *hba, bool status)
>
> static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct phy *phy = host->generic_phy;
> int ret = 0;
>
> @@ -479,7 +479,7 @@ out:
>
> static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct phy *phy = host->generic_phy;
> int err;
>
> @@ -621,7 +621,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba
> *hba,
> struct ufs_pa_layer_attr *dev_req_params)
> {
> u32 val;
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> struct phy *phy = host->generic_phy;
> struct ufs_qcom_dev_params ufs_qcom_cap;
> int ret = 0;
> @@ -696,7 +696,7 @@ out:
>
> static u32 ufs_qcom_get_ufs_hci_version(struct ufs_hba *hba)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>
> if (host->hw_ver.major == 0x1)
> return UFSHCI_VERSION_11;
> @@ -715,7 +715,7 @@ static u32 ufs_qcom_get_ufs_hci_version(struct ufs_hba
> *hba)
> */
> static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>
> if (host->hw_ver.major == 0x01) {
> hba->quirks |= UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
> @@ -740,7 +740,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba
> *hba)
>
> static void ufs_qcom_set_caps(struct ufs_hba *hba)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>
> if (host->hw_ver.major >= 0x2)
> host->caps = UFS_QCOM_CAP_QUNIPRO;
> @@ -811,7 +811,7 @@ static void ufs_qcom_get_speed_mode(struct
> ufs_pa_layer_attr *p, char *result)
>
> static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on)
> {
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> int err = 0;
> int vote = 0;
>
> @@ -866,7 +866,7 @@ show_ufs_to_mem_max_bus_bw(struct device *dev, struct
> device_attribute *attr,
> char *buf)
> {
> struct ufs_hba *hba = dev_get_drvdata(dev);
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>
> return snprintf(buf, PAGE_SIZE, "%u\n",
> host->bus_vote.is_max_bw_needed);
> @@ -877,7 +877,7 @@ store_ufs_to_mem_max_bus_bw(struct device *dev, struct
> device_attribute *attr,
> const char *buf, size_t count)
> {
> struct ufs_hba *hba = dev_get_drvdata(dev);
> - struct ufs_qcom_host *host = hba->priv;
> + struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> uint32_t val
Re: [PATCH v3 7/8] scsi: ufs-qcom: add debug prints for test bus
Reviewed-by: Akinobu Mita
> Adds support for configuring and reading the test bus and debug
> registers. This change also adds another vops in order to print the
> debug registers.
>
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 165
> +++-
> drivers/scsi/ufs/ufs-qcom.h | 37 +-
> drivers/scsi/ufs/ufshcd.c | 2 +
> drivers/scsi/ufs/ufshcd.h | 8 +++
> 4 files changed, 208 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 8027435..4d19c49 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -23,6 +23,24 @@
> #include "unipro.h"
> #include "ufs-qcom.h"
> #include "ufshci.h"
> +#define UFS_QCOM_DEFAULT_DBG_PRINT_EN\
> + (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN)
> +
> +enum {
> + TSTBUS_UAWM,
> + TSTBUS_UARM,
> + TSTBUS_TXUC,
> + TSTBUS_RXUC,
> + TSTBUS_DFC,
> + TSTBUS_TRLUT,
> + TSTBUS_TMRLUT,
> + TSTBUS_OCSC,
> + TSTBUS_UTP_HCI,
> + TSTBUS_COMBINED,
> + TSTBUS_WRAPPER,
> + TSTBUS_UNIPRO,
> + TSTBUS_MAX,
> +};
>
> static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>
> @@ -30,6 +48,15 @@ static void ufs_qcom_get_speed_mode(struct
> ufs_pa_layer_attr *p, char *result);
> static int ufs_qcom_get_bus_vote(struct ufs_qcom_host *host,
> const char *speed_mode);
> static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote);
> +static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> +static void ufs_qcom_dump_regs(struct ufs_hba *hba, int offset, int len,
> + char *prefix)
> +{
> + print_hex_dump(KERN_ERR, prefix,
> + len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,
> + 16, 4, (void __force *)hba->mmio_base + offset,
> + len * 4, false);
> +}
>
> static int ufs_qcom_get_connected_tx_lanes(struct ufs_hba *hba, u32
> *tx_lanes)
> {
> @@ -996,6 +1023,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
> ufs_qcom_hosts[hba->dev->id] = host;
>
> + host->dbg_print_en |= UFS_QCOM_DEFAULT_DBG_PRINT_EN;
> + ufs_qcom_get_default_testbus_cfg(host);
> + err = ufs_qcom_testbus_config(host);
> + if (err) {
> + dev_warn(dev, "%s: failed to configure the testbus %d\n",
> + __func__, err);
> + err = 0;
> + }
> +
> goto out;
>
> out_disable_phy:
> @@ -1025,12 +1061,134 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba
> *hba)
>
> if (!dev_req_params)
> return;
> +}
> +
> +static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host)
> +{
> + /* provide a legal default configuration */
> + host->testbus.select_major = TSTBUS_UAWM;
> + host->testbus.select_minor = 1;
> +}
> +
> +static bool ufs_qcom_testbus_cfg_is_ok(struct ufs_qcom_host *host)
> +{
> + if (host->testbus.select_major >= TSTBUS_MAX) {
> + dev_err(host->hba->dev,
> + "%s: UFS_CFG1[TEST_BUS_SEL} may not equal 0x%05X\n",
> + __func__, host->testbus.select_major);
> + return false;
> + }
> +
> + /*
> + * Not performing check for each individual select_major
> + * mappings of select_minor, since there is no harm in
> + * configuring a non-existent select_minor
> + */
> + if (host->testbus.select_minor > 0x1F) {
> + dev_err(host->hba->dev,
> + "%s: 0x%05X is not a legal testbus option\n",
> + __func__, host->testbus.select_minor);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +int ufs_qcom_testbus_config(struct ufs_qcom_host *host)
> +{
> + int reg;
> + int offset;
> + u32 mask = TEST_BUS_SUB_SEL_MASK;
> +
> + if (!host)
> + return -EINVAL;
>
> - ufs_qcom_cfg_timers(hba, dev_req_params->gear_rx,
> - dev_req_params->pwr_rx,
> - dev_req_params->hs_rate);
> + if (!ufs_qcom_testbus_cfg_is_ok(host))
> + return -EPERM;
> +
> + switch (host->testbus.select_major) {
> + case TSTBUS_UAWM:
> + reg = UFS_TEST_BUS_CTRL_0;
> + offset = 24;
> + break;
> + case TSTBUS_UARM:
> + reg = UFS_TEST_BUS_CTRL_0;
> + offset = 16;
> + break;
> + case TSTBUS_TXUC:
> + reg = UFS_TEST_BUS_CTRL_0;
> + offset = 8;
> + break;
> + case TSTBUS_RXUC:
> + reg = UFS_TEST_BUS_CTRL_0;
> + offset = 0;
> + break;
> + case TSTBUS_DFC:
> + reg = UFS_TEST_BUS_CTRL_1;
> + offset = 24;
> + break;
> + case TSTBUS_TRLUT:
> + reg = UFS_TEST_BUS_CTRL_1
Re: [PATCH v3 8/8] scsi: ufs-qcom: add QUniPro hardware support and power optimizations
Reviewed-by: Akinobu Mita
> New revisions of UFS host controller supports the new UniPro
> hardware controller (referred as QUniPro). This patch adds
> the support to enable this new UniPro controller hardware.
>
> This change also adds power optimization for bus scaling feature,
> as well as support for HS-G3 power mode.
>
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 640
>
> drivers/scsi/ufs/ufs-qcom.h | 31 ++-
> drivers/scsi/ufs/ufshcd.c | 8 +-
> drivers/scsi/ufs/ufshcd.h | 27 +-
> 4 files changed, 525 insertions(+), 181 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 4d19c49..c99eac9 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -44,11 +44,11 @@ enum {
>
> static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>
> -static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char
> *result);
> -static int ufs_qcom_get_bus_vote(struct ufs_qcom_host *host,
> - const char *speed_mode);
> static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote);
> static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> +static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba
> *hba,
> +u32 clk_cycles);
> +
> static void ufs_qcom_dump_regs(struct ufs_hba *hba, int offset, int len,
> char *prefix)
> {
> @@ -177,6 +177,7 @@ static int ufs_qcom_init_lane_clks(struct
> ufs_qcom_host *host)
>
> err = ufs_qcom_host_clk_get(dev, "tx_lane1_sync_clk",
> &host->tx_l1_sync_clk);
> +
> out:
> return err;
> }
> @@ -209,7 +210,9 @@ static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
>
> do {
> err = ufshcd_dme_get(hba,
> - UIC_ARG_MIB(MPHY_TX_FSM_STATE), &tx_fsm_val);
> + UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> + UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> + &tx_fsm_val);
> if (err || tx_fsm_val == TX_FSM_HIBERN8)
> break;
>
> @@ -223,7 +226,9 @@ static int ufs_qcom_check_hibern8(struct ufs_hba *hba)
>*/
> if (time_after(jiffies, timeout))
> err = ufshcd_dme_get(hba,
> - UIC_ARG_MIB(MPHY_TX_FSM_STATE), &tx_fsm_val);
> + UIC_ARG_MIB_SEL(MPHY_TX_FSM_STATE,
> + UIC_ARG_MPHY_TX_GEN_SEL_INDEX(0)),
> + &tx_fsm_val);
>
> if (err) {
> dev_err(hba->dev, "%s: unable to get TX_FSM_STATE, err %d\n",
> @@ -237,6 +242,15 @@ static int ufs_qcom_check_hibern8(struct ufs_hba
> *hba)
> return err;
> }
>
> +static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
> +{
> + ufshcd_rmwl(host->hba, QUNIPRO_SEL,
> +ufs_qcom_cap_qunipro(host) ? QUNIPRO_SEL : 0,
> +REG_UFS_CFG1);
> + /* make sure above configuration is applied before we return */
> + mb();
> +}
> +
> static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> {
> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> @@ -251,9 +265,11 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba
> *hba)
> usleep_range(1000, 1100);
>
> ret = ufs_qcom_phy_calibrate_phy(phy, is_rate_B);
> +
> if (ret) {
> - dev_err(hba->dev, "%s: ufs_qcom_phy_calibrate_phy() failed, ret
> =
> %d\n",
> - __func__, ret);
> + dev_err(hba->dev,
> + "%s: ufs_qcom_phy_calibrate_phy()failed, ret = %d\n",
> + __func__, ret);
> goto out;
> }
>
> @@ -274,9 +290,12 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba
> *hba)
>
> ret = ufs_qcom_phy_is_pcs_ready(phy);
> if (ret)
> - dev_err(hba->dev, "%s: is_physical_coding_sublayer_ready()
> failed, ret
> = %d\n",
> + dev_err(hba->dev,
> + "%s: is_physical_coding_sublayer_ready() failed, ret =
> %d\n",
> __func__, ret);
>
> + ufs_qcom_select_unipro_mode(host);
> +
> out:
> return ret;
> }
> @@ -299,7 +318,8 @@ static void ufs_qcom_enable_hw_clk_gating(struct
> ufs_hba *hba)
> mb();
> }
>
> -static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba, bool status)
> +static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
> + enum ufs_notify_change_status status)
> {
> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> int err = 0;
> @@ -329,12 +349,12 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba
> *hba, bool status)
> }
>
> /**
> - * Returns non-zero for success (which rate of core_clk) and 0
> - * in case of a failure
> + * Returns zero for success and non-zero in case of a failure
Re: [PATCH v3 5/8] scsi: ufs: creates wrapper functions for vops
Reviewed-by: Akinobu Mita
> In order to simplify the code a set of wrapper functions is created
> to test and call each of the variant operations.
>
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/scsi/ufs/ufs-qcom.c | 1 -
> drivers/scsi/ufs/ufshcd.c | 104
> +---
> drivers/scsi/ufs/ufshcd.h | 98
> +
> 3 files changed, 137 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 64c54b7..329ac84 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1049,6 +1049,5 @@ static const struct ufs_hba_variant_ops
> ufs_hba_qcom_vops = {
> .suspend= ufs_qcom_suspend,
> .resume = ufs_qcom_resume,
> };
> -EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b0ade73..9e79c33 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -271,10 +271,8 @@ static inline u32 ufshcd_get_intr_mask(struct ufs_hba
> *hba)
> */
> static inline u32 ufshcd_get_ufs_version(struct ufs_hba *hba)
> {
> - if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION) {
> - if (hba->vops && hba->vops->get_ufs_hci_version)
> - return hba->vops->get_ufs_hci_version(hba);
> - }
> + if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
> + return ufshcd_vops_get_ufs_hci_version(hba);
>
> return ufshcd_readl(hba, REG_UFS_VERSION);
> }
> @@ -2473,9 +2471,8 @@ static int ufshcd_change_power_mode(struct ufs_hba
> *hba,
> dev_err(hba->dev,
> "%s: power mode change failed %d\n", __func__, ret);
> } else {
> - if (hba->vops && hba->vops->pwr_change_notify)
> - hba->vops->pwr_change_notify(hba,
> - POST_CHANGE, NULL, pwr_mode);
> + ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, NULL,
> + pwr_mode);
>
> memcpy(&hba->pwr_info, pwr_mode,
> sizeof(struct ufs_pa_layer_attr));
> @@ -2495,10 +2492,10 @@ static int ufshcd_config_pwr_mode(struct ufs_hba
> *hba,
> struct ufs_pa_layer_attr final_params = { 0 };
> int ret;
>
> - if (hba->vops && hba->vops->pwr_change_notify)
> - hba->vops->pwr_change_notify(hba,
> - PRE_CHANGE, desired_pwr_mode, &final_params);
> - else
> + ret = ufshcd_vops_pwr_change_notify(hba, PRE_CHANGE,
> + desired_pwr_mode, &final_params);
> +
> + if (ret)
> memcpy(&final_params, desired_pwr_mode, sizeof(final_params));
>
> ret = ufshcd_change_power_mode(hba, &final_params);
> @@ -2647,8 +2644,7 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
> /* UniPro link is disabled at this point */
> ufshcd_set_link_off(hba);
>
> - if (hba->vops && hba->vops->hce_enable_notify)
> - hba->vops->hce_enable_notify(hba, PRE_CHANGE);
> + ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
>
> /* start controller initialization sequence */
> ufshcd_hba_start(hba);
> @@ -2681,8 +2677,7 @@ static int ufshcd_hba_enable(struct ufs_hba *hba)
> /* enable UIC related interrupts */
> ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
>
> - if (hba->vops && hba->vops->hce_enable_notify)
> - hba->vops->hce_enable_notify(hba, POST_CHANGE);
> + ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
>
> return 0;
> }
> @@ -2735,8 +2730,7 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
> int retries = DME_LINKSTARTUP_RETRIES;
>
> do {
> - if (hba->vops && hba->vops->link_startup_notify)
> - hba->vops->link_startup_notify(hba, PRE_CHANGE);
> + ufshcd_vops_link_startup_notify(hba, PRE_CHANGE);
>
> ret = ufshcd_dme_link_startup(hba);
>
> @@ -2767,11 +2761,9 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
> }
>
> /* Include any host controller configuration via UIC commands */
> - if (hba->vops && hba->vops->link_startup_notify) {
> - ret = hba->vops->link_startup_notify(hba, POST_CHANGE);
> - if (ret)
> - goto out;
> - }
> + ret = ufshcd_vops_link_startup_notify(hba, POST_CHANGE);
> + if (ret)
> + goto out;
>
> ret = ufshcd_make_hba_operational(hba);
> out:
> @@ -4578,8 +4570,7 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
> }
> }
>
> - if (hba->vops && hba->vops->setup_clocks)
> - ret = hba->vops->setup_clocks(hba, on);
> + ret = ufshcd_vops_setup_clocks(hba, on);
> out:
> if (ret) {
> list_for_each_entry(clki, head, list) {
> @@ -4645,27 +4636
Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device
Reviewed-by: Akinobu Mita
> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS
> a platform device.
> In order to do so a few additional changes are required:
> 1. The ufshcd-pltfrm is no longer serves as a platform device.
>Now it only serves as a group of platform APIs such as PM APIs
>(runtime suspend/resume, system suspend/resume etc), parsers of
>clocks, regulators and pm_levels from DT.
> 2. What used to be the old platform "probe" is now "only"
>a pltfrm_init() routine, that does exactly the same, but only
>being called by the new probe function of the UFS variant.
>
> Signed-off-by: Yaniv Gardi
>
> ---
> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 2 +-
> drivers/scsi/ufs/ufs-qcom.c| 78
> +-
> drivers/scsi/ufs/ufshcd-pltfrm.c | 92
> ++
> drivers/scsi/ufs/ufshcd-pltfrm.h | 41 ++
> drivers/scsi/ufs/ufshcd.c | 10 +++
> drivers/scsi/ufs/ufshcd.h | 1 +
> 6 files changed, 152 insertions(+), 72 deletions(-)
> create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h
>
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index 5357919..b39e765 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -4,7 +4,7 @@ UFSHC nodes are defined to describe on-chip UFS host
> controllers.
> Each UFS controller instance should have its own node.
>
> Required properties:
> -- compatible: compatible list, contains "jedec,ufs-1.1"
> +- compatible: compatible list, contains "jedec,ufs-1.1" or
> "qcom,ufshc"
> - interrupts:
> - reg :
>
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 329ac84..8027435 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -19,6 +19,7 @@
>
> #include
> #include "ufshcd.h"
> +#include "ufshcd-pltfrm.h"
> #include "unipro.h"
> #include "ufs-qcom.h"
> #include "ufshci.h"
> @@ -1036,7 +1037,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba *hba)
> * The variant operations configure the necessary controller and PHY
> * handshake during initialization.
> */
> -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
> .name = "qcom",
> .init = ufs_qcom_init,
> .exit = ufs_qcom_exit,
> @@ -1050,4 +1051,79 @@ static const struct ufs_hba_variant_ops
> ufs_hba_qcom_vops = {
> .resume = ufs_qcom_resume,
> };
>
> +/**
> + * ufs_qcom_probe - probe routine of the driver
> + * @pdev: pointer to Platform device handle
> + *
> + * Always return 0
> + */
> +static int ufs_qcom_probe(struct platform_device *pdev)
> +{
> + int err;
> + struct device *dev = &pdev->dev;
> + struct ufs_hba *hba;
> +
> + /* Perform generic probe */
> + err = ufshcd_pltfrm_init(pdev, &ufs_hba_qcom_vops);
> + if (err) {
> + dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
> + goto out;
> + }
> +
> + hba = platform_get_drvdata(pdev);
> + if (unlikely(!hba)) {
> + dev_err(dev, "no hba structure after successful probing\n");
> + goto dealloc_host;
> + }
> +
> + return 0;
> +
> +dealloc_host:
> + /* disconnect the bind between the qcom host and the hba */
> + ufshcd_set_variant(hba, NULL);
> + ufshcd_dealloc_host(hba);
> +out:
> + return err;
> +}
> +
> +/**
> + * ufs_qcom_remove - set driver_data of the device to NULL
> + * @pdev: pointer to platform device handle
> + *
> + * Always return 0
> + */
> +static int ufs_qcom_remove(struct platform_device *pdev)
> +{
> + struct ufs_hba *hba = platform_get_drvdata(pdev);
> +
> + pm_runtime_get_sync(&(pdev)->dev);
> + ufshcd_remove(hba);
> + return 0;
> +}
> +
> +static const struct of_device_id ufs_qcom_of_match[] = {
> + { .compatible = "qcom,ufshc"},
> + {},
> +};
> +
> +static const struct dev_pm_ops ufs_qcom_pm_ops = {
> + .suspend= ufshcd_pltfrm_suspend,
> + .resume = ufshcd_pltfrm_resume,
> + .runtime_suspend = ufshcd_pltfrm_runtime_suspend,
> + .runtime_resume = ufshcd_pltfrm_runtime_resume,
> + .runtime_idle= ufshcd_pltfrm_runtime_idle,
> +};
> +
> +static struct platform_driver ufs_qcom_pltform = {
> + .probe = ufs_qcom_probe,
> + .remove = ufs_qcom_remove,
> + .shutdown = ufshcd_pltfrm_shutdown,
> + .driver = {
> + .name = "ufshcd-qcom",
> + .pm = &ufs_qcom_pm_ops,
> + .of_match_table = of_match_ptr(ufs_qcom_of_match),
> + },
> +};
> +module_platform_driver(ufs_qcom_pltform);
> +
> MODULE_LICENSE("GPL v2");
> diff
Re: [PATCH v3 1/8] phy: qcom-ufs: fix build error when the component is built as a module
Reviewed-by: Akinobu Mita
> Export the following functions in order to avoid build errors
> when the component PHY_QCOM_UFS is compiled as a module:
>
> ERROR: "ufs_qcom_phy_disable_ref_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_ref_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_is_pcs_ready"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_disable_iface_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_start_serdes"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_calibrate_phy"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_dev_ref_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_set_tx_lane_enable"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_disable_dev_ref_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_save_controller_version"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_iface_clk"
> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> make[1]: *** [__modpost] Error 1
>
> Signed-off-by: Yaniv Gardi
>
> ---
> drivers/phy/phy-qcom-ufs.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
> index f9c618f..6140a8b 100644
> --- a/drivers/phy/phy-qcom-ufs.c
> +++ b/drivers/phy/phy-qcom-ufs.c
> @@ -432,6 +432,7 @@ out_disable_src:
> out:
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_ref_clk);
>
> static
> int ufs_qcom_phy_disable_vreg(struct phy *phy,
> @@ -474,6 +475,7 @@ void ufs_qcom_phy_disable_ref_clk(struct phy
> *generic_phy)
> phy->is_ref_clk_enabled = false;
> }
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_ref_clk);
>
> #define UFS_REF_CLK_EN (1 << 5)
>
> @@ -517,11 +519,13 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy
> *generic_phy)
> {
> ufs_qcom_phy_dev_ref_clk_ctrl(generic_phy, true);
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_dev_ref_clk);
>
> void ufs_qcom_phy_disable_dev_ref_clk(struct phy *generic_phy)
> {
> ufs_qcom_phy_dev_ref_clk_ctrl(generic_phy, false);
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_dev_ref_clk);
>
> /* Turn ON M-PHY RMMI interface clocks */
> int ufs_qcom_phy_enable_iface_clk(struct phy *generic_phy)
> @@ -550,6 +554,7 @@ int ufs_qcom_phy_enable_iface_clk(struct phy
> *generic_phy)
> out:
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_iface_clk);
>
> /* Turn OFF M-PHY RMMI interface clocks */
> void ufs_qcom_phy_disable_iface_clk(struct phy *generic_phy)
> @@ -562,6 +567,7 @@ void ufs_qcom_phy_disable_iface_clk(struct phy
> *generic_phy)
> phy->is_iface_clk_enabled = false;
> }
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_iface_clk);
>
> int ufs_qcom_phy_start_serdes(struct phy *generic_phy)
> {
> @@ -578,6 +584,7 @@ int ufs_qcom_phy_start_serdes(struct phy *generic_phy)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_start_serdes);
>
> int ufs_qcom_phy_set_tx_lane_enable(struct phy *generic_phy, u32
> tx_lanes)
> {
> @@ -595,6 +602,7 @@ int ufs_qcom_phy_set_tx_lane_enable(struct phy
> *generic_phy, u32 tx_lanes)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_set_tx_lane_enable);
>
> void ufs_qcom_phy_save_controller_version(struct phy *generic_phy,
> u8 major, u16 minor, u16 step)
> @@ -605,6 +613,7 @@ void ufs_qcom_phy_save_controller_version(struct phy
> *generic_phy,
> ufs_qcom_phy->host_ctrl_rev_minor = minor;
> ufs_qcom_phy->host_ctrl_rev_step = step;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_save_controller_version);
>
> int ufs_qcom_phy_calibrate_phy(struct phy *generic_phy, bool is_rate_B)
> {
> @@ -625,6 +634,7 @@ int ufs_qcom_phy_calibrate_phy(struct phy
> *generic_phy, bool is_rate_B)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_calibrate_phy);
>
> int ufs_qcom_phy_remove(struct phy *generic_phy,
> struct ufs_qcom_phy *ufs_qcom_phy)
> @@ -662,6 +672,7 @@ int ufs_qcom_phy_is_pcs_ready(struct phy *generic_phy)
> return ufs_qcom_phy->phy_spec_ops->
> is_physical_coding_sublayer_ready(ufs_qcom_phy);
> }
> +EXPORT_SYMBOL_GPL(ufs_qcom_phy_is_pcs_ready);
>
> int ufs_qcom_phy_power_on(struct phy *generic_phy)
> {
> --
> 1.8.5.2
>
> --
> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/8] Fix error message and present UFS variant
Reviewed-by: Akinobu Mita > V3: fixes a few minor issues. > > V2: fixes a few issues of unnecessary EXPORT_SYMBOL, > types of parameters in routine definition, > build errors in case CONFIG_PM is not defined and some > other minor fixes. > > Yaniv Gardi (8): > phy: qcom-ufs: fix build error when the component is built as a module > scsi: ufs-qcom: fix compilation warning if compiled as a module > scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component > add ufshcd_get_variant ufshcd_set_variant > scsi: ufs: creates wrapper functions for vops > scsi: ufs: make the UFS variant a platform device > scsi: ufs-qcom: add debug prints for test bus > scsi: ufs-qcom: add QUniPro hardware support and power optimizations > > .../devicetree/bindings/ufs/ufshcd-pltfrm.txt | 2 +- > drivers/phy/phy-qcom-ufs.c | 11 + > drivers/scsi/ufs/Kconfig | 2 +- > drivers/scsi/ufs/ufs-qcom.c| 921 > - > drivers/scsi/ufs/ufs-qcom.h| 68 +- > drivers/scsi/ufs/ufshcd-pltfrm.c | 92 +- > drivers/scsi/ufs/ufshcd-pltfrm.h | 41 + > drivers/scsi/ufs/ufshcd.c | 122 ++- > drivers/scsi/ufs/ufshcd.h | 149 +++- > 9 files changed, 1072 insertions(+), 336 deletions(-) > create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h > > -- > 1.8.5.2 > > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/8] Fix error message and present UFS variant
Thank you Mita, We appreciate your comments and your time. will add "Reviewed-by". regards, Yaniv > Hi Yaniv, > > 2015-08-23 22:09 GMT+09:00 Yaniv Gardi : >> V3: fixes a few minor issues. >> >> V2: fixes a few issues of unnecessary EXPORT_SYMBOL, >> types of parameters in routine definition, >> build errors in case CONFIG_PM is not defined and some >> other minor fixes. > > I've checked outstanding issues I reported for v1 and v2 are fixed > in this version of series. So please feel free to add: > > Reviewed-by: Akinobu Mita > > I still think that we should introduce print_hex_dump_io() or > something simpler for dumping __iomem pointer instead of casting > 'void __force *'. But it is only used for debug dump function, so > I don't too much worry about it. > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/8] scsi: ufs: make the UFS variant a platform device
> O
> n Aug 20, 2015 6:59 AM, "Yaniv Gardi" wrote:
>>
>> This change turns the UFS variant (SCSI_UFS_QCOM) into a UFS
>> a platform device.
>> In order to do so a few additional changes are required:
>> 1. The ufshcd-pltfrm is no longer serves as a platform device.
>>Now it only serves as a group of platform APIs such as PM APIs
>>(runtime suspend/resume, system suspend/resume etc), parsers of
>>clocks, regulators and pm_levels from DT.
>> 2. What used to be the old platform "probe" is now "only" a
>> pltfrm_init()
>>routine, that does exactly the same, but only being called by the
>>new probe function of the UFS variant.
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/scsi/ufs/ufs-qcom.c | 83
>> +++-
>> drivers/scsi/ufs/ufshcd-pltfrm.c | 92
>> ++--
>> drivers/scsi/ufs/ufshcd-pltfrm.h | 41 ++
>> drivers/scsi/ufs/ufshcd.c| 10 +
>> drivers/scsi/ufs/ufshcd.h| 1 +
>> 5 files changed, 156 insertions(+), 71 deletions(-)
>> create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h
>>
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 329ac84..725cd49 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -19,6 +19,7 @@
>>
>> #include
>> #include "ufshcd.h"
>> +#include "ufshcd-pltfrm.h"
>> #include "unipro.h"
>> #include "ufs-qcom.h"
>> #include "ufshci.h"
>> @@ -1036,7 +1037,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba
>> *hba)
>> * The variant operations configure the necessary controller and PHY
>> * handshake during initialization.
>> */
>> -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>> +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>> .name = "qcom",
>> .init = ufs_qcom_init,
>> .exit = ufs_qcom_exit,
>> @@ -1050,4 +1051,84 @@ static const struct ufs_hba_variant_ops
>> ufs_hba_qcom_vops = {
>> .resume = ufs_qcom_resume,
>> };
>>
>> +/**
>> + * ufs_qcom_probe - probe routine of the driver
>> + * @pdev: pointer to Platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_probe(struct platform_device *pdev)
>> +{
>> + int err;
>> + struct device *dev = &pdev->dev;
>> + struct ufs_hba *hba;
>> +
>> + /* Perform generic probe */
>> + err = ufshcd_pltfrm_init(pdev, &ufs_hba_qcom_vops);
>> + if (err) {
>> + dev_err(dev, "ufshcd_pltfrm_init() failed %d\n", err);
>> + goto out;
>> + }
>> +
>> + hba = platform_get_drvdata(pdev);
>> + if (unlikely(!hba)) {
>
> Shouldn't this condition be caught by init above?
actually yes, it should.
>
>> + dev_err(dev, "no hba structure after successful
>> probing\n");
>> + goto dealloc_host;
>> + }
>> +
>> + return 0;
>> +
>> +dealloc_host:
>> + /* disconnect the bind between the qcom host and the hba */
>> + ufshcd_set_variant(hba, NULL);
>> + ufshcd_dealloc_host(hba);
>> +out:
>> + return err;
>> +}
>> +
>> +/**
>> + * ufs_qcom_remove - set driver_data of the device to NULL
>> + * @pdev: pointer to platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_remove(struct platform_device *pdev)
>> +{
>> + struct ufs_hba *hba = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_get_sync(&(pdev)->dev);
>> + ufshcd_remove(hba);
>> + return 0;
>> +}
>> +
>> +static void ufs_qcom_shutdown(struct platform_device *pdev)
>> +{
>> + ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
>> +}
>> +
>> +static const struct of_device_id ufs_qcom_of_match[] = {
>> + { .compatible = "qcom,ufshc"},
>
> Is this documented?
in V3 it is documented. thanks
>
>> + {},
>> +};
>> +
>> +static const struct dev_pm_ops ufs_qcom_pm_ops = {
>> + .suspend= ufshcd_pltfrm_suspend,
>> + .resume = ufshcd_pltfrm_resume,
>> + .runtime_suspend = ufshcd_pltfrm_runtime_suspend,
>> + .runtime_resume = ufshcd_pltfrm_runtime_resume,
>> + .runtime_idle= ufshcd_pltfrm_runtime_idle,
>> +};
>> +
>> +static struct platform_driver ufs_qcom_pltform = {
>> + .probe = ufs_qcom_probe,
>> + .remove = ufs_qcom_remove,
>> + .shutdown = ufs_qcom_shutdown,
>> + .driver = {
>> + .name = "ufshcd-qcom",
>> + .pm = &ufs_qcom_pm_ops,
>> + .of_match_table = of_match_ptr(ufs_qcom_of_match),
>> + },
>> +};
>> +module_platform_driver(ufs_qcom_pltform);
>> +
>> MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 7db9564..91c73934 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -38,20 +38,7 @@
>> #include
>>
Re: [PATCH v2 7/8] scsi: ufs-qcom: add debug prints for test bus
> 2015-08-20 22:59 GMT+09:00 Yaniv Gardi :
>
>> +static bool ufs_qcom_testbus_cfg_is_ok(struct ufs_qcom_host *host)
>> +{
>> + if (host->testbus.select_major >= TSTBUS_MAX) {
>> + dev_err(host->hba->dev,
>> + "%s: UFS_CFG1[TEST_BUS_SEL} may not equal
>> 0x%05X\n",
>> + __func__, host->testbus.select_major);
>> + return false;
>> + }
>> +
>> + /*
>> +* Not performing check for each individual select_major
>> +* mappings of select_minor, since there is no harm in
>> +* configuring a non-existent select_minor
>> +*/
>> + if (host->testbus.select_major > 0x1F) {
>
> select_minor should be checked instead of select_major here?
you are correct. should be select_minor
>
>> + dev_err(host->hba->dev,
>> + "%s: 0x%05X is not a legal testbus option\n",
>> + __func__, host->testbus.select_minor);
>> + return false;
>> + }
>> +
>> + return true;
>> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] scsi: ufs-qcom: add debug prints for test bus
> 2015-08-20 22:59 GMT+09:00 Yaniv Gardi :
>> @@ -30,6 +48,14 @@ static void ufs_qcom_get_speed_mode(struct
>> ufs_pa_layer_attr *p, char *result);
>> static int ufs_qcom_get_bus_vote(struct ufs_qcom_host *host,
>> const char *speed_mode);
>> static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote);
>> +static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host
>> *host);
>> +static void ufs_qcom_dump_regs(struct ufs_hba *hba, int offset, int
>> len,
>> + char *prefix)
>> +{
>> + print_hex_dump(KERN_ERR, prefix,
>> + len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,
>> + 16, 4, hba->mmio_base + offset, len * 4, false);
>> +}
>
> This causes a sparse warning as __iomem pointer is passed to
> print_hex_dump().
indeed.
any suggestions how it can be fixed ? I guess I shall try casting.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/8] scsi: ufs: make the UFS variant a platform device
> 2015-08-20 22:59 GMT+09:00 Yaniv Gardi :
>> @@ -1036,7 +1037,7 @@ void ufs_qcom_clk_scale_notify(struct ufs_hba
>> *hba)
>> * The variant operations configure the necessary controller and PHY
>> * handshake during initialization.
>> */
>> -static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>> +static struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>
> It's better to keep const. In order to do this, we also need to put
> const to 'vops' member in struct ufs_hba and the second argument of
> ufshcd_pltfrm_init().
Why do you think it would be better to keep the vops as a const and have
the vops member in struct ufs_hba be const ?
I would say it is better to have the vops as is, and let each variany
decidehow it should behave.
>
>> +static void ufs_qcom_shutdown(struct platform_device *pdev)
>> +{
>> + ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
>> +}
>
> We don't need this function anymore since we have
> ufshcd_pltfrm_shutdown() now.
ok, as this variant shutdown is doing the same as pltform shutdown' it can
go away.
>
>> -static void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
>> +void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
>> {
>> - ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
>> + ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
>
> Whitespace is used as code indent. There are same issues in
> this patch series, so I recommend running checkpatch.pl before
> sending patches.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 8/8] scsi: ufs-qcom: add QUniPro hardware support and power optimizations
Thank you Mita for the review.
Your comments were correct and helpful.
I appreciate your time and effort.
please see inline.
> 2015-08-16 19:14 GMT+09:00 Yaniv Gardi :
>> @@ -1208,6 +1510,7 @@ static struct ufs_hba_variant_ops
>> ufs_hba_qcom_vops = {
>> .resume = ufs_qcom_resume,
>> .dbg_register_dump = ufs_qcom_dump_dbg_regs,
>> };
>> +EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> As I said in the view comment for the patch 5, this shouldn't be exported
> again.
I removed this export at all. it is not needed anymore.
>
>> @@ -775,6 +781,12 @@ static inline int ufshcd_vops_resume(struct ufs_hba
>> *hba, enum ufs_pm_op op)
>> return 0;
>> }
>>
>> +static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba)
>> +{
>> + if (hba->vops && hba->vops->dbg_register_dump)
>> + hba->vops->dbg_register_dump(hba);
>> +}
>> +
>
> This change should be done in the patch 7?
you are correct.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 6/8] scsi: ufs: make the UFS variant a platform device
> 2015-08-16 19:14 GMT+09:00 Yaniv Gardi :
>> +/**
>> + * ufs_qcom_remove - set driver_data of the device to NULL
>> + * @pdev: pointer to platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_remove(struct platform_device *pdev)
>> +{
>> + struct ufs_hba *hba = platform_get_drvdata(pdev);
>> +
>> + pm_runtime_get_sync(&(pdev)->dev);
>> + ufshcd_remove(hba);
>> + ufshcd_dealloc_host(hba);
>
> scsi_host_put (== ufshcd_dealloc_host) is already called in
> ufshcd_remove().
> So we shouldn't call it here again.
good point. will be fixed.
>
>> +static struct platform_driver ufs_qcom_pltform = {
>> + .probe = ufs_qcom_probe,
>> + .remove = ufs_qcom_remove,
>> + .shutdown = ufs_qcom_shutdown,
>> + .driver = {
>> + .name = "ufshcd-qcom",
>> + .owner = THIS_MODULE,
>
> We don't need to set .owner. Please see commit 37b6fea57b4
> ("scsi: ufs: drop owner assignment from platform_drivers").
>
correct. will be removed.
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 7db9564..20009a9 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -38,20 +38,9 @@
>> #include
>>
>> #include "ufshcd.h"
>> +#include "ufshcd-pltfrm.h"
>>
>> static const struct of_device_id ufs_of_match[];
>
> We can remove this forward declaration as ufs_of_match is removed below.
done.
>
>> @@ -245,10 +234,11 @@ out:
>> * Returns 0 if successful
>> * Returns non-zero otherwise
>> */
>> -static int ufshcd_pltfrm_suspend(struct device *dev)
>> +int ufshcd_pltfrm_suspend(struct device *dev)
>> {
>> return ufshcd_system_suspend(dev_get_drvdata(dev));
>> }
>> +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_suspend);
>>
>> /**
>> * ufshcd_pltfrm_resume - resume power management function
>> @@ -257,23 +247,30 @@ static int ufshcd_pltfrm_suspend(struct device
>> *dev)
>> * Returns 0 if successful
>> * Returns non-zero otherwise
>> */
>> -static int ufshcd_pltfrm_resume(struct device *dev)
>> +int ufshcd_pltfrm_resume(struct device *dev)
>> {
>> return ufshcd_system_resume(dev_get_drvdata(dev));
>> }
>> +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_resume);
>>
>> -static int ufshcd_pltfrm_runtime_suspend(struct device *dev)
>> +int ufshcd_pltfrm_runtime_suspend(struct device *dev)
>> {
>> return ufshcd_runtime_suspend(dev_get_drvdata(dev));
>> }
>> -static int ufshcd_pltfrm_runtime_resume(struct device *dev)
>> +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_suspend);
>> +
>> +int ufshcd_pltfrm_runtime_resume(struct device *dev)
>> {
>> return ufshcd_runtime_resume(dev_get_drvdata(dev));
>> }
>> -static int ufshcd_pltfrm_runtime_idle(struct device *dev)
>> +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_resume);
>> +
>> +int ufshcd_pltfrm_runtime_idle(struct device *dev)
>> {
>> return ufshcd_runtime_idle(dev_get_drvdata(dev));
>> }
>> +EXPORT_SYMBOL_GPL(ufshcd_pltfrm_runtime_idle);
>> +
>> #else /* !CONFIG_PM */
>> #define ufshcd_pltfrm_suspend NULL
>> #define ufshcd_pltfrm_resume NULL
>
> Since ufshcd_pltfrm_suspend()/resume() and ufshcd_pltfrm_runtime_*()
> are only defined when CONFIG_PM=y, ufs-qcom.c can't be built when
> !CONFIG_PM.
>
> These #ifdef should be moved to ufshcd-pltfrm.h, or we can export
> ufshcd_dev_pm_ops instead of the pm functions.
>
I wasn't sure what you mean by "ufs-qcom.c can't be built when !CONFIG_PM",
but anyhow, you had a point here:
in case !CONFIG_PM:
#define ufshcd_pltfrm_*
are not familiar in ufs_qcom.c
so in V2 i will take care of this, moving it to ufshcd_pltfrm.h file
>> @@ -282,18 +279,15 @@ static int ufshcd_pltfrm_runtime_idle(struct
>> device *dev)
>> #define ufshcd_pltfrm_runtime_idle NULL
>> #endif /* CONFIG_PM */
>>
>> -static void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
>> -{
>> - ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
>> -}
>
> How about exporting this function? ufs-qcom and other variant can use
> this.
>
you got a point here. will undo the deletion of this routine.
>> +#ifndef UFSHCD_PLTFRM_H_
>> +#define UFSHCD_PLTFRM_H_
>> +
>> +#include "ufshcd.h"
>> +
>> +int ufshcd_pltfrm_init(struct platform_device *pdev,
>> + struct ufs_hba_variant_ops *vops);
>
> struct platform_device appears before including .
>
as there are no build errors, i assume that ufshcd.h include some
other headers that contain include of .
>> +/* variant specific ops structures */
>> +#ifdef CONFIG_SCSI_UFS_QCOM
>> +extern struct ufs_hba_variant_ops ufs_hba_qcom_variant;
>> +#endif
>
> What is ufs_hba_qcom_variant? I can't find in kernel source and
> this patch series.
will be removed. (leftovers from different version).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
-
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
> 2015-06-05 5:53 GMT+09:00 :
>>> Hi Yaniv,
>>>
>>> 2015-06-03 18:37 GMT+09:00 Yaniv Gardi :
@@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct
platform_device *pdev)
goto out;
}
- hba->vops = get_variant_ops(&pdev->dev);
+ err = of_platform_populate(node, NULL, NULL, &pdev->dev);
+ if (err)
+ dev_err(&pdev->dev,
+ "%s: of_platform_populate() failed\n",
__func__);
+
+ ufs_variant_node = of_get_next_available_child(node, NULL);
+
+ if (!ufs_variant_node) {
+ dev_dbg(&pdev->dev, "failed to find ufs_variant_node
child\n");
+ } else {
+ ufs_variant_pdev =
of_find_device_by_node(ufs_variant_node);
+
+ if (ufs_variant_pdev)
+ hba->vops = (struct ufs_hba_variant_ops *)
+
dev_get_drvdata(&ufs_variant_pdev->dev);
+ }
>>>
>>> I have no strong objection to 'ufs_variant' sub-node. But why can't we
>>> simply add an of_device_id to ufs_of_match, like below:
>>>
>>> static const struct of_device_id ufs_of_match[] = {
>>> { .compatible = "jedec,ufs-1.1"},
>>> #if IS_ENABLED(SCSI_UFS_QCOM)
>>> { .compatible = "qcom,ufs", .data = &ufs_hba_qcom_vops },
>>> #neidf
>>> {},
>>> };
>>>
>>> and get hba->vops by get_variant_ops()?
>>>
>>
>> Hi Mita,
>> thanks for your comments.
>>
>> The whole idea, of having a sub-node which includes all variant specific
>> attributes is to separate the UFS Platform device component, from the
>> need
>> to know "qcom" or any other future variant.
>> I believe it keeps the code more modular, and clean - meaning - no
>> #ifdef's and no need to include all variant attributes inside the driver
>> DT node.
>> in that case, we simply have a DT node that is compatible to the Jdec
>> standard, and sub-node to include variant info.
>>
>> I hope you agree with this new design, since it provides a good answer
>> to every future variant that will be added, without the need to change
>> the
>> platform file.
>
> Thanks for your explanation, I agree with it.
>
> I found two problems in the current code, but both can be fixed
> relatively easily as described below:
>
> 1) If ufshcd-pltfrm driver is loaded before ufs-qcom driver,
> ufshcd_pltfrm_probe() can't find a ufs_variant device.
>
> In order to trigger re-probing ufs device when ufs-qcom driver has
> been loaded, ufshcd_pltfrm_probe() should return -EPROBE_DEFER in
> case 'ufs_variant' sub-node exists and no hba->vops found.
>
> 2) Nothing prevents ufs-qcom module from being unloaded while the
> variant_ops is referenced by ufshcd-pltfrm.
>
> It can be fixed by incrementing module refcount of ufs_variant module
> by __module_get(ufs_variant_pdev->dev.driver->owener) in
> ufshcd_pltfrm_probe(), and module_put() in ufshcd_pltfrm_remove()
> to descrement the refcount.
>
again, Mita, your comments are very appreciated.
1)
If ufshcd-pltfrm driver is loaded before ufs-qcom, (what actually happens
always), then the calling to of_platform_populate() which is added,
guarantees that ufs-qcom probe will be called and finish, before
ufshcd_pltfrm probe continues.
so ufs_variant device is always there, and ready.
I think it means we are safe - since either way, we make sure ufs-qcom
probe will be called and finish before dealing with ufs_variant device in
ufshcd_pltfrm probe.
2) you are right. the fix added as you suggested.
let us know your thoughts about the V3 once it's uploaded
thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
Thanks Paul for the review and comments.
please see inline.
> On Wed, 2015-06-03 at 12:37 +0300, Yaniv Gardi wrote:
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>
>> EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> Nothing uses this export. It's still a (static) symbol that is not
> included in any header. I think this export serves no purpose. Am I
> missing something subtle here?
>
you are correct. it will be removed.
>> +/**
>> + * ufs_qcom_probe - probe routine of the driver
>> + * @pdev: pointer to Platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_probe(struct platform_device *pdev)
>> +{
>> +dev_set_drvdata(&pdev->dev, (void *)&ufs_hba_qcom_vops);
>
> (Cast to void * should not be needed.)
>
when removing the cast there is compilation error:
ufs-qcom.c: warning: passing argument 2 of dev_set_drvdata discards
const qualifier from pointer target type
error, forbidden warning: ufs-qcom.c
>> +return 0;
>> +}
>> +
>> +/**
>> + * ufs_qcom_remove - set driver_data of the device to NULL
>> + * @pdev: pointer to platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_remove(struct platform_device *pdev)
>> +{
>> +dev_set_drvdata(&pdev->dev, NULL);
>> +return 0;
>> +}
>> +
>> +static const struct of_device_id ufs_qcom_of_match[] = {
>> +{ .compatible = "qcom,ufs_variant"},
>> +{},
>> +};
>> +
>> +static struct platform_driver ufs_qcom_pltform = {
>> +.probe = ufs_qcom_probe,
>> +.remove = ufs_qcom_remove,
>> +.driver = {
>> +.name = "ufs_qcom",
>> +.owner = THIS_MODULE,
>> +.of_match_table = of_match_ptr(ufs_qcom_of_match),
>> +},
>> +};
>> +module_platform_driver(ufs_qcom_pltform);
>
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>
>> +struct device_node *node = pdev->dev.of_node;
>> +struct device_node *ufs_variant_node;
>> +struct platform_device *ufs_variant_pdev;
>
>> -hba->vops = get_variant_ops(&pdev->dev);
>> +err = of_platform_populate(node, NULL, NULL, &pdev->dev);
>> +if (err)
>> +dev_err(&pdev->dev,
>> +"%s: of_platform_populate() failed\n", __func__);
>> +
>> +ufs_variant_node = of_get_next_available_child(node, NULL);
>> +
>> +if (!ufs_variant_node) {
>> +dev_dbg(&pdev->dev, "failed to find ufs_variant_node child\n");
>> +} else {
>> +ufs_variant_pdev = of_find_device_by_node(ufs_variant_node);
>> +
>> +if (ufs_variant_pdev)
>> +hba->vops = (struct ufs_hba_variant_ops *)
>> +dev_get_drvdata(&ufs_variant_pdev->dev);
>
> (Another cast that I think is not needed.)
here you are right - this one can be removed and will be
>
>> +}
>
> If I scanned this correctly, the dev_set_drvdata() and dev_get_drvdata()
> pair adds an actual user of ufs_hba_qcom_vops. So that ends the obvious
> issue I think the code currently has. And I gladly defer to the scsi
> people to determine whether that is done the right way.
>
> Thanks,
>
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
> Hi Yaniv,
>
> 2015-06-03 18:37 GMT+09:00 Yaniv Gardi :
>> @@ -321,7 +313,22 @@ static int ufshcd_pltfrm_probe(struct
>> platform_device *pdev)
>> goto out;
>> }
>>
>> - hba->vops = get_variant_ops(&pdev->dev);
>> + err = of_platform_populate(node, NULL, NULL, &pdev->dev);
>> + if (err)
>> + dev_err(&pdev->dev,
>> + "%s: of_platform_populate() failed\n",
>> __func__);
>> +
>> + ufs_variant_node = of_get_next_available_child(node, NULL);
>> +
>> + if (!ufs_variant_node) {
>> + dev_dbg(&pdev->dev, "failed to find ufs_variant_node
>> child\n");
>> + } else {
>> + ufs_variant_pdev =
>> of_find_device_by_node(ufs_variant_node);
>> +
>> + if (ufs_variant_pdev)
>> + hba->vops = (struct ufs_hba_variant_ops *)
>> + dev_get_drvdata(&ufs_variant_pdev->dev);
>> + }
>
> I have no strong objection to 'ufs_variant' sub-node. But why can't we
> simply add an of_device_id to ufs_of_match, like below:
>
> static const struct of_device_id ufs_of_match[] = {
> { .compatible = "jedec,ufs-1.1"},
> #if IS_ENABLED(SCSI_UFS_QCOM)
> { .compatible = "qcom,ufs", .data = &ufs_hba_qcom_vops },
> #neidf
> {},
> };
>
> and get hba->vops by get_variant_ops()?
>
Hi Mita,
thanks for your comments.
The whole idea, of having a sub-node which includes all variant specific
attributes is to separate the UFS Platform device component, from the need
to know "qcom" or any other future variant.
I believe it keeps the code more modular, and clean - meaning - no
#ifdef's and no need to include all variant attributes inside the driver
DT node.
in that case, we simply have a DT node that is compatible to the Jdec
standard, and sub-node to include variant info.
I hope you agree with this new design, since it provides a good answer
to every future variant that will be added, without the need to change the
platform file.
thanks for your time, Mita
please share your thoughts.
> There is something similar in
> drivers/net/ethernet/freescale/fsl_pq_mdio.c
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device
> On Wed, 2015-06-03 at 12:37 +0300, Yaniv Gardi wrote:
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>
>> EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> Nothing uses this export. It's still a (static) symbol that is not
> included in any header. I think this export serves no purpose. Am I
> missing something subtle here?
>
correct Paul. I will remove it.
>> +/**
>> + * ufs_qcom_probe - probe routine of the driver
>> + * @pdev: pointer to Platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_probe(struct platform_device *pdev)
>> +{
>> +dev_set_drvdata(&pdev->dev, (void *)&ufs_hba_qcom_vops);
>
> (Cast to void * should not be needed.)
>
>> +return 0;
>> +}
>> +
>> +/**
>> + * ufs_qcom_remove - set driver_data of the device to NULL
>> + * @pdev: pointer to platform device handle
>> + *
>> + * Always return 0
>> + */
>> +static int ufs_qcom_remove(struct platform_device *pdev)
>> +{
>> +dev_set_drvdata(&pdev->dev, NULL);
>> +return 0;
>> +}
>> +
>> +static const struct of_device_id ufs_qcom_of_match[] = {
>> +{ .compatible = "qcom,ufs_variant"},
>> +{},
>> +};
>> +
>> +static struct platform_driver ufs_qcom_pltform = {
>> +.probe = ufs_qcom_probe,
>> +.remove = ufs_qcom_remove,
>> +.driver = {
>> +.name = "ufs_qcom",
>> +.owner = THIS_MODULE,
>> +.of_match_table = of_match_ptr(ufs_qcom_of_match),
>> +},
>> +};
>> +module_platform_driver(ufs_qcom_pltform);
>
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>
>> +struct device_node *node = pdev->dev.of_node;
>> +struct device_node *ufs_variant_node;
>> +struct platform_device *ufs_variant_pdev;
>
>> -hba->vops = get_variant_ops(&pdev->dev);
>> +err = of_platform_populate(node, NULL, NULL, &pdev->dev);
>> +if (err)
>> +dev_err(&pdev->dev,
>> +"%s: of_platform_populate() failed\n", __func__);
>> +
>> +ufs_variant_node = of_get_next_available_child(node, NULL);
>> +
>> +if (!ufs_variant_node) {
>> +dev_dbg(&pdev->dev, "failed to find ufs_variant_node child\n");
>> +} else {
>> +ufs_variant_pdev = of_find_device_by_node(ufs_variant_node);
>> +
>> +if (ufs_variant_pdev)
>> +hba->vops = (struct ufs_hba_variant_ops *)
>> +dev_get_drvdata(&ufs_variant_pdev->dev);
>
> (Another cast that I think is not needed.)
>
>> +}
>
> If I scanned this correctly, the dev_set_drvdata() and dev_get_drvdata()
> pair adds an actual user of ufs_hba_qcom_vops. So that ends the obvious
> issue I think the code currently has. And I gladly defer to the scsi
> people to determine whether that is done the right way.
>
yes, you got it right.
these 2 routines use the vops structure, that binds the driver and the
variant (in our case qcom)
thanks for your time, Paul
> Thanks,
>
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component
> On Thu, 2015-05-21 at 10:09 +, yga...@codeaurora.org wrote: >> > On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote: >> > Am I missing something obvious here? Because ufs-qcom currently looks >> > pointless to me, and I actually see little reason to even have it in >> the >> > mainline tree. >> > >> >> we haven't uploaded yet the patch that binds qcom vops to the driver, >> but >> we will soon. > > Perhaps you could make that patch part of v2 of this series. I see > little point in this series without that patch. Perhaps someone else > still cares about it, but I'm not looking at it anymore. > fair enough. i will upload a V2 series soon. thanks for your inputs. > Thanks, > > > Paul Bolle > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component
> On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote:
>> By the way, as far as I can see, this (new) module can only be loaded
>> manually (or via scripts). Is that what people want?
>
> This comment wasn't well thought through. So I hand another look at the
> code of usf-qcom.
>
> I noticed that the single thing ufs-qcom exports is "struct
> ufs_hba_qcom_vops". But that's unused in next-20150520:
> $ git grep -nw ufs_hba_qcom_vops
> drivers/scsi/ufs/ufs-qcom.c:999: * struct ufs_hba_qcom_vops - UFS QCOM
> specific variant operations
> drivers/scsi/ufs/ufs-qcom.c:1004:static const struct
> ufs_hba_variant_ops ufs_hba_qcom_vops = {
> drivers/scsi/ufs/ufs-qcom.c:1016:EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> So it's not used by code outside of ufs-qcom.c. Probably because it
> can't actually be used by outside code. It's not mentioned in any public
> header and it's even static!
>
> Am I missing something obvious here? Because ufs-qcom currently looks
> pointless to me, and I actually see little reason to even have it in the
> mainline tree.
>
we haven't uploaded yet the patch that binds qcom vops to the driver, but
we will soon.
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 5/5] scsi: ufs-qcom-ice: add Inline Crypto Engine (ICE) support for UFS
Paul,
we have decided to revert the ICE change that support UFS.
a change already uploaded:
look for subject:
[PATCH v1] Revert "scsi: ufs-qcom-ice: add Inline Crypto Engine (ICE)
support for UFS"
thanks,
Yaniv
> Yaniv,
>
> On Thu, 2015-01-15 at 16:32 +0200, Yaniv Gardi wrote:
>> From: Yaniv Gardi
>>
>> In-order to enhance storage encryption performance,
>> an Inline Cryptographic Engine is introduced to UFS.
>> This patch adds in-line encryption capabilities to the UFS
>> driver.
>>
>> Signed-off-by: Yaniv Gardi
>
> This patch became commit 8805ccd069b7 ("ufs-qcom-ice: add Inline Crypto
> Engine (ICE) support for UFS") in today's linux-next (ie,
> next-20150123). I noticed because a script I use to check linux-next
> spotted a problem with it.
>
>> ---
>> drivers/scsi/ufs/Kconfig| 12 +
>> drivers/scsi/ufs/Makefile | 1 +
>> drivers/scsi/ufs/ufs-qcom-ice.c | 520
>>
>> drivers/scsi/ufs/ufs-qcom-ice.h | 113 +
>> drivers/scsi/ufs/ufs-qcom.c | 56 -
>> drivers/scsi/ufs/ufs-qcom.h | 25 ++
>> 6 files changed, 726 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/scsi/ufs/ufs-qcom-ice.c
>> create mode 100644 drivers/scsi/ufs/ufs-qcom-ice.h
>>
>> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>> index 8a1f4b3..ecf34ed 100644
>> --- a/drivers/scsi/ufs/Kconfig
>> +++ b/drivers/scsi/ufs/Kconfig
>> @@ -83,3 +83,15 @@ config SCSI_UFS_QCOM
>>
>>Select this if you have UFS controller on QCOM chipset.
>>If unsure, say N.
>> +
>> +config SCSI_UFS_QCOM_ICE
>> +bool "QCOM specific hooks to Inline Crypto Engine for UFS driver"
>> +depends on SCSI_UFS_QCOM && CRYPTO_DEV_QCOM_ICE
>
> There's currently no Kconfig symbol CRYPTO_DEV_QCOM_ICE in linux-next.
> So SCSI_UFS_QCOM_ICE can not be set and these "in-line encryption
> capabilities" can not be enabled.
>
>> +help
>> + This selects the QCOM specific additions to support Inline Crypto
>> + Engine (ICE).
>> + ICE accelerates the crypto operations and maintains the high UFS
>> + performance.
>> +
>> + Select this if you have ICE supported for UFS on QCOM chipset.
>> + If unsure, say N.
>> diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
>> index 8303bcc..31adca5 100644
>> --- a/drivers/scsi/ufs/Makefile
>> +++ b/drivers/scsi/ufs/Makefile
>> @@ -1,5 +1,6 @@
>> # UFSHCD makefile
>> obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o
>> +obj-$(CONFIG_SCSI_UFS_QCOM_ICE) += ufs-qcom-ice.o
>> obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o
>> obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
>> obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
>> diff --git a/drivers/scsi/ufs/ufs-qcom-ice.c
>> b/drivers/scsi/ufs/ufs-qcom-ice.c
>> new file mode 100644
>> index 000..9202b73
>> --- /dev/null
>> +++ b/drivers/scsi/ufs/ufs-qcom-ice.c
>> @@ -0,0 +1,520 @@
>> +/* Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>
> This header is not included in linux-next so manually building
> ufs-qcom-ice.o isn't possible either.
>
> I assume a series to add CRYPTO_DEV_QCOM_ICE and crypto/ice.h (and
> whatever else is needed to build this) is queued somewhere. Is that
> correct?
>
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/4] phy: qcom-ufs: add support for 20nm phy
> Hi,
>
> On Sunday 11 January 2015 06:08 PM, Yaniv Gardi wrote:
>> This change adds a support for a 20nm qcom-ufs phy that is required in
>> platforms that use ufs-qcom controller.
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/phy/Kconfig | 7 +
>> drivers/phy/Makefile| 2 +
>> drivers/phy/phy-qcom-ufs-i.h| 159
>> drivers/phy/phy-qcom-ufs-qmp-20nm.c | 257 +
>> drivers/phy/phy-qcom-ufs-qmp-20nm.h | 235
>> drivers/phy/phy-qcom-ufs.c | 745
>>
>> include/linux/phy/phy-qcom-ufs.h| 59 +++
>> 7 files changed, 1464 insertions(+)
>> create mode 100644 drivers/phy/phy-qcom-ufs-i.h
>> create mode 100644 drivers/phy/phy-qcom-ufs-qmp-20nm.c
>> create mode 100644 drivers/phy/phy-qcom-ufs-qmp-20nm.h
>> create mode 100644 drivers/phy/phy-qcom-ufs.c
>> create mode 100644 include/linux/phy/phy-qcom-ufs.h
>
> I think a single header file should be sufficient here.
>
I believe 2 header files is a better design in this case.
one header file is internal, and should serve internally the phy drivers.
the other one that exposes APIs.
One header file compels us to expose our internal macros, and definitions
in include\linux\phy which is not recommended and not necessary.
the advantage of 2 header files will be more visible if you pick the 14nm
phy code as well. (change#3 in V6, or change#4 in V7)
thanks for your comment.
> It would be helpful if you can split this patch further. First add only
> the
> core ufs layer (Include a documentation on how this layer should be used
> in
> Documentation/phy/) and then use it to add the 14nm and 20nm PHYs.
thanks Kishon for your comment.
I accept the first comment, and in V7, the first change is divided into
2 patches as suggested:
one that adds common support for Qualcomm Technologies UFS Phys
and the other one adds the specific implementation of 20nm phy.
at this point i decided not to add a Documentation, as the
Qualcomm UFS PHY uses the generic PHY framework, and currently we don't
think any further documentation is needed in addition to the Generic Phy
documentation that is presented already in Documentation/phy.txt
thanks for reviewing the change.
>
> Thanks
> Kishon
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index ccad880..26a7623 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -277,4 +277,11 @@ config PHY_STIH41X_USB
>>Enable this to support the USB transceiver that is part of
>>STMicroelectronics STiH41x SoC series.
>>
>> +config PHY_QCOM_UFS
>> +tristate "Qualcomm UFS PHY driver"
>> +depends on OF && ARCH_MSM
>> +select GENERIC_PHY
>> +help
>> + Support for UFS PHY on QCOM chipsets.
>> +
>> endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index aa74f96..781b2fa 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -34,3 +34,5 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) +=
>> phy-spear1340-miphy.o
>> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
>> obj-$(CONFIG_PHY_STIH407_USB) += phy-stih407-usb.o
>> obj-$(CONFIG_PHY_STIH41X_USB) += phy-stih41x-usb.o
>> +obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs.o
>> +obj-$(CONFIG_PHY_QCOM_UFS) += phy-qcom-ufs-qmp-20nm.o
>> diff --git a/drivers/phy/phy-qcom-ufs-i.h b/drivers/phy/phy-qcom-ufs-i.h
>> new file mode 100644
>> index 000..a175069
>> --- /dev/null
>> +++ b/drivers/phy/phy-qcom-ufs-i.h
>> @@ -0,0 +1,159 @@
>> +/*
>> + * Copyright (c) 2013-2015, Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef UFS_QCOM_PHY_I_H_
>> +#define UFS_QCOM_PHY_I_H_
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define readl_poll_timeout(addr, val, cond, sleep_us, timeout_us) \
>> +({ \
>> +ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
>> +might_sleep_if(timeout_us); \
>> +for (;;) { \
>> +(val) = readl(addr); \
>> +if (cond) \
>> +break; \
>> +if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
>> +(val) = readl(addr); \
>> +break; \
>> +} \
>> +if (sleep_us) \
>> +usleep_range(DIV_ROUND_UP(sleep_us, 4), sleep_us); \
>> +} \
>> +(cond) ? 0 : -ETIMEDOUT; \
>> +})
>> +
Re: [PATCH v5 1/4] phy: qcom-ufs: add support for 20nm phy
>
> On Jan 7, 2015, at 9:43 AM, Yaniv Gardi wrote:
>
>> This change adds a support for a 20nm qcom-ufs phy that is
>> required in platforms that use ufs-qcom controller.
>>
>> Signed-off-by: Yaniv Gardi
>>
>> ---
>> drivers/phy/Makefile| 2 +
>> drivers/phy/phy-qcom-ufs-i.h| 167
>
> Curious, if the -i in name means anything, or if we could just call this
> phy-qcom-ufs.h
'-i' stands for 'internal'.
phy-qcom-ufs.h name is already in use in include\linux\phy.
We would like to avoid using same file name
>
>> drivers/phy/phy-qcom-ufs-qmp-20nm.c | 257 +
>> drivers/phy/phy-qcom-ufs-qmp-20nm.h | 235
>> drivers/phy/phy-qcom-ufs.c | 745
>>
>> include/linux/phy/phy-qcom-ufs.h| 33 ++
>> 6 files changed, 1439 insertions(+)
>> create mode 100644 drivers/phy/phy-qcom-ufs-i.h
>> create mode 100644 drivers/phy/phy-qcom-ufs-qmp-20nm.c
>> create mode 100644 drivers/phy/phy-qcom-ufs-qmp-20nm.h
>> create mode 100644 drivers/phy/phy-qcom-ufs.c
>> create mode 100644 include/linux/phy/phy-qcom-ufs.h
>>
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index aa74f96..ba94e85 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -34,3 +34,5 @@ obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) +=
>> phy-spear1340-miphy.o
>> obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
>> obj-$(CONFIG_PHY_STIH407_USB)+= phy-stih407-usb.o
>> obj-$(CONFIG_PHY_STIH41X_USB)+= phy-stih41x-usb.o
>> +obj-$(CONFIG_SCSI_UFS_QCOM) += phy-qcom-ufs.o
>> +obj-$(CONFIG_SCSI_UFS_QCOM) += phy-qcom-ufs-qmp-20nm.o
>> diff --git a/drivers/phy/phy-qcom-ufs-i.h b/drivers/phy/phy-qcom-ufs-i.h
>> new file mode 100644
>> index 000..50f1fdc
>> --- /dev/null
>> +++ b/drivers/phy/phy-qcom-ufs-i.h
>> @@ -0,0 +1,167 @@
>> +/*
>> + * Copyright (c) 2013-2015, Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef UFS_QCOM_PHY_I_H_
>> +#define UFS_QCOM_PHY_I_H_
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define readl_poll_timeout(addr, val, cond, sleep_us, timeout_us) \
>> +({ \
>> +ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
>> +might_sleep_if(timeout_us); \
>> +for (;;) { \
>> +(val) = readl(addr); \
>> +if (cond) \
>> +break; \
>> +if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
>> +(val) = readl(addr); \
>> +break; \
>> +} \
>> +if (sleep_us) \
>> +usleep_range(DIV_ROUND_UP(sleep_us, 4), sleep_us); \
>> +} \
>> +(cond) ? 0 : -ETIMEDOUT; \
>> +})
>
> We have a version of this in drivers/phy/phy-qcom-apq8064-sata.c, can we
> pull them out into something like include/asm-generic/io.h
>
it's a different version and the 'signature' is different.
in our code, incoming parameters are:
(addr, val, cond, sleep_us, timeout_us)
in phy-qcom-apq8064-sata.c the parameters are:
(void __iomem *addr, u32 mask)
we'd rather not bring any changes to phy-qcom-apq8064-sata.c,
if it's not trivial (changing incoming parameters and change logic require
testing) as we can not test them.
>> +
>> +#define UFS_QCOM_PHY_CAL_ENTRY(reg, val)\
>> +{ \
>> +.reg_offset = reg, \
>> +.cfg_value = val, \
>> +}
>> +
>> +#define UFS_QCOM_PHY_NAME_LEN 30
>> +
>> +enum {
>> +MASK_SERDES_START = 0x1,
>> +MASK_PCS_READY = 0x1,
>> +};
>> +
>> +enum {
>> +OFFSET_SERDES_START = 0x0,
>> +};
>
> Any reason these arent #defines?
no special reason.
>
>> +
>> +struct ufs_qcom_phy_stored_attributes {
>> +u32 att;
>> +u32 value;
>> +};
>> +
>> +struct ufs_qcom_phy_calibration {
>> +u32 reg_offset;
>> +u32 cfg_value;
>> +};
>> +
>> +struct ufs_qcom_phy_vreg {
>> +const char *name;
>> +struct regulator *reg;
>> +int max_uA;
>> +int min_uV;
>> +int max_uV;
>> +bool enabled;
>> +bool is_always_on;
>> +};
>> +
>> +struct ufs_qcom_phy {
>> +struct list_head list;
>> +struct device *dev;
>> +void __iomem *mmio;
>> +void __iomem *dev_ref_clk_ctrl_mmio;
>> +struct clk *tx_iface_clk;
>> +struct clk *rx_iface_clk;
>> +bool is_iface_clk_enabled;
>> +struct clk *ref_clk_src;
Re: [PATCH v4] scsi: ufs: add support of generic PHY and ICE in Qualcomm chips
thanks for inputs. shortly I will upload a new version, where I address you comments thanks, Yaniv > > On Thursday 04 December 2014 09:24 PM, Christoph Hellwig wrote: >> On Thu, Nov 27, 2014 at 05:59:58PM +0200, Yaniv Gardi wrote: >>> In this change we add support to the generic PHY framework. >>> Two UFS phys are implemented: >>> qmp-20nm and qmp-28nm. >>> >>> Also, the files in this change implement the UFS HW (controller & PHY) >>> specific behavior in Qualcomm chips. >>> Relocation of a few header files is needed in order to expose routines >>> and data structures between PHY driver and UFS driver. >>> >>> Also, this change include the implementation of Inline Crypto Engine >>> (ICE) >>> in Qualcomm chips. >> >> This whole patch is a mess. It does way to many things in one patch, >> and it doesn't explain enough of it. >> >> Please explain why you need it. Especially as the PHY API is a generic >> phy abstraction, so having to share defintions between the provider and >> consumer seems wrong. Even if you need some shared bits keep them to an >> absolute minium insted of moving so much out of the driver directory. >> Also if at all possible keep the shared data in a single header under >> include/linux instead of having lots of global headers in a deep >> directory structure. >> >> Second split this into patches that do a single things, and explain why >> you're doing each: >> >> 1) header move if/as needed >> 2) add 20nm phy driver >> 3) add 28nm phy driver >> 4) add ufs-qcom driver >> 5) add ufs-qcom-ice support >> >> and so on. > > +1 > > -Kishon > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] scsi: ufs-msm: add UFS controller support for Qualcomm MSM chips
>
> On Aug 14, 2014, at 9:22 AM, Yaniv Gardi wrote:
>
>> The files in this change implement the UFS HW (controller & PHY)
>> specific
>> behavior in Qualcomm MSM chips.
>>
>> Signed-off-by: Yaniv Gardi
>> ---
>> Documentation/devicetree/bindings/ufs/ufs-msm.txt | 37 +
>> .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |4 +
>> drivers/scsi/ufs/Kconfig | 12 +
>> drivers/scsi/ufs/Makefile |4 +
>> drivers/scsi/ufs/ufs-msm-phy-qmp-20nm.c| 254 +
>> drivers/scsi/ufs/ufs-msm-phy-qmp-20nm.h| 216
>> drivers/scsi/ufs/ufs-msm-phy-qmp-28nm.c| 368 +++
>> drivers/scsi/ufs/ufs-msm-phy-qmp-28nm.h| 735 +
>> drivers/scsi/ufs/ufs-msm-phy.c | 646
>> drivers/scsi/ufs/ufs-msm-phy.h | 193
>
> Any reason not to put the phy driver in drivers/phy ?
Yes. Phy driver introduces a generic phy framework.
And as a framework it provides with API's, callbacks,
And data structures.
I think the right place to have the >implementation< of the ufs-msm-phy code
Is under drivers/scsi/ufs as it's more related to ufs than it's related to
the framework itself.
>
>> drivers/scsi/ufs/ufs-msm.c | 1105
>>
>> drivers/scsi/ufs/ufs-msm.h | 158 +++
>> 12 files changed, 3732 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/ufs/ufs-msm.txt
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy-qmp-20nm.c
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy-qmp-20nm.h
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy-qmp-28nm.c
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy-qmp-28nm.h
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy.c
>> create mode 100644 drivers/scsi/ufs/ufs-msm-phy.h
>> create mode 100644 drivers/scsi/ufs/ufs-msm.c
>> create mode 100644 drivers/scsi/ufs/ufs-msm.h
>
> Seems like we should spit this into two patches, one for the phy and one
> for the UFS driver itself. Maybe even three, one for the 20nm phy, one
> for the 28nm phy, and one for ufs-msm.c,h.
we could try to split it, but since we didn't split this change into
functional sub-changes, we decided to upload this change as a whole,
as one change without the other wouldn't work anyhow, and they are both
needed for proper functionality.
>
>>
>> diff --git a/Documentation/devicetree/bindings/ufs/ufs-msm.txt
>> b/Documentation/devicetree/bindings/ufs/ufs-msm.txt
>> new file mode 100644
>> index 000..b5caace
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ufs/ufs-msm.txt
>
> This should probably be bindings/phy/qcom-ufs-phy.txt
>
>> @@ -0,0 +1,37 @@
>> +* MSM Universal Flash Storage (UFS) PHY
>> +
>> +UFSPHY nodes are defined to describe on-chip UFS PHY hardware macro.
>> +Each UFS PHY node should have its own node.
>> +
>> +To bind UFS PHY with UFS host controller, the controller node should
>> +contain a phandle reference to UFS PHY node.
>> +
>> +Required properties:
>> +- compatible: compatible list, contains
>> "qcom,ufs-msm-phy-qmp-28nm"
>> + or "qcom,ufs-msm-phy-qmp-20nm" according to the
>> relevant
>> + phy in use
>
> Do we really need -msm in the compat name?
>
>> +- reg :
>> +- #phy-cells : This property shall be set to 0
>> +- vdda-phy-supply : phandle to main PHY supply for analog domain
>> +- vdda-pll-supply : phandle to PHY PLL and Power-Gen block power
>> supply
>> +
>> +Optional properties:
>> +- vdda-phy-max-microamp : specifies max. load that can be drawn from
>> phy supply
>> +- vdda-pll-max-microamp : specifies max. load that can be drawn from
>> pll supply
>> +
>> +Example:
>> +
>> +ufsphy1: ufsphy@0xfc597000 {
>> +compatible = "qcom,ufs-msm-phy-qmp-28nm";
>> +reg = <0xfc597000 0x800>;
>> +#phy-cells = <0>;
>> +vdda-phy-supply = <&pma8084_l4>;
>> +vdda-pll-supply = <&pma8084_l12>;
>> +vdda-phy-max-microamp = <5>;
>> +vdda-pll-max-microamp = <1000>;
>> +};
>> +
>> +ufshc@0xfc598000 {
>> +...
>> +phys = <&ufsphy1>;
>> +};
>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> index e73a619..378585c 100644
>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> @@ -9,6 +9,9 @@ Required properties:
>> - reg :
>>
>> Optional properties:
>> +- phys : phandle to UFS PHY node
>> +- phy-names : the string "ufs_msm_phy" when is found in a node,
>> along
>> + with "phys" attribute, provides phandle to UFS PHY
>> node
>
> seems like the phy-names should be more generic like ufsphy"
>
>> - vcc-supply: phandle to VCC supply regulat
