| Message ID | 1444094562-31165-7-git-send-email-mw@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote: > The newest revisions of A388-GP (v1.5 and higher) support only > DAT3-based card detection, which is enabled by this commit. Hitherto > revisions, without such modification, will be impacted with a broken > card detection - in order to operate the cards have to be present > during kernel boot. Humm. Is this acceptable, breaking old boards? I would say at minimum, there should be a big fat comment at the top of armada-388-gp.dts explaining that this DTS file is broken on v0.0-v1.4. Or we have two .dts files for the 388-gp file, and a dtsi file. Andrew
Hi Andrew, 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: > On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote: >> The newest revisions of A388-GP (v1.5 and higher) support only >> DAT3-based card detection, which is enabled by this commit. Hitherto >> revisions, without such modification, will be impacted with a broken >> card detection - in order to operate the cards have to be present >> during kernel boot. > > Humm. Is this acceptable, breaking old boards? > > I would say at minimum, there should be a big fat comment at the top > of armada-388-gp.dts explaining that this DTS file is broken on > v0.0-v1.4. > > Or we have two .dts files for the 388-gp file, and a dtsi file. > I expected this patch would be controversial, hence I propose a compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However Marvell insisted on HW card detection, because software polling spoils the SD/MMC benchmarks, but this way the user would decide whether to stay with broken-cd or switch to GPIO/DAT3 detection. What do you think? Best regards, Marcin
On Tue, Oct 06, 2015 at 09:02:25AM +0200, Marcin Wojtas wrote: > Hi Andrew, > > 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: > > On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote: > >> The newest revisions of A388-GP (v1.5 and higher) support only > >> DAT3-based card detection, which is enabled by this commit. Hitherto > >> revisions, without such modification, will be impacted with a broken > >> card detection - in order to operate the cards have to be present > >> during kernel boot. > > > > Humm. Is this acceptable, breaking old boards? > > > > I would say at minimum, there should be a big fat comment at the top > > of armada-388-gp.dts explaining that this DTS file is broken on > > v0.0-v1.4. > > > > Or we have two .dts files for the 388-gp file, and a dtsi file. > > > > I expected this patch would be controversial, hence I propose a > compromise: set A388-GP SDHCI to 'broken-cd' by defeault. So for boards < 1.5, you are introducing a regression. It used to work via GPIO. Now that is ignored and it is declared broken. Or am i mis-understanding what broken-cd means, and it actually means, poll it? > However Marvell insisted on HW card detection Marvell does not get to decide what goes into the kernel and what can be deliberately broken. Marvell can make recommendations, provide benchmarks reports, etc. But the community, and ultimately the responsible maintainer decides. > because software polling spoils the SD/MMC benchmarks, but this way > the user would decide whether to stay with broken-cd or switch to > GPIO/DAT3 detection. What do you think? So rather than presenting a solution, please could you list all the different options you can think of, and how they would work for < 1.5 and >= 1.5. One other useful bit of information. How many < 1.5 boards are there, and who has them. Are they internal to Marvell and Free Electrons, or do a lot of people have them? We have thrown away fixes which are requires for A0 stepping SoCs, because very few people have such devices and they can easily replace them for B1. If nearly nobody actually has < 1.5, .... Thanks Andrew
Andrew, 2015-10-06 16:45 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: >> I expected this patch would be controversial, hence I propose a >> compromise: set A388-GP SDHCI to 'broken-cd' by defeault. > > So for boards < 1.5, you are introducing a regression. It used to work > via GPIO. Now that is ignored and it is declared broken. Or am i > mis-understanding what broken-cd means, and it actually means, poll > it? > "broken-cd" property means that the mmc subsystem is polling instead of using hardware detection. >> However Marvell insisted on HW card detection > > Marvell does not get to decide what goes into the kernel and what can > be deliberately broken. Marvell can make recommendations, provide > benchmarks reports, etc. But the community, and ultimately the > responsible maintainer decides. Sure, this is why I submitted the patch to the list in order to get review. Please bear in mind, that I do not work in Marvell and I just wanted to present some justification of the decision, about which I got informed. Personally I think it would be reasonable to keep GPIO detect as is, but maybe I'm lacking some background. > >> because software polling spoils the SD/MMC benchmarks, but this way >> the user would decide whether to stay with broken-cd or switch to >> GPIO/DAT3 detection. What do you think? > > So rather than presenting a solution, please could you list all the > different options you can think of, and how they would work for < 1.5 > and >= 1.5. > We have three options here: - leave GPIO-based card detection and see if in future anyone complains about not working detection - switch to DAT3 detection - yes you're right, that it is a regression for all boards < 1.5 - enable SW polling for detection ("broken-cd") which should satisfy all kind of boards > One other useful bit of information. How many < 1.5 boards are there, > and who has them. Are they internal to Marvell and Free Electrons, or > do a lot of people have them? We have thrown away fixes which are > requires for A0 stepping SoCs, because very few people have such > devices and they can easily replace them for B1. If nearly nobody > actually has < 1.5, .... I have no information, nor an access to such data, I can have only suspicions which cannot be a reference in any way. Best regards, Marcin
Hi, On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote: > Hi Andrew, > > 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: >> On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote: >>> The newest revisions of A388-GP (v1.5 and higher) support only >>> DAT3-based card detection, which is enabled by this commit. Hitherto >>> revisions, without such modification, will be impacted with a broken >>> card detection - in order to operate the cards have to be present >>> during kernel boot. >> >> Humm. Is this acceptable, breaking old boards? >> >> I would say at minimum, there should be a big fat comment at the top >> of armada-388-gp.dts explaining that this DTS file is broken on >> v0.0-v1.4. >> >> Or we have two .dts files for the 388-gp file, and a dtsi file. >> > > I expected this patch would be controversial, hence I propose a > compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However > Marvell insisted on HW card detection, because software polling spoils > the SD/MMC benchmarks, but this way the user would decide whether to > stay with broken-cd or switch to GPIO/DAT3 detection. What do you > think? I don't know hwo to correctly handle this case. From my point of view in a ideal work it is typically something that sould be updated by the bootloader. However in the real world, we can't rely on the bootloader :( I also don't like having a dts for each new version of the board but as the end you can see them as different boards. Also now it seems that the distribution are moving to link the dtb and the kernel: for eaxh new version of the kernel they also provide a new version of the dtb. That means, that if we modify the dts, then the old board won't work with the new kernel. So maybe creating a armada-388-gp-v1.5.dts could be the best option. What do you think of it? Thanks, Gregory
Gregory, 2015-10-06 17:05 GMT+02:00 Gregory CLEMENT <gregory.clement@free-electrons.com>: > Hi, > > On mar., oct. 06 2015, Marcin Wojtas <mw@semihalf.com> wrote: > >> Hi Andrew, >> >> 2015-10-06 5:31 GMT+02:00 Andrew Lunn <andrew@lunn.ch>: >>> On Tue, Oct 06, 2015 at 03:22:40AM +0200, Marcin Wojtas wrote: >>>> The newest revisions of A388-GP (v1.5 and higher) support only >>>> DAT3-based card detection, which is enabled by this commit. Hitherto >>>> revisions, without such modification, will be impacted with a broken >>>> card detection - in order to operate the cards have to be present >>>> during kernel boot. >>> >>> Humm. Is this acceptable, breaking old boards? >>> >>> I would say at minimum, there should be a big fat comment at the top >>> of armada-388-gp.dts explaining that this DTS file is broken on >>> v0.0-v1.4. >>> >>> Or we have two .dts files for the 388-gp file, and a dtsi file. >>> >> >> I expected this patch would be controversial, hence I propose a >> compromise: set A388-GP SDHCI to 'broken-cd' by defeault. However >> Marvell insisted on HW card detection, because software polling spoils >> the SD/MMC benchmarks, but this way the user would decide whether to >> stay with broken-cd or switch to GPIO/DAT3 detection. What do you >> think? > > I don't know hwo to correctly handle this case. > > From my point of view in a ideal work it is typically something that > sould be updated by the bootloader. However in the real world, we can't > rely on the bootloader :( > > I also don't like having a dts for each new version of the board but as > the end you can see them as different boards. Also now it seems that the > distribution are moving to link the dtb and the kernel: for eaxh new > version of the kernel they also provide a new version of the dtb. That > means, that if we modify the dts, then the old board won't work with the > new kernel. So maybe creating a armada-388-gp-v1.5.dts could be the best > option. > > What do you think of it? > My first feeling is that creating brand new dts just for sdhci detection (afaik there are no other bigger modifications of the PCB) is not the best idea. However maybe changing armada-388-gp.dts to dtsi and then add two single-entries' dts files on top is an option? Of course we can satisfy each type of boards with 'broken-cd' polling detection. I think it's rather a question of a taste and maintanance point of view. Best regards, Marcin
> My first feeling is that creating brand new dts just for sdhci > detection (afaik there are no other bigger modifications of the PCB) > is not the best idea. However maybe changing armada-388-gp.dts to dtsi > and then add two single-entries' dts files on top is an option? Yes, that is the correct way to do this, if we decide to have two DT blobs for different hardware versions. Andrew
> We have three options here: > - leave GPIO-based card detection and see if in future anyone > complains about not working detection > - switch to DAT3 detection - yes you're right, that it is a regression > for all boards < 1.5 > - enable SW polling for detection ("broken-cd") which should satisfy > all kind of boards Then i would suggest either broken-cd, or two .dtb blobs and good comments in the dts files about how to identify the boards and what happens if you run the wrong DTB blob. Andrew
diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts index 391dea9..4855963 100644 --- a/arch/arm/boot/dts/armada-388-gp.dts +++ b/arch/arm/boot/dts/armada-388-gp.dts @@ -213,8 +213,9 @@ sdhci@d8000 { pinctrl-names = "default"; pinctrl-0 = <&sdhci_pins>; - cd-gpios = <&expander0 5 GPIO_ACTIVE_LOW>; no-1-8-v; + dat3-cd; + cd-inverted; wp-inverted; bus-width = <8>; status = "okay";
The newest revisions of A388-GP (v1.5 and higher) support only DAT3-based card detection, which is enabled by this commit. Hitherto revisions, without such modification, will be impacted with a broken card detection - in order to operate the cards have to be present during kernel boot. Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- arch/arm/boot/dts/armada-388-gp.dts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)