On Tue, Dec 20, 2016 at 01:10:56PM +0100, Ulf Hansson wrote:
> Hi Jan,
>
> On 19 December 2016 at 13:15, Jan Glauber <jglauber@xxxxxxxxxx> wrote:
> > While this patch series seems to be somehow overdue, in the meantime the
> > same MMC unit was re-used on Cavium's ThunderX SOC so our interest in making
> > progress upstreaming this driver has doubled now...
> >
> > Glancing over the history of the series I think most of the high-level
> > comments should be adressed by now (like DTS representation of the
> > multiple slots). I've added some new features for the ARM64 port
> > and in the process re-wrote parts of the driver and split it into smaller,
> > hopefully easier to review parts.
>
> I only had a quick review, but the overall impression is that it's
> getting far better. Here follows my summary.
>
> 1) I intend to especially look at DTS representation for the slot
> nodes, to make sure we have a good solution. Allow me to get back on
> this.
>
> 2) I don't like how you have named files, as it doesn't express the
> obvious relationship between the core library and the drivers. I would
> rather see something similar to dw_mmc or sdhci.
I've prefixed all files with "cavium" and adjusted names, incl. Kconfig
names.
> 3) Related to 2), I would also like to have a prefix of the commit
> messages which express the relationships. Again follow dw_mmc/sdhci.
OK.
> 4) GPIO powers should be modelled as GPIO regulators. I believe we
> have discussed this earlier as well (I don't really recall in detail
> about the last things). It gives us the opportunity to via the
> regulator framework to find out the supported voltage levels. This is
> the generic method which is used by mmc drivers, you need to adopt to
> this as well.
I've added a fixed regulator to DT:
vcc_3v3: regulator-vcc_3v3 {
compatible = "regulator-fixed";
regulator-name = "VCC_3V3";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
gpio = <&gpio_6_0 8 0>;
/* enable-gpio = <&gpio_6_0 8 0>; */
enable-active-high;
};
This seems to enable the gpio. Is this sufficient or do I need the
gpio-regulator?
> 5) Please reorder the series so the DT bindings doc change comes
> first. I need an ack from the DT maintainer for it.
OK.
> 6) The most important feedback:
> This driver has been posted in many versions by now. Perhaps I could
> have been more responsive throughout the attempts, I apologize for
> that. On the other hand, you seems to have a round robin schedule for
> whom that sends a new version. :-) That makes me wonder about your
> support in the maintenance phase. I hope my concern is wrong, but how
> about that you point out a responsible maintainer? Especially since
> this seems to become a family of Cavium variants, it would help me if
> I could rely on someone providing acks for future changes. Would you
> be able to accept that role?
>
> >
> > In porting the driver to arm64 I run into some issues.
> >
> > 1. mmc_parse_of is not capable of supporting multiple slots behind one
> > controller. On arm64 the host controller is presented as one PCI device.
> > There are no devices per slot as with the platform variant, so I
> > needed to create dummy devices to make mmc_parse_of work.
> > IMHO it would be cleaner if mmc_parse_of could be extended to cover
> > the multiple slots case.
>
> Yes. I agree that this make sense!
> Seems like we could try to make use of the struct device_node instead
> of the struct device.
>
> I will try to come up with an idea, I keep you posted.
>
> >
> > 2. Without setting MMC_CAP_1_8V_DDR DDR mode is not usable for eMMC.
> > I would prefer to introduce a new cap flag, MMC_CAP_3_3V_DDR,
> > if possible. Currently I need to add "mmc-ddr-1_8v" to DTS,
> > which seems odd for a 3.3v only host controller.
>
> This keep coming back. Both DT bindings and changing to the mmc core
> has been posted.
>
> Allow me to help out and re-post a new series. You can build on top of
> them - I will keep you on cc.
Any news here? Can you give me a pointer?
> >
> > 3. Because the slots share the host controller using the
> > "full-pwr-cycle" attribute turned out to not work well.
> > I'm not 100% sure just ignoring the attribute is correct.
>
> The full-pwr-cycle shall be set whether you are able to power cycle
> the *card*. So this binding should be a part of each slot/child node -
> if the host supports it.
>
> >
> > For the driver to work GPIO support is required, the GPIO driver is
> > not yet available upstream. Therefore, for the time being I removed
> > the GPIO dependency from Kconfig.
>
> Is this is about the GPIO powers or also GPIO card detect?
>
> Anyway, I am fine with not depending on GPIO Kconfig.
>
Meanwhile the GPIO driver was posted here:
https://lkml.org/lkml/2017/1/6/985
> [...]
>
> Kind regards
> Uffe
Thanks for the review,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
