From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932463AbdA3Pf1 (ORCPT ); Mon, 30 Jan 2017 10:35:27 -0500 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:55318 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932424AbdA3Pdk (ORCPT ); Mon, 30 Jan 2017 10:33:40 -0500 Subject: Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality To: Ulf Hansson References: <3546f143091c121face8ecbcbf7f6a4c41d2cec7.1484154449.git-series.gregory.clement@free-electrons.com> <461fd409-81e8-2c3f-3674-ec593e1c7383@marvell.com> CC: Gregory CLEMENT , Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Mike Turquette , Stephen Boyd , linux-clk , "linux-kernel@vger.kernel.org" , Rob Herring , "devicetree@vger.kernel.org" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin From: Ziji Hu Message-ID: Date: Mon, 30 Jan 2017 23:12:10 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-30_09:,, signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701300146 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ulf, On 2017/1/30 16:41, Ulf Hansson wrote: > [...] > >>>> + */ >>>> +static int xenon_child_node_of_parse(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct sdhci_host *host = platform_get_drvdata(pdev); >>>> + struct mmc_host *mmc = host->mmc; >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + struct device_node *child; >>>> + int nr_child; >>>> + >>>> + priv->init_card_type = XENON_CARD_TYPE_UNKNOWN; >>>> + >>>> + nr_child = of_get_child_count(np); >>>> + if (!nr_child) >>>> + return 0; >>>> + >>>> + for_each_child_of_node(np, child) { >>>> + if (of_device_is_compatible(child, "mmc-card")) { >>> >>> To avoid code from being duplicated, I would rather see the DT parsing >>> of the child nodes for "mmc-card", to be done by the mmc core. >>> >>> As a matter of fact it is already being done, but perhaps we need to >>> change that a bit as to fit your case. >>> >>> I suggest you have a look and see how to update this in the core, >>> instead of doing it here in the host driver. >>> >> >> I understand your concern. >> >> It seems that so far "mmc-card" is only used in our Xenon driver. > > git grep "mmc-card" tells you more about where it's being parsed by > the mmc core. > >> Besides, we set Xenon specific parameters and attributions when >> parsing "mmc-card" property. > > I don't see any Xenon specific properties. > > Instead I think it would make sense to update the generic > interpretation of the binding for mmc-card, as you have done here. > OK. I will merge it into core layer. Our Xenon driver requires to determine whether current SDHC is for eMMC before card init. Thus I would like to implement a specific function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c to call it. But there are some detailed issues then. 1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi. So mmc-card parse will still be duplicated. Shall we merge broken-hpi check into mmc-card parse? It might require a new flag to indicate broken-hpi attribution in mmc_host structure. 2. Structure mmc_card is not ready while parsing mmc-card. Thus we are not able to indicate card type in mmc_card. As a result, our Xenon specific parse function still has to parse mmc-card again to check if eMMC card is in used. Could you please help suggest any better place in core layer to parse mmc-card? Thank you. Best regards, Hu Ziji >> >> May I keep current implementation? > > Rather not. Let's try to make the parsing of the child node for > mmc-card generic. > >> In my very own opinion, moving it into core layer should be another >> independent patch. > > Absolutely an independent patch. Whether it can be done as a part of > mmc_of_parse() or whether we need yet another new mmc_of* API, we can > discuss. > > My point is that, I don't this to be specific for Xenon (unless there > are specific reason, which I don't see here). > >> And it will also cost some more time. To be honest, it is difficult >> for me to bring up a generic core layer implementation to parse >> "mmc-card", since I'm not clear about other vendor's requirement. > > Well, then you need to learn more about how the mmc core works. In > this case, it shouldn't be too hard to implement. > > [...] > >> >>> >>>> + MMC_CAP2_NO_SD | >>>> + MMC_CAP2_NO_SDIO; >>> >>> I think we need to update the DT documentation for mmc-card. >>> Typically, we should explicitly state what kind of other existing mmc >>> DT properties that will be automatically selected, when the mmc-card >>> is specified. >>> >>> Can you please look into updating this DT doc as well. >>> >> >> Similar to above, may I keep it now and bring up another patch later >> to update mmc-card DT and parsing? > > Please, no. > > [...] > > Kind regards > Uffe > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ziji Hu Subject: Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality Date: Mon, 30 Jan 2017 23:12:10 +0800 Message-ID: References: <3546f143091c121face8ecbcbf7f6a4c41d2cec7.1484154449.git-series.gregory.clement@free-electrons.com> <461fd409-81e8-2c3f-3674-ec593e1c7383@marvell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Ulf Hansson Cc: Jimmy Xu , Andrew Lunn , "linux-mmc@vger.kernel.org" , Mike Turquette , "linux-kernel@vger.kernel.org" , Nadav Haklai , Victor Gu , Doug Jones , linux-clk , Jisheng Zhang , Yehuda Yitschak , Marcin Wojtas , Kostya Porotchkin , Hanna Hawa , Sebastian Hesselbarth , "devicetree@vger.kernel.org" , Jason Cooper , Rob Herring , Ryan Gao , Gregory CLEMENT , "Wei(SOCP) Liu" , linux-arm-kernel@lists.infradead. List-Id: devicetree@vger.kernel.org Hi Ulf, On 2017/1/30 16:41, Ulf Hansson wrote: > [...] > >>>> + */ >>>> +static int xenon_child_node_of_parse(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct sdhci_host *host = platform_get_drvdata(pdev); >>>> + struct mmc_host *mmc = host->mmc; >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + struct device_node *child; >>>> + int nr_child; >>>> + >>>> + priv->init_card_type = XENON_CARD_TYPE_UNKNOWN; >>>> + >>>> + nr_child = of_get_child_count(np); >>>> + if (!nr_child) >>>> + return 0; >>>> + >>>> + for_each_child_of_node(np, child) { >>>> + if (of_device_is_compatible(child, "mmc-card")) { >>> >>> To avoid code from being duplicated, I would rather see the DT parsing >>> of the child nodes for "mmc-card", to be done by the mmc core. >>> >>> As a matter of fact it is already being done, but perhaps we need to >>> change that a bit as to fit your case. >>> >>> I suggest you have a look and see how to update this in the core, >>> instead of doing it here in the host driver. >>> >> >> I understand your concern. >> >> It seems that so far "mmc-card" is only used in our Xenon driver. > > git grep "mmc-card" tells you more about where it's being parsed by > the mmc core. > >> Besides, we set Xenon specific parameters and attributions when >> parsing "mmc-card" property. > > I don't see any Xenon specific properties. > > Instead I think it would make sense to update the generic > interpretation of the binding for mmc-card, as you have done here. > OK. I will merge it into core layer. Our Xenon driver requires to determine whether current SDHC is for eMMC before card init. Thus I would like to implement a specific function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c to call it. But there are some detailed issues then. 1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi. So mmc-card parse will still be duplicated. Shall we merge broken-hpi check into mmc-card parse? It might require a new flag to indicate broken-hpi attribution in mmc_host structure. 2. Structure mmc_card is not ready while parsing mmc-card. Thus we are not able to indicate card type in mmc_card. As a result, our Xenon specific parse function still has to parse mmc-card again to check if eMMC card is in used. Could you please help suggest any better place in core layer to parse mmc-card? Thank you. Best regards, Hu Ziji >> >> May I keep current implementation? > > Rather not. Let's try to make the parsing of the child node for > mmc-card generic. > >> In my very own opinion, moving it into core layer should be another >> independent patch. > > Absolutely an independent patch. Whether it can be done as a part of > mmc_of_parse() or whether we need yet another new mmc_of* API, we can > discuss. > > My point is that, I don't this to be specific for Xenon (unless there > are specific reason, which I don't see here). > >> And it will also cost some more time. To be honest, it is difficult >> for me to bring up a generic core layer implementation to parse >> "mmc-card", since I'm not clear about other vendor's requirement. > > Well, then you need to learn more about how the mmc core works. In > this case, it shouldn't be too hard to implement. > > [...] > >> >>> >>>> + MMC_CAP2_NO_SD | >>>> + MMC_CAP2_NO_SDIO; >>> >>> I think we need to update the DT documentation for mmc-card. >>> Typically, we should explicitly state what kind of other existing mmc >>> DT properties that will be automatically selected, when the mmc-card >>> is specified. >>> >>> Can you please look into updating this DT doc as well. >>> >> >> Similar to above, may I keep it now and bring up another patch later >> to update mmc-card DT and parsing? > > Please, no. > > [...] > > Kind regards > Uffe > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality To: Ulf Hansson References: <3546f143091c121face8ecbcbf7f6a4c41d2cec7.1484154449.git-series.gregory.clement@free-electrons.com> <461fd409-81e8-2c3f-3674-ec593e1c7383@marvell.com> CC: Gregory CLEMENT , Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Mike Turquette , Stephen Boyd , linux-clk , "linux-kernel@vger.kernel.org" , Rob Herring , "devicetree@vger.kernel.org" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin From: Ziji Hu Message-ID: Date: Mon, 30 Jan 2017 23:12:10 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" List-ID: Hi Ulf, On 2017/1/30 16:41, Ulf Hansson wrote: > [...] > >>>> + */ >>>> +static int xenon_child_node_of_parse(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct sdhci_host *host = platform_get_drvdata(pdev); >>>> + struct mmc_host *mmc = host->mmc; >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + struct device_node *child; >>>> + int nr_child; >>>> + >>>> + priv->init_card_type = XENON_CARD_TYPE_UNKNOWN; >>>> + >>>> + nr_child = of_get_child_count(np); >>>> + if (!nr_child) >>>> + return 0; >>>> + >>>> + for_each_child_of_node(np, child) { >>>> + if (of_device_is_compatible(child, "mmc-card")) { >>> >>> To avoid code from being duplicated, I would rather see the DT parsing >>> of the child nodes for "mmc-card", to be done by the mmc core. >>> >>> As a matter of fact it is already being done, but perhaps we need to >>> change that a bit as to fit your case. >>> >>> I suggest you have a look and see how to update this in the core, >>> instead of doing it here in the host driver. >>> >> >> I understand your concern. >> >> It seems that so far "mmc-card" is only used in our Xenon driver. > > git grep "mmc-card" tells you more about where it's being parsed by > the mmc core. > >> Besides, we set Xenon specific parameters and attributions when >> parsing "mmc-card" property. > > I don't see any Xenon specific properties. > > Instead I think it would make sense to update the generic > interpretation of the binding for mmc-card, as you have done here. > OK. I will merge it into core layer. Our Xenon driver requires to determine whether current SDHC is for eMMC before card init. Thus I would like to implement a specific function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c to call it. But there are some detailed issues then. 1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi. So mmc-card parse will still be duplicated. Shall we merge broken-hpi check into mmc-card parse? It might require a new flag to indicate broken-hpi attribution in mmc_host structure. 2. Structure mmc_card is not ready while parsing mmc-card. Thus we are not able to indicate card type in mmc_card. As a result, our Xenon specific parse function still has to parse mmc-card again to check if eMMC card is in used. Could you please help suggest any better place in core layer to parse mmc-card? Thank you. Best regards, Hu Ziji >> >> May I keep current implementation? > > Rather not. Let's try to make the parsing of the child node for > mmc-card generic. > >> In my very own opinion, moving it into core layer should be another >> independent patch. > > Absolutely an independent patch. Whether it can be done as a part of > mmc_of_parse() or whether we need yet another new mmc_of* API, we can > discuss. > > My point is that, I don't this to be specific for Xenon (unless there > are specific reason, which I don't see here). > >> And it will also cost some more time. To be honest, it is difficult >> for me to bring up a generic core layer implementation to parse >> "mmc-card", since I'm not clear about other vendor's requirement. > > Well, then you need to learn more about how the mmc core works. In > this case, it shouldn't be too hard to implement. > > [...] > >> >>> >>>> + MMC_CAP2_NO_SD | >>>> + MMC_CAP2_NO_SDIO; >>> >>> I think we need to update the DT documentation for mmc-card. >>> Typically, we should explicitly state what kind of other existing mmc >>> DT properties that will be automatically selected, when the mmc-card >>> is specified. >>> >>> Can you please look into updating this DT doc as well. >>> >> >> Similar to above, may I keep it now and bring up another patch later >> to update mmc-card DT and parsing? > > Please, no. > > [...] > > Kind regards > Uffe > From mboxrd@z Thu Jan 1 00:00:00 1970 From: huziji@marvell.com (Ziji Hu) Date: Mon, 30 Jan 2017 23:12:10 +0800 Subject: [PATCH v5 06/12] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality In-Reply-To: References: <3546f143091c121face8ecbcbf7f6a4c41d2cec7.1484154449.git-series.gregory.clement@free-electrons.com> <461fd409-81e8-2c3f-3674-ec593e1c7383@marvell.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Ulf, On 2017/1/30 16:41, Ulf Hansson wrote: > [...] > >>>> + */ >>>> +static int xenon_child_node_of_parse(struct platform_device *pdev) >>>> +{ >>>> + struct device_node *np = pdev->dev.of_node; >>>> + struct sdhci_host *host = platform_get_drvdata(pdev); >>>> + struct mmc_host *mmc = host->mmc; >>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); >>>> + struct device_node *child; >>>> + int nr_child; >>>> + >>>> + priv->init_card_type = XENON_CARD_TYPE_UNKNOWN; >>>> + >>>> + nr_child = of_get_child_count(np); >>>> + if (!nr_child) >>>> + return 0; >>>> + >>>> + for_each_child_of_node(np, child) { >>>> + if (of_device_is_compatible(child, "mmc-card")) { >>> >>> To avoid code from being duplicated, I would rather see the DT parsing >>> of the child nodes for "mmc-card", to be done by the mmc core. >>> >>> As a matter of fact it is already being done, but perhaps we need to >>> change that a bit as to fit your case. >>> >>> I suggest you have a look and see how to update this in the core, >>> instead of doing it here in the host driver. >>> >> >> I understand your concern. >> >> It seems that so far "mmc-card" is only used in our Xenon driver. > > git grep "mmc-card" tells you more about where it's being parsed by > the mmc core. > >> Besides, we set Xenon specific parameters and attributions when >> parsing "mmc-card" property. > > I don't see any Xenon specific properties. > > Instead I think it would make sense to update the generic > interpretation of the binding for mmc-card, as you have done here. > OK. I will merge it into core layer. Our Xenon driver requires to determine whether current SDHC is for eMMC before card init. Thus I would like to implement a specific function for mmc-card parsing in mmc.c and let mmc_of_parse() in host.c to call it. But there are some detailed issues then. 1. mmc_decode_ext_csd() also parses mmc-card to check broken-hpi. So mmc-card parse will still be duplicated. Shall we merge broken-hpi check into mmc-card parse? It might require a new flag to indicate broken-hpi attribution in mmc_host structure. 2. Structure mmc_card is not ready while parsing mmc-card. Thus we are not able to indicate card type in mmc_card. As a result, our Xenon specific parse function still has to parse mmc-card again to check if eMMC card is in used. Could you please help suggest any better place in core layer to parse mmc-card? Thank you. Best regards, Hu Ziji >> >> May I keep current implementation? > > Rather not. Let's try to make the parsing of the child node for > mmc-card generic. > >> In my very own opinion, moving it into core layer should be another >> independent patch. > > Absolutely an independent patch. Whether it can be done as a part of > mmc_of_parse() or whether we need yet another new mmc_of* API, we can > discuss. > > My point is that, I don't this to be specific for Xenon (unless there > are specific reason, which I don't see here). > >> And it will also cost some more time. To be honest, it is difficult >> for me to bring up a generic core layer implementation to parse >> "mmc-card", since I'm not clear about other vendor's requirement. > > Well, then you need to learn more about how the mmc core works. In > this case, it shouldn't be too hard to implement. > > [...] > >> >>> >>>> + MMC_CAP2_NO_SD | >>>> + MMC_CAP2_NO_SDIO; >>> >>> I think we need to update the DT documentation for mmc-card. >>> Typically, we should explicitly state what kind of other existing mmc >>> DT properties that will be automatically selected, when the mmc-card >>> is specified. >>> >>> Can you please look into updating this DT doc as well. >>> >> >> Similar to above, may I keep it now and bring up another patch later >> to update mmc-card DT and parsing? > > Please, no. > > [...] > > Kind regards > Uffe >