MCI bus-width host caps via device tree don't work properly for dw and imx

MCI bus-width host caps via device tree don't work properly for dw and imx

[Trent Piepho]

I found my 4-bit connected eMMC chip wasn't working because barebox was
trying to run it in 8-bit mode.  I had the proper bus-width property in
the device tree, but this doesn't work.

The problem is that mci_of_parse() uses bus-width to _add_ 8-bit and/or
4-bit caps to the host.  It doesn't remove any host_caps the driver
already has set.  So if the driver supports 8-bit but the DT bus-width
is 4, you still get a driver that thinks 8-bit will work.

You can see this in dw_mmc.c, dw_mmc_probe() sets:
host->mci.host_caps = MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA
then later
mci_of_parse(&host->mci);
return mci_register(&host->mci);

The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
any bus-width host_caps in the driver and depend on the DT or
platform_data to have this info.

With drivers split 50/50, which is the right way?  Don't have the driver
report what bits it supports and depend on the device tree to have that
information?  Or have of_mci_parse remove widths that aren't indicated
as supported in the DT, so that the DT can remove widths the hardware
indicates it supports.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: MCI bus-width host caps via device tree don't work properly for dw and imx

[Sascha Hauer]

Hi Trent,

On Fri, Nov 13, 2015 at 12:36:22AM +0000, Trent Piepho wrote:
> I found my 4-bit connected eMMC chip wasn't working because barebox was
> trying to run it in 8-bit mode.  I had the proper bus-width property in
> the device tree, but this doesn't work.
> 
> The problem is that mci_of_parse() uses bus-width to _add_ 8-bit and/or
> 4-bit caps to the host.  It doesn't remove any host_caps the driver
> already has set.  So if the driver supports 8-bit but the DT bus-width
> is 4, you still get a driver that thinks 8-bit will work.
> 
> You can see this in dw_mmc.c, dw_mmc_probe() sets:
> host->mci.host_caps = MMC_CAP_4_BIT_DATA | MMC_CAP_8_BIT_DATA
> then later
> mci_of_parse(&host->mci);
> return mci_register(&host->mci);
> 
> The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
> the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
> mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
> any bus-width host_caps in the driver and depend on the DT or
> platform_data to have this info.
> 
> With drivers split 50/50, which is the right way?  Don't have the driver
> report what bits it supports and depend on the device tree to have that
> information?  Or have of_mci_parse remove widths that aren't indicated
> as supported in the DT, so that the DT can remove widths the hardware
> indicates it supports.

I think the resulting flags should be the subset of what all components
can do, that is (driver_flags & devicetree_flags & card_flags).

So mci_of_parse() should IMO clear the MMC_CAP_8_BIT_DATA flag if the
bus-width property is set to 4.

You should encounter the same problem under Linux, right?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: MCI bus-width host caps via device tree don't work properly for dw and imx

[Trent Piepho]

On Fri, 2015-11-13 at 11:28 +0100, Sascha Hauer wrote:
> > The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
> > the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
> > mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
> > any bus-width host_caps in the driver and depend on the DT or
> > platform_data to have this info.
> > 
> > With drivers split 50/50, which is the right way?  Don't have the driver
> > report what bits it supports and depend on the device tree to have that
> > information?  Or have of_mci_parse remove widths that aren't indicated
> > as supported in the DT, so that the DT can remove widths the hardware
> > indicates it supports.
> 
> I think the resulting flags should be the subset of what all components
> can do, that is (driver_flags & devicetree_flags & card_flags).
> 
> So mci_of_parse() should IMO clear the MMC_CAP_8_BIT_DATA flag if the
> bus-width property is set to 4.
> 
> You should encounter the same problem under Linux, right?

Don't have the problem on Linux.  The OF parsing works the same way, but
the dw driver stats with 0 bus-width caps, like how mxs and tegra-sdmmc
work.

The linux dw_mmc driver allows "sub-drivers" to supply caps, and the
Exynos4/5 driver adds CAP_8_BIT.  Which is probably broken, as it
doesn't add 4_BIT as well and will have the same problem with the device
tree not being able to remove bus widths.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: MCI bus-width host caps via device tree don't work properly for dw and imx

[Sascha Hauer]

On Fri, Nov 13, 2015 at 06:54:14PM +0000, Trent Piepho wrote:
> On Fri, 2015-11-13 at 11:28 +0100, Sascha Hauer wrote:
> > > The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
> > > the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
> > > mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
> > > any bus-width host_caps in the driver and depend on the DT or
> > > platform_data to have this info.
> > > 
> > > With drivers split 50/50, which is the right way?  Don't have the driver
> > > report what bits it supports and depend on the device tree to have that
> > > information?  Or have of_mci_parse remove widths that aren't indicated
> > > as supported in the DT, so that the DT can remove widths the hardware
> > > indicates it supports.
> > 
> > I think the resulting flags should be the subset of what all components
> > can do, that is (driver_flags & devicetree_flags & card_flags).
> > 
> > So mci_of_parse() should IMO clear the MMC_CAP_8_BIT_DATA flag if the
> > bus-width property is set to 4.
> > 
> > You should encounter the same problem under Linux, right?
> 
> Don't have the problem on Linux.  The OF parsing works the same way, but
> the dw driver stats with 0 bus-width caps, like how mxs and tegra-sdmmc
> work.
> 
> The linux dw_mmc driver allows "sub-drivers" to supply caps, and the
> Exynos4/5 driver adds CAP_8_BIT.  Which is probably broken, as it
> doesn't add 4_BIT as well and will have the same problem with the device
> tree not being able to remove bus widths.

Ok, it seems the rationale behind the MMC_CAP_x_BIT_DATA is another one
than thought it is. The host driver cannot know how a card is actually
connected, so it should rely purely on the device tree bus-width
property. So dw_mmc.c should start with host->mci.host_caps = 0 in the
device tree case.
If this is changed we'll also have to fix the platform_data for dw_mmc,
it now needs a bus_width field to let ./arch/arm/mach-socfpga/xload.c
work in 4 bit mode.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: MCI bus-width host caps via device tree don't work properly for dw and imx

[Trent Piepho]

On Mon, 2015-11-16 at 07:32 +0100, Sascha Hauer wrote:
> On Fri, Nov 13, 2015 at 06:54:14PM +0000, Trent Piepho wrote:
> > On Fri, 2015-11-13 at 11:28 +0100, Sascha Hauer wrote:
> > > > The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
> > > > the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
> > > > mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
> > > > any bus-width host_caps in the driver and depend on the DT or
> > > > platform_data to have this info.
> > > > 
> > > > With drivers split 50/50, which is the right way?  Don't have the driver
> > > > report what bits it supports and depend on the device tree to have that
> > > > information?  Or have of_mci_parse remove widths that aren't indicated
> > > > as supported in the DT, so that the DT can remove widths the hardware
> > > > indicates it supports.
> > > 
> > > I think the resulting flags should be the subset of what all components
> > > can do, that is (driver_flags & devicetree_flags & card_flags).
> > > 
> > > So mci_of_parse() should IMO clear the MMC_CAP_8_BIT_DATA flag if the
> > > bus-width property is set to 4.
> > > 
> > > You should encounter the same problem under Linux, right?
> > 
> > Don't have the problem on Linux.  The OF parsing works the same way, but
> > the dw driver stats with 0 bus-width caps, like how mxs and tegra-sdmmc
> > work.
> > 
> > The linux dw_mmc driver allows "sub-drivers" to supply caps, and the
> > Exynos4/5 driver adds CAP_8_BIT.  Which is probably broken, as it
> > doesn't add 4_BIT as well and will have the same problem with the device
> > tree not being able to remove bus widths.
> 
> Ok, it seems the rationale behind the MMC_CAP_x_BIT_DATA is another one
> than thought it is. The host driver cannot know how a card is actually
> connected, so it should rely purely on the device tree bus-width
> property. So dw_mmc.c should start with host->mci.host_caps = 0 in the
> device tree case.
> If this is changed we'll also have to fix the platform_data for dw_mmc,
> it now needs a bus_width field to let ./arch/arm/mach-socfpga/xload.c
> work in 4 bit mode.

It seems that no one could agree how it should work or just didn't think
much about it.  Many host drivers in the kernel set CAP_X_BIT. It seems
to be pretty normal for non-OF drivers to just hard code the width in
the driver instead of using platform_data for it.  But there are a
number of OF drivers that also set widths which can't be removed.

I think the way I would have done it would be to have the driver set the
default width(s) which are used if nothing is in the device tree.  If a
width is in the device tree, it overrides the driver provided width.
That way one gets the largest supported width with no device tree or
platform data and only need to put in a bus-width property if the
hardware is connected differently.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: MCI bus-width host caps via device tree don't work properly for dw and imx

[Sascha Hauer]

On Mon, Nov 16, 2015 at 06:26:30PM +0000, Trent Piepho wrote:
> On Mon, 2015-11-16 at 07:32 +0100, Sascha Hauer wrote:
> > On Fri, Nov 13, 2015 at 06:54:14PM +0000, Trent Piepho wrote:
> > > On Fri, 2015-11-13 at 11:28 +0100, Sascha Hauer wrote:
> > > > > The mci_of_parse call will not remove the 4 and 8 bits caps.  There is
> > > > > the same problem in imx-esdhc.c.  The other two users of mci_of_parse,
> > > > > mxs.c and tegra-sdmmc.c, don't have this problem, since they don't set
> > > > > any bus-width host_caps in the driver and depend on the DT or
> > > > > platform_data to have this info.
> > > > > 
> > > > > With drivers split 50/50, which is the right way?  Don't have the driver
> > > > > report what bits it supports and depend on the device tree to have that
> > > > > information?  Or have of_mci_parse remove widths that aren't indicated
> > > > > as supported in the DT, so that the DT can remove widths the hardware
> > > > > indicates it supports.
> > > > 
> > > > I think the resulting flags should be the subset of what all components
> > > > can do, that is (driver_flags & devicetree_flags & card_flags).
> > > > 
> > > > So mci_of_parse() should IMO clear the MMC_CAP_8_BIT_DATA flag if the
> > > > bus-width property is set to 4.
> > > > 
> > > > You should encounter the same problem under Linux, right?
> > > 
> > > Don't have the problem on Linux.  The OF parsing works the same way, but
> > > the dw driver stats with 0 bus-width caps, like how mxs and tegra-sdmmc
> > > work.
> > > 
> > > The linux dw_mmc driver allows "sub-drivers" to supply caps, and the
> > > Exynos4/5 driver adds CAP_8_BIT.  Which is probably broken, as it
> > > doesn't add 4_BIT as well and will have the same problem with the device
> > > tree not being able to remove bus widths.
> > 
> > Ok, it seems the rationale behind the MMC_CAP_x_BIT_DATA is another one
> > than thought it is. The host driver cannot know how a card is actually
> > connected, so it should rely purely on the device tree bus-width
> > property. So dw_mmc.c should start with host->mci.host_caps = 0 in the
> > device tree case.
> > If this is changed we'll also have to fix the platform_data for dw_mmc,
> > it now needs a bus_width field to let ./arch/arm/mach-socfpga/xload.c
> > work in 4 bit mode.
> 
> It seems that no one could agree how it should work or just didn't think
> much about it.  Many host drivers in the kernel set CAP_X_BIT. It seems
> to be pretty normal for non-OF drivers to just hard code the width in
> the driver instead of using platform_data for it.  But there are a
> number of OF drivers that also set widths which can't be removed.
> 
> I think the way I would have done it would be to have the driver set the
> default width(s) which are used if nothing is in the device tree.  If a
> width is in the device tree, it overrides the driver provided width.
> That way one gets the largest supported width with no device tree or
> platform data and only need to put in a bus-width property if the
> hardware is connected differently.

The device tree is always right. If we have 4bit bus width specified in
device tree then for sure we cannot support 8bit. Clearing the 8bit
flag in mci_of_parse() when we have 4bit in device tree should not be
wrong, so let's do it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox