Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

[Rob Herring]

On Thu, Jun 4, 2015 at 9:32 AM, Akinobu Mita akinobu.m...@gmail.com wrote:
 Hi Yaniv,

 2015-06-03 18:37 GMT+09:00 Yaniv Gardi yga...@codeaurora.org:
 @@ -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:

But I do have objections on both the naming and having a sub-node.


 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 },

Be more specific: qcom,socname-ufs

 #neidf

Drop the ifdef.

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

Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

[Rob Herring]

On Sun, Jun 7, 2015 at 10:32 AM,  yga...@codeaurora.org wrote:
 2015-06-05 5:53 GMT+09:00  yga...@codeaurora.org:
 Hi Yaniv,

 2015-06-03 18:37 GMT+09:00 Yaniv Gardi yga...@codeaurora.org:
 @@ -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.

This is due to the fact that you have 2 platform drivers. You should
only have 1 (and 1 node). If you really think you need 2, then you
should do like many other common *HCIs do and make the base UFS driver
a set of library functions that drivers can use or call. Look at EHCI,
AHCI, SDHCI, etc. for inspiration.

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

Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

[Rob Herring]

On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org wrote:
 On Sun, Jun 7, 2015 at 10:32 AM,  yga...@codeaurora.org wrote:
 2015-06-05 5:53 GMT+09:00  yga...@codeaurora.org:

[...]

 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.

 This is due to the fact that you have 2 platform drivers. You should
 only have 1 (and 1 node). If you really think you need 2, then you
 should do like many other common *HCIs do and make the base UFS driver
 a set of library functions that drivers can use or call. Look at EHCI,
 AHCI, SDHCI, etc. for inspiration.

 Hi Rob,
 We did look at SDHCI and decided to go with this design due to its
 simplicity and lack of library functions. Yaniv described the proper flow
 of probing and, as we understand things, it is guaranteed to work as
 designed.

 Furthermore, the design of having a subcore in the dts is used in the
 Linux kernel. Please have a look at drivers/usb/dwc3 where - as an example
 - both dwc3-msm and dwc3-exynox invoke the probing function in core.c
 (i.e. the shared underlying Synopsys USB dwc3 core) by calling
 of_platform_populate().

That binding has the same problem. Please don't propagate that. There
is no point in a sub-node in this case.

 Do you see a benefit in the SDHCi implementation?

Yes, it does not let the kernel driver design dictate the hardware description.

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

Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

[Rob Herring]

On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org wrote:
 On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org
 wrote:
 On Sun, Jun 7, 2015 at 10:32 AM,  yga...@codeaurora.org wrote:
 2015-06-05 5:53 GMT+09:00  yga...@codeaurora.org:

 [...]

 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.

 This is due to the fact that you have 2 platform drivers. You should
 only have 1 (and 1 node). If you really think you need 2, then you
 should do like many other common *HCIs do and make the base UFS driver
 a set of library functions that drivers can use or call. Look at EHCI,
 AHCI, SDHCI, etc. for inspiration.

 Hi Rob,
 We did look at SDHCI and decided to go with this design due to its
 simplicity and lack of library functions. Yaniv described the proper
 flow
 of probing and, as we understand things, it is guaranteed to work as
 designed.

 Furthermore, the design of having a subcore in the dts is used in the
 Linux kernel. Please have a look at drivers/usb/dwc3 where - as an
 example
 - both dwc3-msm and dwc3-exynox invoke the probing function in core.c
 (i.e. the shared underlying Synopsys USB dwc3 core) by calling
 of_platform_populate().

 That binding has the same problem. Please don't propagate that. There
 is no point in a sub-node in this case.

 Do you see a benefit in the SDHCi implementation?

 Yes, it does not let the kernel driver design dictate the hardware
 description.

 Rob


 Hi Rob,
 We appear to be having a philosophical disagreement on the practicality of
 designing the ufshcd variant's implementation - in other words, we
 disagree on the proper design pattern to follow here.
 If I understand correctly, you are concerned with a design pattern wherein
 a generic implementation is wrapped - at the device-tree level - in a
 variant implementation. The main reason for your concern is that you don't
 want the kernel driver design dictate the hardware description.

 We considered this point when we suggested our implementation (both before
 and after you raised it) and reached the conclusion that - while an
 important consideration - it should not be the prevailing one. I believe
 that you will agree once you read the reasoning. What guided us was the
 following:
 1. Keep our change minimal.
 2. Keep our patch in line with known design patterns in the kernel.
 3. Have our patch extend the existing solution rather than reinvent it.

 It is the 3rd point that is most important to this discussion, since UFS
 has already been deployed by various vendors and is used by OEM. Changing
 ufshcd to a set of library functions that would be called by variants
 would necessarily introduce a significant change to the code flow in many
 places and would pose a backward compatibility issue. By using the tried
 and tested pattern of subnodes in the dts we were able to keep the change
 simple, succinct, understandable, maintainable and backward compatible. In
 fact, the entire logic tying of the generic implementation to the variant
 takes ~20 lines of code - both short and elegant.

The DWC3 binding does this and nothing else that I'm aware of. This
hardly makes for a common pattern. If you really want to split this to
2 devices, you can create platform devices without having a DT node.

If you want to convince me this is the right approach for the binding
then you need to convince me the h/w is actually split this way and
there is functionality separate from the licensed IP.

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

Re: [PATCH v2 4/4] scsi: ufs: probe and init of variant driver from the platform device

[Rob Herring]

On Wed, Jun 17, 2015 at 8:17 AM, Dov Levenglick d...@codeaurora.org wrote:
 On Wed, Jun 17, 2015 at 2:42 AM, Dov Levenglick d...@codeaurora.org
 wrote:
 On Tue, Jun 9, 2015 at 12:53 AM, Dov Levenglick d...@codeaurora.org
 wrote:
 On Sun, Jun 7, 2015 at 10:32 AM,  yga...@codeaurora.org wrote:
 2015-06-05 5:53 GMT+09:00  yga...@codeaurora.org:

 [...]

 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.

 This is due to the fact that you have 2 platform drivers. You should
 only have 1 (and 1 node). If you really think you need 2, then you
 should do like many other common *HCIs do and make the base UFS
 driver
 a set of library functions that drivers can use or call. Look at
 EHCI,
 AHCI, SDHCI, etc. for inspiration.

 Hi Rob,
 We did look at SDHCI and decided to go with this design due to its
 simplicity and lack of library functions. Yaniv described the proper
 flow
 of probing and, as we understand things, it is guaranteed to work as
 designed.

 Furthermore, the design of having a subcore in the dts is used in the
 Linux kernel. Please have a look at drivers/usb/dwc3 where - as an
 example
 - both dwc3-msm and dwc3-exynox invoke the probing function in core.c
 (i.e. the shared underlying Synopsys USB dwc3 core) by calling
 of_platform_populate().

 That binding has the same problem. Please don't propagate that. There
 is no point in a sub-node in this case.

 Do you see a benefit in the SDHCi implementation?

 Yes, it does not let the kernel driver design dictate the hardware
 description.

 Rob


 Hi Rob,
 We appear to be having a philosophical disagreement on the practicality
 of
 designing the ufshcd variant's implementation - in other words, we
 disagree on the proper design pattern to follow here.
 If I understand correctly, you are concerned with a design pattern
 wherein
 a generic implementation is wrapped - at the device-tree level - in a
 variant implementation. The main reason for your concern is that you
 don't
 want the kernel driver design dictate the hardware description.

 We considered this point when we suggested our implementation (both
 before
 and after you raised it) and reached the conclusion that - while an
 important consideration - it should not be the prevailing one. I believe
 that you will agree once you read the reasoning. What guided us was the
 following:
 1. Keep our change minimal.
 2. Keep our patch in line with known design patterns in the kernel.
 3. Have our patch extend the existing solution rather than reinvent it.

 It is the 3rd point that is most important to this discussion, since UFS
 has already been deployed by various vendors and is used by OEM.
 Changing
 ufshcd to a set of library functions that would be called by variants
 would necessarily introduce a significant change to the code flow in
 many
 places and would pose a backward compatibility issue. By using the tried
 and tested pattern of subnodes in the dts we were able to keep the
 change
 simple, succinct, understandable, maintainable and backward compatible.
 In
 fact, the entire logic tying of the generic implementation to the
 variant
 takes ~20 lines of code - both short and elegant.

 The DWC3 binding does this and nothing else that I'm aware of. This
 hardly makes for a common pattern. If you really want to split this to
 2 devices, you can create platform devices without having a DT node.

 If you want to convince me this is the right approach for the binding
 then you need to convince me the h/w is actually split this way and
 there is functionality separate from the licensed IP.

 Rob


 I don't understand the challenge that you just posed. It is clear from our
 implementation that there is the standard and variants thereof. I know
 this to be a fact on the processors that we are working on.

 Furthermore, although I didn't check each and every result in the search,
 of_platform_populate is used in more locations than dwc3 and at least a
 few of them seem have be using the same paradigm as ours
 (http://lxr.free-electrons.com/ident?i=of_platform_populate).

You can ignore everything under arch/ as those are root level calls.
Most of the rest of the list are devices which have multiple unrelated
functions such as PMICs or system controllers which are perfectly
valid uses of of_platform_populate. That leaves at most 10 examples
that may or may not have good bindings. 10 out of hundreds of drivers
in the kernel hardly makes for a pattern to follow.

Let me be perfectly clear on the binding as is: NAK. I'm done replying
until you propose something different.

Rob
--
To unsubscribe from 

Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

[Rob Herring]

On Aug 21, 2015 3:10 PM, Yaniv Gardi yga...@codeaurora.org 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 yga...@codeaurora.org

 ---
  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?

  }

  /**
 @@ -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

Re: [PATCH v2 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

O
n Aug 20, 2015 6:59 AM, Yaniv Gardi yga...@codeaurora.org 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 yga...@codeaurora.org

 ---
  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 linux/phy/phy-qcom-ufs.h
  #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?

 +   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?

 +   {},
 +};
 +
 +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 linux/of.h

  #include ufshcd.h
 -
 -static const struct of_device_id ufs_of_match[];
 -static struct ufs_hba_variant_ops *get_variant_ops(struct device *dev)
 -{
 -   if (dev-of_node) {
 -   const struct of_device_id *match;
 -
 -   match = of_match_node(ufs_of_match, dev-of_node);
 -  

Re: [PATCH v1 15/15] scsi: ufs: add wrapper for retrying sending query attribute

[Rob Herring]

On Aug 21, 2015 3:10 PM, Yaniv Gardi yga...@codeaurora.org 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?



 Signed-off-by: Yaniv Gardi yga...@codeaurora.org

 ---
  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.

 +   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 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 v8 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

On Sun, Oct 25, 2015 at 5:50 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 

I already gave you a reviewed-by, so please include them when posting
new versions. (But no need to repost just to add them).

Rob

>
> ---
>  Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 58 +
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  | 11 ++-
>  drivers/scsi/ufs/ufs-qcom.c| 62 +-
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 98 
> ++
>  drivers/scsi/ufs/ufshcd-pltfrm.h   | 41 +
>  drivers/scsi/ufs/ufshcd.c  | 10 +++
>  drivers/scsi/ufs/ufshcd.h  |  1 +
>  7 files changed, 207 insertions(+), 74 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-qcom.txt
>  create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h
>
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> new file mode 100644
> index 000..070baf4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -0,0 +1,58 @@
> +* Qualcomm Technologies Inc 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-phy-qmp-20nm"
> + or "qcom,ufs-phy-qmp-14nm" according to the relevant 
> phy in use.
> +- reg   : should contain PHY register address space (mandatory),
> +- reg-names : indicates various resources passed to driver (via reg 
> proptery) by name.
> +  Required "reg-names" is "phy_mem".
> +- #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
> +- clocks   : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> + order as the clocks property. "ref_clk_src", "ref_clk",
> + "tx_iface_clk" & "rx_iface_clk" are mandatory but
> + "ref_clk_parent" is optional
> +
> +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
> +- vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
> +- vddp-ref-clk-max-microamp : specifies max. load that can be drawn from 
> this supply
> +- vddp-ref-clk-always-on : specifies if this supply needs to be kept always 
> on
> +
> +Example:
> +
> +   ufsphy1: ufsphy@0xfc597000 {
> +   compatible = "qcom,ufs-phy-qmp-20nm";
> +   reg = <0xfc597000 0x800>;
> +   reg-names = "phy_mem";
> +   #phy-cells = <0>;
> +   vdda-phy-supply = <_l4>;
> +   vdda-pll-supply = <_l12>;
> +   vdda-phy-max-microamp = <5>;
> +   vdda-pll-max-microamp = <1000>;
> +   clock-names = "ref_clk_src",
> +   "ref_clk_parent",
> +   "ref_clk",
> +   "tx_iface_clk",
> +   "rx_iface_clk";
> +   clocks = <_rpm clk_ln_bb_clk>,
> +   <_gcc clk_pcie_1_phy_ldo >,
> +   <_gcc clk_ufs_phy_ldo>,
> +   <_gcc clk_gcc_ufs_tx_cfg_clk>,
> +   <_gcc clk_gcc_ufs_rx_cfg_clk>;
> +   };
> +
> +   ufshc@0xfc598000 {
> +   ...
> +   phys = <>;
> +   phy-names = "ufsphy";
> +   };
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index 5357919..03c0e98 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -4,11 +4,18 @@ UFSHC nodes are defined to describe on-chip UFS host 
> controllers.
>  Each UFS 

Re: [PATCH v3 02/32] devicetree: bindings: scsi: HiSi SAS

[Rob Herring]

On Tue, Nov 10, 2015 at 12:32:07AM +0800, John Garry wrote:
> Add devicetree bindings for HiSilicon SAS driver.
> 
> Signed-off-by: John Garry 
> Signed-off-by: Zhangfei Gao 
> ---
>  .../devicetree/bindings/scsi/hisilicon-sas.txt | 81 
> ++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> 
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> new file mode 100644
> index 000..2333cc3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -0,0 +1,81 @@
> +* HiSilicon SAS controller
> +
> +The HiSilicon SAS controller supports SAS/SATA.
> +
> +Main node required properties:
> +  - compatible : value should be as follows:
> + (a) "hisilicon,sas-controller-v1" for v1 of HiSilicon SAS controller IP

Please do a more specific compatible string with the SOC part number. 
Same versions of IP blocks can have different integration/process 
features/bugs.

> +  - sas-addr : array of 8 bytes for host SAS address
> +  - reg : Address and length of the SAS register
> +  - hisilicon,sas-syscon: phandle of syscon used for sas control
> +  - ctrl-reset-reg : offset to controller reset register in ctrl reg
> +  - ctrl-reset-sts-reg : offset to controller reset status register in ctrl 
> reg
> +  - ctrl-clock-ena-reg : offset to controller clock enable register in ctrl 
> reg
> +  - queue-count : number of delivery and completion queues in the controller
> +  - phy-count : number of phys accessible by the controller
> +  - interrupts : Interrupts for phys, completion queues, and fatal
> + sources; the interrupts are ordered in 3 groups, as follows:
> +   - Phy interrupts
> +   - Completion queue interrupts
> +   - Fatal interrupts
> + Phy interrupts : Each phy has 3 interrupt sources:
> + - broadcast
> + - phyup
> + - abnormal
> + The phy interrupts are ordered into groups of 3 per phy
> + (broadcast, phyup, and abnormal) in increasing order.
> + Completion queue interrupts : each completion queue has 1
> + interrupt source. The interrupts are ordered in
> + increasing order.
> + Fatal interrupts : the fatal interrupts are ordered as follows:
> + - ECC
> + - AXI bus
> +
> +* HiSilicon SAS syscon
> +
> +Required properties:
> +- compatible: should be "hisilicon,sas-ctrl", "syscon"

Please add a more specific compatible here too.

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

Re: [PATCH v5 10/11] Documentation: devicetree: ufs: Add DT bindings for exynos UFS host controller

[Rob Herring]

On Mon, Nov 09, 2015 at 10:56:26AM +0530, Alim Akhtar wrote:
> From: Seungwon Jeon 
> 
> This adds Exynos Universal Flash Storage (UFS) Host Controller DT bindings.
> 
> Signed-off-by: Seungwon Jeon 
> Signed-off-by: Alim Akhtar 
> ---
>  .../devicetree/bindings/ufs/ufs-exynos.txt |  104 
> 
>  1 file changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-exynos.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-exynos.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-exynos.txt
> new file mode 100644
> index ..08e2d1497b1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-exynos.txt
> @@ -0,0 +1,104 @@
> +* Exynos Universal Flash Storage (UFS) Host Controller
> +
> +UFSHC nodes are defined to describe on-chip UFS host controllers.
> +Each UFS controller instance should have its own node.
> +
> +Required properties:
> +- compatible: compatible name, contains "samsung,exynos7-ufs"
> +- interrupts: 
> +- reg   : Should contain HCI, vendor specific, UNIPRO and
> +   UFS protector address space
> +- reg-names  : "hci", "vs_hci", "unipro", "ufsp";

No phy for MPHY?

> +
> +Optional properties:
> +- vdd-hba-supply: phandle to UFS host controller supply regulator 
> node
> +- vcc-supply: phandle to VCC supply regulator node
> +- vccq-supply   : phandle to VCCQ supply regulator node
> +- vccq2-supply  : phandle to VCCQ2 supply regulator node
> +- vcc-supply-1p8: For embedded UFS devices, valid VCC range is 
> 1.7-1.95V
> +  or 2.7-3.6V. This boolean property when set, 
> specifies
> +   to use low voltage range of 1.7-1.95V. Note for 
> external
> +   UFS cards this property is invalid and valid VCC 
> range is
> +   always 2.7-3.6V.
> +- vcc-max-microamp  : specifies max. load that can be drawn from vcc 
> supply
> +- vccq-max-microamp : specifies max. load that can be drawn from vccq 
> supply
> +- vccq2-max-microamp: specifies max. load that can be drawn from vccq2 
> supply

Some of these are supplies to the flash chip, so you should make 
these common properties (in a common doc).

> +- -fixed-regulator : boolean property specifying that -supply is 
> a fixed regulator

This should be determined from the regulator.

> +
> +- clocks: List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> +  order as the clocks property.
> +   "core", "sclk_unipro_main", "ref" and ref_parent
> +
> +- freq-table-hz  : Array of  operating frequencies 
> stored in the same
> +   order as the clocks property. If this property is not
> +   defined or a value in the array is "0" then it is 
> assumed
> +   that the frequency is set by the parent clock or a
> +   fixed rate clock source.
> +- pclk-freq-avail-range : specifies available frequency range(min/max) for 
> APB clock
> +- ufs,pwr-attr-mode : specifies mode value for power mode change, possible 
> values are
> + "FAST", "SLOW", "FAST_auto" and "SLOW_auto"

ufs is not a vendor. Use a '-' rather than ','.

> +- ufs,pwr-attr-lane : specifies lane count value for power mode change
> +   allowed values are 1 or 2
> +- ufs,pwr-attr-gear : specifies gear count value for power mode change
> +   allowed values are 1 or 2
> +- ufs,pwr-attr-hs-series : specifies HS rate series for power mode change
> +can be one of "HS_rate_b" or "HS_rate_a"
> +- ufs,pwr-local-l2-timer : specifies array of local UNIPRO L2 timer values
> +3 timers supported
> + AFC0ReqTimeOutVal>
> +- ufs,pwr-remote-l2-timer : specifies array of remote UNIPRO L2 timer values
> +3 timers supported
> + AFC0ReqTimeOutVal>
> +- ufs-rx-adv-fine-gran-sup_en : specifies support of fine granularity of 
> MPHY,
> +   this is a boolean property.
> +- ufs-rx-adv-fine-gran-step : specifies granularity steps of MPHY,
> +   allowed step size is 0 to 3
> +- ufs-rx-adv-min-activate-time-cap : specifies rx advanced minimum activate 
> time of MPHY
> +  range is 1 to 9
> +- ufs-pa-granularity : specifies Granularity for PA_TActivate and 
> PA_Hibern8Time
> +- ufs-pa-tacctivate : specifies time to wake-up remote M-RX
> +- ufs-pa-hibern8time : specifies minimum time to wait in HIBERN8 state

These are all M-PHY properties?

> +
> 

Re: [PATCH v4 02/32] devicetree: bindings: scsi: HiSi SAS

[Rob Herring]

On Mon, Nov 16, 2015 at 09:05:48PM +0800, John Garry wrote:
> Add devicetree bindings for HiSilicon SAS driver.
> 
> Signed-off-by: John Garry <john.ga...@huawei.com>
> Signed-off-by: Zhangfei Gao <zhangfei@linaro.org>

Acked-by: Rob Herring <r...@kernel.org>

> ---
>  .../devicetree/bindings/scsi/hisilicon-sas.txt | 69 
> ++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> 
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> new file mode 100644
> index 000..f67e761
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -0,0 +1,69 @@
> +* HiSilicon SAS controller
> +
> +The HiSilicon SAS controller supports SAS/SATA.
> +
> +Main node required properties:
> +  - compatible : value should be as follows:
> + (a) "hisilicon,hip05-sas-v1" for v1 hw in hip05 chipset
> +  - sas-addr : array of 8 bytes for host SAS address
> +  - reg : Address and length of the SAS register
> +  - hisilicon,sas-syscon: phandle of syscon used for sas control
> +  - ctrl-reset-reg : offset to controller reset register in ctrl reg
> +  - ctrl-reset-sts-reg : offset to controller reset status register in ctrl 
> reg
> +  - ctrl-clock-ena-reg : offset to controller clock enable register in ctrl 
> reg
> +  - queue-count : number of delivery and completion queues in the controller
> +  - phy-count : number of phys accessible by the controller
> +  - interrupts : Interrupts for phys, completion queues, and fatal
> + sources; the interrupts are ordered in 3 groups, as follows:
> + - Phy interrupts
> + - Completion queue interrupts
> + - Fatal interrupts
> + Phy interrupts : Each phy has 3 interrupt sources:
> + - broadcast
> + - phyup
> + - abnormal
> + The phy interrupts are ordered into groups of 3 per phy
> + (broadcast, phyup, and abnormal) in increasing order.
> + Completion queue interrupts : each completion queue has 1
> + interrupt source.
> + The interrupts are ordered in increasing order.
> + Fatal interrupts : the fatal interrupts are ordered as follows:
> + - ECC
> + - AXI bus
> +
> +Example:
> + sas0: sas@c100 {
> + compatible = "hisilicon,hip05-sas-v1";
> + sas-addr = [50 01 88 20 16 00 00 0a];
> + reg = <0x0 0xc100 0x0 0x1>;
> + hisilicon,sas-syscon = <_sas>;
> + ctrl-reset-reg = <0xa60>;
> + ctrl-reset-sts-reg = <0x5a30>;
> + ctrl-clock-ena-reg = <0x338>;
> + queue-count = <32>;
> + phy-count = <8>;
> + dma-coherent;
> + interrupt-parent = <_dsa>;
> + interrupts =<259 4>,<263 4>,<264 4>,/* phy0 */
> + <269 4>,<273 4>,<274 4>,/* phy1 */
> + <279 4>,<283 4>,<284 4>,/* phy2 */
> + <289 4>,<293 4>,<294 4>,/* phy3 */
> + <299 4>,<303 4>,<304 4>,/* phy4 */
> + <309 4>,<313 4>,<314 4>,/* phy5 */
> + <319 4>,<323 4>,<324 4>,/* phy6 */
> + <329 4>,<333 4>,<334 4>,/* phy7 */
> + <336 1>,<337 1>,<338 1>,/* cq0-2 */
> + <339 1>,<340 1>,<341 1>,/* cq3-5 */
> + <342 1>,<343 1>,<344 1>,/* cq6-8 */
> + <345 1>,<346 1>,<347 1>,/* cq9-11 */
> + <348 1>,<349 1>,<350 1>,/* cq12-14 */
> + <351 1>,<352 1>,<353 1>,/* cq15-17 */
> + <354 1>,<355 1>,<356 1>,/* cq18-20 */
> + <357 1>,<358 1>,<359 1>,/* cq21-23 */
> + <360 1>,<361 1>,<362 1>,/* cq24-26 */
> + <363 1>,<364 1>,<365 1>,/* cq27-29 */
> + <366 1>,<367 1>/* cq30-31 */
> + <376 4>,/* fatal ecc */
> + <381 4>;/* fatal axi */
> + status = "disabled";
> + };
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" 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 02/17] scsi: ufs: add option to change default UFS power management level

[Rob Herring]

On Mon, Oct 26, 2015 at 05:40:57PM +0200, Yaniv Gardi wrote:
> 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 
> 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  8 +
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 17 +-
>  drivers/scsi/ufs/ufshcd.c  | 39 
> ++
>  drivers/scsi/ufs/ufshcd.h  |  4 +--
>  4 files changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index 9fea0de..b7db1c9 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -38,6 +38,14 @@ Optional properties:
> defined or a value in the array is "0" then it is 
> assumed
> that the frequency is set by the parent clock or a
> fixed rate clock source.
> +- rpm-level  : UFS Runtime power management level. Following PM 
> levels are supported:
> +   0 - Both UFS device and Link in active state (Highest 
> power consumption)
> +   1 - UFS device in active state but Link in Hibern8 
> state
> +   2 - UFS device in Sleep state but Link in active state
> +   3 - UFS device in Sleep state and Link in hibern8 
> state (default PM level)
> +   4 - UFS device in Power-down state and Link in 
> Hibern8 state
> +   5 - UFS device in Power-down state and Link in OFF 
> state (Lowest power consumption)
> +- spm-level  : UFS System power management level. Allowed PM levels 
> are same as rpm-level.

These seem too Linux specific. What would make them different for 
runtime-PM vs. system suspend? Power mgmt capabilities in the kernel 
change over time (and vary by OS), so you don't want to encode them into 
DT. I would probably define separately what modes the phy supports and 
the device supports. 

Isn't hibern8 required by the MPHY or UniPro spec? If so, that should be 
assumed to be supported and then have a quirk property to flag it as 
broken.

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

Re: [PATCH v3 02/32] devicetree: bindings: scsi: HiSi SAS

[Rob Herring]

On Tue, Nov 10, 2015 at 5:09 AM, John Garry <john.ga...@huawei.com> wrote:
> On 09/11/2015 18:01, Rob Herring wrote:
>>
>> On Tue, Nov 10, 2015 at 12:32:07AM +0800, John Garry wrote:
>>>
>>> Add devicetree bindings for HiSilicon SAS driver.
>>>
>>> Signed-off-by: John Garry <john.ga...@huawei.com>
>>> Signed-off-by: Zhangfei Gao <zhangfei@linaro.org>
>>> ---
>>>   .../devicetree/bindings/scsi/hisilicon-sas.txt | 81
>>> ++
>>>   1 file changed, 81 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> new file mode 100644
>>> index 000..2333cc3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> @@ -0,0 +1,81 @@
>>> +* HiSilicon SAS controller
>>> +
>>> +The HiSilicon SAS controller supports SAS/SATA.
>>> +
>>> +Main node required properties:
>>> +  - compatible : value should be as follows:
>>> +   (a) "hisilicon,sas-controller-v1" for v1 of HiSilicon SAS
>>> controller IP
>>
>>
>> Please do a more specific compatible string with the SOC part number.
>> Same versions of IP blocks can have different integration/process
>> features/bugs.
>>
>
> How about "hisilicon,hip05-sas-v1"?

That's fine.

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

Re: [PATCH 02/25] devicetree: bindings: scsi: HiSi SAS

[Rob Herring]

On Mon, Oct 12, 2015 at 10:20 AM, John Garry  wrote:
> Add devicetree bindings for HiSilicon SAS driver.

In the future, please use get_maintainers.pl.

> Signed-off-by: John Garry 
> ---
>  .../devicetree/bindings/scsi/hisilicon-sas.txt | 63 
> ++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> new file mode 100644
> index 000..472c022
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -0,0 +1,63 @@
> +* HiSilison SAS controller
> +
> +The HiSilicon SAS controller supports SAS/SATA.
> +
> +Main node required properties:
> +  - compatible : value should be as follows:
> +   (a) "hisilicon,sas-controller-v1" for v1 of HiSilicon SAS controller 
> IP
> +
> +  - controller-id : identifier for controller in the SoC

We don't do indexes in DT (mostly). Why do you need this?

> +  - reg : Address and length of the register sets for the device
> +   - SAS controller registers
> +   - SAS controller control registers
> +
> +  - reset-reg : offset to reset, status, and clock registers in control 
> registers

Within the above register range? If so and if this varies, then that
implies there is more than 1 version of IP. In that case you should
have a more specific compatible string.

How long is this property I count 3 cells here, but the example has 5.
Define what each cell corresponds to specifically.

> +
> +  - queue-count : number of delivery and completion queues in the controller
> +
> +  - phy-count : number of phys accessible by the controller
> +
> +  - interrupts : Interrupts for phys, completion queues, and fatal
> +interrupts:
> + - Each phy has 3 interrupt sources:
> +   - broadcast
> +   - phyup
> +   - abnormal
> + - Each completion queue has 1 interrupt source
> + - Each controller has 2 fatal interrupt sources:
> +   - ECC
> +   - AXI bus
> +
> +Example:
> +   sas0: sas@c100 {
> +   compatible = "hisilicon,sas-controller-v1";
> +   controller-id = <0>;
> +   reg = <0x0 0xc100 0x0 0x1>,
> +   <0x0 0xc000 0x0 0x1>;
> +   reset-reg = <0xa60 0x33c 0x5a30 0xa64 0x338>;
> +   queue-count = <32>;
> +   phy-count = <8>;
> +   #interrupt-cells = <2>;
> +   dma-coherent;
> +   interrupt-parent = <_dsa>;
> +   interrupts = <259 4>, <263 4>,<264 4>,/* phy irq(0~79) */
> +   <269 4>,<273 4>,<274 4>,/* phy irq(0~79) */
> +   <279 4>,<283 4>,<284 4>,/* phy irq(0~79) */
> +   <289 4>,<293 4>,<294 4>,/* phy irq(0~79) */
> +   <299 4>,<303 4>,<304 4>,/* phy irq(0~79) */
> +   <309 4>,<313 4>,<314 4>,/* phy irq(0~79) */
> +   <319 4>,<323 4>,<324 4>,/* phy irq(0~79) */
> +   <329 4>,<333 4>,<334 4>,/* phy irq(0~79) */
> +   <336 1>,<337 1>,<338 1>,<339 1>,<340 1>,
> +   <341 1>,<342 1>,<343 1>,/* cq irq (80~111) */
> +   <344 1>,<345 1>,<346 1>,<347 1>,<348 1>,
> +   <349 1>,<350 1>,<351 1>,/* cq irq (80~111) */
> +   <352 1>,<353 1>,<354 1>,<355 1>,<356 1>,
> +   <357 1>,<358 1>,<359 1>,/* cq irq (80~111) */
> +   <360 1>,<361 1>,<362 1>,<363 1>,<364 1>,
> +   <365 1>,<366 1>,<367 1>,/* cq irq (80~111) */
> +   <376 4>,/* chip fatal error irq(120) */
> +   <381 4>;/* chip fatal error irq(125) */
> +   status = "okay";
> +   };
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" 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 20/32] scsi: hisi_sas: add v1 hw interrupt init

[Rob Herring]

On Tue, Nov 17, 2015 at 10:50 AM, John Garry  wrote:
> Add code to interrupts, so now we can get a phy up
> interrupt when a disk is connected.

So I started looking at why you are using of_irq_count which drivers
shouldn't need to. In patch 5 you use it to allocate memory to store
the irq names, then use them here...


> +static const char phy_int_names[HISI_SAS_PHY_INT_NR][32] = {
> +   {"Phy Up"},
> +};
> +static irq_handler_t phy_interrupts[HISI_SAS_PHY_INT_NR] = {
> +   int_phyup_v1_hw,
> +};
> +
> +static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
> +{
> +   struct device *dev = _hba->pdev->dev;
> +   struct device_node *np = dev->of_node;
> +   char *int_names = hisi_hba->int_names;
> +   int i, j, irq, rc, idx;
> +
> +   if (!np)
> +   return -ENOENT;
> +
> +   for (i = 0; i < hisi_hba->n_phy; i++) {
> +   struct hisi_sas_phy *phy = _hba->phy[i];
> +
> +   idx = i * HISI_SAS_PHY_INT_NR;
> +   for (j = 0; j < HISI_SAS_PHY_INT_NR; j++, idx++) {
> +   irq = irq_of_parse_and_map(np, idx);

It is also preferred that drivers don't use this either. You should
use platform_get_irq() instead.

> +   if (!irq) {
> +   dev_err(dev,
> +   "irq init: fail map phy interrupt 
> %d\n",
> +   idx);
> +   return -ENOENT;
> +   }
> +
> +   (void)snprintf(_names[idx * HISI_SAS_NAME_LEN],
> +  HISI_SAS_NAME_LEN,
> +  "%s %s:%d", dev_name(dev),
> +  phy_int_names[j], i);
> +   rc = devm_request_irq(dev, irq, phy_interrupts[j], 0,
> +   _names[idx * HISI_SAS_NAME_LEN],
> +   phy);

There's no requirement for the name to match the name in the DT or
even that the name needs to be unique.

If you really want the DT names used, then just call
of_property_read_string_index() on interrupt-names here. There is no
point to copy the string.

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

Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

[Rob Herring]

On Thu, Aug 27, 2015 at 7:11 AM,  yga...@codeaurora.org wrote:
 On Tue, Aug 25, 2015 at 7:36 AM,  yga...@codeaurora.org wrote:
 On Aug 21, 2015 3:10 PM, Yaniv Gardi yga...@codeaurora.org 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 yga...@codeaurora.org

 ---
  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.

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

Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

On Sun, Aug 23, 2015 at 8:09 AM, Yaniv Gardi yga...@codeaurora.org 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 yga...@codeaurora.org

 ---
  .../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: interrupt mapping for UFS host controller IRQ
  - reg   : registers mapping

What about phy properties? No Unipro PHY block that requires setup?

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

Re: [PATCH v3 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

On Sun, Aug 30, 2015 at 3:43 AM,  yga...@codeaurora.org wrote:
 On Sun, Aug 23, 2015 at 8:09 AM, Yaniv Gardi yga...@codeaurora.org
 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 yga...@codeaurora.org

 ---
  .../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: interrupt mapping for UFS host controller IRQ
  - reg   : registers mapping

 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.

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

Re: [PATCH v1 14/15] scsi: ufs: commit descriptors before setting the doorbell

[Rob Herring]

On Tue, Aug 25, 2015 at 7:36 AM,  yga...@codeaurora.org wrote:
 On Aug 21, 2015 3:10 PM, Yaniv Gardi yga...@codeaurora.org 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 yga...@codeaurora.org

 ---
  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.

 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

Re: [PATCH v4 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

On Sun, Aug 30, 2015 at 9:52 AM, Yaniv Gardi <yga...@codeaurora.org> 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 <yga...@codeaurora.org>
>
> ---
>  Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 57 ++
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  4 +-

For the binding:

Reviewed-by: Rob Herring <r...@kernel.org>

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 = >dev;
> +   struct ufs_hba *hba;
> +
> +   /* Perform generic probe */
> +   err = ufshcd_pltfrm_init(pdev, _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?

> +   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

Re: [PATCH v7 2/8] scsi: ufs-qcom: fix compilation warning if compiled as a module

[Rob Herring]

On Wed, Sep 2, 2015 at 9:45 AM, Yaniv Gardi  wrote:
> 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:
>
>  #defineANDROID_BOOT_DEV_MAX30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> -static int get_android_boot_dev(char *str)

What is this Android crap doing in a driver to begin with?

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

Re: [PATCH v7 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

On Wed, Sep 2, 2015 at 9:45 AM, Yaniv Gardi <yga...@codeaurora.org> 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 <yga...@codeaurora.org>

Reviewed-by: Rob Herring <r...@kernel.org>

I may or may not get around to providing an ack or reviewed-by for the
rest of the series, but it's not required either.

Rob

>
> ---
>  Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 57 +
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  4 +-
>  drivers/scsi/ufs/ufs-qcom.c| 62 +-
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 98 
> ++
>  drivers/scsi/ufs/ufshcd-pltfrm.h   | 41 +
>  drivers/scsi/ufs/ufshcd.c  | 10 +++
>  drivers/scsi/ufs/ufshcd.h  |  1 +
>  7 files changed, 199 insertions(+), 74 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-qcom.txt
>  create mode 100644 drivers/scsi/ufs/ufshcd-pltfrm.h
>
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> new file mode 100644
> index 000..452e4ef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -0,0 +1,57 @@
> +* Qualcomm Technologies Inc 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-phy-qmp-20nm"
> + or "qcom,ufs-phy-qmp-14nm" according to the relevant 
> phy in use.
> +- reg   : should contain PHY register address space (mandatory),
> +- reg-names : indicates various resources passed to driver (via reg 
> proptery) by name.
> +  Required "reg-names" is "phy_mem".
> +- #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
> +- clocks   : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> + order as the clocks property. "ref_clk_src", "ref_clk",
> + "tx_iface_clk" & "rx_iface_clk" are mandatory but
> + "ref_clk_parent" is optional
> +
> +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
> +- vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
> +- vddp-ref-clk-max-microamp : specifies max. load that can be drawn from 
> this supply
> +- vddp-ref-clk-always-on : specifies if this supply needs to be kept always 
> on
> +
> +Example:
> +
> +   ufsphy1: ufsphy@0xfc597000 {
> +   compatible = "qcom,ufs-phy-qmp-20nm";
> +   reg = <0xfc597000 0x800>;
> +   reg-names = "phy_mem";
> +   #phy-cells = <0>;
> +   vdda-phy-supply = <_l4>;
> +   vdda-pll-supply = <_l12>;
> +   vdda-phy-max-microamp = <5>;
> +   vdda-pll-max-microamp = <1000>;
> +   clock-names = "ref_clk_src",
> +   "ref_clk_parent",
> +   "ref_clk",
> +   "tx_iface_clk",
> +   "rx_iface_clk";
> +   clocks = <_rpm clk_ln_bb_clk>,
> +   <_gcc clk_pcie_1_phy_ldo >,
> +   <_gcc clk_ufs_phy_ldo>,
> +   <_gcc clk_gcc_ufs_tx_cfg_clk>,
> +   <_gcc c

Re: [PATCH v5 6/8] scsi: ufs: make the UFS variant a platform device

[Rob Herring]

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 = >dev;
> +   struct ufs_hba *hba;
> +
> +   /* Perform generic probe */
> +   err = ufshcd_pltfrm_init(pdev, _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

> +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

Re: [PATCH v2 01/11] phy: exynos-ufs: add UFS PHY driver for EXYNOS SoC

[Rob Herring]

On 09/17/2015 04:45 AM, Alim Akhtar wrote:
> From: Seungwon Jeon 
> 
> This patch introduces Exynos UFS PHY driver. This driver
> supports to deal with phy calibration and power control
> according to UFS host driver's behavior.
> 
> Signed-off-by: Seungwon Jeon 
> Signed-off-by: Alim Akhtar 
> Cc: Kishon Vijay Abraham I 
> ---
>  .../devicetree/bindings/phy/samsung-phy.txt|   22 ++

It is preferred to put the binding in a separate patch.

>  drivers/phy/Kconfig|7 +
>  drivers/phy/Makefile   |1 +
>  drivers/phy/phy-exynos-ufs.c   |  262 
> 
>  drivers/phy/phy-exynos-ufs.h   |   85 +++
>  drivers/phy/phy-exynos7-ufs.h  |   89 +++
>  include/linux/phy/phy-exynos-ufs.h |  107 
>  7 files changed, 573 insertions(+)
>  create mode 100644 drivers/phy/phy-exynos-ufs.c
>  create mode 100644 drivers/phy/phy-exynos-ufs.h
>  create mode 100644 drivers/phy/phy-exynos7-ufs.h
>  create mode 100644 include/linux/phy/phy-exynos-ufs.h
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt 
> b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> index 60c6f2a..1abe2c4 100644
> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> @@ -174,3 +174,25 @@ Example:
>   usbdrdphy0 = _phy0;
>   usbdrdphy1 = _phy1;
>   };
> +
> +Samsung Exynos7 soc serise UFS PHY Controller

s/serise/series/

> +-
> +
> +UFS PHY nodes are defined to describe on-chip UFS Physical layer controllers.
> +Each UFS PHY controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains "samsung,exynos7-ufs-phy"
> +- reg : offset and length of the UFS PHY register set;
> +- reg-names : reg name(s) must be 'phy-pma';
> +- #phy-cells : must be zero
> +- samsung,syscon-phandle : a phandle to the PMU system controller, no 
> arguments

How about samsung,pmu-syscon as syscon can mean a variety of things?

> +
> +Example:
> + ufs_phy: ufs-phy@0x15571800 {
> + compatible = "samsung,exynos7-ufs-phy";
> + reg = <0x15571800 0x240>;
> + reg-names = "phy-pma";
> + samsung,syscon-phandle = <_system_controller>;
> + #phy-cells = <0>;
> + };
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 6b8dd16..7449376 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -358,4 +358,11 @@ config PHY_BRCMSTB_SATA
> Enable this to support the SATA3 PHY on 28nm Broadcom STB SoCs.
> Likely useful only with CONFIG_SATA_BRCMSTB enabled.
>  
> +config PHY_EXYNOS_UFS
> + tristate "EXYNOS SoC series UFS PHY driver"
> + depends on OF && ARCH_EXYNOS

|| COMPILE_TEST

> + select GENERIC_PHY
> + help
> +   Support for UFS PHY on Samsung EXYNOS chipsets.
> +
>  endmenu

--
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/13] scsi: ufs: Add exynos ufs platform data

[Rob Herring]

On Mon, Oct 5, 2015 at 4:06 AM, Arnd Bergmann <a...@arndb.de> wrote:
> On Monday 05 October 2015 13:44:29 Alim Akhtar wrote:
>> CCing Rob Herring,
>>
>> Hi Arnd,
>>
>> On 10/01/2015 04:59 PM, Arnd Bergmann wrote:
>> > On Thursday 01 October 2015 18:46:34 kbuild test robot wrote:
>> >> [auto build test results on v4.3-rc3 -- if it's inappropriate base, 
>> >> please ignore]
>> >>
>> >> config: x86_64-allmodconfig (attached as .config)
>> >> reproduce:
>> >>  git checkout 6e153e3bf7c68b019e987c5a0ffadebd9c7d4fbb
>> >>  # save the attached .config to linux build tree
>> >>  make ARCH=x86_64
>> >>
>> >> All error/warnings (new ones prefixed by >>):
>> >>
>> >>>> ERROR: "ufs_hba_exynos_ops" [drivers/scsi/ufs/ufshcd-pltfrm.ko] 
>> >>>> undefined!
>> >>
>> >>
>> >
>> > Ah, this seems to be a case of layering violation. It would be best to
>> > restructure the code so that the exynos driver registers a platform_driver
>> > by itself for the respective DT compatible string, and then calls
>> > into the common code from its probe function, rather than having the
>> > generic driver know about the specific backends.
>> >
>> > That approach will also make the generic driver more scalable as we
>> > add further chip-specific variations, and matches what we do in other
>> > drivers.
>> >
>>
>> Looks like some discussions on ufs variant driver probe method happened
>> here [1] few months back.
>> [1]-> https://lkml.org/lkml/2015/6/3/180
>
> Hmm, too bad we didn't catch it then, it's much more work to fix now.

What you suggested is what is being implemented[1]. It is not merged
yet. The core is a library and the platform specific parts create the
driver.

Rob

[1] https://lkml.org/lkml/2015/9/2/364
--
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] hisi_sas: use platform_get_irq()

[Rob Herring]

On Thu, Dec 10, 2015 at 9:02 AM, John Garry <john.ga...@huawei.com> wrote:
> It is preferred that drivers use platform_get_irq()
> instead of irq_of_parse_and_map(), so replace.

You may be able to stop including of_irq.h with this change. Otherwise,

Acked-by: Rob Herring <r...@kernel.org>

>
> Signed-off-by: John Garry <john.ga...@huawei.com>
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c 
> b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index 89ae31d..e907758 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1673,19 +1673,16 @@ static irq_handler_t 
> fatal_interrupts[HISI_SAS_MAX_QUEUES] = {
>
>  static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba)
>  {
> -   struct device *dev = _hba->pdev->dev;
> -   struct device_node *np = dev->of_node;
> +   struct platform_device *pdev = hisi_hba->pdev;
> +   struct device *dev = >dev;
> int i, j, irq, rc, idx;
>
> -   if (!np)
> -   return -ENOENT;
> -
> for (i = 0; i < hisi_hba->n_phy; i++) {
> struct hisi_sas_phy *phy = _hba->phy[i];
>
> idx = i * HISI_SAS_PHY_INT_NR;
> for (j = 0; j < HISI_SAS_PHY_INT_NR; j++, idx++) {
> -   irq = irq_of_parse_and_map(np, idx);
> +   irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev,
> "irq init: fail map phy interrupt 
> %d\n",
> @@ -1706,7 +1703,7 @@ static int interrupt_init_v1_hw(struct hisi_hba 
> *hisi_hba)
>
> idx = hisi_hba->n_phy * HISI_SAS_PHY_INT_NR;
> for (i = 0; i < hisi_hba->queue_count; i++, idx++) {
> -   irq = irq_of_parse_and_map(np, idx);
> +   irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev, "irq init: could not map cq interrupt 
> %d\n",
> idx);
> @@ -1724,7 +1721,7 @@ static int interrupt_init_v1_hw(struct hisi_hba 
> *hisi_hba)
>
> idx = (hisi_hba->n_phy * HISI_SAS_PHY_INT_NR) + hisi_hba->queue_count;
> for (i = 0; i < HISI_SAS_FATAL_INT_NR; i++, idx++) {
> -   irq = irq_of_parse_and_map(np, idx);
> +   irq = platform_get_irq(pdev, idx);
> if (!irq) {
> dev_err(dev, "irq init: could not map fatal interrupt 
> %d\n",
> idx);
> --
> 1.9.1
>
--
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] scsi: hisi_sas: remove dependency on of_irq_count

[Rob Herring]

On Thu, Nov 19, 2015 at 6:23 AM, John Garry <john.ga...@huawei.com> wrote:
> Originally the driver would use of_irq_count to
> calculate how much memory is required for storing the
> interrupt names, since the number of interrupt sources
> for the controller is variable.
> Since of_irq_count cannot be used by the driver, use
> fixed names.
>
> Signed-off-by: John Garry <john.ga...@huawei.com>
> Signed-off-by: Zhangfei Gao <zhangfei@linaro.org>

Acked-by: Rob Herring <r...@kernel.org>

>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h 
> b/drivers/scsi/hisi_sas/hisi_sas.h
> index 5b790c9..30a9ab9 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas.h
> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
> @@ -38,8 +38,6 @@
>  #define HISI_SAS_MAX_SSP_RESP_SZ (sizeof(struct ssp_frame_hdr) + 1024)
>  #define HISI_SAS_MAX_SMP_RESP_SZ 1028
>
> -#define HISI_SAS_NAME_LEN 32
> -
>  struct hisi_hba;
>
>  enum {
> @@ -178,7 +176,6 @@ struct hisi_hba {
>
> int queue_count;
> int queue;
> -   char*int_names;
> struct hisi_sas_slot*slot_prep;
>
> struct dma_pool *sge_page_pool;
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
> b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 1377625..2929018 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -1160,7 +1160,6 @@ static struct Scsi_Host *hisi_sas_shost_alloc(struct 
> platform_device *pdev,
> struct device *dev = >dev;
> struct device_node *np = pdev->dev.of_node;
> struct property *sas_addr_prop;
> -   int num;
>
> shost = scsi_host_alloc(_sas_sht, sizeof(*hisi_hba));
> if (!shost)
> @@ -1197,13 +1196,6 @@ static struct Scsi_Host *hisi_sas_shost_alloc(struct 
> platform_device *pdev,
> if (of_property_read_u32(np, "queue-count", _hba->queue_count))
> goto err_out;
>
> -   num = of_irq_count(np);
> -   hisi_hba->int_names = devm_kcalloc(dev, num,
> -  HISI_SAS_NAME_LEN,
> -  GFP_KERNEL);
> -   if (!hisi_hba->int_names)
> -   goto err_out;
> -
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> hisi_hba->regs = devm_ioremap_resource(dev, res);
> if (IS_ERR(hisi_hba->regs))
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c 
> b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index e29b7c7..0ed2f92 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1660,18 +1660,6 @@ static irqreturn_t fatal_axi_int_v1_hw(int irq, void 
> *p)
> return IRQ_HANDLED;
>  }
>
> -static const char phy_int_names[HISI_SAS_PHY_INT_NR][32] = {
> -   {"Bcast"},
> -   {"Phy Up"},
> -   {"Abnormal"},
> -};
> -
> -static const char cq_int_name[32] = "cq";
> -static const char fatal_int_name[HISI_SAS_FATAL_INT_NR][32] = {
> -   "fatal ecc",
> -   "fatal axi"
> -};
> -
>  static irq_handler_t phy_interrupts[HISI_SAS_PHY_INT_NR] = {
> int_bcast_v1_hw,
> int_phyup_v1_hw,
> @@ -1687,7 +1675,6 @@ static int interrupt_init_v1_hw(struct hisi_hba 
> *hisi_hba)
>  {
> struct device *dev = _hba->pdev->dev;
> struct device_node *np = dev->of_node;
> -   char *int_names = hisi_hba->int_names;
> int i, j, irq, rc, idx;
>
> if (!np)
> @@ -1706,13 +1693,8 @@ static int interrupt_init_v1_hw(struct hisi_hba 
> *hisi_hba)
> return -ENOENT;
> }
>
> -   (void)snprintf(_names[idx * HISI_SAS_NAME_LEN],
> -  HISI_SAS_NAME_LEN,
> -  "%s %s:%d", dev_name(dev),
> -  phy_int_names[j], i);
> rc = devm_request_irq(dev, irq, phy_interrupts[j], 0,
> -   _names[idx * HISI_SAS_NAME_LEN],
> -   phy);
> + DRV_NAME " phy", phy);
> if (rc) {
> dev_err(dev, "irq init: could not request "
> "phy interrupt %d, rc=%d\n",
> @@ -1730,12 +1712,9 @@ static int interrupt_init_v1_hw(struct hisi_hba 
> *hisi_hba)
> idx);
> return -ENOENT;
&

Re: [v4 01/14] scsi: ufs-qcom: add number of lanes per direction

[Rob Herring]

On Thu, Feb 04, 2016 at 12:36:07PM +0200, Yaniv Gardi wrote:
> Different platform may have different number of lanes
> for the UFS link.
> Add parameter to device tree specifying how many lanes
> should be configured for the UFS link.
> 
> Signed-off-by: Gilad Broner <gbro...@codeaurora.org>
> Signed-off-by: Yaniv Gardi <yga...@codeaurora.org>
> 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  3 ++
>  drivers/scsi/ufs/ufs-qcom.c| 39 
> --
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 19 +++
>  drivers/scsi/ufs/ufshcd.c  |  1 +
>  drivers/scsi/ufs/ufshcd.h  |  2 ++
>  5 files changed, 47 insertions(+), 17 deletions(-)

Acked-by: Rob Herring <r...@kernel.org>
--
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 3/3] add support for DWC UFS Host Controller

[Rob Herring]

On Thu, Feb 11, 2016 at 12:13:06PM +, Joao Pinto wrote:
> This patch has the goal to add support for DesignWare UFS Controller
> specific operations and to add specific platform and pci drivers.
> 
> Signed-off-by: Joao Pinto 
> ---
> Changes v6->v7 (Arnd Bergmann):
> - Changed DT node name (to ufs only) and the memory address (to 0xd00)
> - Removed CONFIG_PM from the PCI glue driver (pm.h already does this)
> - No other changes are necessary in ufshcd.c because of the link up notify
>  function usage (it is simpler now)
> - Removed the PHY mentioning since the Test Chip is not a real PHY for real
>  world usage, since it is a test chip for prototyping with a very specific
>  usage
> - Added again the Test Chip 20-bit option
> Changes v5->v6:
> - Patch bad format fixed
> Changes v4->v5 (Akinobu Mita):
> - All functions used only locally in ufshcd-dwc are now declared as static
> - ufshcd_dwc_configuration() was removed in ufshcd-dwc and a notify
>  function (ufshcd_dwc_link_startup_notify) was created to deal with the
>  DWC specific init routines
> - 20-bit RMMI option was removed from Kconfig. Now if MPHY TC is selected
>  and 40-bit is not then it assumes a 20-bit config
> Changes v3->v4 (Arnd Bergmann and Mark Rutland):
> - SCSI_UFS_DWC_HOOKS is now silent and selected by the SCSI_UFS_DWC_PLAT
>  or SCSI_UFS_DWC_PCI
> - Compatibility string has the ufs core version for info purposes since
>  the driver is capable of getting the controller version from its 
>  registers
> - Created ufs-dwc-pci glue driver with specific DWC data
> - MPHY configuration remains in the ufshcd-dwc since it is unipro
>  attribute writting only not following the a linux phy framework logic
> Changes v2->v3 (Julian Calaby):
> - Implement a common DWC code to be used by the platform and pci glue
>  drivers
> - Synopsys ID & Class added to the existing pci driver and specific DWC
>  was also added to the pci driver
> Changes v1->v2 (Akinobu Mita):
> - Implement a platform driver that uses the existing UFS core driver
> - Add DWC specific code to the existing UFS core driver
> 
>  Documentation/devicetree/bindings/ufs/ufs-dwc.txt |  17 +
>  MAINTAINERS   |   6 +
>  drivers/scsi/ufs/Kconfig  |  51 ++
>  drivers/scsi/ufs/Makefile |   3 +
>  drivers/scsi/ufs/ufs-dwc-pci.c| 172 +
>  drivers/scsi/ufs/ufs-dwc.c| 102 +++
>  drivers/scsi/ufs/ufshcd-dwc.c | 739 
> ++
>  drivers/scsi/ufs/ufshcd-dwc.h |  18 +
>  drivers/scsi/ufs/ufshci-dwc.h |  42 ++
>  drivers/scsi/ufs/unipro.h |  39 ++
>  10 files changed, 1189 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-dwc.txt
>  create mode 100644 drivers/scsi/ufs/ufs-dwc-pci.c
>  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..3cbfd77
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt
> @@ -0,0 +1,17 @@
> +* 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 string ("snps,ufshcd-1.0", "snps,ufshcd-1.1"
> +  or "snps,ufshcd-2.0")

History has taught us this needs to have a vendor specific compatible 
string as well. Don't necessarily have to define it now, but just state 
a vendor string is needed too.

> +- reg   : 
> +- interrupts: 
> +
> +Example:
> + ufs@0xd000 {
> + compatible = "snps,ufshcd-2.0";
> + reg = < 0xd000 0x1 >;
> + interrupts = < 24 >;
> + };
--
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 3/3] add support for DWC UFS Host Controller

[Rob Herring]

On Fri, Feb 12, 2016 at 11:52 AM, Joao Pinto <joao.pi...@synopsys.com> wrote:
> Hi Rob,
>
> On 2/12/2016 4:36 PM, Rob Herring wrote:
>> On Thu, Feb 11, 2016 at 12:13:06PM +, Joao Pinto wrote:
>>> +Required properties:
>>> +- compatible: compatible string ("snps,ufshcd-1.0", 
>>> "snps,ufshcd-1.1"
>>> +  or "snps,ufshcd-2.0")
>>
>> History has taught us this needs to have a vendor specific compatible
>> string as well. Don't necessarily have to define it now, but just state
>> a vendor string is needed too.
>
> The compatibility string already as the "snps" which is the acronym for 
> Synopsys
> which is the HW Controller vendor. Isn't this enough?

Go look drivers for any licensed IP: DW PCIe, DW GMAC, USB EHCI
controllers (pretty much all licensed IP), etc. They all have
variations either from versions of the IP, configuration of the IP,
"enhancements" by the licensee, integration quirks, or all of the
above.

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

Re: [PATCH v2 01/23] devicetree: bindings: hisi_sas: add v2 HW bindings

[Rob Herring]

On Tue, Jan 26, 2016 at 01:21:50AM +0800, j00310691 wrote:
> From: John Garry <john.ga...@huawei.com>
> 
> Add the dt bindings for HiSi SAS controller v2 HW.
> 
> The main difference in the controller from dt perspective
> is interrupts. The v2 controller does not have dedicated
> fatal and broadcast interrupts - they are multiplexed on
> the channel interrupt.
> 
> Each SAS v2 controller can issue upto 64 commands
> (or connection requests) on the system bus without waiting
> for a response - this is know as am-max-transmissions.
> In hip06, sas controller #1 has a limitation that it has to
> limit am-max-transmissions to 32 - this limitation is due
> to chip system bus design. It is not anticipated that any
> future chip incorporating v2 controller will have such a
> limitation.
> 
> Signed-off-by: John Garry <john.ga...@huawei.com>
> ---
>  .../devicetree/bindings/scsi/hisilicon-sas.txt  | 21 
> ++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <r...@kernel.org>
--
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 v8 2/3] added UFS 2.0 capabilities

[Rob Herring]

On Mon, Feb 15, 2016 at 03:25:12PM +, Joao Pinto wrote:
> Adding UFS 2.0 support to the UFS core driver.
> 
> Signed-off-by: Joao Pinto <jpi...@synopsys.com>
> ---
> Changes v7->v8:
> - Added "jedec, ufs-2.0" to the ufschd-platform compatibility strings
> Changes v0->v7:
> - Nothing changed (just to keep up with patch set version).
> 
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  4 +--

Acked-by: Rob Herring <r...@kernel.org>

>  drivers/scsi/ufs/ufshcd.c  | 29 
> +++---
>  drivers/scsi/ufs/ufshci.h  |  1 +
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
--
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 v8 3/3] add support for DWC UFS Host Controller

[Rob Herring]

On Mon, Feb 15, 2016 at 03:25:13PM +, Joao Pinto wrote:
> This patch has the goal to add support for DesignWare UFS Controller
> specific operations and to add specific platform and pci drivers.
> 
> Signed-off-by: Joao Pinto 
> ---
> Changes v7->v8 (Akinobu Mita):
> - DME sets were simplified for easier reading
> - CLK DIV default values definitions names were changed to match the
>  register's name
> - New line added to dev_err and dev_info statements
> Changes v6->v7 (Arnd Bergmann):
> - Changed DT node name (to ufs only) and the memory address (to 0xd00)
> - Removed CONFIG_PM from the PCI glue driver (pm.h already does this)
> - No other changes are necessary in ufshcd.c because of the link up notify
>  function usage (it is simpler now)
> - Removed the PHY mentioning since the Test Chip is not a real PHY for real
>  world usage, since it is a test chip for prototyping with a very specific
>  usage
> - Added again the Test Chip 20-bit option
> Changes v5->v6:
> - Patch bad format fixed
> Changes v4->v5 (Akinobu Mita):
> - All functions used only locally in ufshcd-dwc are now declared as static
> - ufshcd_dwc_configuration() was removed in ufshcd-dwc and a notify
>  function (ufshcd_dwc_link_startup_notify) was created to deal with the
>  DWC specific init routines
> - 20-bit RMMI option was removed from Kconfig. Now if MPHY TC is selected
>  and 40-bit is not then it assumes a 20-bit config
> Changes v3->v4 (Arnd Bergmann and Mark Rutland):
> - SCSI_UFS_DWC_HOOKS is now silent and selected by the SCSI_UFS_DWC_PLAT
>  or SCSI_UFS_DWC_PCI
> - Compatibility string has the ufs core version for info purposes since
>  the driver is capable of getting the controller version from its 
>  registers
> - Created ufs-dwc-pci glue driver with specific DWC data
> - MPHY configuration remains in the ufshcd-dwc since it is unipro
>  attribute writting only not following the a linux phy framework logic
> Changes v2->v3 (Julian Calaby):
> - Implement a common DWC code to be used by the platform and pci glue
>  drivers
> - Synopsys ID & Class added to the existing pci driver and specific DWC
>  was also added to the pci driver
> Changes v1->v2 (Akinobu Mita):
> - Implement a platform driver that uses the existing UFS core driver
> - Add DWC specific code to the existing UFS core driver
> 
>  Documentation/devicetree/bindings/ufs/ufs-dwc.txt |  19 +
>  MAINTAINERS   |   6 +
>  drivers/scsi/ufs/Kconfig  |  51 +++
>  drivers/scsi/ufs/Makefile |   3 +
>  drivers/scsi/ufs/ufs-dwc-pci.c| 172 +
>  drivers/scsi/ufs/ufs-dwc.c|  96 +
>  drivers/scsi/ufs/ufshcd-dwc.c | 431 
> ++
>  drivers/scsi/ufs/ufshcd-dwc.h |  18 +
>  drivers/scsi/ufs/ufshci-dwc.h |  42 +++
>  drivers/scsi/ufs/unipro.h |  39 ++
>  10 files changed, 877 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-dwc.txt
>  create mode 100644 drivers/scsi/ufs/ufs-dwc-pci.c
>  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..59e9822
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt
> @@ -0,0 +1,19 @@
> +* 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 must contain "snps,ufshcd-dwc" and 
> should
> +   also contain the JEDEC version of the controller:
> + "jedec,ufs-1.1"
> + "jedec,ufs-2.0"

As I said before, this needs to mention also requiring an SoC specific 
compatible.

> +- reg   : 
> +- interrupts: 
> +
> +Example:
> + ufs@0xd000 {

Drop '0x'

> + compatible = "jedec,ufs-2.0", "snps,dwc-ufshcd";

This should be reversed. Most specific first.

> + reg = < 0xd000 0x1 >;
> + interrupts = < 24 >;
> + };

--
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 v12 8/9] add TC G210 platform driver

[Rob Herring]

On Mon, Apr 04, 2016 at 11:48:23AM +0100, Joao Pinto wrote:
> 
> Hi Rob,
> 
> On 4/4/2016 6:15 AM, Rob Herring wrote:
> > On Thu, Mar 31, 2016 at 07:57:21PM +0100, Joao Pinto wrote:
> >> This patch adds a glue platform driver for the Synopsys G210 Test Chip.
> >>
> >> Signed-off-by: Joao Pinto <jpi...@synopsys.com>
> >> ---
> 
> [snip]
> 
> >> +
> >> +Required properties:
> >> +- compatible  : compatible list must contain the PHY type & version:
> >> +  "snps, g210-tc-6.00-20bit"
> >> +  "snps, g210-tc-6.00-40bit"
> > Remove the space  ^
> > 
> >> +complemented with the Controller IP version:
> >> +  "snps, dwc-ufshcd-1.40a"
> > 
> > ditto
> 
> Ok, will do that!
> 
> > 
> > Combining the phy and controller compatible strings is a bit strange. 
> > Generally, they would be separate nodes using the common phy binding.
> > 
> 
> Correct, but in this case is just the compatibility string is just to tell the
> dw ufs host that it has a 40-bit or a 20-bit test chip connected. The Test 
> chip
> is initialized by a unipro command sequence and there is no more ops related 
> to it.

Okay. In that case, I think it should be a separate property unless the 
controller h/w is synthesized for one or the other.

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

Re: [PATCH v10 2/6] added UFS 2.0 capabilities

[Rob Herring]

On Fri, Mar 04, 2016 at 05:22:15PM +, Joao Pinto wrote:
> Adding UFS 2.0 support to the UFS core driver.
> 
> Signed-off-by: Joao Pinto <jpi...@synopsys.com>
> ---
> Changes v8->v10:
> - Nothing changed (just to keep up with patch set version).
> Changes v7->v8:
> - Added "jedec, ufs-2.0" to the ufschd-platform compatibility strings
> Changes v0->v7:
> - Nothing changed (just to keep up with patch set version).
> 
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  4 +--
>  drivers/scsi/ufs/ufshcd.c  | 29 
> +++---
>  drivers/scsi/ufs/ufshci.h  |  1 +
>  3 files changed, 28 insertions(+), 6 deletions(-)

Acked-by: Rob Herring <r...@kernel.org>
--
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: [RESEND] [PATCH v11 2/6] added UFS 2.0 capabilities

[Rob Herring]

On Thu, Mar 17, 2016 at 7:39 AM, Joao Pinto <joao.pi...@synopsys.com> wrote:
> Adding UFS 2.0 support to the UFS core driver.
>
> Signed-off-by: Joao Pinto <jpi...@synopsys.com>
> Acked-by: Arnd Bergmann <a...@arndb.de>
> Acked-by: Rob Herring <r...@kernel.org>

You don't need to resend just to add acks. The maintainer will do that.

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

Re: [PATCH v11 5/6] add TC G210 platform driver

[Rob Herring]

On Mon, Mar 07, 2016 at 12:09:20PM +, Joao Pinto wrote:
> This patch adds a glue platform driver for the Synopsys G210 Test Chip.
> 
> Signed-off-by: Joao Pinto 
> ---
> Changes v10->v11 (Arnd Bergmann):
> - vops structs are now passed in .data
> Changes v0->v10:
> - This patch only appeared in v10
> 
>  .../devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt |  26 +
>  drivers/scsi/ufs/Kconfig   |   9 ++
>  drivers/scsi/ufs/Makefile  |   1 +
>  drivers/scsi/ufs/tc-dwc-g210-pltfrm.c  | 113 
> +
>  4 files changed, 149 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt
>  create mode 100644 drivers/scsi/ufs/tc-dwc-g210-pltfrm.c
> 
> diff --git a/Documentation/devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt
> new file mode 100644
> index 000..6ec9647
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/tc-dwc-g210-pltfrm.txt
> @@ -0,0 +1,26 @@
> +* Universal Flash Storage (UFS) DesignWare Host Controller
> +
> +DWC_UFS nodes are defined to describe on-chip UFS host controllers and MPHY.
> +Each UFS controller instance should have its own node.
> +
> +Required properties:
> +- compatible : compatible list must contain the PHY type & version:
> + "snps, g210-tc-6.00-20bit"
> + "snps, g210-tc-6.00-40bit"
drop the spaces   ^


> +   complemented with the Controller IP version:
> + "snps, dwc-ufshcd-1.40a"

ditto

> +   complemented with the JEDEC version:
> + "jedec,ufs-1.1"
> + "jedec,ufs-2.0"
> +
> +- reg: 
> +- interrupts : 
> +
> +Example for a setup using a 1.40a DWC Controller with a 6.00 G210 40-bit TC:
> + dwc_ufs@d000 {
> + compatible = "snps, g210-tc-6.00-40bit",
> +  "snps, dwc-ufshcd-1.40a",

ditto

> +  "jedec,ufs-2.0";
> + reg = < 0xd000 0x1 >;
> + interrupts = < 24 >;
> + };
--
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 01/15] scsi: ufs-qcom: add number of lanes per direction

[Rob Herring]

On Sun, Feb 28, 2016 at 03:32:33PM +0200, Yaniv Gardi wrote:
> Different platform may have different number of lanes
> for the UFS link.
> Add parameter to device tree specifying how many lanes
> should be configured for the UFS link.
> 
> Signed-off-by: Gilad Broner <gbro...@codeaurora.org>
> Signed-off-by: Yaniv Gardi <yga...@codeaurora.org>
> 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  3 ++

Acked-by: Rob Herring <r...@kernel.org>

>  drivers/scsi/ufs/ufs-qcom.c| 39 
> --
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 19 +++
>  drivers/scsi/ufs/ufshcd.c  |  1 +
>  drivers/scsi/ufs/ufshcd.h  |  2 ++
>  5 files changed, 47 insertions(+), 17 deletions(-)
--
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 00/35] treewide trivial patches converting pr_warning to pr_warn

[Rob Herring]

On Fri, Feb 17, 2017 at 1:11 AM, Joe Perches  wrote:
> There are ~4300 uses of pr_warn and ~250 uses of the older
> pr_warning in the kernel source tree.
>
> Make the use of pr_warn consistent across all kernel files.
>
> This excludes all files in tools/ as there is a separate
> define pr_warning for that directory tree and pr_warn is
> not used in tools/.
>
> Done with 'sed s/\bpr_warning\b/pr_warn/' and some emacsing.
>
> Miscellanea:
>
> o Coalesce formats and realign arguments
>
> Some files not compiled - no cross-compilers
>
> Joe Perches (35):
>   alpha: Convert remaining uses of pr_warning to pr_warn
>   ARM: ep93xx: Convert remaining uses of pr_warning to pr_warn
>   arm64: Convert remaining uses of pr_warning to pr_warn
>   arch/blackfin: Convert remaining uses of pr_warning to pr_warn
>   ia64: Convert remaining use of pr_warning to pr_warn
>   powerpc: Convert remaining uses of pr_warning to pr_warn
>   sh: Convert remaining uses of pr_warning to pr_warn
>   sparc: Convert remaining use of pr_warning to pr_warn
>   x86: Convert remaining uses of pr_warning to pr_warn
>   drivers/acpi: Convert remaining uses of pr_warning to pr_warn
>   block/drbd: Convert remaining uses of pr_warning to pr_warn
>   gdrom: Convert remaining uses of pr_warning to pr_warn
>   drivers/char: Convert remaining use of pr_warning to pr_warn
>   clocksource: Convert remaining use of pr_warning to pr_warn
>   drivers/crypto: Convert remaining uses of pr_warning to pr_warn
>   fmc: Convert remaining use of pr_warning to pr_warn
>   drivers/gpu: Convert remaining uses of pr_warning to pr_warn
>   drivers/ide: Convert remaining uses of pr_warning to pr_warn
>   drivers/input: Convert remaining uses of pr_warning to pr_warn
>   drivers/isdn: Convert remaining uses of pr_warning to pr_warn
>   drivers/macintosh: Convert remaining uses of pr_warning to pr_warn
>   drivers/media: Convert remaining use of pr_warning to pr_warn
>   drivers/mfd: Convert remaining uses of pr_warning to pr_warn
>   drivers/mtd: Convert remaining uses of pr_warning to pr_warn
>   drivers/of: Convert remaining uses of pr_warning to pr_warn
>   drivers/oprofile: Convert remaining uses of pr_warning to pr_warn
>   drivers/platform: Convert remaining uses of pr_warning to pr_warn
>   drivers/rapidio: Convert remaining use of pr_warning to pr_warn
>   drivers/scsi: Convert remaining use of pr_warning to pr_warn
>   drivers/sh: Convert remaining use of pr_warning to pr_warn
>   drivers/tty: Convert remaining uses of pr_warning to pr_warn
>   drivers/video: Convert remaining uses of pr_warning to pr_warn
>   kernel/trace: Convert remaining uses of pr_warning to pr_warn
>   lib: Convert remaining uses of pr_warning to pr_warn
>   sound/soc: Convert remaining uses of pr_warning to pr_warn

Where's the removal of pr_warning so we don't have more sneak in?

Rob

Re: [PATCH v2 1/3] devicetree: bindings: scsi: hisi_sas add hip07 support

[Rob Herring]

On Tue, Oct 04, 2016 at 07:11:09PM +0800, John Garry wrote:
> Add support for hip07 chipset to hisi_sas controller.
> 
> Chipset hip07 has v2 hw.
> 
> Signed-off-by: John Garry <john.ga...@huawei.com>
> Signed-off-by: Xiang Chen <chenxian...@hisilicon.com>
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <r...@kernel.org>
--
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 1/3] devicetree: bindings: scsi: hisi_sas hip07 support

[Rob Herring]

On Tue, Sep 20, 2016 at 06:48:58PM +0800, John Garry wrote:
> Add support for hip07 chipset to bindings.
> 
> Chipset hip07 has v2 hw.
> 
> The sas-v2 quirk amt is expanded to cover hip07.
> 
> Signed-off-by: John Garry 
> Signed-off-by: Xiang Chen 
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> index bf2411f..e604527 100644
> --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -6,6 +6,7 @@ Main node required properties:
>- compatible : value should be as follows:
>   (a) "hisilicon,hip05-sas-v1" for v1 hw in hip05 chipset
>   (b) "hisilicon,hip06-sas-v2" for v2 hw in hip06 chipset
> + (c) "hisilicon,hip07-sas-v2" for v2 hw in hip07 chipset
>- sas-addr : array of 8 bytes for host SAS address
>- reg : Address and length of the SAS register
>- hisilicon,sas-syscon: phandle of syscon used for sas control
> @@ -47,7 +48,7 @@ Main node required properties:
>   increasing order.
>  
>  Optional main node properties:
> - - hip06-sas-v2-quirk-amt : when set, indicates that the v2 controller has 
> the
> + - hip06/7-sas-v2-quirk-amt : when set, indicates that the v2 controller has 
> the

Just use the existing property name for hip07.

>   "am-max-transmissions" limitation.
>  
>  Example:
> -- 
> 1.9.1
> 
--
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/12] scsi: ufs: add option to change default UFS power management level

[Rob Herring]

On Mon, Dec 12, 2016 at 04:54:20PM -0800, Subhash Jadavani wrote:
> 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).
> 
> Reviewed-by: Yaniv Gardi 
> Signed-off-by: Subhash Jadavani 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  | 10 ++
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 14 
>  drivers/scsi/ufs/ufshcd.c  | 39 
> ++
>  drivers/scsi/ufs/ufshcd.h  |  4 +--
>  4 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index a99ed55..c3836c5 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -41,6 +41,14 @@ Optional properties:
>  -lanes-per-direction : number of lanes available per direction - either 1 or 
> 2.
> Note that it is assume same number of lanes is used 
> both
> directions at once. If not specified, default is 2 
> lanes per direction.
> +- rpm-level  : UFS Runtime power management level. Following PM 
> levels are supported:
> +   0 - Both UFS device and Link in active state (Highest 
> power consumption)
> +   1 - UFS device in active state but Link in Hibern8 
> state
> +   2 - UFS device in Sleep state but Link in active state
> +   3 - UFS device in Sleep state and Link in hibern8 
> state (default PM level)
> +   4 - UFS device in Power-down state and Link in 
> Hibern8 state
> +   5 - UFS device in Power-down state and Link in OFF 
> state (Lowest power consumption)
> +- spm-level  : UFS System power management level. Allowed PM levels 
> are same as rpm-level.

This looks like you are putting policy for Linux into DT.

What I would expect to see here is disabling of states that don't work 
due to some h/w limitation. Otherwise, it is a user decision for what 
modes to go into. Also, I think link and device states should be 
separate.

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

Re: [PATCH v1 07/12] scsi: ufs: add option to change default UFS power management level

[Rob Herring]

On Tue, Dec 13, 2016 at 2:16 PM, Subhash Jadavani
<subha...@codeaurora.org> wrote:
> On 2016-12-13 12:04, Rob Herring wrote:
>>
>> On Mon, Dec 12, 2016 at 04:54:20PM -0800, Subhash Jadavani wrote:
>>>
>>> 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).
>>>
>>> Reviewed-by: Yaniv Gardi <yga...@codeaurora.org>
>>> Signed-off-by: Subhash Jadavani <subha...@codeaurora.org>
>>> ---
>>>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  | 10 ++
>>>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 14 
>>>  drivers/scsi/ufs/ufshcd.c  | 39
>>> ++
>>>  drivers/scsi/ufs/ufshcd.h  |  4 +--
>>>  4 files changed, 65 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> index a99ed55..c3836c5 100644
>>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> @@ -41,6 +41,14 @@ Optional properties:
>>>  -lanes-per-direction   : number of lanes available per direction -
>>> either 1 or 2.
>>>   Note that it is assume same number of lanes is
>>> used both
>>>   directions at once. If not specified, default
>>> is 2 lanes per direction.
>>> +- rpm-level: UFS Runtime power management level. Following
>>> PM levels are supported:
>>> + 0 - Both UFS device and Link in active state
>>> (Highest power consumption)
>>> + 1 - UFS device in active state but Link in
>>> Hibern8 state
>>> + 2 - UFS device in Sleep state but Link in
>>> active state
>>> + 3 - UFS device in Sleep state and Link in
>>> hibern8 state (default PM level)
>>> + 4 - UFS device in Power-down state and Link in
>>> Hibern8 state
>>> + 5 - UFS device in Power-down state and Link in
>>> OFF state (Lowest power consumption)
>>> +- spm-level: UFS System power management level. Allowed PM
>>> levels are same as rpm-level.
>>
>>
>> This looks like you are putting policy for Linux into DT.
>>
>> What I would expect to see here is disabling of states that don't work
>> due to some h/w limitation. Otherwise, it is a user decision for what
>> modes to go into. Also, I think link and device states should be
>> separate.
>
>
> Yes, generally default level (3) is good enough (and recommended) for all
> platforms and most likely user is only expected to change this if they see
> issues (most H/W) on their platform or they want even more aggressive power
> state (level-4 or level-5) and ready to take the performance hit associated
> with resume latencies.

What latencies can be tolerated is going to depend on the application
and could vary while running, so putting in DT doesn't make sense. I
would break down settings like this:

broken h/w -> DT
user tuning/config -> sysfs
sensible defaults -> driver

> Also, I think it is better to keep Link and device states tied, one reason
> is that we can't keep device in sleep/active state when Link is in OFF
> state.

The driver can tie the states to together if needed. Just document
what's broken in DT and let the driver make decisions.

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

[PATCH] scsi: Convert to using %pOF instead of full_name

[Rob Herring]

Now that we have a custom printf format specifier, convert users of
full_name to use %pOF instead. This is preparation to remove storing
of the full path string for each node.

Signed-off-by: Rob Herring <r...@kernel.org>
Cc: "James E.J. Bottomley" <j...@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.peter...@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/mac53c94.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mac53c94.c b/drivers/scsi/mac53c94.c
index a6682c508c4c..8c4d3003b68b 100644
--- a/drivers/scsi/mac53c94.c
+++ b/drivers/scsi/mac53c94.c
@@ -448,15 +448,14 @@ static int mac53c94_probe(struct macio_dev *mdev, const 
struct of_device_id *mat
ioremap(macio_resource_start(mdev, 1), 0x1000);
state->dmaintr = macio_irq(mdev, 1);
if (state->regs == NULL || state->dma == NULL) {
-   printk(KERN_ERR "mac53c94: ioremap failed for %s\n",
-  node->full_name);
+   printk(KERN_ERR "mac53c94: ioremap failed for %pOF\n", node);
goto out_free;
}

clkprop = of_get_property(node, "clock-frequency", );
if (clkprop == NULL || proplen != sizeof(int)) {
-   printk(KERN_ERR "%s: can't get clock frequency, "
-  "assuming 25MHz\n", node->full_name);
+   printk(KERN_ERR "%pOF: can't get clock frequency, "
+  "assuming 25MHz\n", node);
state->clk_freq = 2500;
} else
state->clk_freq = *(int *)clkprop;
@@ -469,7 +468,7 @@ static int mac53c94_probe(struct macio_dev *mdev, const 
struct of_device_id *mat
sizeof(struct dbdma_cmd), GFP_KERNEL);
if (dma_cmd_space == 0) {
printk(KERN_ERR "mac53c94: couldn't allocate dma "
-  "command space for %s\n", node->full_name);
+  "command space for %pOF\n", node);
rc = -ENOMEM;
goto out_free;
}
@@ -481,8 +480,8 @@ static int mac53c94_probe(struct macio_dev *mdev, const 
struct of_device_id *mat
mac53c94_init(state);

if (request_irq(state->intr, do_mac53c94_interrupt, 0, "53C94",state)) {
-   printk(KERN_ERR "mac53C94: can't get irq %d for %s\n",
-  state->intr, node->full_name);
+   printk(KERN_ERR "mac53C94: can't get irq %d for %pOF\n",
+  state->intr, node);
goto out_free_dma;
}

--
2.11.0

Re: [PATCH v3 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Tue, Aug 29, 2017 at 04:41:13PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon
> 
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 35 
> ++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..cfc84c821d50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,35 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs" for hisi ufs host controller
> +  present on Hi3660 chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> +   order as the clocks property. "clk_ref", "clk_phy" is 
> optional
> +
> +Optional properties for board device:
> +- reset-gpio : specifies to reset devices

reset-gpios

Really, this is should be in a sub node representing the device. It's 
fine if this is it, but if we start adding power supplies and other 
properties it should be a separate node.

> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "jedec,ufs-1.1", "hisilicon,hi3660-ufs";

Need to reverse the order here. Most specific first.

> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + clocks = <_ctrl HI3660_CLK_GATE_UFSIO_REF>,
> + <_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
> + clock-names = "clk_ref", "clk_phy";
> + freq-table-hz = <0 0>, <0 0>;
> + reset-gpio = < 1 0>;
> + }
> -- 
> 2.11.0
> 

Re: [PATCH v4 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Fri, Oct 13, 2017 at 03:49:33PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 47 
> ++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..ee114a65143d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,47 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs" for hisi ufs host controller
> +  present on Hi3660 chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> +   order as the clocks property. "clk_ref", "clk_phy" is 
> optional
> +- resets: reset node register, one reset the clk and the other 
> reset the controller
> +- reset-names   : describe reset node register
> +
> +Optional properties for board device:
> +- reset-gpio : specifies to reset devices

Should be "reset-gpios".

> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + clocks = <_ctrl HI3660_CLK_GATE_UFSIO_REF>,
> + <_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
> + clock-names = "clk_ref", "clk_phy";
> + freq-table-hz = <0 0>, <0 0>;
> + /* offset: 0x84; bit: 12 */
> + /* offset: 0x84; bit: 7  */
> + resets = <_rst 0x84 12>,
> + <_rst 0x84 7>;
> + reset-names = "rst", "assert";
> + };
> +
> +  {
> + reset-gpio = < 1 0>;
> + status = "okay";

Don't show status or the chip/board split in examples.

> + };
> +
> -- 
> 2.11.0
> 

Re: [PATCH v6 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Thu, Dec 07, 2017 at 06:20:23PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei 
> ---

Version history?

>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 38 
> ++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..73e10698960e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,38 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs", "jedec,ufs-1.1" 
> for hisi ufs
> + host controller present on Hi36xx 
> chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> + order as the clocks property. 
> "ref_clk", "phy_clk" is optional
> +- resets: reset node register, one reset the clk and the other 
> reset the controller
> +- reset-names   : describe reset node register
> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + clocks = <_ctrl HI3660_CLK_GATE_UFSIO_REF>,
> + <_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
> + clock-names = "ref_clk", "phy_clk";
> + freq-table-hz = <0 0>, <0 0>;

? Not documented.

> + /* offset: 0x84; bit: 12 */
> + /* offset: 0x84; bit: 7  */
> + resets = <_rst 0x84 12>,
> + <_rst 0x84 7>;
> + reset-names = "rst", "assert";
> + };
> -- 
> 2.15.0
> 

Re: 答复: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Tue, Apr 24, 2018 at 8:54 AM, liwei (CM) <liwei...@huawei.com> wrote:
> Hi, Rob
>
> Thanks for your patience.
>
> Hi, Arnd
>
> From Rob's suggestion, we have to list the properties node in ufs-hisi.txt 
> bingings even if documented in the common binding.
>
> -邮件原件-
> 发件人: Rob Herring [mailto:r...@kernel.org]
> 发送时间: 2018年4月24日 20:58
> 收件人: liwei (CM)
> 抄送: mark.rutl...@arm.com; catalin.mari...@arm.com; will.dea...@arm.com; 
> vinholika...@gmail.com; j...@linux.vnet.ibm.com; martin.peter...@oracle.com; 
> khil...@baylibre.com; a...@arndb.de; gregory.clem...@free-electrons.com; 
> thomas.petazz...@free-electrons.com; yamada.masah...@socionext.com; 
> riku.voi...@linaro.org; tred...@nvidia.com; k...@kernel.org; 
> devicet...@vger.kernel.org; linux-ker...@vger.kernel.org; 
> linux-arm-ker...@lists.infradead.org; linux-scsi@vger.kernel.org; 
> zangleigang; Gengjianfeng; guodong...@linaro.org
> 主题: Re: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs
>
> On Tue, Apr 17, 2018 at 10:08:11PM +0800, Li Wei wrote:
>> add ufs node document for Hisilicon.
>>
>> Signed-off-by: Li Wei <liwei...@huawei.com>
>> ---
>>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 29 
>> ++
>>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  | 10 +---
>>  2 files changed, 36 insertions(+), 3 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
>> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> new file mode 100644
>> index ..d49ab7d8f31d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
>> @@ -0,0 +1,29 @@
>> +* Hisilicon Universal Flash Storage (UFS) Host Controller
>> +
>> +UFS nodes are defined to describe on-chip UFS hardware macro.
>> +Each UFS Host Controller should have its own node.
>> +
>> +Required properties:
>> +- compatible: compatible list, contains one of the following -
>> + "hisilicon,hi3660-ufs", 
>> "jedec,ufs-1.1" for hisi ufs
>> + host controller present on Hi36xx 
>> chipset.
>> +- reg   : should contain UFS register address space & UFS SYS 
>> CTRL register address,
>> +- interrupt-parent  : interrupt device
>> +- interrupts: interrupt number
>> +- resets: reset node register, the "arst" corresponds to reset 
>> the APB/AXI bus.
>
> arst belongs in reset-names.
>
> OK, I will fix it in next patch;
>
>> +- reset-names   : describe reset node register
>
> What happened to clocks? You still have to list which ones apply even if
> documented in the common binding.
>
> OK, I will fix it in next patch;
>
>> +
>> +Example:
>> +
>> + ufs: ufs@ff3b {
>> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
>> + /* 0: HCI standard */
>> + /* 1: UFS SYS CTRL */
>> + reg = <0x0 0xff3b 0x0 0x1000>,
>> + <0x0 0xff3b1000 0x0 0x1000>;
>> + interrupt-parent = <>;
>> + interrupts = ;
>> + /* offset: 0x84; bit: 7  */
>> + resets = <_rst 0x84 7>;
>> + reset-names = "arst";
>> + };
>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> index c39dfef76a18..adcfb79f63f5 100644
>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>> @@ -41,6 +41,8 @@ Optional properties:
>>  -lanes-per-direction : number of lanes available per direction - either 1 
>> or 2.
>> Note that it is assume same number of lanes is used 
>> both
>> directions at once. If not specified, default is 2 
>> lanes per direction.
>> +- resets: reset node register, the "rst" corresponds to reset 
>> the whole UFS IP.
>> +- reset-names   : describe reset node register
>
> Does your controller have 1 or 2 resets? There's no point in adding this
> here if it doesn't apply to your controller.
>
> There are 2 reset in our soc init, the "rst" corresponds to reset the whole 
> UFS IP, and " arst " only reset the APB/AXI bus.
> Discussed with our soc colleagues that "arst" is assert by default and needs 
> to deassert,but it done in bootloader,so will remove 'arst' in next patch.
>
> About the 'reset' property，it seems that Arnd Bergmann has different 
> suggestion，he suggested that add 'rst' to ufshcd-pltfrm because it seems 
> common.
> But it looks like only our soc init needs it. What's your opinion? Does it 
> still needs add to common bindings?

For a single reset, then I'm fine with it being in the common
bindings. If there are multiple then it should be in the specific
bindings as the reset names are likely different.

Rob

Re: [PATCH RFC 1/3] scsi: ufs: set the device reference clock setting

[Rob Herring]

On Tue, May 22, 2018 at 09:51:38AM +0530, Sayali Lokhande wrote:
> From: Subhash Jadavani 
> 
> UFS host supplies the reference clock to UFS device and UFS device
> specification allows host to provide one of the 4 frequencies (19.2 MHz,
> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> device reference clock frequency setting in the device based on what
> frequency it is supplying to UFS device.
> 
> Signed-off-by: Subhash Jadavani 
> [c...@codeaurora.org: Resolved trivial merge conflicts]
> Signed-off-by: Can Guo 
> Signed-off-by: Sayali Lokhande 
> ---
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  8 +++
>  drivers/scsi/ufs/ufs.h |  9 
>  drivers/scsi/ufs/ufshcd-pltfrm.c   | 20 
>  drivers/scsi/ufs/ufshcd.c  | 60 
> ++
>  drivers/scsi/ufs/ufshcd.h  |  2 +
>  5 files changed, 99 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index c39dfef..ac94220 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -41,6 +41,12 @@ Optional properties:
>  -lanes-per-direction : number of lanes available per direction - either 1 or 
> 2.
> Note that it is assume same number of lanes is used 
> both
> directions at once. If not specified, default is 2 
> lanes per direction.
> +- dev-ref-clk-freq   : Specify the device reference clock frequency, must be 
> one of the following:
> +   0: 19.2 MHz
> +   1: 26 MHz
> +   2: 38.4 MHz
> +   3: 52 MHz
> +   Defaults to 26 MHz if not specified.

You already have "ref_clk", can't you just read its frequency?

Rob

Re: [PATCH v4 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Fri, Oct 20, 2017 at 04:50:42PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 47 
> ++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..ee114a65143d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,47 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs" for hisi ufs host controller
> +  present on Hi3660 chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> +   order as the clocks property. "clk_ref", "clk_phy" is 
> optional
> +- resets: reset node register, one reset the clk and the other 
> reset the controller
> +- reset-names   : describe reset node register
> +
> +Optional properties for board device:
> +- reset-gpio : specifies to reset devices

reset-gpios is the preferred form.

> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + clocks = <_ctrl HI3660_CLK_GATE_UFSIO_REF>,
> + <_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
> + clock-names = "clk_ref", "clk_phy";
> + freq-table-hz = <0 0>, <0 0>;

Not documented.

> + /* offset: 0x84; bit: 12 */
> + /* offset: 0x84; bit: 7  */
> + resets = <_rst 0x84 12>,
> + <_rst 0x84 7>;
> + reset-names = "rst", "assert";
> + };
> +
> +  {

Don't show the SoC/Board split in examples. That's just convention, not 
part of the binding.

> + reset-gpio = < 1 0>;
> + status = "okay";

Plus it's wrong because the default is okay.

> + };
> +
> -- 
> 2.11.0
> 

Re: [PATCH v7 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Sat, Jan 06, 2018 at 05:51:14PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 43 
> ++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..175693e47d6b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,43 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs", "jedec,ufs-1.1" 
> for hisi ufs
> + host controller present on Hi36xx 
> chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- clocks : List of phandle and clock specifier pairs
> +- clock-names   : List of clock input name strings sorted in the same
> + order as the clocks property. 
> "ref_clk", "phy_clk" is optional
> +- freq-table-hz  : Array of  operating frequencies 
> stored in the same
> +  order as the clocks property. If this property is 
> not
> +   defined or a value in the array is "0" then it is 
> assumed
> +   that the frequency is set by the parent clock or a
> +   fixed rate clock source.

Doesn't the assigned-clocks binding work here? I'd suggest dropping this 
until you really need it.

> +- resets: reset node register, one reset the clk and the other 
> reset the controller
> +- reset-names   : describe reset node register
> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + clocks = <_ctrl HI3660_CLK_GATE_UFSIO_REF>,
> + <_ctrl HI3660_CLK_GATE_UFSPHY_CFG>;
> + clock-names = "ref_clk", "phy_clk";
> + freq-table-hz = <0 0>, <0 0>;
> + /* offset: 0x84; bit: 12 */
> + /* offset: 0x84; bit: 7  */
> + resets = <_rst 0x84 12>,
> + <_rst 0x84 7>;
> + reset-names = "rst", "assert";
> + };
> -- 
> 2.15.0
> 

Re: [PATCH 1/3] devicetree: bindings: scsi: hisi_sas: add LED feature for v2 hw

[Rob Herring]

On Thu, Jan 18, 2018 at 12:46:52AM +0800, John Garry wrote:
> From: Xiaofei Tan <tanxiao...@huawei.com>

"dt-bindings: ..." is the preferred subject prefix.

> 
> Add directly attached disk LED feature for v2 hw.
> 
> Signed-off-by: Xiaofei Tan <tanxiao...@huawei.com>
> Signed-off-by: John Garry <john.ga...@huawei.com>
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> index b6a869f..df3bef7 100644
> --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -8,7 +8,10 @@ Main node required properties:
>   (b) "hisilicon,hip06-sas-v2" for v2 hw in hip06 chipset
>   (c) "hisilicon,hip07-sas-v2" for v2 hw in hip07 chipset
>- sas-addr : array of 8 bytes for host SAS address
> -  - reg : Address and length of the SAS register
> +  - reg : Contains two regions. The first is the address and length of the 
> SAS
> +  register. The second is the address and length of CPLD register for
> +  SGPIO control. The second is optional, and should be set only when
> +  we use a CPLD for directly attached disk LED control.

Ah SGPIO, what nice memories I have of trying to support that after the 
chip was done. Seems you have the same fortune. :)

What happens if you have a different CPLD or SGPIO control? Really this 
should probably be a separate node, but the kernel isn't setup for 
separate SGPIO drivers either. 

Reviewed-by: Rob Herring <r...@kernel.org>

Rob

Re: [PATCH] scsi: ufs-qcom: add number of lanes per direction

[Rob Herring]

On Mon, Feb 05, 2018 at 08:02:07PM +0800, Can Guo wrote:
> From: Gilad Broner 
> 
> Different platforms may have different number of lanes for the UFS link.
> Add parameter to device tree specifying how many lanes should be
> configured for the UFS link. And don't print err message for clocks
> that are optional, this leads to unnecessary confusion about failure.
> 
> Signed-off-by: Gilad Broner 
> Signed-off-by: Subhash Jadavani 
> Signed-off-by: Can Guo 
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index 5357919..4cee3f9 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -31,6 +31,9 @@ Optional properties:
> defined or a value in the array is "0" then it is 
> assumed
> that the frequency is set by the parent clock or a
> fixed rate clock source.
> +- lanes-per-direction:   number of lanes available per direction - 
> either 1 or 2.
> + Note that it is assume same number of lanes is used 
> both directions at once.

Seems reasonable until someone does not make things symmetrical. We 
should design for that case.

> + If not specified, default is 2 lanes per direction.
>  
>  Note: If above properties are not defined it can be assumed that the supply
>  regulators or clocks are always on.

Re: [PATCH v8 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Tue, Feb 13, 2018 at 06:14:09PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei <liwei...@huawei.com>
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 37 
> ++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt

Reviewed-by: Rob Herring <r...@kernel.org>

Re: [PATCH v4 01/10] dt-bindings: scsi: hisi_sas: add an property of signal attenuation

[Rob Herring]

On Wed, Mar 07, 2018 at 08:25:05PM +0800, John Garry wrote:
> From: Xiaofei Tan <tanxiao...@huawei.com>
> 
> For some new boards with hip07 chipset we are required to
> set PHY config registers differently. The hw property which
> determines how to set these registers is in the PHY signal
> attenuation readings.
> 
> This patch add an devicetree property, "hisilicon,signal-attenuation",
> which is used to describe the signal attenuation of an board.
> 
> Cc: Rob Herring <robh...@kernel.org>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Signed-off-by: Xiaofei Tan <tanxiao...@huawei.com>
> Signed-off-by: John Garry <john.ga...@huawei.com>
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Rob Herring <r...@kernel.org>

Re: [PATCH 1/2] scsi: ufs: Add UFS platform driver for Cadence UFS

[Rob Herring]

On Wed, Mar 28, 2018 at 11:25:34AM +, Janek Kotas wrote:
> This patch adds a device tree platform
> driver description for Cadence UFS Controller.

You have exactly the same subject for both patches. Don't do that. For 
bindings, the preferred subject prefix is "dt-bindings: ufs: ".

> 
> Signed-off-by: Jan Kotas 
> ---
>  .../devicetree/bindings/ufs/cdns-ufs-pltfrm.txt|   31 
> 
>  1 files changed, 31 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt
> new file mode 100644
> index 000..d10229d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/cdns-ufs-pltfrm.txt

rename to cdns,ufshc.txt

> @@ -0,0 +1,31 @@
> +* Cadence Universal Flash Storage (UFS) Controller
> +
> +UFS 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 the following controller:
> + "cdns,ufshc"
> +   complemented with the JEDEC version:
> + "jedec,ufs-2.0"
> +
> +- reg: registers mapping
> +- interrupts : interrupts mapping

How many?

> +- clocks : List of phandle and clock specifier pairs.
> +- clock-names: List of clock input name strings sorted in the same
> +   order as the clocks property. "core_clk" is mandatory.

"_clk" is redundant.

Really only one clock for the block? No clock from the PHY? No bus 
clock?

> +- freq-table-hz  : Array of  operating frequencies stored in 
> the same
> +   order as the clocks property. If this property is not
> +   defined or a value in the array is "0" then it is assumed
> +   that the frequency is set by the parent clock or a
> +   fixed rate clock source.

The 'assigned-clocks' property and friends should be able to handle 
this.

> +
> +Example:
> + ufs@fd03 {
> + compatible = "cdns,ufshc", "jedec,ufs-2.0";
> + reg = <0xfd03 0x1>;
> + interrupts = <0 1 0>;
> + freq-table-hz = <0 0>;
> + clocks = <_core_clk 0>;
> + clock-names = "core_clk";
> + };
> -- 
> 1.7.1

Re: [PATCH v9 2/5] dt-bindings: scsi: ufs: add document for hisi-ufs

[Rob Herring]

On Tue, Apr 17, 2018 at 10:08:11PM +0800, Li Wei wrote:
> add ufs node document for Hisilicon.
> 
> Signed-off-by: Li Wei 
> ---
>  Documentation/devicetree/bindings/ufs/ufs-hisi.txt | 29 
> ++
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  | 10 +---
>  2 files changed, 36 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-hisi.txt 
> b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> new file mode 100644
> index ..d49ab7d8f31d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-hisi.txt
> @@ -0,0 +1,29 @@
> +* Hisilicon Universal Flash Storage (UFS) Host Controller
> +
> +UFS nodes are defined to describe on-chip UFS hardware macro.
> +Each UFS Host Controller should have its own node.
> +
> +Required properties:
> +- compatible: compatible list, contains one of the following -
> + "hisilicon,hi3660-ufs", "jedec,ufs-1.1" 
> for hisi ufs
> + host controller present on Hi36xx 
> chipset.
> +- reg   : should contain UFS register address space & UFS SYS 
> CTRL register address,
> +- interrupt-parent  : interrupt device
> +- interrupts: interrupt number
> +- resets: reset node register, the "arst" corresponds to reset 
> the APB/AXI bus.

arst belongs in reset-names.

> +- reset-names   : describe reset node register

What happened to clocks? You still have to list which ones apply even if 
documented in the common binding.

> +
> +Example:
> +
> + ufs: ufs@ff3b {
> + compatible = "hisilicon,hi3660-ufs", "jedec,ufs-1.1";
> + /* 0: HCI standard */
> + /* 1: UFS SYS CTRL */
> + reg = <0x0 0xff3b 0x0 0x1000>,
> + <0x0 0xff3b1000 0x0 0x1000>;
> + interrupt-parent = <>;
> + interrupts = ;
> + /* offset: 0x84; bit: 7  */
> + resets = <_rst 0x84 7>;
> + reset-names = "arst";
> + };
> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt 
> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> index c39dfef76a18..adcfb79f63f5 100644
> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> @@ -41,6 +41,8 @@ Optional properties:
>  -lanes-per-direction : number of lanes available per direction - either 1 or 
> 2.
> Note that it is assume same number of lanes is used 
> both
> directions at once. If not specified, default is 2 
> lanes per direction.
> +- resets: reset node register, the "rst" corresponds to reset 
> the whole UFS IP.
> +- reset-names   : describe reset node register

Does your controller have 1 or 2 resets? There's no point in adding this 
here if it doesn't apply to your controller.


>  Note: If above properties are not defined it can be assumed that the supply
>  regulators or clocks are always on.
> @@ -61,9 +63,11 @@ Example:
>   vccq-max-microamp = 20;
>   vccq2-max-microamp = 20;
>  
> - clocks = < 0>, < 0>, < 0>;
> - clock-names = "core_clk", "ref_clk", "iface_clk";
> - freq-table-hz = <1 2>, <0 0>, <0 0>;
> + clocks = < 0>, < 0>, < 0>, < 0>;
> + clock-names = "core_clk", "ref_clk", "phy_clk", "iface_clk";
> + freq-table-hz = <1 2>, <0 0>, <0 0>, <0 0>;
> + resets = < 0 1>;
> + reset-names = "rst";
>   phys = <>;
>   phy-names = "ufsphy";
>   };
> -- 
> 2.15.0
> 

Re: [PATCH 1/8] dt-bindings: scsi: hisi_sas: add an property of signal attenuation

[Rob Herring]

On Tue, Feb 20, 2018 at 03:13:24AM +0800, John Garry wrote:
> From: Xiaofei Tan <tanxiao...@huawei.com>
> 
> For some new boards with hip07 chipset we are required to
> set PHY config registers differently. The hw property which
> determines how to set these registers is in the PHY signal
> attenuation readings.
> 
> This patch add an devicetree property, signal-attenuation, which
> is used to describe the signal attenuation of an board.
> 
> Cc: Rob Herring <robh...@kernel.org>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Signed-off-by: Xiaofei Tan <tanxiao...@huawei.com>
> Signed-off-by: John Garry <john.ga...@huawei.com>
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> index df3bef7..bd32ecd 100644
> --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -53,6 +53,13 @@ Main node required properties:
>  Optional main node properties:
>   - hip06-sas-v2-quirk-amt : when set, indicates that the v2 controller has 
> the
>   "am-max-transmissions" limitation.
> + - signal-attenuation : array of 3 32-bit values, containing de-emphasis,

Needs a vendor prefix.

> + preshoot, and boost attenuation readings for the board. They
> + are used to describe the signal attenuation of the board. These
> + values' range is 7600 to 12400, and used to represent -24dB to
> + 24dB.
> + The formula is "y = (x-1)/1". For example, 10478
> + means 4.78dB.
>  
>  Example:
>   sas0: sas@c100 {
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/8] dt-bindings: scsi: hisi_sas: add an property of signal attenuation

[Rob Herring]

On Fri, Mar 2, 2018 at 10:10 AM, John Garry <john.ga...@huawei.com> wrote:
> On 02/03/2018 15:57, Rob Herring wrote:
>>
>> On Fri, Mar 2, 2018 at 9:06 AM, John Garry <john.ga...@huawei.com> wrote:
>>>
>>> > From: Xiaofei Tan <tanxiao...@huawei.com>
>>> >
>>> > For some new boards with hip07 chipset we are required to
>>> > set PHY config registers differently. The hw property which
>>> > determines how to set these registers is in the PHY signal
>>> > attenuation readings.
>>> >
>>> > This patch add an devicetree property, hisi-signal-attenuation,
>>> > which is used to describe the signal attenuation of an board.
>>> >
>>> > Cc: Rob Herring <robh...@kernel.org>
>>> > Cc: Mark Rutland <mark.rutl...@arm.com>
>>> > Signed-off-by: Xiaofei Tan <tanxiao...@huawei.com>
>>> > Signed-off-by: John Garry <john.ga...@huawei.com>
>>> > ---
>>> >  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 7 +++
>>> >  1 file changed, 7 insertions(+)
>>> >
>>> > diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> > b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> > index df3bef7..6f6876a 100644
>>> > --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> > +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>>> > @@ -53,6 +53,13 @@ Main node required properties:
>>> >  Optional main node properties:
>>> >   - hip06-sas-v2-quirk-amt : when set, indicates that the v2 controller
>>> > has the
>>> > "am-max-transmissions" limitation.
>>> > + - hisi-signal-attenuation : array of 3 32-bit values, containing
>>> > de-emphasis,
>>
>> The format for vendor prefixes is: ,property-name
>>
>> And should only be ones defined in vendor-prefixes.txt
>
>
> Hi Rob,
>
> To save a v4, that would be "hisilicon,signal-attenuation", right?

Right.

Rob

Re: [PATCH v2] scsi: ufs-qcom: add number of lanes for Tx and Rx links

[Rob Herring]

On Tue, Feb 27, 2018 at 01:46:17PM +0800, Can Guo wrote:
> From: Gilad Broner <gbro...@codeaurora.org>
> 
> Different platforms may have different number of lanes for the UFS Tx/Rx
> links. Add parameter to device tree specifying how many lanes should be
> configured for the UFS Tx/Rx links. And don't print err message for clocks
> that are optional, this leads to unnecessary confusion about failure.
> 
> Signed-off-by: Gilad Broner <gbro...@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subha...@codeaurora.org>
> Signed-off-by: Can Guo <c...@codeaurora.org>
> ---
> 
> Changes since v1:
>   - Change commit subject for better description.
>   - Incorporated Rob's review comments to use lanes-tx and lanes-rx
> to handle asymmetric Tx/Rx links.
> 
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |  4 ++

Reviewed-by: Rob Herring <r...@kernel.org>

>  drivers/scsi/ufs/ufs-qcom.c| 65 
> +-
>  drivers/scsi/ufs/ufshcd.c  | 29 ++
>  drivers/scsi/ufs/ufshcd.h  |  5 ++
>  4 files changed, 78 insertions(+), 25 deletions(-)

Re: [PATCH v2 1/8] dt-bindings: scsi: hisi_sas: add an property of signal attenuation

[Rob Herring]

On Fri, Mar 2, 2018 at 9:06 AM, John Garry <john.ga...@huawei.com> wrote:
> From: Xiaofei Tan <tanxiao...@huawei.com>
>
> For some new boards with hip07 chipset we are required to
> set PHY config registers differently. The hw property which
> determines how to set these registers is in the PHY signal
> attenuation readings.
>
> This patch add an devicetree property, hisi-signal-attenuation,
> which is used to describe the signal attenuation of an board.
>
> Cc: Rob Herring <robh...@kernel.org>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Signed-off-by: Xiaofei Tan <tanxiao...@huawei.com>
> Signed-off-by: John Garry <john.ga...@huawei.com>
> ---
>  Documentation/devicetree/bindings/scsi/hisilicon-sas.txt | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt 
> b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> index df3bef7..6f6876a 100644
> --- a/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> +++ b/Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
> @@ -53,6 +53,13 @@ Main node required properties:
>  Optional main node properties:
>   - hip06-sas-v2-quirk-amt : when set, indicates that the v2 controller has 
> the
> "am-max-transmissions" limitation.
> + - hisi-signal-attenuation : array of 3 32-bit values, containing 
> de-emphasis,

The format for vendor prefixes is: ,property-name

And should only be ones defined in vendor-prefixes.txt

> +   preshoot, and boost attenuation readings for the board. They
> +   are used to describe the signal attenuation of the board. 
> These
> +   values' range is 7600 to 12400, and used to represent -24dB to
> +   24dB.
> +   The formula is "y = (x-1)/1". For example, 10478
> +   means 4.78dB.
>
>  Example:
> sas0: sas@c100 {
> --
> 1.9.1
>