Hi Martin
Comments inline...
On Fri, Mar 20, 2015 at 8:42 PM, Martin Stein <[email protected]>
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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/genode-main
>
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/genode-main
