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@xxxxxxxxxx> 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@xxxxxxxxxx> 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
