| Message ID | 20190307084329.25416-2-christoph.muellner@theobroma-systems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] mmc: sdhci-of-arasan: Add DTS property to disable DCMDs. | expand |
On 7/03/19 10:43 AM, Christoph Muellner wrote: > This patch documents the new property disable-cqe-dcmd > for the Arasan eMMC 5.1 driver. > > Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > index 1edbb049cccb..ec699bf98b7c 100644 > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > @@ -44,6 +44,10 @@ Optional Properties: > properly. Test mode can be used to force the controller to function. > - xlnx,int-clock-stable-broken: when present, the controller always reports > that the internal clock is stable even when it is not. > + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) > + as part of the command queue engine (CQE). On controllers with a CQHCI, > + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. > + This is done unless disable-cqe-dcmd is specified. If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also?
> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 7/03/19 10:43 AM, Christoph Muellner wrote: >> This patch documents the new property disable-cqe-dcmd >> for the Arasan eMMC 5.1 driver. >> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >> --- >> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >> index 1edbb049cccb..ec699bf98b7c 100644 >> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >> @@ -44,6 +44,10 @@ Optional Properties: >> properly. Test mode can be used to force the controller to function. >> - xlnx,int-clock-stable-broken: when present, the controller always reports >> that the internal clock is stable even when it is not. >> + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) >> + as part of the command queue engine (CQE). On controllers with a CQHCI, >> + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. >> + This is done unless disable-cqe-dcmd is specified. > > If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also? The file mmc.txt says on top: "These properties are common to multiple MMC host controllers". As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan, I would say it should not go into that file. Also I wonder why "supports-cqe" is in mmc.txt, because only sdhci-tegra.c is evaluating that property. So I would expect it to be documented in Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt However, I see that "disable-cqe-dcmd" could go into other drivers as well. But is this enough to document it in mmc.txt?
Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner: > > > On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote: > > > > On 7/03/19 10:43 AM, Christoph Muellner wrote: > >> This patch documents the new property disable-cqe-dcmd > >> for the Arasan eMMC 5.1 driver. > >> > >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> > >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > >> --- > >> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >> index 1edbb049cccb..ec699bf98b7c 100644 > >> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >> @@ -44,6 +44,10 @@ Optional Properties: > >> properly. Test mode can be used to force the controller to function. > >> - xlnx,int-clock-stable-broken: when present, the controller always reports > >> that the internal clock is stable even when it is not. > >> + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) > >> + as part of the command queue engine (CQE). On controllers with a CQHCI, > >> + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. > >> + This is done unless disable-cqe-dcmd is specified. This needs a rewording please. See below for hw-description vs. driver, so the description should be centered around why this is a property of the hw [like faulty controller implementation or whatever] > > If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also? > > The file mmc.txt says on top: > "These properties are common to multiple MMC host controllers". > As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan, > I would say it should not go into that file. > > Also I wonder why "supports-cqe" is in mmc.txt, because > only sdhci-tegra.c is evaluating that property. > So I would expect it to be documented in > Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt > > However, I see that "disable-cqe-dcmd" could go into other drivers as well. > But is this enough to document it in mmc.txt? Devicetree is always about describing the hardware capabilites and never about the actual nitty-gritty of driver implementation, aka it is not meant as a space for hardware-independent config-settings or such. As for only tegra evaluating this, is probably because it is still so new, like january 2019 and Rob explicitly suggested it becoming common [0], which suggests that the disable-cqe-dcmds should probably also be common. Heiko [0] https://lore.kernel.org/patchwork/patch/1031163/
> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote: > > Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner: >> >>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> >>> On 7/03/19 10:43 AM, Christoph Muellner wrote: >>>> This patch documents the new property disable-cqe-dcmd >>>> for the Arasan eMMC 5.1 driver. >>>> >>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> >>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> >>>> --- >>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>> index 1edbb049cccb..ec699bf98b7c 100644 >>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>> @@ -44,6 +44,10 @@ Optional Properties: >>>> properly. Test mode can be used to force the controller to function. >>>> - xlnx,int-clock-stable-broken: when present, the controller always reports >>>> that the internal clock is stable even when it is not. >>>> + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) >>>> + as part of the command queue engine (CQE). On controllers with a CQHCI, >>>> + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. >>>> + This is done unless disable-cqe-dcmd is specified. > > This needs a rewording please. See below for hw-description vs. driver, so > the description should be centered around why this is a property of the hw > [like faulty controller implementation or whatever] I understand, that you prefer a HW-specific name for a property over one, that explains the actual effect. However, using disable-<feature> (which is always a directive for the driver and not a HW description) is not uncommon: * disable-wp should be "no-wp-line-connection" * disable-over-current -> "no-over-current-line-connection" * srp-disable -> "not-existing-srp-implementation" * ... But I don't mind using something else. Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok? Or other suggestions? Also I'd like to mention, that my first implementation was "supports-cqe-dcmd". I guess that would be a good choice, if it would have been there from the beginning. Introducing it now, would silently disable DCMD for existing DTBs. Therefore I went on to "disable-cqe-dcmd". > > >>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there also? >> >> The file mmc.txt says on top: >> "These properties are common to multiple MMC host controllers". >> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan, >> I would say it should not go into that file. >> >> Also I wonder why "supports-cqe" is in mmc.txt, because >> only sdhci-tegra.c is evaluating that property. >> So I would expect it to be documented in >> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt >> >> However, I see that "disable-cqe-dcmd" could go into other drivers as well. >> But is this enough to document it in mmc.txt? > > Devicetree is always about describing the hardware capabilites and never > about the actual nitty-gritty of driver implementation, aka it is not meant > as a space for hardware-independent config-settings or such. > > As for only tegra evaluating this, is probably because it is still so new, like > january 2019 and Rob explicitly suggested it becoming common [0], which > suggests that the disable-cqe-dcmds should probably also be common. So mmc.txt lists "standardised" names for properties to reduce the risk of having similar, but distinct names for different MMC drivers. So whenever you want to introduce a new property for a driver, check if there isn't already something defined in mmc.txt. When seeing it this way, it clearly makes sense to have the property there. Thanks, Christoph
Hi Christoph, Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner: > > On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote: > > > > Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner: > >>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote: > >>> > >>> On 7/03/19 10:43 AM, Christoph Muellner wrote: > >>>> This patch documents the new property disable-cqe-dcmd > >>>> for the Arasan eMMC 5.1 driver. > >>>> > >>>> Signed-off-by: Christoph Muellner > >>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp > >>>> Tomsich <philipp.tomsich@theobroma-systems.com> --- > >>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index > >>>> 1edbb049cccb..ec699bf98b7c 100644 > >>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > >>>> > >>>> @@ -44,6 +44,10 @@ Optional Properties: > >>>> properly. Test mode can be used to force the controller to function. > >>>> > >>>> - xlnx,int-clock-stable-broken: when present, the controller always > >>>> reports > >>>> > >>>> that the internal clock is stable even when it is not. > >>>> > >>>> + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands > >>>> (DCMDs) + as part of the command queue engine (CQE). On controllers > >>>> with a CQHCI, + such as the Arasan eMMC 5.1 host controller, the > >>>> driver has to enable DCMDs. + This is done unless disable-cqe-dcmd > >>>> is specified. > > > > This needs a rewording please. See below for hw-description vs. driver, so > > the description should be centered around why this is a property of the hw > > [like faulty controller implementation or whatever] > > I understand, that you prefer a HW-specific name for a property > over one, that explains the actual effect. Actually I think the property-name is just fine :-) . The description might profit from a rewording though. Aka not driver- centric but hw-centric, describing why some controllers may need the option to disable these dcmds (controller bugs or whatever). Heiko > However, using disable-<feature> (which is always a directive for the driver > and not a HW description) is not uncommon: > > * disable-wp should be "no-wp-line-connection" > * disable-over-current -> "no-over-current-line-connection" > * srp-disable -> "not-existing-srp-implementation" > * ... > > But I don't mind using something else. > Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok? > Or other suggestions? > > Also I'd like to mention, that my first implementation was > "supports-cqe-dcmd". I guess that would be a good choice, if it would have > been there > from the beginning. Introducing it now, would silently disable DCMD > for existing DTBs. Therefore I went on to "disable-cqe-dcmd". > > >>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there > >>> also? > >> > >> The file mmc.txt says on top: > >> "These properties are common to multiple MMC host controllers". > >> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan, > >> I would say it should not go into that file. > >> > >> Also I wonder why "supports-cqe" is in mmc.txt, because > >> only sdhci-tegra.c is evaluating that property. > >> So I would expect it to be documented in > >> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt > >> > >> However, I see that "disable-cqe-dcmd" could go into other drivers as > >> well. > >> But is this enough to document it in mmc.txt? > > > > Devicetree is always about describing the hardware capabilites and never > > about the actual nitty-gritty of driver implementation, aka it is not > > meant > > as a space for hardware-independent config-settings or such. > > > > As for only tegra evaluating this, is probably because it is still so new, > > like january 2019 and Rob explicitly suggested it becoming common [0], > > which suggests that the disable-cqe-dcmds should probably also be common. > So mmc.txt lists "standardised" names for properties to reduce the risk > of having similar, but distinct names for different MMC drivers. > So whenever you want to introduce a new property for a driver, > check if there isn't already something defined in mmc.txt. > > When seeing it this way, it clearly makes sense to have the property there. > > Thanks, > Christoph
> On 12.03.2019, at 18:10, Heiko Stübner <heiko@sntech.de> wrote: > > Hi Christoph, > > Am Dienstag, 12. März 2019, 15:46:44 CET schrieb Christoph Müllner: >>> On 12.03.2019, at 14:17, Heiko Stuebner <heiko@sntech.de> wrote: >>> >>> Am Freitag, 8. März 2019, 14:10:45 CET schrieb Christoph Müllner: >>>>> On 08.03.2019, at 13:46, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> >>>>> On 7/03/19 10:43 AM, Christoph Muellner wrote: >>>>>> This patch documents the new property disable-cqe-dcmd >>>>>> for the Arasan eMMC 5.1 driver. >>>>>> >>>>>> Signed-off-by: Christoph Muellner >>>>>> <christoph.muellner@theobroma-systems.com> Signed-off-by: Philipp >>>>>> Tomsich <philipp.tomsich@theobroma-systems.com> --- >>>>>> Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>>>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index >>>>>> 1edbb049cccb..ec699bf98b7c 100644 >>>>>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>>>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt >>>>>> >>>>>> @@ -44,6 +44,10 @@ Optional Properties: >>>>>> properly. Test mode can be used to force the controller to function. >>>>>> >>>>>> - xlnx,int-clock-stable-broken: when present, the controller always >>>>>> reports >>>>>> >>>>>> that the internal clock is stable even when it is not. >>>>>> >>>>>> + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands >>>>>> (DCMDs) + as part of the command queue engine (CQE). On controllers >>>>>> with a CQHCI, + such as the Arasan eMMC 5.1 host controller, the >>>>>> driver has to enable DCMDs. + This is done unless disable-cqe-dcmd >>>>>> is specified. >>> >>> This needs a rewording please. See below for hw-description vs. driver, so >>> the description should be centered around why this is a property of the hw >>> [like faulty controller implementation or whatever] >> >> I understand, that you prefer a HW-specific name for a property >> over one, that explains the actual effect. > > Actually I think the property-name is just fine :-) . > > The description might profit from a rewording though. Aka not driver- > centric but hw-centric, describing why some controllers may need the > option to disable these dcmds (controller bugs or whatever). Ah, ok. Now I understand what you mean. Will do for v3. Thanks, Christoph > > > Heiko > > > >> However, using disable-<feature> (which is always a directive for the driver >> and not a HW description) is not uncommon: >> >> * disable-wp should be "no-wp-line-connection" >> * disable-over-current -> "no-over-current-line-connection" >> * srp-disable -> "not-existing-srp-implementation" >> * ... >> >> But I don't mind using something else. >> Would "broken-cqe-dcmd" (like broken-cd or broken-flash-reset) be ok? >> Or other suggestions? >> >> Also I'd like to mention, that my first implementation was >> "supports-cqe-dcmd". I guess that would be a good choice, if it would have >> been there >> from the beginning. Introducing it now, would silently disable DCMD >> for existing DTBs. Therefore I went on to "disable-cqe-dcmd". >> >>>>> If "supports-cqe" is in mmc.txt, should "disable-cqe-dcmd" be there >>>>> also? >>>> >>>> The file mmc.txt says on top: >>>> "These properties are common to multiple MMC host controllers". >>>> As my patchset introduces "disable-cqe-dcmd" just for sdhci-of-arasan, >>>> I would say it should not go into that file. >>>> >>>> Also I wonder why "supports-cqe" is in mmc.txt, because >>>> only sdhci-tegra.c is evaluating that property. >>>> So I would expect it to be documented in >>>> Documentation/devicetree/bindings/mmc/nvidia,tegra20-sdhci.txt >>>> >>>> However, I see that "disable-cqe-dcmd" could go into other drivers as >>>> well. >>>> But is this enough to document it in mmc.txt? >>> >>> Devicetree is always about describing the hardware capabilites and never >>> about the actual nitty-gritty of driver implementation, aka it is not >>> meant >>> as a space for hardware-independent config-settings or such. >>> >>> As for only tegra evaluating this, is probably because it is still so new, >>> like january 2019 and Rob explicitly suggested it becoming common [0], >>> which suggests that the disable-cqe-dcmds should probably also be common. >> So mmc.txt lists "standardised" names for properties to reduce the risk >> of having similar, but distinct names for different MMC drivers. >> So whenever you want to introduce a new property for a driver, >> check if there isn't already something defined in mmc.txt. >> >> When seeing it this way, it clearly makes sense to have the property there. >> >> Thanks, >> Christoph
On Thu, Mar 07, 2019 at 09:43:27AM +0100, Christoph Muellner wrote: > This patch documents the new property disable-cqe-dcmd > for the Arasan eMMC 5.1 driver. > > Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com> > Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com> > --- > Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > index 1edbb049cccb..ec699bf98b7c 100644 > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt > @@ -44,6 +44,10 @@ Optional Properties: > properly. Test mode can be used to force the controller to function. > - xlnx,int-clock-stable-broken: when present, the controller always reports > that the internal clock is stable even when it is not. > + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) > + as part of the command queue engine (CQE). On controllers with a CQHCI, > + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. > + This is done unless disable-cqe-dcmd is specified. Should be common either for all controllers or all SDHCI controllers?
diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt index 1edbb049cccb..ec699bf98b7c 100644 --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt @@ -44,6 +44,10 @@ Optional Properties: properly. Test mode can be used to force the controller to function. - xlnx,int-clock-stable-broken: when present, the controller always reports that the internal clock is stable even when it is not. + - disable-cqe-dcmd: The eMMC 5.1 standard specifies direct commands (DCMDs) + as part of the command queue engine (CQE). On controllers with a CQHCI, + such as the Arasan eMMC 5.1 host controller, the driver has to enable DCMDs. + This is done unless disable-cqe-dcmd is specified. Example: sdhci@e0100000 {