| Message ID | 20230830031846.127957-2-william.qiu@starfivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Change tuning implementation | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > Due to the change of tuning implementation, it's no longer necessary to > use the "starfive,sysreg" property in dts, so drop the relevant > description in dt-bindings here. How does changing your software implantation invalidate a description of the hardware? > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > --- > .../bindings/mmc/starfive,jh7110-mmc.yaml | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml > index 51e1b04e799f..10df41941331 100644 > --- a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml > +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml > @@ -36,26 +36,12 @@ properties: > interrupts: > maxItems: 1 > > - starfive,sysreg: > - $ref: /schemas/types.yaml#/definitions/phandle-array > - items: > - - items: > - - description: phandle to System Register Controller syscon node > - - description: offset of SYS_SYSCONSAIF__SYSCFG register for MMC controller > - - description: shift of SYS_SYSCONSAIF__SYSCFG register for MMC controller > - - description: mask of SYS_SYSCONSAIF__SYSCFG register for MMC controller > - description: > - Should be four parameters, the phandle to System Register Controller > - syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register > - for MMC controller. > - > required: > - compatible > - reg > - clocks > - clock-names > - interrupts > - - starfive,sysreg > > unevaluatedProperties: false > > @@ -73,5 +59,4 @@ examples: > fifo-depth = <32>; > fifo-watermark-aligned; > data-addr = <0>; > - starfive,sysreg = <&sys_syscon 0x14 0x1a 0x7c000000>; > }; > -- > 2.34.1 >
On 30/08/2023 08:50, Conor Dooley wrote: > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >> Due to the change of tuning implementation, it's no longer necessary to >> use the "starfive,sysreg" property in dts, so drop the relevant >> description in dt-bindings here. > > How does changing your software implantation invalidate a description of > the hardware? > Which is kind of proof that this syscon was just to substitute incomplete hardware description (e.g. missing clocks and phys). We should have rejected it. Just like we should reject them in the future. There are just few cases where syscon is reasonable. All others is just laziness. It's not only starfivetech, of course. Several other contributors do the same. Best regards, Krzysztof
On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: > On 30/08/2023 08:50, Conor Dooley wrote: > > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > >> Due to the change of tuning implementation, it's no longer necessary to > >> use the "starfive,sysreg" property in dts, so drop the relevant > >> description in dt-bindings here. > > > > How does changing your software implantation invalidate a description of > > the hardware? > > > > Which is kind of proof that this syscon was just to substitute > incomplete hardware description (e.g. missing clocks and phys). We > should have rejected it. Just like we should reject them in the future. :s I dunno what to do with this... I'm inclined to say not to remove it from the binding or dts at all & only change the software. > There are just few cases where syscon is reasonable. All others is just > laziness. It's not only starfivetech, of course. Several other > contributors do the same. I'm not sure if laziness is fair, lack of understanding is usually more likely.
On 2023/8/30 16:34, Conor Dooley wrote: > On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: >> On 30/08/2023 08:50, Conor Dooley wrote: >> > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >> >> Due to the change of tuning implementation, it's no longer necessary to >> >> use the "starfive,sysreg" property in dts, so drop the relevant >> >> description in dt-bindings here. >> > >> > How does changing your software implantation invalidate a description of >> > the hardware? >> > >> >> Which is kind of proof that this syscon was just to substitute >> incomplete hardware description (e.g. missing clocks and phys). We >> should have rejected it. Just like we should reject them in the future. > > :s I dunno what to do with this... I'm inclined to say not to remove it > from the binding or dts at all & only change the software. > >> There are just few cases where syscon is reasonable. All others is just >> laziness. It's not only starfivetech, of course. Several other >> contributors do the same. > > I'm not sure if laziness is fair, lack of understanding is usually more > likely. For this, I tend to keep it in binding, but remove it from required. Because we only modify the tuning implementation, it doesn't mean that this property need to be removed, it's just no longer be the required one. Best Regards, William
On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: > > > On 2023/8/30 16:34, Conor Dooley wrote: > > On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: > >> On 30/08/2023 08:50, Conor Dooley wrote: > >> > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > >> >> Due to the change of tuning implementation, it's no longer necessary to > >> >> use the "starfive,sysreg" property in dts, so drop the relevant > >> >> description in dt-bindings here. > >> > > >> > How does changing your software implantation invalidate a description of > >> > the hardware? > >> > > >> > >> Which is kind of proof that this syscon was just to substitute > >> incomplete hardware description (e.g. missing clocks and phys). We > >> should have rejected it. Just like we should reject them in the future. > > > > :s I dunno what to do with this... I'm inclined to say not to remove it > > from the binding or dts at all & only change the software. > > > >> There are just few cases where syscon is reasonable. All others is just > >> laziness. It's not only starfivetech, of course. Several other > >> contributors do the same. > > > > I'm not sure if laziness is fair, lack of understanding is usually more > > likely. > > For this, I tend to keep it in binding, but remove it from required. Because > we only modify the tuning implementation, it doesn't mean that this property > need to be removed, it's just no longer be the required one. Please only remove it from required if the current driver doesn't break if the regmap is removed.
On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: > > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: >> >> >> On 2023/8/30 16:34, Conor Dooley wrote: >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: >>>> On 30/08/2023 08:50, Conor Dooley wrote: >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >>>>>> Due to the change of tuning implementation, it's no longer necessary to >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant >>>>>> description in dt-bindings here. >>>>> >>>>> How does changing your software implantation invalidate a description of >>>>> the hardware? >>>>> >>>> >>>> Which is kind of proof that this syscon was just to substitute >>>> incomplete hardware description (e.g. missing clocks and phys). We >>>> should have rejected it. Just like we should reject them in the future. >>> >>> :s I dunno what to do with this... I'm inclined to say not to remove it >>> from the binding or dts at all & only change the software. >>> >>>> There are just few cases where syscon is reasonable. All others is just >>>> laziness. It's not only starfivetech, of course. Several other >>>> contributors do the same. >>> >>> I'm not sure if laziness is fair, lack of understanding is usually more >>> likely. >> >> For this, I tend to keep it in binding, but remove it from required. Because >> we only modify the tuning implementation, it doesn't mean that this property >> need to be removed, it's just no longer be the required one. > > Please only remove it from required if the current driver doesn't break > if the regmap is removed. Either way please make sure the documentation clearly states “never use this, if you’re using it you’re doing it wrong, this only exists because it was wrongly used in the past”. Otherwise people writing drivers for other OSes will probably use it too thinking they need to. Jess
On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: > On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: > > > > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: > >> > >> > >> On 2023/8/30 16:34, Conor Dooley wrote: > >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: > >>>> On 30/08/2023 08:50, Conor Dooley wrote: > >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > >>>>>> Due to the change of tuning implementation, it's no longer necessary to > >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant > >>>>>> description in dt-bindings here. > >>>>> > >>>>> How does changing your software implantation invalidate a description of > >>>>> the hardware? > >>>>> > >>>> > >>>> Which is kind of proof that this syscon was just to substitute > >>>> incomplete hardware description (e.g. missing clocks and phys). We > >>>> should have rejected it. Just like we should reject them in the future. > >>> > >>> :s I dunno what to do with this... I'm inclined to say not to remove it > >>> from the binding or dts at all & only change the software. > >>> > >>>> There are just few cases where syscon is reasonable. All others is just > >>>> laziness. It's not only starfivetech, of course. Several other > >>>> contributors do the same. > >>> > >>> I'm not sure if laziness is fair, lack of understanding is usually more > >>> likely. > >> > >> For this, I tend to keep it in binding, but remove it from required. Because > >> we only modify the tuning implementation, it doesn't mean that this property > >> need to be removed, it's just no longer be the required one. > > > > Please only remove it from required if the current driver doesn't break > > if the regmap is removed. > > Either way please make sure the documentation clearly states “never use > this, if you’re using it you’re doing it wrong, this only exists > because it was wrongly used in the past”. Otherwise people writing > drivers for other OSes will probably use it too thinking they need to. Maybe we should just delete it if the impact is going to be negligible, sounds like you're not using it in FreeBSD, which was part of what I was worried about. Guess it depends on what Emil & the distro heads think.
On 1 Sep 2023, at 18:43, Conor Dooley <conor@kernel.org> wrote: > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: >>> >>> On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: >>>> >>>> >>>> On 2023/8/30 16:34, Conor Dooley wrote: >>>>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: >>>>>> On 30/08/2023 08:50, Conor Dooley wrote: >>>>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >>>>>>>> Due to the change of tuning implementation, it's no longer necessary to >>>>>>>> use the "starfive,sysreg" property in dts, so drop the relevant >>>>>>>> description in dt-bindings here. >>>>>>> >>>>>>> How does changing your software implantation invalidate a description of >>>>>>> the hardware? >>>>>>> >>>>>> >>>>>> Which is kind of proof that this syscon was just to substitute >>>>>> incomplete hardware description (e.g. missing clocks and phys). We >>>>>> should have rejected it. Just like we should reject them in the future. >>>>> >>>>> :s I dunno what to do with this... I'm inclined to say not to remove it >>>>> from the binding or dts at all & only change the software. >>>>> >>>>>> There are just few cases where syscon is reasonable. All others is just >>>>>> laziness. It's not only starfivetech, of course. Several other >>>>>> contributors do the same. >>>>> >>>>> I'm not sure if laziness is fair, lack of understanding is usually more >>>>> likely. >>>> >>>> For this, I tend to keep it in binding, but remove it from required. Because >>>> we only modify the tuning implementation, it doesn't mean that this property >>>> need to be removed, it's just no longer be the required one. >>> >>> Please only remove it from required if the current driver doesn't break >>> if the regmap is removed. >> >> Either way please make sure the documentation clearly states “never use >> this, if you’re using it you’re doing it wrong, this only exists >> because it was wrongly used in the past”. Otherwise people writing >> drivers for other OSes will probably use it too thinking they need to. > > Maybe we should just delete it if the impact is going to be negligible, > sounds like you're not using it in FreeBSD, which was part of what I was > worried about. Guess it depends on what Emil & the distro heads think. FreeBSD doesn’t have StarFive drivers yet; I don’t have time to write them, and a community member has taken it upon themselves as a hobby but is rather inexperienced and has been struggling for months. OpenBSD has drivers, including a modified dwmmc, but doesn’t use this property (in fact its driver doesn’t use the compatible other than to probe the generic driver). I don’t think anyone else has a serious port; Haiku’s the closest but also has no StarFive support. Jess
On 2023/9/2 1:43, Conor Dooley wrote: > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: >> > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: >> >> >> >> >> >> On 2023/8/30 16:34, Conor Dooley wrote: >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: >> >>>> On 30/08/2023 08:50, Conor Dooley wrote: >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant >> >>>>>> description in dt-bindings here. >> >>>>> >> >>>>> How does changing your software implantation invalidate a description of >> >>>>> the hardware? >> >>>>> >> >>>> >> >>>> Which is kind of proof that this syscon was just to substitute >> >>>> incomplete hardware description (e.g. missing clocks and phys). We >> >>>> should have rejected it. Just like we should reject them in the future. >> >>> >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it >> >>> from the binding or dts at all & only change the software. >> >>> >> >>>> There are just few cases where syscon is reasonable. All others is just >> >>>> laziness. It's not only starfivetech, of course. Several other >> >>>> contributors do the same. >> >>> >> >>> I'm not sure if laziness is fair, lack of understanding is usually more >> >>> likely. >> >> >> >> For this, I tend to keep it in binding, but remove it from required. Because >> >> we only modify the tuning implementation, it doesn't mean that this property >> >> need to be removed, it's just no longer be the required one. >> > >> > Please only remove it from required if the current driver doesn't break >> > if the regmap is removed. >> >> Either way please make sure the documentation clearly states “never use >> this, if you’re using it you’re doing it wrong, this only exists >> because it was wrongly used in the past”. Otherwise people writing >> drivers for other OSes will probably use it too thinking they need to. > > Maybe we should just delete it if the impact is going to be negligible, > sounds like you're not using it in FreeBSD, which was part of what I was > worried about. Guess it depends on what Emil & the distro heads think. Hi Conor, After discussing it with our colleagues, we decided that deleting it was the best course of action. Since there will no longer be a related implementation of "starfive,sysreg" in future drivers, even if the dt-binding is described, it will be "never use", so I think it should be deleted. What do you think? Best regards, William
On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote: > On 2023/9/2 1:43, Conor Dooley wrote: > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: > >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: > >> > > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: > >> >> > >> >> > >> >> On 2023/8/30 16:34, Conor Dooley wrote: > >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: > >> >>>> On 30/08/2023 08:50, Conor Dooley wrote: > >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to > >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant > >> >>>>>> description in dt-bindings here. > >> >>>>> > >> >>>>> How does changing your software implantation invalidate a description of > >> >>>>> the hardware? > >> >>>>> > >> >>>> > >> >>>> Which is kind of proof that this syscon was just to substitute > >> >>>> incomplete hardware description (e.g. missing clocks and phys). We > >> >>>> should have rejected it. Just like we should reject them in the future. > >> >>> > >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it > >> >>> from the binding or dts at all & only change the software. > >> >>> > >> >>>> There are just few cases where syscon is reasonable. All others is just > >> >>>> laziness. It's not only starfivetech, of course. Several other > >> >>>> contributors do the same. > >> >>> > >> >>> I'm not sure if laziness is fair, lack of understanding is usually more > >> >>> likely. > >> >> > >> >> For this, I tend to keep it in binding, but remove it from required. Because > >> >> we only modify the tuning implementation, it doesn't mean that this property > >> >> need to be removed, it's just no longer be the required one. > >> > > >> > Please only remove it from required if the current driver doesn't break > >> > if the regmap is removed. > >> > >> Either way please make sure the documentation clearly states “never use > >> this, if you’re using it you’re doing it wrong, this only exists > >> because it was wrongly used in the past”. Otherwise people writing > >> drivers for other OSes will probably use it too thinking they need to. > > > > Maybe we should just delete it if the impact is going to be negligible, > > sounds like you're not using it in FreeBSD, which was part of what I was > > worried about. Guess it depends on what Emil & the distro heads think. > Hi Conor, > > After discussing it with our colleagues, we decided that deleting it was the best > course of action. Since there will no longer be a related implementation of > "starfive,sysreg" in future drivers, even if the dt-binding is described, it will > be "never use", so I think it should be deleted. > > What do you think? The device tree should be a description of the hardware and there really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg registers[1] on the JH7110 that seems to do _something_ related to the sdio interface. So I don't think the fact that the Linux driver no longer uses it is a good reason to remove it, but if there are some other pragmatic reasons to do so then I'm fine with it. Removing it from the list of required properties should be fine though. /Emil [1]: https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/sys_syscon.html
On Fri, Sep 08, 2023 at 03:32:36PM +0200, Emil Renner Berthing wrote: > On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote: > > On 2023/9/2 1:43, Conor Dooley wrote: > > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: > > >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: > > >> > > > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: > > >> >> > > >> >> > > >> >> On 2023/8/30 16:34, Conor Dooley wrote: > > >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: > > >> >>>> On 30/08/2023 08:50, Conor Dooley wrote: > > >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: > > >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to > > >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant > > >> >>>>>> description in dt-bindings here. > > >> >>>>> > > >> >>>>> How does changing your software implantation invalidate a description of > > >> >>>>> the hardware? > > >> >>>>> > > >> >>>> > > >> >>>> Which is kind of proof that this syscon was just to substitute > > >> >>>> incomplete hardware description (e.g. missing clocks and phys). We > > >> >>>> should have rejected it. Just like we should reject them in the future. > > >> >>> > > >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it > > >> >>> from the binding or dts at all & only change the software. > > >> >>> > > >> >>>> There are just few cases where syscon is reasonable. All others is just > > >> >>>> laziness. It's not only starfivetech, of course. Several other > > >> >>>> contributors do the same. > > >> >>> > > >> >>> I'm not sure if laziness is fair, lack of understanding is usually more > > >> >>> likely. > > >> >> > > >> >> For this, I tend to keep it in binding, but remove it from required. Because > > >> >> we only modify the tuning implementation, it doesn't mean that this property > > >> >> need to be removed, it's just no longer be the required one. > > >> > > > >> > Please only remove it from required if the current driver doesn't break > > >> > if the regmap is removed. > > >> > > >> Either way please make sure the documentation clearly states “never use > > >> this, if you’re using it you’re doing it wrong, this only exists > > >> because it was wrongly used in the past”. Otherwise people writing > > >> drivers for other OSes will probably use it too thinking they need to. > > > > > > Maybe we should just delete it if the impact is going to be negligible, > > > sounds like you're not using it in FreeBSD, which was part of what I was > > > worried about. Guess it depends on what Emil & the distro heads think. > > Hi Conor, > > > > After discussing it with our colleagues, we decided that deleting it was the best > > course of action. Since there will no longer be a related implementation of > > "starfive,sysreg" in future drivers, even if the dt-binding is described, it will > > be "never use", so I think it should be deleted. > > > > What do you think? > > The device tree should be a description of the hardware and there > really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg > registers[1] on the JH7110 that seems to do _something_ related to the > sdio interface. So I don't think the fact that the Linux driver no > longer uses it is a good reason to remove it, but if there are some > other pragmatic reasons to do so then I'm fine with it. Removing it > from the list of required properties should be fine though. SGTM. Can you update the patch to do that please William? Thanks, Conor.
On 2023/9/12 0:14, Conor Dooley wrote: > On Fri, Sep 08, 2023 at 03:32:36PM +0200, Emil Renner Berthing wrote: >> On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote: >> > On 2023/9/2 1:43, Conor Dooley wrote: >> > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote: >> > >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote: >> > >> > >> > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote: >> > >> >> >> > >> >> >> > >> >> On 2023/8/30 16:34, Conor Dooley wrote: >> > >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote: >> > >> >>>> On 30/08/2023 08:50, Conor Dooley wrote: >> > >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote: >> > >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to >> > >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant >> > >> >>>>>> description in dt-bindings here. >> > >> >>>>> >> > >> >>>>> How does changing your software implantation invalidate a description of >> > >> >>>>> the hardware? >> > >> >>>>> >> > >> >>>> >> > >> >>>> Which is kind of proof that this syscon was just to substitute >> > >> >>>> incomplete hardware description (e.g. missing clocks and phys). We >> > >> >>>> should have rejected it. Just like we should reject them in the future. >> > >> >>> >> > >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it >> > >> >>> from the binding or dts at all & only change the software. >> > >> >>> >> > >> >>>> There are just few cases where syscon is reasonable. All others is just >> > >> >>>> laziness. It's not only starfivetech, of course. Several other >> > >> >>>> contributors do the same. >> > >> >>> >> > >> >>> I'm not sure if laziness is fair, lack of understanding is usually more >> > >> >>> likely. >> > >> >> >> > >> >> For this, I tend to keep it in binding, but remove it from required. Because >> > >> >> we only modify the tuning implementation, it doesn't mean that this property >> > >> >> need to be removed, it's just no longer be the required one. >> > >> > >> > >> > Please only remove it from required if the current driver doesn't break >> > >> > if the regmap is removed. >> > >> >> > >> Either way please make sure the documentation clearly states “never use >> > >> this, if you’re using it you’re doing it wrong, this only exists >> > >> because it was wrongly used in the past”. Otherwise people writing >> > >> drivers for other OSes will probably use it too thinking they need to. >> > > >> > > Maybe we should just delete it if the impact is going to be negligible, >> > > sounds like you're not using it in FreeBSD, which was part of what I was >> > > worried about. Guess it depends on what Emil & the distro heads think. >> > Hi Conor, >> > >> > After discussing it with our colleagues, we decided that deleting it was the best >> > course of action. Since there will no longer be a related implementation of >> > "starfive,sysreg" in future drivers, even if the dt-binding is described, it will >> > be "never use", so I think it should be deleted. >> > >> > What do you think? >> >> The device tree should be a description of the hardware and there >> really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg >> registers[1] on the JH7110 that seems to do _something_ related to the >> sdio interface. So I don't think the fact that the Linux driver no >> longer uses it is a good reason to remove it, but if there are some >> other pragmatic reasons to do so then I'm fine with it. Removing it >> from the list of required properties should be fine though. > > SGTM. Can you update the patch to do that please William? > > Thanks, > Conor. OK, I will update the patch as suggested by Emil. Best Regards, William
diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml index 51e1b04e799f..10df41941331 100644 --- a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml @@ -36,26 +36,12 @@ properties: interrupts: maxItems: 1 - starfive,sysreg: - $ref: /schemas/types.yaml#/definitions/phandle-array - items: - - items: - - description: phandle to System Register Controller syscon node - - description: offset of SYS_SYSCONSAIF__SYSCFG register for MMC controller - - description: shift of SYS_SYSCONSAIF__SYSCFG register for MMC controller - - description: mask of SYS_SYSCONSAIF__SYSCFG register for MMC controller - description: - Should be four parameters, the phandle to System Register Controller - syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register - for MMC controller. - required: - compatible - reg - clocks - clock-names - interrupts - - starfive,sysreg unevaluatedProperties: false @@ -73,5 +59,4 @@ examples: fifo-depth = <32>; fifo-watermark-aligned; data-addr = <0>; - starfive,sysreg = <&sys_syscon 0x14 0x1a 0x7c000000>; };
Due to the change of tuning implementation, it's no longer necessary to use the "starfive,sysreg" property in dts, so drop the relevant description in dt-bindings here. Signed-off-by: William Qiu <william.qiu@starfivetech.com> --- .../bindings/mmc/starfive,jh7110-mmc.yaml | 15 --------------- 1 file changed, 15 deletions(-)