| Message ID | 20191120154148.22067-2-orson.zhai@unisoc.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add syscon name support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Wed, Nov 20, 2019 at 4:44 PM Orson Zhai <orson.zhai@unisoc.com> wrote: > > + > +Optional property: > +- #.*-cells: Represents the number of arguments in single phandle in syscons > + list. ".*" is vendor specific. If this property is not set, default value > + will be 0. > + > +Examples: > + > +apb_regs: syscon@20008000 { > + compatible = "sprd,apb-glb", "syscon"; > + reg = <0x20008000 0x100>; > +}; > + > +aon_regs: syscon@40008000 { > + compatible = "sprd,aon-glb", "syscon"; > + reg = <0x40008000 0x100>; > +}; > + > +display@40500000 { > + ... > + #syscon-disp-cells = <2>; > + syscons = <&ap_apb_regs 0x4 0xf00>, <&aon_regs 0x8 0x7>; > + syscon-names = "enable", "power"; > +}; Hi Orson, With the changes from the syscon nodes removed, it looks better already, but I'd also like to not see the #.*-cells mentioned at all, as there is not really a way to parse this automatically, or make sense of the data without already knowing how many cells there are. Arnd
On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote: > Make life easier when syscon consumer want to access multiple syscon > nodes with dozens of items. > Add syscon-names and relative properties to help to manage different > cases when accessing more than one syscon node even with arguments. > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com> > --- > .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt > index 25d9e9c2fd53..4c7bdb74bb0a 100644 > --- a/Documentation/devicetree/bindings/mfd/syscon.txt > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 { > reg = <0x40500000 0x1000>; > #hwlock-cells = <1>; > }; > + > + > + > +==Syscon Name== > + > +Syscon name is a helper to be used in consumer nodes who refer to system > +controller node. It provides a way to refer to syscon node by names with > +phandle args in syscon consumer node. It will help people who have a lot > +of syscon references to be managed. It is not a must feature and has no > +effect to syscon node itself at all. > + > +Required properties: > +- syscons: List of phandles and any number of arguments if needed. Arguments > + is specific to differnet vendors and its usage should be described at each > + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated > + as register address offset and the 2nd is bit mask. > + > +- syscon-names: List of syscon node name strings sorted in the same order as > + what it represents in property syscons. > + > +Optional property: > +- #.*-cells: Represents the number of arguments in single phandle in syscons > + list. ".*" is vendor specific. If this property is not set, default value > + will be 0. This breaks the normal pattern of how '#.*-cells' works. While Arnd suggests removing it, I don't think that works well either with having a generic 'syscons' property. That means every syscon in a system has to have the same number of cells. I don't really want to see syscon binding expanded. Really, I'd like 'syscon' to go away. It's nothing more than a flag to create a regmap. I think it is better to keep the property names specific to exactly what the functionality is for each syscon phandle rather than a generic binding. What are the eamples of where you want to use this? Keep in mind that this sort of connection should *only* be used for things that have no other binding and kernel subsystem. Rob
On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote: > > Make life easier when syscon consumer want to access multiple syscon > > nodes with dozens of items. > > Add syscon-names and relative properties to help to manage different > > cases when accessing more than one syscon node even with arguments. > > > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com> > > --- > > .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt > > index 25d9e9c2fd53..4c7bdb74bb0a 100644 > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt > > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 { > > reg = <0x40500000 0x1000>; > > #hwlock-cells = <1>; > > }; > > + > > + > > + > > +==Syscon Name== > > + > > +Syscon name is a helper to be used in consumer nodes who refer to system > > +controller node. It provides a way to refer to syscon node by names with > > +phandle args in syscon consumer node. It will help people who have a lot > > +of syscon references to be managed. It is not a must feature and has no > > +effect to syscon node itself at all. > > + > > +Required properties: > > +- syscons: List of phandles and any number of arguments if needed. Arguments > > + is specific to differnet vendors and its usage should be described at each > > + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated > > + as register address offset and the 2nd is bit mask. > > + > > +- syscon-names: List of syscon node name strings sorted in the same order as > > + what it represents in property syscons. > > + > > +Optional property: > > +- #.*-cells: Represents the number of arguments in single phandle in syscons > > + list. ".*" is vendor specific. If this property is not set, default value > > + will be 0. > > This breaks the normal pattern of how '#.*-cells' works. While Arnd > suggests removing it, I don't think that works well either with having a > generic 'syscons' property. That means every syscon in a system has to > have the same number of cells. > > I don't really want to see syscon binding expanded. Really, I'd like > 'syscon' to go away. It's nothing more than a flag to create a regmap. Not expanding the syscon binding is the point about not having a #*-cells: In the examples that Orson listed, the cell count would always be specific to the user of the syscon regmap, and not interpreted by the syscon itself. > I think it is better to keep the property names specific to exactly what > the functionality is for each syscon phandle rather than a generic > binding. > > What are the eamples of where you want to use this? I think generally speaking this would be useful for random registers that logically belong to one device but are grouped with other unrelated registers in a syscon, and that are in a different register offset for each chip that has them. Using named properties instead of a list of phandle/arg tuples with names is clearly a simpler alternative and more like we do it today, but I can also see some API simplification with Orson's patch without the binding getting out of hand. > Keep in mind that > this sort of connection should *only* be used for things that have no > other binding and kernel subsystem. +1 Arnd
On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote: > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Nov 20, 2019 at 11:41:47PM +0800, Orson Zhai wrote: > > > Make life easier when syscon consumer want to access multiple syscon > > > nodes with dozens of items. > > > Add syscon-names and relative properties to help to manage different > > > cases when accessing more than one syscon node even with arguments. > > > > > > Signed-off-by: Orson Zhai <orson.zhai@unisoc.com> > > > --- > > > .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt > > > index 25d9e9c2fd53..4c7bdb74bb0a 100644 > > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt > > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt > > > @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 { > > > reg = <0x40500000 0x1000>; > > > #hwlock-cells = <1>; > > > }; > > > + > > > + > > > + > > > +==Syscon Name== > > > + > > > +Syscon name is a helper to be used in consumer nodes who refer to system > > > +controller node. It provides a way to refer to syscon node by names with > > > +phandle args in syscon consumer node. It will help people who have a lot > > > +of syscon references to be managed. It is not a must feature and has no > > > +effect to syscon node itself at all. > > > + > > > +Required properties: > > > +- syscons: List of phandles and any number of arguments if needed. Arguments > > > + is specific to differnet vendors and its usage should be described at each > > > + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated > > > + as register address offset and the 2nd is bit mask. > > > + > > > +- syscon-names: List of syscon node name strings sorted in the same order as > > > + what it represents in property syscons. > > > + > > > +Optional property: > > > +- #.*-cells: Represents the number of arguments in single phandle in syscons > > > + list. ".*" is vendor specific. If this property is not set, default value > > > + will be 0. > > > > This breaks the normal pattern of how '#.*-cells' works. While Arnd > > suggests removing it, I don't think that works well either with having a > > generic 'syscons' property. That means every syscon in a system has to > > have the same number of cells. > > > > I don't really want to see syscon binding expanded. Really, I'd like > > 'syscon' to go away. It's nothing more than a flag to create a regmap. > > Not expanding the syscon binding is the point about not having a #*-cells: > In the examples that Orson listed, the cell count would always be > specific to the user of the syscon regmap, and not interpreted by the > syscon itself. > > > I think it is better to keep the property names specific to exactly what > > the functionality is for each syscon phandle rather than a generic > > binding. > > > > What are the eamples of where you want to use this? > > I think generally speaking this would be useful for random registers that > logically belong to one device but are grouped with other unrelated > registers in a syscon, and that are in a different register offset for > each chip that has them. Using named properties instead of a list > of phandle/arg tuples with names is clearly a simpler alternative > and more like we do it today, but I can also see some API simplification > with Orson's patch without the binding getting out of hand. I understand when a phandle to a syscon is used. That's nothing new. What's special about Unisoc SoC that needs something new/different? I saw there's a large number of syscons, but I don't understand what's in them. If the API is this: struct regmap *syscon_regmap_lookup_by_name(struct device_node *np, const char *name, int arg_count, __u32 *out_args) How is 'name' being an entry in syscon-names simpler than just being the property name? The implementation for the latter would certainly be simpler. It also makes the property unparseable without knowledge outside of the DT (i.e. in the driver). I suppose if the number of cells for each entry is fixed, we could count the number of syscon-names entries to figure out the stride. But then if one entry needs a lot of cells, then they all have to have padding cells. Rob
On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <robh@kernel.org> wrote: > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote: > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > > I think generally speaking this would be useful for random registers that > > logically belong to one device but are grouped with other unrelated > > registers in a syscon, and that are in a different register offset for > > each chip that has them. Using named properties instead of a list > > of phandle/arg tuples with names is clearly a simpler alternative > > and more like we do it today, but I can also see some API simplification > > with Orson's patch without the binding getting out of hand. > > I understand when a phandle to a syscon is used. That's nothing new. > What's special about Unisoc SoC that needs something new/different? > I saw there's a large number of syscons, but I don't understand what's > in them. > > If the API is this: > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np, > const char *name, > int arg_count, __u32 *out_args) > > How is 'name' being an entry in syscon-names simpler than just being the > property name? The implementation for the latter would certainly be > simpler. > > It also makes the property unparseable without knowledge outside of the > DT (i.e. in the driver). I suppose if the number of cells for each entry > is fixed, we could count the number of syscon-names entries to figure > out the stride. But then if one entry needs a lot of cells, then they > all have to have padding cells. Good point. The syscon_regmap_lookup_by_name() interface would work just as well when passing a property name compared to a name listed in another property, and this would still be more in line with what we do on other SoCs. The only advantage I can see in having a list of phandle/arg tuples rather than a set of properties is that it is a slightly more compact representation in source form, but otherwise they should be equivalent and agree about this being harder to parse in an automated way. Orson, do you see any other reason for the combined property? If not, could you respin the series once more with syscon_regmap_lookup_by_name() replaced by something like:? struct regmap * syscon_regmap_lookup_args_by_phandle(struct device_node *np, const char *property, int arg_count, __u32 *out_args) Arnd
Hi Arnd & Rob, On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote: > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote: > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > > > > I think generally speaking this would be useful for random registers that > > > logically belong to one device but are grouped with other unrelated > > > registers in a syscon, and that are in a different register offset for > > > each chip that has them. Using named properties instead of a list > > > of phandle/arg tuples with names is clearly a simpler alternative > > > and more like we do it today, but I can also see some API simplification > > > with Orson's patch without the binding getting out of hand. > > > > I understand when a phandle to a syscon is used. That's nothing new. > > What's special about Unisoc SoC that needs something new/different? > > I saw there's a large number of syscons, but I don't understand what's > > in them. > > > > If the API is this: > > > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np, > > const char *name, > > int arg_count, __u32 *out_args) > > > > How is 'name' being an entry in syscon-names simpler than just being the > > property name? The implementation for the latter would certainly be > > simpler. > > > > It also makes the property unparseable without knowledge outside of the > > DT (i.e. in the driver). I suppose if the number of cells for each entry > > is fixed, we could count the number of syscon-names entries to figure > > out the stride. But then if one entry needs a lot of cells, then they > > all have to have padding cells. > > Good point. The syscon_regmap_lookup_by_name() interface would > work just as well when passing a property name compared to > a name listed in another property, and this would still be more in > line with what we do on other SoCs. > udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>, udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>, udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>, udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>, udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>; udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep", "corereset", udx710-modem.dtsi-75- "sysreset", "getstatus"; ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; ud710.dtsi:1272: syscon-names = "sd_detect_pol", ud710.dtsi-1273- "sd_hotplug_protect_en", ud710.dtsi-1274- "sd_hotplug_debounce_en", ud710.dtsi-1275- "sd_hotplug_debounce_cn"; > The only advantage I can see in having a list of phandle/arg tuples > rather than a set of properties is that it is a slightly more compact > representation in source form, but otherwise they should be equivalent Yes, I agree. They are equivalent. But sprd SoCs have too many registers and the representation might matter. Here's some real code from local, orca.dtsi:1276: syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >, orca.dtsi-1277- <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>, orca.dtsi-1278- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>, orca.dtsi-1279- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>, orca.dtsi-1280- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>, orca.dtsi-1281- <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>, orca.dtsi-1282- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>, orca.dtsi-1283- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>, orca.dtsi-1284- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>, -- orca.dtsi:1288: syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset", orca.dtsi-1289- "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus", orca.dtsi-1290- "bootprotect", "bootvector", "bootaddress_sel"; ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; ud710.dtsi:1272: syscon-names = "sd_detect_pol", ud710.dtsi-1273- "sd_hotplug_protect_en", ud710.dtsi-1274- "sd_hotplug_debounce_en", ud710.dtsi-1275- "sd_hotplug_debounce_cn"; Compare to following, sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>; sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>; sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; ..... For me, my choice would be the former. It looks more clear. > and agree about this being harder to parse in an automated way. > > Orson, do you see any other reason for the combined property? No other reason. > If not, could you respin the series once more with > syscon_regmap_lookup_by_name() replaced by something like:? > > struct regmap * > syscon_regmap_lookup_args_by_phandle(struct device_node *np, > const char *property, > int arg_count, __u32 *out_args) I like this idea. syscon_regmap_lookup_by_phandle_args() would be better? May I impelement them both? But I will be OK if no one here except me likes to expand name list. Best, Orson > > Arnd
On Thu, Dec 05, 2019 at 08:55:39PM +0800, Orson Zhai wrote: > Hi Arnd & Rob, > > On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote: > > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <robh@kernel.org> wrote: > > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote: > > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > > > > > > I think generally speaking this would be useful for random registers that > > > > logically belong to one device but are grouped with other unrelated > > > > registers in a syscon, and that are in a different register offset for > > > > each chip that has them. Using named properties instead of a list > > > > of phandle/arg tuples with names is clearly a simpler alternative > > > > and more like we do it today, but I can also see some API simplification > > > > with Orson's patch without the binding getting out of hand. > > > > > > I understand when a phandle to a syscon is used. That's nothing new. > > > What's special about Unisoc SoC that needs something new/different? > > > I saw there's a large number of syscons, but I don't understand what's > > > in them. > > > > > > If the API is this: > > > > > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np, > > > const char *name, > > > int arg_count, __u32 *out_args) > > > > > > How is 'name' being an entry in syscon-names simpler than just being the > > > property name? The implementation for the latter would certainly be > > > simpler. > > > > > > It also makes the property unparseable without knowledge outside of the > > > DT (i.e. in the driver). I suppose if the number of cells for each entry > > > is fixed, we could count the number of syscon-names entries to figure > > > out the stride. But then if one entry needs a lot of cells, then they > > > all have to have padding cells. > > > > Good point. The syscon_regmap_lookup_by_name() interface would > > work just as well when passing a property name compared to > > a name listed in another property, and this would still be more in > > line with what we do on other SoCs. > > > > udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>, > udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>, > udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>, > udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>, > udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>; > udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep", "corereset", > udx710-modem.dtsi-75- "sysreset", "getstatus"; Reset at least has a standard binding. > ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, > ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, > ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, > ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > ud710.dtsi:1272: syscon-names = "sd_detect_pol", > ud710.dtsi-1273- "sd_hotplug_protect_en", > ud710.dtsi-1274- "sd_hotplug_debounce_en", > ud710.dtsi-1275- "sd_hotplug_debounce_cn"; This looks to me like it should be a single phandle. How many different register layouts across how many SoCs do you need to support? > > The only advantage I can see in having a list of phandle/arg tuples > > rather than a set of properties is that it is a slightly more compact > > representation in source form, but otherwise they should be equivalent > > Yes, I agree. > They are equivalent. > > But sprd SoCs have too many registers and the representation might matter. > Here's some real code from local, > > orca.dtsi:1276: syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >, > orca.dtsi-1277- <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>, > orca.dtsi-1278- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>, > orca.dtsi-1279- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>, > orca.dtsi-1280- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>, > orca.dtsi-1281- <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>, > orca.dtsi-1282- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>, > orca.dtsi-1283- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>, > orca.dtsi-1284- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>, > -- > orca.dtsi:1288: syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset", > orca.dtsi-1289- "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus", > orca.dtsi-1290- "bootprotect", "bootvector", "bootaddress_sel"; Again, reset has standard binding. Also consider if you really need to access all of these vs. assuming a fixed mode that the firmware/bootloader sets up. > ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, > ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, > ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, > ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > ud710.dtsi:1272: syscon-names = "sd_detect_pol", > ud710.dtsi-1273- "sd_hotplug_protect_en", > ud710.dtsi-1274- "sd_hotplug_debounce_en", > ud710.dtsi-1275- "sd_hotplug_debounce_cn"; > > Compare to following, > > sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>; > sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>; > sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > ..... > > For me, my choice would be the former. > It looks more clear. > > > and agree about this being harder to parse in an automated way. > > > > Orson, do you see any other reason for the combined property? > No other reason. > > > If not, could you respin the series once more with > > syscon_regmap_lookup_by_name() replaced by something like:? > > > > struct regmap * > > syscon_regmap_lookup_args_by_phandle(struct device_node *np, > > const char *property, > > int arg_count, __u32 *out_args) > > I like this idea. syscon_regmap_lookup_by_phandle_args() would be better? > > May I impelement them both? No. Rob
Sorry! Re-send as plain-text mode. On Thu, Dec 5, 2019 at 11:13 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Dec 05, 2019 at 08:55:39PM +0800, Orson Zhai wrote: > > Hi Arnd & Rob, > > > > On Thu, Dec 05, 2019 at 10:20:03AM +0100, Arnd Bergmann wrote: > > > On Wed, Dec 4, 2019 at 8:26 PM Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Dec 04, 2019 at 06:00:17PM +0100, Arnd Bergmann wrote: > > > > > On Wed, Dec 4, 2019 at 5:38 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > I think generally speaking this would be useful for random registers that > > > > > logically belong to one device but are grouped with other unrelated > > > > > registers in a syscon, and that are in a different register offset for > > > > > each chip that has them. Using named properties instead of a list > > > > > of phandle/arg tuples with names is clearly a simpler alternative > > > > > and more like we do it today, but I can also see some API simplification > > > > > with Orson's patch without the binding getting out of hand. > > > > > > > > I understand when a phandle to a syscon is used. That's nothing new. > > > > What's special about Unisoc SoC that needs something new/different? > > > > I saw there's a large number of syscons, but I don't understand what's > > > > in them. > > > > > > > > If the API is this: > > > > > > > > struct regmap *syscon_regmap_lookup_by_name(struct device_node *np, > > > > const char *name, > > > > int arg_count, __u32 *out_args) > > > > > > > > How is 'name' being an entry in syscon-names simpler than just being the > > > > property name? The implementation for the latter would certainly be > > > > simpler. > > > > > > > > It also makes the property unparseable without knowledge outside of the > > > > DT (i.e. in the driver). I suppose if the number of cells for each entry > > > > is fixed, we could count the number of syscon-names entries to figure > > > > out the stride. But then if one entry needs a lot of cells, then they > > > > all have to have padding cells. > > > > > > Good point. The syscon_regmap_lookup_by_name() interface would > > > work just as well when passing a property name compared to > > > a name listed in another property, and this would still be more in > > > line with what we do on other SoCs. > > > > > > > udx710-modem.dtsi:69: syscons = <&pmu_apb_regs 0x18 0x2000000>, > > udx710-modem.dtsi-70- <&pmu_apb_regs 0x544 0x1>, > > udx710-modem.dtsi-71- <&aon_apb_regs 0x218 0x7e00>, > > udx710-modem.dtsi-72- <&pmu_apb_regs 0xb0 0x20000>, > > udx710-modem.dtsi-73- <&pmu_apb_regs 0xff 0x100>; > > udx710-modem.dtsi:74: syscon-names = "shutdown", "deepsleep", "corereset", > > udx710-modem.dtsi-75- "sysreset", "getstatus"; > > Reset at least has a standard binding. You mean the reset-controller binding? I have noticed that before. Okay, it's time for us to consider to setup it. > > > > ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, > > ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, > > ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, > > ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > > ud710.dtsi:1272: syscon-names = "sd_detect_pol", > > ud710.dtsi-1273- "sd_hotplug_protect_en", > > ud710.dtsi-1274- "sd_hotplug_debounce_en", > > ud710.dtsi-1275- "sd_hotplug_debounce_cn"; > > This looks to me like it should be a single phandle. How many different > register layouts across how many SoCs do you need to support? Not sure. It's up to the IC designer how to layout their next chip registers. For a simple example: Chip A: offset Chip B: offset -------------------------+---------------------------- glb-reg0 0x0 | glb-reg0 0x0 glb-reg1 0x4 | a-new-reg 0x4 (newly insert in the middle of list) glb-reg2 0x8 | glb-reg1 0x8 (same function with A's reg1) .... | glb-reg2 0xc (same function with A's reg2) So for chip B's driver which is same to chip A, but the offsets are partly different. Earlier we use chip macro to separate them. #ifdef CHIP_A #define REG1 0x4 #endif #ifdef CHIP_B #define REG1 0x8 #endif ...... That time we also used single phandle like sprd,xxxx-syscon = <&yyy>; But these macro will prevent us from building all-in-one kernel image for different SoCs and this also make dtb less powerful -- we can't use multiple dtbs for different SoCs with a single image -- although these SoCs are compatible in many drivers. > > > > The only advantage I can see in having a list of phandle/arg tuples > > > rather than a set of properties is that it is a slightly more compact > > > representation in source form, but otherwise they should be equivalent > > > > Yes, I agree. > > They are equivalent. > > > > But sprd SoCs have too many registers and the representation might matter. > > Here's some real code from local, > > > > orca.dtsi:1276: syscons = <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_SYS_CFG MASK_PMU_APB_RF_PD_AUDCP_SYS_FORCE_SHUTDOWN >, > > orca.dtsi-1277- <&pmu_apb_regs REG_PMU_APB_RF_PD_AUDCP_AUDDSP_CFG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_AUTO_SHUTDOWN_EN>, > > orca.dtsi-1278- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_CTRL MASK_PMU_APB_RF_AUDCP_FORCE_DEEP_SLEEP>, > > orca.dtsi-1279- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_AUDDSP_SOFT_RST>, > > orca.dtsi-1280- <&pmu_apb_regs REG_PMU_APB_RF_CP_SOFT_RST MASK_PMU_APB_RF_AUDCP_SYS_SOFT_RST>, > > orca.dtsi-1281- <&pmu_apb_regs REG_PMU_APB_RF_SOFT_RST_SEL MASK_PMU_APB_RF_SOFT_RST_SEL>, > > orca.dtsi-1282- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_SYS_STATE>, > > orca.dtsi-1283- <&pmu_apb_regs REG_PMU_APB_RF_PWR_STATUS3_DBG MASK_PMU_APB_RF_PD_AUDCP_AUDDSP_STATE>, > > orca.dtsi-1284- <&pmu_apb_regs REG_PMU_APB_RF_SLEEP_STATUS MASK_PMU_APB_RF_AUDCP_SLP_STATUS>, > > -- > > orca.dtsi:1288: syscon-names = "sysshutdown", "coreshutdown", "deepsleep", "corereset", > > orca.dtsi-1289- "sysreset", "reset_sel", "sysstatus", "corestatus", "sleepstatus", > > orca.dtsi-1290- "bootprotect", "bootvector", "bootaddress_sel"; > > Again, reset has standard binding. > > Also consider if you really need to access all of these vs. assuming a > fixed mode that the firmware/bootloader sets up. I believe most of them are used in kernel suspend/resume process. > > > ud710.dtsi:1268: syscons = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>, > > ud710.dtsi-1269- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>, > > ud710.dtsi-1270- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_EN_32K>, > > ud710.dtsi-1271- <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > > ud710.dtsi:1272: syscon-names = "sd_detect_pol", > > ud710.dtsi-1273- "sd_hotplug_protect_en", > > ud710.dtsi-1274- "sd_hotplug_debounce_en", > > ud710.dtsi-1275- "sd_hotplug_debounce_cn"; > > > > Compare to following, > > > > sd_hotplug_protect_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PRESENT_32K>; > > sd_hotplug_debounce_en-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARD_PROTECT_32K>; > > sd_hotplug_debounce_cn-syscon = <&aon_apb_regs REG_AON_APB_SDIO0_CTRL_REG MASK_AON_APB_SDIO0_EMMC_CARDDET_DBNC_THD_32K>; > > ..... > > > > For me, my choice would be the former. > > It looks more clear. > > > > > and agree about this being harder to parse in an automated way. > > > > > > Orson, do you see any other reason for the combined property? > > No other reason. > > > > > If not, could you respin the series once more with > > > syscon_regmap_lookup_by_name() replaced by something like:? > > > > > > struct regmap * > > > syscon_regmap_lookup_args_by_phandle(struct device_node *np, > > > const char *property, > > > int arg_count, __u32 *out_args) > > > > I like this idea. syscon_regmap_lookup_by_phandle_args() would be better? > > > > May I impelement them both? > > No. Okay. I will send patch V3. Thanks for your comments! BR, Orson > > Rob
diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt index 25d9e9c2fd53..4c7bdb74bb0a 100644 --- a/Documentation/devicetree/bindings/mfd/syscon.txt +++ b/Documentation/devicetree/bindings/mfd/syscon.txt @@ -30,3 +30,46 @@ hwlock1: hwspinlock@40500000 { reg = <0x40500000 0x1000>; #hwlock-cells = <1>; }; + + + +==Syscon Name== + +Syscon name is a helper to be used in consumer nodes who refer to system +controller node. It provides a way to refer to syscon node by names with +phandle args in syscon consumer node. It will help people who have a lot +of syscon references to be managed. It is not a must feature and has no +effect to syscon node itself at all. + +Required properties: +- syscons: List of phandles and any number of arguments if needed. Arguments + is specific to differnet vendors and its usage should be described at each + vendor's bindings. For example: In Unisoc SoCs, the 1st arg will be treated + as register address offset and the 2nd is bit mask. + +- syscon-names: List of syscon node name strings sorted in the same order as + what it represents in property syscons. + +Optional property: +- #.*-cells: Represents the number of arguments in single phandle in syscons + list. ".*" is vendor specific. If this property is not set, default value + will be 0. + +Examples: + +apb_regs: syscon@20008000 { + compatible = "sprd,apb-glb", "syscon"; + reg = <0x20008000 0x100>; +}; + +aon_regs: syscon@40008000 { + compatible = "sprd,aon-glb", "syscon"; + reg = <0x40008000 0x100>; +}; + +display@40500000 { + ... + #syscon-disp-cells = <2>; + syscons = <&ap_apb_regs 0x4 0xf00>, <&aon_regs 0x8 0x7>; + syscon-names = "enable", "power"; +};
Make life easier when syscon consumer want to access multiple syscon nodes with dozens of items. Add syscon-names and relative properties to help to manage different cases when accessing more than one syscon node even with arguments. Signed-off-by: Orson Zhai <orson.zhai@unisoc.com> --- .../devicetree/bindings/mfd/syscon.txt | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) -- 2.18.0