* [RFC PATCH 1/5] of: introduce the overlay manager
From: Mathieu Poirier @ 2016-10-27 14:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161027140342.kidk4gqxwbtphrbs@kwain>
On 27 October 2016 at 08:03, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hello Mathieu,
>
> On Wed, Oct 26, 2016 at 10:29:59AM -0600, Mathieu Poirier wrote:
>>
>> Please find my comments below.
>
> Thanks for the comments. I expected more distant reviews, on the overall
> architecture to know if this could fit the needs of others. But anyway
> your comments are helpful if we ever decide to go with an overlay
> manager like this one.
I agree - something like this should have attracted more reviews. I
suggest providing a better explanation, i.e what you are doing and why
in more details. I reviewed your set and I'm still not sure of
exactly what it does, hence not being able to comment on the validity
of the approach.
I'm pretty sure there is value in what you are suggesting, it's a
matter of getting people to understand the approach and motivation.
>
>> On 26 October 2016 at 08:57, Antoine Tenart
>> <antoine.tenart@free-electrons.com> wrote:
>> > +
>> > +/*
>> > + * overlay_mgr_register_format()
>> > + *
>> > + * Adds a new format candidate to the list of supported formats. The registered
>> > + * formats are used to parse the headers stored on the dips.
>> > + */
>> > +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
>> > +{
>> > + struct overlay_mgr_format *format;
>> > + int err = 0;
>> > +
>> > + spin_lock(&overlay_mgr_format_lock);
>> > +
>> > + /* Check if the format is already registered */
>> > + list_for_each_entry(format, &overlay_mgr_formats, list) {
>> > + if (!strcpy(format->name, candidate->name)) {
>>
>> This function is public to the rest of the kernel - limiting the
>> lenght of ->name and using strncpy() is probably a good idea.
>
> I totally agree. This was in fact something I wanted to do.
>
>> > +
>> > +/*
>> > + * overlay_mgr_parse()
>> > + *
>> > + * Parse raw data with registered format parsers. Fills the candidate string if
>> > + * one parser understood the raw data format.
>> > + */
>> > +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
>>
>> I'm pretty sure there is another way to proceed than using 3 levels of
>> references. It makes the code hard to read and a prime candidate for
>> errors.
>
> Sure. I guess we could allocate an array of fixed-length strings which
> would be less flexible but I don't think we need something that flexible
> here.
>
>>
>> > +
>> > + format->parse(dev, data, candidates, n);
>>
>> ->parse() returns an error code. It is probably a good idea to check
>> it. If it isn't needed then a comment explaining why it is the case
>> would be appreciated.
>
> So the point of the parse function is to determine if the data read from
> a source is a compatible header of a given format. Returning an error
> doesn't mean the header won't be recognized by another one.
>
> We could maybe handle this better, by returning an error iif different
> that -EINVAL. Or we could have one function to check the compatibility
> and one to parse it, if compatible.
>
>> > +static int _overlay_mgr_apply(struct device *dev, char *candidate)
>> > +{
>> > + struct overlay_mgr_overlay *overlay;
>> > + struct device_node *node;
>> > + const struct firmware *firmware;
>> > + char *firmware_name;
>> > + int err = 0;
>> > +
>> > + spin_lock(&overlay_mgr_lock);
>> > +
>> > + list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
>> > + if (!strcmp(overlay->name, candidate)) {
>> > + dev_err(dev, "overlay already loaded\n");
>> > + err = -EEXIST;
>> > + goto err_lock;
>> > + }
>> > + }
>> > +
>> > + overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
>>
>> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
>> surprised the kernel didn't complain here. Allocate the memory before
>> holding the lock. If the overly is already loaded simply free it on
>> the error path.
>
> Right.
>
> Thanks,
>
> Antoine
>
> --
> Antoine T?nart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH v3 2/3] drm: zte: add initial vou drm driver
From: Shawn Guo @ 2016-10-27 14:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOw6vbKbdWJ8eJD3oXwPd6-ykaOu+QP6isqAz8HkjS0p+2wX=w@mail.gmail.com>
> On Thu, Oct 20, 2016 at 3:30 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > It adds the initial ZTE VOU display controller DRM driver. There are
> > still some features to be added, like overlay plane, scaling, and more
> > output devices support. But it's already useful with dual CRTCs and
> > HDMI monitor working.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
<snip>
> > +static int zx_drm_bind(struct device *dev)
> > +{
> > + struct drm_device *drm;
> > + struct zx_drm_private *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + drm = drm_dev_alloc(&zx_drm_driver, dev);
> > + if (!drm)
> > + return -ENOMEM;
> > +
> > + drm->dev_private = priv;
> > + dev_set_drvdata(dev, drm);
> > +
> > + drm_mode_config_init(drm);
> > + drm->mode_config.min_width = 16;
> > + drm->mode_config.min_height = 16;
> > + drm->mode_config.max_width = 4096;
> > + drm->mode_config.max_height = 4096;
> > + drm->mode_config.funcs = &zx_drm_mode_config_funcs;
> > +
> > + ret = component_bind_all(dev, drm);
> > + if (ret) {
> > + dev_err(dev, "failed to bind all components: %d\n", ret);
>
> Consider using the new DRM_DEV_* messages instead of the plain old dev_*
Okay, I will switch to DRM_DEV_* for log messages.
>
> > + goto out_unregister;
> > + }
> > +
> > + ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to init vblank: %d\n", ret);
> > + goto out_unbind;
> > + }
> > +
> > + /*
> > + * We will manage irq handler on our own. In this case, irq_enabled
> > + * need to be true for using vblank core support.
> > + */
> > + drm->irq_enabled = true;
> > +
> > + drm_mode_config_reset(drm);
> > + drm_kms_helper_poll_init(drm);
> > +
> > + priv->fbdev = drm_fbdev_cma_init(drm, 32, drm->mode_config.num_crtc,
> > + drm->mode_config.num_connector);
> > + if (IS_ERR(priv->fbdev)) {
> > + ret = PTR_ERR(priv->fbdev);
> > + dev_err(dev, "failed to init cma fbdev: %d\n", ret);
> > + priv->fbdev = NULL;
> > + goto out_poll_fini;
> > + }
> > +
> > + ret = drm_dev_register(drm, 0);
> > + if (ret)
> > + goto out_fbdev_fini;
> > +
> > + return 0;
> > +
> > +out_fbdev_fini:
> > + if (priv->fbdev) {
> > + drm_fbdev_cma_fini(priv->fbdev);
> > + priv->fbdev = NULL;
> > + }
> > +out_poll_fini:
> > + drm_kms_helper_poll_fini(drm);
> > + drm_mode_config_cleanup(drm);
> > + drm_vblank_cleanup(drm);
> > +out_unbind:
> > + component_unbind_all(dev, drm);
> > +out_unregister:
> > + dev_set_drvdata(dev, NULL);
> > + drm->dev_private = NULL;
> > + drm_dev_unref(drm);
> > + return ret;
> > +}
<snip>
> > +static int zx_hdmi_i2c_read(struct zx_hdmi *hdmi, struct i2c_msg *msg)
> > +{
> > + int len = msg->len;
> > + u8 *buf = msg->buf;
> > + int retry = 0;
> > + int ret = 0;
> > +
> > + /* Bits [9:8] of bytes */
> > + hdmi_writeb(hdmi, ZX_DDC_DIN_CNT2, (len >> 8) & 0xff);
> > + /* Bits [7:0] of bytes */
> > + hdmi_writeb(hdmi, ZX_DDC_DIN_CNT1, len & 0xff);
> > +
> > + /* Clear FIFO */
> > + hdmi_writeb_mask(hdmi, ZX_DDC_CMD, DDC_CMD_MASK, DDC_CMD_CLEAR_FIFO);
> > +
> > + /* Kick off the read */
> > + hdmi_writeb_mask(hdmi, ZX_DDC_CMD, DDC_CMD_MASK,
> > + DDC_CMD_SEQUENTIAL_READ);
> > +
> > + while (len > 0) {
> > + int cnt, i;
> > +
> > + /* FIFO needs some time to get ready */
> > + usleep_range(500, 1000);
> > +
> > + cnt = hdmi_readb(hdmi, ZX_DDC_DOUT_CNT) & DDC_DOUT_CNT_MASK;
> > + if (cnt == 0) {
> > + if (++retry > 5) {
> > + dev_err(hdmi->dev, "DDC FIFO read timed out!");
> > + ret = -ETIMEDOUT;
> > + break;
>
> Why not just return -ETIMEDOUT here?
Yes. Stupid me.
> > + }
> > + continue;
> > + }
> > +
> > + for (i = 0; i < cnt; i++)
> > + *buf++ = hdmi_readb(hdmi, ZX_DDC_DATA);
> > + len -= cnt;
> > + }
> > +
> > + return ret;
> > +}
<snip>
> > +static int zx_hdmi_bind(struct device *dev, struct device *master, void *data)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct drm_device *drm = data;
> > + struct resource *res;
> > + struct zx_hdmi *hdmi;
> > + int irq;
> > + int ret;
> > +
> > + hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> > + if (!hdmi)
> > + return -ENOMEM;
> > +
> > + hdmi->dev = dev;
> > + hdmi->drm = drm;
> > + hdmi->inf = &vou_inf_hdmi;
> > +
> > + dev_set_drvdata(dev, hdmi);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + hdmi->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(hdmi->mmio)) {
> > + ret = PTR_ERR(hdmi->mmio);
> > + dev_err(dev, "failed to remap hdmi region: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + hdmi->cec_clk = devm_clk_get(hdmi->dev, "osc_cec");
> > + if (IS_ERR(hdmi->cec_clk)) {
> > + ret = PTR_ERR(hdmi->cec_clk);
> > + dev_err(dev, "failed to get cec_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + hdmi->osc_clk = devm_clk_get(hdmi->dev, "osc_clk");
> > + if (IS_ERR(hdmi->osc_clk)) {
> > + ret = PTR_ERR(hdmi->osc_clk);
> > + dev_err(dev, "failed to get osc_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + hdmi->xclk = devm_clk_get(hdmi->dev, "xclk");
> > + if (IS_ERR(hdmi->xclk)) {
> > + ret = PTR_ERR(hdmi->xclk);
> > + dev_err(dev, "failed to get xclk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + zx_hdmi_hw_init(hdmi);
> > +
> > + ret = clk_prepare_enable(hdmi->cec_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable cec_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(hdmi->osc_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable osc_clk: %d\n", ret);
> > + goto disable_cec_clk;
> > + }
> > +
> > + ret = clk_prepare_enable(hdmi->xclk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable xclk: %d\n", ret);
> > + goto disable_osc_clk;
> > + }
>
> Perhaps add a TODO above hdmi_hw_init() to move it and the clock
> enables to .enable and conversely implement .disable?
Instead of leaving a TODO item there, I would like to change it right
away in the next version.
> > +
> > +
> > + ret = zx_hdmi_ddc_register(hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to register ddc: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + ret = zx_hdmi_register(drm, hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to register hdmi: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, irq, zx_hdmi_irq_handler,
> > + zx_hdmi_irq_thread, IRQF_SHARED,
> > + dev_name(dev), hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to request threaded irq: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + return 0;
> > +
> > +disable_xclk:
> > + clk_disable_unprepare(hdmi->xclk);
> > +disable_osc_clk:
> > + clk_disable_unprepare(hdmi->osc_clk);
> > +disable_cec_clk:
> > + clk_disable_unprepare(hdmi->cec_clk);
> > + return ret;
> > +}
<snip>
> > +static int zx_gl_plane_atomic_check(struct drm_plane *plane,
> > + struct drm_plane_state *plane_state)
> > +{
> > + struct drm_framebuffer *fb = plane_state->fb;
> > + struct drm_crtc *crtc = plane_state->crtc;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_rect clip;
> > +
> > + if (!crtc || !fb)
> > + return 0;
> > +
> > + crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > + crtc);
> > + if (WARN_ON(!crtc_state))
> > + return -EINVAL;
> > +
> > + /* plane must match crtc enable state */
> > + if (crtc_state->enable != !!plane_state->crtc)
> > + return -EINVAL;
> > +
> > + /* nothing to check when disabling or disabled */
> > + if (!crtc_state->enable)
> > + return 0;
>
> I think you can shuffle things around here to read a bit clearer:
>
> if (!crtc_state->enable)
> return 0;
>
> if (!plane_state->crtc)
> return -EINVAL
Indeed.
> > +
> > + clip.x1 = 0;
> > + clip.y1 = 0;
> > + clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > + clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +
> > + return drm_plane_helper_check_state(plane_state, &clip,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + false, true);
> > +}
<snip>
> > +/* Sub-module offset */
> > +#define MAIN_GL_OFFSET 0x130
> > +#define MAIN_CSC_OFFSET 0x580
> > +#define MAIN_HBSC_OFFSET 0x820
> > +#define MAIN_RSZ_OFFSET 0x600 /* OTFPPU sub-module */
> > +
> > +#define AUX_GL_OFFSET 0x200
> > +#define AUX_CSC_OFFSET 0x5d0
> > +#define AUX_HBSC_OFFSET 0x860
> > +#define AUX_RSZ_OFFSET 0x800
> > +
> > +/* OSD (GPC_GLOBAL) registers */
> > +#define OSD_INT_STA 0x04
> > +#define OSD_INT_CLRSTA 0x08
> > +#define OSD_INT_MSK 0x0c
> > +#define OSD_INT_AUX_UPT BIT(14)
> > +#define OSD_INT_MAIN_UPT BIT(13)
> > +#define OSD_INT_GL1_LBW BIT(10)
> > +#define OSD_INT_GL0_LBW BIT(9)
> > +#define OSD_INT_VL2_LBW BIT(8)
> > +#define OSD_INT_VL1_LBW BIT(7)
> > +#define OSD_INT_VL0_LBW BIT(6)
> > +#define OSD_INT_BUS_ERR BIT(3)
> > +#define OSD_INT_CFG_ERR BIT(2)
> > +#define OSD_INT_ERROR (\
> > + OSD_INT_GL1_LBW | OSD_INT_GL0_LBW | \
> > + OSD_INT_VL2_LBW | OSD_INT_VL1_LBW | OSD_INT_VL0_LBW | \
> > + OSD_INT_BUS_ERR | OSD_INT_CFG_ERR \
> > +)
> > +#define OSD_INT_ENABLE (OSD_INT_ERROR | OSD_INT_AUX_UPT | OSD_INT_MAIN_UPT)
> > +#define OSD_CTRL0 0x10
> > +#define OSD_CTRL0_GL0_EN BIT(7)
> > +#define OSD_CTRL0_GL0_SEL BIT(6)
> > +#define OSD_CTRL0_GL1_EN BIT(5)
> > +#define OSD_CTRL0_GL1_SEL BIT(4)
> > +#define OSD_RST_CLR 0x1c
> > +#define RST_PER_FRAME BIT(19)
> > +
> > +/* Main/Aux channel registers */
> > +#define OSD_MAIN_CHN 0x470
> > +#define OSD_AUX_CHN 0x4d0
> > +#define CHN_CTRL0 0x00
> > +#define CHN_ENABLE BIT(0)
> > +#define CHN_CTRL1 0x04
> > +#define CHN_SCREEN_W_SHIFT 18
> > +#define CHN_SCREEN_W_MASK (0x1fff << CHN_SCREEN_W_SHIFT)
> > +#define CHN_SCREEN_H_SHIFT 5
> > +#define CHN_SCREEN_H_MASK (0x1fff << CHN_SCREEN_H_SHIFT)
> > +#define CHN_UPDATE 0x08
> > +
> > +/* TIMING_CTRL registers */
> > +#define TIMING_TC_ENABLE 0x04
> > +#define AUX_TC_EN BIT(1)
> > +#define MAIN_TC_EN BIT(0)
> > +#define FIR_MAIN_ACTIVE 0x08
> > +#define FIR_AUX_ACTIVE 0x0c
> > +#define V_ACTIVE_SHIFT 16
> > +#define V_ACTIVE_MASK (0xffff << V_ACTIVE_SHIFT)
> > +#define H_ACTIVE_SHIFT 0
> > +#define H_ACTIVE_MASK (0xffff << H_ACTIVE_SHIFT)
> > +#define FIR_MAIN_H_TIMING 0x10
> > +#define FIR_MAIN_V_TIMING 0x14
> > +#define FIR_AUX_H_TIMING 0x18
> > +#define FIR_AUX_V_TIMING 0x1c
> > +#define SYNC_WIDE_SHIFT 22
> > +#define SYNC_WIDE_MASK (0x3ff << SYNC_WIDE_SHIFT)
> > +#define BACK_PORCH_SHIFT 11
> > +#define BACK_PORCH_MASK (0x7ff << BACK_PORCH_SHIFT)
> > +#define FRONT_PORCH_SHIFT 0
> > +#define FRONT_PORCH_MASK (0x7ff << FRONT_PORCH_SHIFT)
> > +#define TIMING_CTRL 0x20
> > +#define AUX_POL_SHIFT 3
> > +#define AUX_POL_MASK (0x7 << AUX_POL_SHIFT)
> > +#define MAIN_POL_SHIFT 0
> > +#define MAIN_POL_MASK (0x7 << MAIN_POL_SHIFT)
> > +#define POL_DE_SHIFT 2
> > +#define POL_VSYNC_SHIFT 1
> > +#define POL_HSYNC_SHIFT 0
> > +#define TIMING_INT_CTRL 0x24
> > +#define TIMING_INT_STATE 0x28
> > +#define TIMING_INT_AUX_FRAME BIT(3)
> > +#define TIMING_INT_MAIN_FRAME BIT(1)
> > +#define TIMING_INT_AUX_FRAME_SEL_VSW (0x2 << 10)
> > +#define TIMING_INT_MAIN_FRAME_SEL_VSW (0x2 << 6)
> > +#define TIMING_INT_ENABLE (\
> > + TIMING_INT_MAIN_FRAME_SEL_VSW | TIMING_INT_AUX_FRAME_SEL_VSW | \
> > + TIMING_INT_MAIN_FRAME | TIMING_INT_AUX_FRAME \
> > +)
> > +#define TIMING_MAIN_SHIFT 0x2c
> > +#define TIMING_AUX_SHIFT 0x30
> > +#define H_SHIFT_VAL 0x0048
> > +#define TIMING_MAIN_PI_SHIFT 0x68
> > +#define TIMING_AUX_PI_SHIFT 0x6c
> > +#define H_PI_SHIFT_VAL 0x000f
> > +
> > +#define V_ACTIVE(x) (((x) << V_ACTIVE_SHIFT) & V_ACTIVE_MASK)
> > +#define H_ACTIVE(x) (((x) << H_ACTIVE_SHIFT) & H_ACTIVE_MASK)
> > +
> > +#define SYNC_WIDE(x) (((x) << SYNC_WIDE_SHIFT) & SYNC_WIDE_MASK)
> > +#define BACK_PORCH(x) (((x) << BACK_PORCH_SHIFT) & BACK_PORCH_MASK)
> > +#define FRONT_PORCH(x) (((x) << FRONT_PORCH_SHIFT) & FRONT_PORCH_MASK)
> > +
> > +/* DTRC registers */
> > +#define DTRC_F0_CTRL 0x2c
> > +#define DTRC_F1_CTRL 0x5c
> > +#define DTRC_DECOMPRESS_BYPASS BIT(17)
> > +#define DTRC_DETILE_CTRL 0x68
> > +#define TILE2RASTESCAN_BYPASS_MODE BIT(30)
> > +#define DETILE_ARIDR_MODE_MASK (0x3 << 0)
> > +#define DETILE_ARID_ALL 0
> > +#define DETILE_ARID_IN_ARIDR 1
> > +#define DETILE_ARID_BYP_BUT_ARIDR 2
> > +#define DETILE_ARID_IN_ARIDR2 3
> > +#define DTRC_ARID 0x6c
> > +#define DTRC_ARID3_SHIFT 24
> > +#define DTRC_ARID3_MASK (0xff << DTRC_ARID3_SHIFT)
> > +#define DTRC_ARID2_SHIFT 16
> > +#define DTRC_ARID2_MASK (0xff << DTRC_ARID2_SHIFT)
> > +#define DTRC_ARID1_SHIFT 8
> > +#define DTRC_ARID1_MASK (0xff << DTRC_ARID1_SHIFT)
> > +#define DTRC_ARID0_SHIFT 0
> > +#define DTRC_ARID0_MASK (0xff << DTRC_ARID0_SHIFT)
> > +#define DTRC_DEC2DDR_ARID 0x70
> > +
> > +#define DTRC_ARID3(x) (((x) << DTRC_ARID3_SHIFT) & DTRC_ARID3_MASK)
> > +#define DTRC_ARID2(x) (((x) << DTRC_ARID2_SHIFT) & DTRC_ARID2_MASK)
> > +#define DTRC_ARID1(x) (((x) << DTRC_ARID1_SHIFT) & DTRC_ARID1_MASK)
> > +#define DTRC_ARID0(x) (((x) << DTRC_ARID0_SHIFT) & DTRC_ARID0_MASK)
> > +
> > +/* VOU_CTRL registers */
> > +#define VOU_INF_EN 0x00
> > +#define VOU_INF_CH_SEL 0x04
> > +#define VOU_INF_DATA_SEL 0x08
> > +#define VOU_SOFT_RST 0x14
> > +#define VOU_CLK_SEL 0x18
> > +#define VOU_CLK_GL1_SEL BIT(5)
> > +#define VOU_CLK_GL0_SEL BIT(4)
> > +#define VOU_CLK_REQEN 0x20
> > +#define VOU_CLK_EN 0x24
> > +
> > +/* OTFPPU_CTRL registers */
> > +#define OTFPPU_RSZ_DATA_SOURCE 0x04
> > +
>
> I find the register definitions pretty distracting here (and elsewhere
> in the driver), I suppose my personal preference would be to hide them
> in the headers.
I do not have a strong preference on this. Okay, will do that to make
it easier for you to look at the driver :)
> > +#define GL_NUM 2
> > +#define VL_NUM 3
> > +
> > +enum vou_chn_type {
> > + VOU_CHN_MAIN,
> > + VOU_CHN_AUX,
> > +};
> > +
> > +struct zx_crtc_regs {
> > + u32 fir_active;
> > + u32 fir_htiming;
> > + u32 fir_vtiming;
> > + u32 timing_shift;
> > + u32 timing_pi_shift;
> > +};
<snip>
> > +static inline struct drm_crtc *zx_find_crtc(struct drm_device *drm, int pipe)
> > +{
> > + struct drm_crtc *crtc;
> > + int i = 0;
> > +
> > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> > + if (i++ == pipe)
> > + return crtc;
> > +
> > + return NULL;
> > +}
>
> We have drm_plane_from_index, it seems like at least 2 drivers would
> benefit from drm_crtc_from_index. Either way, you should probably
> change this code to compare pipe to crtc->index instead of rolling
> your own index counter.
Right. I will change it to use crtc->index at this point. And after
the driver gets landed, I will cook up a patch for drm_crtc_from_index.
> > +int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe)
> > +{
> > + struct drm_crtc *crtc;
> > + struct zx_vou_hw *vou;
> > +
> > + crtc = zx_find_crtc(drm, pipe);
> > + if (!crtc)
> > + return 0;
> > +
> > + vou = crtc_to_vou(crtc);
> > +
> > + if (pipe == 0)
> > + zx_writel_mask(vou->timing + TIMING_INT_CTRL,
> > + TIMING_INT_MAIN_FRAME, TIMING_INT_MAIN_FRAME);
> > + else
> > + zx_writel_mask(vou->timing + TIMING_INT_CTRL,
> > + TIMING_INT_AUX_FRAME, TIMING_INT_AUX_FRAME);
> > +
>
> It seems like this could also benefit from crtc_bits/crtc_regs
Yes, you're right.
Thanks for all the comments.
Shawn
> > + return 0;
> > +}
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Jason Gunthorpe @ 2016-10-27 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c0e99b6e-ae72-5c64-34c1-91865d0e00b5@suse.com>
On Thu, Oct 27, 2016 at 10:50:48AM +0200, Matthias Brugger wrote:
> >+ /* Sanity check the proposed bitstream. It must start with the
> >+ * sync word in the correct byte order and be a multiple of 4
> >+ * bytes.
> >+ */
> >+ if (count <= 4 || buf[0] != 0x66 || buf[1] != 0x55 ||
> >+ buf[2] != 0x99 || buf[3] != 0xaa) {
>
> This checks if the bit stream is bigger then 4 bytes. We error out before,
> if it is smaller.
We do? Where?
> So you should fix the wording in the comment and check for count ==
> 4.
Ah right, the comment reflected an earlier revision that had the
length check here.
The count <= 4 should stay here since it is primarily guarding against
read past the buffer in the if.
Jason
^ permalink raw reply
* [PATCH] drm/sun4i: Add a few formats
From: Maxime Ripard @ 2016-10-27 14:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGb2v65=6YGaaC3QimPHyP=FKJofygReozmnKLV8CGXvjWGtvQ@mail.gmail.com>
Hi,
On Tue, Oct 25, 2016 at 08:42:26AM +0800, Chen-Yu Tsai wrote:
> On Mon, Oct 24, 2016 at 10:40 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Fri, Oct 21, 2016 at 11:15:32AM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Oct 18, 2016 at 4:46 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > The planes can do more than what was previously exposed. Add support for
> >> > them.
> >> >
> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >> > ---
> >> > drivers/gpu/drm/sun4i/sun4i_backend.c | 20 ++++++++++++++++++++
> >> > drivers/gpu/drm/sun4i/sun4i_layer.c | 6 ++++++
> >> > 2 files changed, 26 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> > index afb7ddf660ef..b184a476a480 100644
> >> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> >> > @@ -96,6 +96,22 @@ static int sun4i_backend_drm_format_to_layer(struct drm_plane *plane,
> >> > *mode = SUN4I_BACKEND_LAY_FBFMT_ARGB8888;
> >> > break;
> >> >
> >> > + case DRM_FORMAT_ARGB4444:
> >> > + *mode = SUN4I_BACKEND_LAY_FBFMT_ARGB4444;
> >> > + break;
> >> > +
> >> > + case DRM_FORMAT_ARGB1555:
> >> > + *mode = SUN4I_BACKEND_LAY_FBFMT_ARGB1555;
> >> > + break;
> >> > +
> >> > + case DRM_FORMAT_RGBA5551:
> >> > + *mode = SUN4I_BACKEND_LAY_FBFMT_RGBA5551;
> >> > + break;
> >> > +
> >> > + case DRM_FORMAT_RGBA4444:
> >> > + *mode = SUN4I_BACKEND_LAY_FBFMT_RGBA4444;
> >>
> >> The A20 manual only lists ARGB4444, not RGBA4444. There might be
> >> some discrepancy here. We can deal with them
> >
> > Hmm, yes, that's weird. But I guess this would be part of porting it
> > to the A20.
> >
> >> Also there are some more formats missing from the list, could you
> >> add them as well?
> >
> > Which one do you refer to?
>
> RGB556 and RGB655.
These formats are not supported by Linux yet though.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/0673d783/attachment.sig>
^ permalink raw reply
* [PATCH v3] ARM: davinci: da8xx: Fix some redefined symbol warnings
From: Alexandre Bailon @ 2016-10-27 14:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ca32fd12-5299-02e8-24c4-501dda4291ab@ti.com>
On 10/27/2016 01:54 PM, Sekhar Nori wrote:
> On Thursday 27 October 2016 05:13 PM, Sekhar Nori wrote:
>> On Wednesday 26 October 2016 06:09 PM, Alexandre Bailon wrote:
>>> Some macro for DA8xx CFGCHIP are defined in usb-davinci.h,
>>> but da8xx-cfgchip.h intend to replace them.
>>> The usb-da8xx.c is using both headers, causing redefined symbol warnings.
>>
>> Looks like this is not true for v4.9-rc2 and so I don't see any warnings
>
> Ah, just noticed that _this_ is the patch that introduces
> da8xx-cfgchip.h into usb-da8xx.c. So this is the patch that introduces
> the warnings (and fixes them).
>
> I can queue this for v4.10 (with Greg's ack) if you change the
> description to make it about cleaning up duplicated defines between
> da8xx-cfgchip.h and usb-davinci.h and not talk about "redefined symbol
> warnings".
I will do it.
>
> Also, when adding a header file, can you please keep it sorted in
> alphabetical order.
Ok.
>
> Thanks,
> Sekhar
>
Thanks,
Alexandre
^ permalink raw reply
* [PATCH] fpga zynq: Check the bitstream for validity
From: Jason Gunthorpe @ 2016-10-27 14:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <bdae79f1-dfed-d735-cb49-9ce8aa0b6e4f@xilinx.com>
On Thu, Oct 27, 2016 at 09:42:03AM +0200, Michal Simek wrote:
> I am not quite sure about this and I didn't try it on real hw.
> But minimum bitstream size 52+B and more likely much more than this.
Oh probably, I didn't try to guess what the minimum size is, that
check is just to prevent reading past the end of the buffer.
> This is taken from u-boot source code and this is full BIN header.
> The code above is checking only the last word.
There can be garbage before the sync word. The hardware ignores
everything till it gets the sync word. Prior versions of the driver
with the autodetection would discard the garbage.
Since the autodetection was ripped out I didn't want to search since
the intent seems to be for user space to provide a full bitstream,
which should start at the sync word, but that is another option.
> 0x000000bb, /* Sync word */
> 0x11220044, /* Sync word */
> DUMMY_WORD,
> DUMMY_WORD,
> 0xaa995566, /* Sync word */
This is the bus width detection pattern, I understood the Zync DevC
interface was wired to 32 bits and did not respond to this.
Jason
^ permalink raw reply
* [RFC PATCH 0/5] Add an overlay manager to handle board capes
From: Antoine Tenart @ 2016-10-27 14:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL_JsqL9yWBj0yYE54XGi87YPGugGAACzr=CuW6dk5kk3EuyCA@mail.gmail.com>
Hi Rob,
On Thu, Oct 27, 2016 at 08:41:56AM -0500, Rob Herring wrote:
> Please Cc the maintainers of drivers/of/.
>
> + Frank R, Hans, Dmitry S
Yes, sorry for that.
> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> >
> > Many boards now come with dips and compatible capes; among others the
> > C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
> > out-of-tree "cape manager" which is used to detected capes, retrieve
> > their description and apply a corresponding overlay. This series is an
> > attempt to start a discussion, with an implementation of such a manager
> > which is somehow generic (i.e. formats or cape detectors can be added).
> > Other use cases could make use of this manager to dynamically load dt
> > overlays based on some input / hw presence.
>
> I'd like to see an input source be the kernel command line and/or a DT
> chosen property.
We now have overlay support in U-Boot so we could modify the device tree
from the bootloader and not use the command line. But you can argue the
boot loader can't always be upgraded (to support overlays, or to improve
it). So I guess we can think of using the command line as a input
source.
> Another overlay manager was proposed not to long ago[1] as well.
Thanks for the hint.
> Another thing to consider is different sources of overlays. Besides in
> the filesystem, overlays could be built into the kernel (already
> supported), embedded in the dtb (as the other overlay mgr did) or we
> could extend FDT format to append them.
Sure. Using this series it should be quite easy to support other
sources. We would need to improve the function loading the overlay, to
try other sources. This could even comes in following up patches.
> > The proposed design is a library which can be used by detector drivers
> > to parse headers and load the corresponding overlay. Helpers are
> > provided for this purpose. The whole thing is divided into 3 entities:
> >
> > - The parser which is project-specific (to allow supporting headers
> > already into the wild). It registers a function parsing an header's
> > data and filling one or more strings which will be used to find
> > matching dtbo on the fs.
> >
> > - The overlay manager helpers allowing to parse a header to retrieve
> > the previously mentioned strings and to load a compatible overlay.
> >
> > - The detectors which are used to detect capes and get their description
> > (to be parsed).
>
> What about things like power has to be turned on first to detect
> boards and read their ID? I think this needs to be tied into the
> driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
> a driver gets bound to a connector node, but we've got to sort out
> connector bindings first.
Right. I don't know yet how to handle this. Do you have an existing
example in mind of such a power requirement?
> > An example of parser and detector is given, compatible with what's done
> > for the C.H.I.P. As the w1 framework is really bad (and we should
> > probably do something about that) the detector code is far from being
> > perfect; but that's not related to what we try to achieve here.
> >
> > The actual implementation has a limitation: the detectors cannot be
> > built-in the kernel image as they would likely detect capes at boot time
> > but will fail to get their corresponding dt overlays as the fs isn't
> > mounted yet. The only case this can work is when dt overlays are
> > built-in firmwares. This isn't an issue for the C.H.I.P. use case right
> > now. There was a discussion about making an helper to wait for the
> > rootfs to be mount but the answer was "this is the driver's problem".
>
> I thought there are firmware loading calls that will wait. I think
> this all needs to work asynchronously both for firmware loading and
> because w1 is really slow.
There is an asynchronous one, but it will also fail if the rootfs isn't
mounted yet.
Thanks!
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/30ab5fd8/attachment-0001.sig>
^ permalink raw reply
* [PATCH] ARM: davinci: register the usb20_phy clock on the DT file
From: kbuild test robot @ 2016-10-27 14:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161026194916.14546-1-ahaslam@baylibre.com>
Hi Axel,
[auto build test ERROR on arm-soc/for-next]
[also build test ERROR on v4.9-rc2 next-20161027]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/ahaslam-baylibre-com/ARM-davinci-register-the-usb20_phy-clock-on-the-DT-file/20161027-040551
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git for-next
config: arm-davinci_all_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
arch/arm/mach-davinci/da8xx-dt.c: In function 'da850_init_machine':
>> arch/arm/mach-davinci/da8xx-dt.c:48:2: error: implicit declaration of function 'da8xx_register_usb20_phy_clk' [-Werror=implicit-function-declaration]
da8xx_register_usb20_phy_clk(false);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/arm/mach-davinci/da8xx-dt.c:49:2: error: implicit declaration of function 'da8xx_register_usb11_phy_clk' [-Werror=implicit-function-declaration]
da8xx_register_usb11_phy_clk(false);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/da8xx_register_usb20_phy_clk +48 arch/arm/mach-davinci/da8xx-dt.c
42 };
43
44 #ifdef CONFIG_ARCH_DAVINCI_DA850
45
46 static void __init da850_init_machine(void)
47 {
> 48 da8xx_register_usb20_phy_clk(false);
> 49 da8xx_register_usb11_phy_clk(false);
50 of_platform_default_populate(NULL, da850_auxdata_lookup, NULL);
51 }
52
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 21064 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/615d600d/attachment-0001.gz>
^ permalink raw reply
* [PATCH] ASoC: hdmi-codec: Fix hdmi_of_xlate_dai_name when #sound-dai-cells = <0>
From: Mark Brown @ 2016-10-27 14:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477568568.2908.14.camel@linaro.org>
On Thu, Oct 27, 2016 at 12:42:48PM +0100, Jon Medhurst (Tixy) wrote:
> - int id = args->args[0];
> + int id = args->args_count ? args->args[0] : 0;
Please write normal if statements for legibility.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/35ec96ca/attachment.sig>
^ permalink raw reply
* [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
From: Mark Rutland @ 2016-10-27 14:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <DB4PR04MB07813D4DA5D5567229394A7D8FAB0@DB4PR04MB0781.eurprd04.prod.outlook.com>
On Wed, Oct 26, 2016 at 10:09:07PM +0000, Leo Li wrote:
>
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland at arm.com]
> > Sent: Wednesday, October 26, 2016 5:31 AM
> > To: M.H. Lian <minghuan.lian@nxp.com>
> > Cc: linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> > devicetree at vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Stuart
> > Yoder <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> > <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> > <mingkai.hu@nxp.com>
> > Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
> >
> > On Tue, Oct 25, 2016 at 08:35:40PM +0800, Minghuan Lian wrote:
> > > -- compatible: should be "fsl,<soc-name>-msi" to identify
> > > - Layerscape PCIe MSI controller block such as:
> > > - "fsl,1s1021a-msi"
> > > - "fsl,1s1043a-msi"
> > > +- compatible: should be "fsl,ls-scfg-msi"
> >
> > This breaks old DTBs, and throws away information which you describe above as
> > valuable. So another NAK for that.
>
> I agree with you that we should maintain the backward compatibility.
> But on the other hand, I just found that there is a silly typo in the
> original binding that "ls" is wrongly spelled as "1s" and they look
> too close to be noticed in previous patch reviews. :(
Sure, that's annoying, but we're stuck with it.
> The driver and all the DTSes used the binding with the typo which
> covered up the problem. So even if we want to keep the
> "fsl,<soc-name>-msi" binding, we probably want to fix the typo, right?
> And that breaks the backward compatibility too.
Regardless of what we do, we should *not* break compatibility. The old
strings must remain.
However, we can *add* correctly-spelt variants, and mark the existing
strings as deprecated (in both the binding and driver). The in-kernel
dts can be updated to use the correctly-spelt strings.
Thanks,
Mark.
^ permalink raw reply
* [PATCH v2 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2016-10-27 14:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8724b41d-158a-f52c-8df6-4e8913b4094a@ti.com>
On 10/27/2016 04:02 AM, Tero Kristo wrote:
> On 27/10/16 01:04, Rob Herring wrote:
>> On Wed, Oct 19, 2016 at 03:33:45PM -0500, Dave Gerlach wrote:
>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>> will hook into the genpd framework and allow the TI SCI protocol to
>>> control device power states.
>>>
>>> Also, provide macros representing each device index as understood
>>> by TI SCI to be used in the device node power-domain references.
>>> These are identifiers for the K2G devices managed by the PMMC.
>>>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>> ---
>>> .../devicetree/bindings/soc/ti/sci-pm-domain.txt | 54 +++++++++++++
>>> MAINTAINERS | 2 +
>>> include/dt-bindings/genpd/k2g.h | 90 ++++++++++++++++++++++
>>> 3 files changed, 146 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>> create mode 100644 include/dt-bindings/genpd/k2g.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>> b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>> new file mode 100644
>>> index 000000000000..32f38a349656
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>> @@ -0,0 +1,54 @@
>>> +Texas Instruments TI-SCI Generic Power Domain
>>> +---------------------------------------------
>>> +
>>> +Some TI SoCs contain a system controller (like the PMMC, etc...) that is
>>> +responsible for controlling the state of the IPs that are present.
>>> +Communication between the host processor running an OS and the system
>>> +controller happens through a protocol known as TI-SCI [1]. This pm domain
>>> +implementation plugs into the generic pm domain framework and makes use of
>>> +the TI SCI protocol power on and off each device when needed.
>>> +
>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>> +
>>> +PM Domain Node
>>> +==============
>>> +The PM domain node represents the global PM domain managed by the PMMC,
>>> +which in this case is the single implementation as documented by the generic
>>> +PM domain bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>>> +
>>> +Required Properties:
>>> +--------------------
>>> +- compatible: should be "ti,sci-pm-domain"
>>> +- #power-domain-cells: Must be 0.
>>> +- ti,sci: Phandle to the TI SCI device to use for managing the devices.
>>> +
>>> +Example:
>>> +--------------------
>>> +k2g_pds: k2g_pds {
>>> + compatible = "ti,sci-pm-domain";
>>> + #power-domain-cells = <0>;
>>> + ti,sci = <&pmmc>;
>>> +};
>>
>> Why not just make the PMMC node be the power-domain provider itself? If
>> not that, then make this a child node of it. The same comment applies to
>> all the SCI functions, but I guess I've already acked some of them.
>
> This seems to be a bug in this documentation actually. ti,sci handle is no
> longer supported, and all the sci stuff must be under the parent sci node.
>
>>
>> I really don't like reviewing all these TI SCI bindings one by one. Each
>> one on its own seems fine, but I don't see the full picture.
>
> The full picture is represented under the documentation for the main protocol
> support itself. See this patch:
>
> https://patchwork.kernel.org/patch/9383281/
>
> Copy pasted here as ref:
>
> Example (K2G):
> -------------
> pmmc: pmmc {
> compatible = "ti,k2g-sci";
> ...
>
> my_clk_node: clk_node {
> ...
> ...
> };
>
> my_pd_node: pd_node {
> ...
> ...
> };
> };
>
>
Yes my bad I will fix this in V3 once we straighten out the ID portion of the
binding.
Regards,
Dave
^ permalink raw reply
* [RFC PATCH 1/5] of: introduce the overlay manager
From: Antoine Tenart @ 2016-10-27 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANLsYkyszsn2TLYhUjHMt2ZnEUmE9eSc71W2KWBL0fWhh3PPBw@mail.gmail.com>
Hello Mathieu,
On Wed, Oct 26, 2016 at 10:29:59AM -0600, Mathieu Poirier wrote:
>
> Please find my comments below.
Thanks for the comments. I expected more distant reviews, on the overall
architecture to know if this could fit the needs of others. But anyway
your comments are helpful if we ever decide to go with an overlay
manager like this one.
> On 26 October 2016 at 08:57, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> > +
> > +/*
> > + * overlay_mgr_register_format()
> > + *
> > + * Adds a new format candidate to the list of supported formats. The registered
> > + * formats are used to parse the headers stored on the dips.
> > + */
> > +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> > +{
> > + struct overlay_mgr_format *format;
> > + int err = 0;
> > +
> > + spin_lock(&overlay_mgr_format_lock);
> > +
> > + /* Check if the format is already registered */
> > + list_for_each_entry(format, &overlay_mgr_formats, list) {
> > + if (!strcpy(format->name, candidate->name)) {
>
> This function is public to the rest of the kernel - limiting the
> lenght of ->name and using strncpy() is probably a good idea.
I totally agree. This was in fact something I wanted to do.
> > +
> > +/*
> > + * overlay_mgr_parse()
> > + *
> > + * Parse raw data with registered format parsers. Fills the candidate string if
> > + * one parser understood the raw data format.
> > + */
> > +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
>
> I'm pretty sure there is another way to proceed than using 3 levels of
> references. It makes the code hard to read and a prime candidate for
> errors.
Sure. I guess we could allocate an array of fixed-length strings which
would be less flexible but I don't think we need something that flexible
here.
>
> > +
> > + format->parse(dev, data, candidates, n);
>
> ->parse() returns an error code. It is probably a good idea to check
> it. If it isn't needed then a comment explaining why it is the case
> would be appreciated.
So the point of the parse function is to determine if the data read from
a source is a compatible header of a given format. Returning an error
doesn't mean the header won't be recognized by another one.
We could maybe handle this better, by returning an error iif different
that -EINVAL. Or we could have one function to check the compatibility
and one to parse it, if compatible.
> > +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> > +{
> > + struct overlay_mgr_overlay *overlay;
> > + struct device_node *node;
> > + const struct firmware *firmware;
> > + char *firmware_name;
> > + int err = 0;
> > +
> > + spin_lock(&overlay_mgr_lock);
> > +
> > + list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> > + if (!strcmp(overlay->name, candidate)) {
> > + dev_err(dev, "overlay already loaded\n");
> > + err = -EEXIST;
> > + goto err_lock;
> > + }
> > + }
> > +
> > + overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
>
> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
> surprised the kernel didn't complain here. Allocate the memory before
> holding the lock. If the overly is already loaded simply free it on
> the error path.
Right.
Thanks,
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/ca58ff75/attachment.sig>
^ permalink raw reply
* [PATCH 02/12] ASoC: dapm: Implement stereo mixer control support
From: Chen-Yu Tsai @ 2016-10-27 14:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161026175052.GE25322@sirena.org.uk>
On Thu, Oct 27, 2016 at 1:50 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 03, 2016 at 07:07:54PM +0800, Chen-Yu Tsai wrote:
>
>> /* find dapm widget path assoc with kcontrol */
>> dapm_kcontrol_for_each_path(path, kcontrol) {
>> + /*
>> + * If status for the second channel was given ( >= 0 ),
>> + * consider the second and later paths as the second
>> + * channel.
>> + */
>> + if (found && rconnect >= 0)
>> + soc_dapm_connect_path(path, rconnect, "mixer update");
>> + else
>> + soc_dapm_connect_path(path, connect, "mixer update");
>> found = 1;
>> - soc_dapm_connect_path(path, connect, "mixer update");
>
> This really only works for two channels with the current inteface - the
> comment makes it sound like it'll work for more but we can only pass in
> two (and there's only support for specifying two everywhere).
I could rework it to pass a list of connected status' and the number
of elements instead, but it wouldn't work well for situations where
the number of channels on the kcontrol != the number of paths. I'm not
sure if that's even a valid setup, but it does work with the current
core code.
On the other hand, are there kcontrols that are multi-channel
(> 2 channels)?
I'm inclined to just fixup the comment to make it clear that the
implementation supports stereo, i.e. 2 channels, only.
>
>> - change = dapm_kcontrol_set_value(kcontrol, val);
>> + /* This assumes field width < (bits in unsigned int / 2) */
>> + change = dapm_kcontrol_set_value(kcontrol, val | (rval << width));
>
> That seems like a bit of an assumption in cases where we've got a single
> control for both power and volume? They're very rare though, I'm not
> even sure any exist. It'd be good to have a check in the code just in
> case it does come up, it'll likely be a nightmare to debug if someone
> does run into it.
Agreed. I'll put a check and warning before it.
>
> Otherwise I think this makes sense.
Thanks for the review!
Regards
ChenYu
^ permalink raw reply
* [PATCH V3 6/6] bus: Add support for Tegra Generic Memory Interface
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
The Generic Memory Interface bus can be used to connect high-speed
devices such as NOR flash, FPGAs, DSPs...
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- Fixed some checkpatch errors
- Re-ordered probe to get rid of local variables
- Moved of_platform_default_populate call to the end of probe
- Use the timing and configuration properties from the child device
- Added warning if more then 1 child device exist
Changes in v3:
- added helper function to disable the controller which is used in remove and
on error.
- Added logic to parse CS# from "ranges" property with fallback to "reg"
property
drivers/bus/Kconfig | 8 ++
drivers/bus/Makefile | 1 +
drivers/bus/tegra-gmi.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 276 insertions(+)
create mode 100644 drivers/bus/tegra-gmi.c
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 4ed7d26..2e75a7f 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -141,6 +141,14 @@ config TEGRA_ACONNECT
Driver for the Tegra ACONNECT bus which is used to interface with
the devices inside the Audio Processing Engine (APE) for Tegra210.
+config TEGRA_GMI
+ tristate "Tegra Generic Memory Interface bus driver"
+ depends on ARCH_TEGRA
+ help
+ Driver for the Tegra Generic Memory Interface bus which can be used
+ to attach devices such as NOR, UART, FPGA and more.
+
+
config UNIPHIER_SYSTEM_BUS
tristate "UniPhier System Bus driver"
depends on ARCH_UNIPHIER && OF
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ac84cc4..34e2bab 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o
obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o
obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o
obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o
+obj-$(CONFIG_TEGRA_GMI) += tegra-gmi.o
obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o
obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o
diff --git a/drivers/bus/tegra-gmi.c b/drivers/bus/tegra-gmi.c
new file mode 100644
index 0000000..dd9623e
--- /dev/null
+++ b/drivers/bus/tegra-gmi.c
@@ -0,0 +1,267 @@
+/*
+ * Driver for NVIDIA Generic Memory Interface
+ *
+ * Copyright (C) 2016 Host Mobility AB. All rights reserved.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/reset.h>
+
+#define TEGRA_GMI_CONFIG 0x00
+#define TEGRA_GMI_CONFIG_GO BIT(31)
+#define TEGRA_GMI_BUS_WIDTH_32BIT BIT(30)
+#define TEGRA_GMI_MUX_MODE BIT(28)
+#define TEGRA_GMI_RDY_BEFORE_DATA BIT(24)
+#define TEGRA_GMI_RDY_ACTIVE_HIGH BIT(23)
+#define TEGRA_GMI_ADV_ACTIVE_HIGH BIT(22)
+#define TEGRA_GMI_OE_ACTIVE_HIGH BIT(21)
+#define TEGRA_GMI_CS_ACTIVE_HIGH BIT(20)
+#define TEGRA_GMI_CS_SELECT(x) ((x & 0x7) << 4)
+
+#define TEGRA_GMI_TIMING0 0x10
+#define TEGRA_GMI_MUXED_WIDTH(x) ((x & 0xf) << 12)
+#define TEGRA_GMI_HOLD_WIDTH(x) ((x & 0xf) << 8)
+#define TEGRA_GMI_ADV_WIDTH(x) ((x & 0xf) << 4)
+#define TEGRA_GMI_CE_WIDTH(x) (x & 0xf)
+
+#define TEGRA_GMI_TIMING1 0x14
+#define TEGRA_GMI_WE_WIDTH(x) ((x & 0xff) << 16)
+#define TEGRA_GMI_OE_WIDTH(x) ((x & 0xff) << 8)
+#define TEGRA_GMI_WAIT_WIDTH(x) (x & 0xff)
+
+struct tegra_gmi_priv {
+ void __iomem *base;
+ struct reset_control *rst;
+ struct clk *clk;
+
+ u32 snor_config;
+ u32 snor_timing0;
+ u32 snor_timing1;
+};
+
+static void tegra_gmi_disable(struct tegra_gmi_priv *priv)
+{
+ u32 config;
+
+ /* stop GMI operation */
+ config = readl(priv->base + TEGRA_GMI_CONFIG);
+ config &= ~TEGRA_GMI_CONFIG_GO;
+ writel(config, priv->base + TEGRA_GMI_CONFIG);
+
+ reset_control_assert(priv->rst);
+ clk_disable_unprepare(priv->clk);
+}
+
+static void tegra_gmi_init(struct device *dev, struct tegra_gmi_priv *priv)
+{
+ writel(priv->snor_timing0, priv->base + TEGRA_GMI_TIMING0);
+ writel(priv->snor_timing1, priv->base + TEGRA_GMI_TIMING1);
+
+ priv->snor_config |= TEGRA_GMI_CONFIG_GO;
+ writel(priv->snor_config, priv->base + TEGRA_GMI_CONFIG);
+}
+
+static int tegra_gmi_parse_dt(struct device *dev, struct tegra_gmi_priv *priv)
+{
+ struct device_node *child = of_get_next_available_child(dev->of_node,
+ NULL);
+ u32 property, ranges[4];
+ int ret;
+
+ if (!child) {
+ dev_warn(dev, "no child nodes found\n");
+ return 0;
+ }
+
+ /*
+ * We currently only support one child device due to lack of
+ * chip-select address decoding. Which means that we only have one
+ * chip-select line from the GMI controller.
+ */
+ if (of_get_child_count(dev->of_node) > 1)
+ dev_warn(dev, "only one child device is supported.");
+
+ if (of_property_read_bool(child, "nvidia,snor-data-width-32bit"))
+ priv->snor_config |= TEGRA_GMI_BUS_WIDTH_32BIT;
+
+ if (of_property_read_bool(child, "nvidia,snor-mux-mode"))
+ priv->snor_config |= TEGRA_GMI_MUX_MODE;
+
+ if (of_property_read_bool(child, "nvidia,snor-rdy-active-before-data"))
+ priv->snor_config |= TEGRA_GMI_RDY_BEFORE_DATA;
+
+ if (of_property_read_bool(child, "nvidia,snor-rdy-inv"))
+ priv->snor_config |= TEGRA_GMI_RDY_ACTIVE_HIGH;
+
+ if (of_property_read_bool(child, "nvidia,snor-adv-inv"))
+ priv->snor_config |= TEGRA_GMI_ADV_ACTIVE_HIGH;
+
+ if (of_property_read_bool(child, "nvidia,snor-oe-inv"))
+ priv->snor_config |= TEGRA_GMI_OE_ACTIVE_HIGH;
+
+ if (of_property_read_bool(child, "nvidia,snor-cs-inv"))
+ priv->snor_config |= TEGRA_GMI_CS_ACTIVE_HIGH;
+
+ /* Decode the CS# */
+ ret = of_property_read_u32_array(child, "ranges", ranges, 4);
+ if (ret < 0) {
+ /* Invalid binding */
+ if (ret == -EOVERFLOW) {
+ dev_err(dev, "invalid ranges length\n");
+ goto error_cs_decode;
+ }
+
+ /*
+ * If we reach here it means that the child node has an empty
+ * ranges or it does not exist@all. Attempt to decode the
+ * CS# from the reg property instead.
+ */
+ ret = of_property_read_u32(child, "reg", &property);
+ if (ret < 0) {
+ dev_err(dev, "no reg property found\n");
+ goto error_cs_decode;
+ }
+ } else {
+ property = ranges[1];
+ }
+
+ priv->snor_config |= TEGRA_GMI_CS_SELECT(property);
+
+ /* The default values that are provided below are reset values */
+ if (!of_property_read_u32(child, "nvidia,snor-muxed-width", &property))
+ priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(property);
+ else
+ priv->snor_timing0 |= TEGRA_GMI_MUXED_WIDTH(1);
+
+ if (!of_property_read_u32(child, "nvidia,snor-hold-width", &property))
+ priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(property);
+ else
+ priv->snor_timing0 |= TEGRA_GMI_HOLD_WIDTH(1);
+
+ if (!of_property_read_u32(child, "nvidia,snor-adv-width", &property))
+ priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(property);
+ else
+ priv->snor_timing0 |= TEGRA_GMI_ADV_WIDTH(1);
+
+ if (!of_property_read_u32(child, "nvidia,snor-ce-width", &property))
+ priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(property);
+ else
+ priv->snor_timing0 |= TEGRA_GMI_CE_WIDTH(4);
+
+ if (!of_property_read_u32(child, "nvidia,snor-we-width", &property))
+ priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(property);
+ else
+ priv->snor_timing1 |= TEGRA_GMI_WE_WIDTH(1);
+
+ if (!of_property_read_u32(child, "nvidia,snor-oe-width", &property))
+ priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(property);
+ else
+ priv->snor_timing1 |= TEGRA_GMI_OE_WIDTH(1);
+
+ if (!of_property_read_u32(child, "nvidia,snor-wait-width", &property))
+ priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(property);
+ else
+ priv->snor_timing1 |= TEGRA_GMI_WAIT_WIDTH(3);
+
+error_cs_decode:
+ if (ret < 0)
+ dev_err(dev, "failed to decode chip-select number\n");
+
+ of_node_put(child);
+ return ret;
+}
+
+static int tegra_gmi_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ struct device *dev = &pdev->dev;
+ struct tegra_gmi_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk = devm_clk_get(dev, "gmi");
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "can not get clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ priv->rst = devm_reset_control_get(dev, "gmi");
+ if (IS_ERR(priv->rst)) {
+ dev_err(dev, "can not get reset\n");
+ return PTR_ERR(priv->rst);
+ }
+
+ ret = tegra_gmi_parse_dt(dev, priv);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret) {
+ dev_err(dev, "fail to enable clock.\n");
+ return ret;
+ }
+
+ reset_control_assert(priv->rst);
+ udelay(2);
+ reset_control_deassert(priv->rst);
+
+ tegra_gmi_init(dev, priv);
+
+ ret = of_platform_default_populate(dev->of_node, NULL, dev);
+ if (ret < 0) {
+ dev_err(dev, "fail to create devices.\n");
+ tegra_gmi_disable(priv);
+ return ret;
+ }
+
+ dev_set_drvdata(dev, priv);
+
+ return 0;
+}
+
+static int tegra_gmi_remove(struct platform_device *pdev)
+{
+ struct tegra_gmi_priv *priv = dev_get_drvdata(&pdev->dev);
+
+ of_platform_depopulate(&pdev->dev);
+
+ tegra_gmi_disable(priv);
+
+ return 0;
+}
+
+static const struct of_device_id tegra_gmi_id_table[] = {
+ { .compatible = "nvidia,tegra20-gmi", },
+ { .compatible = "nvidia,tegra30-gmi", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, tegra_gmi_id_table);
+
+static struct platform_driver tegra_gmi_driver = {
+ .probe = tegra_gmi_probe,
+ .remove = tegra_gmi_remove,
+ .driver = {
+ .name = "tegra-gmi",
+ .of_match_table = tegra_gmi_id_table,
+ },
+};
+module_platform_driver(tegra_gmi_driver);
+
+MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com");
+MODULE_DESCRIPTION("NVIDIA Tegra GMI Bus Driver");
+MODULE_LICENSE("GPL v2");
--
2.1.4
^ permalink raw reply related
* [PATCH V3 5/6] ARM: tegra: Add Tegra20 GMI support
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
Add a device node for the GMI controller found on Tegra20.
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- added address-cells, size-cells and ranges properties
Changes in v3:
- fixed range address which is not the same as Tegra30.
arch/arm/boot/dts/tegra20.dtsi | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 2207c08..b22cddb 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -1,4 +1,4 @@
-#include <dt-bindings/clock/tegra20-car.h>
+include <dt-bindings/clock/tegra20-car.h>
#include <dt-bindings/gpio/tegra-gpio.h>
#include <dt-bindings/pinctrl/pinctrl-tegra.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -376,6 +376,20 @@
status = "disabled";
};
+
+ gmi at 70009000 {
+ compatible = "nvidia,tegra20-gmi";
+ reg = <0x70009000 0x1000>;
+ #address-cells = <2>;
+ #size-cells = <1>;
+ ranges = <0 0 0xd0000000 0xfffffff>;
+ clocks = <&tegra_car TEGRA20_CLK_NOR>;
+ clock-names = "gmi";
+ resets = < &tegra_car 42>;
+ reset-names = "gmi";
+ status = "disabled";
+ };
+
pwm: pwm at 7000a000 {
compatible = "nvidia,tegra20-pwm";
reg = <0x7000a000 0x100>;
--
2.1.4
^ permalink raw reply related
* [PATCH V3 4/6] ARM: tegra: Add Tegra30 GMI support
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
Add a device node for the GMI controller found on Tegra30.
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- added address-cells, size-cells and ranges properties
Changes in v3:
- no changes
arch/arm/boot/dts/tegra30.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 5030065..bbb1c00 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -439,6 +439,19 @@
status = "disabled";
};
+ gmi at 70009000 {
+ compatible = "nvidia,tegra30-gmi";
+ reg = <0x70009000 0x1000>;
+ #address-cells = <2>;
+ #size-cells = <1>;
+ ranges = <0 0 0x48000000 0x7ffffff>;
+ clocks = <&tegra_car TEGRA30_CLK_NOR>;
+ clock-names = "gmi";
+ resets = <&tegra_car 42>;
+ reset-names = "gmi";
+ status = "disabled";
+ };
+
pwm: pwm at 7000a000 {
compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm";
reg = <0x7000a000 0x100>;
--
2.1.4
^ permalink raw reply related
* [PATCH V3 3/6] dt/bindings: Add bindings for Tegra GMI controller
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
Document the devicetree bindings for the Generic Memory Interface (GMI)
bus driver found on Tegra SOCs.
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- Updated examples and some information based on comments from Jon Hunter.
Changes in v3:
- Updates ranges description based on comments from Rob Herring
.../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 132 +++++++++++++++++++++
1 file changed, 132 insertions(+)
create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
diff --git a/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
new file mode 100644
index 0000000..49bda2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
@@ -0,0 +1,132 @@
+Device tree bindings for NVIDIA Tegra Generic Memory Interface bus
+
+The Generic Memory Interface bus enables memory transfers between internal and
+external memory. Can be used to attach various high speed devices such as
+synchronous/asynchronous NOR, FPGA, UARTS and more.
+
+The actual devices are instantiated from the child nodes of a GMI node.
+
+Required properties:
+ - compatible : Should contain one of the following:
+ For Tegra20 must contain "nvidia,tegra20-gmi".
+ For Tegra30 must contain "nvidia,tegra30-gmi".
+ - reg: Should contain GMI controller registers location and length.
+ - clocks: Must contain an entry for each entry in clock-names.
+ - clock-names: Must include the following entries: "gmi"
+ - resets : Must contain an entry for each entry in reset-names.
+ - reset-names : Must include the following entries: "gmi"
+ - #address-cells: The number of cells used to represent physical base
+ addresses in the GMI address space. Should be 2.
+ - #size-cells: The number of cells used to represent the size of an address
+ range in the GMI address space. Should be 1.
+ - ranges: Must be set up to reflect the memory layout with three integer values
+ for each chip-select line in use (only one entry is supported, see below
+ comments):
+ <cs-number> <offset> <physical address of mapping> <size>
+
+Note that the GMI controller does not have any internal chip-select address
+decoding, because of that chip-selects either need to be managed via software
+or by employing external chip-select decoding logic.
+
+If external chip-select logic is used to support multiple devices it is assumed
+that the devices use the same timing and so are probably the same type. It also
+assumes that they can fit in the 256MB address range. In this case only one
+child device is supported which represents the active chip-select line, see
+examples for more insight.
+
+The chip-select number is decoded from the child nodes second address cell of
+'ranges' property, if 'ranges' property is not present or empty chip-select will
+then be decoded from the first cell of the 'reg' property.
+
+Optional child cs node properties:
+
+ - nvidia,snor-data-width-32bit: Use 32bit data-bus, default is 16bit.
+ - nvidia,snor-mux-mode: Enable address/data MUX mode.
+ - nvidia,snor-rdy-active-before-data: Assert RDY signal one cycle before data.
+ If omitted it will be asserted with data.
+ - nvidia,snor-rdy-inv: RDY signal is active high
+ - nvidia,snor-adv-inv: ADV signal is active high
+ - nvidia,snor-oe-inv: WE/OE signal is active high
+ - nvidia,snor-cs-inv: CS signal is active high
+
+ Note that there is some special handling for the timing values.
+ From Tegra TRM:
+ Programming 0 means 1 clock cycle: actual cycle = programmed cycle + 1
+
+ - nvidia,snor-muxed-width: Number of cycles MUX address/data asserted on the
+ bus. Valid values are 0-15, default is 1
+ - nvidia,snor-hold-width: Number of cycles CE stays asserted after the
+ de-assertion of WR_N (in case of SLAVE/MASTER Request) or OE_N
+ (in case of MASTER Request). Valid values are 0-15, default is 1
+ - nvidia,snor-adv-width: Number of cycles during which ADV stays asserted.
+ Valid values are 0-15, default is 1.
+ - nvidia,snor-ce-width: Number of cycles before CE is asserted.
+ Valid values are 0-15, default is 4
+ - nvidia,snor-we-width: Number of cycles during which WE stays asserted.
+ Valid values are 0-15, default is 1
+ - nvidia,snor-oe-width: Number of cycles during which OE stays asserted.
+ Valid values are 0-255, default is 1
+ - nvidia,snor-wait-width: Number of cycles before READY is asserted.
+ Valid values are 0-255, default is 3
+
+Example with two SJA1000 CAN controllers connected to the GMI bus. We wrap the
+controllers with a simple-bus node since they are all connected to the same
+chip-select (CS4), in this example external address decoding is provided:
+
+gmi at 70090000 {
+ compatible = "nvidia,tegra20-gmi";
+ reg = <0x70009000 0x1000>;
+ #address-cells = <2>;
+ #size-cells = <1>;
+ clocks = <&tegra_car TEGRA20_CLK_NOR>;
+ clock-names = "gmi";
+ resets = <&tegra_car 42>;
+ reset-names = "gmi";
+ ranges = <4 0 0xd0000000 0xfffffff>;
+
+ status = "okay";
+
+ bus at 4,0 {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 4 0 0x40100>;
+
+ nvidia,snor-mux-mode;
+ nvidia,snor-adv-inv;
+
+ can at 0 {
+ reg = <0 0x100>;
+ ...
+ };
+
+ can at 40000 {
+ reg = <0x40000 0x100>;
+ ...
+ };
+ };
+};
+
+Example with one SJA1000 CAN controller connected to the GMI bus
+on CS4:
+
+gmi at 70090000 {
+ compatible = "nvidia,tegra20-gmi";
+ reg = <0x70009000 0x1000>;
+ #address-cells = <2>;
+ #size-cells = <1>;
+ clocks = <&tegra_car TEGRA20_CLK_NOR>;
+ clock-names = "gmi";
+ resets = <&tegra_car 42>;
+ reset-names = "gmi";
+ ranges = <4 0 0xd0000000 0xfffffff>;
+
+ status = "okay";
+
+ can at 4,0 {
+ reg = <4 0 0x100>;
+ nvidia,snor-mux-mode;
+ nvidia,snor-adv-inv;
+ ...
+ };
+};
--
2.1.4
^ permalink raw reply related
* [PATCH V3 2/6] clk: tegra: add TEGRA30_CLK_NOR to init table
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
Add TEGRA30_CLK_NOR to init table and set default rate to 127 MHz which
is max rate.
The maximum rate value of 127 MHz is pulled from the downstream L4T
kernel.
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- no changes
Changes in v3:
- Added comment in commit message where I got the maximum rates from.
drivers/clk/tegra/clk-tegra30.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 8e2db5e..67f1677 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1252,6 +1252,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
{ TEGRA30_CLK_SDMMC1, TEGRA30_CLK_PLL_P, 48000000, 0 },
{ TEGRA30_CLK_SDMMC2, TEGRA30_CLK_PLL_P, 48000000, 0 },
{ TEGRA30_CLK_SDMMC3, TEGRA30_CLK_PLL_P, 48000000, 0 },
+ { TEGRA30_CLK_NOR, TEGRA30_CLK_PLL_P, 127000000, 0 },
{ TEGRA30_CLK_PLL_M, TEGRA30_CLK_CLK_MAX, 0, 1 },
{ TEGRA30_CLK_PCLK, TEGRA30_CLK_CLK_MAX, 0, 1 },
{ TEGRA30_CLK_CSITE, TEGRA30_CLK_CLK_MAX, 0, 1 },
--
2.1.4
^ permalink raw reply related
* [PATCH V3 1/6] clk: tegra: add TEGRA20_CLK_NOR to init table
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477576872-2665-1-git-send-email-mirza.krak@gmail.com>
From: Mirza Krak <mirza.krak@gmail.com>
Add TEGRA20_CLK_NOR to init table and set default rate to 92 MHz which
is max rate.
The maximum rate value of 92 MHz is pulled from the downstream L4T
kernel.
Signed-off-by: Mirza Krak <mirza.krak@gmail.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Tested-on: Colibri T20/T30 on EvalBoard V3.x and GMI-Memory Board
---
Changes in v2:
- no changes
Changes in v3:
- Added comment in commit message where I got the maximum rates from.
drivers/clk/tegra/clk-tegra20.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 837e5cb..13d3b5a 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1047,6 +1047,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
{ TEGRA20_CLK_SDMMC3, TEGRA20_CLK_PLL_P, 48000000, 0 },
{ TEGRA20_CLK_SDMMC4, TEGRA20_CLK_PLL_P, 48000000, 0 },
{ TEGRA20_CLK_SPI, TEGRA20_CLK_PLL_P, 20000000, 0 },
+ { TEGRA20_CLK_NOR, TEGRA20_CLK_PLL_P, 92000000, 0 },
{ TEGRA20_CLK_SBC1, TEGRA20_CLK_PLL_P, 100000000, 0 },
{ TEGRA20_CLK_SBC2, TEGRA20_CLK_PLL_P, 100000000, 0 },
{ TEGRA20_CLK_SBC3, TEGRA20_CLK_PLL_P, 100000000, 0 },
--
2.1.4
^ permalink raw reply related
* [PATCH V3 0/6] Add support for Tegra GMI bus controller
From: Mirza Krak @ 2016-10-27 14:01 UTC (permalink / raw)
To: linux-arm-kernel
From: Mirza Krak <mirza.krak@gmail.com.com>
Hi.
This patch series adds support for the Tegra GMI bus controller.
I have tested this series on a Tegra30 using a Colibri T30 SOM on a custom
carrier board which has multiple CAN controllers (SJA1000) connected to the
GMI bus.
I have re-based on top of latest tegra/for-next in V3. Also see individual
patches for changes in V3.
I have picked up all the comments and suggestions from V2, but I still do not
have an ACK from Rob on the bindings and discussions have halted for some time
now and I hope that this re-send could be the basis for new discussions.
See below links for previous discussions.
Comments on RFC:
https://marc.info/?l=linux-clk&m=146893557629903&w=2
https://marc.info/?l=linux-tegra&m=146893541829801&w=2
https://marc.info/?l=linux-tegra&m=146893542429814&w=2
Comments on V1:
https://marc.info/?l=linux-arm-kernel&m=147051551821122&w=2
https://marc.info/?l=linux-arm-kernel&m=147051553121150&w=2
https://marc.info/?l=linux-arm-kernel&m=147194856600627&w=2
https://marc.info/?l=linux-arm-kernel&m=147072742432211&w=2
Comments on V2:
https://marc.info/?l=devicetree&m=147522253920226&w=2
https://marc.info/?l=linux-tegra&m=147204588027687&w=2
https://marc.info/?l=linux-tegra&m=147204588027687&w=2
https://marc.info/?l=devicetree&m=147256931318922&w=2
Mirza Krak (6):
clk: tegra: add TEGRA20_CLK_NOR to init table
clk: tegra: add TEGRA30_CLK_NOR to init table
dt/bindings: Add bindings for Tegra GMI controller
ARM: tegra: Add Tegra30 GMI support
ARM: tegra: Add Tegra20 GMI support
bus: Add support for Tegra Generic Memory Interface
.../devicetree/bindings/bus/nvidia,tegra20-gmi.txt | 132 ++++++++++
arch/arm/boot/dts/tegra20.dtsi | 16 +-
arch/arm/boot/dts/tegra30.dtsi | 13 +
drivers/bus/Kconfig | 8 +
drivers/bus/Makefile | 1 +
drivers/bus/tegra-gmi.c | 267 +++++++++++++++++++++
drivers/clk/tegra/clk-tegra20.c | 1 +
drivers/clk/tegra/clk-tegra30.c | 1 +
8 files changed, 438 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/bus/nvidia,tegra20-gmi.txt
create mode 100644 drivers/bus/tegra-gmi.c
--
2.1.4
^ permalink raw reply
* [PATCH v2 2/2] arm64: dts: hi6220: add resets property into dwmmc nodes
From: Vincent Guittot @ 2016-10-27 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1472094041-5357-2-git-send-email-guodong.xu@linaro.org>
Hi,
My hikey board failed to detect and mount sdcard with v4.9-rc1 and i
have bisected the issue to this patch. Once reverted, the sdcard is
detected again.
Regards,
Vincent
On 25 August 2016 at 05:00, Guodong Xu <guodong.xu@linaro.org> wrote:
> Add resets property into dwmmc_0, dwmmc_1 and dwmmc_2 for hi6220
>
> Code and documentation to this property were confirmed by maintainers.
> See:
> [1] https://patchwork.kernel.org/patch/9276607/
> [2] https://patchwork.kernel.org/patch/8487151/
> [3] https://lkml.org/lkml/2016/8/12/91
>
> cc: Jaehoon Chung <jh80.chung@samsung.com>
> cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Guodong Xu <guodong.xu@linaro.org>
> ---
> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index d0b887a..63608e9 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -771,6 +771,7 @@
> interrupts = <0x0 0x48 0x4>;
> clocks = <&sys_ctrl 2>, <&sys_ctrl 1>;
> clock-names = "ciu", "biu";
> + resets = <&sys_ctrl PERIPH_RSTDIS0_MMC0>;
> bus-width = <0x8>;
> vmmc-supply = <&ldo19>;
> pinctrl-names = "default";
> @@ -790,6 +791,7 @@
> #size-cells = <0x0>;
> clocks = <&sys_ctrl 4>, <&sys_ctrl 3>;
> clock-names = "ciu", "biu";
> + resets = <&sys_ctrl PERIPH_RSTDIS0_MMC1>;
> vqmmc-supply = <&ldo7>;
> vmmc-supply = <&ldo10>;
> bus-width = <0x4>;
> @@ -807,6 +809,7 @@
> interrupts = <0x0 0x4a 0x4>;
> clocks = <&sys_ctrl HI6220_MMC2_CIUCLK>, <&sys_ctrl HI6220_MMC2_CLK>;
> clock-names = "ciu", "biu";
> + resets = <&sys_ctrl PERIPH_RSTDIS0_MMC2>;
> bus-width = <0x4>;
> broken-cd;
> pinctrl-names = "default", "idle";
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1
From: Antoine Tenart @ 2016-10-27 13:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e839e92f-127b-efc3-b10f-37477d1c5d9a@suse.com>
Hello Matthias,
On Thu, Oct 27, 2016 at 11:19:14AM +0200, Matthias Brugger wrote:
> On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
>
> Please provide a commit message.
Sure. There are other modifications I'd like to do in the series if it
happens to be an use case for people. This patch is given as an example
of how we could implement this.
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/109caa98/attachment-0001.sig>
^ permalink raw reply
* [RFC PATCH 0/5] Add an overlay manager to handle board capes
From: Rob Herring @ 2016-10-27 13:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161026145756.21689-1-antoine.tenart@free-electrons.com>
Please Cc the maintainers of drivers/of/.
+ Frank R, Hans, Dmitry S
On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hi all,
>
> Many boards now come with dips and compatible capes; among others the
> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an
> out-of-tree "cape manager" which is used to detected capes, retrieve
> their description and apply a corresponding overlay. This series is an
> attempt to start a discussion, with an implementation of such a manager
> which is somehow generic (i.e. formats or cape detectors can be added).
> Other use cases could make use of this manager to dynamically load dt
> overlays based on some input / hw presence.
I'd like to see an input source be the kernel command line and/or a DT
chosen property. Another overlay manager was proposed not to long
ago[1] as well. There's also the Allwinner tablet use case from Hans
where i2c devices are probed and detected. That's not using overlays
currently, but maybe could.
Another thing to consider is different sources of overlays. Besides in
the filesystem, overlays could be built into the kernel (already
supported), embedded in the dtb (as the other overlay mgr did) or we
could extend FDT format to append them.
> The proposed design is a library which can be used by detector drivers
> to parse headers and load the corresponding overlay. Helpers are
> provided for this purpose. The whole thing is divided into 3 entities:
>
> - The parser which is project-specific (to allow supporting headers
> already into the wild). It registers a function parsing an header's
> data and filling one or more strings which will be used to find
> matching dtbo on the fs.
>
> - The overlay manager helpers allowing to parse a header to retrieve
> the previously mentioned strings and to load a compatible overlay.
>
> - The detectors which are used to detect capes and get their description
> (to be parsed).
What about things like power has to be turned on first to detect
boards and read their ID? I think this needs to be tied into the
driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
a driver gets bound to a connector node, but we've got to sort out
connector bindings first.
> An example of parser and detector is given, compatible with what's done
> for the C.H.I.P. As the w1 framework is really bad (and we should
> probably do something about that) the detector code is far from being
> perfect; but that's not related to what we try to achieve here.
>
> The actual implementation has a limitation: the detectors cannot be
> built-in the kernel image as they would likely detect capes at boot time
> but will fail to get their corresponding dt overlays as the fs isn't
> mounted yet. The only case this can work is when dt overlays are
> built-in firmwares. This isn't an issue for the C.H.I.P. use case right
> now. There was a discussion about making an helper to wait for the
> rootfs to be mount but the answer was "this is the driver's problem".
I thought there are firmware loading calls that will wait. I think
this all needs to work asynchronously both for firmware loading and
because w1 is really slow.
> I'd like to get comments, specifically from people using custom cape
> managers, to see if this could fill their needs (with I guess some
> modifications).
Having 2 would certainly give a better sense this is generic.
Rob
[1] https://patchwork.ozlabs.org/patch/667805/
^ permalink raw reply
* [PATCH v2 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2016-10-27 13:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL_JsqJvm8+L0-pFQRQGYhvSzvqubWsBp8Q5kU-BPSiDmMau0A@mail.gmail.com>
+Jon
On 10/26/2016 04:59 PM, Rob Herring wrote:
> On Mon, Oct 24, 2016 at 12:00 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> Dave Gerlach <d-gerlach@ti.com> writes:
>>
>>> Hi,
>>> On 10/21/2016 01:48 PM, Kevin Hilman wrote:
>>>> Dave Gerlach <d-gerlach@ti.com> writes:
>>>>
>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>>> control device power states.
>>>>>
>>>>> Also, provide macros representing each device index as understood
>>>>> by TI SCI to be used in the device node power-domain references.
>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>
>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>>>> ---
>>>>> .../devicetree/bindings/soc/ti/sci-pm-domain.txt | 54 +++++++++++++
>>>>> MAINTAINERS | 2 +
>>>>> include/dt-bindings/genpd/k2g.h | 90 ++++++++++++++++++++++
>>>>> 3 files changed, 146 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>> create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>> new file mode 100644
>>>>> index 000000000000..32f38a349656
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>> @@ -0,0 +1,54 @@
>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>> +---------------------------------------------
>>>>> +
>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...) that is
>>>>> +responsible for controlling the state of the IPs that are present.
>>>>> +Communication between the host processor running an OS and the system
>>>>> +controller happens through a protocol known as TI-SCI [1]. This pm domain
>>>>> +implementation plugs into the generic pm domain framework and makes use of
>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>> +
>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>> +
>>>>> +PM Domain Node
>>>>> +==============
>>>>> +The PM domain node represents the global PM domain managed by the PMMC,
>>>>> +which in this case is the single implementation as documented by the generic
>>>>> +PM domain bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>>>>> +
>>>>> +Required Properties:
>>>>> +--------------------
>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>> +- #power-domain-cells: Must be 0.
>>>>> +- ti,sci: Phandle to the TI SCI device to use for managing the devices.
>>>>>
>>>>> +Example:
>>>>> +--------------------
>>>>> +k2g_pds: k2g_pds {
>>>>
>>>> should use generic name like "power-contoller", e.g. k2g_pds: power-controller
>>>
>>> Ok, that makes more sense.
>>>
>>>>
>>>>> + compatible = "ti,sci-pm-domain";
>>>>> + #power-domain-cells = <0>;
>>>>> + ti,sci = <&pmmc>;
>>>>> +};
>>>>> +
>>>>> +PM Domain Consumers
>>>>> +===================
>>>>> +Hardware blocks that require SCI control over their state must provide
>>>>> +a reference to the sci-pm-domain they are part of and a unique device
>>>>> +specific ID that identifies the device.
>>>>> +
>>>>> +Required Properties:
>>>>> +--------------------
>>>>> +- power-domains: phandle pointing to the corresponding PM domain node.
>>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI to
>>>>> + be used for device control.
>>>>
>>>> This ID doesn't look right.
>>>>
>>>> Why not use #power-domain-cells = <1> and pass the index in the DT? ...
>
> Exactly. ti,sci-id is a NAK for me.
I was told not to use the onecell during v1 discussion. I agree this would be
ideal but I cannot due to what the bindings represent, the phandle parameter is
an index into a list of genpds, whereas we need an actual ID number we can use
and I do not have the ability to get that from the phandle.
@Ulf/Jon, is there any hope of bringing back custom xlate functions for genpd
providers? I don't have a good background on why it was even removed. I can
maintain a single genpd for all devices but I need a way to parse this ID,
whether it's from a separate property or a phandle. It is locked now to indexing
into a list of genpds but I need additional per device information for devices
bound to a genpd and I need either a custom parameter or the ability to parse
the phandle myself.
>
>>>>
>>>>> +See dt-bindings/genpd/k2g.h for the list of valid identifiers for k2g.
>>>>> +
>>>>> +Example:
>>>>> +--------------------
>>>>> +uart0: serial at 02530c00 {
>>>>> + compatible = "ns16550a";
>>>>> + ...
>>>>> + power-domains = <&k2g_pds>;
>>>>> + ti,sci-id = <K2G_DEV_UART0>;
>>>>
>>>> ... like this:
>>>>
>>>> power-domains = <&k2g_pds K2G_DEV_UART0>;
>>>
>>> That's how I did it in version one actually. I was able to define my
>>> own xlate function to parse the phandle and get that index, but Ulf
>>> pointed me to this series by Jon Hunter [1] that simplified genpd
>>> providers and dropped the concept of adding your own xlate. This locks
>>> the onecell approach to using a fixed static array of genpds that get
>>> indexed into (without passing the index to the provider, just the
>>> genpd that's looked up), which doesn't fit our usecase, as we don't
>>> want a 1 to 1 genpd to device mapping based on the comments provided
>>> in v1. Now we just use the genpd device attach/detach hooks to parse
>>> the sci-id and then use it in the genpd device start/stop hooks.
>
> I have no idea what any of this means. All sounds like driver
> architecture, not anything to do with bindings.
This was a response to Kevin, not part of binding description.
>
>>
>> Ah, right. I remember now. This approach allows you to use a single
>> genpd as discussed earlier.
>>
>> Makes sense now, suggestion retracted.
>
> IIRC, the bindings in Jon's case had a node for each domain and didn't
> need any additional property.
Yes but we only have one domain and index into it, not into a list of domains,
so the additional property is solving a different problem.
Regards,
Dave
>
> Rob
>
^ permalink raw reply
* [PATCH] i2c: rk3x: Give the tuning value 0 during rk3x_i2c_v0_calc_timings
From: Wolfram Sang @ 2016-10-27 13:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1477125822-30644-1-git-send-email-david.wu@rock-chips.com>
On Sat, Oct 22, 2016 at 04:43:42PM +0800, David Wu wrote:
> We found a bug that i2c transfer sometimes failed on 3066a board with
> stabel-4.8, the con register would be updated by uninitialized tuning
> value, it made the i2c transfer failed.
>
> So give the tuning value to be zero during rk3x_i2c_v0_calc_timings.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
What I missed to say in my review:
Please use a subject line that describes WHY the change is needed not so
much WHAT is done. Like: "fix missing initialization causing boot
problems"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161027/c2981e1f/attachment.sig>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox