Re: [PATCH v10 0/8] Cavium MMC driver

[Jan Glauber <jan.glauber@caviumnetworks.com>]

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@cavium.com> 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