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

From: Orson Zhai
Date: Thu Dec 05 2019 - 11:55:48 EST

Next message: RCS loan: "Apply for a Personal/Business Loan"
Previous message: Naveen N. Rao: "Re: [PATCH 2/3] powerpc/sysfs: Show idle_purr and idle_spurr for every CPU"
In reply to: Rob Herring: "Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to syscon easily"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Sorry! Re-send as plain-text mode.

On Thu, Dec 5, 2019 at 11:13 PM Rob Herring <robh@xxxxxxxxxx> 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@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.

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

Next message: RCS loan: "Apply for a Personal/Business Loan"
Previous message: Naveen N. Rao: "Re: [PATCH 2/3] powerpc/sysfs: Show idle_purr and idle_spurr for every CPU"
In reply to: Rob Herring: "Re: [PATCH V2 1/2] dt-bindings: syscon: Add syscon-names to refer to syscon easily"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]