Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to syscon easily

[Orson Zhai]

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";

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
________________________________
 This email (including its attachments) is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. Unauthorized use, dissemination, distribution or copying of this email or the information herein or taking any action in reliance on the contents of this email or the information herein, by anyone other than the intended recipient, or an employee or agent responsible for delivering the message to the intended recipient, is strictly prohibited. If you are not the intended recipient, please do not read, copy, use or disclose any part of this e-mail to others. Please notify the sender immediately and permanently delete this e-mail and any attachments if you received it in error. Internet communications cannot be guaranteed to be timely, secure, error-free or virus-free. The sender does not accept liability for any errors or omissions.
æéäååéäåæäåæèïåæåäæäåæéïäåéçæéäææçåæääãäçéçææäçãåäãååæååæéäæåååãèéèçåæääïèåéèãååã äçææéæéäçääååãèèææéäïèäççäæäæåéæéäåææéäïåäååéäçæååååçåääãææäèäèçéäåæãååãæèæéæãåääåääéæåäææèäã