[U-Boot] ARM: zynq: sdhci clock frequency init question

[U-Boot] ARM: zynq: sdhci clock frequency init question

[Krunal Desai]

Hi all -

I noticed that in zynq_sdhci.c, responsible for initializing PS SD controller(s), host controller max clock frequency is always set to 52MHz (http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/zynq_sdhci.c;h=fdce2c2c10ec85c4a291532f927eae4a0b5627c9;hb=master#l34). In cases where user is using EMIO connectivity, max clock speed is limited to 25MHz. This results in out-of-spec operation as the divider calculation logic trusts the input to add_sdhci() and does not check the SLCR itself to confirm what the IO clock to SD controller actually is.

I think I saw OF/device-tree support patched in recently (I am on the 2013.4 tag); I think the "right" way to solve this is add capability in zynq_sdhci_init to read device-tree for 'clock-frequency' property, and use that to populate the arguments to add_sdhci() with that information, defaulting to a safe minimum (25MHz?) if no entry is found. Otherwise, I suppose a config could be added akin to 'ZYNQ_SD0_MIO'/'ZYNQ_SD1_MIO' to populate the correct minimum value.

Does this sound sane? I am thinking of implementing that as a patch for ourselves internally, but I think it will be of value to the greater community as well.

Thanks,
Krunal

[U-Boot] ARM: zynq: sdhci clock frequency init question

[Michal Simek]

Hi Krunal,

sorry for delay.

On 04/25/2014 06:19 PM, Krunal Desai wrote:
> Hi all -
> 
> I noticed that in zynq_sdhci.c, responsible for initializing PS SD controller(s), host controller max clock frequency is always set to 52MHz (http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mmc/zynq_sdhci.c;h=fdce2c2c10ec85c4a291532f927eae4a0b5627c9;hb=master#l34). In cases where user is using EMIO connectivity, max clock speed is limited to 25MHz. This results in out-of-spec operation as the divider calculation logic trusts the input to add_sdhci() and does not check the SLCR itself to confirm what the IO clock to SD controller actually is.
> 
> I think I saw OF/device-tree support patched in recently (I am on the 2013.4 tag); I think the "right" way to solve this is add capability in zynq_sdhci_init to read device-tree for 'clock-frequency' property, and use that to populate the arguments to add_sdhci() with that information, defaulting to a safe minimum (25MHz?) if no entry is found. Otherwise, I suppose a config could be added akin to 'ZYNQ_SD0_MIO'/'ZYNQ_SD1_MIO' to populate the correct minimum value.
> 
> Does this sound sane? I am thinking of implementing that as a patch for ourselves internally, but I think it will be of value to the greater community as well.

we didn't test this configuration that's why 52MHz is there as default case.
I think that should be easily possible to detect MIO setting
as we are doing for qspi/nand/usb.

We have this code in our xilinx repository and I have sent patches for mainline review
2 weeks ago or something like that.
I haven't got any NACK and I am going to send pull request to ARM custodian
when I fix fpga patches.

It means detection via MIO setting is reasonable way how to do it.
When you know that this go through EMIO 25MHz should be used.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140507/390b8526/attachment.pgp>

[U-Boot] ARM: zynq: sdhci clock frequency init question

[Krunal Desai]

> From: Michal Simek [mailto:monstr AT monstr.eu]
> Sent: Wednesday, May 07, 2014 04:46
> To: Krunal Desai; u-boot AT lists.denx.de
> Subject: Re: [U-Boot] ARM: zynq: sdhci clock frequency init question
> 
> we didn't test this configuration that's why 52MHz is there as default case.
> I think that should be easily possible to detect MIO setting as we are 
> doing for qspi/nand/usb.

(Trying again due to base64-oddness with Outlook)

Thanks for the reply Michal; that sounds very sane to me. To be clear, you'd basically read the MIO configuration registers to see whether SD is active on MIO, and if not, assume EMIO? (Or not used?)
	 
> We have this code in our xilinx repository and I have sent patches for 
> mainline review
> 2 weeks ago or something like that.
> I haven't got any NACK and I am going to send pull request to ARM 
> custodian when I fix fpga patches.

Looking forward to seeing it, thanks for following up!

Krunal Desai
Avionics Engineer
Planetary Resources

[U-Boot] ARM: zynq: sdhci clock frequency init question

[Michal Simek]

On 05/12/2014 08:48 PM, Krunal Desai wrote:
>> From: Michal Simek [mailto:monstr AT monstr.eu]
>> Sent: Wednesday, May 07, 2014 04:46
>> To: Krunal Desai; u-boot AT lists.denx.de
>> Subject: Re: [U-Boot] ARM: zynq: sdhci clock frequency init question
>>
>> we didn't test this configuration that's why 52MHz is there as default case.
>> I think that should be easily possible to detect MIO setting as we are 
>> doing for qspi/nand/usb.
> 
> (Trying again due to base64-oddness with Outlook)
> 
> Thanks for the reply Michal; that sounds very sane to me. To be clear, you'd basically read the MIO configuration registers to see whether SD is active on MIO, and if not, assume EMIO? (Or not used?)
> 	 
>> We have this code in our xilinx repository and I have sent patches for 
>> mainline review
>> 2 weeks ago or something like that.
>> I haven't got any NACK and I am going to send pull request to ARM 
>> custodian when I fix fpga patches.
> 
> Looking forward to seeing it, thanks for following up!

Sorry for delay.
I have sent pull request to Albert

http://git.denx.de/?p=u-boot/u-boot-microblaze.git;a=shortlog;h=refs/heads/zynq
but here is the code we are talking about.
ARM: zynq: Add MIO detection code

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140516/d50abe64/attachment.pgp>