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

[Jan Glauber]

On Thu, Jan 19, 2017 at 09:47:33AM -0800, David Daney wrote:
> On 01/19/2017 06:50 AM, Jan Glauber wrote:
> [...]
> >
> >>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";
> 
> 
> Very minor point not really directly related to the MMC driver:
> "VCC_3V3" implies a general purpose supply for many things on the
> board.  This is not the case for these boards.  The "regulator"
> controls power only to eMMC and SD devices, so a name that conveys
> that function should be invented.

OK, I'll rename it to:

mmc_supply_3v3: mmc_supply_3v3 {
compatible = "regulator-fixed";
regulator-name = "mmc_supply_3v3";
[...]


> Actually on some boards GPIO 8 doesn't even control a regulator, but
> instead only activates a bus isolator on the control and data
> signals to the eMMC and SD devices.  In this case we would be using
> the regulator framework only because the code is already there and
> it happens to work, not because we actually have a regulator.
> 
> >regulator-min-microvolt = <330>;
> >regulator-max-microvolt = <330>;
> >
> >gpio = <_6_0 8 0>;
> >/* enable-gpio = <_6_0 8 0>; */
> >enable-active-high;
> >};
> >
> >This seems to enable the gpio. Is this sufficient or do I need the
> >gpio-regulator?
> >
> 
> Does the "regulator-fixed" allow us to properly turn it on and off?

Yes, I've disabled all gpio calls from our mmc driver, with only the gpio 
settings in the regulator entry I get:

root@sff:~# cat /sys/kernel/debug/gpio 
gpiochip0: GPIOs 464-511, parent: pci/:00:06.0, gpio_thunderx:
 gpio-472 (|mmc_supply_3v3  ) out hi 

So the regulator driver enables GPIO 8 as it should.

> If not, we may have to switch to "gpio-regulator".  Which ever is
> simplest should be used.

"regulator-fixed" seems sufficient to me as we don't need the additional
power states that "gpio-regulator" supports.

> [...]

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

[Jan Glauber]

On Thu, Jan 19, 2017 at 09:47:33AM -0800, David Daney wrote:
> On 01/19/2017 06:50 AM, Jan Glauber wrote:
> [...]
> >
> >>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";
> 
> 
> Very minor point not really directly related to the MMC driver:
> "VCC_3V3" implies a general purpose supply for many things on the
> board.  This is not the case for these boards.  The "regulator"
> controls power only to eMMC and SD devices, so a name that conveys
> that function should be invented.

OK, I'll rename it to:

mmc_supply_3v3: mmc_supply_3v3 {
compatible = "regulator-fixed";
regulator-name = "mmc_supply_3v3";
[...]


> Actually on some boards GPIO 8 doesn't even control a regulator, but
> instead only activates a bus isolator on the control and data
> signals to the eMMC and SD devices.  In this case we would be using
> the regulator framework only because the code is already there and
> it happens to work, not because we actually have a regulator.
> 
> >regulator-min-microvolt = <330>;
> >regulator-max-microvolt = <330>;
> >
> >gpio = <_6_0 8 0>;
> >/* enable-gpio = <_6_0 8 0>; */
> >enable-active-high;
> >};
> >
> >This seems to enable the gpio. Is this sufficient or do I need the
> >gpio-regulator?
> >
> 
> Does the "regulator-fixed" allow us to properly turn it on and off?

Yes, I've disabled all gpio calls from our mmc driver, with only the gpio 
settings in the regulator entry I get:

root@sff:~# cat /sys/kernel/debug/gpio 
gpiochip0: GPIOs 464-511, parent: pci/:00:06.0, gpio_thunderx:
 gpio-472 (|mmc_supply_3v3  ) out hi 

So the regulator driver enables GPIO 8 as it should.

> If not, we may have to switch to "gpio-regulator".  Which ever is
> simplest should be used.

"regulator-fixed" seems sufficient to me as we don't need the additional
power states that "gpio-regulator" supports.

> [...]

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

[Ulf Hansson]

[...]

> I've added a fixed regulator to DT:
>
> vcc_3v3: regulator-vcc_3v3 {
> compatible = "regulator-fixed";
> regulator-name = "VCC_3V3";
> regulator-min-microvolt = <330>;
> regulator-max-microvolt = <330>;
>
> gpio = <_6_0 8 0>;
> /* enable-gpio = <_6_0 8 0>; */
> enable-active-high;
> };
>
> This seems to enable the gpio. Is this sufficient or do I need the
> gpio-regulator?

Looks good to me.

[...]

>> > 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?

For 1), I need some more time. Feel free to try it out your self.

For 2), I am working on it. Likely in the beginning of next week I
will post something.

[...]

Kind regards
Uffe

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

[Ulf Hansson]

[...]

> I've added a fixed regulator to DT:
>
> vcc_3v3: regulator-vcc_3v3 {
> compatible = "regulator-fixed";
> regulator-name = "VCC_3V3";
> regulator-min-microvolt = <330>;
> regulator-max-microvolt = <330>;
>
> gpio = <_6_0 8 0>;
> /* enable-gpio = <_6_0 8 0>; */
> enable-active-high;
> };
>
> This seems to enable the gpio. Is this sufficient or do I need the
> gpio-regulator?

Looks good to me.

[...]

>> > 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?

For 1), I need some more time. Feel free to try it out your self.

For 2), I am working on it. Likely in the beginning of next week I
will post something.

[...]

Kind regards
Uffe

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

[David Daney]

On 01/19/2017 06:50 AM, Jan Glauber wrote:
[...]



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";



Very minor point not really directly related to the MMC driver: 
"VCC_3V3" implies a general purpose supply for many things on the board. 
 This is not the case for these boards.  The "regulator" controls power 
only to eMMC and SD devices, so a name that conveys that function should 
be invented.


Actually on some boards GPIO 8 doesn't even control a regulator, but 
instead only activates a bus isolator on the control and data signals to 
the eMMC and SD devices.  In this case we would be using the regulator 
framework only because the code is already there and it happens to work, 
not because we actually have a regulator.



regulator-min-microvolt = <330>;
regulator-max-microvolt = <330>;

gpio = <_6_0 8 0>;
/* enable-gpio = <_6_0 8 0>; */
enable-active-high;
};

This seems to enable the gpio. Is this sufficient or do I need the
gpio-regulator?



Does the "regulator-fixed" allow us to properly turn it on and off?

If not, we may have to switch to "gpio-regulator".  Which ever is 
simplest should be used.


[...]

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

[David Daney]

On 01/19/2017 06:50 AM, Jan Glauber wrote:
[...]



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";



Very minor point not really directly related to the MMC driver: 
"VCC_3V3" implies a general purpose supply for many things on the board. 
 This is not the case for these boards.  The "regulator" controls power 
only to eMMC and SD devices, so a name that conveys that function should 
be invented.


Actually on some boards GPIO 8 doesn't even control a regulator, but 
instead only activates a bus isolator on the control and data signals to 
the eMMC and SD devices.  In this case we would be using the regulator 
framework only because the code is already there and it happens to work, 
not because we actually have a regulator.



regulator-min-microvolt = <330>;
regulator-max-microvolt = <330>;

gpio = <_6_0 8 0>;
/* enable-gpio = <_6_0 8 0>; */
enable-active-high;
};

This seems to enable the gpio. Is this sufficient or do I need the
gpio-regulator?



Does the "regulator-fixed" allow us to properly turn it on and off?

If not, we may have to switch to "gpio-regulator".  Which ever is 
simplest should be used.


[...]

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

[Jan Glauber]

On Tue, Dec 20, 2016 at 01:10:56PM +0100, Ulf Hansson wrote:
> Hi Jan,
> 
> On 19 December 2016 at 13:15, Jan Glauber  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 = <330>;
regulator-max-microvolt = <330>;

gpio = <_6_0 8 0>;
/* enable-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 

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

[Jan Glauber]

On Tue, Dec 20, 2016 at 01:10:56PM +0100, Ulf Hansson wrote:
> Hi Jan,
> 
> On 19 December 2016 at 13:15, Jan Glauber  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 = <330>;
regulator-max-microvolt = <330>;

gpio = <_6_0 8 0>;
/* enable-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

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

[Ulf Hansson]

>
> Hi Uffe,
>
> thanks for your feedback! To answer only point 6 for now, I was not to
> keen on being the next poster of this series ;- Nevertheless, I'm
> comitted to keep working on this driver to bring it finally upstream and
> also to maintain it in the future. To make this clear I'll add myself
> and possibly also David or Steven to MAINTAINERS.

Thanks! That shows your commitment.

Kind regards
Uffe

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

[Ulf Hansson]

>
> Hi Uffe,
>
> thanks for your feedback! To answer only point 6 for now, I was not to
> keen on being the next poster of this series ;- Nevertheless, I'm
> comitted to keep working on this driver to bring it finally upstream and
> also to maintain it in the future. To make this clear I'll add myself
> and possibly also David or Steven to MAINTAINERS.

Thanks! That shows your commitment.

Kind regards
Uffe

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

[Jan Glauber]

On Tue, Dec 20, 2016 at 01:10:56PM +0100, Ulf Hansson wrote:
> Hi Jan,
> 
> On 19 December 2016 at 13:15, Jan Glauber  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.
> 
> 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.
> 
> 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.
> 
> 5) Please reorder the series so the DT bindings doc change comes
> first. I need an ack from the DT maintainer for it.
> 
> 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?

Hi Uffe,

thanks for your feedback! To answer only point 6 for now, I was not to
keen on being the next poster of this series ;- Nevertheless, I'm
comitted to keep working on this driver to bring it finally upstream and
also to maintain it in the future. To make this clear I'll add myself
and possibly also David or Steven to MAINTAINERS.

Cheers,
Jan

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

[Jan Glauber]

On Tue, Dec 20, 2016 at 01:10:56PM +0100, Ulf Hansson wrote:
> Hi Jan,
> 
> On 19 December 2016 at 13:15, Jan Glauber  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.
> 
> 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.
> 
> 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.
> 
> 5) Please reorder the series so the DT bindings doc change comes
> first. I need an ack from the DT maintainer for it.
> 
> 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?

Hi Uffe,

thanks for your feedback! To answer only point 6 for now, I was not to
keen on being the next poster of this series ;- Nevertheless, I'm
comitted to keep working on this driver to bring it finally upstream and
also to maintain it in the future. To make this clear I'll add myself
and possibly also David or Steven to MAINTAINERS.

Cheers,
Jan

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

[Ulf Hansson]

Hi Jan,

On 19 December 2016 at 13:15, Jan Glauber  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.

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.

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.

5) Please reorder the series so the DT bindings doc change comes
first. I need an ack from the DT maintainer for it.

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.

>
> 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.

[...]

Kind regards
Uffe

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

[Ulf Hansson]

Hi Jan,

On 19 December 2016 at 13:15, Jan Glauber  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.

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.

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.

5) Please reorder the series so the DT bindings doc change comes
first. I need an ack from the DT maintainer for it.

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.

>
> 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.

[...]

Kind regards
Uffe

[PATCH v10 0/8] Cavium MMC driver

[Jan Glauber]

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.

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.

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.

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.

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.

Feedback welcome!

Changes to v9:
- Split into several patches for easier review
- Re-factor code into smaller functions
- Introduce callback functions for platform specific code
- Remove some dead code
- Remove lots of type casts
- Use NSEC_PER_SEC
- Invert if's to reduce code indentation
- Remove host->linear_buf for now, maybe we can use MMC bounce buffers feature 
instead
- Use dma_[un]map_sg and length/address accessors instead of direct access
- Set DMA mask
- Add scatter-gather support
- Check for switch errors
- Wait until switch is done
- Enable DDR support for eMMC
- Post CMD23 capability
- Add pr_debug logs
- Split lock acquire from switch_to
- Sort include headers
- Fix code indentation and some checkpatch errors
- Fix includes for octeon specific file
- Fixed modular build on Octeon
- Fail fast on CRC errors (Steven)
- Document devicetree bindings

Cheers,
Jan



Jan Glauber (8):
  mmc: cavium: Add core MMC driver for Cavium SOCs
  mmc: octeon: Add MMC platform driver for Octeon SOCs
  mmc: octeon: Work-around hardware bug on cn6xxx and cnf7xxx
  mmc: octeon: Add support for Octeon cn7890
  mmc: thunderx: Add MMC PCI driver for ThunderX SOCs
  mmc: thunderx: Add scatter-gather DMA support
  mmc: thunderx: Support DDR mode for eMMC devices
  dt-bindings: mmc: Add Cavium SOCs MMC bindings

 .../devicetree/bindings/mmc/octeon-mmc.txt |   59 +
 arch/mips/cavium-octeon/Makefile   |1 +
 arch/mips/cavium-octeon/octeon-mmc.c   |   98 ++
 drivers/mmc/host/Kconfig   |   19 +
 drivers/mmc/host/Makefile  |4 +
 drivers/mmc/host/cavium_core_mmc.c | 1137 
 drivers/mmc/host/cavium_mmc.h  |  425 
 drivers/mmc/host/octeon_platdrv_mmc.c  |  260 +
 drivers/mmc/host/thunderx_pcidrv_mmc.c |  217 
 9 files changed, 2220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/octeon-mmc.txt
 create mode 100644 arch/mips/cavium-octeon/octeon-mmc.c
 create mode 100644 drivers/mmc/host/cavium_core_mmc.c
 create mode 100644 drivers/mmc/host/cavium_mmc.h
 create mode 100644 drivers/mmc/host/octeon_platdrv_mmc.c
 create mode 100644 drivers/mmc/host/thunderx_pcidrv_mmc.c

-- 
2.9.0.rc0.21.g322

[PATCH v10 0/8] Cavium MMC driver

[Jan Glauber]

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.

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.

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.

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.

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.

Feedback welcome!

Changes to v9:
- Split into several patches for easier review
- Re-factor code into smaller functions
- Introduce callback functions for platform specific code
- Remove some dead code
- Remove lots of type casts
- Use NSEC_PER_SEC
- Invert if's to reduce code indentation
- Remove host->linear_buf for now, maybe we can use MMC bounce buffers feature 
instead
- Use dma_[un]map_sg and length/address accessors instead of direct access
- Set DMA mask
- Add scatter-gather support
- Check for switch errors
- Wait until switch is done
- Enable DDR support for eMMC
- Post CMD23 capability
- Add pr_debug logs
- Split lock acquire from switch_to
- Sort include headers
- Fix code indentation and some checkpatch errors
- Fix includes for octeon specific file
- Fixed modular build on Octeon
- Fail fast on CRC errors (Steven)
- Document devicetree bindings

Cheers,
Jan



Jan Glauber (8):
  mmc: cavium: Add core MMC driver for Cavium SOCs
  mmc: octeon: Add MMC platform driver for Octeon SOCs
  mmc: octeon: Work-around hardware bug on cn6xxx and cnf7xxx
  mmc: octeon: Add support for Octeon cn7890
  mmc: thunderx: Add MMC PCI driver for ThunderX SOCs
  mmc: thunderx: Add scatter-gather DMA support
  mmc: thunderx: Support DDR mode for eMMC devices
  dt-bindings: mmc: Add Cavium SOCs MMC bindings

 .../devicetree/bindings/mmc/octeon-mmc.txt |   59 +
 arch/mips/cavium-octeon/Makefile   |1 +
 arch/mips/cavium-octeon/octeon-mmc.c   |   98 ++
 drivers/mmc/host/Kconfig   |   19 +
 drivers/mmc/host/Makefile  |4 +
 drivers/mmc/host/cavium_core_mmc.c | 1137 
 drivers/mmc/host/cavium_mmc.h  |  425 
 drivers/mmc/host/octeon_platdrv_mmc.c  |  260 +
 drivers/mmc/host/thunderx_pcidrv_mmc.c |  217 
 9 files changed, 2220 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/octeon-mmc.txt
 create mode 100644 arch/mips/cavium-octeon/octeon-mmc.c
 create mode 100644 drivers/mmc/host/cavium_core_mmc.c
 create mode 100644 drivers/mmc/host/cavium_mmc.h
 create mode 100644 drivers/mmc/host/octeon_platdrv_mmc.c
 create mode 100644 drivers/mmc/host/thunderx_pcidrv_mmc.c

-- 
2.9.0.rc0.21.g322