| Message ID | 1420656594-8908-3-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri <m-karicheri2@ti.com> wrote: > Move of_dma_configure() to device.c so that same function can be re-used > for PCI devices to obtain DMA configuration from DT. Also add a second > argument so that for PCI, DT node of root bus host bridge can be used to > obtain the DMA configuration for the slave PCI device. Additionally fix > the dma-range size when the DT attribute is missing. i.e set size to > dev->coherent_dma_mask + 1 instead of dev->coherent_dma_mask. Additionally is a red flag for put in another patch. There is an issue I think as well. > + ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > + if (ret < 0) { > + dma_addr = offset = 0; > + size = dev->coherent_dma_mask + 1; If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and have a size of 0. There may also be a problem when the mask is only 32-bit type. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: > On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri <m-karicheri2@ti.com> wrote: > > > + ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > > + if (ret < 0) { > > + dma_addr = offset = 0; > > + size = dev->coherent_dma_mask + 1; > > If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and > have a size of 0. There may also be a problem when the mask is only > 32-bit type. The mask is always a 64-bit type, it's not optional. But you are right, the 64-bit mask case is broken, so I guess we have to fix it differently by always passing the smaller value into arch_setup_dma_ops and adapting that function instead. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/08/2015 03:40 AM, Arnd Bergmann wrote: > On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: >> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> wrote: >> >>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); >>> + if (ret< 0) { >>> + dma_addr = offset = 0; >>> + size = dev->coherent_dma_mask + 1; >> >> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and >> have a size of 0. There may also be a problem when the mask is only >> 32-bit type. > > The mask is always a 64-bit type, it's not optional. But you are right, > the 64-bit mask case is broken, so I guess we have to fix it differently > by always passing the smaller value into arch_setup_dma_ops and > adapting that function instead. Arnd, What is the smaller value you are referring to in the below code? between *dev->dma_mask and size from DT? But overflow can still happen when size is to be calculated in arch_setup_dma_ops() for Non DT case or when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can we discuss the code change you have in mind when you get a chance? Murali > > Arnd
On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote: > On 01/08/2015 03:40 AM, Arnd Bergmann wrote: > > On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: > >> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> wrote: > >> > >>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); > >>> + if (ret< 0) { > >>> + dma_addr = offset = 0; > >>> + size = dev->coherent_dma_mask + 1; > >> > >> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and > >> have a size of 0. There may also be a problem when the mask is only > >> 32-bit type. > > > > The mask is always a 64-bit type, it's not optional. But you are right, > > the 64-bit mask case is broken, so I guess we have to fix it differently > > by always passing the smaller value into arch_setup_dma_ops and > > adapting that function instead. > Arnd, > > What is the smaller value you are referring to in the below code? > between *dev->dma_mask and size from DT? But overflow can still happen > when size is to be calculated in arch_setup_dma_ops() for Non DT case or > when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can > we discuss the code change you have in mind when you get a chance? I meant changing every function that the size values gets passed into to take a mask like 0xffffffff instead of a size like 0x100000000, so we can represent a 64-bit capable bus correctly. This means we also need to adapt the value returned from of_dma_get_range. A minor complication here is that the DT properties sometimes already contain the mask value, in particular when we want to represent a full mapping like bus { #address-cells = <1>; #size-cells = <1>; dma-ranges = <0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */ }; as opposed to bus { #address-cells = <1>; #size-cells = <1>; dma-ranges = <0 0 0x80000000>; /* only lower 2GB, DMA_BIT_MASK(31) */ }; or bus { #address-cells = <2>; #size-cells = <2>; dma-ranges = <0 0 0x0000000100000000>; /* 4GB of 64-bit address space */ }; Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/08/2015 05:24 PM, Arnd Bergmann wrote: > On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote: >> On 01/08/2015 03:40 AM, Arnd Bergmann wrote: >>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: >>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> wrote: >>>> >>>>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); >>>>> + if (ret< 0) { >>>>> + dma_addr = offset = 0; >>>>> + size = dev->coherent_dma_mask + 1; >>>> >>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and >>>> have a size of 0. There may also be a problem when the mask is only >>>> 32-bit type. >>> >>> The mask is always a 64-bit type, it's not optional. But you are right, >>> the 64-bit mask case is broken, so I guess we have to fix it differently >>> by always passing the smaller value into arch_setup_dma_ops and >>> adapting that function instead. >> Arnd, >> >> What is the smaller value you are referring to in the below code? >> between *dev->dma_mask and size from DT? But overflow can still happen >> when size is to be calculated in arch_setup_dma_ops() for Non DT case or >> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can >> we discuss the code change you have in mind when you get a chance? > > I meant changing every function that the size values gets passed into > to take a mask like 0xffffffff instead of a size like 0x100000000, so > we can represent a 64-bit capable bus correctly. The function here refers to arch_setup_dma_ops() and anything that is called by this API, right? > > This means we also need to adapt the value returned from of_dma_get_range. > A minor complication here is that the DT properties sometimes already > contain the mask value, in particular when we want to represent a > full mapping like > The grep of dma-ranges for arch/arm shows the size used is mask + 1 as ./boot/dts/keystone.dtsi: dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>; ./boot/dts/keystone.dtsi: dma-ranges; ./boot/dts/keystone.dtsi: dma-ranges; ./boot/dts/r8a7790.dtsi: dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 ./boot/dts/integratorap.dts: dma-ranges = <0x80000000 0x0 0x80000000>; ./boot/dts/r8a7791.dtsi: dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x80000000 ./boot/dts/.k2hk-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>; ./boot/dts/.k2hk-evm.dtb.dts.tmp: dma-ranges; ./boot/dts/.k2l-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>; ./boot/dts/.k2l-evm.dtb.dts.tmp: dma-ranges; ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>; ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges; ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges; ./boot/dts/k2e.dtsi: dma-ranges; ./boot/dts/k2e.dtsi: dma-ranges; So for ARM 32 the below case doesn't seem to apply. > bus { > #address-cells =<1>; > #size-cells =<1>; > dma-ranges =<0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */ > }; > > as opposed to > A search under arch/arm64 and arch/powerpc showed another format for dma-ranges for PCI. Now I am bit nervous on how of_pci_dma_configure() behave for these platforms as PCI nodes on these platforms may have dma-ranges in a different format than that in platform bus nodes. These DT property has additional cell for resource identification (example 0x42000000 as the first cell). Also one of them, amd-seattle-soc.dtsi uses the format of size (0x7fffffffff) similar to what you refered above. ./boot/dts/apm/apm-storm.dtsi: dma-ranges = <0x42000000 0x80 0x00000000 0x80 0x00000000 0x00 0x80000000 ./boot/dts/apm/apm-storm.dtsi: dma-ranges = <0x42000000 0x80 0x00000000 0x80 0x00000000 0x00 0x80000000 ./boot/dts/apm/apm-storm.dtsi: dma-ranges = <0x42000000 0x80 0x00000000 0x80 0x00000000 0x00 0x80000000 ./boot/dts/apm/apm-storm.dtsi: dma-ranges = <0x42000000 0x80 0x00000000 0x80 0x00000000 0x00 0x80000000 ./boot/dts/apm/apm-storm.dtsi: dma-ranges = <0x42000000 0x80 0x00000000 0x80 0x00000000 0x00 0x80000000 ./boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x80 0x0 0x80 0x0 0x7f 0xffffffff>; ./boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x43000000 0x80 0x0 0x80 0x0 0x7f 0xffffffff>; Or I got this wrong. Any comment? For ARM 32, DT size value can be extracted and size-1 can be passed to arch_setup_dma_ops() provided it handles this value properly. > bus { > #address-cells =<1>; > #size-cells =<1>; > dma-ranges =<0 0 0x80000000>; /* only lower 2GB, DMA_BIT_MASK(31) */ > }; > > or > > bus { > #address-cells =<2>; > #size-cells =<2>; > dma-ranges =<0 0 0x0000000100000000>; /* 4GB of 64-bit address space */ > }; > > > Arnd
On Thursday 08 January 2015 18:44:31 Murali Karicheri wrote: > The grep of dma-ranges for arch/arm shows the size used is mask + 1 as > > ./boot/dts/keystone.dtsi: dma-ranges = <0x80000000 0x8 0x00000000 > 0x80000000>; > ./boot/dts/keystone.dtsi: dma-ranges; > ./boot/dts/keystone.dtsi: dma-ranges; > ./boot/dts/r8a7790.dtsi: dma-ranges = <0x42000000 0 0x40000000 0 > 0x40000000 0 0x80000000 > ./boot/dts/integratorap.dts: dma-ranges = <0x80000000 0x0 0x80000000>; > ./boot/dts/r8a7791.dtsi: dma-ranges = <0x42000000 0 0x40000000 0 > 0x40000000 0 0x80000000 > ./boot/dts/.k2hk-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 > 0x00000000 0x80000000>; > ./boot/dts/.k2hk-evm.dtb.dts.tmp: dma-ranges; > ./boot/dts/.k2l-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 > 0x00000000 0x80000000>; > ./boot/dts/.k2l-evm.dtb.dts.tmp: dma-ranges; > ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges = <0x80000000 0x8 > 0x00000000 0x80000000>; > ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges; > ./boot/dts/.k2e-evm.dtb.dts.tmp: dma-ranges; > ./boot/dts/k2e.dtsi: dma-ranges; > ./boot/dts/k2e.dtsi: dma-ranges; > > So for ARM 32 the below case doesn't seem to apply. > Ah, I guess that is because an empty dma-ranges property serves the same purpose. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote: >> On 01/08/2015 03:40 AM, Arnd Bergmann wrote: >> > On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: >> >> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> wrote: >> >> >> >>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); >> >>> + if (ret< 0) { >> >>> + dma_addr = offset = 0; >> >>> + size = dev->coherent_dma_mask + 1; >> >> >> >> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and >> >> have a size of 0. There may also be a problem when the mask is only >> >> 32-bit type. >> > >> > The mask is always a 64-bit type, it's not optional. But you are right, >> > the 64-bit mask case is broken, so I guess we have to fix it differently >> > by always passing the smaller value into arch_setup_dma_ops and >> > adapting that function instead. >> Arnd, >> >> What is the smaller value you are referring to in the below code? >> between *dev->dma_mask and size from DT? But overflow can still happen >> when size is to be calculated in arch_setup_dma_ops() for Non DT case or >> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can >> we discuss the code change you have in mind when you get a chance? > > I meant changing every function that the size values gets passed into > to take a mask like 0xffffffff instead of a size like 0x100000000, so > we can represent a 64-bit capable bus correctly. Or you could special case a size of 0 to mean all/max? I'm not sure if we need to handle size=0 for other reasons beyond just wrong DT data. > This means we also need to adapt the value returned from of_dma_get_range. > A minor complication here is that the DT properties sometimes already > contain the mask value, in particular when we want to represent a > full mapping like > > bus { > #address-cells = <1>; > #size-cells = <1>; > dma-ranges = <0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */ This is wrong though, right? The DT should be size. Certainly, this could be a valid size, but that would not make the mask 0xfffffffe. We would still want it to be 0xffffffff. We could do a fixup for these cases adding 1 if bit 0 is set (or not subtracting 1 if we want the mask). Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/09/2015 10:34 AM, Rob Herring wrote: > On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann<arnd@arndb.de> wrote: >> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote: >>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote: >>>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: >>>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> wrote: >>>>> >>>>>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); >>>>>> + if (ret< 0) { >>>>>> + dma_addr = offset = 0; >>>>>> + size = dev->coherent_dma_mask + 1; >>>>> >>>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and >>>>> have a size of 0. There may also be a problem when the mask is only >>>>> 32-bit type. >>>> >>>> The mask is always a 64-bit type, it's not optional. But you are right, >>>> the 64-bit mask case is broken, so I guess we have to fix it differently >>>> by always passing the smaller value into arch_setup_dma_ops and >>>> adapting that function instead. >>> Arnd, >>> >>> What is the smaller value you are referring to in the below code? >>> between *dev->dma_mask and size from DT? But overflow can still happen >>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or >>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can >>> we discuss the code change you have in mind when you get a chance? >> >> I meant changing every function that the size values gets passed into >> to take a mask like 0xffffffff instead of a size like 0x100000000, so >> we can represent a 64-bit capable bus correctly. > > Or you could special case a size of 0 to mean all/max? I'm not sure if > we need to handle size=0 for other reasons beyond just wrong DT data. > >> This means we also need to adapt the value returned from of_dma_get_range. >> A minor complication here is that the DT properties sometimes already >> contain the mask value, in particular when we want to represent a >> full mapping like >> >> bus { >> #address-cells =<1>; >> #size-cells =<1>; >> dma-ranges =<0 0 0xffffffff>; /* all 4 GB, DMA_BIT_MASK(32) */ > > This is wrong though, right? The DT should be size. Certainly, this > could be a valid size, but that would not make the mask 0xfffffffe. We > would still want it to be 0xffffffff. > > We could do a fixup for these cases adding 1 if bit 0 is set (or not > subtracting 1 if we want the mask). > > Rob Arnd, Rob, et all, Do we have preference one way or other for the size format? If we need to follow the mask format, all of the calling functions below and the arm_iommu_create_mapping() has to change as well to use this changed format. drivers/gpu/drm/rockchip/rockchip_drm_drv.c: mapping = arm_iommu_create_mapping(&platform_bus_type, 0x00000000, drivers/gpu/drm/exynos/exynos_drm_iommu.c: mapping = arm_iommu_create_mapping(&platform_bus_type, priv->da_start, drivers/media/platform/omap3isp/isp.c: mapping = arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G); drivers/iommu/shmobile-iommu.c: mapping = arm_iommu_create_mapping(&platform_bus_type, 0, drivers/iommu/ipmmu-vmsa.c: mapping = arm_iommu_create_mapping(&platform_bus_type, So IMO, keeping current convention of size and taking care of exception in DT handling is the right thing to do instead of changing all of the above functions. i.e in of_dma_configure(), if DT provide a mask for size (lsb set), we will check that and add 1 to it. Only case in DTS that I can see this usage is arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x80 0x0 0x80 0x0 0x7f 0xffffffff>; arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x43000000 0x80 0x0 0x80 0x0 0x7f 0xffffffff>; This logic should take care of setting the size to ox80_00000000 for these cases. if dma_coherent_mask is set to max of u64, then this will result in a zero size (both DT case and non DT case). So treat a size of zero as error being overflow. Also arm_iommu_create_mapping() currently accept a size of type size_t which means, this function expect the size to be max of 0xffffffff. So in arm_setup_iommu_dma_ops(), we need to check if size if >0xffffffff and return an error. If in future this function support u64 for size, this check can be removed. I will update the series with this change and post it if we have an agreement on this. Please repond. Thanks
On Fri, Jan 23, 2015 at 12:19 PM, Murali Karicheri <m-karicheri2@ti.com> wrote: > On 01/09/2015 10:34 AM, Rob Herring wrote: >> >> On Thu, Jan 8, 2015 at 4:24 PM, Arnd Bergmann<arnd@arndb.de> wrote: >>> >>> On Thursday 08 January 2015 14:26:36 Murali Karicheri wrote: >>>> >>>> On 01/08/2015 03:40 AM, Arnd Bergmann wrote: >>>>> >>>>> On Wednesday 07 January 2015 17:37:56 Rob Herring wrote: >>>>>> >>>>>> On Wed, Jan 7, 2015 at 12:49 PM, Murali Karicheri<m-karicheri2@ti.com> >>>>>> wrote: >>>>>> >>>>>>> + ret = of_dma_get_range(np,&dma_addr,&paddr,&size); >>>>>>> + if (ret< 0) { >>>>>>> + dma_addr = offset = 0; >>>>>>> + size = dev->coherent_dma_mask + 1; >>>>>> >>>>>> >>>>>> If coherent_dma_mask is DMA_BIT_MASK(64), then you will overflow and >>>>>> have a size of 0. There may also be a problem when the mask is only >>>>>> 32-bit type. >>>>> >>>>> >>>>> The mask is always a 64-bit type, it's not optional. But you are right, >>>>> the 64-bit mask case is broken, so I guess we have to fix it >>>>> differently >>>>> by always passing the smaller value into arch_setup_dma_ops and >>>>> adapting that function instead. >>>> >>>> Arnd, >>>> >>>> What is the smaller value you are referring to in the below code? >>>> between *dev->dma_mask and size from DT? But overflow can still happen >>>> when size is to be calculated in arch_setup_dma_ops() for Non DT case or >>>> when DT size is configured to be equivalent of DMA_BIT_MASK(64) + 1. Can >>>> we discuss the code change you have in mind when you get a chance? >>> >>> >>> I meant changing every function that the size values gets passed into >>> to take a mask like 0xffffffff instead of a size like 0x100000000, so >>> we can represent a 64-bit capable bus correctly. >> >> >> Or you could special case a size of 0 to mean all/max? I'm not sure if >> we need to handle size=0 for other reasons beyond just wrong DT data. >> >>> This means we also need to adapt the value returned from >>> of_dma_get_range. >>> A minor complication here is that the DT properties sometimes already >>> contain the mask value, in particular when we want to represent a >>> full mapping like >>> >>> bus { >>> #address-cells =<1>; >>> #size-cells =<1>; >>> dma-ranges =<0 0 0xffffffff>; /* all 4 GB, >>> DMA_BIT_MASK(32) */ >> >> >> This is wrong though, right? The DT should be size. Certainly, this >> could be a valid size, but that would not make the mask 0xfffffffe. We >> would still want it to be 0xffffffff. >> >> We could do a fixup for these cases adding 1 if bit 0 is set (or not >> subtracting 1 if we want the mask). >> >> Rob > > Arnd, Rob, et all, > > Do we have preference one way or other for the size format? If we need to > follow the mask format, all of the calling functions below and the > arm_iommu_create_mapping() has to change as well to use this changed format. > > drivers/gpu/drm/rockchip/rockchip_drm_drv.c: mapping = > arm_iommu_create_mapping(&platform_bus_type, 0x00000000, > drivers/gpu/drm/exynos/exynos_drm_iommu.c: mapping = > arm_iommu_create_mapping(&platform_bus_type, priv->da_start, > drivers/media/platform/omap3isp/isp.c: mapping = > arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G); > drivers/iommu/shmobile-iommu.c: mapping = > arm_iommu_create_mapping(&platform_bus_type, 0, > drivers/iommu/ipmmu-vmsa.c: mapping = > arm_iommu_create_mapping(&platform_bus_type, > > So IMO, keeping current convention of size and taking care of exception in > DT handling is the right thing to do instead of changing all of the above > functions. i.e in of_dma_configure(), if DT provide a mask for size (lsb > set), we will check that and add 1 to it. Only case in DTS that I can see > this usage is > > arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = <0x80 > 0x0 0x80 0x0 0x7f 0xffffffff>; > arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi: dma-ranges = > <0x43000000 0x80 0x0 0x80 0x0 0x7f 0xffffffff>; This should be fixed regardless. I doubt anyone is worried about 512GB quite yet. > This logic should take care of setting the size to ox80_00000000 for these > cases. if dma_coherent_mask is set to max of u64, then this will result in a > zero size (both DT case and non DT case). So treat a size of zero as error > being overflow. I think this would work, but I really need to see patches. > Also arm_iommu_create_mapping() currently accept a size of type size_t which > means, this function expect the size to be max of 0xffffffff. So in > arm_setup_iommu_dma_ops(), we need to check if size if >0xffffffff and > return an error. If in future this function support u64 for size, this check > can be removed. The aim is to get rid of this function I believe. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" 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/of/device.c b/drivers/of/device.c index 46d6c75c..ccbc958 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -2,6 +2,9 @@ #include <linux/kernel.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/of_address.h> +#include <linux/of_iommu.h> +#include <linux/dma-mapping.h> #include <linux/init.h> #include <linux/module.h> #include <linux/mod_devicetable.h> @@ -66,6 +69,61 @@ int of_device_add(struct platform_device *ofdev) return device_add(&ofdev->dev); } +/** + * of_dma_configure - Setup DMA configuration + * @dev: Device to apply DMA configuration + * @np: ptr to of node having dma configuration + * + * Try to get devices's DMA configuration from DT and update it + * accordingly. + * + * In case if platform code need to use own special DMA configuration,it + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event + * to fix up DMA configuration. + */ +void of_dma_configure(struct device *dev, struct device_node *np) +{ + u64 dma_addr, paddr, size; + int ret; + bool coherent; + unsigned long offset; + struct iommu_ops *iommu; + + /* + * Set default dma-mask to 32 bit. Drivers are expected to setup + * the correct supported dma_mask. + */ + dev->coherent_dma_mask = DMA_BIT_MASK(32); + + /* + * Set it to coherent_dma_mask by default if the architecture + * code has not set it. + */ + if (!dev->dma_mask) + dev->dma_mask = &dev->coherent_dma_mask; + + ret = of_dma_get_range(np, &dma_addr, &paddr, &size); + if (ret < 0) { + dma_addr = offset = 0; + size = dev->coherent_dma_mask + 1; + } else { + offset = PFN_DOWN(paddr - dma_addr); + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); + } + dev->dma_pfn_offset = offset; + + coherent = of_dma_is_coherent(np); + dev_dbg(dev, "device is%sdma coherent\n", + coherent ? " " : " not "); + + iommu = of_iommu_configure(dev, np); + dev_dbg(dev, "device is%sbehind an iommu\n", + iommu ? " " : " not "); + + arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); +} +EXPORT_SYMBOL_GPL(of_dma_configure); + int of_device_register(struct platform_device *pdev) { device_initialize(&pdev->dev); diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 7675b79..6403501 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -19,7 +19,6 @@ #include <linux/slab.h> #include <linux/of_address.h> #include <linux/of_device.h> -#include <linux/of_iommu.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> @@ -150,59 +149,6 @@ struct platform_device *of_device_alloc(struct device_node *np, } EXPORT_SYMBOL(of_device_alloc); -/** - * of_dma_configure - Setup DMA configuration - * @dev: Device to apply DMA configuration - * - * Try to get devices's DMA configuration from DT and update it - * accordingly. - * - * In case if platform code need to use own special DMA configuration,it - * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event - * to fix up DMA configuration. - */ -static void of_dma_configure(struct device *dev) -{ - u64 dma_addr, paddr, size; - int ret; - bool coherent; - unsigned long offset; - struct iommu_ops *iommu; - - /* - * Set default dma-mask to 32 bit. Drivers are expected to setup - * the correct supported dma_mask. - */ - dev->coherent_dma_mask = DMA_BIT_MASK(32); - - /* - * Set it to coherent_dma_mask by default if the architecture - * code has not set it. - */ - if (!dev->dma_mask) - dev->dma_mask = &dev->coherent_dma_mask; - - ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); - if (ret < 0) { - dma_addr = offset = 0; - size = dev->coherent_dma_mask; - } else { - offset = PFN_DOWN(paddr - dma_addr); - dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); - } - dev->dma_pfn_offset = offset; - - coherent = of_dma_is_coherent(dev->of_node); - dev_dbg(dev, "device is%sdma coherent\n", - coherent ? " " : " not "); - - iommu = of_iommu_configure(dev, dev->of_node); - dev_dbg(dev, "device is%sbehind an iommu\n", - iommu ? " " : " not "); - - arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent); -} - static void of_dma_deconfigure(struct device *dev) { arch_teardown_dma_ops(dev); @@ -236,7 +182,7 @@ static struct platform_device *of_platform_device_create_pdata( dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; - of_dma_configure(&dev->dev); + of_dma_configure(&dev->dev, dev->dev.of_node); if (of_device_add(dev) != 0) { of_dma_deconfigure(&dev->dev); @@ -299,7 +245,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node, dev_set_name(&dev->dev, "%s", bus_id); else of_device_make_bus_id(&dev->dev); - of_dma_configure(&dev->dev); + of_dma_configure(&dev->dev, dev->dev.of_node); /* Allow the HW Peripheral ID to be overridden */ prop = of_get_property(node, "arm,primecell-periphid", NULL); diff --git a/include/linux/of_device.h b/include/linux/of_device.h index ef37021..c661496 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -53,6 +53,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) return of_node_get(cpu_dev->of_node); } +void of_dma_configure(struct device *dev, struct device_node *np); #else /* CONFIG_OF */ static inline int of_driver_match_device(struct device *dev, @@ -90,6 +91,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) { return NULL; } +void of_dma_configure(struct device *dev, struct device_node *np) { } #endif /* CONFIG_OF */ #endif /* _LINUX_OF_DEVICE_H */
Move of_dma_configure() to device.c so that same function can be re-used for PCI devices to obtain DMA configuration from DT. Also add a second argument so that for PCI, DT node of root bus host bridge can be used to obtain the DMA configuration for the slave PCI device. Additionally fix the dma-range size when the DT attribute is missing. i.e set size to dev->coherent_dma_mask + 1 instead of dev->coherent_dma_mask. Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> --- drivers/of/device.c | 58 +++++++++++++++++++++++++++++++++++++++++++++ drivers/of/platform.c | 58 ++------------------------------------------- include/linux/of_device.h | 2 ++ 3 files changed, 62 insertions(+), 56 deletions(-)