Re: [PATCH 3/3] add support for DWC UFS Host Controller
On 2/8/2016 4:15 PM, Mark Rutland wrote: > On Mon, Feb 08, 2016 at 03:36:52PM +, Joao Pinto wrote: >> Hi Mark, >> >> On 2/8/2016 3:30 PM, Mark Rutland wrote: >>> On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: Hi Mark and Arnd, Are you saying that a user that puts "snps,ufshcd-1.1" in the DT compatibility string disables the UFS 2.0 in the core driver despite the controller is 2.0? Please clarify. >>> >>> If you can consistently and safely detect that the HW is 2.0, using 2.0 >>> functionality is fine. >>> >>> Regardless, you should have a -1.1 compatible string for the 1.1 HW, and >>> a -2.0 string for the 2.0 HW, so that DTs are explicit about what the >>> hardware is. If 2.0 is intended to be a superset of 1.1, you can have a >>> 1.1 fallback entry for the 2.0 hardware. >>> >> >> Ok, I will include the version in the compatibility strings, but if someone >> mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 >> capable it will activate the 2.0 features independently of what mentioned in >> the >> DT, correct? > > As above, if that can be detected safely and reliably, then I don't see > a problem with that. Ok, thanks for the comments! I am working a bit in PCI next version patch and so I predict to produce a new version for UFS next Wednesday. Joao > > Thanks, > Mark. >
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Mon, Feb 08, 2016 at 03:36:52PM +, Joao Pinto wrote: > Hi Mark, > > On 2/8/2016 3:30 PM, Mark Rutland wrote: > > On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: > >> Hi Mark and Arnd, > >> Are you saying that a user that puts "snps,ufshcd-1.1" > >> in the DT compatibility string disables the UFS 2.0 in the core driver > >> despite > >> the controller is 2.0? Please clarify. > > > > If you can consistently and safely detect that the HW is 2.0, using 2.0 > > functionality is fine. > > > > Regardless, you should have a -1.1 compatible string for the 1.1 HW, and > > a -2.0 string for the 2.0 HW, so that DTs are explicit about what the > > hardware is. If 2.0 is intended to be a superset of 1.1, you can have a > > 1.1 fallback entry for the 2.0 hardware. > > > > Ok, I will include the version in the compatibility strings, but if someone > mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 > capable it will activate the 2.0 features independently of what mentioned in > the > DT, correct? As above, if that can be detected safely and reliably, then I don't see a problem with that. Thanks, Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Mark, On 2/8/2016 3:30 PM, Mark Rutland wrote: > On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: >> Hi Mark and Arnd, >> >> I am planning the v2 of this patch set. I have a doubt in the version >> compatibility strings... The core driver must support the UFS 2.0 controller >> and >> this patch set includes a patch that adds 2.0 capabilities to it. > > Ok. It wasn't clear to me that this series added support for features > specific to 2.0. Yes, the patch set contains a patch to add 2.0 to the UFS core driver. The cover letter: https://lkml.org/lkml/2016/2/3/331 The Patch: https://lkml.org/lkml/2016/2/3/330 > >> The core driver can get from the controller's version and with that >> use or not a specific 2.0 feature. > > It can be detected from the hardware? Yes, the hardware has a register that contains the version, and so if a driver has workarounds then it can adapt. > >> What would be the real added-value of having a compatibility string like >> "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if >> it >> detects a 2.0 controller? > > Generally having specify strings ensure that it's possible to handle > things in future (e.g. errata workarounds), or if we realise something > isn't as clear-cut as we thought it was (i.e. 2.0 not being a strict > superset of 1.1). > > It's difficult to predict when you need that, so we err on the side of > requiring it. At worst it means you have a small redundant few > characters in a DT, but that's a much better proposition than having too > little information. > >> Are you saying that a user that puts "snps,ufshcd-1.1" >> in the DT compatibility string disables the UFS 2.0 in the core driver >> despite >> the controller is 2.0? Please clarify. > > If you can consistently and safely detect that the HW is 2.0, using 2.0 > functionality is fine. > > Regardless, you should have a -1.1 compatible string for the 1.1 HW, and > a -2.0 string for the 2.0 HW, so that DTs are explicit about what the > hardware is. If 2.0 is intended to be a superset of 1.1, you can have a > 1.1 fallback entry for the 2.0 hardware. > Ok, I will include the version in the compatibility strings, but if someone mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 capable it will activate the 2.0 features independently of what mentioned in the DT, correct? > Thanks, > Mark. > Thanks, Joao
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: > Hi Mark and Arnd, > > I am planning the v2 of this patch set. I have a doubt in the version > compatibility strings... The core driver must support the UFS 2.0 controller > and > this patch set includes a patch that adds 2.0 capabilities to it. Ok. It wasn't clear to me that this series added support for features specific to 2.0. > The core driver can get from the controller's version and with that > use or not a specific 2.0 feature. It can be detected from the hardware? > What would be the real added-value of having a compatibility string like > "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if it > detects a 2.0 controller? Generally having specify strings ensure that it's possible to handle things in future (e.g. errata workarounds), or if we realise something isn't as clear-cut as we thought it was (i.e. 2.0 not being a strict superset of 1.1). It's difficult to predict when you need that, so we err on the side of requiring it. At worst it means you have a small redundant few characters in a DT, but that's a much better proposition than having too little information. > Are you saying that a user that puts "snps,ufshcd-1.1" > in the DT compatibility string disables the UFS 2.0 in the core driver despite > the controller is 2.0? Please clarify. If you can consistently and safely detect that the HW is 2.0, using 2.0 functionality is fine. Regardless, you should have a -1.1 compatible string for the 1.1 HW, and a -2.0 string for the 2.0 HW, so that DTs are explicit about what the hardware is. If 2.0 is intended to be a superset of 1.1, you can have a 1.1 fallback entry for the 2.0 hardware. Thanks, Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Mark and Arnd, I am planning the v2 of this patch set. I have a doubt in the version compatibility strings... The core driver must support the UFS 2.0 controller and this patch set includes a patch that adds 2.0 capabilities to it. The core driver can get from the controller's version and with that use or not a specific 2.0 feature. What would be the real added-value of having a compatibility string like "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if it detects a 2.0 controller? Are you saying that a user that puts "snps,ufshcd-1.1" in the DT compatibility string disables the UFS 2.0 in the core driver despite the controller is 2.0? Please clarify. Thanks, Joao On 2/4/2016 4:27 PM, Mark Rutland wrote: > On Wed, Feb 03, 2016 at 03:54:48PM +, Joao Pinto wrote: >> Hi, >> >> On 2/3/2016 3:39 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: Hi Arnd, On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: >> >> Signed-off-by: Joao Pinto > > This needs a changelog comment, like every patch. > >> @@ -0,0 +1,16 @@ >> +* Universal Flash Storage (UFS) DesignWare Host Controller >> + >> +DWC_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 "snps,ufshcd" > > Are there multiple versions of this controller? Usually for designware > parts the version is known, so we should document which versions exist This controller recent releases was 2.0, but we released last year 1.1. The driver works with both. The driver must work with all DWC UFS versions. >>> >>> Ok, then make the driver match on the "snps,ufshcd-1.1" compatible >>> string, but document both strings in the binding document, and make >>> it mandatory to specify the 1.1 version as a compatible fallback. >>> >>> If we ever need to handle a quirk for the 2.0 version then, it can >>> easily be done. >> >> We need the driver to support UFS 2.0 because it is our latest release and is >> the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" >> then. >> What do you think? > > Arnd's point was that the driver can handle only "snps,ufshcd-1.1" for > now, and in your DT you can have: > > compatible = "snps,ufshcd-2.0", "snps,ufshcd-1.1"; > > That allows driver to handle 2.0 and 1.1 without knowing anything about > 2.0 for now. If in future the two need to be handled differently we can > update the driver to explicitly match "snps,ufshcd-2.0". > > Regardless, both compatible string should go in the documentation, and > it should be explicitly mentioned that "snps,ufshcd-1.1" should be used > as a fallback entry. > > Mark. >
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On 2/8/2016 4:15 PM, Mark Rutland wrote: > On Mon, Feb 08, 2016 at 03:36:52PM +, Joao Pinto wrote: >> Hi Mark, >> >> On 2/8/2016 3:30 PM, Mark Rutland wrote: >>> On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: Hi Mark and Arnd, Are you saying that a user that puts "snps,ufshcd-1.1" in the DT compatibility string disables the UFS 2.0 in the core driver despite the controller is 2.0? Please clarify. >>> >>> If you can consistently and safely detect that the HW is 2.0, using 2.0 >>> functionality is fine. >>> >>> Regardless, you should have a -1.1 compatible string for the 1.1 HW, and >>> a -2.0 string for the 2.0 HW, so that DTs are explicit about what the >>> hardware is. If 2.0 is intended to be a superset of 1.1, you can have a >>> 1.1 fallback entry for the 2.0 hardware. >>> >> >> Ok, I will include the version in the compatibility strings, but if someone >> mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 >> capable it will activate the 2.0 features independently of what mentioned in >> the >> DT, correct? > > As above, if that can be detected safely and reliably, then I don't see > a problem with that. Ok, thanks for the comments! I am working a bit in PCI next version patch and so I predict to produce a new version for UFS next Wednesday. Joao > > Thanks, > Mark. >
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Mon, Feb 08, 2016 at 03:36:52PM +, Joao Pinto wrote: > Hi Mark, > > On 2/8/2016 3:30 PM, Mark Rutland wrote: > > On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: > >> Hi Mark and Arnd, > >> Are you saying that a user that puts "snps,ufshcd-1.1" > >> in the DT compatibility string disables the UFS 2.0 in the core driver > >> despite > >> the controller is 2.0? Please clarify. > > > > If you can consistently and safely detect that the HW is 2.0, using 2.0 > > functionality is fine. > > > > Regardless, you should have a -1.1 compatible string for the 1.1 HW, and > > a -2.0 string for the 2.0 HW, so that DTs are explicit about what the > > hardware is. If 2.0 is intended to be a superset of 1.1, you can have a > > 1.1 fallback entry for the 2.0 hardware. > > > > Ok, I will include the version in the compatibility strings, but if someone > mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 > capable it will activate the 2.0 features independently of what mentioned in > the > DT, correct? As above, if that can be detected safely and reliably, then I don't see a problem with that. Thanks, Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Mark and Arnd, I am planning the v2 of this patch set. I have a doubt in the version compatibility strings... The core driver must support the UFS 2.0 controller and this patch set includes a patch that adds 2.0 capabilities to it. The core driver can get from the controller's version and with that use or not a specific 2.0 feature. What would be the real added-value of having a compatibility string like "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if it detects a 2.0 controller? Are you saying that a user that puts "snps,ufshcd-1.1" in the DT compatibility string disables the UFS 2.0 in the core driver despite the controller is 2.0? Please clarify. Thanks, Joao On 2/4/2016 4:27 PM, Mark Rutland wrote: > On Wed, Feb 03, 2016 at 03:54:48PM +, Joao Pinto wrote: >> Hi, >> >> On 2/3/2016 3:39 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: Hi Arnd, On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: >> >> Signed-off-by: Joao Pinto> > This needs a changelog comment, like every patch. > >> @@ -0,0 +1,16 @@ >> +* Universal Flash Storage (UFS) DesignWare Host Controller >> + >> +DWC_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 "snps,ufshcd" > > Are there multiple versions of this controller? Usually for designware > parts the version is known, so we should document which versions exist This controller recent releases was 2.0, but we released last year 1.1. The driver works with both. The driver must work with all DWC UFS versions. >>> >>> Ok, then make the driver match on the "snps,ufshcd-1.1" compatible >>> string, but document both strings in the binding document, and make >>> it mandatory to specify the 1.1 version as a compatible fallback. >>> >>> If we ever need to handle a quirk for the 2.0 version then, it can >>> easily be done. >> >> We need the driver to support UFS 2.0 because it is our latest release and is >> the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" >> then. >> What do you think? > > Arnd's point was that the driver can handle only "snps,ufshcd-1.1" for > now, and in your DT you can have: > > compatible = "snps,ufshcd-2.0", "snps,ufshcd-1.1"; > > That allows driver to handle 2.0 and 1.1 without knowing anything about > 2.0 for now. If in future the two need to be handled differently we can > update the driver to explicitly match "snps,ufshcd-2.0". > > Regardless, both compatible string should go in the documentation, and > it should be explicitly mentioned that "snps,ufshcd-1.1" should be used > as a fallback entry. > > Mark. >
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: > Hi Mark and Arnd, > > I am planning the v2 of this patch set. I have a doubt in the version > compatibility strings... The core driver must support the UFS 2.0 controller > and > this patch set includes a patch that adds 2.0 capabilities to it. Ok. It wasn't clear to me that this series added support for features specific to 2.0. > The core driver can get from the controller's version and with that > use or not a specific 2.0 feature. It can be detected from the hardware? > What would be the real added-value of having a compatibility string like > "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if it > detects a 2.0 controller? Generally having specify strings ensure that it's possible to handle things in future (e.g. errata workarounds), or if we realise something isn't as clear-cut as we thought it was (i.e. 2.0 not being a strict superset of 1.1). It's difficult to predict when you need that, so we err on the side of requiring it. At worst it means you have a small redundant few characters in a DT, but that's a much better proposition than having too little information. > Are you saying that a user that puts "snps,ufshcd-1.1" > in the DT compatibility string disables the UFS 2.0 in the core driver despite > the controller is 2.0? Please clarify. If you can consistently and safely detect that the HW is 2.0, using 2.0 functionality is fine. Regardless, you should have a -1.1 compatible string for the 1.1 HW, and a -2.0 string for the 2.0 HW, so that DTs are explicit about what the hardware is. If 2.0 is intended to be a superset of 1.1, you can have a 1.1 fallback entry for the 2.0 hardware. Thanks, Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Mark, On 2/8/2016 3:30 PM, Mark Rutland wrote: > On Mon, Feb 08, 2016 at 03:17:11PM +, Joao Pinto wrote: >> Hi Mark and Arnd, >> >> I am planning the v2 of this patch set. I have a doubt in the version >> compatibility strings... The core driver must support the UFS 2.0 controller >> and >> this patch set includes a patch that adds 2.0 capabilities to it. > > Ok. It wasn't clear to me that this series added support for features > specific to 2.0. Yes, the patch set contains a patch to add 2.0 to the UFS core driver. The cover letter: https://lkml.org/lkml/2016/2/3/331 The Patch: https://lkml.org/lkml/2016/2/3/330 > >> The core driver can get from the controller's version and with that >> use or not a specific 2.0 feature. > > It can be detected from the hardware? Yes, the hardware has a register that contains the version, and so if a driver has workarounds then it can adapt. > >> What would be the real added-value of having a compatibility string like >> "snps,ufshcd-1.1" and "snps,ufshcd-2.0" if the driver can perform as 2.0 if >> it >> detects a 2.0 controller? > > Generally having specify strings ensure that it's possible to handle > things in future (e.g. errata workarounds), or if we realise something > isn't as clear-cut as we thought it was (i.e. 2.0 not being a strict > superset of 1.1). > > It's difficult to predict when you need that, so we err on the side of > requiring it. At worst it means you have a small redundant few > characters in a DT, but that's a much better proposition than having too > little information. > >> Are you saying that a user that puts "snps,ufshcd-1.1" >> in the DT compatibility string disables the UFS 2.0 in the core driver >> despite >> the controller is 2.0? Please clarify. > > If you can consistently and safely detect that the HW is 2.0, using 2.0 > functionality is fine. > > Regardless, you should have a -1.1 compatible string for the 1.1 HW, and > a -2.0 string for the 2.0 HW, so that DTs are explicit about what the > hardware is. If 2.0 is intended to be a superset of 1.1, you can have a > 1.1 fallback entry for the 2.0 hardware. > Ok, I will include the version in the compatibility strings, but if someone mentions "snps,ufshcd-1.1" only and the driver detects that the HW is 2.0 capable it will activate the 2.0 features independently of what mentioned in the DT, correct? > Thanks, > Mark. > Thanks, Joao
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wed, Feb 03, 2016 at 03:54:48PM +, Joao Pinto wrote: > Hi, > > On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: > >> > >> Hi Arnd, > >> > >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > > Signed-off-by: Joao Pinto > >>> > >>> This needs a changelog comment, like every patch. > >>> > @@ -0,0 +1,16 @@ > +* Universal Flash Storage (UFS) DesignWare Host Controller > + > +DWC_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 "snps,ufshcd" > >>> > >>> Are there multiple versions of this controller? Usually for designware > >>> parts the version is known, so we should document which versions exist > >> > >> This controller recent releases was 2.0, but we released last year 1.1. The > >> driver works with both. The driver must work with all DWC UFS versions. > > > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > > string, but document both strings in the binding document, and make > > it mandatory to specify the 1.1 version as a compatible fallback. > > > > If we ever need to handle a quirk for the 2.0 version then, it can > > easily be done. > > We need the driver to support UFS 2.0 because it is our latest release and is > the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" > then. > What do you think? Arnd's point was that the driver can handle only "snps,ufshcd-1.1" for now, and in your DT you can have: compatible = "snps,ufshcd-2.0", "snps,ufshcd-1.1"; That allows driver to handle 2.0 and 1.1 without knowing anything about 2.0 for now. If in future the two need to be handled differently we can update the driver to explicitly match "snps,ufshcd-2.0". Regardless, both compatible string should go in the documentation, and it should be explicitly mentioned that "snps,ufshcd-1.1" should be used as a fallback entry. Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wed, Feb 03, 2016 at 03:54:48PM +, Joao Pinto wrote: > Hi, > > On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: > >> > >> Hi Arnd, > >> > >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > > Signed-off-by: Joao Pinto> >>> > >>> This needs a changelog comment, like every patch. > >>> > @@ -0,0 +1,16 @@ > +* Universal Flash Storage (UFS) DesignWare Host Controller > + > +DWC_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 "snps,ufshcd" > >>> > >>> Are there multiple versions of this controller? Usually for designware > >>> parts the version is known, so we should document which versions exist > >> > >> This controller recent releases was 2.0, but we released last year 1.1. The > >> driver works with both. The driver must work with all DWC UFS versions. > > > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > > string, but document both strings in the binding document, and make > > it mandatory to specify the 1.1 version as a compatible fallback. > > > > If we ever need to handle a quirk for the 2.0 version then, it can > > easily be done. > > We need the driver to support UFS 2.0 because it is our latest release and is > the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" > then. > What do you think? Arnd's point was that the driver can handle only "snps,ufshcd-1.1" for now, and in your DT you can have: compatible = "snps,ufshcd-2.0", "snps,ufshcd-1.1"; That allows driver to handle 2.0 and 1.1 without knowing anything about 2.0 for now. If in future the two need to be handled differently we can update the driver to explicitly match "snps,ufshcd-2.0". Regardless, both compatible string should go in the documentation, and it should be explicitly mentioned that "snps,ufshcd-1.1" should be used as a fallback entry. Mark.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
++ Vinayak Holikatti Hi Vinayak, Could I please get your opinion about the patch set? thanks. On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: >> >> Hi Arnd, >> >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: Signed-off-by: Joao Pinto >>> >>> This needs a changelog comment, like every patch. >>> @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" >>> >>> Are there multiple versions of this controller? Usually for designware >>> parts the version is known, so we should document which versions exist >> >> This controller recent releases was 2.0, but we released last year 1.1. The >> driver works with both. The driver must work with all DWC UFS versions. > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > string, but document both strings in the binding document, and make > it mandatory to specify the 1.1 version as a compatible fallback. > > If we ever need to handle a quirk for the 2.0 version then, it can > easily be done. > +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- +This selects the DesignWare hooks for the UFS host controller. + +Select this if you have a DesignWare UFS controller. +If unsure, say N. >>> >>> This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT >> >> We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys >> hooks would be selected also which in my opinion is not very accurate. >> In my opinion we should have a selectable DWC_HOOKS. > > I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS > even if nothing uses it and you only have SCSI_UFS_QCOM enabled. > > With my suggestion, the hooks would disappear unless they are > actually used. > > Then again, with my later comments, we no longer need the hooks. > > +/** + * ufshcd_dwc_setup_mphy() + * This function configures Local (host) Synopsys MPHY specific attributes + * + * @hba: Pointer to drivers structure + * + * Returns 0 on success non-zero value on failure + */ +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) +{ + int ret = 0; + +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); + ret = ufshcd_dwc_setup_40bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "40-bit RMMI configuration failed"); + goto out; + } +#else +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); + ret = ufshcd_dwc_setup_20bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "20-bit RMMI configuration failed"); + goto out; + } +#endif +#endif + /* To write Shadow register bank to effective configuration block */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); + if (ret) + goto out; + + /* To configure Debug OMC */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); + +out: + return ret; +} >>> >>> Try to use the generic PHY abstraction here and remove all the #ifdef etc. >> >> Could you please point an example for me to check? > > drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through > the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. > > This should probably be moved into the generic UFS platform driver so the PHY > handling can be shared between all backends. > }; >>> >>> I think you're better off with a separate PCI driver for this. Remove >>> all the #ifdef mess here, put whatever is dwc specific into a new file, >>> and perhaps move the common parts into a shared file that can be used >>> by both the samsung and designware drivers. >> >> I have a branch with that approach, but honestly it would be a big change in >> the >> UFS arch for the pci and I decided to make it simple. I sent that suggestion >> for >> the scsi mailing list and the comments showed me that. Does anyone have >> anything >> against putting ufshcd-pci.c as a pci common code and then have a >> ufs-dwc-pci.c >> and a ufs-samsung-pci.c that uses that common code? > > Another approach would be to just rename the existing file to > ufs-samsung-pci.c > and start the ufs-dwc-pci.c as a copy of that. The file is not really all > that > large anyway. > > Arnd >
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi, On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: >> >> Hi Arnd, >> >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: Signed-off-by: Joao Pinto >>> >>> This needs a changelog comment, like every patch. >>> @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" >>> >>> Are there multiple versions of this controller? Usually for designware >>> parts the version is known, so we should document which versions exist >> >> This controller recent releases was 2.0, but we released last year 1.1. The >> driver works with both. The driver must work with all DWC UFS versions. > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > string, but document both strings in the binding document, and make > it mandatory to specify the 1.1 version as a compatible fallback. > > If we ever need to handle a quirk for the 2.0 version then, it can > easily be done. We need the driver to support UFS 2.0 because it is our latest release and is the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" then. What do you think? > +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- +This selects the DesignWare hooks for the UFS host controller. + +Select this if you have a DesignWare UFS controller. +If unsure, say N. >>> >>> This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT >> >> We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys >> hooks would be selected also which in my opinion is not very accurate. >> In my opinion we should have a selectable DWC_HOOKS. > > I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS > even if nothing uses it and you only have SCSI_UFS_QCOM enabled. SCSI_UFS_QCOM is select if you are using a QCOM and PLAT is enabled. We can do it the following way: If DWC Platform or DWC PCI are selected than automatically select the HOOKS. What do you think? > > With my suggestion, the hooks would disappear unless they are > actually used. > > Then again, with my later comments, we no longer need the hooks. > > +/** + * ufshcd_dwc_setup_mphy() + * This function configures Local (host) Synopsys MPHY specific attributes + * + * @hba: Pointer to drivers structure + * + * Returns 0 on success non-zero value on failure + */ +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) +{ + int ret = 0; + +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); + ret = ufshcd_dwc_setup_40bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "40-bit RMMI configuration failed"); + goto out; + } +#else +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); + ret = ufshcd_dwc_setup_20bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "20-bit RMMI configuration failed"); + goto out; + } +#endif +#endif + /* To write Shadow register bank to effective configuration block */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); + if (ret) + goto out; + + /* To configure Debug OMC */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); + +out: + return ret; +} >>> >>> Try to use the generic PHY abstraction here and remove all the #ifdef etc. >> >> Could you please point an example for me to check? > > drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through > the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. > > This should probably be moved into the generic UFS platform driver so the PHY > handling can be shared between all backends. > I will check it out. }; >>> >>> I think you're better off with a separate PCI driver for this. Remove >>> all the #ifdef mess here, put whatever is dwc specific into a new file, >>> and perhaps move the common parts into a shared file that can be used >>> by both the samsung and designware drivers. >> >> I have a branch with that approach, but honestly it would be a big change in >> the >> UFS arch for the pci and I decided to make it simple. I sent that suggestion >> for >> the scsi mailing list and the comments showed me that. Does anyone have >> anything >> against putting ufshcd-pci.c as a pci common code and then have a >> ufs-dwc-pci.c >> and a ufs-samsung-pci.c that uses that common code? > > Another
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: > > Hi Arnd, > > On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > >> > >> Signed-off-by: Joao Pinto > > > > This needs a changelog comment, like every patch. > > > >> @@ -0,0 +1,16 @@ > >> +* Universal Flash Storage (UFS) DesignWare Host Controller > >> + > >> +DWC_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 "snps,ufshcd" > > > > Are there multiple versions of this controller? Usually for designware > > parts the version is known, so we should document which versions exist > > This controller recent releases was 2.0, but we released last year 1.1. The > driver works with both. The driver must work with all DWC UFS versions. Ok, then make the driver match on the "snps,ufshcd-1.1" compatible string, but document both strings in the binding document, and make it mandatory to specify the 1.1 version as a compatible fallback. If we ever need to handle a quirk for the 2.0 version then, it can easily be done. > >> +config SCSI_UFS_DWC_HOOKS > >> + bool "DesignWare hooks to UFS controller" > >> + depends on SCSI_UFSHCD > >> + ---help--- > >> +This selects the DesignWare hooks for the UFS host controller. > >> + > >> +Select this if you have a DesignWare UFS controller. > >> +If unsure, say N. > > > > This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT > > We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys > hooks would be selected also which in my opinion is not very accurate. > In my opinion we should have a selectable DWC_HOOKS. I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS even if nothing uses it and you only have SCSI_UFS_QCOM enabled. With my suggestion, the hooks would disappear unless they are actually used. Then again, with my later comments, we no longer need the hooks. > >> +/** > >> + * ufshcd_dwc_setup_mphy() > >> + * This function configures Local (host) Synopsys MPHY specific attributes > >> + * > >> + * @hba: Pointer to drivers structure > >> + * > >> + * Returns 0 on success non-zero value on failure > >> + */ > >> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) > >> +{ > >> + int ret = 0; > >> + > >> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); > >> + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "40-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#else > >> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); > >> + ret = ufshcd_dwc_setup_20bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "20-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#endif > >> +#endif > >> + /* To write Shadow register bank to effective configuration block */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); > >> + if (ret) > >> + goto out; > >> + > >> + /* To configure Debug OMC */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); > >> + > >> +out: > >> + return ret; > >> +} > > > > Try to use the generic PHY abstraction here and remove all the #ifdef etc. > > Could you please point an example for me to check? drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. This should probably be moved into the generic UFS platform driver so the PHY handling can be shared between all backends. > >> }; > > > > I think you're better off with a separate PCI driver for this. Remove > > all the #ifdef mess here, put whatever is dwc specific into a new file, > > and perhaps move the common parts into a shared file that can be used > > by both the samsung and designware drivers. > > I have a branch with that approach, but honestly it would be a big change in > the > UFS arch for the pci and I decided to make it simple. I sent that suggestion > for > the scsi mailing list and the comments showed me that. Does anyone have > anything > against putting ufshcd-pci.c as a pci common code and then have a > ufs-dwc-pci.c > and a ufs-samsung-pci.c that uses that common code? Another approach would be to just rename the existing file to ufs-samsung-pci.c and start the ufs-dwc-pci.c as a copy of that. The file is not really all that large anyway. Arnd
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Arnd, On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: >> >> Signed-off-by: Joao Pinto > > This needs a changelog comment, like every patch. > >> @@ -0,0 +1,16 @@ >> +* Universal Flash Storage (UFS) DesignWare Host Controller >> + >> +DWC_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 "snps,ufshcd" > > Are there multiple versions of this controller? Usually for designware > parts the version is known, so we should document which versions exist This controller recent releases was 2.0, but we released last year 1.1. The driver works with both. The driver must work with all DWC UFS versions. > >> + >> +config SCSI_UFS_DWC_HOOKS >> +bool "DesignWare hooks to UFS controller" >> +depends on SCSI_UFSHCD >> +---help--- >> + This selects the DesignWare hooks for the UFS host controller. >> + >> + Select this if you have a DesignWare UFS controller. >> + If unsure, say N. > > This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys hooks would be selected also which in my opinion is not very accurate. In my opinion we should have a selectable DWC_HOOKS. > >> +config SCSI_UFS_DWC_MPHY_TC >> +bool "Support for the Synopsys MPHY Test Chip" >> +depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) >> +---help--- >> + This selects the support for the Synopsys MPHY Test Chip. >> + >> + Select this if you have a Synopsys MPHY Test Chip. >> + If unsure, say N. >> + >> +config SCSI_UFS_DWC_20BIT_RMMI >> +bool "20-bit RMMI MPHY" >> +depends on SCSI_UFS_DWC_MPHY_TC >> +---help--- >> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. >> + >> + Select this if you are using a 40-bit RMMI Synopsys MPHY. >> + If unsure, say N. >> + >> +config SCSI_UFS_DWC_40BIT_RMMI >> +bool "40-bit RMMI MPHY" >> +depends on SCSI_UFS_DWC_MPHY_TC >> +---help--- >> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. >> + >> + Select this if you are using a 40-bit RMMI Synopsys MPHY. >> + If unsure, say N. > > Move the PHY config options to drivers/phy/ along with the respective > drivers, the device using the PHY should not need to be aware which > one is being used. This MPHY config is just to enable parts of the host config which consists of specific unipro commands and nothing else. The unipro command execution uses functions that are in the ufshcd.c. But if everyone agree we can do as you suggest. > >> +/** >> + * ufshcd_dwc_setup_mphy() >> + * This function configures Local (host) Synopsys MPHY specific attributes >> + * >> + * @hba: Pointer to drivers structure >> + * >> + * Returns 0 on success non-zero value on failure >> + */ >> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) >> +{ >> +int ret = 0; >> + >> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI >> +dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); >> +ret = ufshcd_dwc_setup_40bit_rmmi(hba); >> +if (ret) { >> +dev_err(hba->dev, "40-bit RMMI configuration failed"); >> +goto out; >> +} >> +#else >> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI >> +dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); >> +ret = ufshcd_dwc_setup_20bit_rmmi(hba); >> +if (ret) { >> +dev_err(hba->dev, "20-bit RMMI configuration failed"); >> +goto out; >> +} >> +#endif >> +#endif >> +/* To write Shadow register bank to effective configuration block */ >> +ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); >> +if (ret) >> +goto out; >> + >> +/* To configure Debug OMC */ >> +ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); >> + >> +out: >> +return ret; >> +} > > Try to use the generic PHY abstraction here and remove all the #ifdef etc. Could you please point an example for me to check? > >> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c >> index d15eaa4..66518a1 100644 >> --- a/drivers/scsi/ufs/ufshcd-pci.c >> +++ b/drivers/scsi/ufs/ufshcd-pci.c >> @@ -34,6 +34,9 @@ >> */ >> >> #include "ufshcd.h" >> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS >> +#include "ufshcd-dwc.h" >> +#endif >> #include >> #include >> >> @@ -83,6 +86,16 @@ static int ufshcd_pci_runtime_idle(struct device *dev) >> #define ufshcd_pci_runtime_idle NULL >> #endif /* CONFIG_PM */ >> >> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS >> +/** >> + * struct ufs_hba_dwc_vops - UFS DWC specific variant operations >> + */ >> +static struct ufs_hba_variant_ops ufs_dwc_pci_hba_vops = { >> +.name = "dwc-pci", >> +.custom_probe_hba = ufshcd_dwc_configuration, >> +}; >> +#endif >> + >>
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > > Signed-off-by: Joao Pinto This needs a changelog comment, like every patch. > @@ -0,0 +1,16 @@ > +* Universal Flash Storage (UFS) DesignWare Host Controller > + > +DWC_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 "snps,ufshcd" Are there multiple versions of this controller? Usually for designware parts the version is known, so we should document which versions exist > + > +config SCSI_UFS_DWC_HOOKS > + bool "DesignWare hooks to UFS controller" > + depends on SCSI_UFSHCD > + ---help--- > + This selects the DesignWare hooks for the UFS host controller. > + > + Select this if you have a DesignWare UFS controller. > + If unsure, say N. This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT > +config SCSI_UFS_DWC_MPHY_TC > + bool "Support for the Synopsys MPHY Test Chip" > + depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) > + ---help--- > + This selects the support for the Synopsys MPHY Test Chip. > + > + Select this if you have a Synopsys MPHY Test Chip. > + If unsure, say N. > + > +config SCSI_UFS_DWC_20BIT_RMMI > + bool "20-bit RMMI MPHY" > + depends on SCSI_UFS_DWC_MPHY_TC > + ---help--- > + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. > + > + Select this if you are using a 40-bit RMMI Synopsys MPHY. > + If unsure, say N. > + > +config SCSI_UFS_DWC_40BIT_RMMI > + bool "40-bit RMMI MPHY" > + depends on SCSI_UFS_DWC_MPHY_TC > + ---help--- > + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. > + > + Select this if you are using a 40-bit RMMI Synopsys MPHY. > + If unsure, say N. Move the PHY config options to drivers/phy/ along with the respective drivers, the device using the PHY should not need to be aware which one is being used. > +/** > + * ufshcd_dwc_setup_mphy() > + * This function configures Local (host) Synopsys MPHY specific attributes > + * > + * @hba: Pointer to drivers structure > + * > + * Returns 0 on success non-zero value on failure > + */ > +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) > +{ > + int ret = 0; > + > +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); > + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > + if (ret) { > + dev_err(hba->dev, "40-bit RMMI configuration failed"); > + goto out; > + } > +#else > +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI > + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); > + ret = ufshcd_dwc_setup_20bit_rmmi(hba); > + if (ret) { > + dev_err(hba->dev, "20-bit RMMI configuration failed"); > + goto out; > + } > +#endif > +#endif > + /* To write Shadow register bank to effective configuration block */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); > + if (ret) > + goto out; > + > + /* To configure Debug OMC */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); > + > +out: > + return ret; > +} Try to use the generic PHY abstraction here and remove all the #ifdef etc. > diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c > index d15eaa4..66518a1 100644 > --- a/drivers/scsi/ufs/ufshcd-pci.c > +++ b/drivers/scsi/ufs/ufshcd-pci.c > @@ -34,6 +34,9 @@ > */ > > #include "ufshcd.h" > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > +#include "ufshcd-dwc.h" > +#endif > #include > #include > > @@ -83,6 +86,16 @@ static int ufshcd_pci_runtime_idle(struct device *dev) > #define ufshcd_pci_runtime_idle NULL > #endif /* CONFIG_PM */ > > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > +/** > + * struct ufs_hba_dwc_vops - UFS DWC specific variant operations > + */ > +static struct ufs_hba_variant_ops ufs_dwc_pci_hba_vops = { > + .name = "dwc-pci", > + .custom_probe_hba = ufshcd_dwc_configuration, > +}; > +#endif > + > /** > * ufshcd_pci_shutdown - main function to put the controller in reset state > * @pdev: pointer to PCI device handle > @@ -144,6 +157,9 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *id) > > INIT_LIST_HEAD(>clk_list_head); > > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > + hba->vops = _dwc_pci_hba_vops; > +#endif > err = ufshcd_init(hba, mmio_base, pdev->irq); > if (err) { > dev_err(>dev, "Initialization failed\n"); > @@ -167,6 +183,10 @@ static const struct dev_pm_ops ufshcd_pci_pm_ops = { > > static const struct pci_device_id ufshcd_pci_tbl[] = { > { PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 }, > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > + { PCI_VENDOR_ID_SYNOPSYS, 0xB101, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
[PATCH 3/3] add support for DWC UFS Host Controller
Signed-off-by: Joao Pinto --- Documentation/devicetree/bindings/ufs/ufs-dwc.txt | 16 + MAINTAINERS | 6 + drivers/scsi/ufs/Kconfig | 45 ++ drivers/scsi/ufs/Makefile | 2 + drivers/scsi/ufs/ufs-dwc.c| 96 +++ drivers/scsi/ufs/ufshcd-dwc.c | 768 ++ drivers/scsi/ufs/ufshcd-dwc.h | 26 + drivers/scsi/ufs/ufshcd-pci.c | 20 + drivers/scsi/ufs/ufshcd.c | 32 +- drivers/scsi/ufs/ufshcd.h | 22 + drivers/scsi/ufs/ufshci-dwc.h | 42 ++ drivers/scsi/ufs/unipro.h | 39 ++ 12 files changed, 1100 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/ufs/ufs-dwc.txt create mode 100644 drivers/scsi/ufs/ufs-dwc.c create mode 100644 drivers/scsi/ufs/ufshcd-dwc.c create mode 100644 drivers/scsi/ufs/ufshcd-dwc.h create mode 100644 drivers/scsi/ufs/ufshci-dwc.h diff --git a/Documentation/devicetree/bindings/ufs/ufs-dwc.txt b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt new file mode 100644 index 000..fa361f2 --- /dev/null +++ b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" +- reg : +- interrupts: + +Example: + dwc_ufshcd@0xD000 { + compatible = "snps,ufshcd"; + reg = < 0xD000 0x1 >; + interrupts = < 24 >; + }; diff --git a/MAINTAINERS b/MAINTAINERS index d2f94e2..b5d2fdb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11006,6 +11006,12 @@ S: Supported F: Documentation/scsi/ufs.txt F: drivers/scsi/ufs/ +UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER DWC HOOKS +M: Joao Pinto +L: linux-s...@vger.kernel.org +S: Supported +F: drivers/scsi/ufs/*-dwc-* + UNSORTED BLOCK IMAGES (UBI) M: Artem Bityutskiy M: Richard Weinberger diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index 5f45307..86be4c8 100644 --- a/drivers/scsi/ufs/Kconfig +++ b/drivers/scsi/ufs/Kconfig @@ -83,3 +83,48 @@ config SCSI_UFS_QCOM Select this if you have UFS controller on QCOM chipset. If unsure, say N. + +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- + This selects the DesignWare hooks for the UFS host controller. + + Select this if you have a DesignWare UFS controller. + If unsure, say N. + +config SCSI_UFS_DWC_PLAT + tristate "DesignWare UFS controller platform glue driver" + depends on SCSI_UFS_DWC_HOOKS && SCSI_UFSHCD_PLATFORM + ---help--- + This selects the DesignWare UFS host controller platform glue driver. + + Select this if you have a DesignWare UFS controller on Platform bus. + If unsure, say N. + +config SCSI_UFS_DWC_MPHY_TC + bool "Support for the Synopsys MPHY Test Chip" + depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) + ---help--- + This selects the support for the Synopsys MPHY Test Chip. + + Select this if you have a Synopsys MPHY Test Chip. + If unsure, say N. + +config SCSI_UFS_DWC_20BIT_RMMI + bool "20-bit RMMI MPHY" + depends on SCSI_UFS_DWC_MPHY_TC + ---help--- + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. + + Select this if you are using a 40-bit RMMI Synopsys MPHY. + If unsure, say N. + +config SCSI_UFS_DWC_40BIT_RMMI + bool "40-bit RMMI MPHY" + depends on SCSI_UFS_DWC_MPHY_TC + ---help--- + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. + + Select this if you are using a 40-bit RMMI Synopsys MPHY. + If unsure, say N. diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile index 8303bcc..5be2363 100644 --- a/drivers/scsi/ufs/Makefile +++ b/drivers/scsi/ufs/Makefile @@ -1,4 +1,6 @@ # UFSHCD makefile +obj-$(CONFIG_SCSI_UFS_DWC_HOOKS) += ufshcd-dwc.o +obj-$(CONFIG_SCSI_UFS_DWC_PLAT) += ufs-dwc.o obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o diff --git a/drivers/scsi/ufs/ufs-dwc.c b/drivers/scsi/ufs/ufs-dwc.c new file mode 100644 index 000..6582b9e --- /dev/null +++ b/drivers/scsi/ufs/ufs-dwc.c @@ -0,0 +1,96 @@ +/* + * UFS Host driver for Synopsys Designware Core + * + * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com) + * + * Authors: Joao Pinto + * + * This program is free software; you can
[PATCH 3/3] add support for DWC UFS Host Controller
Signed-off-by: Joao Pinto--- Documentation/devicetree/bindings/ufs/ufs-dwc.txt | 16 + MAINTAINERS | 6 + drivers/scsi/ufs/Kconfig | 45 ++ drivers/scsi/ufs/Makefile | 2 + drivers/scsi/ufs/ufs-dwc.c| 96 +++ drivers/scsi/ufs/ufshcd-dwc.c | 768 ++ drivers/scsi/ufs/ufshcd-dwc.h | 26 + drivers/scsi/ufs/ufshcd-pci.c | 20 + drivers/scsi/ufs/ufshcd.c | 32 +- drivers/scsi/ufs/ufshcd.h | 22 + drivers/scsi/ufs/ufshci-dwc.h | 42 ++ drivers/scsi/ufs/unipro.h | 39 ++ 12 files changed, 1100 insertions(+), 14 deletions(-) create mode 100644 Documentation/devicetree/bindings/ufs/ufs-dwc.txt create mode 100644 drivers/scsi/ufs/ufs-dwc.c create mode 100644 drivers/scsi/ufs/ufshcd-dwc.c create mode 100644 drivers/scsi/ufs/ufshcd-dwc.h create mode 100644 drivers/scsi/ufs/ufshci-dwc.h diff --git a/Documentation/devicetree/bindings/ufs/ufs-dwc.txt b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt new file mode 100644 index 000..fa361f2 --- /dev/null +++ b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" +- reg : +- interrupts: + +Example: + dwc_ufshcd@0xD000 { + compatible = "snps,ufshcd"; + reg = < 0xD000 0x1 >; + interrupts = < 24 >; + }; diff --git a/MAINTAINERS b/MAINTAINERS index d2f94e2..b5d2fdb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11006,6 +11006,12 @@ S: Supported F: Documentation/scsi/ufs.txt F: drivers/scsi/ufs/ +UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER DWC HOOKS +M: Joao Pinto +L: linux-s...@vger.kernel.org +S: Supported +F: drivers/scsi/ufs/*-dwc-* + UNSORTED BLOCK IMAGES (UBI) M: Artem Bityutskiy M: Richard Weinberger diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig index 5f45307..86be4c8 100644 --- a/drivers/scsi/ufs/Kconfig +++ b/drivers/scsi/ufs/Kconfig @@ -83,3 +83,48 @@ config SCSI_UFS_QCOM Select this if you have UFS controller on QCOM chipset. If unsure, say N. + +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- + This selects the DesignWare hooks for the UFS host controller. + + Select this if you have a DesignWare UFS controller. + If unsure, say N. + +config SCSI_UFS_DWC_PLAT + tristate "DesignWare UFS controller platform glue driver" + depends on SCSI_UFS_DWC_HOOKS && SCSI_UFSHCD_PLATFORM + ---help--- + This selects the DesignWare UFS host controller platform glue driver. + + Select this if you have a DesignWare UFS controller on Platform bus. + If unsure, say N. + +config SCSI_UFS_DWC_MPHY_TC + bool "Support for the Synopsys MPHY Test Chip" + depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) + ---help--- + This selects the support for the Synopsys MPHY Test Chip. + + Select this if you have a Synopsys MPHY Test Chip. + If unsure, say N. + +config SCSI_UFS_DWC_20BIT_RMMI + bool "20-bit RMMI MPHY" + depends on SCSI_UFS_DWC_MPHY_TC + ---help--- + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. + + Select this if you are using a 40-bit RMMI Synopsys MPHY. + If unsure, say N. + +config SCSI_UFS_DWC_40BIT_RMMI + bool "40-bit RMMI MPHY" + depends on SCSI_UFS_DWC_MPHY_TC + ---help--- + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. + + Select this if you are using a 40-bit RMMI Synopsys MPHY. + If unsure, say N. diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile index 8303bcc..5be2363 100644 --- a/drivers/scsi/ufs/Makefile +++ b/drivers/scsi/ufs/Makefile @@ -1,4 +1,6 @@ # UFSHCD makefile +obj-$(CONFIG_SCSI_UFS_DWC_HOOKS) += ufshcd-dwc.o +obj-$(CONFIG_SCSI_UFS_DWC_PLAT) += ufs-dwc.o obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o obj-$(CONFIG_SCSI_UFSHCD) += ufshcd.o obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o diff --git a/drivers/scsi/ufs/ufs-dwc.c b/drivers/scsi/ufs/ufs-dwc.c new file mode 100644 index 000..6582b9e --- /dev/null +++ b/drivers/scsi/ufs/ufs-dwc.c @@ -0,0 +1,96 @@ +/* + * UFS Host driver for Synopsys Designware Core + * + * Copyright (C) 2015-2016 Synopsys, Inc.
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi Arnd, On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: >> >> Signed-off-by: Joao Pinto> > This needs a changelog comment, like every patch. > >> @@ -0,0 +1,16 @@ >> +* Universal Flash Storage (UFS) DesignWare Host Controller >> + >> +DWC_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 "snps,ufshcd" > > Are there multiple versions of this controller? Usually for designware > parts the version is known, so we should document which versions exist This controller recent releases was 2.0, but we released last year 1.1. The driver works with both. The driver must work with all DWC UFS versions. > >> + >> +config SCSI_UFS_DWC_HOOKS >> +bool "DesignWare hooks to UFS controller" >> +depends on SCSI_UFSHCD >> +---help--- >> + This selects the DesignWare hooks for the UFS host controller. >> + >> + Select this if you have a DesignWare UFS controller. >> + If unsure, say N. > > This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys hooks would be selected also which in my opinion is not very accurate. In my opinion we should have a selectable DWC_HOOKS. > >> +config SCSI_UFS_DWC_MPHY_TC >> +bool "Support for the Synopsys MPHY Test Chip" >> +depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) >> +---help--- >> + This selects the support for the Synopsys MPHY Test Chip. >> + >> + Select this if you have a Synopsys MPHY Test Chip. >> + If unsure, say N. >> + >> +config SCSI_UFS_DWC_20BIT_RMMI >> +bool "20-bit RMMI MPHY" >> +depends on SCSI_UFS_DWC_MPHY_TC >> +---help--- >> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. >> + >> + Select this if you are using a 40-bit RMMI Synopsys MPHY. >> + If unsure, say N. >> + >> +config SCSI_UFS_DWC_40BIT_RMMI >> +bool "40-bit RMMI MPHY" >> +depends on SCSI_UFS_DWC_MPHY_TC >> +---help--- >> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. >> + >> + Select this if you are using a 40-bit RMMI Synopsys MPHY. >> + If unsure, say N. > > Move the PHY config options to drivers/phy/ along with the respective > drivers, the device using the PHY should not need to be aware which > one is being used. This MPHY config is just to enable parts of the host config which consists of specific unipro commands and nothing else. The unipro command execution uses functions that are in the ufshcd.c. But if everyone agree we can do as you suggest. > >> +/** >> + * ufshcd_dwc_setup_mphy() >> + * This function configures Local (host) Synopsys MPHY specific attributes >> + * >> + * @hba: Pointer to drivers structure >> + * >> + * Returns 0 on success non-zero value on failure >> + */ >> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) >> +{ >> +int ret = 0; >> + >> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI >> +dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); >> +ret = ufshcd_dwc_setup_40bit_rmmi(hba); >> +if (ret) { >> +dev_err(hba->dev, "40-bit RMMI configuration failed"); >> +goto out; >> +} >> +#else >> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI >> +dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); >> +ret = ufshcd_dwc_setup_20bit_rmmi(hba); >> +if (ret) { >> +dev_err(hba->dev, "20-bit RMMI configuration failed"); >> +goto out; >> +} >> +#endif >> +#endif >> +/* To write Shadow register bank to effective configuration block */ >> +ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); >> +if (ret) >> +goto out; >> + >> +/* To configure Debug OMC */ >> +ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); >> + >> +out: >> +return ret; >> +} > > Try to use the generic PHY abstraction here and remove all the #ifdef etc. Could you please point an example for me to check? > >> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c >> index d15eaa4..66518a1 100644 >> --- a/drivers/scsi/ufs/ufshcd-pci.c >> +++ b/drivers/scsi/ufs/ufshcd-pci.c >> @@ -34,6 +34,9 @@ >> */ >> >> #include "ufshcd.h" >> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS >> +#include "ufshcd-dwc.h" >> +#endif >> #include >> #include >> >> @@ -83,6 +86,16 @@ static int ufshcd_pci_runtime_idle(struct device *dev) >> #define ufshcd_pci_runtime_idle NULL >> #endif /* CONFIG_PM */ >> >> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS >> +/** >> + * struct ufs_hba_dwc_vops - UFS DWC specific variant operations >> + */ >> +static struct ufs_hba_variant_ops ufs_dwc_pci_hba_vops = { >> +.name = "dwc-pci", >> +.custom_probe_hba = ufshcd_dwc_configuration, >>
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: > > Hi Arnd, > > On 2/3/2016 12:54 PM, Arnd Bergmann wrote: > > On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > >> > >> Signed-off-by: Joao Pinto> > > > This needs a changelog comment, like every patch. > > > >> @@ -0,0 +1,16 @@ > >> +* Universal Flash Storage (UFS) DesignWare Host Controller > >> + > >> +DWC_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 "snps,ufshcd" > > > > Are there multiple versions of this controller? Usually for designware > > parts the version is known, so we should document which versions exist > > This controller recent releases was 2.0, but we released last year 1.1. The > driver works with both. The driver must work with all DWC UFS versions. Ok, then make the driver match on the "snps,ufshcd-1.1" compatible string, but document both strings in the binding document, and make it mandatory to specify the 1.1 version as a compatible fallback. If we ever need to handle a quirk for the 2.0 version then, it can easily be done. > >> +config SCSI_UFS_DWC_HOOKS > >> + bool "DesignWare hooks to UFS controller" > >> + depends on SCSI_UFSHCD > >> + ---help--- > >> +This selects the DesignWare hooks for the UFS host controller. > >> + > >> +Select this if you have a DesignWare UFS controller. > >> +If unsure, say N. > > > > This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT > > We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys > hooks would be selected also which in my opinion is not very accurate. > In my opinion we should have a selectable DWC_HOOKS. I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS even if nothing uses it and you only have SCSI_UFS_QCOM enabled. With my suggestion, the hooks would disappear unless they are actually used. Then again, with my later comments, we no longer need the hooks. > >> +/** > >> + * ufshcd_dwc_setup_mphy() > >> + * This function configures Local (host) Synopsys MPHY specific attributes > >> + * > >> + * @hba: Pointer to drivers structure > >> + * > >> + * Returns 0 on success non-zero value on failure > >> + */ > >> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) > >> +{ > >> + int ret = 0; > >> + > >> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); > >> + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "40-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#else > >> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI > >> + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); > >> + ret = ufshcd_dwc_setup_20bit_rmmi(hba); > >> + if (ret) { > >> + dev_err(hba->dev, "20-bit RMMI configuration failed"); > >> + goto out; > >> + } > >> +#endif > >> +#endif > >> + /* To write Shadow register bank to effective configuration block */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); > >> + if (ret) > >> + goto out; > >> + > >> + /* To configure Debug OMC */ > >> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); > >> + > >> +out: > >> + return ret; > >> +} > > > > Try to use the generic PHY abstraction here and remove all the #ifdef etc. > > Could you please point an example for me to check? drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. This should probably be moved into the generic UFS platform driver so the PHY handling can be shared between all backends. > >> }; > > > > I think you're better off with a separate PCI driver for this. Remove > > all the #ifdef mess here, put whatever is dwc specific into a new file, > > and perhaps move the common parts into a shared file that can be used > > by both the samsung and designware drivers. > > I have a branch with that approach, but honestly it would be a big change in > the > UFS arch for the pci and I decided to make it simple. I sent that suggestion > for > the scsi mailing list and the comments showed me that. Does anyone have > anything > against putting ufshcd-pci.c as a pci common code and then have a > ufs-dwc-pci.c > and a ufs-samsung-pci.c that uses that common code? Another approach would be to just rename the existing file to ufs-samsung-pci.c and start the ufs-dwc-pci.c as a copy of that. The file is not really all that large anyway. Arnd
Re: [PATCH 3/3] add support for DWC UFS Host Controller
Hi, On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: >> >> Hi Arnd, >> >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: Signed-off-by: Joao Pinto>>> >>> This needs a changelog comment, like every patch. >>> @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" >>> >>> Are there multiple versions of this controller? Usually for designware >>> parts the version is known, so we should document which versions exist >> >> This controller recent releases was 2.0, but we released last year 1.1. The >> driver works with both. The driver must work with all DWC UFS versions. > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > string, but document both strings in the binding document, and make > it mandatory to specify the 1.1 version as a compatible fallback. > > If we ever need to handle a quirk for the 2.0 version then, it can > easily be done. We need the driver to support UFS 2.0 because it is our latest release and is the done that Synopsys is focused now. We could call it "snps, ufshcd-2.0" then. What do you think? > +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- +This selects the DesignWare hooks for the UFS host controller. + +Select this if you have a DesignWare UFS controller. +If unsure, say N. >>> >>> This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT >> >> We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys >> hooks would be selected also which in my opinion is not very accurate. >> In my opinion we should have a selectable DWC_HOOKS. > > I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS > even if nothing uses it and you only have SCSI_UFS_QCOM enabled. SCSI_UFS_QCOM is select if you are using a QCOM and PLAT is enabled. We can do it the following way: If DWC Platform or DWC PCI are selected than automatically select the HOOKS. What do you think? > > With my suggestion, the hooks would disappear unless they are > actually used. > > Then again, with my later comments, we no longer need the hooks. > > +/** + * ufshcd_dwc_setup_mphy() + * This function configures Local (host) Synopsys MPHY specific attributes + * + * @hba: Pointer to drivers structure + * + * Returns 0 on success non-zero value on failure + */ +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) +{ + int ret = 0; + +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); + ret = ufshcd_dwc_setup_40bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "40-bit RMMI configuration failed"); + goto out; + } +#else +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); + ret = ufshcd_dwc_setup_20bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "20-bit RMMI configuration failed"); + goto out; + } +#endif +#endif + /* To write Shadow register bank to effective configuration block */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); + if (ret) + goto out; + + /* To configure Debug OMC */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); + +out: + return ret; +} >>> >>> Try to use the generic PHY abstraction here and remove all the #ifdef etc. >> >> Could you please point an example for me to check? > > drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through > the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. > > This should probably be moved into the generic UFS platform driver so the PHY > handling can be shared between all backends. > I will check it out. }; >>> >>> I think you're better off with a separate PCI driver for this. Remove >>> all the #ifdef mess here, put whatever is dwc specific into a new file, >>> and perhaps move the common parts into a shared file that can be used >>> by both the samsung and designware drivers. >> >> I have a branch with that approach, but honestly it would be a big change in >> the >> UFS arch for the pci and I decided to make it simple. I sent that suggestion >> for >> the scsi mailing list and the comments showed me that. Does anyone have >> anything >> against putting ufshcd-pci.c as a pci common code and then have a >> ufs-dwc-pci.c >> and a ufs-samsung-pci.c that uses that common
Re: [PATCH 3/3] add support for DWC UFS Host Controller
On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: > > Signed-off-by: Joao PintoThis needs a changelog comment, like every patch. > @@ -0,0 +1,16 @@ > +* Universal Flash Storage (UFS) DesignWare Host Controller > + > +DWC_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 "snps,ufshcd" Are there multiple versions of this controller? Usually for designware parts the version is known, so we should document which versions exist > + > +config SCSI_UFS_DWC_HOOKS > + bool "DesignWare hooks to UFS controller" > + depends on SCSI_UFSHCD > + ---help--- > + This selects the DesignWare hooks for the UFS host controller. > + > + Select this if you have a DesignWare UFS controller. > + If unsure, say N. This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT > +config SCSI_UFS_DWC_MPHY_TC > + bool "Support for the Synopsys MPHY Test Chip" > + depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT) > + ---help--- > + This selects the support for the Synopsys MPHY Test Chip. > + > + Select this if you have a Synopsys MPHY Test Chip. > + If unsure, say N. > + > +config SCSI_UFS_DWC_20BIT_RMMI > + bool "20-bit RMMI MPHY" > + depends on SCSI_UFS_DWC_MPHY_TC > + ---help--- > + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. > + > + Select this if you are using a 40-bit RMMI Synopsys MPHY. > + If unsure, say N. > + > +config SCSI_UFS_DWC_40BIT_RMMI > + bool "40-bit RMMI MPHY" > + depends on SCSI_UFS_DWC_MPHY_TC > + ---help--- > + This specifies that the Synopsys MPHY supports 40-bit RMMI operations. > + > + Select this if you are using a 40-bit RMMI Synopsys MPHY. > + If unsure, say N. Move the PHY config options to drivers/phy/ along with the respective drivers, the device using the PHY should not need to be aware which one is being used. > +/** > + * ufshcd_dwc_setup_mphy() > + * This function configures Local (host) Synopsys MPHY specific attributes > + * > + * @hba: Pointer to drivers structure > + * > + * Returns 0 on success non-zero value on failure > + */ > +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) > +{ > + int ret = 0; > + > +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI > + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); > + ret = ufshcd_dwc_setup_40bit_rmmi(hba); > + if (ret) { > + dev_err(hba->dev, "40-bit RMMI configuration failed"); > + goto out; > + } > +#else > +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI > + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); > + ret = ufshcd_dwc_setup_20bit_rmmi(hba); > + if (ret) { > + dev_err(hba->dev, "20-bit RMMI configuration failed"); > + goto out; > + } > +#endif > +#endif > + /* To write Shadow register bank to effective configuration block */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); > + if (ret) > + goto out; > + > + /* To configure Debug OMC */ > + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); > + > +out: > + return ret; > +} Try to use the generic PHY abstraction here and remove all the #ifdef etc. > diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c > index d15eaa4..66518a1 100644 > --- a/drivers/scsi/ufs/ufshcd-pci.c > +++ b/drivers/scsi/ufs/ufshcd-pci.c > @@ -34,6 +34,9 @@ > */ > > #include "ufshcd.h" > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > +#include "ufshcd-dwc.h" > +#endif > #include > #include > > @@ -83,6 +86,16 @@ static int ufshcd_pci_runtime_idle(struct device *dev) > #define ufshcd_pci_runtime_idle NULL > #endif /* CONFIG_PM */ > > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > +/** > + * struct ufs_hba_dwc_vops - UFS DWC specific variant operations > + */ > +static struct ufs_hba_variant_ops ufs_dwc_pci_hba_vops = { > + .name = "dwc-pci", > + .custom_probe_hba = ufshcd_dwc_configuration, > +}; > +#endif > + > /** > * ufshcd_pci_shutdown - main function to put the controller in reset state > * @pdev: pointer to PCI device handle > @@ -144,6 +157,9 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *id) > > INIT_LIST_HEAD(>clk_list_head); > > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > + hba->vops = _dwc_pci_hba_vops; > +#endif > err = ufshcd_init(hba, mmio_base, pdev->irq); > if (err) { > dev_err(>dev, "Initialization failed\n"); > @@ -167,6 +183,10 @@ static const struct dev_pm_ops ufshcd_pci_pm_ops = { > > static const struct pci_device_id ufshcd_pci_tbl[] = { > { PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 }, > +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS > + { PCI_VENDOR_ID_SYNOPSYS, 0xB101,
Re: [PATCH 3/3] add support for DWC UFS Host Controller
++ Vinayak Holikatti Hi Vinayak, Could I please get your opinion about the patch set? thanks. On 2/3/2016 3:39 PM, Arnd Bergmann wrote: > On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote: >> >> Hi Arnd, >> >> On 2/3/2016 12:54 PM, Arnd Bergmann wrote: >>> On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote: Signed-off-by: Joao Pinto>>> >>> This needs a changelog comment, like every patch. >>> @@ -0,0 +1,16 @@ +* Universal Flash Storage (UFS) DesignWare Host Controller + +DWC_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 "snps,ufshcd" >>> >>> Are there multiple versions of this controller? Usually for designware >>> parts the version is known, so we should document which versions exist >> >> This controller recent releases was 2.0, but we released last year 1.1. The >> driver works with both. The driver must work with all DWC UFS versions. > > Ok, then make the driver match on the "snps,ufshcd-1.1" compatible > string, but document both strings in the binding document, and make > it mandatory to specify the 1.1 version as a compatible fallback. > > If we ever need to handle a quirk for the 2.0 version then, it can > easily be done. > +config SCSI_UFS_DWC_HOOKS + bool "DesignWare hooks to UFS controller" + depends on SCSI_UFSHCD + ---help--- +This selects the DesignWare hooks for the UFS host controller. + +Select this if you have a DesignWare UFS controller. +If unsure, say N. >>> >>> This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT >> >> We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys >> hooks would be selected also which in my opinion is not very accurate. >> In my opinion we should have a selectable DWC_HOOKS. > > I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS > even if nothing uses it and you only have SCSI_UFS_QCOM enabled. > > With my suggestion, the hooks would disappear unless they are > actually used. > > Then again, with my later comments, we no longer need the hooks. > > +/** + * ufshcd_dwc_setup_mphy() + * This function configures Local (host) Synopsys MPHY specific attributes + * + * @hba: Pointer to drivers structure + * + * Returns 0 on success non-zero value on failure + */ +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba) +{ + int ret = 0; + +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI"); + ret = ufshcd_dwc_setup_40bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "40-bit RMMI configuration failed"); + goto out; + } +#else +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI"); + ret = ufshcd_dwc_setup_20bit_rmmi(hba); + if (ret) { + dev_err(hba->dev, "20-bit RMMI configuration failed"); + goto out; + } +#endif +#endif + /* To write Shadow register bank to effective configuration block */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01); + if (ret) + goto out; + + /* To configure Debug OMC */ + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01); + +out: + return ret; +} >>> >>> Try to use the generic PHY abstraction here and remove all the #ifdef etc. >> >> Could you please point an example for me to check? > > drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through > the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces. > > This should probably be moved into the generic UFS platform driver so the PHY > handling can be shared between all backends. > }; >>> >>> I think you're better off with a separate PCI driver for this. Remove >>> all the #ifdef mess here, put whatever is dwc specific into a new file, >>> and perhaps move the common parts into a shared file that can be used >>> by both the samsung and designware drivers. >> >> I have a branch with that approach, but honestly it would be a big change in >> the >> UFS arch for the pci and I decided to make it simple. I sent that suggestion >> for >> the scsi mailing list and the comments showed me that. Does anyone have >> anything >> against putting ufshcd-pci.c as a pci common code and then have a >> ufs-dwc-pci.c >> and a ufs-samsung-pci.c that uses that common code? > > Another approach would be to just rename the existing file to > ufs-samsung-pci.c > and start the ufs-dwc-pci.c as a copy of that. The file is not really all > that > large anyway. > > Arnd >