| Message ID | 34e51d32c82b57571e061e80df3bbaed731db554.1487091464.git-series.gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14 February 2017 at 18:01, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > From: Hu Ziji <huziji@marvell.com> > > Some vendor host, like Xenon, can support multiple types. > In dts, use mmc-card dt sub-node to indicate eMMC is in use. > > Add a generic mmc-card parse function in mmc core layer. > If mmc-card sub-node is detected, set eMMC common caps, such as > MMC_CAP_NONREMOVABLE, MMC_CAP2_NO_SD and MMC_CAP2_NO_SDIO. > > Since it is likely that struct mmc_card is not allocated yet when > this mmc-card parse function is called, it will return true if > mmc-card sub-node is detected. Otherwise, return false. > It can help vendor host determine if the card type is eMMC. > > Signed-off-by: Hu Ziji <huziji@marvell.com> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > drivers/mmc/core/mmc.c | 24 ++++++++++++++++++++++++ > include/linux/mmc/core.h | 2 ++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index b61b52f9da3d..dc480006a303 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -2111,6 +2111,30 @@ static const struct mmc_bus_ops mmc_ops = { > }; > > /* > + * Parse mmc-card dt sub-node and set eMMC common caps > + * if mmc-card exists. > + * If mmc-card is detected, return true. > + * Otherwise, return false. > + */ > +bool mmc_of_parse_mmc_card(struct mmc_host *host) > +{ > + struct device_node *np; > + bool ret = false; > + > + np = mmc_of_find_child_device(host, 0); There are already some places in the mmc core where child nodes are being parsed. You may have a look for mmc_of_find_child_device() and find the callers of it. Additionally, we need a generic method of how to describe in DT, when an mmc host controller has more than one mmc slot. This will also be done using child nodes. So, before starting hacking on this, it seems like we need some consolidation of the code. In principle, I would like to move the APIs for parsing of the child nodes into host.c, along with existing mmc_of_parse() function. > + if (np && of_device_is_compatible(np, "mmc-card")) { > + /* mmc-card sub-node indicates eMMC card is in use. */ > + host->caps |= MMC_CAP_NONREMOVABLE; > + host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD; > + ret = true; > + } > + So instead of providing a new mmc OF parse API, let's make mmc_of_parse() call another internal function which deals with child node parsing. > + of_node_put(np); > + return ret; > +} > +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card); > + > +/* > * Starting point for MMC card init. > */ > int mmc_attach_mmc(struct mmc_host *host) > diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h > index e33cc748dcfe..1b27323f3b6e 100644 > --- a/include/linux/mmc/core.h > +++ b/include/linux/mmc/core.h > @@ -234,4 +234,6 @@ struct device_node; > extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max); > extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask); > > +extern bool mmc_of_parse_mmc_card(struct mmc_host *host); > + > #endif /* LINUX_MMC_CORE_H */ > -- > git-series 0.9.1 If you didn't quite understand my comments, then please tell me. Then I can help out cooking some patches for you. Kind regards Uffe
Hi Ulf, On 2017/3/15 20:43, Ulf Hansson wrote: > On 14 February 2017 at 18:01, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> From: Hu Ziji <huziji@marvell.com> >> >> Some vendor host, like Xenon, can support multiple types. >> In dts, use mmc-card dt sub-node to indicate eMMC is in use. >> >> Add a generic mmc-card parse function in mmc core layer. >> If mmc-card sub-node is detected, set eMMC common caps, such as >> MMC_CAP_NONREMOVABLE, MMC_CAP2_NO_SD and MMC_CAP2_NO_SDIO. >> >> Since it is likely that struct mmc_card is not allocated yet when >> this mmc-card parse function is called, it will return true if >> mmc-card sub-node is detected. Otherwise, return false. >> It can help vendor host determine if the card type is eMMC. >> <snip> >> +bool mmc_of_parse_mmc_card(struct mmc_host *host) >> +{ >> + struct device_node *np; >> + bool ret = false; >> + >> + np = mmc_of_find_child_device(host, 0); > > There are already some places in the mmc core where child nodes are > being parsed. You may have a look for mmc_of_find_child_device() and > find the callers of it. > > Additionally, we need a generic method of how to describe in DT, when > an mmc host controller has more than one mmc slot. This will also be > done using child nodes. > I'd like to confirm if you require a universal child node parsing function to deal with all the child node use cases. It might be a little difficult to us. > So, before starting hacking on this, it seems like we need some > consolidation of the code. In principle, I would like to move the APIs > for parsing of the child nodes into host.c, along with existing > mmc_of_parse() function. > I guess that your requirement is like: int mmc_of_pars(struct mmc_host *host) { ... np = mmc_of_find_child_device(host, 0); if (np && of_device_is_compatible(np, "mmc-card")) { ... } ... } Am I correct? Actually, I previously tried this implementation. But I found that it is difficult to return eMMC card type information to our vendor driver. Our Xenon driver expects to know if the device is eMMC card through this parse. >> + if (np && of_device_is_compatible(np, "mmc-card")) { >> + /* mmc-card sub-node indicates eMMC card is in use. */ >> + host->caps |= MMC_CAP_NONREMOVABLE; >> + host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD; >> + ret = true; >> + } >> + > > So instead of providing a new mmc OF parse API, let's make > mmc_of_parse() call another internal function which deals with child > node parsing. > >> + of_node_put(np); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card); >> + >> +/* >> * Starting point for MMC card init. >> */ >> int mmc_attach_mmc(struct mmc_host *host) >> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h >> index e33cc748dcfe..1b27323f3b6e 100644 >> --- a/include/linux/mmc/core.h >> +++ b/include/linux/mmc/core.h >> @@ -234,4 +234,6 @@ struct device_node; >> extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max); >> extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask); >> >> +extern bool mmc_of_parse_mmc_card(struct mmc_host *host); >> + >> #endif /* LINUX_MMC_CORE_H */ >> -- >> git-series 0.9.1 > > If you didn't quite understand my comments, then please tell me. Then > I can help out cooking some patches for you. Thanks a lot. Actually I'm not sure if our implementation to detect eMMC card type can meet your requirement of child node parsing. Please help provide me some examples or patches. Thank you. Best regards, Hu Ziji > > Kind regards > Uffe >
Hi Ulf, On mer., mars 15 2017, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 14 February 2017 at 18:01, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: >> From: Hu Ziji <huziji@marvell.com> >> >> Some vendor host, like Xenon, can support multiple types. >> In dts, use mmc-card dt sub-node to indicate eMMC is in use. >> >> Add a generic mmc-card parse function in mmc core layer. >> If mmc-card sub-node is detected, set eMMC common caps, such as >> MMC_CAP_NONREMOVABLE, MMC_CAP2_NO_SD and MMC_CAP2_NO_SDIO. >> >> Since it is likely that struct mmc_card is not allocated yet when >> this mmc-card parse function is called, it will return true if >> mmc-card sub-node is detected. Otherwise, return false. >> It can help vendor host determine if the card type is eMMC. >> >> Signed-off-by: Hu Ziji <huziji@marvell.com> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> --- >> drivers/mmc/core/mmc.c | 24 ++++++++++++++++++++++++ >> include/linux/mmc/core.h | 2 ++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >> index b61b52f9da3d..dc480006a303 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -2111,6 +2111,30 @@ static const struct mmc_bus_ops mmc_ops = { >> }; >> >> /* >> + * Parse mmc-card dt sub-node and set eMMC common caps >> + * if mmc-card exists. >> + * If mmc-card is detected, return true. >> + * Otherwise, return false. >> + */ >> +bool mmc_of_parse_mmc_card(struct mmc_host *host) >> +{ >> + struct device_node *np; >> + bool ret = false; >> + >> + np = mmc_of_find_child_device(host, 0); > > There are already some places in the mmc core where child nodes are > being parsed. You may have a look for mmc_of_find_child_device() and > find the callers of it. mmc_of_find_child_device is called in 3 places: - in mmc_add_card() from drivers/mmc/core/bus.c to populate the of_node field of card->dev. Here we don't care of the mmc-card property - in mmc_decode_ext_csd() from drivers/mmc/core/mmc.c, here the purpose is exactly to check if the mmc-card property is present so we could replace it by mmc_of_parse_mmc_card(). - in sdio_set_of_node from drivers/mmc/core/sdio_bus.c to populate the of_node field of func->dev. Here again, we don't care of the mmc-card property So in one case we could use the same function but for the 2 other cases continue to use mmc_of_find_child_device() alone make sens. > > Additionally, we need a generic method of how to describe in DT, when > an mmc host controller has more than one mmc slot. This will also be > done using child nodes. > > So, before starting hacking on this, it seems like we need some > consolidation of the code. In principle, I would like to move the APIs > for parsing of the child nodes into host.c, along with existing > mmc_of_parse() function. I understand that you want consolidate this subsystem but it seems beyond the this series. I would not want that the merge of this driver will be delayed more because of it. We submitted the first version 5 month ago now, made several changes and we will reach the 7th version. Along these version we also already contribute to improve the mmc subsystem. I agree to work on the change you want, but I would like not depending of it to merge this series. We will address all the other comment on v6 from you and Andrian for a v7. And for this patch we can: - keep it as is and only modify the mmc_decode_ext_csd() to use mmc_of_find_child_device(). - Moving it back inside our driver waiting for a better API in the mmc subsystem. What would be your preference? > >> + if (np && of_device_is_compatible(np, "mmc-card")) { >> + /* mmc-card sub-node indicates eMMC card is in use. */ >> + host->caps |= MMC_CAP_NONREMOVABLE; >> + host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD; >> + ret = true; >> + } >> + > > So instead of providing a new mmc OF parse API, let's make > mmc_of_parse() call another internal function which deals with child > node parsing. As the driver need to know if the card is an emmc then we need to alos expose this information in one of the mmc structure, waht about "struct mmc_host"? Thanks, Gregory > >> + of_node_put(np); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card); >> + >> +/* >> * Starting point for MMC card init. >> */ >> int mmc_attach_mmc(struct mmc_host *host) >> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h >> index e33cc748dcfe..1b27323f3b6e 100644 >> --- a/include/linux/mmc/core.h >> +++ b/include/linux/mmc/core.h >> @@ -234,4 +234,6 @@ struct device_node; >> extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max); >> extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask); >> >> +extern bool mmc_of_parse_mmc_card(struct mmc_host *host); >> + >> #endif /* LINUX_MMC_CORE_H */ >> -- >> git-series 0.9.1 > > If you didn't quite understand my comments, then please tell me. Then > I can help out cooking some patches for you. > > Kind regards > Uffe
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index b61b52f9da3d..dc480006a303 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -2111,6 +2111,30 @@ static const struct mmc_bus_ops mmc_ops = { }; /* + * Parse mmc-card dt sub-node and set eMMC common caps + * if mmc-card exists. + * If mmc-card is detected, return true. + * Otherwise, return false. + */ +bool mmc_of_parse_mmc_card(struct mmc_host *host) +{ + struct device_node *np; + bool ret = false; + + np = mmc_of_find_child_device(host, 0); + if (np && of_device_is_compatible(np, "mmc-card")) { + /* mmc-card sub-node indicates eMMC card is in use. */ + host->caps |= MMC_CAP_NONREMOVABLE; + host->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_NO_SD; + ret = true; + } + + of_node_put(np); + return ret; +} +EXPORT_SYMBOL_GPL(mmc_of_parse_mmc_card); + +/* * Starting point for MMC card init. */ int mmc_attach_mmc(struct mmc_host *host) diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index e33cc748dcfe..1b27323f3b6e 100644 --- a/include/linux/mmc/core.h +++ b/include/linux/mmc/core.h @@ -234,4 +234,6 @@ struct device_node; extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max); extern int mmc_of_parse_voltage(struct device_node *np, u32 *mask); +extern bool mmc_of_parse_mmc_card(struct mmc_host *host); + #endif /* LINUX_MMC_CORE_H */