Commit cdfee5623290 ("driver core: initialize a default DMA mask for
platform device") initialize the DMA of a platform device. But if the
parent doesn't have a dma_mask set, for example if it's an I2C device,
the dma_mask of the child platform device will be set to zero again.
Which leads to many "DMA mask not set" warnings, if the MFD cell has the
of_compatible property set.
[ 1.877937] sl28cpld-pwm sl28cpld-pwm: DMA mask not set
[ 1.883282] sl28cpld-pwm sl28cpld-pwm.0: DMA mask not set
[ 1.888795] sl28cpld-gpio sl28cpld-gpio: DMA mask not set
Thus don't overwrite the dma_mask of the children. Instead set the
dma_mask of the platform device.
Signed-off-by: Michael Walle <[email protected]>
Suggested-by: Robin Murphy <[email protected]>
---
drivers/mfd/mfd-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd4..e735565969b3 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -138,7 +138,7 @@ static int mfd_add_device(struct device *parent, int id,
pdev->dev.parent = parent;
pdev->dev.type = &mfd_dev_type;
- pdev->dev.dma_mask = parent->dma_mask;
+ pdev->platform_dma_mask = parent->dma_mask ? *parent->dma_mask : 0;
pdev->dev.dma_parms = parent->dma_parms;
pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
--
2.20.1
On Thu, Apr 23, 2020 at 07:45:29PM +0200, Michael Walle wrote:
> Commit cdfee5623290 ("driver core: initialize a default DMA mask for
> platform device") initialize the DMA of a platform device. But if the
> parent doesn't have a dma_mask set, for example if it's an I2C device,
> the dma_mask of the child platform device will be set to zero again.
> Which leads to many "DMA mask not set" warnings, if the MFD cell has the
> of_compatible property set.
I'm wondering why parent doesn't have it.
I remember we have explicit patches in the past for buses such as PCI and AMBA
to set default DMA mask for all physical devices on the respective bus, of
course they can individually override it later.
So, this seems to me a paper over the real issue (absence of default DMA mask
where it's needed) and devices should explicitly define it if they disagree
with default.
If I'm wrong, you really need elaborate commit message much better.
--
With Best Regards,
Andy Shevchenko
On 2020-04-28 1:45 pm, Andy Shevchenko wrote:
> On Thu, Apr 23, 2020 at 07:45:29PM +0200, Michael Walle wrote:
>> Commit cdfee5623290 ("driver core: initialize a default DMA mask for
>> platform device") initialize the DMA of a platform device. But if the
>> parent doesn't have a dma_mask set, for example if it's an I2C device,
>> the dma_mask of the child platform device will be set to zero again.
>> Which leads to many "DMA mask not set" warnings, if the MFD cell has the
>> of_compatible property set.
>
> I'm wondering why parent doesn't have it.
Because the parent isn't on a DMA-capable bus, and thus really shouldn't
have a valid DMA configuration ever.
> I remember we have explicit patches in the past for buses such as PCI and AMBA
> to set default DMA mask for all physical devices on the respective bus, of
> course they can individually override it later.
>
> So, this seems to me a paper over the real issue (absence of default DMA mask
> where it's needed) and devices should explicitly define it if they disagree
> with default.
>
> If I'm wrong, you really need elaborate commit message much better.
The problem here is that MFD children are created as platform devices
(regardless of what their parent is) and assigned an of_node, at which
point they look pretty much indistinguishable from SoC devices created
by the of_platform code, that *do* have to be assumed to be DMA-capable
to prevent ~90% of existing devicetrees from breaking.
Of course the real fundamental issue is the platform bus itself, but
it's way too late to fix that :(
Robin.
On Tue, Apr 28, 2020 at 02:06:20PM +0100, Robin Murphy wrote:
> On 2020-04-28 1:45 pm, Andy Shevchenko wrote:
> > On Thu, Apr 23, 2020 at 07:45:29PM +0200, Michael Walle wrote:
> > > Commit cdfee5623290 ("driver core: initialize a default DMA mask for
> > > platform device") initialize the DMA of a platform device. But if the
> > > parent doesn't have a dma_mask set, for example if it's an I2C device,
> > > the dma_mask of the child platform device will be set to zero again.
> > > Which leads to many "DMA mask not set" warnings, if the MFD cell has the
> > > of_compatible property set.
> >
> > I'm wondering why parent doesn't have it.
>
> Because the parent isn't on a DMA-capable bus, and thus really shouldn't
> have a valid DMA configuration ever.
Then how come a child is DMA capable? MFD takes a physical device node as a
parent and creates one of several children with that device as a parent. DMA
mask is a property of the device which *does DMA*. Obviously a child is not
correct device for that.
Where am I mistaken?
> > I remember we have explicit patches in the past for buses such as PCI and AMBA
> > to set default DMA mask for all physical devices on the respective bus, of
> > course they can individually override it later.
> >
> > So, this seems to me a paper over the real issue (absence of default DMA mask
> > where it's needed) and devices should explicitly define it if they disagree
> > with default.
> >
> > If I'm wrong, you really need elaborate commit message much better.
>
> The problem here is that MFD children are created as platform devices
> (regardless of what their parent is) and assigned an of_node, at which point
> they look pretty much indistinguishable from SoC devices created by the
> of_platform code, that *do* have to be assumed to be DMA-capable to prevent
> ~90% of existing devicetrees from breaking.
>
> Of course the real fundamental issue is the platform bus itself, but it's
> way too late to fix that :(
I don't think it's an issue, rather in model you are describing. Or I miss
something not so obvious.
--
With Best Regards,
Andy Shevchenko
On 2020-04-28 3:29 pm, Andy Shevchenko wrote:
> On Tue, Apr 28, 2020 at 02:06:20PM +0100, Robin Murphy wrote:
>> On 2020-04-28 1:45 pm, Andy Shevchenko wrote:
>>> On Thu, Apr 23, 2020 at 07:45:29PM +0200, Michael Walle wrote:
>>>> Commit cdfee5623290 ("driver core: initialize a default DMA mask for
>>>> platform device") initialize the DMA of a platform device. But if the
>>>> parent doesn't have a dma_mask set, for example if it's an I2C device,
>>>> the dma_mask of the child platform device will be set to zero again.
>>>> Which leads to many "DMA mask not set" warnings, if the MFD cell has the
>>>> of_compatible property set.
>>>
>>> I'm wondering why parent doesn't have it.
>>
>> Because the parent isn't on a DMA-capable bus, and thus really shouldn't
>> have a valid DMA configuration ever.
>
> Then how come a child is DMA capable?
Because it's a platform device, and thanks to decades of legacy we have
to assume that any platform devices *is* DMA capable.
> MFD takes a physical device node as a
> parent and creates one of several children with that device as a parent. DMA
> mask is a property of the device which *does DMA*. Obviously a child is not
> correct device for that.
>
> Where am I mistaken?
In theory you're not, however in practice the driver model doesn't
really give us a nice way to express the necessary subtle distinctions
between this and other similar-looking but fundamentally different
parent-child relationships - if it did, we probably wouldn't need the
whole MFD layer in the first place. The logical ideal would be to create
the children on the same bus as the parent, but as it is doing that
would likely lead to the I2C/SPI/whatever bus code assuming they are
first-class devices and open up a whole new world of problems.
For better or worse, the platform bus is the dumping ground for random
crap, so we just have to deal with all the abstraction breakage that
leaks out of that.
Robin.
>>> I remember we have explicit patches in the past for buses such as PCI and AMBA
>>> to set default DMA mask for all physical devices on the respective bus, of
>>> course they can individually override it later.
>>>
>>> So, this seems to me a paper over the real issue (absence of default DMA mask
>>> where it's needed) and devices should explicitly define it if they disagree
>>> with default.
>>>
>>> If I'm wrong, you really need elaborate commit message much better.
>>
>> The problem here is that MFD children are created as platform devices
>> (regardless of what their parent is) and assigned an of_node, at which point
>> they look pretty much indistinguishable from SoC devices created by the
>> of_platform code, that *do* have to be assumed to be DMA-capable to prevent
>> ~90% of existing devicetrees from breaking.
>>
>> Of course the real fundamental issue is the platform bus itself, but it's
>> way too late to fix that :(
>
> I don't think it's an issue, rather in model you are describing. Or I miss
> something not so obvious.
>
On Tue, Apr 28, 2020 at 03:49:49PM +0100, Robin Murphy wrote:
> For better or worse, the platform bus is the dumping ground for random crap,
> so we just have to deal with all the abstraction breakage that leaks out of
> that.
The reason we're using the platform bus for this is that historically
people were creating buses which were essentially carbon copies of the
platform bus with the name changed and it was felt that rather than
duplicate code it was better to just use platform devices with no MMIO
ranges defined. If there's some assumptions about DMA for platform
devices floating about somewhere it might be reasonable to revisit this
and create a non-DMA variant of platform devices since there is a
meaningful difference.
Am 2020-04-28 17:25, schrieb Mark Brown:
> On Tue, Apr 28, 2020 at 03:49:49PM +0100, Robin Murphy wrote:
>
>> For better or worse, the platform bus is the dumping ground for random
>> crap,
>> so we just have to deal with all the abstraction breakage that leaks
>> out of
>> that.
>
> The reason we're using the platform bus for this is that historically
> people were creating buses which were essentially carbon copies of the
> platform bus with the name changed and it was felt that rather than
> duplicate code it was better to just use platform devices with no MMIO
> ranges defined. If there's some assumptions about DMA for platform
> devices floating about somewhere it might be reasonable to revisit this
> and create a non-DMA variant of platform devices since there is a
> meaningful difference.
Was there any conclusion? Should I keep or drop this patch in the next
version
of this series?
--
-michael