Re: [PATCH v2 1/3] mmc: add Coldfire esdhc support
On 6/07/19 2:20 PM, Angelo Dureghello wrote: > Hi Adrian, > > On Tue, Jul 02, 2019 at 12:10:54PM +0300, Adrian Hunter wrote: >> On 20/06/19 1:22 AM, Angelo Dureghello wrote: >>> Hi Christoph, >>> >>> On Sun, Jun 16, 2019 at 11:58:07PM -0700, Christoph Hellwig wrote: On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote: > This driver has been developed as a separate module starting > from the similar sdhci-esdhc-fls.c. > Separation has been mainly driven from change in endianness. Can't we have a way to define the endianess at build or even runtime? We have plenty of that elsewhere in the kernel. >>> >>> well, the base sdhci layer wants to access byte-size fields of the >>> esdhc controller registers. >>> But this same Freescale esdhc controller may be found in 2 >>> flavors, big-endian or little-endian organized. >>> So in this driver i am actually correcting byte-addresses to >>> access the wanted byte-field in the big-endian hw controller. >>> >>> So this is a bit different from a be-le endian swap of a >>> long or a short that the kernel is organized to do.. >> >> Did you consider just using different sdhci_ops so that you could support >> different sdhci I/O accessors? > > Initially i tried to modify the IMX/Vybrid (arm) driver. But was stopped from > several points, trying to remember now, > > - the I/O accessors was a const struct, but this of course is not a big > issue, > - here and there bitfields and positions of the ColdFire controller > registers, compared to the arm versions, are different, so controller hw > is not exactly the same, > - on ColdFire controller and some behaviors and errata are different, > - dma endiannes not hw-configurable, > - ColdFire has max clock limitations, a bit different clock init. > > Finally, since there is already a common library (shdci.c) i decided to go > as a separate driver, instead of filling the driver of "if (coldfire)" also > mainly for the following reasons: > > 1) separated ColdFire driver has a quite small amount of code, simple to > understand. > 2) having drivers used by multiple architectures, it add risks, each time > i perform a change, i can test it only on ColdFire, and can break > the driver for the other architectures (i see this not rarely happening for > multiple-arch used drivers). > > So let me know if the way chosen can be ok. Otherwise i will roll back > trying to modify the iMX/Vybrid driver, likely adding a lot of "if (coldfire)" > complicating it quite a lot. "if (coldfire)" is not very nice, and there doesn't seem to be that much common code, so let's stick with a separate driver, but please change the commit message in consideration of this discussion.
Re: [PATCH v2 1/3] mmc: add Coldfire esdhc support
Hi Adrian, On Tue, Jul 02, 2019 at 12:10:54PM +0300, Adrian Hunter wrote: > On 20/06/19 1:22 AM, Angelo Dureghello wrote: > > Hi Christoph, > > > > On Sun, Jun 16, 2019 at 11:58:07PM -0700, Christoph Hellwig wrote: > >> On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote: > >>> This driver has been developed as a separate module starting > >>> from the similar sdhci-esdhc-fls.c. > >>> Separation has been mainly driven from change in endianness. > >> > >> Can't we have a way to define the endianess at build or even runtime? > >> We have plenty of that elsewhere in the kernel. > > > > well, the base sdhci layer wants to access byte-size fields of the > > esdhc controller registers. > > But this same Freescale esdhc controller may be found in 2 > > flavors, big-endian or little-endian organized. > > So in this driver i am actually correcting byte-addresses to > > access the wanted byte-field in the big-endian hw controller. > > > > So this is a bit different from a be-le endian swap of a > > long or a short that the kernel is organized to do.. > > Did you consider just using different sdhci_ops so that you could support > different sdhci I/O accessors? Initially i tried to modify the IMX/Vybrid (arm) driver. But was stopped from several points, trying to remember now, - the I/O accessors was a const struct, but this of course is not a big issue, - here and there bitfields and positions of the ColdFire controller registers, compared to the arm versions, are different, so controller hw is not exactly the same, - on ColdFire controller and some behaviors and errata are different, - dma endiannes not hw-configurable, - ColdFire has max clock limitations, a bit different clock init. Finally, since there is already a common library (shdci.c) i decided to go as a separate driver, instead of filling the driver of "if (coldfire)" also mainly for the following reasons: 1) separated ColdFire driver has a quite small amount of code, simple to understand. 2) having drivers used by multiple architectures, it add risks, each time i perform a change, i can test it only on ColdFire, and can break the driver for the other architectures (i see this not rarely happening for multiple-arch used drivers). So let me know if the way chosen can be ok. Otherwise i will roll back trying to modify the iMX/Vybrid driver, likely adding a lot of "if (coldfire)" complicating it quite a lot. Regards, Angelo
Re: [PATCH v2 1/3] mmc: add Coldfire esdhc support
On 20/06/19 1:22 AM, Angelo Dureghello wrote: > Hi Christoph, > > On Sun, Jun 16, 2019 at 11:58:07PM -0700, Christoph Hellwig wrote: >> On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote: >>> This driver has been developed as a separate module starting >>> from the similar sdhci-esdhc-fls.c. >>> Separation has been mainly driven from change in endianness. >> >> Can't we have a way to define the endianess at build or even runtime? >> We have plenty of that elsewhere in the kernel. > > well, the base sdhci layer wants to access byte-size fields of the > esdhc controller registers. > But this same Freescale esdhc controller may be found in 2 > flavors, big-endian or little-endian organized. > So in this driver i am actually correcting byte-addresses to > access the wanted byte-field in the big-endian hw controller. > > So this is a bit different from a be-le endian swap of a > long or a short that the kernel is organized to do.. Did you consider just using different sdhci_ops so that you could support different sdhci I/O accessors?
Re: [PATCH v2 1/3] mmc: add Coldfire esdhc support
Hi Christoph, On Sun, Jun 16, 2019 at 11:58:07PM -0700, Christoph Hellwig wrote: > On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote: > > This driver has been developed as a separate module starting > > from the similar sdhci-esdhc-fls.c. > > Separation has been mainly driven from change in endianness. > > Can't we have a way to define the endianess at build or even runtime? > We have plenty of that elsewhere in the kernel. well, the base sdhci layer wants to access byte-size fields of the esdhc controller registers. But this same Freescale esdhc controller may be found in 2 flavors, big-endian or little-endian organized. So in this driver i am actually correcting byte-addresses to access the wanted byte-field in the big-endian hw controller. So this is a bit different from a be-le endian swap of a long or a short that the kernel is organized to do.. Regards, Angelo
Re: [PATCH v2 1/3] mmc: add Coldfire esdhc support
On Sun, Jun 16, 2019 at 10:48:21PM +0200, Angelo Dureghello wrote: > This driver has been developed as a separate module starting > from the similar sdhci-esdhc-fls.c. > Separation has been mainly driven from change in endianness. Can't we have a way to define the endianess at build or even runtime? We have plenty of that elsewhere in the kernel.
