| Message ID | 1444663237-238302-8-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Monday 12 October 2015 23:20:19 John Garry wrote: > +int hisi_sas_ioremap(struct hisi_hba *hisi_hba) > +{ > + struct platform_device *pdev = hisi_hba->pdev; > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + hisi_hba->regs = devm_ioremap(dev, > + res->start, > + resource_size(res)); > + if (!hisi_hba->regs) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + hisi_hba->ctrl_regs = devm_ioremap(dev, > + res->start, > + resource_size(res)); > + if (!hisi_hba->ctrl_regs) > + return -ENOMEM; > + > + return 0; > +} > > static const struct of_device_id sas_of_match[] = { > Better use devm_ioremap_resource() here, which registers the resource so they are checked for conflicts and listed in /proc/iomem. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi John,
[auto build test WARNING on scsi/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/John-Garry/HiSilicon-SAS-driver/20151012-231929
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/scsi/hisi_sas/hisi_sas_init.c:198:5: sparse: symbol 'hisi_sas_ioremap' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/2015 11:21 PM, Arnd Bergmann wrote: > On Monday 12 October 2015 23:20:19 John Garry wrote: >> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba) >> +{ >> + struct platform_device *pdev = hisi_hba->pdev; >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + hisi_hba->regs = devm_ioremap(dev, >> + res->start, >> + resource_size(res)); >> + if (!hisi_hba->regs) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + hisi_hba->ctrl_regs = devm_ioremap(dev, >> + res->start, >> + resource_size(res)); >> + if (!hisi_hba->ctrl_regs) >> + return -ENOMEM; >> + >> + return 0; >> +} >> >> static const struct of_device_id sas_of_match[] = { >> > > Better use devm_ioremap_resource() here, which registers the resource so they > are checked for conflicts and listed in /proc/iomem. > Yes, hisi_hba->regs can use devm_ioremap_resource. However ctrl_regs have to use devm_ioremap, since the address are sharing among different nodes, unfortunately, and devm_ioremap_resource will fail. Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 13 October 2015 17:47:02 zhangfei wrote: > On 10/12/2015 11:21 PM, Arnd Bergmann wrote: > > On Monday 12 October 2015 23:20:19 John Garry wrote: > >> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba) > >> +{ > >> + struct platform_device *pdev = hisi_hba->pdev; > >> + struct device *dev = &pdev->dev; > >> + struct resource *res; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + hisi_hba->regs = devm_ioremap(dev, > >> + res->start, > >> + resource_size(res)); > >> + if (!hisi_hba->regs) > >> + return -ENOMEM; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > >> + hisi_hba->ctrl_regs = devm_ioremap(dev, > >> + res->start, > >> + resource_size(res)); > >> + if (!hisi_hba->ctrl_regs) > >> + return -ENOMEM; > >> + > >> + return 0; > >> +} > >> > >> static const struct of_device_id sas_of_match[] = { > >> > > > > Better use devm_ioremap_resource() here, which registers the resource so they > > are checked for conflicts and listed in /proc/iomem. > > > > Yes, hisi_hba->regs can use devm_ioremap_resource. > > However ctrl_regs have to use devm_ioremap, since the address are > sharing among different nodes, unfortunately, and devm_ioremap_resource > will fail. This sounds like it should be fixed in the DT binding then, to ensure that the ranges don't overlap. Mapping the same register region multiple times is generally considered a bad idea because the drivers that map them often don't have global locks that serialize the access, so it's better to have code in place that ensures that they are distinct. What is the purpose of the ctrl_regs region, and why is it shared across multiple devices? Are all users of these registers in the same driver? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/13/2015 08:20 PM, Arnd Bergmann wrote: > On Tuesday 13 October 2015 17:47:02 zhangfei wrote: >> On 10/12/2015 11:21 PM, Arnd Bergmann wrote: >>> On Monday 12 October 2015 23:20:19 John Garry wrote: >>>> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba) >>>> +{ >>>> + struct platform_device *pdev = hisi_hba->pdev; >>>> + struct device *dev = &pdev->dev; >>>> + struct resource *res; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + hisi_hba->regs = devm_ioremap(dev, >>>> + res->start, >>>> + resource_size(res)); >>>> + if (!hisi_hba->regs) >>>> + return -ENOMEM; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); >>>> + hisi_hba->ctrl_regs = devm_ioremap(dev, >>>> + res->start, >>>> + resource_size(res)); >>>> + if (!hisi_hba->ctrl_regs) >>>> + return -ENOMEM; >>>> + >>>> + return 0; >>>> +} >>>> >>>> static const struct of_device_id sas_of_match[] = { >>>> >>> >>> Better use devm_ioremap_resource() here, which registers the resource so they >>> are checked for conflicts and listed in /proc/iomem. >>> >> >> Yes, hisi_hba->regs can use devm_ioremap_resource. >> >> However ctrl_regs have to use devm_ioremap, since the address are >> sharing among different nodes, unfortunately, and devm_ioremap_resource >> will fail. > > This sounds like it should be fixed in the DT binding then, to ensure > that the ranges don't overlap. > > Mapping the same register region multiple times is generally considered > a bad idea because the drivers that map them often don't have global > locks that serialize the access, so it's better to have code in place > that ensures that they are distinct. > > What is the purpose of the ctrl_regs region, and why is it shared > across multiple devices? > > Are all users of these registers in the same driver? > We are considering using syscon for ctrl_regs. Thanks Arnd. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h index 929c513..5f72d98 100644 --- a/drivers/scsi/hisi_sas/hisi_sas.h +++ b/drivers/scsi/hisi_sas/hisi_sas.h @@ -69,6 +69,8 @@ struct hisi_hba { struct platform_device *pdev; + void __iomem *regs; + void __iomem *ctrl_regs; u8 sas_addr[SAS_ADDR_SIZE]; diff --git a/drivers/scsi/hisi_sas/hisi_sas_init.c b/drivers/scsi/hisi_sas/hisi_sas_init.c index 38b9ed2..8f1e856 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_init.c +++ b/drivers/scsi/hisi_sas/hisi_sas_init.c @@ -195,6 +195,28 @@ static void hisi_sas_free(struct hisi_hba *hisi_hba) hisi_hba->sata_breakpoint_dma); } +int hisi_sas_ioremap(struct hisi_hba *hisi_hba) +{ + struct platform_device *pdev = hisi_hba->pdev; + struct device *dev = &pdev->dev; + struct resource *res; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + hisi_hba->regs = devm_ioremap(dev, + res->start, + resource_size(res)); + if (!hisi_hba->regs) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + hisi_hba->ctrl_regs = devm_ioremap(dev, + res->start, + resource_size(res)); + if (!hisi_hba->ctrl_regs) + return -ENOMEM; + + return 0; +} static const struct of_device_id sas_of_match[] = { { .compatible = "hisilicon,sas-controller-v1",}, @@ -241,6 +263,9 @@ static struct hisi_hba *hisi_sas_hba_alloc( hisi_hba->shost = shost; + if (hisi_sas_ioremap(hisi_hba)) + goto err_out; + if (hisi_sas_alloc(hisi_hba, shost)) { hisi_sas_free(hisi_hba); goto err_out;
This includes registers for core SAS controllers and also SAS control registers. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/hisi_sas/hisi_sas.h | 2 ++ drivers/scsi/hisi_sas/hisi_sas_init.c | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+)