On 07/23/2018 05:16 PM, Robin Murphy wrote:
> Now that we can track upstream DMA constraints properly with
> bus_dma_mask instead of trying (and failing) to maintain it in
> coherent_dma_mask, it doesn't make much sense for the firmware code to
> be touching the latter at all. It's merely papering over bugs wherein a
> driver has failed to call dma_set_coherent_mask() *and* the bus code has
> not initialised any default value.
>
> We don't really want to encourage more drivers coercing dma_mask so
> we'll continue to fix that up if necessary, but add a warning to help
> flush out any such buggy bus code that remains.
>
> CC: Rob Herring <robh+dt@xxxxxxxxxx>
> CC: Frank Rowand <frowand.list@xxxxxxxxx>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> drivers/of/device.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 0d39633e8545..5957cd4fa262 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -127,20 +127,20 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
> }
>
> /*
> - * Set default coherent_dma_mask to 32 bit. Drivers are expected to
> - * setup the correct supported mask.
> + * If @dev is expected to be DMA-capable then the bus code that created
> + * it should have initialised its dma_mask pointer by this point. For
> + * now, we'll continue the legacy behaviour of coercing it to the
> + * coherent mask if not, but we'll no longer do so quietly.
> */
> - if (!dev->coherent_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)
> + if (!dev->dma_mask) {
> + dev_warn(dev, "DMA mask not set\n");
> dev->dma_mask = &dev->coherent_dma_mask;
> + }
>
> - if (!size)
> + if (!size && dev->coherent_dma_mask)
> size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> + else if (!size)
> + size = 1ULL << 32;
>
> dev->dma_pfn_offset = offset;
>
>
the result of this change is pretty strange as for me :(
Resulted code:
/*
* If @dev is expected to be DMA-capable then the bus code that created
* it should have initialised its dma_mask pointer by this point. For
* now, we'll continue the legacy behaviour of coercing it to the
* coherent mask if not, but we'll no longer do so quietly.
*/
if (!dev->dma_mask) {
dev_warn(dev, "DMA mask not set\n");
dev->dma_mask = &dev->coherent_dma_mask;
^this will always produce warning in case of platform-bus or if there are no bus driver.
even if DT contains correct definition of dma-range
}
if (!size && dev->coherent_dma_mask)
^ coherent_dma_mask is zero, so size will not be calculated
size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
else if (!size)
size = 1ULL << 32;
dev->dma_pfn_offset = offset;
/*
* Limit coherent and dma mask based on size and default mask
* set by the driver.
*/
mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
dev->bus_dma_mask = mask;
dev->coherent_dma_mask &= mask;
^^ if nobody set coherent_dma_mask before it will stay null forever unless drivers
will overwrite it. Again even if DT has correct definition for dma-range.
*dev->dma_mask &= mask;
coherent = of_dma_is_coherent(np);
dev_dbg(dev, "device is%sdma coherent\n",
coherent ? " " : " not ");
--
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
