Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
[...] > > > > > > My current patch-set is also to cleaning up the driver waiting for > > > splitting them someday on the other hand. After you check > > > CONFIG_FSL_ESDHC in u-boot source, if you think it's better we > > > should split them right now, I could work on the driver splitting. > > > > You can think about having common part (in one file - fsl_esdhc.c) and > > then separate files with code specific to imx and ppc. This would > > reduce the number of changes needed. > > [Y.b. Lu] Actually, it's not only imx and ppc. > In linux, esdhc driver is for MPC83XX/MPC85XX, QorIQ PowerPC processors, > QoriQ ARM processors (Layerscape). > Esdhc-imx driver is for i.MX processors. > > Anyway there would be a lot files changed if we split the driver. Let me send > out a patch-set to split them for your reviewing. > Thanks a lot for your comments. > [Y.b. Lu] Any comments or suggestions on the patch-set. Thanks a lot :) http://patchwork.ozlabs.org/project/uboot/list/?series=93317=* [...] ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
Hi Lukasz,
> -Original Message-
> From: Lukasz Majewski
> Sent: Tuesday, February 19, 2019 2:39 PM
> To: Y.b. Lu
> Cc: u-boot@lists.denx.de
> Subject: Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
>
> Hi "Y.b. Lu",
>
> > Hi Lukasz,
> >
> > > -Original Message-
> > > From: Lukasz Majewski
> > > Sent: Monday, February 18, 2019 7:12 AM
> > > To: Y.b. Lu
> > > Cc: u-boot@lists.denx.de
> > > Subject: Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
> > >
> > > Dear "Y.b. Lu",
> > >
> > > > The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
> > > > initially. The later QoriQ series processors (which were
> > > > evolutions of MPC83XX/MPC85XX) and i.MX series processors were
> > > > using this driver for their eSDHCs too.
> > > >
> > > > So there are two evolution directions for eSDHC now. For the two
> > > > series processors, the eSDHCs are becoming more and more
> > > > different. We should have split it into two drivers, like them
> > > > (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
> > >
> > > Why we cannot do it right just from the very beginning?
> > >
> > > In the end of the day we will try to mimic Linux kernel anyway, IMHO
> > > it is better to start the split now and save some effort on a
> > > half-step solution.
> >
> > [Y.b. Lu] I understand. The perfect way is to split them.
> > However, if you grep 'CONFIG_FSL_ESDHC' in the whole u-boot source,
> > you would find there are 607 results.
>
> Those are boards, which use the driver. The modification would be done in one
> or two files (fsl_esdhc.c|h).
[Y.b. Lu] Sorry, I can't get your point.
Since we split fsl_esdhc into fsl_esdhc and fsl_esdhc_imx driver, all i.mx
boards files should convert to use new CONFIG_FSL_ESDHC_IMX instead as I
understand.
>
> > There will be so many companies
> > boards affected.
>
> I guess that the task of your patch set is to separate imx6q and ppc specific
> code for those IP blocks.
>
> > I just don't know who could make all of these boards tested.
>
> Your patch set also makes some changes in the generic driver depending on
> the priv->esdhc_imx flag. Those changes also need to be tested on all before
> mentioned boards.
[Y.b. Lu] Right...
>
> In the end you logically split the driver, having it in a single file, which
> IMHO is
> not proper long-term solution.
>
> >
> > My current patch-set is also to cleaning up the driver waiting for
> > splitting them someday on the other hand. After you check
> > CONFIG_FSL_ESDHC in u-boot source, if you think it's better we should
> > split them right now, I could work on the driver splitting.
>
> You can think about having common part (in one file - fsl_esdhc.c) and then
> separate files with code specific to imx and ppc. This would reduce the number
> of changes needed.
[Y.b. Lu] Actually, it's not only imx and ppc.
In linux, esdhc driver is for MPC83XX/MPC85XX, QorIQ PowerPC processors, QoriQ
ARM processors (Layerscape).
Esdhc-imx driver is for i.MX processors.
Anyway there would be a lot files changed if we split the driver. Let me send
out a patch-set to split them for your reviewing.
Thanks a lot for your comments.
>
> >
> > Thanks a lot.
> >
> > > > But it seemed
> > > > to be a lot of work now. So let's keep as it is. Be very careful
> > > > to change the driver if the changes are not common for all eSDHCs,
> > > > and clarify i.MX eSDHC specific things to distingush them with
> > > > QorIQ eSDHC.
> > > >
> > > > This patch is to added an esdhc_imx flag for the development of
> > > > i.MX eSDHC, to distinguish it with QoriQ eSDHC.
> > > >
> > > > Signed-off-by: Yangbo Lu
> > > > ---
> > > > Changes for v2:
> > > > - Converted to use device_is_compatible().
> > > > ---
> > > > drivers/mmc/fsl_esdhc.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > > > index 21fa2ab1d4..a647bc7f1c 100644
> > > > --- a/drivers/mmc/fsl_esdhc.c
> > > > +++ b/drivers/mmc/fsl_esdhc.c
> > > > @@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
> > > > struct gpio_desc cd_gpio;
> > > > struct gpio_desc wp_gpio;
> > > > #endif
> > > > +
Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
Hi "Y.b. Lu",
> Hi Lukasz,
>
> > -Original Message-
> > From: Lukasz Majewski
> > Sent: Monday, February 18, 2019 7:12 AM
> > To: Y.b. Lu
> > Cc: u-boot@lists.denx.de
> > Subject: Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
> >
> > Dear "Y.b. Lu",
> >
> > > The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
> > > initially. The later QoriQ series processors (which were
> > > evolutions of MPC83XX/MPC85XX) and i.MX series processors were
> > > using this driver for their eSDHCs too.
> > >
> > > So there are two evolution directions for eSDHC now. For the two
> > > series processors, the eSDHCs are becoming more and more
> > > different. We should have split it into two drivers, like them
> > > (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
> >
> > Why we cannot do it right just from the very beginning?
> >
> > In the end of the day we will try to mimic Linux kernel anyway,
> > IMHO it is better to start the split now and save some effort on a
> > half-step solution.
>
> [Y.b. Lu] I understand. The perfect way is to split them.
> However, if you grep 'CONFIG_FSL_ESDHC' in the whole u-boot source,
> you would find there are 607 results.
Those are boards, which use the driver. The modification would be done
in one or two files (fsl_esdhc.c|h).
> There will be so many companies
> boards affected.
I guess that the task of your patch set is to separate imx6q and ppc
specific code for those IP blocks.
> I just don't know who could make all of these boards
> tested.
Your patch set also makes some changes in the generic driver depending
on the priv->esdhc_imx flag. Those changes also need to be tested on
all before mentioned boards.
In the end you logically split the driver,
having it in a single file, which IMHO is not proper long-term solution.
>
> My current patch-set is also to cleaning up the driver waiting for
> splitting them someday on the other hand. After you check
> CONFIG_FSL_ESDHC in u-boot source, if you think it's better we should
> split them right now, I could work on the driver splitting.
You can think about having common part (in one file - fsl_esdhc.c) and
then separate files with code specific to imx and ppc. This would
reduce the number of changes needed.
>
> Thanks a lot.
>
> > > But it seemed
> > > to be a lot of work now. So let's keep as it is. Be very careful
> > > to change the driver if the changes are not common for all
> > > eSDHCs, and clarify i.MX eSDHC specific things to distingush them
> > > with QorIQ eSDHC.
> > >
> > > This patch is to added an esdhc_imx flag for the development of
> > > i.MX eSDHC, to distinguish it with QoriQ eSDHC.
> > >
> > > Signed-off-by: Yangbo Lu
> > > ---
> > > Changes for v2:
> > > - Converted to use device_is_compatible().
> > > ---
> > > drivers/mmc/fsl_esdhc.c | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > > index 21fa2ab1d4..a647bc7f1c 100644
> > > --- a/drivers/mmc/fsl_esdhc.c
> > > +++ b/drivers/mmc/fsl_esdhc.c
> > > @@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
> > > struct gpio_desc cd_gpio;
> > > struct gpio_desc wp_gpio;
> > > #endif
> > > + bool esdhc_imx;
> > > };
> > >
> > > /* Return the XFERTYP flags for a given command and data packet
> > > */ @@ -1462,6 +1463,16 @@ static int fsl_esdhc_probe(struct
> > > udevice *dev) priv->caps = data->caps;
> > > }
> > >
> > > + /*
> > > + * This is to specify whether current eSDHC is an i.MX
> > > eSDHC,
> > > + * since the i.MX eSDHC has been becoming more and more
> > > different
> > > + * with QorIQ eSDHC and initial MPC83XX/MPC85XX.
> > > + */
> > > + if (!device_is_compatible(dev, "fsl,esdhc"))
> > > + priv->esdhc_imx = true;
> > > + else
> > > + priv->esdhc_imx = false;
> > > +
> > > val = dev_read_u32_default(dev, "bus-width", -1);
> > > if (val == 8)
> > > priv->bus_width = 8;
> >
> >
> >
> >
> > Best regards,
> >
> > Lukasz Majewski
> >
> > --
> >
> > DENX Software Engineering GmbH, Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lu...@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lu...@denx.de
pgpBL2nsKmIlp.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
Hi Lukasz,
> -Original Message-
> From: Lukasz Majewski
> Sent: Monday, February 18, 2019 7:12 AM
> To: Y.b. Lu
> Cc: u-boot@lists.denx.de
> Subject: Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
>
> Dear "Y.b. Lu",
>
> > The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
> > initially. The later QoriQ series processors (which were evolutions of
> > MPC83XX/MPC85XX) and i.MX series processors were using this driver for
> > their eSDHCs too.
> >
> > So there are two evolution directions for eSDHC now. For the two
> > series processors, the eSDHCs are becoming more and more different.
> > We should have split it into two drivers, like them
> > (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
>
> Why we cannot do it right just from the very beginning?
>
> In the end of the day we will try to mimic Linux kernel anyway, IMHO it is
> better to start the split now and save some effort on a half-step solution.
>
[Y.b. Lu] I understand. The perfect way is to split them.
However, if you grep 'CONFIG_FSL_ESDHC' in the whole u-boot source, you would
find there are 607 results.
There will be so many companies boards affected. I just don't know who could
make all of these boards tested.
My current patch-set is also to cleaning up the driver waiting for splitting
them someday on the other hand.
After you check CONFIG_FSL_ESDHC in u-boot source, if you think it's better we
should split them right now, I could work on the driver splitting.
Thanks a lot.
> > But it seemed
> > to be a lot of work now. So let's keep as it is. Be very careful to
> > change the driver if the changes are not common for all eSDHCs, and
> > clarify i.MX eSDHC specific things to distingush them with QorIQ
> > eSDHC.
> >
> > This patch is to added an esdhc_imx flag for the development of i.MX
> > eSDHC, to distinguish it with QoriQ eSDHC.
> >
> > Signed-off-by: Yangbo Lu
> > ---
> > Changes for v2:
> > - Converted to use device_is_compatible().
> > ---
> > drivers/mmc/fsl_esdhc.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> > 21fa2ab1d4..a647bc7f1c 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
> > struct gpio_desc cd_gpio;
> > struct gpio_desc wp_gpio;
> > #endif
> > + bool esdhc_imx;
> > };
> >
> > /* Return the XFERTYP flags for a given command and data packet */ @@
> > -1462,6 +1463,16 @@ static int fsl_esdhc_probe(struct udevice *dev)
> > priv->caps = data->caps;
> > }
> >
> > + /*
> > +* This is to specify whether current eSDHC is an i.MX eSDHC,
> > +* since the i.MX eSDHC has been becoming more and more
> > different
> > +* with QorIQ eSDHC and initial MPC83XX/MPC85XX.
> > +*/
> > + if (!device_is_compatible(dev, "fsl,esdhc"))
> > + priv->esdhc_imx = true;
> > + else
> > + priv->esdhc_imx = false;
> > +
> > val = dev_read_u32_default(dev, "bus-width", -1);
> > if (val == 8)
> > priv->bus_width = 8;
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lu...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
Dear "Y.b. Lu",
> The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
> initially. The later QoriQ series processors (which were evolutions
> of MPC83XX/MPC85XX) and i.MX series processors were using this
> driver for their eSDHCs too.
>
> So there are two evolution directions for eSDHC now. For the two
> series processors, the eSDHCs are becoming more and more different.
> We should have split it into two drivers, like them
> (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
Why we cannot do it right just from the very beginning?
In the end of the day we will try to mimic Linux kernel anyway, IMHO it
is better to start the split now and save some effort on a half-step
solution.
> But it seemed
> to be a lot of work now. So let's keep as it is. Be very careful to
> change the driver if the changes are not common for all eSDHCs, and
> clarify i.MX eSDHC specific things to distingush them with QorIQ
> eSDHC.
>
> This patch is to added an esdhc_imx flag for the development of i.MX
> eSDHC, to distinguish it with QoriQ eSDHC.
>
> Signed-off-by: Yangbo Lu
> ---
> Changes for v2:
> - Converted to use device_is_compatible().
> ---
> drivers/mmc/fsl_esdhc.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index 21fa2ab1d4..a647bc7f1c 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
> struct gpio_desc cd_gpio;
> struct gpio_desc wp_gpio;
> #endif
> + bool esdhc_imx;
> };
>
> /* Return the XFERTYP flags for a given command and data packet */
> @@ -1462,6 +1463,16 @@ static int fsl_esdhc_probe(struct udevice *dev)
> priv->caps = data->caps;
> }
>
> + /*
> + * This is to specify whether current eSDHC is an i.MX eSDHC,
> + * since the i.MX eSDHC has been becoming more and more
> different
> + * with QorIQ eSDHC and initial MPC83XX/MPC85XX.
> + */
> + if (!device_is_compatible(dev, "fsl,esdhc"))
> + priv->esdhc_imx = true;
> + else
> + priv->esdhc_imx = false;
> +
> val = dev_read_u32_default(dev, "bus-width", -1);
> if (val == 8)
> priv->bus_width = 8;
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lu...@denx.de
pgpk92agCt6vB.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
> -Original Message-
> From: Y.b. Lu
> Sent: 2019年2月15日 10:26
> To: u-boot@lists.denx.de
> Cc: Jaehoon Chung ; Prabhakar Kushwaha
> ; Peng Fan ; Y.b. Lu
>
> Subject: [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
>
> The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX initially.
> The later QoriQ series processors (which were evolutions of
> MPC83XX/MPC85XX) and i.MX series processors were using this driver for
> their eSDHCs too.
>
> So there are two evolution directions for eSDHC now. For the two series
> processors, the eSDHCs are becoming more and more different.
> We should have split it into two drivers, like them
> (sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel. But it seemed to be a
> lot
> of work now. So let's keep as it is. Be very careful to change the driver if
> the
> changes are not common for all eSDHCs, and clarify i.MX eSDHC specific
> things to distingush them with QorIQ eSDHC.
>
> This patch is to added an esdhc_imx flag for the development of i.MX eSDHC,
> to distinguish it with QoriQ eSDHC.
>
> Signed-off-by: Yangbo Lu
> ---
> Changes for v2:
> - Converted to use device_is_compatible().
> ---
> drivers/mmc/fsl_esdhc.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index
> 21fa2ab1d4..a647bc7f1c 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
> struct gpio_desc cd_gpio;
> struct gpio_desc wp_gpio;
> #endif
> + bool esdhc_imx;
> };
>
> /* Return the XFERTYP flags for a given command and data packet */ @@
> -1462,6 +1463,16 @@ static int fsl_esdhc_probe(struct udevice *dev)
> priv->caps = data->caps;
> }
>
> + /*
> + * This is to specify whether current eSDHC is an i.MX eSDHC,
> + * since the i.MX eSDHC has been becoming more and more different
> + * with QorIQ eSDHC and initial MPC83XX/MPC85XX.
> + */
> + if (!device_is_compatible(dev, "fsl,esdhc"))
> + priv->esdhc_imx = true;
> + else
> + priv->esdhc_imx = false;
> +
> val = dev_read_u32_default(dev, "bus-width", -1);
> if (val == 8)
> priv->bus_width = 8;
Reviewed-by: Peng Fan
> --
> 2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [v2, 1/3] mmc: fsl_esdhc: add esdhc_imx flag
The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
initially. The later QoriQ series processors (which were evolutions
of MPC83XX/MPC85XX) and i.MX series processors were using this
driver for their eSDHCs too.
So there are two evolution directions for eSDHC now. For the two
series processors, the eSDHCs are becoming more and more different.
We should have split it into two drivers, like them
(sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel. But it seemed
to be a lot of work now. So let's keep as it is. Be very careful to
change the driver if the changes are not common for all eSDHCs, and
clarify i.MX eSDHC specific things to distingush them with QorIQ
eSDHC.
This patch is to added an esdhc_imx flag for the development of i.MX
eSDHC, to distinguish it with QoriQ eSDHC.
Signed-off-by: Yangbo Lu
---
Changes for v2:
- Converted to use device_is_compatible().
---
drivers/mmc/fsl_esdhc.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 21fa2ab1d4..a647bc7f1c 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -147,6 +147,7 @@ struct fsl_esdhc_priv {
struct gpio_desc cd_gpio;
struct gpio_desc wp_gpio;
#endif
+ bool esdhc_imx;
};
/* Return the XFERTYP flags for a given command and data packet */
@@ -1462,6 +1463,16 @@ static int fsl_esdhc_probe(struct udevice *dev)
priv->caps = data->caps;
}
+ /*
+* This is to specify whether current eSDHC is an i.MX eSDHC,
+* since the i.MX eSDHC has been becoming more and more different
+* with QorIQ eSDHC and initial MPC83XX/MPC85XX.
+*/
+ if (!device_is_compatible(dev, "fsl,esdhc"))
+ priv->esdhc_imx = true;
+ else
+ priv->esdhc_imx = false;
+
val = dev_read_u32_default(dev, "bus-width", -1);
if (val == 8)
priv->bus_width = 8;
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
