SD card driver for Freescale i.MX6 in Genode

[Praveen B]

Hi Martin

Comments inline...

On Fri, Mar 20, 2015 at 8:42 PM, Martin Stein <martin.stein at ...1...>
wrote:

> Hi Praveen, Norman,
>
> First of all, it's cool to see you sharing this code with us and I
> enjoyed reading it!
>

Thank you

>
> According to the i.MX6 manual, the uSDHC is a new implementation that is
> at least not fully compatible with the eSDHC. For example, there are new
> registers, new features (tuning), and common registers differ. I think
> it's a good idea to treat uSDHC and eSDHC separately for now and maybe
> later determine in which way they can share more code.
>
>
That's true.


> Regarding your commit, Praveen, there are two suggestions that came to
> my mind while having a first look at it:
>
> * I saw that you've opened up a new sd_card.h in the uSDHC sub-directory
> of the driver. However, the concept of the existing generic sd_card.h is
> to provide SD-card functionality that is independent of the specific
> hardware implementation. Thus, if the code you've added in
> usdhc/sd_card.h is of such characteristic, I would suggest to provide it
> through the existing sd_card.h instead of adding a new header. If the
> existing implementations in sd_card.h are not sufficient to reuse them
> properly for the uSDHC, they should rather be adapted.
>

The reason we created another sd_card.h is, USDHC returns CSD register
shifted by 8 bits. That could also have been handled in the read_csd()
function of the controller instead of sd_card.h. We have made that change
and added a new commit.

We have also added Set_blocklen() command to sd_card.h. We currently see
that the change is present in the staging branch of Genode (for FS iMX53)
and does not collide with any other sd_card driver implementation.


>
> * For sources you've added around the driver like
>     repos/ports-foc/run/linux_imx6.run
>     repos/os/run/part_blk_sd.run
>     repos/dde_rump/run/rump_ext2_sd.run
> I would create dedicated commits so the driver commit really
> concentrates on the driver and its dependencies. This way, one can add
> and test this feature without the need for fetching such sattelite code.
>

Thanks for the suggestions.
I've created two different commits, please take a look and let us know if
they look fine.

Regards

Praveen Srinivas
IIT Madras


>
> Beside that, the code looks clean and comprehensible to me :)
>
> Best regards,
> Martin
>
>
> On 20.03.2015 11:40, Norman Feske wrote:
> > Hello Praveen,
> >
> >> We have developed a Genode device driver for USDHC card reader for
> >> Freescale i.MX6 procesor.
> >>
> >> You may find the corresponding code in the repository,
> >> https://github.com/srinivasprv/genode
> >>
> >> The specific driver code is available in usdhc directory of sd_card
> >> directory.
> > thank you for sharing your work with us! Your patch looks very nice.
> > Coincidentally, we just added an SD-card driver for the i.MX53 platform
> > last week. See the following commit referenced at the following issue:
> >
> >   https://github.com/genodelabs/genode/issues/1458
> >
> > Whereas the i.MX53 driver addresses the so-called ESDHC controller, your
> > patch addresses the USDHC controller. Are both drivers complementary?
> >
> > On another note, the mainline Genode version does not officially support
> > i.MX6 yet. So I am afraid that including your driver without prior
> > enabling i.MX6 would not be very sensible. As a precondition, we should
> > enable at least one i.MX6-based platform (e.g., Wandboard?). Otherwise,
> > we would not able to add your driver to our regular test infrastructure.
> > What do you think about how to go about it?
> >
> > Best regards
> > Norman
> >
>
>
>
>
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming The Go Parallel Website,
> sponsored
> by Intel and developed in partnership with Slashdot Media, is your hub for
> all
> things parallel software development, from weekly thought leadership blogs
> to
> news, videos, case studies, tutorials and more. Take a look and join the
> conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> genode-main mailing list
> genode-main at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/genode-main
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.genode.org/pipermail/users/attachments/20150324/3774a6cf/attachment.html>