[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Ajay Bhargav]

----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

> > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > sdhci_readw does not work for host version read in Armada100
> series
> > > > SoCs. This patch fix this issue by making a sdhci_readl call to
> get
> > > 
> > > host
> > > 
> > > > version.
> > > > 
> > > > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > > > ---
> > > > 
> > > >  drivers/mmc/mv_sdhci.c |    6 ++++++
> > > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> > > > index f92caeb..c7fd287 100644
> > > > --- a/drivers/mmc/mv_sdhci.c
> > > > +++ b/drivers/mmc/mv_sdhci.c
> > > > @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct
> > > 
> > > sdhci_host
> > > 
> > > > *host, u8 val, int reg) #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS
> */
> > > > 
> > > >  static char *MVSDH_NAME = "mv_sdh";
> > > > 
> > > > +
> > > > 
> > > >  int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32
> quirks)
> > > >  {
> > > >  
> > > >  	struct sdhci_host *host = NULL;
> > > > 
> > > > @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk,
> u32
> > > 
> > > min_clk,
> > > 
> > > > u32 quirks) mv_ops.write_b = mv_sdhci_writeb;
> > > > 
> > > >  	host->ops = &mv_ops;
> > > >  
> > > >  #endif
> > > > 
> > > > +#ifdef CONFIG_ARMADA100
> > > > +	/* SDHCI host version read workaround for Armada100 series */
> > > > +	host->version = sdhci_readl(host, SDHCI_HOST_VERSION - 2) >>
> 16;
> > > > +#else
> > > > 
> > > >  	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> > > > 
> > > > +#endif
> > > > 
> > > >  	add_sdhci(host, max_clk, min_clk);
> > > >  	return 0;
> > > >  
> > > >  }
> > > 
> > > Basically armada100 can't do 16bit access to that register. Ok,
> but
> > > why does it
> > > work for other registers? Also, can you make this a config option
> like
> > > 
> > > CONFIG_SDHCI_REG32_QUIRK (invent some better name ;-) ).
> > > 
> > > Cheers
> > 
> > Hi Marek,
> > 
> > Armada100 SOC has this bug for particularly that register. This is
> > particularly for that register so Do you think a config is required
> for
> > this?
> 
> Well such crippled hardware might appear somewhere else too, so yes.
> But still, 
> it's surprising it's only one register, really. Can you point me to a
> 
> datasheet/errata stating it?
> 
> Thanks!
> 
I agree, adding a quirk for this is best option this way driver will still be generic for marvell series.
Marek, I am really sorry but I am not allowed to share errata here :) I will ask my senior if i can atleast send you a screenshot regarding this issue.

Regards,
Ajay Bhargav

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Marek Vasut]

> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > sdhci_readw does not work for host version read in Armada100
> > 
[...]

> > Well such crippled hardware might appear somewhere else too, so yes.
> > But still,
> > it's surprising it's only one register, really. Can you point me to a
> > 
> > datasheet/errata stating it?
> > 
> > Thanks!
> 
> I agree, adding a quirk for this is best option this way driver will still
> be generic for marvell series. Marek, I am really sorry but I am not
> allowed to share errata here :) I will ask my senior if i can atleast send
> you a screenshot regarding this issue.

Stupid Marvell crap again :-(

> 
> Regards,
> Ajay Bhargav

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-
> bounces at lists.denx.de] On Behalf Of Marek Vasut
> Sent: Wednesday, November 02, 2011 3:55 PM
> To: Ajay Bhargav
> Cc: u-boot at lists.denx.de; afleming at gmail.com
> Subject: Re: [U-Boot] [PATCH] mmc: mv_sdhci: Fix host version
> read for Armada100
> 
> > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > > sdhci_readw does not work for host version read in
> Armada100
> > >
> [...]
> 
> > > Well such crippled hardware might appear somewhere else
> too, so yes.
> > > But still,
> > > it's surprising it's only one register, really. Can you
> point me to a
> > >
> > > datasheet/errata stating it?
> > >
> > > Thanks!
> >
> > I agree, adding a quirk for this is best option this way
> driver will still
> > be generic for marvell series. Marek, I am really sorry but I
> am not
> > allowed to share errata here :) I will ask my senior if i can
> atleast send
> > you a screenshot regarding this issue.
> 
> Stupid Marvell crap again :-(

Hi Marek
I understand your concern.
Please understand there are company policies we need to follow.

Meanwhile have a look at documentation (software manual) at below pointer, if it helps you (I hope you already had it :-D )
http://www.marvell.com/application-processors/armada-100/

Copying Lei too, if he can help you further on this.

Regards..
Prafulla . .

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Marek Vasut]

> > -----Original Message-----
> > From: u-boot-bounces at lists.denx.de [mailto:u-boot-
> > bounces at lists.denx.de] On Behalf Of Marek Vasut
> > Sent: Wednesday, November 02, 2011 3:55 PM
> > To: Ajay Bhargav
> > Cc: u-boot at lists.denx.de; afleming at gmail.com
> > Subject: Re: [U-Boot] [PATCH] mmc: mv_sdhci: Fix host version
> > read for Armada100
> > 
> > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > > > sdhci_readw does not work for host version read in
> > 
> > Armada100
> > 
> > [...]
> > 
> > > > Well such crippled hardware might appear somewhere else
> > 
> > too, so yes.
> > 
> > > > But still,
> > > > it's surprising it's only one register, really. Can you
> > 
> > point me to a
> > 
> > > > datasheet/errata stating it?
> > > > 
> > > > Thanks!
> > > 
> > > I agree, adding a quirk for this is best option this way
> > 
> > driver will still
> > 
> > > be generic for marvell series. Marek, I am really sorry but I
> > 
> > am not
> > 
> > > allowed to share errata here :) I will ask my senior if i can
> > 
> > atleast send
> > 
> > > you a screenshot regarding this issue.
> > 
> > Stupid Marvell crap again :-(
> 
> Hi Marek
> I understand your concern.
> Please understand there are company policies we need to follow.

Which are ... see above :-)

> 
> Meanwhile have a look at documentation (software manual) at below pointer,
> if it helps you (I hope you already had it :-D )
> http://www.marvell.com/application-processors/armada-100/

Good, thank you. Do you plan to release armada610 TRM too (used in OLPC 1.75) ?

Cheers

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: Marek Vasut [mailto:marek.vasut at gmail.com]
> Sent: Wednesday, November 02, 2011 7:59 PM
> To: Prafulla Wadaskar
> Cc: Ajay Bhargav; u-boot at lists.denx.de; afleming at gmail.com; Lei
> Wen; Chris Ball
> Subject: Re: [U-Boot] [PATCH] mmc: mv_sdhci: Fix host version
> read for Armada100
> 
> > > -----Original Message-----
> > > From: u-boot-bounces at lists.denx.de [mailto:u-boot-
> > > bounces at lists.denx.de] On Behalf Of Marek Vasut
> > > Sent: Wednesday, November 02, 2011 3:55 PM
> > > To: Ajay Bhargav
> > > Cc: u-boot at lists.denx.de; afleming at gmail.com
> > > Subject: Re: [U-Boot] [PATCH] mmc: mv_sdhci: Fix host
> version
> > > read for Armada100
> > >
> > > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > > ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > > > > > > sdhci_readw does not work for host version read
> in
> > >
> > > Armada100
> > >
> > > [...]
> > >
> > > > > Well such crippled hardware might appear somewhere else
> > >
> > > too, so yes.
> > >
> > > > > But still,
> > > > > it's surprising it's only one register, really. Can you
> > >
> > > point me to a
> > >
> > > > > datasheet/errata stating it?
> > > > >
> > > > > Thanks!
> > > >
> > > > I agree, adding a quirk for this is best option this way
> > >
> > > driver will still
> > >
> > > > be generic for marvell series. Marek, I am really sorry
> but I
> > >
> > > am not
> > >
> > > > allowed to share errata here :) I will ask my senior if i
> can
> > >
> > > atleast send
> > >
> > > > you a screenshot regarding this issue.
> > >
> > > Stupid Marvell crap again :-(
> >
> > Hi Marek
> > I understand your concern.
> > Please understand there are company policies we need to
> follow.
> 
> Which are ... see above :-)
> 
> >
> > Meanwhile have a look at documentation (software manual) at
> below pointer,
> > if it helps you (I hope you already had it :-D )
> > http://www.marvell.com/application-processors/armada-100/
> 
> Good, thank you. Do you plan to release armada610 TRM too (used
> in OLPC 1.75) ?

I will query on this and let you know.

Regards..
Prafulla . . .

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Ajay Bhargav]

----- "Marek Vasut" <marek.vasut@gmail.com> wrote:

> > sdhci_readw does not work for host version read in Armada100 series
> > SoCs. This patch fix this issue by making a sdhci_readl call to get
> host
> > version.
> > 
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> >  drivers/mmc/mv_sdhci.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> > index f92caeb..c7fd287 100644
> > --- a/drivers/mmc/mv_sdhci.c
> > +++ b/drivers/mmc/mv_sdhci.c
> > @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct
> sdhci_host
> > *host, u8 val, int reg) #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
> > 
> >  static char *MVSDH_NAME = "mv_sdh";
> > +
> >  int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
> >  {
> >  	struct sdhci_host *host = NULL;
> > @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32
> min_clk,
> > u32 quirks) mv_ops.write_b = mv_sdhci_writeb;
> >  	host->ops = &mv_ops;
> >  #endif
> > +#ifdef CONFIG_ARMADA100
> > +	/* SDHCI host version read workaround for Armada100 series */
> > +	host->version = sdhci_readl(host, SDHCI_HOST_VERSION - 2) >> 16;
> > +#else
> >  	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> > +#endif
> >  	add_sdhci(host, max_clk, min_clk);
> >  	return 0;
> >  }
> 
> Basically armada100 can't do 16bit access to that register. Ok, but
> why does it 
> work for other registers? Also, can you make this a config option like
> 
> CONFIG_SDHCI_REG32_QUIRK (invent some better name ;-) ).
> 
> Cheers
> 
Hi Marek,

Armada100 SOC has this bug for particularly that register. This is particularly for that register so Do you think a config is required for this?

Regards,
Ajay Bhargav

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Marek Vasut]

> ----- "Marek Vasut" <marek.vasut@gmail.com> wrote:
> > > sdhci_readw does not work for host version read in Armada100 series
> > > SoCs. This patch fix this issue by making a sdhci_readl call to get
> > 
> > host
> > 
> > > version.
> > > 
> > > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > > ---
> > > 
> > >  drivers/mmc/mv_sdhci.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> > > index f92caeb..c7fd287 100644
> > > --- a/drivers/mmc/mv_sdhci.c
> > > +++ b/drivers/mmc/mv_sdhci.c
> > > @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct
> > 
> > sdhci_host
> > 
> > > *host, u8 val, int reg) #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
> > > 
> > >  static char *MVSDH_NAME = "mv_sdh";
> > > 
> > > +
> > > 
> > >  int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
> > >  {
> > >  
> > >  	struct sdhci_host *host = NULL;
> > > 
> > > @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32
> > 
> > min_clk,
> > 
> > > u32 quirks) mv_ops.write_b = mv_sdhci_writeb;
> > > 
> > >  	host->ops = &mv_ops;
> > >  
> > >  #endif
> > > 
> > > +#ifdef CONFIG_ARMADA100
> > > +	/* SDHCI host version read workaround for Armada100 series */
> > > +	host->version = sdhci_readl(host, SDHCI_HOST_VERSION - 2) >> 16;
> > > +#else
> > > 
> > >  	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> > > 
> > > +#endif
> > > 
> > >  	add_sdhci(host, max_clk, min_clk);
> > >  	return 0;
> > >  
> > >  }
> > 
> > Basically armada100 can't do 16bit access to that register. Ok, but
> > why does it
> > work for other registers? Also, can you make this a config option like
> > 
> > CONFIG_SDHCI_REG32_QUIRK (invent some better name ;-) ).
> > 
> > Cheers
> 
> Hi Marek,
> 
> Armada100 SOC has this bug for particularly that register. This is
> particularly for that register so Do you think a config is required for
> this?

Well such crippled hardware might appear somewhere else too, so yes. But still, 
it's surprising it's only one register, really. Can you point me to a 
datasheet/errata stating it?

Thanks!

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Ajay Bhargav]

sdhci_readw does not work for host version read in Armada100 series
SoCs. This patch fix this issue by making a sdhci_readl call to get host
version.

Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
---
 drivers/mmc/mv_sdhci.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
index f92caeb..c7fd287 100644
--- a/drivers/mmc/mv_sdhci.c
+++ b/drivers/mmc/mv_sdhci.c
@@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
 #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
 
 static char *MVSDH_NAME = "mv_sdh";
+
 int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
 {
 	struct sdhci_host *host = NULL;
@@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
 		mv_ops.write_b = mv_sdhci_writeb;
 	host->ops = &mv_ops;
 #endif
+#ifdef CONFIG_ARMADA100
+	/* SDHCI host version read workaround for Armada100 series */
+	host->version = sdhci_readl(host, SDHCI_HOST_VERSION - 2) >> 16;
+#else
 	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
+#endif
 	add_sdhci(host, max_clk, min_clk);
 	return 0;
 }
-- 
1.7.0.4

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Marek Vasut]

> sdhci_readw does not work for host version read in Armada100 series
> SoCs. This patch fix this issue by making a sdhci_readl call to get host
> version.
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---
>  drivers/mmc/mv_sdhci.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> index f92caeb..c7fd287 100644
> --- a/drivers/mmc/mv_sdhci.c
> +++ b/drivers/mmc/mv_sdhci.c
> @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct sdhci_host
> *host, u8 val, int reg) #endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
> 
>  static char *MVSDH_NAME = "mv_sdh";
> +
>  int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
>  {
>  	struct sdhci_host *host = NULL;
> @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk,
> u32 quirks) mv_ops.write_b = mv_sdhci_writeb;
>  	host->ops = &mv_ops;
>  #endif
> +#ifdef CONFIG_ARMADA100
> +	/* SDHCI host version read workaround for Armada100 series */
> +	host->version = sdhci_readl(host, SDHCI_HOST_VERSION - 2) >> 16;
> +#else
>  	host->version = sdhci_readw(host, SDHCI_HOST_VERSION);
> +#endif
>  	add_sdhci(host, max_clk, min_clk);
>  	return 0;
>  }

Basically armada100 can't do 16bit access to that register. Ok, but why does it 
work for other registers? Also, can you make this a config option like 
CONFIG_SDHCI_REG32_QUIRK (invent some better name ;-) ).

Cheers

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Lei Wen]

Hi Ajay,

On Tue, Nov 1, 2011 at 5:39 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> sdhci_readw does not work for host version read in Armada100 series
> SoCs. This patch fix this issue by making a sdhci_readl call to get host
> version.
>
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---
> ?drivers/mmc/mv_sdhci.c | ? ?6 ++++++
> ?1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> index f92caeb..c7fd287 100644
> --- a/drivers/mmc/mv_sdhci.c
> +++ b/drivers/mmc/mv_sdhci.c
> @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
> ?#endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
>
> ?static char *MVSDH_NAME = "mv_sdh";
> +
> ?int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
> ?{
> ? ? ? ?struct sdhci_host *host = NULL;
> @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
> ? ? ? ? ? ? ? ?mv_ops.write_b = mv_sdhci_writeb;
> ? ? ? ?host->ops = &mv_ops;
> ?#endif
> +#ifdef CONFIG_ARMADA100

You should following my previous fixing sample to add the workaround:
http://patchwork.ozlabs.org/patch/117575/

Just keep in mind that, it is generic driver and don't put private
fixing there. :)

Thanks,
Lei

[U-Boot] [PATCH] mmc: mv_sdhci: Fix host version read for Armada100

[Ajay Bhargav]

----- "Lei Wen" <adrian.wenl@gmail.com> wrote:

> Hi Ajay,
> 
> On Tue, Nov 1, 2011 at 5:39 PM, Ajay Bhargav
> <ajay.bhargav@einfochips.com> wrote:
> > sdhci_readw does not work for host version read in Armada100 series
> > SoCs. This patch fix this issue by making a sdhci_readl call to get
> host
> > version.
> >
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> > ?drivers/mmc/mv_sdhci.c | ? ?6 ++++++
> > ?1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/mv_sdhci.c b/drivers/mmc/mv_sdhci.c
> > index f92caeb..c7fd287 100644
> > --- a/drivers/mmc/mv_sdhci.c
> > +++ b/drivers/mmc/mv_sdhci.c
> > @@ -30,6 +30,7 @@ static inline void mv_sdhci_writeb(struct
> sdhci_host *host, u8 val, int reg)
> > ?#endif /* CONFIG_MMC_SDHCI_IO_ACCESSORS */
> >
> > ?static char *MVSDH_NAME = "mv_sdh";
> > +
> > ?int mv_sdh_init(u32 regbase, u32 max_clk, u32 min_clk, u32 quirks)
> > ?{
> > ? ? ? ?struct sdhci_host *host = NULL;
> > @@ -48,7 +49,12 @@ int mv_sdh_init(u32 regbase, u32 max_clk, u32
> min_clk, u32 quirks)
> > ? ? ? ? ? ? ? ?mv_ops.write_b = mv_sdhci_writeb;
> > ? ? ? ?host->ops = &mv_ops;
> > ?#endif
> > +#ifdef CONFIG_ARMADA100
> 
> You should following my previous fixing sample to add the workaround:
> http://patchwork.ozlabs.org/patch/117575/
> 
Patches acked? I didn't know it. Or I should wait for your patches to get mainlined first.

> Just keep in mind that, it is generic driver and don't put private
> fixing there. :)
> 
> Thanks,
> Lei
> 
Any good way to put that fix? Or else I can simply follow Marek's solution to add a quirk for this instead of defining it for a CPU. What you say?

Regards,
Ajay Bhargav